diff --git a/changelog.d/2524-split-editions.md b/changelog.d/2524-split-editions.md new file mode 100644 index 000000000..4190c218d --- /dev/null +++ b/changelog.d/2524-split-editions.md @@ -0,0 +1,2 @@ +### Fixed +- **Split-edition catalogue rows no longer count as missing books you already own** (#2524): some Hardcover catalogs list a novel's own split-into-parts editions beside the whole novel (The Way of Kings at position 1, with "Part 1" and "Part 2" at 1.1 and 1.2). Already owning the whole novel left those parts listed as missing and offered an add button that queued a duplicate download. They now show in their own collapsed section, linked to the book that covers them, and Fill gaps no longer creates them. Thanks magrhino for the Stormlight and Wheel of Time catalogue data that shaped the rule. diff --git a/docs/Hardcover-Series-Wiki.md b/docs/Hardcover-Series-Wiki.md index 7b89ced87..4fd778be8 100644 --- a/docs/Hardcover-Series-Wiki.md +++ b/docs/Hardcover-Series-Wiki.md @@ -13,7 +13,7 @@ When enhanced Hardcover series data is enabled, Bindery can: - search Hardcover for the matching catalog series - link or unlink a local series to a Hardcover series - compare the local series against the Hardcover catalog -- show present, missing, local-only, and uncertain catalog entries +- show present, missing, local-only, uncertain, and covered (split-edition) catalog entries - add one missing Hardcover book, or add all missing books for a linked series - create wanted and monitored book rows for missing catalog entries and queue searches for them @@ -93,7 +93,7 @@ The fill action may create new authors and books from Hardcover metadata when th When fill re-queues the books already in the series, it also leaves out any that are unmonitored and not imported and that the profile's skip part books or skip missing release date filter rejects, so a fill no longer puts those back on Wanted. A book that is already monitored, for example one you added by hand, is queued as before. A book with no known language is not rejected by the language filter during a fill, whatever the profile does with unknown languages, because Hardcover series entries carry no language; it is resolved later. The other filters need a fresh provider lookup; use **More → Reconcile catalogue…** on the author to apply every filter to books already stored. -Some catalogs list a novel's own split edition beside it, such as "The Way of Kings, Part 1" and "Part 2" at 1.1 and 1.2 under "The Way of Kings" at 1. A book in the series counts as such a split part when both hold: its position is a fraction under the whole's position, and its title is the whole's title followed by "Part N". When the whole is imported, or monitored and wanted, fill does not re-queue the parts and the scheduled wanted search does not search them, so a part of a book you already have is never downloaded. The parts are marked **Split part** on the series card and are not counted as missing. If a fill from an older version left them monitored, **Unmonitor N split parts** on the series card unmonitors them in one click. Nothing is deleted, and an imported part is never touched. A real novella at a fractional position ("Edgedancer" at 2.5) has no "Part N" marker and stays a normal book, and parts with no whole at the integer position are the real volumes and are searched as usual. +Some catalogs list a novel's own split edition beside it, such as "The Way of Kings, Part 1" and "Part 2" at 1.1 and 1.2 under "The Way of Kings" at 1. A book counts as such a split part when both hold: its position is a fraction under the whole's position, and its title is the whole's title followed by "Part N" (a subtitle after the marker is ignored). Position alone is not enough: a real novella at a fractional position, such as "Edgedancer" at 2.5 beside "Words of Radiance" at 2, has no "Part N" marker and stays a normal book, and a catalog that represents a work only as its split parts, with no unsplit row at the integer position at all (Robert Jordan's Wheel of Time "Two Volume Edition" catalog is like this), never triggers the rule either, since there is nothing to cover the parts. On the Hardcover catalog diff a covered part is reported as covered, not missing: it is not counted, not offered an **add** button, and not created by fill, and appears in its own collapsed section on the Series page ("N split editions of books you own") linking to the whole that covers it. The same rule covers rows already stored locally: once the whole is imported, or monitored and not excluded, fill no longer re-queues a stored part and the scheduled wanted search no longer searches it, so a part of a book you already have is never downloaded. Stored parts are marked **Split part** on the series card and are not counted as missing. If a fill from an older version left one monitored, **Unmonitor N split parts** on the series card unmonitors them in one click. Nothing is deleted, and an imported part is never touched. The format dropdown beside **add all** sets the media type of every book the fill creates. Pick **Ebook** and the created books are ebook only, even when Hardcover lists an audiobook edition of the same work, so only one search is queued per book. Pick **Audiobook** for audio only. Pick **Both** if you want Bindery to look for both formats. Books that are already in the series keep whatever media type they were added with, so change those on the book itself. diff --git a/internal/api/series.go b/internal/api/series.go index 9f2720990..c2d7fbd36 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -914,14 +914,19 @@ type seriesHardcoverDiffBook struct { } type seriesHardcoverDiffResponse struct { - SeriesID int64 `json:"seriesId"` - Link *models.SeriesHardcoverLink `json:"link"` - Present []seriesHardcoverDiffBook `json:"present"` - Missing []seriesHardcoverDiffBook `json:"missing"` - LocalOnly []seriesHardcoverDiffBook `json:"localOnly"` - Uncertain []seriesHardcoverDiffBook `json:"uncertain"` - PresentCount int `json:"presentCount"` - MissingCount int `json:"missingCount"` + SeriesID int64 `json:"seriesId"` + Link *models.SeriesHardcoverLink `json:"link"` + Present []seriesHardcoverDiffBook `json:"present"` + Missing []seriesHardcoverDiffBook `json:"missing"` + LocalOnly []seriesHardcoverDiffBook `json:"localOnly"` + Uncertain []seriesHardcoverDiffBook `json:"uncertain"` + // Covered is a catalogue row that is not a missing volume but a split + // edition (#2524) of a book already present: the row's own LocalBookID/ + // LocalTitle point at the whole work that covers it, not at a row of its + // own. Never counted in MissingCount, never offered to Fill. + Covered []seriesHardcoverDiffBook `json:"covered"` + PresentCount int `json:"presentCount"` + MissingCount int `json:"missingCount"` } var ( @@ -1432,6 +1437,7 @@ func buildHardcoverDiff(ctx context.Context, books *db.BookRepo, userID int64, s Missing: []seriesHardcoverDiffBook{}, LocalOnly: []seriesHardcoverDiffBook{}, Uncertain: []seriesHardcoverDiffBook{}, + Covered: []seriesHardcoverDiffBook{}, } // matchedCatalog doubles as the exclusion set for bestCatalogMatch: a // catalog entry a previous local book already claimed is off the table for @@ -1440,6 +1446,12 @@ func buildHardcoverDiff(ctx context.Context, books *db.BookRepo, userID int64, s // and pushed the second local's real catalog entry into Missing behind an // Add button that ensureHardcoverCatalogBook's guards then refuse. matchedCatalog := make(map[int]struct{}) + // presentWholes collects the subset of Present rows eligible to cover a + // split-edition part (#2524/#3048): not excluded, and imported or + // monitored, the same gate db.ListCoveredSplitEditionParts applies to + // stored rows, so the diff and that sweep never disagree about which + // wholes cover which parts. + presentWholes := make([]presentWhole, 0, len(series.Books)) // Identities bind first (#2553). Assigning in library order let an // earlier local's fuzzy title match claim the catalogue slot that a later // local owned by foreign ID: "He Who Fights with Monsters 4" took volume @@ -1534,6 +1546,14 @@ func buildHardcoverDiff(ctx context.Context, books *db.BookRepo, userID int64, s if match.score >= 90 || match.foreignID { diff.Present = append(diff.Present, item) matchedCatalog[match.index] = struct{}{} + if eligibleSplitEditionWhole(local.Book) { + presentWholes = append(presentWholes, presentWhole{ + position: item.Position, + title: item.Title, + localBookID: local.Book.ID, + localTitle: local.Book.Title, + }) + } } else if match.score >= 70 { diff.Uncertain = append(diff.Uncertain, item) matchedCatalog[match.index] = struct{}{} @@ -1553,6 +1573,14 @@ func buildHardcoverDiff(ctx context.Context, books *db.BookRepo, userID int64, s if title := firstNonEmpty(book.Book.Title, book.Title); metadata.IsUnambiguousBundleTitle(title) { continue } + if whole, ok := splitEditionWhole(book, presentWholes); ok { + item := catalogDiffBook(book, catalog.AuthorName) + id := whole.localBookID + item.LocalBookID = &id + item.LocalTitle = whole.localTitle + diff.Covered = append(diff.Covered, item) + continue + } item := catalogDiffBook(book, catalog.AuthorName) enrichMissingDiffBook(ctx, books, userID, book, &item) diff.Missing = append(diff.Missing, item) @@ -1562,6 +1590,67 @@ func buildHardcoverDiff(ctx context.Context, books *db.BookRepo, userID int64, s return diff } +// presentWhole is the subset of a Present row that splitEditionWhole needs: +// a catalogue diff row the library holds, eligible to cover a split-edition +// part (#2524/#3048). Kept separate from seriesHardcoverDiffBook so the +// eligibility gate (not excluded, imported or monitored) is decided once, +// where local.Book is still in scope, rather than re-derived from the API +// response shape. +type presentWhole struct { + position string + title string + localBookID int64 + localTitle string +} + +// eligibleSplitEditionWhole reports whether book is a book the user actually +// has or wants, so it is allowed to cover a split-edition part (#3048): not +// excluded, and imported or monitored. +// +// This MUST stay in agreement with the SQL predicate in +// db.ListCoveredSplitEditionParts (internal/db/split_edition_parts.go), +// which applies the identical rule to rows already stored. The two cannot +// share one literal implementation across the SQL/Go boundary, so an +// eligibility change here needs the same change made there, and vice versa, +// or the catalogue diff and the stored-row sweep will disagree about which +// wholes cover which parts. +func eligibleSplitEditionWhole(b *models.Book) bool { + return !b.Excluded && (b.Status == models.BookStatusImported || b.Monitored) +} + +// splitEditionWhole reports whether book is a split-edition part of some +// already-present, eligible whole work, and if so returns that whole. +// +// #2524/#3048: some Hardcover catalogues list a novel AND its split parts at +// fractional positions under the novel's own position — Stormlight's "The +// Way of Kings" at 1 alongside "…, Part 1" and "…, Part 2" at 1.1 and 1.2. +// Owning the whole novel should not leave its own parts reading as missing +// volumes, counting against the series, and sitting behind an Add button +// that would create and queue a download for text already on the shelf. +// +// The two-signal rule itself is seriesmatch.SplitEditionPartOf, shared with +// db.ListCoveredSplitEditionParts (#3048), which applies the same rule to +// rows already stored for an install that filled before this landed. A +// series represented ONLY by its split parts — Wheel of Time's "Two Volume +// Edition", where there is no unsplit whole at the integer position — never +// matches this, because there is no eligible whole to match against; its +// parts are the real volumes and stay Missing. +// +// wholes is pre-filtered to Present rows that are also eligible (not +// excluded, imported or monitored), so an excluded or purely-wanted, +// unmonitored Present row covers nothing — the same gate the stored-row +// sweep applies, so the two never disagree about which wholes cover which +// parts. +func splitEditionWhole(book metadata.SeriesCatalogBook, wholes []presentWhole) (presentWhole, bool) { + partTitle := firstNonEmpty(book.Book.Title, book.Title) + for _, whole := range wholes { + if seriesmatch.SplitEditionPartOf(partTitle, book.Position, whole.title, whole.position) { + return whole, true + } + } + return presentWhole{}, false +} + // logDiffDecision records which catalogue entry a local series book bound // to, and how. The diff matcher had no logging at all, so a wrong binding // could only be diagnosed by rebuilding the library in a scratch container diff --git a/internal/api/series_test.go b/internal/api/series_test.go index a20fb97f2..95c7a7932 100644 --- a/internal/api/series_test.go +++ b/internal/api/series_test.go @@ -1567,6 +1567,83 @@ func TestSeriesFillCreatesMissingHardcoverBook(t *testing.T) { } } +// TestSeriesFillSkipsOwnedSplitEditions is the write-path form of #2524: +// both createMissingHardcoverBooks (bulk Fill) and createMissingHardcoverBook +// (per-book Fill) iterate diff.Missing, so once buildHardcoverDiff moves a +// split-edition row to Covered instead, Fill stops creating — and so stops +// queuing a download for — a book already on the shelf. Every genuinely +// missing book in the same series must still be created. +func TestSeriesFillSkipsOwnedSplitEditions(t *testing.T) { + catalog := splitEditionCatalog() + searcher := newMockBookSearcher() + h, seriesRepo, authorRepo, bookRepo := seriesFixtureWithProvider(t, &stubSeriesProvider{ + catalogs: map[string]*metadata.SeriesCatalog{catalog.ForeignID: catalog}, + }, searcher) + ctx := context.Background() + + author := &models.Author{ForeignID: "hc:brandon-sanderson", Name: "Brandon Sanderson", SortName: "Sanderson, Brandon"} + if err := authorRepo.Create(ctx, author); err != nil { + t.Fatal(err) + } + owned := &models.Book{ + ForeignID: "hc:the-way-of-kings", + AuthorID: author.ID, + Title: "The Way of Kings", + SortTitle: "The Way of Kings", + Status: models.BookStatusImported, + Genres: []string{}, + } + if err := bookRepo.Create(ctx, owned); err != nil { + t.Fatal(err) + } + + series := &models.Series{ForeignID: "ol-series:stormlight", Title: "The Stormlight Archive"} + if err := seriesRepo.Create(ctx, series); err != nil { + t.Fatal(err) + } + if err := seriesRepo.LinkBook(ctx, series.ID, owned.ID, "1", true); err != nil { + t.Fatal(err) + } + link := &models.SeriesHardcoverLink{ + SeriesID: series.ID, + HardcoverSeriesID: catalog.ForeignID, + HardcoverProviderID: catalog.ProviderID, + HardcoverTitle: catalog.Title, + HardcoverAuthorName: catalog.AuthorName, + HardcoverBookCount: catalog.BookCount, + Confidence: 1, + LinkedBy: "manual", + } + if err := seriesRepo.UpsertHardcoverLink(ctx, link); err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + h.Fill(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/1/fill", nil), "id", strconv.FormatInt(series.ID, 10))) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + for _, foreignID := range []string{"hc:the-way-of-kings-part-1", "hc:the-way-of-kings-part-2"} { + created, err := bookRepo.GetByForeignID(ctx, foreignID) + if err != nil { + t.Fatal(err) + } + if created != nil { + t.Fatalf("split-edition part %q must not be created when the whole work is owned", foreignID) + } + } + for _, foreignID := range []string{"hc:words-of-radiance", "hc:edgedancer"} { + created, err := bookRepo.GetByForeignID(ctx, foreignID) + if err != nil { + t.Fatal(err) + } + if created == nil { + t.Fatalf("genuinely missing book %q should still be created by Fill", foreignID) + } + } +} + // TestSeriesFillHonoursRequestedMediaType covers #1124: the per-book "add" // selector must create the missing Hardcover book with the chosen media type // rather than always defaulting to ebook. @@ -2958,6 +3035,366 @@ func stormlightCatalog() *metadata.SeriesCatalog { } } +// splitEditionCatalog builds on stormlightCatalog with the real-data shape +// from the #2524 report: "The Way of Kings" at position 1 is split into its +// own "Part 1"/"Part 2" at 1.1/1.2, "Words of Radiance" sits at position 2, +// and "Edgedancer" — a real novella, not a split edition of anything — sits +// at the neighbouring fractional position 2.5. +func splitEditionCatalog() *metadata.SeriesCatalog { + catalog := stormlightCatalog() + author := catalog.Books[0].Book.Author + catalog.Books = append(catalog.Books, + metadata.SeriesCatalogBook{ + ForeignID: "hc:the-way-of-kings-part-1", + ProviderID: "101-1", + Title: "The Way of Kings, Part 1", + Position: "1.1", + Book: models.Book{ + ForeignID: "hc:the-way-of-kings-part-1", + Title: "The Way of Kings, Part 1", + Author: author, + }, + }, + metadata.SeriesCatalogBook{ + ForeignID: "hc:the-way-of-kings-part-2", + ProviderID: "101-2", + Title: "The Way of Kings, Part 2", + Position: "1.2", + Book: models.Book{ + ForeignID: "hc:the-way-of-kings-part-2", + Title: "The Way of Kings, Part 2", + Author: author, + }, + }, + metadata.SeriesCatalogBook{ + ForeignID: "hc:words-of-radiance", + ProviderID: "102", + Title: "Words of Radiance", + Position: "2", + Book: models.Book{ + ForeignID: "hc:words-of-radiance", + Title: "Words of Radiance", + Author: author, + }, + }, + metadata.SeriesCatalogBook{ + ForeignID: "hc:edgedancer", + ProviderID: "102-5", + Title: "Edgedancer", + Position: "2.5", + Book: models.Book{ + ForeignID: "hc:edgedancer", + Title: "Edgedancer", + Author: author, + }, + }, + ) + catalog.BookCount = len(catalog.Books) + return catalog +} + +// TestSplitEditionWhole unit-tests the #2524 suppression rule directly +// against crafted Present rows, independent of how a book came to be +// Present. That sidesteps engineering a fuzzy title score into the 70-89 +// Uncertain band: a whole work that is Uncertain or Missing simply never +// appears in the `present` slice this function is handed, which is exactly +// the real-world shape (buildHardcoverDiff only passes diff.Present). +// TestSplitEditionWhole unit-tests the matching loop directly against +// crafted presentWhole candidates, independent of how a candidate came to be +// eligible — that eligibility gate (not excluded, imported or monitored) is +// decided by the caller before a whole ever reaches this function (see the +// buildHardcoverDiff-level tests for the gate itself), so an absent whole +// here stands for either "no Present row at this position" (the Two Volume +// Edition trap) or "a Present row that failed the eligibility gate." +func TestSplitEditionWhole(t *testing.T) { + wholes := []presentWhole{{ + position: "1", + title: "The Way of Kings", + localBookID: 1, + localTitle: "The Way of Kings", + }} + + cases := []struct { + name string + book metadata.SeriesCatalogBook + wholes []presentWhole + wantOK bool + }{ + { + name: "first part of an owned whole", + book: metadata.SeriesCatalogBook{ + Position: "1.1", + Title: "The Way of Kings, Part 1", + Book: models.Book{Title: "The Way of Kings, Part 1"}, + }, + wholes: wholes, + wantOK: true, + }, + { + name: "second part of an owned whole", + book: metadata.SeriesCatalogBook{ + Position: "1.2", + Title: "The Way of Kings, Part 2", + Book: models.Book{Title: "The Way of Kings, Part 2"}, + }, + wholes: wholes, + wantOK: true, + }, + { + name: "novella at a fractional position is not a split edition", + book: metadata.SeriesCatalogBook{ + Position: "1.5", + Title: "Dawnshard", + Book: models.Book{Title: "Dawnshard"}, + }, + wholes: wholes, + wantOK: false, + }, + { + name: "whole number position never matches, even with a part title", + book: metadata.SeriesCatalogBook{ + Position: "2", + Title: "The Way of Kings, Part 1", + Book: models.Book{Title: "The Way of Kings, Part 1"}, + }, + wholes: wholes, + wantOK: false, + }, + { + name: "no eligible whole at all (Two Volume Edition shape, or one the caller already filtered out)", + book: metadata.SeriesCatalogBook{ + Position: "1.1", + Title: "The Eye of the World, Part 1 of 2", + Book: models.Book{Title: "The Eye of the World, Part 1 of 2"}, + }, + wholes: nil, + wantOK: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, ok := splitEditionWhole(tc.book, tc.wholes) + if ok != tc.wantOK { + t.Fatalf("splitEditionWhole() ok = %v, want %v (got %+v)", ok, tc.wantOK, got) + } + if ok && got.localBookID != 1 { + t.Fatalf("splitEditionWhole() local = %d, want 1", got.localBookID) + } + }) + } +} + +// TestBuildHardcoverDiffCoversSplitEditionsOfOwnedWork is the #2524 +// acceptance case from the issue thread: Stormlight's "The Way of Kings" is +// filed at position 1 alongside its own split parts at 1.1/1.2, and a real +// novella sits at the neighbouring fractional position 2.5 under an unowned +// "Words of Radiance". Owning the whole novel must not leave its own parts +// reading as missing volumes; the novella, owning nothing to its name, must +// not be swept up just because it shares the fractional-position shape. +func TestBuildHardcoverDiffCoversSplitEditionsOfOwnedWork(t *testing.T) { + catalog := splitEditionCatalog() + series := &models.Series{ + ID: 1, + Title: "The Stormlight Archive", + Books: []models.SeriesBook{ + localSeriesBook(1, "The Way of Kings"), + }, + } + link := &models.SeriesHardcoverLink{SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID} + + diff := buildHardcoverDiff(context.Background(), nil, 0, series, link, catalog) + + covered := map[string]seriesHardcoverDiffBook{} + for _, row := range diff.Covered { + covered[row.ForeignBookID] = row + } + if len(diff.Covered) != 2 { + t.Fatalf("Covered = %v, want exactly the two Way of Kings parts", diffForeignIDs(diff.Covered)) + } + for _, id := range []string{"hc:the-way-of-kings-part-1", "hc:the-way-of-kings-part-2"} { + row, ok := covered[id] + if !ok { + t.Fatalf("expected %q in Covered, got %v", id, diffForeignIDs(diff.Covered)) + } + if row.LocalBookID == nil || *row.LocalBookID != 1 { + t.Fatalf("%q LocalBookID = %v, want 1 (The Way of Kings)", id, row.LocalBookID) + } + } + + missing := map[string]bool{} + for _, row := range diff.Missing { + missing[row.ForeignBookID] = true + } + for _, id := range []string{"hc:the-way-of-kings-part-1", "hc:the-way-of-kings-part-2"} { + if missing[id] { + t.Fatalf("%q was covered but still counted in Missing: %v", id, diffForeignIDs(diff.Missing)) + } + } + if !missing["hc:words-of-radiance"] || !missing["hc:edgedancer"] { + t.Fatalf("Words of Radiance and the Edgedancer novella are genuinely missing, want both in Missing: %v", diffForeignIDs(diff.Missing)) + } + if diff.MissingCount != len(diff.Missing) { + t.Fatalf("missingCount %d disagrees with list length %d", diff.MissingCount, len(diff.Missing)) + } + if diff.MissingCount != 2 { + t.Fatalf("MissingCount = %d, want 2 (Words of Radiance, Edgedancer)", diff.MissingCount) + } +} + +// TestBuildHardcoverDiffSplitEditionsNeedAnOwnedWhole is the Wheel of Time +// "Two Volume Edition" trap from the #2524 thread: a catalogue can represent +// a work ONLY as its split parts, with no unsplit row at the integer +// position at all. There is nothing Present to cover the parts, so they +// must stay Missing — they are the real volumes here, not duplicates of one. +func TestBuildHardcoverDiffSplitEditionsNeedAnOwnedWhole(t *testing.T) { + author := &models.Author{ForeignID: "hc:robert-jordan", Name: "Robert Jordan"} + catalog := &metadata.SeriesCatalog{ + ForeignID: "hc-series:two-volume", + ProviderID: "7000", + Title: "The Wheel of Time (Two Volume Edition)", + AuthorName: "Robert Jordan", + Books: []metadata.SeriesCatalogBook{ + { + ForeignID: "hc:the-eye-of-the-world-part-1-of-2", + Title: "The Eye of the World, Part 1 of 2", + Position: "1.1", + Book: models.Book{ + ForeignID: "hc:the-eye-of-the-world-part-1-of-2", + Title: "The Eye of the World, Part 1 of 2", + Author: author, + }, + }, + { + ForeignID: "hc:the-eye-of-the-world-part-2-of-2", + Title: "The Eye of the World, Part 2 of 2", + Position: "1.2", + Book: models.Book{ + ForeignID: "hc:the-eye-of-the-world-part-2-of-2", + Title: "The Eye of the World, Part 2 of 2", + Author: author, + }, + }, + }, + } + catalog.BookCount = len(catalog.Books) + series := &models.Series{ID: 2, Title: "The Wheel of Time (Two Volume Edition)"} + link := &models.SeriesHardcoverLink{SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID} + + diff := buildHardcoverDiff(context.Background(), nil, 0, series, link, catalog) + + if len(diff.Covered) != 0 { + t.Fatalf("Covered = %v, want empty: there is no unsplit whole for these parts to cover", diffForeignIDs(diff.Covered)) + } + if len(diff.Missing) != 2 || diff.MissingCount != 2 { + t.Fatalf("Missing = %v (count %d), want both parts (they are the real volumes here)", diffForeignIDs(diff.Missing), diff.MissingCount) + } +} + +// TestBuildHardcoverDiffExcludedWholeDoesNotCoverParts is the #3109 review +// follow-up: db.ListCoveredSplitEditionParts only lets a not-excluded whole +// cover a stored part, and the catalogue diff has to apply the same gate so +// the two never disagree about which wholes cover which parts. An excluded +// Present row — the user said "not this edition" — covers nothing; its parts +// fall back to Missing like any other unclaimed catalogue row. +func TestBuildHardcoverDiffExcludedWholeDoesNotCoverParts(t *testing.T) { + catalog := splitEditionCatalog() + series := &models.Series{ + ID: 1, + Title: "The Stormlight Archive", + Books: []models.SeriesBook{{ + BookID: 1, + Book: &models.Book{ + ID: 1, + ForeignID: "calibre:book:1", + Title: "The Way of Kings", + SortTitle: "The Way of Kings", + Status: models.BookStatusImported, + Excluded: true, + }, + }}, + } + link := &models.SeriesHardcoverLink{SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID} + + diff := buildHardcoverDiff(context.Background(), nil, 0, series, link, catalog) + + if len(diff.Covered) != 0 { + t.Fatalf("Covered = %v, want empty: an excluded whole must not cover its parts", diffForeignIDs(diff.Covered)) + } + missing := map[string]bool{} + for _, row := range diff.Missing { + missing[row.ForeignBookID] = true + } + for _, id := range []string{"hc:the-way-of-kings-part-1", "hc:the-way-of-kings-part-2"} { + if !missing[id] { + t.Fatalf("%q should fall back to Missing when its whole is excluded: %v", id, diffForeignIDs(diff.Missing)) + } + } +} + +// TestBuildHardcoverDiffUnmonitoredWantedWholeDoesNotCoverParts pins the +// other half of the #3109 gate: a Present whole that is neither imported nor +// monitored (Wanted and unmonitored, perhaps added and then turned off) is +// not a book the user actually has or wants, so it must not cover parts +// either. TestBuildHardcoverDiffCoversSplitEditionsOfOwnedWork already pins +// the imported case; this and the next test pin both halves of the "or +// monitored" branch. +func TestBuildHardcoverDiffUnmonitoredWantedWholeDoesNotCoverParts(t *testing.T) { + catalog := splitEditionCatalog() + series := &models.Series{ + ID: 1, + Title: "The Stormlight Archive", + Books: []models.SeriesBook{{ + BookID: 1, + Book: &models.Book{ + ID: 1, + ForeignID: "calibre:book:1", + Title: "The Way of Kings", + SortTitle: "The Way of Kings", + Status: models.BookStatusWanted, + Monitored: false, + }, + }}, + } + link := &models.SeriesHardcoverLink{SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID} + + diff := buildHardcoverDiff(context.Background(), nil, 0, series, link, catalog) + + if len(diff.Covered) != 0 { + t.Fatalf("Covered = %v, want empty: a wanted-but-unmonitored whole must not cover its parts", diffForeignIDs(diff.Covered)) + } +} + +// TestBuildHardcoverDiffMonitoredWantedWholeCoversParts is the positive half +// of the "or monitored" branch: a Present whole that is Wanted but monitored +// — the user asked Bindery to go find it — is exactly the #3048 wiki case +// ("monitored and wanted"), and does cover its parts even though it is not +// yet imported. +func TestBuildHardcoverDiffMonitoredWantedWholeCoversParts(t *testing.T) { + catalog := splitEditionCatalog() + series := &models.Series{ + ID: 1, + Title: "The Stormlight Archive", + Books: []models.SeriesBook{{ + BookID: 1, + Book: &models.Book{ + ID: 1, + ForeignID: "calibre:book:1", + Title: "The Way of Kings", + SortTitle: "The Way of Kings", + Status: models.BookStatusWanted, + Monitored: true, + }, + }}, + } + link := &models.SeriesHardcoverLink{SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID} + + diff := buildHardcoverDiff(context.Background(), nil, 0, series, link, catalog) + + if len(diff.Covered) != 2 { + t.Fatalf("Covered = %v, want exactly the two Way of Kings parts (whole is wanted and monitored)", diffForeignIDs(diff.Covered)) + } +} + // TestSeriesHandler_LifetimeCtxFallsBackToBackground is the #846 follow-up // guard for fanOutSeriesSearches. Same contract as BookHandler.bgCtx(). func TestSeriesHandler_LifetimeCtxFallsBackToBackground(t *testing.T) { diff --git a/internal/db/split_edition_parts.go b/internal/db/split_edition_parts.go index c99e963d2..a4c97941b 100644 --- a/internal/db/split_edition_parts.go +++ b/internal/db/split_edition_parts.go @@ -35,8 +35,11 @@ type SplitEditionPart struct { // Only a part that is not imported is reported: an imported part is already on // disk and there is nothing to stop searching for. The whole has to be a book // the user actually has or wants, so it must be imported or monitored, and not -// excluded. Both rows must belong to the same owner, so one user's library -// never decides what another user's sweep searches for. +// excluded — the WHERE clause below and api.eligibleSplitEditionWhole +// (internal/api/series.go) apply the identical rule to the catalogue diff; +// an eligibility change here needs the same change made there. Both rows +// must belong to the same owner, so one user's library never decides what +// another user's sweep searches for. func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID int64) ([]SplitEditionPart, error) { query := ` SELECT sp.series_id, sp.book_id, sp.position_in_series, bp.title, bp.monitored, diff --git a/internal/importer/volume.go b/internal/importer/volume.go index 9a9437122..e25751833 100644 --- a/internal/importer/volume.go +++ b/internal/importer/volume.go @@ -2,15 +2,10 @@ package importer import ( "regexp" - "strings" "github.com/vavallee/bindery/internal/seriesmatch" ) -// partMarkerRe matches a "Part N" marker, with an optional "of M": "Part 1", -// "Pt. 2", "Part 1 of 3". -var partMarkerRe = regexp.MustCompile(`(?i)\b(?:part|pt)\.?\s*\d+(?:\.\d+)?(?:\s+of\s+\d+)?\b`) - // volumeTitles returns a and b ready for a volume-number comparison. // // seriesmatch.VolumeNumber counts "Part N" as a volume marker, which is right @@ -22,22 +17,21 @@ var partMarkerRe = regexp.MustCompile(`(?i)\b(?:part|pt)\.?\s*\d+(?:\.\d+)?(?:\s // (The Stormlight Archive, Book 4)", the "Part 1" read as volume 1 and vetoed // the book's own files (#2810 review). So a Part marker on one side only is // dropped before the comparison, and kept when both sides carry one. +// +// partMarkerRe/stripPartMarker moved to seriesmatch.HasPartMarker / +// seriesmatch.StripPartMarker (#2524), which also needs them to recognize a +// split-edition catalogue row — one regex, one package, instead of two. func volumeTitles(a, b string) (string, string) { - ap, bp := partMarkerRe.MatchString(a), partMarkerRe.MatchString(b) + ap, bp := seriesmatch.HasPartMarker(a), seriesmatch.HasPartMarker(b) switch { case ap && !bp: - a = stripPartMarker(a) + a = seriesmatch.StripPartMarker(a) case bp && !ap: - b = stripPartMarker(b) + b = seriesmatch.StripPartMarker(b) } return a, b } -func stripPartMarker(s string) string { - s = partMarkerRe.ReplaceAllString(s, " ") - return strings.TrimRight(strings.TrimSpace(s), " ,.:;-_") -} - // differentVolumes is volumesDisagree after volumeTitles: true only when both // titles carry a volume number and the numbers disagree. func differentVolumes(a, b string) bool { diff --git a/internal/seriesmatch/match.go b/internal/seriesmatch/match.go index e462a48c4..8b33a989e 100644 --- a/internal/seriesmatch/match.go +++ b/internal/seriesmatch/match.go @@ -275,6 +275,21 @@ func CleanTitle(title string) string { return strings.Join(out, " ") } +// HasPartMarker reports whether s carries an explicit "Part N" / "Pt. N" +// marker. Shares splitEditionMarkerRe with SplitEditionPartOf (#3048, #2524) +// rather than keeping a second copy of the same pattern. +func HasPartMarker(s string) bool { + return splitEditionMarkerRe.MatchString(s) +} + +// StripPartMarker removes a "Part N" marker from s and cleans up the +// whitespace and punctuation left behind: "The Way of Kings, Part 1" -> +// "The Way of Kings". +func StripPartMarker(s string) string { + s = splitEditionMarkerRe.ReplaceAllString(s, " ") + return strings.TrimRight(strings.TrimSpace(s), " ,.:;-_") +} + // volumeNumberRe matches an EXPLICIT volume marker followed by a number: // "Vol. 3", "Volume 3", "Book 3", "Part 3", "#3". A bare trailing number is // deliberately not matched — "Fahrenheit 451" and "Catch 22" are titles, not diff --git a/internal/seriesmatch/match_test.go b/internal/seriesmatch/match_test.go index 053c5ee2c..53969acb7 100644 --- a/internal/seriesmatch/match_test.go +++ b/internal/seriesmatch/match_test.go @@ -285,3 +285,51 @@ func TestDifferentVolumesCatchesBareTrailingNumbers(t *testing.T) { }) } } + +// TestHasPartMarker and TestStripPartMarker exercise the two helpers +// directly: moving them here from internal/importer (#2524, so series.go +// could use them too) left them reached only transitively through +// internal/importer's own tests, which a per-package coverage profile does +// not credit to this package. +func TestHasPartMarker(t *testing.T) { + cases := []struct { + name string + in string + want bool + }{ + {name: "part", in: "The Way of Kings, Part 1", want: true}, + {name: "pt abbreviation", in: "The Way of Kings, Pt. 2", want: true}, + {name: "part of", in: "The Great Hunt, Part 2 of 2", want: true}, + {name: "no marker", in: "The Way of Kings", want: false}, + {name: "blank", in: "", want: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := HasPartMarker(tc.in); got != tc.want { + t.Fatalf("HasPartMarker(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +func TestStripPartMarker(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {name: "trailing comma and marker", in: "The Way of Kings, Part 1", want: "The Way of Kings"}, + {name: "part of", in: "The Great Hunt, Part 2 of 2", want: "The Great Hunt"}, + {name: "no marker is unchanged", in: "The Way of Kings", want: "The Way of Kings"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := StripPartMarker(tc.in); got != tc.want { + t.Fatalf("StripPartMarker(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// The #2524 fractional-position and title-prefix rules are now +// seriesmatch.SplitEditionPartOf (#3048), tested in split_edition_test.go. diff --git a/web/src/api/series.ts b/web/src/api/series.ts index 6431cbcb5..802f2aee5 100644 --- a/web/src/api/series.ts +++ b/web/src/api/series.ts @@ -70,6 +70,11 @@ export interface SeriesHardcoverDiff { missing: SeriesHardcoverDiffBook[] localOnly: SeriesHardcoverDiffBook[] uncertain: SeriesHardcoverDiffBook[] + // covered is a split edition of a book already in Present (#2524) — e.g. a + // "Part 1"/"Part 2" catalogue row for a novel you already own whole. Its + // localBookId/localTitle point at that whole work, not at a row of its own. + // Never counted in missingCount, never offered an add button. + covered: SeriesHardcoverDiffBook[] presentCount: number missingCount: number } diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 758490c8c..2bd495edb 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -872,7 +872,10 @@ "add": "add", "addTitle": "Add this missing Hardcover book and search indexers", "moreMissing_one": "{{count}} more missing book", - "moreMissing_other": "{{count}} more missing books" + "moreMissing_other": "{{count}} more missing books", + "coveredSection_one": "{{count}} split edition of a book you own", + "coveredSection_other": "{{count}} split editions of books you own", + "coveredSubtitle": "Part of {{title}}, already in your library" }, "nameModal": { "name": "Name", diff --git a/web/src/pages/SeriesPage.test.tsx b/web/src/pages/SeriesPage.test.tsx index 744d1b150..e6cdded76 100644 --- a/web/src/pages/SeriesPage.test.tsx +++ b/web/src/pages/SeriesPage.test.tsx @@ -433,6 +433,7 @@ describe('SeriesPage', () => { ], localOnly: [], uncertain: [], + covered: [], presentCount: 2, missingCount: 1, }) @@ -818,6 +819,7 @@ describe('SeriesPage', () => { ], localOnly: [], uncertain: [], + covered: [], presentCount: 0, missingCount: 1, }) @@ -879,6 +881,7 @@ describe('SeriesPage', () => { ], localOnly: [], uncertain: [], + covered: [], presentCount: 0, missingCount: 1, }) @@ -952,6 +955,7 @@ describe('SeriesPage', () => { ], localOnly: [], uncertain: [], + covered: [], presentCount: 0, missingCount: 1, }) @@ -966,6 +970,85 @@ describe('SeriesPage', () => { expect(row).not.toBeInTheDocument() }) + // #2524: a split-edition catalogue row is reported under `covered`, not + // `missing`, so it must render in its own collapsed section, link to the + // whole work that covers it, and never show an add button. + it('shows a covered split-edition row in its own section, linked to the owned whole', async () => { + const hardcoverLink = { + id: 1, + seriesId: 43, + hardcoverSeriesId: 'hc-series:42', + hardcoverProviderId: '42', + hardcoverTitle: 'The Stormlight Archive', + hardcoverAuthorName: 'Brandon Sanderson', + hardcoverBookCount: 3, + confidence: 1, + linkedBy: 'manual', + linkedAt: '2026-01-01T00:00:00Z', + createdAt: '2026-01-01T00:00:00Z', + updatedAt: '2026-01-01T00:00:00Z', + } + const series: Series = { + id: 43, + foreignSeriesId: 'series-43', + title: 'The Stormlight Archive', + description: '', + monitored: true, + books: [], + hardcoverLink, + } + vi.mocked(api.getSeriesHardcoverDiff).mockResolvedValue({ + seriesId: 43, + link: hardcoverLink, + present: [ + { + foreignBookId: 'hc:the-way-of-kings', + providerId: '101', + title: 'The Way of Kings', + position: '1', + authorName: 'Brandon Sanderson', + localBookId: 7, + }, + ], + missing: [], + localOnly: [], + uncertain: [], + covered: [ + { + foreignBookId: 'hc:the-way-of-kings-part-1', + providerId: '101-1', + title: 'The Way of Kings, Part 1', + position: '1.1', + authorName: 'Brandon Sanderson', + localBookId: 7, + localTitle: 'The Way of Kings', + }, + { + foreignBookId: 'hc:the-way-of-kings-part-2', + providerId: '101-2', + title: 'The Way of Kings, Part 2', + position: '1.2', + authorName: 'Brandon Sanderson', + localBookId: 7, + localTitle: 'The Way of Kings', + }, + ], + presentCount: 1, + missingCount: 0, + }) + + renderSeriesPage([series]) + + fireEvent.click(await screen.findByRole('heading', { name: 'The Stormlight Archive' })) + // Plural form (i18next _other), not a hand-rolled "s" suffix. + fireEvent.click(await screen.findByText('2 split editions of books you own')) + + const partLink = await screen.findByRole('link', { name: /The Way of Kings, Part 1/ }) + expect(partLink).toHaveAttribute('href', '/book/7') + expect(within(partLink).queryByRole('button', { name: 'add' })).not.toBeInTheDocument() + expect(within(partLink).getByText('Part of The Way of Kings, already in your library')).toBeInTheDocument() + }) + it('keeps a Hardcover missing row without a library match non-clickable', async () => { const hardcoverLink = { id: 1, @@ -1005,6 +1088,7 @@ describe('SeriesPage', () => { ], localOnly: [], uncertain: [], + covered: [], presentCount: 0, missingCount: 1, }) @@ -1054,6 +1138,7 @@ describe('SeriesPage Hardcover links out (#1708)', () => { missing: [], localOnly: [], uncertain: [], + covered: [], presentCount: 0, missingCount: 0, } diff --git a/web/src/pages/SeriesPage.tsx b/web/src/pages/SeriesPage.tsx index 29ee1c2b5..ffd193297 100644 --- a/web/src/pages/SeriesPage.tsx +++ b/web/src/pages/SeriesPage.tsx @@ -742,6 +742,38 @@ export default function SeriesPage() { )} )} + {/* #2524: a split edition (e.g. "Part 1"/"Part 2") of a book + already present is not a missing volume, so it is never + counted or offered an add button — just linked to the + whole work that covers it. Collapsed by default since + this is the exception, not the thing a user came to + check. */} + {diff && diff.covered.length > 0 && ( +
+ + {t('series.hardcover.coveredSection', { count: diff.covered.length })} + +
+ {diff.covered.map(book => ( + + + #{book.position || '?'} + +
+

{book.title}

+

+ {t('series.hardcover.coveredSubtitle', { title: book.localTitle })} +

+
+ + ))} +
+
+ )} )}