Repository navigation
fix(series): count split editions of an owned book as covered, not missing (#2524) - #2915
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8d4a14a to
7f8669b
Compare
|
Closing/reopening to retrigger CI — CodeQL failed on all three languages with |
|
Marking this ready for review. Two red checks are visible but both are unrelated to this diff:
Everything else — full Go suite, lint, govulncheck, smoke, ABS contract, CodeQL (go/js/ts all passed on the latest run after a transient bundle-version mismatch cleared), frontend build/lint/test, licenses, secrets/IaC/container/DAST scans — is green. |
vavallee
left a comment
There was a problem hiding this comment.
Nice work on this, the two signal rule is the right call and I couldn't make it misfire. Henry IV style titles, numbered volumes and the Two Volume Edition shape all stay missing, and the tests fail on main the way they should. Merged current main into it locally with no conflicts and the api, seriesmatch and importer suites are green.
Two small things before I merge. The changelog fragment has an em dash in it (our README example does too, my fault), so please drop it and trim the entry a bit since it reads as release notes. And the new covered section on SeriesPage has hardcoded English plus a hand rolled plural, while the rest of that page goes through t(). Could you move those strings into en.json under series.hardcover with i18next plurals?
Not for this PR, just so it's written down: anyone who already filled Stormlight before this has the part rows locally, and Fill will keep queueing them. I'll open a follow-up for that.
7f8669b to
f79d5f3
Compare
|
Fixed both:
Also rebased onto current main (#2554's series rewrite landed in the meantime) — no conflicts in the logic, just the two string additions needing to land in the Noted on the Stormlight-already-filled follow-up, that makes sense as its own issue. |
…ta profile filters to series fill (#3048, #2208) (#3109) * fix(series): skip split edition parts and apply profile filters in series fill (#3048, #2208) Fill and the wanted sweep skip a stored book that is a split edition part of a whole already in the library, using the same two signal rule as #2915. A new series endpoint unmonitors those parts. Series fill now screens the books it creates and re queues through the author's metadata profile, and two admin endpoints preview and unmonitor wanted books a profile filter turned on later would have kept out. Nothing is deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com> * fix(series): series page cleanup, docs and fragments for split parts and profile filters (#3048, #2208) Series page marks split edition parts, leaves them out of the missing count, offers a one click unmonitor and reports what a fill skipped. Drops the profile level retroactive endpoints from the first commit: per author Reconcile catalogue (#2247) already applies every filter to stored rows. Adds the db test for the covered split part query, docs and changelog fragments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com> * fix(series): let unknown languages through a series fill and stop counting hand monitored books as skipped (#2208) Review follow up. Hardcover series entries carry no language, so a profile with unknown languages set to fail rejected every book of an add all whenever OpenLibrary was primary. A blank language now passes during a fill, and a failed language evidence lookup passes too, the same unknown passes rule MinPages and SkipMissingISBN use. Only a definitive "not allowed" rejects. The re-queue loop now screens only unmonitored stored rows. A monitored book is already being sought, often because it was added by hand, so it is queued as before and no longer reported as skipped on every fill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com> --------- Signed-off-by: vavallee <vavallee@protonmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
vavallee
left a comment
There was a problem hiding this comment.
Thanks for both fixes, the en.json plurals and the clean changelog look good. Since this was opened, #3109 landed on main with seriesmatch.SplitEditionPartOf and BookRepo.ListCoveredSplitEditionParts, and its description asks whichever PR lands second to line up with the other.
Could you switch splitEditionWhole over to SplitEditionPartOf and drop IntegerFloorPosition, SplitPartOf and the second copy of the Part N regex? HasPartMarker and StripPartMarker can stay and use the shared regex.
Also, the rule on main only counts a whole that is not excluded and is imported or monitored. Right now this PR covers parts under any present row, so an excluded or unmonitored whole hides its parts here while the sweep still treats them as books. Gating the wholes the same way, with a test for each case, would keep the two in agreement.
Last thing: the wiki now has two paragraphs on this (lines 90 and 98), so merging them into one without the em dashes would be great. Really nice work on the Edgedancer and Wheel of Time cases.
…ssing (vavallee#2524) Some Hardcover catalogues list a novel's own split-into-parts editions beside the whole novel: Stormlight's "The Way of Kings" at position 1, with "Part 1"/"Part 2" at 1.1/1.2. Already owning the whole novel left those parts reading as missing volumes, counted against the series, and sitting behind an add button that would create and queue a download for text already on the shelf. buildHardcoverDiff now checks each unclaimed catalogue row against the series' own Present rows: a row at a fractional position whose integer floor is a Present whole work, and whose title is that work's title plus a "Part N" marker, is reported as Covered instead of Missing. Both signals are required — position alone would also catch a real novella at a neighbouring fractional position (Edgedancer at 2.5 beside Words of Radiance at 2), and title alone says nothing about position. A series represented ONLY by its split parts (Wheel of Time's "Two Volume Edition", no unsplit row to speak of) is unaffected: there is nothing Present to cover the parts, so they stay Missing. Both Fill write paths iterate diff.Missing, so Covered rows stop being created and queued automatically, with no change needed there. internal/seriesmatch gains IntegerFloorPosition and SplitPartOf, plus the HasPartMarker/StripPartMarker pair moved from internal/importer (one regex, one package, now that series.go needs them too). The Series page shows covered rows in their own collapsed section, linked to the book that already covers them, with no add button. Refs vavallee#2524 (not Closes: this is the agreed first narrow slice — the broader work-identity check the issue asks for stays open). Signed-off-by: gch ahcg <gchahcg@proton.me>
Moving them here from internal/importer (vavallee#2524) left them reached only transitively through importer's own tests, which this package's own coverage profile does not credit — Codecov flagged both at 0% on the vavallee#2524 PR despite internal/importer's existing tests exercising them on every run. Direct table tests fix the gap without changing behaviour. Signed-off-by: gch ahcg <gchahcg@proton.me>
Review feedback: the new covered section on SeriesPage had hardcoded English and a hand-rolled plural, while the rest of the page goes through t(). Moved both strings to series.hardcover.coveredSection (i18next _one/_other) and series.hardcover.coveredSubtitle. Also drops the em dash from the changelog fragment and trims it — review feedback that it read as release notes rather than a changelog bullet. Signed-off-by: gch ahcg <gchahcg@proton.me>
…avallee#2524) vavallee#3109 landed on main while this was open and introduced its own copy of the same two-signal rule (seriesmatch.SplitEditionPartOf, db.ListCoveredSplitEditionParts) for rows already stored, asking whichever PR landed second to line up with the other. - splitEditionWhole now calls seriesmatch.SplitEditionPartOf instead of the separate IntegerFloorPosition/SplitPartOf pair, which are removed; HasPartMarker/StripPartMarker keep their own tests but now share split_edition.go's splitEditionMarkerRe instead of a second copy of the same regex. - The eligibility gate ListCoveredSplitEditionParts applies to a stored whole (not excluded, imported or monitored) now gates a Present row's eligibility to cover a catalogue-diff part too, via a new eligibleSplitEditionWhole helper cross-referenced from both sides of the SQL/Go boundary, with a test for each half of the gate (excluded, wanted-and-unmonitored, wanted-and-monitored). - Merged the wiki's two split-edition paragraphs into one and dropped the em dashes. Review feedback from vavallee. Signed-off-by: gch ahcg <gchahcg@proton.me>
f79d5f3 to
b53f844
Compare
|
Consolidated with #3109:
Ran Full suite green: gofmt/vet/lint/govulncheck, full Go tests, frontend lint/typecheck/build/tests. |
|
The |
vavallee
left a comment
There was a problem hiding this comment.
Thanks gch, this is exactly what I was after. Dropping the duplicate helpers and calling SplitEditionPartOf directly, plus the status gate that mirrors the SQL with a comment pointing at it, makes the diff and the sweep agree. The excluded and unmonitored whole tests both fail when I break the gate, which is what I wanted to see.
One small wording thing, non-blocking: "books you own" in the series section title, the wiki and the changelog is a bit off, since a monitored wanted whole counts too. "books in your library" would be closer. Otherwise good to go.
The red lint and Container Scan checks are the Go 1.26.8 issue, fixed on main by #3131, so merging main in will clear them.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
Provider lists in the README, the roadmap Delivered and Next up sections, the In Library meaning for partly owned dual format books, ratings, the Authors list have / total count and the author page sort and filters, the series field in the book edit table, the availableBookCount and rating fields, the rating sort values and the primary provider values in the API reference, and the one time dedup key recompute on first boot. Documents #3125, #3095, #3088, #3134, #3090, #3127, #3150, #3158, #3116, #2915 and #2994. No new environment variables, dependencies or breaking behaviour shipped, so DEPLOYMENT.md env tables, THIRD_PARTY_LICENSES.md and docs/upgrade-v1.md are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
Provider lists in the README, the roadmap Delivered and Next up sections, the In Library meaning for partly owned dual format books, ratings, the Authors list have / total count and the author page sort and filters, the series field in the book edit table, the availableBookCount and rating fields, the rating sort values and the primary provider values in the API reference, and the one time dedup key recompute on first boot. Documents #3125, #3095, #3088, #3134, #3090, #3127, #3150, #3158, #3116, #2915 and #2994. No new environment variables, dependencies or breaking behaviour shipped, so DEPLOYMENT.md env tables, THIRD_PARTY_LICENSES.md and docs/upgrade-v1.md are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee <vavallee@protonmail.com>
Summary
Some Hardcover catalogues list a novel's own split-into-parts editions beside the whole novel — Stormlight's The Way of Kings at series position 1, with its "Part 1"/"Part 2" at 1.1/1.2. Already owning the whole novel left those parts reading as missing volumes on the Series page: counted against the series, and sitting behind an add button that would create and queue a download for text already on the shelf. This is the agreed first narrow slice of #2524 — the "split editions of an owned book" shape from the issue thread's real Stormlight/Wheel of Time catalogue data. Refs #2524 (not Closes: the broader work-identity check the issue asks for — reprints, translations, audiobook-slot filling, a reconciliation preview — stays open).
The rule
A catalogue row is reported as covered, not missing, when both hold:
1.1/1.2under1).Part N/Pt. N/Part N of Mmarker.Both signals are required on purpose:
Part Nmarker — position alone is not enoughImplementation notes
internal/seriesmatch:IntegerFloorPosition(is a position fractional, and what's its integer floor) andSplitPartOf(does a title reduce to a whole title plus a part marker) are the two new primitives.HasPartMarker/StripPartMarkermoved here frominternal/importer/volume.go— one regex, one package, now thatseries.goneeds the same marker.internal/api/series.go:buildHardcoverDiffgains asplitEditionWholecheck in the unclaimed-catalogue loop, run after the existing box-set prune. A newCovered []seriesHardcoverDiffBookfield on the diff response carries the suppressed rows, each pointing at the whole work'sLocalBookID/LocalTitlerather than one of its own.createMissingHardcoverBooks,createMissingHardcoverBook) iteratediff.Missingonly, so Covered rows stop being created and queued automatically — no change needed there.web/src/pages/SeriesPage.tsx: covered rows render in their own collapsed<details>section below Missing, each linking to the book that already covers it, no add button.Where it's mounted
internal/api/series.go:buildHardcoverDiff, newsplitEditionWholehelper right after it.internal/seriesmatch/match.go:IntegerFloorPosition,SplitPartOf,HasPartMarker,StripPartMarker.internal/importer/volume.go:volumeTitlesnow calls the movedseriesmatchhelpers instead of its own copy.web/src/api/series.ts,web/src/pages/SeriesPage.tsx.Checklist
seriesmatchtable tests for both new primitives; a direct unit test ofsplitEditionWholeagainst crafted Present rows (covers the Uncertain/Missing-whole case without needing to engineer a fuzzy title score); twobuildHardcoverDiffhandler tests (the Stormlight shape, the Wheel-of-Time "no unsplit whole" trap); aFillwrite-path test; aSeriesPagerender testdocs/Hardcover-Series-Wiki.md(What It Does list + a new paragraph after the box-set rule)changelog.d/2524-split-editions.mdTest plan
gofmt -l .,go vet ./...,golangci-lint run(v2.11.4),govulncheck ./...go test ./cmd/... ./internal/...(full suite, no race)make test-race— all shards pass exceptinternal/db, which hits this sandbox's own slow-hardware timeout unrelated to this change (zero lines ofinternal/dbtouched here; reproduces identically in isolation with nothing else running, and the individual test that was mid-flight passes in 3s on its own)make smokecd web && npm run lint && npm run typecheck && npm run build && npm test/code-review(high effort) on the diff — came back clean🤖 Generated with Claude Code
https://claude.ai/code/session_01WA2CUNg5o4wUBaX6yWXNNk