-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix datacite renderer #117
Conversation
Pull Request Test Coverage Report for Build 3689087086
💛 - Coveralls |
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 changes looks ok. But I guess they changes the metatda model, so we need to add the migration to this PR.
Did you test this on test machine already? For which instance?
|
||
if not name: | ||
name = '{}, {}'.format(last_name, first_name) | ||
|
||
self.node(person_type + 'Name', {}, name) | ||
self.node(person_type + 'Name', {'nameType': name_type}, name) |
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.
Where is nameType
coming from? Is that just an additional field in the JSON entry for authors, contributors?
If yes then we should check fro missing values:
if name_type is not None:
self.node(person_type + 'Name', {'nameType': name_type}, name)
else:
self.node(person_type + 'Name', {}, name)
Except there is some kind of validation of the JSON fields before calling the render_person
method? In which case we need to add a validation for name_type.
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 name_type
comes from
name_type = person.get('name_type')
This returns None
if name_type
is not defined in metadata and it will not appear on the datacite rendered page. Just checked it again on my local machine.
An example (name_type
is not defined for my name but for Joe Doe)
<creators>
<creator>
<creatorName nameType="Organizational">Leibniz Institute for Astrophysics Potsdam</creatorName>
</creator>
<creator>
<creatorName>Makan, Kirill</creatorName>
<givenName>Kirill</givenName>
<familyName>Makan</familyName>
<nameIdentifier nameIdentifierScheme="ORCID" schemeURI="http://orcid.org/">0000-0000-0000-0000</nameIdentifier>
<affiliation affiliationIdentifier="https://ror.org/03mrbr458" affiliationIdentifierScheme="ROR" schemeURI="http://ror.org">Leibniz Institute for Astrophysics Potsdam</affiliation>
</creator>
<creator>
<creatorName nameType="Personal">Doe, Joe</creatorName>
<givenName>Joe</givenName>
<familyName>Doe</familyName>
<nameIdentifier nameIdentifierScheme="ORCID" schemeURI="http://orcid.org/">0000-0000-0000-0000</nameIdentifier>
</creator>
</creators>
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.
ok then all fine. Let us give a try and see how it renders on the registry side.
The fields |
I see, and since the licences identifiers are MethodsSerializers they do not require either changes in the model. |
Although in this case the entry is defined: |
There is one additional change that I would like make. Currently, we provide only the "dates": [
{
"date": "2022-06-13",
"dateType": "Updated"
},
{
"date": "2022",
"dateType": "Issued"
}
], This date follows the I suggest that we provide the |
@kimakan 👍
|
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.
ok for me!
Fixes the datacite renderer in a way that make it compatible with https://schema.datacite.org/meta/kernel-4.4/doc/DataCite-MetadataKernel_v4.4.pdf
Some of the fixes are: