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
Upgrade to the ORCID v3 API #2780
Conversation
please note that the travis build of this PR skips all the e2e tests see the detail here instead on our repository, as we have configured the ORCID_CLIENTID / SECRET as ENV variables the test are successful run |
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.
@abollini : Overall, this looks good. But, I'd like to see a separate IT that can validate this code works against a static/mock file from ORCID. Otherwise, I worry we risk making future code changes that could accidentally break ORCID integration (unless we remember to enable the end-to-end test). Beyond that, minor suggestions below.
dspace-api/pom.xml
Outdated
<groupId>org.dspace</groupId> | ||
<artifactId>orcid-jaxb-api</artifactId> | ||
<version>3.0.0-SNAPSHOT</version> | ||
</dependency> |
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.
The spacing here is messed up. It looks like you are using tabs instead of spaces. Could you correct the alignment here?
* @author Mykhaylo Boychuk (4Science.it) | ||
* | ||
*/ | ||
public class OrcidExternalSourcesIT extends AbstractControllerIntegrationTest { |
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'd like to see a second Integration Test that runs against a sample (static) response from ORCID (which we can enable to run always in Travis CI). Otherwise, we only have this disabled end-to-end test, which wouldn't allow us to continually validate that our code works with the ORCID API.
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.
It looks like previous feedback was resolved by adding some static XML test output and adding a few *MockitoTest()
methods to this same class.
@abollini : Could you update the JavaDocs at the top of this class to note that it does static testing by default (using the *MockitoTest()
methods below), but will do full end-to-end testing (of the real API) when you set orcid.clientid
.travis.yml
Outdated
@@ -21,6 +21,8 @@ jdk: | |||
|
|||
before_install: | |||
# Remove outdated settings.xml from Travis builds. Workaround for https://github.com/travis-ci/travis-ci/issues/4629 | |||
- sed -i 's/^orcid\.clientid.*/orcid.clientid='$ORCID_CLIENTID'/g' dspace-api/src/test/data/dspaceFolder/config/local.cfg | |||
- sed -i 's/^orcid\.clientsecret.*/orcid.clientsecret='$ORCID_CLIENTSECRET'/g' dspace-api/src/test/data/dspaceFolder/config/local.cfg |
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.
You shouldn't need to do any of this at all. The DSpace configuration system will also read values set as environment variables automatically. See https://github.com/DSpace/DSpace/blob/master/dspace/config/config-definition.xml#L29-L37
So, instead of all this logic, you can simply set environment variables named orcid.clientid
and orcid.clientsecret
, and those should override any defaults in local.cfg
.
@@ -120,3 +120,6 @@ rest.properties.exposed = configuration.not.existing | |||
configuration.not.exposed = secret_value | |||
configuration.exposed.single.value = public_value | |||
configuration.exposed.array.value = public_value_1, public_value_2 | |||
|
|||
orcid.clientid = | |||
orcid.clientsecret = |
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.
These should not be needed. Again, these can be set directly as environment variables, so you don't need to insert them here so that they can be replaced later.
@abollini Don't know if you already noticed, but the ORCID V3 model is now on maven: https://mvnrepository.com/artifact/org.orcid/orcid-model/3.0.0, so perhaps we can drop our custom orcid-jaxb-api dependency ? |
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.
This PR adds some needed configurations and replaces the ORCID support with v3. As @KevinVdV referred there is an Orcid artifact (since late April) that can be used.
If you try it out, it will have some impact on the proposed changes:
Dependency convergence error for javax.validation:validation-api:2.0.1.Final paths to dependency are:
+-org.dspace:dspace-api:7.0-beta3-SNAPSHOT
+-org.hibernate.validator:hibernate-validator-cdi:6.0.18.Final
+-org.hibernate.validator:hibernate-validator:6.0.18.Final
+-javax.validation:validation-api:2.0.1.Final
and
+-org.dspace:dspace-api:7.0-beta3-SNAPSHOT
+-org.orcid:orcid-model:3.0.0
+-io.swagger:swagger-jersey-jaxrs:1.5.16
+-io.swagger:swagger-jaxrs:1.5.16
+-io.swagger:swagger-core:1.5.16
+-javax.validation:validation-api:1.1.0.Final
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.
@abollini : I gave this a quick code review (haven't tested it yet). Mostly, it's looking good, but I have a few minor things that require cleanup. See comments below.
* @author Mykhaylo Boychuk (4Science.it) | ||
* | ||
*/ | ||
public class OrcidExternalSourcesIT extends AbstractControllerIntegrationTest { |
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.
It looks like previous feedback was resolved by adding some static XML test output and adding a few *MockitoTest()
methods to this same class.
@abollini : Could you update the JavaDocs at the top of this class to note that it does static testing by default (using the *MockitoTest()
methods below), but will do full end-to-end testing (of the real API) when you set orcid.clientid
hi @tdonohue thanks to review that, your feedback should be addressed now |
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.
👍 Looks good to me now. The ITs look good. I tested this via the External Services and searching ORCID still is working. I did notice that this PR will require minor changes (to i18n keys) in the Angular UI. But, I'm OK with merging this as-is, provided we create a small Angular PR to merge shortly thereafter.
Thanks @abollini !
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.
Thank you @abollini for addressing my comments!
References
https://jira.lyrasis.org/browse/DS-4525
DSpace/orcid-jaxb-api#3
Description
This PR move our integration to the ORCID v3 API adding some configuration placeholder that were missing making a bit harder to enable the feature
Instructions for Reviewers
You need to get at least credentials for public ORCID API or for the sandbox environment and configure the following properties in your local.cfg
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
We would like to add extra IT to prove the end2end intergation with ORCID when client credentials are provided as ENV variables (to allow automatic build on Travis without disclosure of secrets). If the credentials are missing the test will be skipped, this can be also a mechanism to allow execution of these e2e test only for some builds
pom.xml
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.