Ensure citation html builder accepts dois with doi: prefix#218
Open
DarianGill wants to merge 4 commits intodevelopfrom
Open
Ensure citation html builder accepts dois with doi: prefix#218DarianGill wants to merge 4 commits intodevelopfrom
DarianGill wants to merge 4 commits intodevelopfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes dataset citation rendering for DOIs that may be stored with a doi: prefix (as produced by the API), ensuring the citation builder generates a correct DOI link and avoids Shiny render errors in the citation subpanel.
Changes:
- Rename
build_dataset_citation_text()tobuild_dataset_citation_html()and update callers/tests accordingly. - Expand DOI detection to accept optional
doi:prefix and normalize the DOI for display/linking. - Adjust citation plain-text extraction to parse from wrapped HTML content before copying.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
R/detail_dataset.R |
Updates DOI detection/normalization and switches detail view citation rendering to the new HTML builder. |
tests/testthat/test_detail_dataset.R |
Renames tests to the new function and adds coverage for doi:-prefixed DOIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nks for datasets instead of vb_code cite links
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deployed on develop rc: https://dev.vegbank.org/rc?cite=doi:10.82902/J17P4J
What
Fixes #217 by ensuring that the citation text builder recognizes both dois that are prefixed with doi: and those without it.
Also updates all copy permalink buttons to specify https instead of a bare vegbank.org url and adds the ability to pass a specific non-vegbank citation url to them as well. The dataset detail view uses that ability to copy doi.org citations for datasets that have dois and identifiers.org citations for legacy accession codes. If neither pattern matches, it falls back to copying https://vegbank.org/cite/ds_code.
Finally, the detail panel css was updated to use dvh instead of just vh to hopefully ensure all details scroll into view (works on my phone now on the dev rc site!). When I opened your linked problem page on mobile the browser controls obscured part of the view and prevented me from actually seeing the error.
Why
Because the api's create_dataset() saves an accession_code with the doi: prefix and this was breaking the application as documented in 217.
How
Testing and Documentation
All 1586 tests pass and check() has no errors, warnings, or notes!