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.

Sign up for GitHub

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

Jump to bottom

Issue-1602 Added test case for normalizing ORCID ids with and withou … #164

Merged
merged 5 commits into from
Oct 5, 2023
Merged

Issue-1602 Added test case for normalizing ORCID ids with and withou … #164

merged 5 commits into from
Oct 5, 2023

Conversation

ashwinisukale
Copy link
Contributor

@ashwinisukale ashwinisukale commented Oct 4, 2023 •
edited
Loading

…schemeURI

Purpose

Added test case normalising ORCID ids with and without schemeURI

closes: datacite/datacite#1602

Approach

Open Questions and Pre-Merge TODOs

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@ashwinisukale ashwinisukale requested a review from a team October 4, 2023 10:56
digitaldogsbody
digitaldogsbody approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@digitaldogsbody digitaldogsbody 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.

It might be worth adding an additional test case for the original behaviour as well (i.e that the following identifier still comes out ok: <nameIdentifier schemeURI="https://orcid.org/" nameIdentifierScheme="ORCID"> https://orcid.org/0000-0001-9998-0117 </nameIdentifier>) but I don't consider this a blocker to merging :)

@ashwinisukale
Copy link
Contributor Author

Thank you @digitaldogsbody :) for the review. I will add the test.

@ashwinisukale
Copy link
Contributor Author

@digitaldogsbody while adding test case, I found one bug in our code, that if we have leading and trialing spaces in the ORCID id then we consider that id as invalid id, I have fixed the bug in the same PR. Could you please review this again.

<creator>
  <creatorName nameType="Personal">Mike B</creatorName>
  <nameIdentifier nameIdentifierScheme="ORCID"> 0000-0001-9998-0118 </nameIdentifier>
 </creator>
"  0000-0001-9998-0118  ",  # valid ORCID with leading/trailing spaces

@digitaldogsbody
Copy link
Member

Great catch!

digitaldogsbody
digitaldogsbody approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@digitaldogsbody digitaldogsbody left a comment

Choose a reason for hiding this comment

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

👍 let's get this rolled out and deployed to the services that use Bolognese

@ashwinisukale
Copy link
Contributor Author

Let me change the gem version if we are going to deploy this change.

@ashwinisukale ashwinisukale merged commit 7b3e8cd into master Oct 5, 2023
5 checks passed
@ashwinisukale ashwinisukale deleted the issues-1602 branch October 5, 2023 10:15
@ashwinisukale ashwinisukale self-assigned this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digitaldogsbody digitaldogsbody digitaldogsbody approved these changes

Assignees

@ashwinisukale ashwinisukale

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ORCID iD normalization may not be working
2 participants
@ashwinisukale @digitaldogsbody