Royal-Society-of-New-Zealand / NZ-ORCID-Hub

The home of development for the New Zealand ORCID Hub
MIT License
13 stars 8 forks source link

Other-IDs task requires a URL when the ORCID model doesn't #1270

Closed Jason-Gush closed 4 years ago

Jason-Gush commented 4 years ago

Whether by batch upload or via form, the Hub requires a external-id-url in Other IDs despite the model being:

Jason-Gush commented 4 years ago

Seems to be the problem.

https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/blob/a2e5af4273aa3924e187504e7c093ee999f3208a/orcid_hub/models.py#L4304

Only passes when both URL and relationship are present, despite both being optional.

@rpaw053 any recall what you were intending here? Seems like lines 4304-4308 should just be deleted.

rpaw053 commented 4 years ago

@Jason-Gush I don't actually remember why i did that https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/blob/a2e5af4273aa3924e187504e7c093ee999f3208a/orcid_hub/models.py#L4304

Most likely my copy paste error in commit https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/commit/15236ea635e26af45e15d862f3a774dc7b7138f1

Since in our reference model both url and relationship are optional, we can safely delete the lines(4304-4308) as you suggested https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/blob/a2e5af4273aa3924e187504e7c093ee999f3208a/orcid_hub/models.py#L4092

load from json doesn't have that condition https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/blob/a2e5af4273aa3924e187504e7c093ee999f3208a/orcid_hub/models.py#L4369

So just if condition in load from csv method needs to be deleted https://github.com/Royal-Society-of-New-Zealand/NZ-ORCID-Hub/blob/a2e5af4273aa3924e187504e7c093ee999f3208a/orcid_hub/models.py#L4304

rpaw053 commented 4 years ago

image

I was about to make changes but this still doesnt work from ORCID side

rpaw053 commented 4 years ago

You can test this here: https://api.sandbox.orcid.org/v2.0/#!/Development_Member_API_v3.0/createExternalIdentifierv3

Jason-Gush commented 4 years ago

Turns out url is the canonical required field (tho not specified in either xsd or swagger :() https://github.com/ORCID/ORCID-Source/blob/master/orcid-api-web/tutorial/personal_identifiers.md As a consequence, shouldn't be merged unless there's a surprise and ORCID changes the model.