Skip to content

Port to OMP#22

Open
defstat wants to merge 1 commit into
pkp:mainfrom
defstat:omp-port
Open

Port to OMP#22
defstat wants to merge 1 commit into
pkp:mainfrom
defstat:omp-port

Conversation

@defstat

@defstat defstat commented Aug 24, 2017

Copy link
Copy Markdown

No description provided.

* @return $string
*/
function addCheckOrcidButton($output, &$templateMgr) {
$sessionManager = SessionManager::getManager();

Copy link
Copy Markdown
Member

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) {

Copy link
Copy Markdown
Member

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.

}

/**
* Output filter adds ORCiD interaction to OJS login form.

Copy link
Copy Markdown
Member

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();

Copy link
Copy Markdown
Member

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.

// $params['path'] = array_merge($path, $params['path']);
// } elseif (!empty($params['path'])) {
// $params['path'] = array_merge($path, array($params['path']));
// } else {

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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() {

Copy link
Copy Markdown
Member

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.

Comment thread locale/en_US/locale.xml
<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>

Copy link
Copy Markdown
Member

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;

Copy link
Copy Markdown
Member

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.

Comment thread templates/orcidLogin.tpl
</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}">

Copy link
Copy Markdown
Member

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 &amp;.

{**
* plugins/generic/orcidProfile/orcidProfile.tpl
*
* Copyright (c) 2015-2016 University of Pittsburgh

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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"

@asmecher

Copy link
Copy Markdown
Member

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.

@nils-stefan-weiher

Copy link
Copy Markdown

@asmecher @defstat Isn't the plugin already working with OMP 3.1.1?
Can this be closed?

@asmecher

asmecher commented Jul 4, 2019

Copy link
Copy Markdown
Member

@defstat, I don't believe it's released for OMP yet; is there a compatible version that you're aware of?

@defstat

defstat commented Jul 8, 2019

Copy link
Copy Markdown
Author

@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.
I am not sure about the current stage of all of these and whether they can still be used on current version of OMP (I am pretty sure that they need some work to be able to work on the current release of OMP)

@asmecher

asmecher commented Jul 8, 2019

Copy link
Copy Markdown
Member

@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.

Base automatically changed from master to main February 18, 2021 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants