Skip to content

ORCID plugin 2.4.8#14

Open
atarix83 wants to merge 33 commits into
pkp:ojs-dev-2_4from
4Science:ojs-dev-2_4
Open

ORCID plugin 2.4.8#14
atarix83 wants to merge 33 commits into
pkp:ojs-dev-2_4from
4Science:ojs-dev-2_4

Conversation

@atarix83

@atarix83 atarix83 commented Apr 7, 2017

Copy link
Copy Markdown

Hi,

I proceeded to add some features to orcidProfile plugin for OJS 2.4.x and I'd like to commit to OJS codebase.

What we have changes is :

  • added search functionality for additional authors;
  • added translation file for Italian language;
  • some improvements in the graphic;
  • bug fixes in the request email.

Thanks.
Giuseppe
4Science

@asmecher

asmecher commented Apr 7, 2017

Copy link
Copy Markdown
Member

@atarix83, before I start on a code review, would you mind converting indentation to tabs instead of spaces? Thanks!

@atarix83

Copy link
Copy Markdown
Author

@asmecher
Done.

Comment thread locale/en_US/locale.xml Outdated
<message key="plugins.generic.orcidProfile.noData">Couldn't find any data from ORCID.</message>
<message key="plugins.generic.orcidProfile.emailOrOrcid">Email address or ORCID iD:</message>
<message key="plugins.generic.orcidProfile.submitAction">Submit</message>
<message key="plugins.generic.orcidProfile.submitAction">Submit</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.

There are still some space-based indents here.

Comment thread locale/it_IT/locale.xml Outdated
<message key="plugins.generic.orcidProfile.instructions"><![CDATA[È possibile precompilare il modulo con le informazioni da un profilo orcid. Inserire l'indirizzo email o ORCID iD associati al profilo ORCID, quindi fare clic su "Conferma".]]></message>
<message key="plugins.generic.orcidProfile.noData">Nessuna informazione trovata dal registro di ORCID.</message>
<message key="plugins.generic.orcidProfile.emailOrOrcid">Indirizzo Email o ORCID iD:</message>
<message key="plugins.generic.orcidProfile.submitAction">Conferma</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.

Some space-based indents

Comment thread pages/OrcidHandler.inc.php Outdated
echo '<html><body><script type="text/javascript">
opener.document.getElementById("authors-0-orcid").value = ' . json_encode('http://orcid.org/' . $response['orcid']). ';
opener.document.getElementById("connect-orcid-button").style.display = "none";
opener.document.getElementById("connect-orcid-button").style.display = "none";

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

Comment thread templates/orcidProfile.tpl Outdated

function openORCID() {ldelim}
var oauthWindow = window.open("{$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}", "_blank", "toolbar=no, scrollbars=yes, width=500, height=600, top=500, left=500");
console.log(self);

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 this is a debugging leftover.

@asmecher

Copy link
Copy Markdown
Member

Sorry to be so picky about indentation -- it's my obsession :)

@atarix83

Copy link
Copy Markdown
Author

Sorry for not having carefully checked, now It'd be ok

Comment thread css/orcidProfile.css Outdated
font-weight: bold;
font-size: .8em;
line-height: 24px;
margin: .3em;

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.

Sorry, one more here :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted

Comment thread css/orcidProfile.css Outdated
}

.search-content {
padding: 10px !important;

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.

And here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted

@asmecher

Copy link
Copy Markdown
Member

@ctgraham, have you tinkered with any of this recently?

@ctgraham

Copy link
Copy Markdown

@asmecher, I have not done any recent work on the orcidProfile plugin.

@atarix83 atarix83 changed the title Ojs dev 2 4 ORCID plugin 2.4.8 May 15, 2017
@atarix83

Copy link
Copy Markdown
Author

Hi,
any news about this PR?

Thanks.
Giuseppe
4Science

@asmecher

Copy link
Copy Markdown
Member

Sorry, @atarix83, it's been on my list to look at but I've been having trouble keeping up with other priorities. @defstat, would you be able to look at it?

@defstat defstat left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atarix83 A first review round with few suggestions.

Comment thread OrcidProfilePlugin.inc.php Outdated
$templateMgr =& $args[0];
$template =& $args[1];
$request =& PKPApplication::getRequest();
$request =& PKPApplication::getRequest();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread OrcidProfilePlugin.inc.php Outdated
// Assign our private stylesheet.
$templateMgr->addStylesheet($request->getBaseUrl() . '/' . $this->getStyleSheet());
// Assign our private stylesheet.
$templateMgr->addStylesheet($request->getBaseUrl() . '/' . $this->getStyleSheet());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread OrcidProfilePlugin.inc.php Outdated
$templateMgr->register_outputfilter(array(&$this, 'profileFilter'));
break;
case 'author/submit/step3.tpl':
case 'submission/metadata/metadataEdit.tpl':

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread OrcidProfilePlugin.inc.php Outdated
});</script>';
$newOutput .= substr($output, $offset + strlen($match));
$output = $newOutput;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several indentation issues at that function.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to what you are referring, maybe line 231?

Comment thread locale/en_US/locale.xml Outdated
<message key="plugins.generic.orcidProfile.searchResultsList">Choose and select the exact match from the list below:</message>
<message key="plugins.generic.orcidProfile.searchResultsList">Choose and select the exact match from the list below:</message>
<message key="plugins.generic.orcidProfile.privateEmail">Not viewable</message>
<message key="plugins.generic.orcidProfile.noResults">No search results</message>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some indentation issues here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok in my branch



</body>
<description>This email template is used to collect the ORCID id's from co-authors.</description>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only for 'co-authors'?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Comment thread locale/it_IT/locale.xml Outdated
<!DOCTYPE locale SYSTEM "../../../../../lib/pkp/dtd/locale.dtd">

<!--
* plugins/generic/orcidPlugin/locale/en_US/locale.xml

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en_US -> it_IT

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orcidPlugin -> orcidProfile?

Comment thread templates/orcidProfile.tpl Outdated
@@ -1,11 +1,13 @@
{**
* plugins/generic/orcidProfile/orcidProfile.tpl

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix path

Comment thread templates/orcidProfileSearch.tpl Outdated
@@ -0,0 +1,23 @@
{**
* plugins/generic/orcidProfile/orcidProfileSearch.tpl

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix path

Comment thread templates/orcidProfileSearchResults.tpl Outdated
@@ -0,0 +1,162 @@
{**
* plugins/generic/orcidProfile/orcidProfileSearchResults.tpl

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix path

Comment thread templates/orcidProfileSearchResults.tpl Outdated
</tr>
<tr class="heading" valign="top">
<td width="5%">&nbsp;</td>
<td width="25%">{translate key="plugins.generic.openAIRE.projectID"}</td>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix translation key

Comment thread locale/it_IT/emailTemplates.xml Outdated
* Localized email templates XML file.
-->

<email_texts locale="en_US">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change locale

Comment thread templates/orcidProfileSearchResults.tpl Outdated
<h3>{translate key='plugins.generic.orcidProfile.searchPageTitle'}</h3>
<div id="content" class="search-content">
<p>{translate key='plugins.generic.orcidProfile.searchResultsList'}</p>
<form action="{plugin_url path="process"}" method="post" id="issuesForm">

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest change the name of the form [id="issuesForm"]

Comment thread templates/orcidProfileSearch.tpl Outdated
{rdelim}
</script>

<button id="{$params.orcidButtonId}" onclick="return openSearchORCID{$params.authorIndex}();" {if !$params.orcidButtonVisible}style="display:none;" {/if}><img class="orcid-id-logo" src="http://orcid.org/sites/default/files/images/orcid_24x24.png" width="24" height="24" alt="{translate key='plugins.generic.orcidProfile.submitAction'}"/>Search the ORCID iD</button>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest change "Search the ORCID iD" to {translate key='...'}

<link rel="stylesheet" href="{$baseUrl}/styles/comments.css" type="text/css" />
<link rel="stylesheet" href="{$baseUrl}/plugins/generic/orcidProfile/css/orcidProfile.css" type="text/css" />

{foreach from=$stylesheets item=cssUrl}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is orcidProfile.css declared twice? (also elsewhere)

$("#orcidSearchResultsSubmit").click(function(event) {
event.preventDefault();
var profiles = {/literal}{$orcidSearchResults|@json_encode}{literal};
var authorIndex = {/literal}{$authorIndex}{literal};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $authorIndex seems not to be filled after the user uses the searchResults pager [go from page1 to page2 for example]. That seems to result to a javascript error which "breaks" the javascript flow.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

Comment thread templates/orcidProfileSearchResults.tpl Outdated
var profileSelectedIndex = $("input[name=orcidProfile]:checked").val();
var profileSelected = profiles.theArray[profileSelectedIndex];

opener.document.getElementById("{/literal}{$orcidButtonId}{literal}").style.display = "none";;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra ';' at the end of the line?

Comment thread templates/orcidProfileSearchResults.tpl Outdated
var profileSelected = profiles.theArray[profileSelectedIndex];

opener.document.getElementById("{/literal}{$orcidButtonId}{literal}").style.display = "none";;
opener.document.getElementById("remove-orcid-button-" + authorIndex).style.display = "inline";;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra ';' at the end of the line?

@atarix83

Copy link
Copy Markdown
Author

Hi @defstat thanks for reviewing.

I've made changes in base of your suggestions and fixed the issues.
I hope I've not forgotten anything

@asmecher

Copy link
Copy Markdown
Member

@atarix83 and @defstat, part of merging this will also involve forward-porting it to OJS 3.x so it's not lost. @atarix83, do you have plans to upgrade to OJS 3.x?

@atarix83

Copy link
Copy Markdown
Author

@asmecher yes I would to upgrade to OJS 3.x but now I can't schedule an upgrade

@defstat

defstat commented Jul 25, 2017

Copy link
Copy Markdown

@atarix83 I have done some work for OMP if that could be of help - https://github.com/defstat/orcidProfile/tree/hirmeos_orcid

@asmecher

Copy link
Copy Markdown
Member

@defstat, this is an old PR but I see a few commits sneaking into it more recently -- are you keeping an eye on it?

@defstat

defstat commented Mar 16, 2018

Copy link
Copy Markdown

@asmecher No but I can.

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.

5 participants