-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port to OMP #22
base: main
Are you sure you want to change the base?
Port to OMP #22
Conversation
* @return $string | ||
*/ | ||
function addCheckOrcidButton($output, &$templateMgr) { | ||
$sessionManager = SessionManager::getManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space-based indents.
* @param $templateMgr TemplateManager | ||
* @return $string | ||
*/ | ||
function addCheckOrcidButton($output, &$templateMgr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, references on objects aren't needed any more; I'd suggest removing throughout unless they're needed.
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Output filter adds ORCiD interaction to OJS login form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this refer to OJS specifically?
if (preg_match('/<form.*id="editAuthor"[^>]+>/', $output, $matches, PREG_OFFSET_CAPTURE)) { | ||
$match = $matches[0][0]; | ||
$offset = $matches[0][1]; | ||
$context = Request::getContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's possible, I'd suggest exposing the $request
object to this function rather than calling Request::(whatever)
statically. Static calls to request functions are deprecated.
// if (is_array($params['path'])) { | ||
// $params['path'] = array_merge($path, $params['path']); | ||
// } elseif (!empty($params['path'])) { | ||
// $params['path'] = array_merge($path, array($params['path'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code left behind?
<?php | ||
|
||
/** | ||
* @file classes/user/UserSettingsDAO.inc.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class name/path incorrect
/** | ||
* Constructor | ||
*/ | ||
function __construct() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wherever possible, remove unneeded (empty) constructors.
@@ -33,4 +32,8 @@ | |||
<message key="plugins.generic.orcidProfile.author.submission.success">Your submission has successfully been associated with your ORCID iD.</message> | |||
<message key="plugins.generic.orcidProfile.author.submission.failure">Your submission could not be successfully associated with your ORCID iD. Please contact the journal manager with your name, ORCID, and details of your submission.</message> | |||
<message key="plugins.generic.orcidProfile.authFailure">OJS was not able to communicate with the ORCID service. Please contact the journal manager with your name, ORCID iD, and details of your submission.</message> | |||
<message key="plugins.generic.orcidProfile.linkMessage">Connect with ORCID</message> | |||
<message key="plugins.generic.orcidProfile.oauthLoginError">OJS was not able to communicate with the ORCID service. Please contact the journal manager with your name, ORCID, and details of your submission.</message> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refers to OJS
$givenName = $profile['given-names']['value']; | ||
$familyName = $profile['family-name']['value']; | ||
|
||
$response = "The ORCID " . $orcid . " correspond to a registered researcher - Name: " . $givenName . " - Family Name: " . $familyName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English-language text should be localized.
</style> | ||
{/literal} | ||
<div id='orcidLoginLink'> | ||
<a href="{$orcidProfileOauthPath|escape}authorize?client_id={$orcidClientId|urlencode}&response_type=code&scope=/authenticate&redirect_uri={url|urlencode page="orcidapi" op="orcidAuthorize" targetOp=$targetOp params=$params escape=false}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the &
s need to be escaped as &
.
{** | ||
* plugins/generic/orcidProfile/orcidProfile.tpl | ||
* | ||
* Copyright (c) 2015-2016 University of Pittsburgh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dates for new files should all be -2017
* plugins/generic/orcidProfile/orcidProfile.tpl | ||
* | ||
* Copyright (c) 2015-2016 University of Pittsburgh | ||
* Copyright (c) 2014-2016 Simon Fraser University Library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All "Simon Fraser University Library" copyrights should be changed to "Simon Fraser University"
Thanks, @defstat -- let me know when you've had a chance to look through my comments (and perform a rebase -- currently this won't merge) and I'll take a second look. |
@defstat, I don't believe it's released for OMP yet; is there a compatible version that you're aware of? |
@asmecher during Hirmeos project there where changes on the orcidProfile plugin regarding porting in OMP (for OMP 1.2 back then). EKT's OMP platform (http://ebooks.epublishing.ekt.gr) had incorporated the changes. That plugin (https://github.com/defstat/orcidProfileLink) had also been developed in order for the orcid of the monographs author to be displayed on the monographs landing page. |
@defstat, hmm, it probably needs some review and testing. The orcidProfile plugin has never been included in the OMP codebase or flagged in the Plugin Gallery for compatibility with OMP, and those are the two places I'd expect to see it if it were released for OMP. |
No description provided.