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

contrib: fix orcid configuration #211

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Conversation

jma
Copy link
Contributor

@jma jma commented Jun 16, 2020

  • Adds error message when a remote server error occurs.

Signed-off-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch

@@ -91,7 +91,7 @@
'show_login': 'true'},
base_url='https://pub.orcid.org/v1.2/',
request_token_url=None,
access_token_url="https://api.orcid.org/oauth/token",
access_token_url="https://pub.orcid.org/oauth/token",
Copy link
Member

Choose a reason for hiding this comment

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

@jma did orcid change their access token URL? The old doesn't exist anymore?

Copy link
Contributor Author

@jma jma Jun 16, 2020

Choose a reason for hiding this comment

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

No, this is probably a copy/paste error. Look line 92. pub is for public and api is for members which is define latter in the file.

Copy link
Member

Choose a reason for hiding this comment

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

Can you fix it then? :)

Copy link
Contributor Author

@jma jma Jun 24, 2020

Choose a reason for hiding this comment

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

This is the purpose of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I misread your message and thought you made a copy/paste error, not that the issue being fixed was a copy/paste error. Sorry for the misunderstanding 😅

Now I understand the issue :). We will just need to crosscheck the change with Zenodo.

Copy link
Member

@slint slint Jun 24, 2020

Choose a reason for hiding this comment

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

And apparently, it seems that we're all wrong :)

Not sure what version we're using but according to the article the {pub,api}.orcid.org endpoints are deprecated (in practice we are using them in Zenodo production though...).

Copy link
Member

Choose a reason for hiding this comment

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

So, @slint we should change the access_token_url to https://orcid.org/oauth/token, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zzacharo I also misunderstood you comment, I apologise for this.

Copy link
Member

Choose a reason for hiding this comment

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

@jma no worries! We will test the change in our side with @slint and I will take care of any other change needed/merge/release :) I will inform you when the new version is out!

Copy link
Member

Choose a reason for hiding this comment

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

Tested locally with access_token_url='https://orcid.org/oauth/token' and everything works as expected

jma and others added 2 commits June 24, 2020 15:42
* Adds error message when a remote server error occurs.

Signed-off-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>
@zzacharo
Copy link
Member

@slint @jma I updated the PR with the access_token_url changes. Please have another look so we can merge :)

Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

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

LGTM, :shipit:

@lnielsen lnielsen merged commit 085cf70 into inveniosoftware:master Jun 24, 2020
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.

None yet

4 participants