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

Integrate the public Orcid API to simplify person lookup for project-affiliated contacts #986

Draft
wants to merge 23 commits into
base: development
Choose a base branch
from

Conversation

sven1103
Copy link
Contributor

@sven1103 sven1103 commented Jan 16, 2025

Uses the public Orcid API to look up persons that the user might want to add as contact in a project.

@sven1103 sven1103 changed the title feature/#948-integrate-orcid-public-api-search-for-person-query Integrate the public Orcid API to simplify person lookup for project-affiliated contacts Jan 16, 2025
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Steffengreiner and others added 17 commits February 25, 2025 11:39
…y link to sandbox during development.

Finally ensure that edge cases are handled (Exception handling, wild card searches etc.)
…on-query' of https://github.com/qbicsoftware/data-manager-app into feature/#948-integrate-orcid-public-api-search-for-person-query
@Steffengreiner
Copy link
Contributor

Just to give a quick update: This PR now works as intended, however the UI is basic and still needs to be confirmed with @Shraddha0903.

Additionally the PR was tested with orcid Sandbox Instance so this needs to be adapted to the real orcid repository.
Finally make sure to set the environment variables to the sandbox instance as required:

ORCID_API_CLIENT_ID=Your_Awesome_Application_Client_Registered_With_Sandbox_Orcid; ORCID_API_CLIENT_SECRET=Your_Awesome_Application_Secret_Registered_With_Sandbox; ORCID_AUTHORIZATION_URL=https://sandbox.orcid.org/oauth/authorize; ORCID_JWK_SET_URI=https://sandbox.orcid.org/oauth/jwks; ORCID_TOKEN_URI=https://sandbox.orcid.org/oauth/token; ORCID_USERINFO_URI=https://sandbox.orcid.org/oauth/userinfo

Open Questions include:

1.) I had to extend the project information in the database to contain the oidc information for all 3 contacts (principal investigator, project manager, person responsible). I'd rather refactor this into a seperate table, maybe in a new PR?

2.) Understanding and extending the BoundContactField is actually not trivial and we get a lot of complicated edge cases due to the Vaadin implementations. This could be refactored way easier without employing any of the Vaadin Customfield logic and just write it more basic.

3.) The Contact information was now extended to also include the oidc and oidcissuer information since some of our users have it and some don't. The question is then why we still employ 2 distinct qbicuserdetails.

4.) I also had to account for a lot of edge cases due to the HTTP responses from the orcid responses such as:

a) Special Character usage like e.g. two dashes "--" leads to a 401 response from the repository
b) Checking if the connection to the database could be made and logging errors if it doesn't
c) Invalid Entries within the sandbox instance had to be filtered (e.g. no fullname or email), Unsure if this happens in the real repository.

I really appreciate a throughout review and feedback 👍

@Override
public List<Contact> findAll(String query, int limit, int offset) {
//Orcid queries will fail if the user input is not sanitized
var sanitizedInput = query.replaceAll("[^a-zA-Z0-9]", "");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me sanitation is a service concern, not a repository one. I would also not allow it, instead of magically removing stuff. If the user puts in weird characters, you can return an empty result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants