Skip to content
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

Additional fixes and text changes for the about page Stable 3 1 2 #77

Open
wants to merge 15 commits into
base: stable-3_1_2
Choose a base branch
from

Conversation

nils-stefan-weiher
Copy link

@nils-stefan-weiher nils-stefan-weiher commented Sep 11, 2019

Hi everyone,

I had some time to work on several Issues with the ORCID plugin in OJS 3.1.2.

In this pull request the ORCID icon display problems should be addressed for multiple authors.
Collected here in this issue: https://github.com/pkp/pkp-lib/issues/4935.

Also additional checks during the ORCID authorization should not write empty ORCID Ids to the database, should address the following: https://github.com/pkp/pkp-lib/issues/4852

The about page got the correct text and locale keys for english and german locales, should fix the following issue:
https://github.com/pkp/pkp-lib/issues/5007

I also added code to distinguish betweend sandbox ORCID ids in the user profile display, but not yet for the article page or the contributor form: pkp/pkp-lib#4893

Other issues would be addressed in the future, as we are in the progress of updating our OJS instances to 3.1.2 and the plugin may be used more.

Regards,

Nils Weiher

Copy link
Member

@asmecher asmecher left a comment

Choose a reason for hiding this comment

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

Looks good, @isgrim, just a couple of small tweaks to recommend. There are a bunch of functions missing their code self-documentation so that would be a welcome addition as well, if you like. Let me know once you've had a chance to look these comments over and push up any changes, and I'll merge and forward-port to master! I haven't tested the changes, just reviewed the code by eye. Thanks!

$response = json_decode($result, true);
$orcid = $response['orcid'];
if (!$orcid) {
// error during authorization
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using the fatalError function in this case, especially since we're not using e.g. a template to format a user-facing error page in the standard OJS layout. Alternately, we're starting to make more use of Exceptions; that would be OK as well.

<div class="description">
{translate key="plugins.generic.orcidProfile.about.orcidExplanation"}
</div>
<h3>{translate key="plugins.generic.orcidProfile.about.howAndWhy.title"}</h3>
<div class="description">
{translate key="plugins.generic.orcidProfile.about.howAndWhy"}
{$contextName}
Copy link
Member

Choose a reason for hiding this comment

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

Needs |escape Smarty filter

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.

2 participants