fix(doi link): show appropriate DOI link in test(any) environment#2269
fix(doi link): show appropriate DOI link in test(any) environment#2269fenekku wants to merge 1 commit intoinveniosoftware:masterfrom
Conversation
max-moser
left a comment
max-moser
left a comment
There was a problem hiding this comment.
Looks good from my perspective; just a few questions to make sure I understand everything correctly before accepting 😃
| record_dumped.setdefault("_extras", {}) | ||
| record_dumped["_extras"]["links"] = record.get("links", {}) | ||
|
|
||
| return get_citation_string(record_dumped, record["id"], style_filepath, locale) |
There was a problem hiding this comment.
Just for my understanding: Effectively, the "protocol" for get_citation_string() gets extended so that if an _extras field with a DOI link is present, then that will be used to replace the default doi.org link?
In this code path (serialize_object()) that will always be the case; external calls can use it if they want, or keep using it without (in which case the logic is backwards-compatible).
There was a problem hiding this comment.
Effectively, the "protocol" for get_citation_string() gets extended so that if an _extras field with a DOI link is present, then that will be used to replace the default doi.org link?
Yes that's right. I didn't want to add additional keyword arguments to get_citation_string as it would be getting christmas tree like and the doi link seemed like it really had to do with the serialized record. All the replacements and fetching of that data is very backwards compatible/cautious so doesn't alter anything unduly.
| result = serializer.serialize_object(full_record_to_dict) | ||
|
|
||
| expected = ( | ||
| "Nielsen, L. H.and B. Tom. Inveniordm. v1.0, InvenioRDM, 2018–Sept. 2020, " |
There was a problem hiding this comment.
I think that'd be an issue in citeproc, but is there a space missing between the the end of the first author and and? 👀
There was a problem hiding this comment.
Oh good eye on that one. It's indeed the citeproc's MLA citation that is like that e.g., https://zenodo.org/records/18927380 (and select MLA) :/ Not much we can do for now.
| invenio-communities>=23.0.0,<24.0.0 | ||
| invenio-drafts-resources>=8.0.0,<9.0.0 | ||
| invenio-records-resources>=9.0.0,<10.0.0 | ||
| invenio-records-resources>=9.1.0,<10.0.0 |
There was a problem hiding this comment.
Just wondering; this PR looks pretty self-contained, I couldn't find anything that obviously needs a dependency bump?
If it's useful, it can of course stay.
There was a problem hiding this comment.
Ah yeah this wasn't related to this PR directly but I bumped into it when running tests: this commit 179949e introduced import of DateFacet, but DateFacet are only available in invenio-records==9.1.0+, so that dependency had to be bumped.
2 participants
This fixes DOI links shown in citations.
Related to inveniosoftware/invenio-app-rdm#3356 .