Cambridge Core: Don't prefix abstracts of articles with a graphical abstract - #3636
Merged
AbeJellinek merged 4 commits intoAug 6, 2026
Merged
Conversation
…bstract Articles with a graphical abstract have two div.abstract nodes: the first holds only the figure and has no text. ZU.xpathText() joined both with its default ", " delimiter, so those abstracts were saved starting with a stray comma and space. Use the first node that actually has text. The text is now passed through ZU.trimInternal() as well -- xpathText() does no trimming, so abstracts could also carry a trailing space. Adds a test case for an affected article, which fails without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cambridge now redirects /article/abs/<slug>/<id> to /article/<slug>/<id>, so
the url recorded in four test cases no longer matched what the translator
saves. Only those four item urls change.
The book and bookSection cases still fail, as they already do on master, for
an unrelated reason: Cambridge's RIS export now emits the DOI with the value
itself prefixed ("DO - DOI: 10.1017/..."), and the connector's bundled
schema (version 28) does not allow DOI on book or bookSection, so the value
is dropped rather than falling back to Extra. A current desktop client keeps
it as a proper DOI field, so saved items are unaffected. Left alone here
rather than recording the loss as expected output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit b5e747b. /article/abs/<slug>/<id> only redirects to /article/<slug>/<id> for readers who have access to the article, so the url the translator saves depends on the environment: an institutional network gets the canonical form, while GitHub's runners get the /abs/ form the test cases already recorded. The original URLs are correct for CI, and those four cases were only failing on my own machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Small correction on my own part, since the first CI run here looked worse than it should have: I'd also refreshed the
CI on the current head fails only cases 5, 6 and 12 — the book/bookSection DOI issue described in the description, which fail on master too. Case 13, the one this fix adds, passes. |
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.
Abstracts of Cambridge Core articles that have a graphical abstract are saved with a stray
,in front:Such articles have two
div.abstractnodes — the first holds only the figure and has no text.ZU.xpathText()joins all matched nodes with its default", "delimiter, so the empty first node contributed the separator. This affects a lot of Journal of Fluid Mechanics articles; 188 of the 1367 JFM items in my own library are affected.The fix takes the first
div.abstractthat actually has text. It deliberately does not passdelimiter: ''toxpathText(), which would silently concatenate genuinely multi-node abstracts instead.The text now also goes through
ZU.trimInternal().xpathText()does no trimming, so abstracts could pick up a trailing space as well — e.g. the abstract of Micro-bubble morphologies following drop impacts onto a pool surface is saved 869 characters long, one trailing space more than its text. All seven existing test cases with abstracts are alreadytrimInternal-identical, so nothing there changes.Verification
The new test case (
10.1017/jfm.2023.224) fails on master's code and passes with this change — I ran the harness on master's code with the new test case added to confirm the guard actually catches the bug. No other case changes state:Case 13 is the only one whose state changes. Cases 1, 2, 9 and 10 fail locally for an
unrelated, environment-dependent reason described below; on CI they pass, so this branch's
CI shows only 5, 6 and 12 — the same three as master.
A note on four other cases
I initially also refreshed the
urlof cases 1, 2, 9 and 10, having seen them fail locally: Cambridge redirects/article/abs/<slug>/<id>to/article/<slug>/<id>, so the saved url didn't match. That was wrong and I've reverted it — the redirect only happens for readers with access to the article, so an institutional network gets the canonical form while CI gets the/abs/form the test cases already record. The existing URLs are correct; those four only fail on a subscribed network.Left alone: the three book/bookSection cases
Cases 5, 6 and 12 fail on master too, for a reason unrelated to this PR, and I'd rather flag it than record it as expected output.
Cambridge's RIS export now emits the DOI with the value itself prefixed:
The connector's bundled schema (version 28) doesn't allow
DOIonbookorbookSection, so the value is dropped instead of falling back toExtra— which is where these test cases expect it ("extra": "DOI: 10.1017/9781108638210"). A current desktop client keeps it as a properDOIfield (I checked: it imports as10.1017/9781108770750either way, socleanDOIcopes with the prefix), so saved items are unaffected.So this looks like a test-environment artifact rather than a user-facing regression, and refreshing those fixtures would bake "no DOI" into the repo. Happy to include a fix if you'd prefer one here — stripping the prefix in this translator would be a one-liner — but it seemed better not to guess at how you'd want the schema pinning handled.