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

feat: fetch published doi if preprint #6311

Merged
merged 26 commits into from
Jan 6, 2024

Conversation

danieljhegeman
Copy link
Contributor

@danieljhegeman danieljhegeman commented Dec 4, 2023

  • default to preprint if published doi cannot be fetched
  • remove unused backend.common.providers.crossref_provider
  • change crossref unit tests to use correct crossref provider

Reason for Change

Changes

  • add
  • remove
  • modify

Testing steps

  • Either list QA steps or reasoning you feel QA is unnecessary
  • Describe how you made sure to know that your changes worked. Should allow someone else to go verify your code without in depth knowledge.
  • "Unit tested only", "tested in rdev by a, b, c, verifying feature worked by... ", "manually ran pipeline locally with these results: ..."

Checklist 🛎️

  • Add product, design, and eng as reviewers for rdev review
  • For UI changes, add screenshots/videos, so the reviewers know what you expect them to see
  • For UI changes, add e2e tests to prevent regressions

Notes for Reviewer

- default to preprint if published doi cannot be fetched
- remove unused backend.common.providers.crossref_provider
- change crossref unit tests to use correct crossref provider
Copy link
Contributor

github-actions bot commented Dec 4, 2023

Deployment Summary

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (58d4708) 92.42% compared to head (5305e47) 92.51%.

Files Patch % Lines
backend/layers/thirdparty/crossref_provider.py 85.71% 3 Missing ⚠️
backend/layers/business/business_interface.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6311      +/-   ##
==========================================
+ Coverage   92.42%   92.51%   +0.08%     
==========================================
  Files         179      179              
  Lines       14702    14783      +81     
==========================================
+ Hits        13588    13676      +88     
+ Misses       1114     1107       -7     
Flag Coverage Δ
unittests 92.51% <97.08%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

try:
published_doi = doi_response_message["relation"]["is-preprint-of"]
# the new DOI to query for ...
if published_doi[0]["id-type"] == "doi":
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if they ever actually put anything other than a DOI, but since the API returns a list here, we should iterate and look for an entry with id-type 'doi' rather than just checking the first entry

try:
published_doi = doi_response_message["relation"]["is-preprint-of"]
# the new DOI to query for ...
if published_doi[0]["id-type"] == "doi":
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if they ever actually put anything other than a DOI, but since the API returns a list here, we should iterate and look for an entry with id-type 'doi' rather than just checking the first entry

published_doi = doi_response_message["relation"]["is-preprint-of"]
# the new DOI to query for ...
if published_doi[0]["id-type"] == "doi":
return self.fetch_metadata(published_doi[0]["id"])
Copy link
Contributor

@nayib-jose-gloria nayib-jose-gloria Dec 4, 2023

Choose a reason for hiding this comment

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

I noticed that the CrossRef API returns DOIs as strings with the /'s escaped (i.e. "10.3389\/fcvm.2020.00052" from https://api.crossref.org/works/10.1101/2019.12.31.892166). I think we'd need to strip these esc characters if they exist (unless urlparse and/or requests.get already implicitly account for that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed; the single \ in the context of \/ gets removed once you load from json into dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

curl https://api.crossref.org/works/10.1101/2019.12.31.892166 | jq '.message.DOI'

Copy link
Contributor

@nayib-jose-gloria nayib-jose-gloria left a comment

Choose a reason for hiding this comment

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

approving pending an rdev test confirming functionality

Copy link
Contributor

github-actions bot commented Jan 1, 2024

This PR has not seen any activity in the past 2 weeks; if nobody comments or reviews it in the next 3 days, this PR will be closed.

@github-actions github-actions bot added the stale label Jan 1, 2024
@github-actions github-actions bot removed the stale label Jan 3, 2024
@danieljhegeman danieljhegeman merged commit 59c3b49 into main Jan 6, 2024
40 checks passed
@danieljhegeman danieljhegeman deleted the dan/575-preprint-published-doi branch January 6, 2024 01:21
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.

2 participants