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

Service: Fix bug where contributor email disappears #179

Conversation

ValentinFutterer
Copy link
Collaborator

Description

Bugfix

What does this PR do?

#128 introduced the fetching of ORCID-emails on Contributor create or update. The fetched email was alaways used instead of the current one, leading to it being overwritten - most likely with null.
Now the ORCID email is only used when no other email is present (this leads to ORCID email updates not being picked up by the tool, but this should not be a big problem in my opinion).

Checks

  • Tested with Oracle/PostgreSQL
  • Export updated
  • Documentation added
  • Tests added
  • E2e tests created
  • Successfully ran e2e tests before merge

closes GH-178

Copy link
Collaborator

@rekt-hard rekt-hard left a comment

Choose a reason for hiding this comment

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

Thank you for addressing this issue. Works fine.

Can you also address other fields? The personIdentifier and the affiliation might be set via the university already and would be overridden by an ORCID search.

Right now, we do not allow to modify already added contributors via the UI (but would be possible through the API). If we allow edits through the UI, we have to revisit this and make sure that new data takes precedence over old data.

@ValentinFutterer
Copy link
Collaborator Author

Can you also address other fields? The personIdentifier and the affiliation might be set via the university already and would be overridden by an ORCID search.

Good point, I did not think about the other fields :)

@ValentinFutterer ValentinFutterer force-pushed the vf/178-email-disappears-for-contributor-if-they-have-ORCID-ID branch from 1d5d67b to 7c00fcf Compare April 4, 2024 09:23
@ValentinFutterer
Copy link
Collaborator Author

The ContributorDO generated from ORCID has all fields set to null other than name, affiliation, personID and mbox. When the ContributorDOMapper is used, these null values override legit fields. What do you think of the current (naive) solution? The logic should probably be in its own mapper. Should we just add a method to ORCIDMapper like mapORCIDPersonDOToEntity? But this would just chain mapper calls. Maybe adjust mapRecordEntityToPersonDO(ORCIDRecord orcidRecord, ContributorDO contributorDO) to mapRecordEntityToPersonEntity(ORCIDRecord orcidRecord, Contributor contributor) and move the new logic in this mapper method.

@rekt-hard
Copy link
Collaborator

The ContributorDO generated from ORCID has all fields set to null other than name, affiliation, personID and mbox. When the ContributorDOMapper is used, these null values override legit fields. What do you think of the current (naive) solution?

Seems fine to be honest. This is a special case anyway. Adapting a general mapper would most likely lead to future issues.

@ValentinFutterer ValentinFutterer force-pushed the vf/178-email-disappears-for-contributor-if-they-have-ORCID-ID branch from 7c00fcf to 9dead45 Compare April 4, 2024 11:07
@ValentinFutterer ValentinFutterer force-pushed the vf/178-email-disappears-for-contributor-if-they-have-ORCID-ID branch from 9dead45 to 9852fc3 Compare April 4, 2024 11:40
Copy link

sonarqubecloud bot commented Apr 4, 2024

@rekt-hard rekt-hard merged commit 6809d7e into next Apr 4, 2024
2 checks passed
@rekt-hard rekt-hard deleted the vf/178-email-disappears-for-contributor-if-they-have-ORCID-ID branch June 25, 2024 08:39
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.

Email disappears for contributor if they have ORCID ID
2 participants