From 3f1c8ea2542a81b04e6f01f9437c5e642bd10868 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 7 Oct 2026 23:29:01 -0300 Subject: [PATCH 1/3] 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 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- cmd/bindery/authz_gate_routes_test.go | 32 +++ cmd/bindery/main.go | 4 +- cmd/bindery/requester_routes_test.go | 5 + cmd/bindery/sensitive_routes.go | 19 ++ cmd/bindery/series_routes.go | 2 + cmd/bindery/series_routes_test.go | 5 + internal/api/metadata_profiles.go | 3 +- internal/api/metadata_profiles_library.go | 135 +++++++++++ .../api/metadata_profiles_library_test.go | 144 +++++++++++ internal/api/profile_filters.go | 129 ++++++++++ internal/api/series.go | 227 ++++++++++++++++-- internal/api/series_profile_filters_test.go | 180 ++++++++++++++ internal/api/series_split_edition_test.go | 202 ++++++++++++++++ internal/db/books_metadata_profile.go | 21 ++ internal/db/split_edition_parts.go | 112 +++++++++ internal/models/series.go | 4 + internal/scheduler/scheduler.go | 26 ++ .../scheduler/scheduler_split_edition_test.go | 59 +++++ internal/seriesmatch/split_edition.go | 62 +++++ internal/seriesmatch/split_edition_test.go | 32 +++ 20 files changed, 1385 insertions(+), 18 deletions(-) create mode 100644 internal/api/metadata_profiles_library.go create mode 100644 internal/api/metadata_profiles_library_test.go create mode 100644 internal/api/profile_filters.go create mode 100644 internal/api/series_profile_filters_test.go create mode 100644 internal/api/series_split_edition_test.go create mode 100644 internal/db/books_metadata_profile.go create mode 100644 internal/db/split_edition_parts.go create mode 100644 internal/scheduler/scheduler_split_edition_test.go create mode 100644 internal/seriesmatch/split_edition.go create mode 100644 internal/seriesmatch/split_edition_test.go diff --git a/cmd/bindery/authz_gate_routes_test.go b/cmd/bindery/authz_gate_routes_test.go index fa1f10982..329f46884 100644 --- a/cmd/bindery/authz_gate_routes_test.go +++ b/cmd/bindery/authz_gate_routes_test.go @@ -37,6 +37,38 @@ func (h *stubAuthzGateHandler) Delete(w http.ResponseWriter, _ *http.Request) { func (h *stubAuthzGateHandler) BulkDelete(w http.ResponseWriter, _ *http.Request) { h.record("bulk-delete", w) } +func (h *stubAuthzGateHandler) FilteredBooks(w http.ResponseWriter, _ *http.Request) { + h.record("filtered-books", w) +} +func (h *stubAuthzGateHandler) UnmonitorFilteredBooks(w http.ResponseWriter, _ *http.Request) { + h.record("unmonitor-filtered-books", w) +} + +// metadataProfileLibraryRoutes apply a profile to books already stored +// (#2208). The preview lists wanted books across every library and the +// action changes them, so both are admin only. +var metadataProfileLibraryRoutes = []authzGateRoute{ + {name: "preview filtered books", method: http.MethodGet, path: "/metadataprofile/3/filtered-books", called: "filtered-books"}, + {name: "unmonitor filtered books", method: http.MethodPost, path: "/metadataprofile/3/filtered-books/unmonitor", called: "unmonitor-filtered-books"}, +} + +func TestMetadataProfileLibraryRoutesRequireAdmin(t *testing.T) { + register := func(r chi.Router, h *stubAuthzGateHandler) { registerMetadataProfileLibraryRoutes(r, h) } + for _, rt := range metadataProfileLibraryRoutes { + t.Run("user "+rt.name, func(t *testing.T) { + rec, h := serveAuthzGateRoute(t, register, rt, "user") + if rec.Code != http.StatusForbidden || len(h.called) != 0 { + t.Fatalf("status = %d called = %v; want 403 and no handler call", rec.Code, h.called) + } + }) + t.Run("admin "+rt.name, func(t *testing.T) { + rec, h := serveAuthzGateRoute(t, register, rt, "admin") + if rec.Code != http.StatusNoContent || len(h.called) != 1 || h.called[0] != rt.called { + t.Fatalf("status = %d called = %v; want 204 [%s]", rec.Code, h.called, rt.called) + } + }) + } +} type authzGateRoute struct { name string diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index cfc2c51b6..84af3ecd0 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -788,9 +788,10 @@ func main() { WithHardcoverFeatureSettings(settingsRepo, cfg.EnhancedHardcoverAPI). WithFinder(importScanner). WithEditionHydration(editionRepo). + WithMetadataProfiles(metadataProfileRepo). WithLifetimeCtx(appCtx) importListHandler := api.NewImportListHandler(importListRepo, settingsRepo, hcSyncer, userRepo) - metadataProfileHandler := api.NewMetadataProfileHandler(metadataProfileRepo) + metadataProfileHandler := api.NewMetadataProfileHandler(metadataProfileRepo).WithLibrary(bookRepo) delayProfileHandler := api.NewDelayProfileHandler(delayProfileRepo) customFormatHandler := api.NewCustomFormatHandler(customFormatRepo) bulkHandler := api.NewBulkHandler(authorRepo, bookRepo, blocklistRepo, sched). @@ -1209,6 +1210,7 @@ func main() { // Metadata profiles: reads open, writes admin only (see // registerMetadataProfileRoutes). registerMetadataProfileRoutes(r, metadataProfileHandler) + registerMetadataProfileLibraryRoutes(r, metadataProfileHandler) // Backups — Restore replaces the live database (staged now, swapped // in by db.ApplyPendingRestore at the next start), Delete removes diff --git a/cmd/bindery/requester_routes_test.go b/cmd/bindery/requester_routes_test.go index 654dba2b0..048a917f0 100644 --- a/cmd/bindery/requester_routes_test.go +++ b/cmd/bindery/requester_routes_test.go @@ -486,3 +486,8 @@ func TestRequesterGuard_OPDSModeNeverElevatesARequester(t *testing.T) { } } } + +// UnmonitorSplitEditionParts completes the series route surface (#3048). +func (s universalStub) UnmonitorSplitEditionParts(w http.ResponseWriter, _ *http.Request) { + s.h(w) +} diff --git a/cmd/bindery/sensitive_routes.go b/cmd/bindery/sensitive_routes.go index 88b4f3d6f..6279b83f1 100644 --- a/cmd/bindery/sensitive_routes.go +++ b/cmd/bindery/sensitive_routes.go @@ -323,3 +323,22 @@ func registerMetadataProfileRoutes(r chi.Router, h profileRouteHandler) { r.Delete("/metadataprofile/{id}", h.Delete) }) } + +// metadataProfileLibraryRouteHandler is the surface +// registerMetadataProfileLibraryRoutes needs. +type metadataProfileLibraryRouteHandler interface { + FilteredBooks(http.ResponseWriter, *http.Request) + UnmonitorFilteredBooks(http.ResponseWriter, *http.Request) +} + +// registerMetadataProfileLibraryRoutes mounts the routes that apply a metadata +// profile's filters to books already in the library (#2208). Both are admin +// only: the preview lists wanted books across every user's library, and the +// action changes them. +func registerMetadataProfileLibraryRoutes(r chi.Router, h metadataProfileLibraryRouteHandler) { + r.Group(func(r chi.Router) { + r.Use(auth.RequireAdmin) + r.Get("/metadataprofile/{id}/filtered-books", h.FilteredBooks) + r.Post("/metadataprofile/{id}/filtered-books/unmonitor", h.UnmonitorFilteredBooks) + }) +} diff --git a/cmd/bindery/series_routes.go b/cmd/bindery/series_routes.go index aa923a82f..c9265fa86 100644 --- a/cmd/bindery/series_routes.go +++ b/cmd/bindery/series_routes.go @@ -28,6 +28,7 @@ type seriesRouteHandler interface { PutHardcoverLink(http.ResponseWriter, *http.Request) DeleteHardcoverLink(http.ResponseWriter, *http.Request) HardcoverDiff(http.ResponseWriter, *http.Request) + UnmonitorSplitEditionParts(http.ResponseWriter, *http.Request) } func registerSeriesRoutes(r chi.Router, seriesHandler seriesRouteHandler) { @@ -48,6 +49,7 @@ func registerSeriesRoutes(r chi.Router, seriesHandler seriesRouteHandler) { r.Delete("/series/{id}/books/{bookId}", seriesHandler.RemoveBook) r.Put("/series/{id}/books/{bookId}/primary", seriesHandler.SetPrimaryBook) r.Post("/series/{id}/fill", seriesHandler.Fill) + r.Post("/series/{id}/split-parts/unmonitor", seriesHandler.UnmonitorSplitEditionParts) r.Put("/series/{id}/genres", seriesHandler.ApplyGenres) r.Delete("/series/{id}/genres", seriesHandler.ClearGenres) r.Post("/series/{id}/hardcover-link/auto", seriesHandler.AutoLinkHardcover) diff --git a/cmd/bindery/series_routes_test.go b/cmd/bindery/series_routes_test.go index d50e49498..6c29a8b50 100644 --- a/cmd/bindery/series_routes_test.go +++ b/cmd/bindery/series_routes_test.go @@ -95,6 +95,10 @@ func (h *stubSeriesRouteHandler) HardcoverDiff(w http.ResponseWriter, _ *http.Re h.record("hardcover-diff", w) } +func (h *stubSeriesRouteHandler) UnmonitorSplitEditionParts(w http.ResponseWriter, _ *http.Request) { + h.record("unmonitor-split-edition-parts", w) +} + func TestSeriesMutationRoutesRequireAdmin(t *testing.T) { tests := []struct { name string @@ -110,6 +114,7 @@ func TestSeriesMutationRoutesRequireAdmin(t *testing.T) { {name: "remove book", method: http.MethodDelete, path: "/series/1/books/2"}, {name: "set primary book", method: http.MethodPut, path: "/series/1/books/2/primary"}, {name: "fill series", method: http.MethodPost, path: "/series/1/fill"}, + {name: "unmonitor split edition parts", method: http.MethodPost, path: "/series/1/split-parts/unmonitor"}, {name: "auto link hardcover", method: http.MethodPost, path: "/series/1/hardcover-link/auto"}, {name: "put hardcover link", method: http.MethodPut, path: "/series/1/hardcover-link"}, {name: "delete hardcover link", method: http.MethodDelete, path: "/series/1/hardcover-link"}, diff --git a/internal/api/metadata_profiles.go b/internal/api/metadata_profiles.go index 788437700..5691bee90 100644 --- a/internal/api/metadata_profiles.go +++ b/internal/api/metadata_profiles.go @@ -13,7 +13,8 @@ import ( ) type MetadataProfileHandler struct { - repo *db.MetadataProfileRepo + repo *db.MetadataProfileRepo + books *db.BookRepo } func NewMetadataProfileHandler(repo *db.MetadataProfileRepo) *MetadataProfileHandler { diff --git a/internal/api/metadata_profiles_library.go b/internal/api/metadata_profiles_library.go new file mode 100644 index 000000000..260ed5e27 --- /dev/null +++ b/internal/api/metadata_profiles_library.go @@ -0,0 +1,135 @@ +package api + +import ( + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "strconv" + + "github.com/go-chi/chi/v5" + + "github.com/vavallee/bindery/internal/db" +) + +// WithLibrary attaches the book repo the filtered books endpoints read and +// unmonitor (#2208). Without it they answer 503. +func (h *MetadataProfileHandler) WithLibrary(books *db.BookRepo) *MetadataProfileHandler { + h.books = books + return h +} + +// profileFilteredBook is one library book the profile's filters would have +// kept out had they been on when the book was added. +type profileFilteredBook struct { + BookID int64 `json:"bookId"` + Title string `json:"title"` + AuthorID int64 `json:"authorId"` + AuthorName string `json:"authorName,omitempty"` + Reason string `json:"reason"` +} + +// filteredBooks loads the profile and the wanted, monitored books it governs +// that its stored row filters reject. It writes the error response itself +// and returns ok=false when the caller should stop. +func (h *MetadataProfileHandler) filteredBooks(w http.ResponseWriter, r *http.Request) ([]profileFilteredBook, bool) { + if h.books == nil { + writeJSON(w, http.StatusServiceUnavailable, map[string]string{"error": "library is not available"}) + return nil, false + } + id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid id"}) + return nil, false + } + p, err := h.repo.GetByID(r.Context(), id) + if err != nil { + writeServerError(w, r, err) + return nil, false + } + if p == nil { + writeJSON(w, http.StatusNotFound, map[string]string{"error": "metadata profile not found"}) + return nil, false + } + books, err := h.books.ListWantedMonitoredByMetadataProfile(r.Context(), id) + if err != nil { + writeServerError(w, r, err) + return nil, false + } + out := make([]profileFilteredBook, 0) + for i := range books { + b := &books[i] + reason := storedBookProfileFilter(p, b) + if reason == "" { + continue + } + item := profileFilteredBook{BookID: b.ID, Title: b.Title, AuthorID: b.AuthorID, Reason: reason} + if b.Author != nil { + item.AuthorName = b.Author.Name + } + out = append(out, item) + } + return out, true +} + +// FilteredBooks previews the wanted, monitored books already in the library +// that this profile's filters screen out (#2208). The filters only screen new +// books during a sync, so turning one on never touched what was already +// there. Only the filters a stored row can be judged by are applied: +// SkipPartBooks and SkipMissingDate (see storedBookProfileFilter). Nothing +// changes here; UnmonitorFilteredBooks is the action. +func (h *MetadataProfileHandler) FilteredBooks(w http.ResponseWriter, r *http.Request) { + books, ok := h.filteredBooks(w, r) + if !ok { + return + } + writeJSON(w, http.StatusOK, map[string]any{"books": books, "total": len(books)}) +} + +// UnmonitorFilteredBooks unmonitors books from the FilteredBooks preview. The +// body may name bookIds to act on a subset; without it every previewed book +// is unmonitored. Ids are checked against a fresh preview, so a stale or +// hand written list can never unmonitor a book the filters do not reject. +// Nothing is deleted and imported books are never touched: an unmonitored +// book stays in the library, leaves Wanted, and can be monitored again from +// its page. +func (h *MetadataProfileHandler) UnmonitorFilteredBooks(w http.ResponseWriter, r *http.Request) { + var body struct { + BookIDs []int64 `json:"bookIds"` + } + if r.Body != nil { + if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid request body"}) + return + } + } + books, ok := h.filteredBooks(w, r) + if !ok { + return + } + var want map[int64]struct{} + if len(body.BookIDs) > 0 { + want = make(map[int64]struct{}, len(body.BookIDs)) + for _, id := range body.BookIDs { + want[id] = struct{}{} + } + } + ids := make([]int64, 0, len(books)) + for _, b := range books { + if want != nil { + if _, ok := want[b.BookID]; !ok { + continue + } + } + ids = append(ids, b.BookID) + } + n, err := h.books.UnmonitorNotImported(r.Context(), ids) + if err != nil { + writeServerError(w, r, err) + return + } + slog.Info("metadata profile: unmonitored books its filters screen out", + "profileID", chi.URLParam(r, "id"), "unmonitored", n) + writeJSON(w, http.StatusOK, map[string]int64{"unmonitored": n}) +} diff --git a/internal/api/metadata_profiles_library_test.go b/internal/api/metadata_profiles_library_test.go new file mode 100644 index 000000000..88d2dafa5 --- /dev/null +++ b/internal/api/metadata_profiles_library_test.go @@ -0,0 +1,144 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// TestMetadataProfileFilteredBooks is the #2208 retroactive test. Every +// profile filter is gated on the book being new, so turning SkipPartBooks or +// SkipMissingDate on never touched the box sets and undated works already on +// Wanted. The preview lists exactly those, and the action unmonitors them +// without deleting anything or touching an imported book, a book whose author +// uses another profile, or a book the filters let through. +func TestMetadataProfileFilteredBooks(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + profiles := db.NewMetadataProfileRepo(database) + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + h := NewMetadataProfileHandler(profiles).WithLibrary(books) + + p, err := profiles.GetByID(ctx, models.DefaultMetadataProfileID) + if err != nil || p == nil { + t.Fatalf("default profile: %v %v", p, err) + } + p.SkipPartBooks = true + p.SkipMissingDate = true + if err := profiles.Update(ctx, p); err != nil { + t.Fatal(err) + } + other := &models.MetadataProfile{Name: "Anything", AllowedLanguages: "any"} + if err := profiles.Create(ctx, other); err != nil { + t.Fatal(err) + } + + newAuthor := func(name string, profileID *int64) *models.Author { + t.Helper() + a := &models.Author{ForeignID: "ol:" + name, Name: name, SortName: name, MetadataProvider: "openlibrary", MetadataProfileID: profileID} + if err := authors.Create(ctx, a); err != nil { + t.Fatal(err) + } + return a + } + corey := newAuthor("James S. A. Corey", nil) + otherAuthor := newAuthor("Other Author", &other.ID) + released := time.Date(2011, 6, 15, 0, 0, 0, 0, time.UTC) + newBook := func(a *models.Author, title, status string, monitored, dated bool) *models.Book { + t.Helper() + b := &models.Book{ + ForeignID: "ol:" + title, AuthorID: a.ID, Title: title, SortTitle: title, + Status: status, Monitored: monitored, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "openlibrary", + } + if dated { + b.ReleaseDate = &released + } + if err := books.Create(ctx, b); err != nil { + t.Fatal(err) + } + return b + } + boxSet := newBook(corey, "The Expanse Books 1-3", models.BookStatusWanted, true, true) + undated := newBook(corey, "Caliban's War", models.BookStatusWanted, true, false) + kept := newBook(corey, "Leviathan Wakes", models.BookStatusWanted, true, true) + ownedBoxSet := newBook(corey, "The Expanse Books 4-6", models.BookStatusImported, true, true) + otherBoxSet := newBook(otherAuthor, "Other Books 1-3", models.BookStatusWanted, true, true) + + id := strconv.FormatInt(models.DefaultMetadataProfileID, 10) + rec := httptest.NewRecorder() + h.FilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/metadataprofile/1/filtered-books", nil), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("preview: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var preview struct { + Books []profileFilteredBook `json:"books"` + Total int `json:"total"` + } + if err := json.NewDecoder(rec.Body).Decode(&preview); err != nil { + t.Fatal(err) + } + reasons := map[int64]string{} + for _, b := range preview.Books { + reasons[b.BookID] = b.Reason + } + if preview.Total != 2 || reasons[boxSet.ID] != profileFilterPartBook || reasons[undated.ID] != profileFilterMissingDate { + t.Fatalf("preview = %+v, want the box set (partBook) and the undated work (missingDate) only", preview) + } + + // A subset: only the box set. + rec = httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/metadataprofile/1/filtered-books/unmonitor", + bytes.NewBufferString(`{"bookIds":[`+strconv.FormatInt(boxSet.ID, 10)+`,`+strconv.FormatInt(kept.ID, 10)+`]}`)) + h.UnmonitorFilteredBooks(rec, withURLParam(req, "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("unmonitor: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var res map[string]int + if err := json.NewDecoder(rec.Body).Decode(&res); err != nil { + t.Fatal(err) + } + if res["unmonitored"] != 1 { + t.Errorf("unmonitored = %d, want 1: a book the filters keep must not be unmonitored because its id was sent", res["unmonitored"]) + } + + // Everything left in the preview. + rec = httptest.NewRecorder() + h.UnmonitorFilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/metadataprofile/1/filtered-books/unmonitor", nil), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("unmonitor all: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + want := map[int64]bool{boxSet.ID: false, undated.ID: false, kept.ID: true, ownedBoxSet.ID: true, otherBoxSet.ID: true} + for bookID, monitored := range want { + got, err := books.GetByID(ctx, bookID) + if err != nil { + t.Fatal(err) + } + if got == nil { + t.Fatalf("book %d was deleted", bookID) + } + if got.Monitored != monitored { + t.Errorf("%q monitored = %v, want %v", got.Title, got.Monitored, monitored) + } + } + + rec = httptest.NewRecorder() + h.FilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/metadataprofile/99/filtered-books", nil), "id", "99")) + if rec.Code != http.StatusNotFound { + t.Errorf("unknown profile: status = %d, want 404", rec.Code) + } +} diff --git a/internal/api/profile_filters.go b/internal/api/profile_filters.go new file mode 100644 index 000000000..29054f455 --- /dev/null +++ b/internal/api/profile_filters.go @@ -0,0 +1,129 @@ +package api + +import ( + "context" + "log/slog" + "strings" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +// Reasons a metadata profile filter rejects a book. They travel in API +// responses, so they are stable identifiers rather than prose. +const ( + profileFilterPartBook = "partBook" + profileFilterMissingDate = "missingDate" + profileFilterLanguage = "language" + profileFilterMinEditionCount = "minEditionCount" + profileFilterMinPages = "minPages" + profileFilterMissingISBN = "missingIsbn" +) + +// effectiveMetadataProfile returns the metadata profile that governs an +// author: its own, or the seeded default. Nil when profiles is nil or the +// lookup fails, which every caller treats as "filter nothing", the same +// fallback the author sync's resolve helpers use, so an unresolvable profile +// never turns into unexpected catalogue loss. +func effectiveMetadataProfile(ctx context.Context, profiles *db.MetadataProfileRepo, author *models.Author) *models.MetadataProfile { + if profiles == nil { + return nil + } + id := models.DefaultMetadataProfileID + if author != nil && author.MetadataProfileID != nil { + id = *author.MetadataProfileID + } + p, err := profiles.GetByID(ctx, id) + if err != nil { + slog.Debug("metadata profile lookup failed; not filtering", "profileID", id, "error", err) + return nil + } + return p +} + +// storedBookProfileFilter reports which metadata profile filter rejects b, +// judged only from what a stored book row already carries: its title and its +// release date. "" means it passes. +// +// These are the two filters the author sync applies without a provider round +// trip (#2208). The others need data a stored row does not hold reliably: +// language is decided at sync time from provider edition evidence that is not +// stored, and MinPages, SkipMissingISBN and MinEditionCount need a fresh +// edition lookup. Judging those from the row would flag books the sync +// correctly kept. +func storedBookProfileFilter(p *models.MetadataProfile, b *models.Book) string { + if p == nil || b == nil { + return "" + } + if p.SkipPartBooks && isPartBookTitle(b.Title) { + return profileFilterPartBook + } + if p.SkipMissingDate && b.ReleaseDate == nil { + return profileFilterMissingDate + } + return "" +} + +// catalogBookProfileFilter reports which metadata profile filter rejects a +// book a series fill is about to create, or "" when it passes. It applies the +// same filters, with the same "unknown passes" semantics, as the author sync's +// discovery loop in fetchAuthorBooks (#2208): +// +// - SkipPartBooks and SkipMissingDate from the title and release date. +// - MinEditionCount against the work's own edition count; zero is unknown +// and passes. +// - The language allow list, against the provider's language. A language the +// list rejects is checked once more against the provider's edition +// evidence, which the author sync also consults, so a work whose display +// language is foreign but which has an allowed language edition is kept. +// - MinPages and SkipMissingISBN from an edition lookup, made only when the +// profile turns either on. A failed lookup enforces neither. +func catalogBookProfileFilter(ctx context.Context, meta *metadata.Aggregator, p *models.MetadataProfile, b *models.Book) string { + if p == nil || b == nil { + return "" + } + if reason := storedBookProfileFilter(p, b); reason != "" { + return reason + } + if p.MinEditionCount > 0 && b.EditionCount > 0 && b.EditionCount < p.MinEditionCount { + return profileFilterMinEditionCount + } + if allowed := models.ParseAllowedLanguages(p.AllowedLanguages); len(allowed) > 0 { + unknownFail := p.UnknownLanguageBehavior == models.UnknownLanguageFail + if !models.IsLanguageAllowed(b.Language, allowed, unknownFail) && !languageEvidenceAllows(ctx, meta, b, allowed) { + return profileFilterLanguage + } + } + if (p.MinPages > 0 || p.SkipMissingISBN) && meta != nil && b.ForeignID != "" { + editions, err := meta.GetEditions(ctx, b.ForeignID) + if err != nil { + slog.Debug("edition lookup failed while checking MinPages/SkipMissingISBN; not enforcing for this book", + "title", b.Title, "foreignId", b.ForeignID, "error", err) + return "" + } + if p.SkipMissingISBN && !anyEditionHasISBN(editions) { + return profileFilterMissingISBN + } + if p.MinPages > 0 && !passesMinPagesFilter(editions, p.MinPages) { + return profileFilterMinPages + } + } + return "" +} + +// languageEvidenceAllows asks the provider whether any edition of b is in an +// allowed language. Only a definitive "allowed" answer counts; an error or an +// indeterminate answer leaves the scalar language's verdict standing. +func languageEvidenceAllows(ctx context.Context, meta *metadata.Aggregator, b *models.Book, allowed []string) bool { + if meta == nil || b.ForeignID == "" { + return false + } + evidence, err := meta.GetAuthorWorkLanguageEvidence(ctx, []models.Book{*b}, allowed) + if err != nil { + slog.Debug("language evidence lookup failed; using the book's own language", "title", b.Title, "error", err) + return false + } + resolved, found := evidence[strings.TrimSpace(b.ForeignID)] + return found && resolved.State == metadata.AuthorWorkLanguageAllowed +} diff --git a/internal/api/series.go b/internal/api/series.go index c95c8423f..9e68d3613 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -47,6 +47,7 @@ type SeriesHandler struct { finder LibraryFinder enhancedHardcoverEnvEnabled bool editions *db.EditionRepo + profiles *db.MetadataProfileRepo editionFetcher bookhydrate.EditionFetcher @@ -102,6 +103,14 @@ func (h *SeriesHandler) WithEditionHydration(editions *db.EditionRepo) *SeriesHa return h } +// WithMetadataProfiles attaches the metadata profile repo, so a series fill +// screens the books it creates and re-queues through the same profile filters +// an author sync applies (#2208). Without it nothing is filtered. +func (h *SeriesHandler) WithMetadataProfiles(profiles *db.MetadataProfileRepo) *SeriesHandler { + h.profiles = profiles + return h +} + // WithEditionFetcher overrides the edition fetcher used by tests. func (h *SeriesHandler) WithEditionFetcher(fetcher bookhydrate.EditionFetcher) *SeriesHandler { h.editionFetcher = fetcher @@ -209,6 +218,7 @@ func (h *SeriesHandler) List(w http.ResponseWriter, r *http.Request) { if series == nil { series = []models.Series{} } + h.annotateSplitEditionParts(r.Context(), 0, series) writeJSON(w, http.StatusOK, series) return } @@ -222,6 +232,7 @@ func (h *SeriesHandler) List(w http.ResponseWriter, r *http.Request) { if series == nil { series = []models.Series{} } + h.annotateSplitEditionParts(r.Context(), 0, series) writeJSON(w, http.StatusOK, seriesListResponse{ Items: series, Total: total, @@ -249,7 +260,86 @@ func (h *SeriesHandler) Get(w http.ResponseWriter, r *http.Request) { if s.Books == nil { s.Books = []models.SeriesBook{} } - writeJSON(w, http.StatusOK, s) + one := []models.Series{*s} + h.annotateSplitEditionParts(r.Context(), s.ID, one) + writeJSON(w, http.StatusOK, one[0]) +} + +// annotateSplitEditionParts fills SplitEditionPartBookIDs on each series +// (#3048), so the series page can mark those rows and offer to unmonitor them. +// Only books the series already lists are reported, which keeps the caller's +// book scoping intact. seriesID 0 looks at every series in one query. +func (h *SeriesHandler) annotateSplitEditionParts(ctx context.Context, seriesID int64, series []models.Series) { + if h.books == nil || len(series) == 0 { + return + } + parts, err := h.books.ListCoveredSplitEditionParts(ctx, seriesID) + if err != nil { + slog.Warn("series: failed to list split edition parts", "seriesID", seriesID, "error", err) + return + } + if len(parts) == 0 { + return + } + bySeries := make(map[int64]map[int64]struct{}, len(parts)) + for _, p := range parts { + if bySeries[p.SeriesID] == nil { + bySeries[p.SeriesID] = make(map[int64]struct{}) + } + bySeries[p.SeriesID][p.BookID] = struct{}{} + } + for i := range series { + covered := bySeries[series[i].ID] + if len(covered) == 0 { + continue + } + for _, member := range series[i].Books { + if _, ok := covered[member.BookID]; ok { + series[i].SplitEditionPartBookIDs = append(series[i].SplitEditionPartBookIDs, member.BookID) + } + } + } +} + +// UnmonitorSplitEditionParts unmonitors the split edition parts of books the +// library already has in this series (#3048): the "Part 1" and "Part 2" rows +// a fill created before the series diff learned to recognise them (#2524). It +// never deletes a row and never touches an imported book; a part can be +// monitored again from its book page. +func (h *SeriesHandler) UnmonitorSplitEditionParts(w http.ResponseWriter, r *http.Request) { + id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid id"}) + return + } + series, err := h.series.GetByID(r.Context(), id) + if err != nil { + writeServerError(w, r, err) + return + } + if series == nil { + writeJSON(w, http.StatusNotFound, map[string]string{"error": "series not found"}) + return + } + parts, err := h.books.ListCoveredSplitEditionParts(r.Context(), id) + if err != nil { + writeServerError(w, r, err) + return + } + ids := make([]int64, 0, len(parts)) + for _, p := range parts { + if p.Monitored { + ids = append(ids, p.BookID) + } + } + n, err := h.books.UnmonitorNotImported(r.Context(), ids) + if err != nil { + writeServerError(w, r, err) + return + } + slog.Info("series: unmonitored split edition parts of books already in the library", + "seriesID", id, "unmonitored", n) + writeJSON(w, http.StatusOK, map[string]int64{"unmonitored": n}) } func (h *SeriesHandler) Create(w http.ResponseWriter, r *http.Request) { @@ -569,6 +659,9 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { if !h.requireEnhancedHardcoverAPI(w, r) { return } + // An explicit pick of one catalogue row is not screened by the + // metadata profile, the same exemption a single work add from an + // author page has (#1612): the user chose this book by name. book, err := h.createMissingHardcoverBook(r.Context(), id, body) if err != nil { if errors.Is(err, errSeriesNotFound) { @@ -603,8 +696,11 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { return } + skippedByProfile := 0 if h.enhancedHardcoverEnabled(r.Context()) { - if err := h.createMissingHardcoverBooks(r.Context(), id, body.format()); err != nil { + skipped, err := h.createMissingHardcoverBooks(r.Context(), id, body.format()) + skippedByProfile += skipped + if err != nil { if !errors.Is(err, errSeriesMetadataProvider) { writeServerError(w, r, err) return @@ -619,9 +715,41 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { return } - queued := 0 + // Split edition parts of a book the library already has are not gaps + // (#3048). A fill made before #2524 created them as books, and this loop + // put them back on Wanted on every later fill. They are left exactly as + // they are; the series page offers to unmonitor them. + splitParts := make(map[int64]struct{}) + if parts, err := h.books.ListCoveredSplitEditionParts(r.Context(), id); err != nil { + slog.Warn("series fill: failed to list split edition parts; queueing every book", "seriesID", id, "error", err) + } else { + for _, p := range parts { + splitParts[p.BookID] = struct{}{} + } + } + profiles := make(map[int64]*models.MetadataProfile) + + queued, skippedSplitParts := 0, 0 searchTargets := make([]models.Book, 0, len(books)) for _, b := range books { + if b.Status != models.BookStatusImported { + if _, ok := splitParts[b.ID]; ok { + skippedSplitParts++ + slog.Debug("series fill: not queueing a split edition part of a book already in the library", + "seriesID", id, "bookID", b.ID, "title", b.Title) + continue + } + // The same filters an author sync applies, judged from the + // stored row (#2208). Without this, a fill re-monitored the + // very box sets the profile screens out, including ones the + // user had just unmonitored from the profile settings. + if reason := storedBookProfileFilter(h.profileForAuthor(r.Context(), profiles, b.AuthorID), &b); reason != "" { + skippedByProfile++ + slog.Debug("series fill: not queueing a book the metadata profile filters out", + "seriesID", id, "bookID", b.ID, "title", b.Title, "filter", reason) + continue + } + } didQueue, queuedBook, err := h.queueSeriesBook(r.Context(), b) if err != nil { slog.Warn("series fill: failed to update book", "book", b.Title, "error", err) @@ -635,7 +763,31 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { h.fanOutSeriesSearches(r.Context(), searchTargets) - writeJSON(w, http.StatusOK, map[string]int{"queued": queued}) + writeJSON(w, http.StatusOK, map[string]int{ + "queued": queued, + "skippedByProfile": skippedByProfile, + "skippedSplitParts": skippedSplitParts, + }) +} + +// profileForAuthor resolves an author's effective metadata profile, caching +// it in cache for the length of one fill. Nil filters nothing. +func (h *SeriesHandler) profileForAuthor(ctx context.Context, cache map[int64]*models.MetadataProfile, authorID int64) *models.MetadataProfile { + if h.profiles == nil { + return nil + } + if p, ok := cache[authorID]; ok { + return p + } + var author *models.Author + if h.authors != nil { + if a, err := h.authors.GetByID(ctx, authorID); err == nil { + author = a + } + } + p := effectiveMetadataProfile(ctx, h.profiles, author) + cache[authorID] = p + return p } // fanOutSeriesSearches dispatches per-book indexer searches for a Series.Fill @@ -767,6 +919,9 @@ var ( errSeriesMetadataProvider = errors.New("series metadata provider") errSeriesNotFound = errors.New("series not found") errSeriesCatalogBookNotFound = errors.New("series catalog book not found") + // errSeriesBookFilteredByProfile marks a catalogue book a bulk fill did + // not create because the author's metadata profile filters it out (#2208). + errSeriesBookFilteredByProfile = errors.New("series catalog book filtered by metadata profile") ) func (h *SeriesHandler) SearchHardcover(w http.ResponseWriter, r *http.Request) { @@ -1616,27 +1771,30 @@ func localDiffBook(local models.SeriesBook) seriesHardcoverDiffBook { return item } -func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesID int64, format requestedFormat) error { +// createMissingHardcoverBooks creates every missing catalogue book of a +// series, and reports how many the metadata profile filtered out. +func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesID int64, format requestedFormat) (int, error) { if !h.enhancedHardcoverEnabled(ctx) { - return nil + return 0, nil } if h.meta == nil || h.authors == nil { - return nil + return 0, nil } series, err := h.series.GetByID(ctx, seriesID) if err != nil { - return err + return 0, err } if series == nil || series.HardcoverLink == nil { - return nil + return 0, nil } catalog, err := h.meta.GetSeriesCatalog(ctx, series.HardcoverLink.HardcoverSeriesID) if err != nil { - return fmt.Errorf("%w: %w", errSeriesMetadataProvider, err) + return 0, fmt.Errorf("%w: %w", errSeriesMetadataProvider, err) } if catalog == nil { - return nil + return 0, nil } + filtered := 0 // Write path: only ForeignBookID/Position are consumed below, so skip the // per-row library cross-reference by passing a nil books repo. diff := buildHardcoverDiff(ctx, nil, 0, series, series.HardcoverLink, catalog) @@ -1645,7 +1803,11 @@ func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesI if !ok { continue } - if _, err := h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, format); err != nil { + if _, err := h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, format, true); err != nil { + if errors.Is(err, errSeriesBookFilteredByProfile) { + filtered++ + continue + } // One bad volume must not truncate the rest of the series // (#1682). This used to return, so a single failure part-way // through silently abandoned every remaining volume and the user @@ -1654,7 +1816,7 @@ func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesI // uses. An upstream metadata failure still propagates, since that // is a whole-catalog problem rather than one book's. if errors.Is(err, errSeriesMetadataProvider) { - return err + return filtered, err } slog.Warn("series fill: failed to create catalog book; continuing", "seriesID", seriesID, "foreignBookID", missing.ForeignBookID, @@ -1662,7 +1824,7 @@ func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesI continue } } - return nil + return filtered, nil } func (h *SeriesHandler) createMissingHardcoverBook(ctx context.Context, seriesID int64, selector seriesFillRequest) (*models.Book, error) { @@ -1699,7 +1861,7 @@ func (h *SeriesHandler) createMissingHardcoverBook(ctx context.Context, seriesID if !ok { return nil, errSeriesCatalogBookNotFound } - return h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, selector.format()) + return h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, selector.format(), false) } return nil, errSeriesCatalogBookNotFound } @@ -1731,7 +1893,14 @@ func findCatalogBook(books []metadata.SeriesCatalogBook, foreignID, position str return metadata.SeriesCatalogBook{}, false } -func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series *models.Series, fallbackAuthor string, catalogBook metadata.SeriesCatalogBook, format requestedFormat) (*models.Book, error) { +// ensureHardcoverCatalogBook resolves one catalogue book to a library book, +// creating it when the library has none. applyProfile screens a book it is +// about to create through the author's metadata profile (#2208), returning +// errSeriesBookFilteredByProfile when a filter rejects it. A bulk fill sets +// it and an explicit single add does not. A book that already exists is +// linked whatever the profile says, the same way an author sync keeps +// maintaining books it already has. +func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series *models.Series, fallbackAuthor string, catalogBook metadata.SeriesCatalogBook, format requestedFormat, applyProfile bool) (*models.Book, error) { if series == nil { return nil, errSeriesNotFound } @@ -1808,7 +1977,30 @@ func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series * if skip { return nil, nil } + profileFiltered := func(profileAuthor *models.Author) bool { + if !applyProfile { + return false + } + candidate := book + candidate.Title = firstNonEmpty(book.Title, catalogBook.Title) + p := effectiveMetadataProfile(ctx, h.profiles, profileAuthor) + reason := catalogBookProfileFilter(ctx, h.meta, p, &candidate) + if reason == "" { + return false + } + slog.Debug("series fill: not creating a catalog book the metadata profile filters out", + "seriesID", series.ID, "title", candidate.Title, "foreignID", candidate.ForeignID, + "position", catalogBook.Position, "filter", reason) + return true + } if storedAuthor == nil { + // A brand new author has no books for the title match below to + // find, so screen now, before creating an author row for a book that + // will not be created. + if profileFiltered(author) { + return nil, errSeriesBookFilteredByProfile + } + applyProfile = false // Backfilled from a series catalogue, so there is no user monitor // choice to carry — take the install-wide default (#1666). db.ApplyAuthorMonitorDefaults(ctx, h.settings, author) @@ -1921,6 +2113,9 @@ func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series * if blockedByExcludedTitle { return nil, nil } + if profileFiltered(storedAuthor) { + return nil, errSeriesBookFilteredByProfile + } book.AuthorID = storedAuthor.ID // Tenancy (#1457): inherit the author's owner. diff --git a/internal/api/series_profile_filters_test.go b/internal/api/series_profile_filters_test.go new file mode 100644 index 000000000..7adbacbd4 --- /dev/null +++ b/internal/api/series_profile_filters_test.go @@ -0,0 +1,180 @@ +package api + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +func expanseCatalog() *metadata.SeriesCatalog { + author := &models.Author{ForeignID: "hc:james-s-a-corey", Name: "James S. A. Corey", SortName: "Corey, James S. A.", MetadataProvider: "hardcover"} + released := time.Date(2011, 6, 15, 0, 0, 0, 0, time.UTC) + entry := func(foreignID, providerID, title, position string, dated bool) metadata.SeriesCatalogBook { + b := models.Book{ForeignID: foreignID, Title: title, SortTitle: title, MetadataProvider: "hardcover", Author: author} + if dated { + b.ReleaseDate = &released + } + return metadata.SeriesCatalogBook{ForeignID: foreignID, ProviderID: providerID, Title: title, Position: position, Book: b} + } + books := []metadata.SeriesCatalogBook{ + entry("hc:leviathan-wakes", "1001", "Leviathan Wakes", "1", true), + entry("hc:calibans-war", "1002", "Caliban's War", "2", false), + entry("hc:abaddons-gate", "1003", "Abaddon's Gate", "3", true), + entry("hc:the-expanse-books-1-3", "1004", "The Expanse Books 1-3", "0", true), + } + return &metadata.SeriesCatalog{ + ForeignID: "hc-series:expanse", ProviderID: "777", Title: "The Expanse", + AuthorName: "James S. A. Corey", BookCount: len(books), Books: books, + } +} + +// profileSeriesFixture is enhancedSeriesFixture with the metadata profile +// repo wired, and the seeded default profile set to skip part books and +// undated works. +func profileSeriesFixture(t *testing.T, catalog *metadata.SeriesCatalog, searcher BookSearcher) (*SeriesHandler, *db.SeriesRepo, *db.AuthorRepo, *db.BookRepo) { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + seriesRepo := db.NewSeriesRepo(database) + bookRepo := db.NewBookRepo(database) + authorRepo := db.NewAuthorRepo(database) + settingsRepo := db.NewSettingsRepo(database) + profiles := db.NewMetadataProfileRepo(database) + if err := settingsRepo.Set(ctx, SettingHardcoverAPIToken, "hc-secret"); err != nil { + t.Fatal(err) + } + if err := settingsRepo.Set(ctx, SettingHardcoverEnhancedSeriesEnabled, "true"); err != nil { + t.Fatal(err) + } + p, err := profiles.GetByID(ctx, models.DefaultMetadataProfileID) + if err != nil || p == nil { + t.Fatalf("default profile: %v %v", p, err) + } + p.SkipPartBooks = true + p.SkipMissingDate = true + if err := profiles.Update(ctx, p); err != nil { + t.Fatal(err) + } + provider := &stubSeriesProvider{catalogs: map[string]*metadata.SeriesCatalog{catalog.ForeignID: catalog}} + h := NewSeriesHandler(seriesRepo, bookRepo, authorRepo, metadata.NewAggregator(provider).WithAudnexClient(nil), searcher). + WithHardcoverFeatureSettings(settingsRepo, true). + WithMetadataProfiles(profiles) + return h, seriesRepo, authorRepo, bookRepo +} + +// TestSeriesFillAppliesMetadataProfileFilters is the #2208 series fill +// regression test. ensureHardcoverCatalogBook consulted no metadata profile, +// so "add all" created the part books and undated works the author's profile +// screens out, and the re-queue loop re-monitored stored rows the profile +// rejects. An explicit add of one named book stays exempt, as a single work +// add from an author page is (#1612). +func TestSeriesFillAppliesMetadataProfileFilters(t *testing.T) { + catalog := expanseCatalog() + h, seriesRepo, authorRepo, bookRepo := profileSeriesFixture(t, catalog, newMockBookSearcher()) + ctx := context.Background() + series := linkedSeries(t, seriesRepo, catalog) + + // A box set row an earlier fill created, which the user has since + // unmonitored. Fill must not put it back on Wanted. + author := &models.Author{ForeignID: "hc:james-s-a-corey", Name: "James S. A. Corey", SortName: "Corey, James S. A.", MetadataProvider: "hardcover"} + if err := authorRepo.Create(ctx, author); err != nil { + t.Fatal(err) + } + released := time.Date(2016, 1, 1, 0, 0, 0, 0, time.UTC) + boxSet := &models.Book{ + ForeignID: "hc:the-expanse-books-4-6", AuthorID: author.ID, Title: "The Expanse Books 4-6", SortTitle: "The Expanse Books 4-6", + Status: models.BookStatusWanted, Monitored: false, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "hardcover", ReleaseDate: &released, + } + if err := bookRepo.Create(ctx, boxSet); err != nil { + t.Fatal(err) + } + if err := seriesRepo.LinkBook(ctx, series.ID, boxSet.ID, "4.5", true); 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()) + } + var body map[string]int + if err := json.NewDecoder(rec.Body).Decode(&body); err != nil { + t.Fatal(err) + } + + for _, fid := range []string{"hc:leviathan-wakes", "hc:abaddons-gate"} { + got, err := bookRepo.GetByForeignID(ctx, fid) + if err != nil { + t.Fatal(err) + } + if got == nil { + t.Errorf("%s passes every filter but was not created", fid) + } + } + for _, fid := range []string{"hc:calibans-war", "hc:the-expanse-books-1-3"} { + got, err := bookRepo.GetByForeignID(ctx, fid) + if err != nil { + t.Fatal(err) + } + if got != nil { + t.Errorf("fill created %q although the metadata profile filters it out", got.Title) + } + } + stored, err := bookRepo.GetByID(ctx, boxSet.ID) + if err != nil { + t.Fatal(err) + } + if stored.Monitored { + t.Error("fill re-monitored a stored box set the metadata profile filters out") + } + if body["queued"] != 2 || body["skippedByProfile"] != 3 { + t.Errorf("response = %+v, want queued 2 and skippedByProfile 3", body) + } + + // The per row add names one book on purpose and is not screened. + rec = httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/v1/series/1/fill", + bytes.NewBufferString(`{"foreignBookId":"hc:calibans-war","providerId":"1002","position":"2"}`)) + h.Fill(rec, withURLParam(req, "id", strconv.FormatInt(series.ID, 10))) + if rec.Code != http.StatusOK { + t.Fatalf("explicit add: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if got, err := bookRepo.GetByForeignID(ctx, "hc:calibans-war"); err != nil || got == nil { + t.Errorf("explicit add of a filtered book did not create it: %v %v", got, err) + } +} + +// TestSeriesFillWithoutProfileRepoFiltersNothing pins the fallback: a handler +// with no profile repo wired behaves as before rather than guessing. +func TestSeriesFillWithoutProfileRepoFiltersNothing(t *testing.T) { + catalog := expanseCatalog() + h, seriesRepo, _, bookRepo, _ := enhancedSeriesFixture(t, catalog, newMockBookSearcher()) + series := linkedSeries(t, seriesRepo, catalog) + + 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()) + } + got, err := bookRepo.GetByForeignID(context.Background(), "hc:calibans-war") + if err != nil { + t.Fatal(err) + } + if got == nil { + t.Error("an undated book was filtered with no profile repo wired") + } +} diff --git a/internal/api/series_split_edition_test.go b/internal/api/series_split_edition_test.go new file mode 100644 index 000000000..31a82326f --- /dev/null +++ b/internal/api/series_split_edition_test.go @@ -0,0 +1,202 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// splitEditionLibrary is the state an install that pressed Fill on +// Stormlight before #2524 is in: the whole "The Way of Kings" imported at 1, +// its two split parts created by that fill at 1.1 and 1.2, a real novella at +// 2.5, and Oathbringer's parts with no whole Oathbringer row at all. +type splitEditionLibrary struct { + series *models.Series + whole, part1, part2, radiance, edgedancer *models.Book + lonePart *models.Book +} + +func seedSplitEditionLibrary(t *testing.T, seriesRepo *db.SeriesRepo, authorRepo *db.AuthorRepo, bookRepo *db.BookRepo) splitEditionLibrary { + t.Helper() + ctx := context.Background() + author := &models.Author{ForeignID: "hc:brandon-sanderson", Name: "Brandon Sanderson", SortName: "Sanderson, Brandon", MetadataProvider: "hardcover"} + if err := authorRepo.Create(ctx, author); err != nil { + t.Fatal(err) + } + series := &models.Series{ForeignID: "hc-series:stormlight", Title: "The Stormlight Archive"} + if err := seriesRepo.Create(ctx, series); err != nil { + t.Fatal(err) + } + released := time.Date(2010, 8, 31, 0, 0, 0, 0, time.UTC) + add := func(title, position, status string, monitored bool) *models.Book { + t.Helper() + b := &models.Book{ + ForeignID: "hc:" + title, AuthorID: author.ID, Title: title, SortTitle: title, + Status: status, Monitored: monitored, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "hardcover", ReleaseDate: &released, + } + if err := bookRepo.Create(ctx, b); err != nil { + t.Fatal(err) + } + if err := seriesRepo.LinkBook(ctx, series.ID, b.ID, position, true); err != nil { + t.Fatal(err) + } + return b + } + return splitEditionLibrary{ + series: series, + whole: add("The Way of Kings", "1", models.BookStatusImported, true), + part1: add("The Way of Kings, Part 1", "1.1", models.BookStatusWanted, true), + part2: add("The Way of Kings, Part 2", "1.2", models.BookStatusWanted, false), + radiance: add("Words of Radiance", "2", models.BookStatusImported, true), + edgedancer: add("Edgedancer", "2.5", models.BookStatusWanted, false), + lonePart: add("Oathbringer, Part 1", "3.1", models.BookStatusWanted, false), + } +} + +// TestSeriesFillDoesNotRequeueSplitEditionPartsOfAnOwnedBook is the #3048 +// regression test. Fill re-queued every series book that was not imported, +// so the split parts a pre #2524 fill created went back to Wanted and +// monitored on every later fill, and were searched straight away. A real +// novella and a part with no whole beside it are still volumes and must still +// be queued. +func TestSeriesFillDoesNotRequeueSplitEditionPartsOfAnOwnedBook(t *testing.T) { + h, seriesRepo, authorRepo, bookRepo := seriesFixture(t) + lib := seedSplitEditionLibrary(t, seriesRepo, authorRepo, bookRepo) + ctx := context.Background() + + rec := httptest.NewRecorder() + h.Fill(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/1/fill", nil), "id", strconv.FormatInt(lib.series.ID, 10))) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var body map[string]int + if err := json.NewDecoder(rec.Body).Decode(&body); err != nil { + t.Fatal(err) + } + if body["queued"] != 2 { + t.Errorf("queued = %d, want 2 (Edgedancer and the lone Oathbringer part only): %+v", body["queued"], body) + } + + part2, err := bookRepo.GetByID(ctx, lib.part2.ID) + if err != nil { + t.Fatal(err) + } + if part2.Monitored { + t.Error("fill re-monitored a split part of a book already imported") + } + for _, b := range []*models.Book{lib.edgedancer, lib.lonePart} { + got, err := bookRepo.GetByID(ctx, b.ID) + if err != nil { + t.Fatal(err) + } + if !got.Monitored || got.Status != models.BookStatusWanted { + t.Errorf("%q was not queued: monitored=%v status=%s", b.Title, got.Monitored, got.Status) + } + } +} + +// TestSeriesUnmonitorSplitEditionParts covers the one click cleanup (#3048): +// it unmonitors the parts that are monitored, deletes nothing, and leaves the +// whole, the novella and the lone part alone. +func TestSeriesUnmonitorSplitEditionParts(t *testing.T) { + h, seriesRepo, authorRepo, bookRepo := seriesFixture(t) + lib := seedSplitEditionLibrary(t, seriesRepo, authorRepo, bookRepo) + ctx := context.Background() + if err := bookRepo.MarkWantedMonitored(ctx, lib.edgedancer.ID); err != nil { + t.Fatal(err) + } + if err := bookRepo.MarkWantedMonitored(ctx, lib.lonePart.ID); err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + h.UnmonitorSplitEditionParts(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/1/split-parts/unmonitor", nil), "id", strconv.FormatInt(lib.series.ID, 10))) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var body map[string]int + if err := json.NewDecoder(rec.Body).Decode(&body); err != nil { + t.Fatal(err) + } + if body["unmonitored"] != 1 { + t.Errorf("unmonitored = %d, want 1 (part 1; part 2 already was)", body["unmonitored"]) + } + want := map[int64]bool{ + lib.whole.ID: true, lib.part1.ID: false, lib.part2.ID: false, + lib.radiance.ID: true, lib.edgedancer.ID: true, lib.lonePart.ID: true, + } + for id, monitored := range want { + got, err := bookRepo.GetByID(ctx, id) + if err != nil { + t.Fatal(err) + } + if got == nil { + t.Fatalf("book %d was deleted", id) + } + if got.Monitored != monitored { + t.Errorf("%q monitored = %v, want %v", got.Title, got.Monitored, monitored) + } + } + + rec = httptest.NewRecorder() + h.UnmonitorSplitEditionParts(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/999/split-parts/unmonitor", nil), "id", "999")) + if rec.Code != http.StatusNotFound { + t.Errorf("unknown series: status = %d, want 404", rec.Code) + } +} + +// TestSeriesGetMarksSplitEditionParts checks the series payload names the +// part rows, which is what the series page reads to badge them, leave them +// out of the missing count and offer the cleanup. +func TestSeriesGetMarksSplitEditionParts(t *testing.T) { + h, seriesRepo, authorRepo, bookRepo := seriesFixture(t) + lib := seedSplitEditionLibrary(t, seriesRepo, authorRepo, bookRepo) + + rec := httptest.NewRecorder() + h.Get(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/series/1", nil), "id", strconv.FormatInt(lib.series.ID, 10))) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + var got models.Series + if err := json.NewDecoder(rec.Body).Decode(&got); err != nil { + t.Fatal(err) + } + if !sameIDs(got.SplitEditionPartBookIDs, []int64{lib.part1.ID, lib.part2.ID}) { + t.Errorf("Get splitEditionPartBookIds = %v, want parts %d and %d", got.SplitEditionPartBookIDs, lib.part1.ID, lib.part2.ID) + } + + rec = httptest.NewRecorder() + h.List(rec, httptest.NewRequest(http.MethodGet, "/api/v1/series", nil)) + var list []models.Series + if err := json.NewDecoder(rec.Body).Decode(&list); err != nil { + t.Fatal(err) + } + if len(list) != 1 || !sameIDs(list[0].SplitEditionPartBookIDs, []int64{lib.part1.ID, lib.part2.ID}) { + t.Errorf("List splitEditionPartBookIds = %+v", list) + } +} + +func sameIDs(got, want []int64) bool { + if len(got) != len(want) { + return false + } + seen := make(map[int64]int, len(want)) + for _, id := range want { + seen[id]++ + } + for _, id := range got { + if seen[id] == 0 { + return false + } + seen[id]-- + } + return true +} diff --git a/internal/db/books_metadata_profile.go b/internal/db/books_metadata_profile.go new file mode 100644 index 000000000..35eda6bd6 --- /dev/null +++ b/internal/db/books_metadata_profile.go @@ -0,0 +1,21 @@ +package db + +import ( + "context" + + "github.com/vavallee/bindery/internal/models" +) + +// ListWantedMonitoredByMetadataProfile returns the monitored, wanted, not +// excluded books whose author is governed by the given metadata profile. An +// author with no profile of its own counts as governed by the seeded default. +// These are the books a profile filter turned on later could still have an +// opinion about (#2208): an imported book is already on disk and an +// unmonitored one is already out of the search. +func (r *BookRepo) ListWantedMonitoredByMetadataProfile(ctx context.Context, profileID int64) ([]models.Book, error) { + return r.query(ctx, bookCTE+" SELECT "+bookColumns+" FROM books "+bookJoins+ + ` WHERE books.status = ? AND books.monitored = 1 AND books.excluded = 0 + AND COALESCE(au.metadata_profile_id, ?) = ? + ORDER BY au.sort_name, books.sort_key, books.sort_title`, + []any{models.BookStatusWanted, models.DefaultMetadataProfileID, profileID}) +} diff --git a/internal/db/split_edition_parts.go b/internal/db/split_edition_parts.go new file mode 100644 index 000000000..c99e963d2 --- /dev/null +++ b/internal/db/split_edition_parts.go @@ -0,0 +1,112 @@ +package db + +import ( + "context" + "fmt" + "strings" + "time" + + "github.com/vavallee/bindery/internal/models" + "github.com/vavallee/bindery/internal/seriesmatch" +) + +// SplitEditionPart is a local book that is one part of a split edition of +// another book in the same series (#3048): "The Way of Kings, Part 1" at 1.1 +// while "The Way of Kings" itself sits at 1. +type SplitEditionPart struct { + SeriesID int64 + BookID int64 + Title string + Monitored bool + WholeBookID int64 + WholeTitle string +} + +// ListCoveredSplitEditionParts returns the books that are split edition parts +// of a whole book the library already owns or is already looking for. A +// seriesID of 0 covers every series. +// +// The rule is seriesmatch.SplitEditionPartOf, the same two signals the series +// diff uses for catalogue rows (#2524): a fractional position under the whole's +// integer position, and the whole's title plus a "Part N" marker. Here it is +// applied to rows already stored, which is what an install that pressed Fill +// before #2524 has. +// +// 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. +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, + sw.book_id, sw.position_in_series, bw.title + FROM series_books sp + JOIN books bp ON bp.id = sp.book_id + JOIN series_books sw ON sw.series_id = sp.series_id AND sw.book_id <> sp.book_id + JOIN books bw ON bw.id = sw.book_id + WHERE bp.status <> ? + AND instr(sp.position_in_series, '.') > 0 + AND bw.excluded = 0 + AND (bw.status = ? OR bw.monitored = 1) + AND bw.owner_user_id IS bp.owner_user_id` + args := []any{models.BookStatusImported, models.BookStatusImported} + if seriesID > 0 { + query += ` AND sp.series_id = ?` + args = append(args, seriesID) + } + query += ` ORDER BY sp.series_id, sp.book_id, sw.book_id` + + rows, err := r.db.QueryContext(ctx, query, args...) + if err != nil { + return nil, fmt.Errorf("list split edition parts: %w", err) + } + defer rows.Close() + + type key struct{ series, book int64 } + seen := make(map[key]struct{}) + var parts []SplitEditionPart + for rows.Next() { + var p SplitEditionPart + var partPos, wholePos string + var monitored int + if err := rows.Scan(&p.SeriesID, &p.BookID, &partPos, &p.Title, &monitored, + &p.WholeBookID, &wholePos, &p.WholeTitle); err != nil { + return nil, fmt.Errorf("scan split edition part: %w", err) + } + if _, dup := seen[key{p.SeriesID, p.BookID}]; dup { + continue + } + if !seriesmatch.SplitEditionPartOf(p.Title, partPos, p.WholeTitle, wholePos) { + continue + } + p.Monitored = monitored == 1 + seen[key{p.SeriesID, p.BookID}] = struct{}{} + parts = append(parts, p) + } + return parts, rows.Err() +} + +// UnmonitorNotImported clears the monitored flag on the given books, skipping +// any that are already imported, and reports how many rows changed. Nothing is +// deleted: the books stay in the library and can be monitored again by hand. +func (r *BookRepo) UnmonitorNotImported(ctx context.Context, ids []int64) (int64, error) { + if len(ids) == 0 { + return 0, nil + } + placeholders := make([]string, len(ids)) + args := make([]any, 0, len(ids)+2) + args = append(args, timeValueArg(time.Now().UTC()), models.BookStatusImported) + for i, id := range ids { + placeholders[i] = "?" + args = append(args, id) + } + //nolint:gosec // placeholders are a fixed list of "?" markers; every value is bound via args + res, err := r.db.ExecContext(ctx, + `UPDATE books SET monitored = 0, updated_at = ? + WHERE monitored = 1 AND status <> ? AND id IN (`+strings.Join(placeholders, ",")+`)`, args...) + if err != nil { + return 0, fmt.Errorf("unmonitor books: %w", err) + } + return res.RowsAffected() +} diff --git a/internal/models/series.go b/internal/models/series.go index 1aea38bd1..b89702bf4 100644 --- a/internal/models/series.go +++ b/internal/models/series.go @@ -20,6 +20,10 @@ type Series struct { // Joined data Books []SeriesBook `json:"books,omitempty"` HardcoverLink *SeriesHardcoverLink `json:"hardcoverLink,omitempty"` + // SplitEditionPartBookIDs lists the books in this series that are split + // edition parts of a whole the library already has (#3048), filled by the + // series API only. Fill and the wanted sweep skip them. + SplitEditionPartBookIDs []int64 `json:"splitEditionPartBookIds,omitempty"` } type SeriesHardcoverLink struct { diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 9596df0f5..fbc87a13c 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1470,12 +1470,22 @@ func (s *Scheduler) wantedSearchQueue(ctx context.Context) []wantedSearch { } inFlight := s.inFlightFormatsByBook(ctx) + splitParts := s.coveredSplitEditionParts(ctx) searchQueue := make([]wantedSearch, 0, len(wanted)) for _, book := range wanted { if book.Excluded { continue } + // A split edition part of a book the library already has or is + // already looking for is not a missing volume (#3048). Searching it + // downloads text that is already on the shelf. The row stays as it + // is; the series page offers to unmonitor it. + if whole, ok := splitParts[book.ID]; ok { + slog.Debug("skipping wanted search: the book is a split edition part of a book already in the library", + "book", book.Title, "bookID", book.ID, "wholeBookID", whole) + continue + } held := inFlight[book.ID] // A book monitored for a SINGLE format has only one slot a download can // be filling, whatever container tokens the release title happens to @@ -1515,6 +1525,22 @@ func (s *Scheduler) wantedSearchQueue(ctx context.Context) []wantedSearch { return searchQueue } +// coveredSplitEditionParts maps the id of every book that is a split edition +// part of a whole already in the library to that whole's id (#3048). A lookup +// failure skips nothing, so a broken query can never stop the sweep. +func (s *Scheduler) coveredSplitEditionParts(ctx context.Context) map[int64]int64 { + parts, err := s.books.ListCoveredSplitEditionParts(ctx, 0) + if err != nil { + slog.Warn("failed to list split edition parts; searching every wanted book", "error", err) + return nil + } + covered := make(map[int64]int64, len(parts)) + for _, p := range parts { + covered[p.BookID] = p.WholeBookID + } + return covered +} + // inFlightFormatsByBook maps a book id to the set of media types that already // have a download working toward an import. The empty-string key is the // catch-all for a download whose format could not be determined; the caller diff --git a/internal/scheduler/scheduler_split_edition_test.go b/internal/scheduler/scheduler_split_edition_test.go new file mode 100644 index 000000000..0ffaf9ff4 --- /dev/null +++ b/internal/scheduler/scheduler_split_edition_test.go @@ -0,0 +1,59 @@ +package scheduler + +import ( + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// TestWantedSearchQueue_SkipsSplitEditionPartsOfAnOwnedBook is the #3048 +// regression test. An install that pressed Fill on Stormlight before #2524 +// holds "The Way of Kings, Part 1" and "Part 2" as wanted rows at 1.1 and 1.2 +// next to the imported whole at 1. The sweep kept searching them and could +// download a part of a book already on the shelf. A real novella at a +// fractional position, and a split part with no whole beside it, must still be +// searched. +func TestWantedSearchQueue_SkipsSplitEditionPartsOfAnOwnedBook(t *testing.T) { + f := newWantedQueueFixture(t) + series := &models.Series{ForeignID: "hc-series:stormlight", Title: "The Stormlight Archive"} + seriesRepo := db.NewSeriesRepo(f.database) + if err := seriesRepo.Create(f.ctx, series); err != nil { + t.Fatal(err) + } + link := func(b *models.Book, pos string) { + t.Helper() + if err := seriesRepo.LinkBook(f.ctx, series.ID, b.ID, pos, true); err != nil { + t.Fatal(err) + } + } + + whole := f.book("The Way of Kings", models.MediaTypeEbook) + if _, err := f.database.ExecContext(f.ctx, "UPDATE books SET status=? WHERE id=?", models.BookStatusImported, whole.ID); err != nil { + t.Fatal(err) + } + link(whole, "1") + part1 := f.book("The Way of Kings, Part 1", models.MediaTypeEbook) + link(part1, "1.1") + part2 := f.book("The Way of Kings, Part 2", models.MediaTypeEbook) + link(part2, "1.2") + radiance := f.book("Words of Radiance", models.MediaTypeEbook) + link(radiance, "2") + novella := f.book("Edgedancer", models.MediaTypeEbook) + link(novella, "2.5") + // Oathbringer's parts with no whole Oathbringer row: they are the volumes. + lonePart := f.book("Oathbringer, Part 1", models.MediaTypeEbook) + link(lonePart, "3.1") + + queue := f.queue() + for _, b := range []*models.Book{part1, part2} { + if entryFor(queue, b.ID) != nil { + t.Errorf("%q was queued for search although the whole book is imported", b.Title) + } + } + for _, b := range []*models.Book{radiance, novella, lonePart} { + if entryFor(queue, b.ID) == nil { + t.Errorf("%q was dropped from the sweep", b.Title) + } + } +} diff --git a/internal/seriesmatch/split_edition.go b/internal/seriesmatch/split_edition.go new file mode 100644 index 000000000..098e43416 --- /dev/null +++ b/internal/seriesmatch/split_edition.go @@ -0,0 +1,62 @@ +package seriesmatch + +import ( + "math" + "regexp" + "strconv" + "strings" +) + +// splitEditionMarkerRe matches a "Part N" marker, with an optional "of M": +// "Part 1", "Pt. 2", "Part 1 of 3". It is the same pattern the importer and +// the series diff (#2524) use for the same marker. +var splitEditionMarkerRe = regexp.MustCompile(`(?i)\b(?:part|pt)\.?\s*\d+(?:\.\d+)?(?:\s+of\s+\d+)?\b`) + +// SplitEditionPartOf reports whether the book titled partTitle at +// partPosition is one part of a split edition of the book titled wholeTitle +// at wholePosition: Stormlight's "The Way of Kings, Part 1" at 1.1 under +// "The Way of Kings" at 1. +// +// It is the two signal rule #2524 introduced for the Hardcover series diff, +// applied to rows already in the library (#3048). Both signals are required: +// +// 1. partPosition is fractional and its integer floor is wholePosition. +// 2. partTitle is wholeTitle followed by a "Part N" marker. A subtitle after +// the marker is ignored ("The Great Hunt, Part 2 of 2: New Threads in +// the Pattern"). +// +// Position alone is not enough: a real novella sits at a fractional position +// too (Edgedancer at 2.5 beside Words of Radiance at 2) and has no marker. A +// title alone is not enough either: a series made only of split parts, with +// no whole at the integer position, has nothing to be covered by. +func SplitEditionPartOf(partTitle, partPosition, wholeTitle, wholePosition string) bool { + floor, ok := fractionalFloor(partPosition) + if !ok || !SamePosition(floor, wholePosition) { + return false + } + loc := splitEditionMarkerRe.FindStringIndex(partTitle) + if loc == nil { + return false + } + prefix := strings.TrimRight(strings.TrimSpace(partTitle[:loc[0]]), " ,:;-") + if prefix == "" { + return false + } + whole := CleanTitle(wholeTitle) + return whole != "" && CleanTitle(prefix) == whole +} + +// fractionalFloor returns the integer floor of pos when pos is a fractional +// number ("1.1" gives "1"), and false for a whole number, a blank or anything +// that does not parse. +func fractionalFloor(pos string) (string, bool) { + f, err := strconv.ParseFloat(strings.TrimSpace(pos), 64) + if err != nil || math.IsNaN(f) || math.IsInf(f, 0) { + return "", false + } + floor := math.Floor(f) + if math.Abs(f-floor) < 1e-9 { + return "", false + } + return strconv.FormatFloat(floor, 'f', -1, 64), true +} diff --git a/internal/seriesmatch/split_edition_test.go b/internal/seriesmatch/split_edition_test.go new file mode 100644 index 000000000..e63a234b6 --- /dev/null +++ b/internal/seriesmatch/split_edition_test.go @@ -0,0 +1,32 @@ +package seriesmatch + +import "testing" + +func TestSplitEditionPartOf(t *testing.T) { + cases := []struct { + name string + partTitle, partPos string + wholeTitle, wholePos string + want bool + }{ + {"stormlight part 1", "The Way of Kings, Part 1", "1.1", "The Way of Kings", "1", true}, + {"stormlight part 2", "The Way of Kings, Part 2", "1.2", "The Way of Kings", "1", true}, + {"pt abbreviation", "The Way of Kings Pt. 2", "1.2", "The Way of Kings", "1", true}, + {"subtitle after marker", "The Great Hunt, Part 2 of 2: New Threads in the Pattern", "2.2", "The Great Hunt", "2", true}, + {"novella without marker", "Edgedancer", "2.5", "Words of Radiance", "2", false}, + {"part of a different book", "Words of Radiance, Part 1", "1.1", "The Way of Kings", "1", false}, + {"whole position elsewhere", "The Way of Kings, Part 1", "1.1", "The Way of Kings", "2", false}, + {"integer part position", "The Way of Kings, Part 1", "1", "The Way of Kings", "1", false}, + {"bare marker", "Part 1", "1.1", "", "1", false}, + {"blank position", "The Way of Kings, Part 1", "", "The Way of Kings", "1", false}, + {"unparseable position", "The Way of Kings, Part 1", "1a", "The Way of Kings", "1", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := SplitEditionPartOf(tc.partTitle, tc.partPos, tc.wholeTitle, tc.wholePos); got != tc.want { + t.Fatalf("SplitEditionPartOf(%q, %q, %q, %q) = %v, want %v", + tc.partTitle, tc.partPos, tc.wholeTitle, tc.wholePos, got, tc.want) + } + }) + } +} From c92743b652db8b9f7f46a8fb60438bf8d8833555 Mon Sep 17 00:00:00 2001 From: vavallee Date: Thu, 8 Oct 2026 00:34:14 -0300 Subject: [PATCH 2/3] 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 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- .../2208-series-fill-profile-filters.md | 2 + changelog.d/3048-split-edition-parts.md | 2 + cmd/bindery/authz_gate_routes_test.go | 32 ---- cmd/bindery/main.go | 3 +- cmd/bindery/sensitive_routes.go | 19 --- docs/API.md | 3 +- docs/Hardcover-Series-Wiki.md | 8 +- docs/User-Guide-Wiki.md | 10 +- internal/api/metadata_profiles.go | 3 +- internal/api/metadata_profiles_library.go | 135 ---------------- .../api/metadata_profiles_library_test.go | 144 ------------------ internal/api/profile_filters.go | 14 +- internal/api/series.go | 13 +- internal/db/books_metadata_profile.go | 21 --- internal/db/split_edition_parts_test.go | 124 +++++++++++++++ web/src/api/series.ts | 19 ++- web/src/i18n/locales/en.json | 17 ++- web/src/pages/SeriesPage.test.tsx | 42 +++++ web/src/pages/SeriesPage.tsx | 60 +++++++- 19 files changed, 292 insertions(+), 379 deletions(-) create mode 100644 changelog.d/2208-series-fill-profile-filters.md create mode 100644 changelog.d/3048-split-edition-parts.md delete mode 100644 internal/api/metadata_profiles_library.go delete mode 100644 internal/api/metadata_profiles_library_test.go delete mode 100644 internal/db/books_metadata_profile.go create mode 100644 internal/db/split_edition_parts_test.go diff --git a/changelog.d/2208-series-fill-profile-filters.md b/changelog.d/2208-series-fill-profile-filters.md new file mode 100644 index 000000000..f776bf5d4 --- /dev/null +++ b/changelog.d/2208-series-fill-profile-filters.md @@ -0,0 +1,2 @@ +### Fixed +- Fill gaps and add all on a series now respect the author's metadata profile, so box sets, undated works and anything else the profile filters out are no longer created by a series fill, and a fill no longer puts stored books the profile rejects back on Wanted (#2208). The result line says how many the profile skipped. Adding a single book from a series row still works for anything you pick by hand. Thanks to ianepreston for the report. diff --git a/changelog.d/3048-split-edition-parts.md b/changelog.d/3048-split-edition-parts.md new file mode 100644 index 000000000..7ab52ae35 --- /dev/null +++ b/changelog.d/3048-split-edition-parts.md @@ -0,0 +1,2 @@ +### Fixed +- Split edition parts of a book you already have, like "The Way of Kings, Part 1" and "Part 2" next to The Way of Kings itself, are no longer put back on Wanted by Fill gaps or searched by the scheduled wanted search, so Bindery stops downloading parts of a book on your shelf (#3048). The series card marks them as split parts, leaves them out of the missing count, and offers to unmonitor any a fill from an older version left monitored. Nothing is deleted. diff --git a/cmd/bindery/authz_gate_routes_test.go b/cmd/bindery/authz_gate_routes_test.go index 329f46884..fa1f10982 100644 --- a/cmd/bindery/authz_gate_routes_test.go +++ b/cmd/bindery/authz_gate_routes_test.go @@ -37,38 +37,6 @@ func (h *stubAuthzGateHandler) Delete(w http.ResponseWriter, _ *http.Request) { func (h *stubAuthzGateHandler) BulkDelete(w http.ResponseWriter, _ *http.Request) { h.record("bulk-delete", w) } -func (h *stubAuthzGateHandler) FilteredBooks(w http.ResponseWriter, _ *http.Request) { - h.record("filtered-books", w) -} -func (h *stubAuthzGateHandler) UnmonitorFilteredBooks(w http.ResponseWriter, _ *http.Request) { - h.record("unmonitor-filtered-books", w) -} - -// metadataProfileLibraryRoutes apply a profile to books already stored -// (#2208). The preview lists wanted books across every library and the -// action changes them, so both are admin only. -var metadataProfileLibraryRoutes = []authzGateRoute{ - {name: "preview filtered books", method: http.MethodGet, path: "/metadataprofile/3/filtered-books", called: "filtered-books"}, - {name: "unmonitor filtered books", method: http.MethodPost, path: "/metadataprofile/3/filtered-books/unmonitor", called: "unmonitor-filtered-books"}, -} - -func TestMetadataProfileLibraryRoutesRequireAdmin(t *testing.T) { - register := func(r chi.Router, h *stubAuthzGateHandler) { registerMetadataProfileLibraryRoutes(r, h) } - for _, rt := range metadataProfileLibraryRoutes { - t.Run("user "+rt.name, func(t *testing.T) { - rec, h := serveAuthzGateRoute(t, register, rt, "user") - if rec.Code != http.StatusForbidden || len(h.called) != 0 { - t.Fatalf("status = %d called = %v; want 403 and no handler call", rec.Code, h.called) - } - }) - t.Run("admin "+rt.name, func(t *testing.T) { - rec, h := serveAuthzGateRoute(t, register, rt, "admin") - if rec.Code != http.StatusNoContent || len(h.called) != 1 || h.called[0] != rt.called { - t.Fatalf("status = %d called = %v; want 204 [%s]", rec.Code, h.called, rt.called) - } - }) - } -} type authzGateRoute struct { name string diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index 84af3ecd0..717131165 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -791,7 +791,7 @@ func main() { WithMetadataProfiles(metadataProfileRepo). WithLifetimeCtx(appCtx) importListHandler := api.NewImportListHandler(importListRepo, settingsRepo, hcSyncer, userRepo) - metadataProfileHandler := api.NewMetadataProfileHandler(metadataProfileRepo).WithLibrary(bookRepo) + metadataProfileHandler := api.NewMetadataProfileHandler(metadataProfileRepo) delayProfileHandler := api.NewDelayProfileHandler(delayProfileRepo) customFormatHandler := api.NewCustomFormatHandler(customFormatRepo) bulkHandler := api.NewBulkHandler(authorRepo, bookRepo, blocklistRepo, sched). @@ -1210,7 +1210,6 @@ func main() { // Metadata profiles: reads open, writes admin only (see // registerMetadataProfileRoutes). registerMetadataProfileRoutes(r, metadataProfileHandler) - registerMetadataProfileLibraryRoutes(r, metadataProfileHandler) // Backups — Restore replaces the live database (staged now, swapped // in by db.ApplyPendingRestore at the next start), Delete removes diff --git a/cmd/bindery/sensitive_routes.go b/cmd/bindery/sensitive_routes.go index 6279b83f1..88b4f3d6f 100644 --- a/cmd/bindery/sensitive_routes.go +++ b/cmd/bindery/sensitive_routes.go @@ -323,22 +323,3 @@ func registerMetadataProfileRoutes(r chi.Router, h profileRouteHandler) { r.Delete("/metadataprofile/{id}", h.Delete) }) } - -// metadataProfileLibraryRouteHandler is the surface -// registerMetadataProfileLibraryRoutes needs. -type metadataProfileLibraryRouteHandler interface { - FilteredBooks(http.ResponseWriter, *http.Request) - UnmonitorFilteredBooks(http.ResponseWriter, *http.Request) -} - -// registerMetadataProfileLibraryRoutes mounts the routes that apply a metadata -// profile's filters to books already in the library (#2208). Both are admin -// only: the preview lists wanted books across every user's library, and the -// action changes them. -func registerMetadataProfileLibraryRoutes(r chi.Router, h metadataProfileLibraryRouteHandler) { - r.Group(func(r chi.Router) { - r.Use(auth.RequireAdmin) - r.Get("/metadataprofile/{id}/filtered-books", h.FilteredBooks) - r.Post("/metadataprofile/{id}/filtered-books/unmonitor", h.UnmonitorFilteredBooks) - }) -} diff --git a/docs/API.md b/docs/API.md index 3132d6396..4a342e74e 100644 --- a/docs/API.md +++ b/docs/API.md @@ -302,7 +302,8 @@ GET /api/v1/book/{id}/calibre where the book stands in the C ``` GET /api/v1/series list series with their linked books GET /api/v1/series/{id} one series -POST /api/v1/series/{id}/fill add the series' missing books as wanted (admin) +POST /api/v1/series/{id}/fill add the series' missing books as wanted (admin); answers {queued, skippedByProfile, skippedSplitParts} +POST /api/v1/series/{id}/split-parts/unmonitor unmonitor the split edition parts of books already in the series, listed as splitEditionPartBookIds on the series (admin); answers {unmonitored} PATCH /api/v1/series/{id} monitor / unmonitor (admin) POST /api/v1/series/{id}/merge merge other series into this one: {"sourceIds":[..],"title":"optional rename","dryRun":true} previews (admin) ``` diff --git a/docs/Hardcover-Series-Wiki.md b/docs/Hardcover-Series-Wiki.md index 78170a1c8..3f4a5235d 100644 --- a/docs/Hardcover-Series-Wiki.md +++ b/docs/Hardcover-Series-Wiki.md @@ -89,6 +89,12 @@ Catalog entries whose titles name a box set rather than a book ("box set", "boxe The fill action may create new authors and books from Hardcover metadata when the catalog entry is not already in Bindery. Those books are linked back to the series with the catalog position. +**Fill gaps** and **add all** apply the author's metadata profile to every book they would create, the same filters an author catalogue sync applies: skip part books, skip missing release date, minimum editions, allowed languages, minimum pages and skip missing ISBN. A book a filter rejects is not created, and the result line says how many the profile skipped. An author new to the library gets the default profile. The single row **add** is an explicit pick of one book, so it is not filtered, the same way adding one work from an author page is not. A book that is already in the library is linked to the series whatever the profile says. + +When fill re-queues the books already in the series, it also leaves out any that are 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. 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. + 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. ## Merging Series @@ -120,7 +126,7 @@ Undoing an import (ABS, Calibre) made before a merge does not take the merged bo - Enhanced controls are missing: save a Hardcover API token, then enable **Enhanced Hardcover series** in `Settings -> API Keys`. If your operator disabled the deployment-wide feature flag, they must remove `BINDERY_ENHANCED_HARDCOVER_API=false` and restart Bindery. - Hardcover test fails: verify the token at [Hardcover API settings](https://hardcover.app/account/api) and make sure Bindery can reach `hardcover.app`. Both `hc_pat_` personal access tokens and the older JWTs are accepted. The test tells you which side failed: `token rejected (HTTP 401: ...)` is a token to reissue, while `HTTP 500 (upstream returned a non-JSON response ...)` is a Hardcover outage to wait out. See [Troubleshooting](./Troubleshooting-Wiki.md#hardcover-fails-with-http-500-or-the-token-test-shows-a-hardcover-error-page). - Search finds the wrong series: choose a different result manually, or remove the link and search again with a more specific local series name. -- Fill gaps adds nothing: check whether the missing books already exist locally, are excluded, or whether the linked Hardcover catalog has no missing entries relative to the local series. +- Fill gaps adds nothing: check whether the missing books already exist locally, are excluded, are filtered out by the author's metadata profile (the result line counts those), or whether the linked Hardcover catalog has no missing entries relative to the local series. - Searches are not queued after fill: check the **Auto-grab** toggle in `Settings -> General` first, since it suppresses fill's searches deliberately, then verify your indexers and download-client search flow are working for normal wanted books. ## See Also diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index a6bf45b81..69eaf9ef9 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -858,10 +858,12 @@ When metadata is wrong, you have three levels of fix: 2. **Re-bind** the book, or **relink** the author ("Find better match"), to a different provider record when the match itself is wrong. 3. A **metadata profile** (languages, minimum page count, minimum edition - count, skip part books) filters what a catalogue sync lets in. Filling a - series skips every metadata profile filter today, the edition count included - ([#2208](https://github.com/vavallee/bindery/issues/2208)), so a filled - series can still bring in thin works. + count, skip part books) filters what a catalogue sync lets in, and what + **Fill gaps** or **add all** on a series creates + ([#2208](https://github.com/vavallee/bindery/issues/2208)). Adding a single + row from a series is an explicit pick and is not filtered. Filters screen + books as they arrive; **Reconcile catalogue** on an author applies them to + books already stored. Box sets need no setting. A work whose title plainly names a bundle ("... Box Set", "3 Books Set", "Carton of 10 Signed Copies") is dropped from every diff --git a/internal/api/metadata_profiles.go b/internal/api/metadata_profiles.go index 5691bee90..788437700 100644 --- a/internal/api/metadata_profiles.go +++ b/internal/api/metadata_profiles.go @@ -13,8 +13,7 @@ import ( ) type MetadataProfileHandler struct { - repo *db.MetadataProfileRepo - books *db.BookRepo + repo *db.MetadataProfileRepo } func NewMetadataProfileHandler(repo *db.MetadataProfileRepo) *MetadataProfileHandler { diff --git a/internal/api/metadata_profiles_library.go b/internal/api/metadata_profiles_library.go deleted file mode 100644 index 260ed5e27..000000000 --- a/internal/api/metadata_profiles_library.go +++ /dev/null @@ -1,135 +0,0 @@ -package api - -import ( - "encoding/json" - "errors" - "io" - "log/slog" - "net/http" - "strconv" - - "github.com/go-chi/chi/v5" - - "github.com/vavallee/bindery/internal/db" -) - -// WithLibrary attaches the book repo the filtered books endpoints read and -// unmonitor (#2208). Without it they answer 503. -func (h *MetadataProfileHandler) WithLibrary(books *db.BookRepo) *MetadataProfileHandler { - h.books = books - return h -} - -// profileFilteredBook is one library book the profile's filters would have -// kept out had they been on when the book was added. -type profileFilteredBook struct { - BookID int64 `json:"bookId"` - Title string `json:"title"` - AuthorID int64 `json:"authorId"` - AuthorName string `json:"authorName,omitempty"` - Reason string `json:"reason"` -} - -// filteredBooks loads the profile and the wanted, monitored books it governs -// that its stored row filters reject. It writes the error response itself -// and returns ok=false when the caller should stop. -func (h *MetadataProfileHandler) filteredBooks(w http.ResponseWriter, r *http.Request) ([]profileFilteredBook, bool) { - if h.books == nil { - writeJSON(w, http.StatusServiceUnavailable, map[string]string{"error": "library is not available"}) - return nil, false - } - id, err := strconv.ParseInt(chi.URLParam(r, "id"), 10, 64) - if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid id"}) - return nil, false - } - p, err := h.repo.GetByID(r.Context(), id) - if err != nil { - writeServerError(w, r, err) - return nil, false - } - if p == nil { - writeJSON(w, http.StatusNotFound, map[string]string{"error": "metadata profile not found"}) - return nil, false - } - books, err := h.books.ListWantedMonitoredByMetadataProfile(r.Context(), id) - if err != nil { - writeServerError(w, r, err) - return nil, false - } - out := make([]profileFilteredBook, 0) - for i := range books { - b := &books[i] - reason := storedBookProfileFilter(p, b) - if reason == "" { - continue - } - item := profileFilteredBook{BookID: b.ID, Title: b.Title, AuthorID: b.AuthorID, Reason: reason} - if b.Author != nil { - item.AuthorName = b.Author.Name - } - out = append(out, item) - } - return out, true -} - -// FilteredBooks previews the wanted, monitored books already in the library -// that this profile's filters screen out (#2208). The filters only screen new -// books during a sync, so turning one on never touched what was already -// there. Only the filters a stored row can be judged by are applied: -// SkipPartBooks and SkipMissingDate (see storedBookProfileFilter). Nothing -// changes here; UnmonitorFilteredBooks is the action. -func (h *MetadataProfileHandler) FilteredBooks(w http.ResponseWriter, r *http.Request) { - books, ok := h.filteredBooks(w, r) - if !ok { - return - } - writeJSON(w, http.StatusOK, map[string]any{"books": books, "total": len(books)}) -} - -// UnmonitorFilteredBooks unmonitors books from the FilteredBooks preview. The -// body may name bookIds to act on a subset; without it every previewed book -// is unmonitored. Ids are checked against a fresh preview, so a stale or -// hand written list can never unmonitor a book the filters do not reject. -// Nothing is deleted and imported books are never touched: an unmonitored -// book stays in the library, leaves Wanted, and can be monitored again from -// its page. -func (h *MetadataProfileHandler) UnmonitorFilteredBooks(w http.ResponseWriter, r *http.Request) { - var body struct { - BookIDs []int64 `json:"bookIds"` - } - if r.Body != nil { - if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid request body"}) - return - } - } - books, ok := h.filteredBooks(w, r) - if !ok { - return - } - var want map[int64]struct{} - if len(body.BookIDs) > 0 { - want = make(map[int64]struct{}, len(body.BookIDs)) - for _, id := range body.BookIDs { - want[id] = struct{}{} - } - } - ids := make([]int64, 0, len(books)) - for _, b := range books { - if want != nil { - if _, ok := want[b.BookID]; !ok { - continue - } - } - ids = append(ids, b.BookID) - } - n, err := h.books.UnmonitorNotImported(r.Context(), ids) - if err != nil { - writeServerError(w, r, err) - return - } - slog.Info("metadata profile: unmonitored books its filters screen out", - "profileID", chi.URLParam(r, "id"), "unmonitored", n) - writeJSON(w, http.StatusOK, map[string]int64{"unmonitored": n}) -} diff --git a/internal/api/metadata_profiles_library_test.go b/internal/api/metadata_profiles_library_test.go deleted file mode 100644 index 88d2dafa5..000000000 --- a/internal/api/metadata_profiles_library_test.go +++ /dev/null @@ -1,144 +0,0 @@ -package api - -import ( - "bytes" - "context" - "encoding/json" - "net/http" - "net/http/httptest" - "strconv" - "testing" - "time" - - "github.com/vavallee/bindery/internal/db" - "github.com/vavallee/bindery/internal/models" -) - -// TestMetadataProfileFilteredBooks is the #2208 retroactive test. Every -// profile filter is gated on the book being new, so turning SkipPartBooks or -// SkipMissingDate on never touched the box sets and undated works already on -// Wanted. The preview lists exactly those, and the action unmonitors them -// without deleting anything or touching an imported book, a book whose author -// uses another profile, or a book the filters let through. -func TestMetadataProfileFilteredBooks(t *testing.T) { - database, err := db.OpenMemory() - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { database.Close() }) - ctx := context.Background() - profiles := db.NewMetadataProfileRepo(database) - authors := db.NewAuthorRepo(database) - books := db.NewBookRepo(database) - h := NewMetadataProfileHandler(profiles).WithLibrary(books) - - p, err := profiles.GetByID(ctx, models.DefaultMetadataProfileID) - if err != nil || p == nil { - t.Fatalf("default profile: %v %v", p, err) - } - p.SkipPartBooks = true - p.SkipMissingDate = true - if err := profiles.Update(ctx, p); err != nil { - t.Fatal(err) - } - other := &models.MetadataProfile{Name: "Anything", AllowedLanguages: "any"} - if err := profiles.Create(ctx, other); err != nil { - t.Fatal(err) - } - - newAuthor := func(name string, profileID *int64) *models.Author { - t.Helper() - a := &models.Author{ForeignID: "ol:" + name, Name: name, SortName: name, MetadataProvider: "openlibrary", MetadataProfileID: profileID} - if err := authors.Create(ctx, a); err != nil { - t.Fatal(err) - } - return a - } - corey := newAuthor("James S. A. Corey", nil) - otherAuthor := newAuthor("Other Author", &other.ID) - released := time.Date(2011, 6, 15, 0, 0, 0, 0, time.UTC) - newBook := func(a *models.Author, title, status string, monitored, dated bool) *models.Book { - t.Helper() - b := &models.Book{ - ForeignID: "ol:" + title, AuthorID: a.ID, Title: title, SortTitle: title, - Status: status, Monitored: monitored, MediaType: models.MediaTypeEbook, - Genres: []string{}, MetadataProvider: "openlibrary", - } - if dated { - b.ReleaseDate = &released - } - if err := books.Create(ctx, b); err != nil { - t.Fatal(err) - } - return b - } - boxSet := newBook(corey, "The Expanse Books 1-3", models.BookStatusWanted, true, true) - undated := newBook(corey, "Caliban's War", models.BookStatusWanted, true, false) - kept := newBook(corey, "Leviathan Wakes", models.BookStatusWanted, true, true) - ownedBoxSet := newBook(corey, "The Expanse Books 4-6", models.BookStatusImported, true, true) - otherBoxSet := newBook(otherAuthor, "Other Books 1-3", models.BookStatusWanted, true, true) - - id := strconv.FormatInt(models.DefaultMetadataProfileID, 10) - rec := httptest.NewRecorder() - h.FilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/metadataprofile/1/filtered-books", nil), "id", id)) - if rec.Code != http.StatusOK { - t.Fatalf("preview: expected 200, got %d: %s", rec.Code, rec.Body.String()) - } - var preview struct { - Books []profileFilteredBook `json:"books"` - Total int `json:"total"` - } - if err := json.NewDecoder(rec.Body).Decode(&preview); err != nil { - t.Fatal(err) - } - reasons := map[int64]string{} - for _, b := range preview.Books { - reasons[b.BookID] = b.Reason - } - if preview.Total != 2 || reasons[boxSet.ID] != profileFilterPartBook || reasons[undated.ID] != profileFilterMissingDate { - t.Fatalf("preview = %+v, want the box set (partBook) and the undated work (missingDate) only", preview) - } - - // A subset: only the box set. - rec = httptest.NewRecorder() - req := httptest.NewRequest(http.MethodPost, "/api/v1/metadataprofile/1/filtered-books/unmonitor", - bytes.NewBufferString(`{"bookIds":[`+strconv.FormatInt(boxSet.ID, 10)+`,`+strconv.FormatInt(kept.ID, 10)+`]}`)) - h.UnmonitorFilteredBooks(rec, withURLParam(req, "id", id)) - if rec.Code != http.StatusOK { - t.Fatalf("unmonitor: expected 200, got %d: %s", rec.Code, rec.Body.String()) - } - var res map[string]int - if err := json.NewDecoder(rec.Body).Decode(&res); err != nil { - t.Fatal(err) - } - if res["unmonitored"] != 1 { - t.Errorf("unmonitored = %d, want 1: a book the filters keep must not be unmonitored because its id was sent", res["unmonitored"]) - } - - // Everything left in the preview. - rec = httptest.NewRecorder() - h.UnmonitorFilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/metadataprofile/1/filtered-books/unmonitor", nil), "id", id)) - if rec.Code != http.StatusOK { - t.Fatalf("unmonitor all: expected 200, got %d: %s", rec.Code, rec.Body.String()) - } - - want := map[int64]bool{boxSet.ID: false, undated.ID: false, kept.ID: true, ownedBoxSet.ID: true, otherBoxSet.ID: true} - for bookID, monitored := range want { - got, err := books.GetByID(ctx, bookID) - if err != nil { - t.Fatal(err) - } - if got == nil { - t.Fatalf("book %d was deleted", bookID) - } - if got.Monitored != monitored { - t.Errorf("%q monitored = %v, want %v", got.Title, got.Monitored, monitored) - } - } - - rec = httptest.NewRecorder() - h.FilteredBooks(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/metadataprofile/99/filtered-books", nil), "id", "99")) - if rec.Code != http.StatusNotFound { - t.Errorf("unknown profile: status = %d, want 404", rec.Code) - } -} diff --git a/internal/api/profile_filters.go b/internal/api/profile_filters.go index 29054f455..e0c74556e 100644 --- a/internal/api/profile_filters.go +++ b/internal/api/profile_filters.go @@ -44,14 +44,16 @@ func effectiveMetadataProfile(ctx context.Context, profiles *db.MetadataProfileR // storedBookProfileFilter reports which metadata profile filter rejects b, // judged only from what a stored book row already carries: its title and its -// release date. "" means it passes. +// release date. "" means it passes. Series fill uses it so a fill does not put +// a stored row the profile rejects back on Wanted (#2208). // // These are the two filters the author sync applies without a provider round -// trip (#2208). The others need data a stored row does not hold reliably: -// language is decided at sync time from provider edition evidence that is not -// stored, and MinPages, SkipMissingISBN and MinEditionCount need a fresh -// edition lookup. Judging those from the row would flag books the sync -// correctly kept. +// trip. The others need data a stored row does not hold reliably: language is +// decided from provider edition evidence that is not stored, and MinPages, +// SkipMissingISBN and MinEditionCount need a fresh edition lookup. Judging +// those from the row would skip books the sync correctly kept. Reconcile +// catalogue (catalogue_reconciliation.go) is the place that applies every +// filter to stored rows, with the provider lookups that needs. func storedBookProfileFilter(p *models.MetadataProfile, b *models.Book) string { if p == nil || b == nil { return "" diff --git a/internal/api/series.go b/internal/api/series.go index 9e68d3613..10a87db63 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -289,13 +289,14 @@ func (h *SeriesHandler) annotateSplitEditionParts(ctx context.Context, seriesID bySeries[p.SeriesID][p.BookID] = struct{}{} } for i := range series { - covered := bySeries[series[i].ID] + s := &series[i] + covered := bySeries[s.ID] if len(covered) == 0 { continue } - for _, member := range series[i].Books { + for _, member := range s.Books { if _, ok := covered[member.BookID]; ok { - series[i].SplitEditionPartBookIDs = append(series[i].SplitEditionPartBookIDs, member.BookID) + s.SplitEditionPartBookIDs = append(s.SplitEditionPartBookIDs, member.BookID) } } } @@ -740,9 +741,9 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { continue } // The same filters an author sync applies, judged from the - // stored row (#2208). Without this, a fill re-monitored the - // very box sets the profile screens out, including ones the - // user had just unmonitored from the profile settings. + // stored row (#2208). Without this, a fill put the very box + // sets the profile screens out back on Wanted and monitored, + // including ones the user had unmonitored by hand. if reason := storedBookProfileFilter(h.profileForAuthor(r.Context(), profiles, b.AuthorID), &b); reason != "" { skippedByProfile++ slog.Debug("series fill: not queueing a book the metadata profile filters out", diff --git a/internal/db/books_metadata_profile.go b/internal/db/books_metadata_profile.go deleted file mode 100644 index 35eda6bd6..000000000 --- a/internal/db/books_metadata_profile.go +++ /dev/null @@ -1,21 +0,0 @@ -package db - -import ( - "context" - - "github.com/vavallee/bindery/internal/models" -) - -// ListWantedMonitoredByMetadataProfile returns the monitored, wanted, not -// excluded books whose author is governed by the given metadata profile. An -// author with no profile of its own counts as governed by the seeded default. -// These are the books a profile filter turned on later could still have an -// opinion about (#2208): an imported book is already on disk and an -// unmonitored one is already out of the search. -func (r *BookRepo) ListWantedMonitoredByMetadataProfile(ctx context.Context, profileID int64) ([]models.Book, error) { - return r.query(ctx, bookCTE+" SELECT "+bookColumns+" FROM books "+bookJoins+ - ` WHERE books.status = ? AND books.monitored = 1 AND books.excluded = 0 - AND COALESCE(au.metadata_profile_id, ?) = ? - ORDER BY au.sort_name, books.sort_key, books.sort_title`, - []any{models.BookStatusWanted, models.DefaultMetadataProfileID, profileID}) -} diff --git a/internal/db/split_edition_parts_test.go b/internal/db/split_edition_parts_test.go new file mode 100644 index 000000000..3ee7e1031 --- /dev/null +++ b/internal/db/split_edition_parts_test.go @@ -0,0 +1,124 @@ +package db + +import ( + "context" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// TestListCoveredSplitEditionParts pins which wholes count as "already in the +// library" for #3048, and that one user's whole never covers another user's +// part. +func TestListCoveredSplitEditionParts(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + authors := NewAuthorRepo(database) + books := NewBookRepo(database) + series := NewSeriesRepo(database) + users := NewUserRepo(database) + alice, err := users.Create(ctx, "alice", "h1") + if err != nil { + t.Fatal(err) + } + bob, err := users.Create(ctx, "bob", "h2") + if err != nil { + t.Fatal(err) + } + + author := &models.Author{ForeignID: "hc:sanderson", Name: "Brandon Sanderson", SortName: "Sanderson, Brandon", MetadataProvider: "hardcover"} + if err := authors.Create(ctx, author); err != nil { + t.Fatal(err) + } + type row struct { + title, position, status string + monitored, excluded bool + owner int64 + } + seed := func(seriesTitle string, rows ...row) (int64, []int64) { + t.Helper() + s := &models.Series{ForeignID: "hc-series:" + seriesTitle, Title: seriesTitle} + if err := series.Create(ctx, s); err != nil { + t.Fatal(err) + } + ids := make([]int64, 0, len(rows)) + for _, r := range rows { + b := &models.Book{ + ForeignID: "hc:" + seriesTitle + ":" + r.title, AuthorID: author.ID, Title: r.title, SortTitle: r.title, + Status: r.status, Monitored: r.monitored, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "hardcover", OwnerUserID: r.owner, + } + if err := books.Create(ctx, b); err != nil { + t.Fatal(err) + } + if r.excluded { + if _, err := database.ExecContext(ctx, "UPDATE books SET excluded = 1 WHERE id = ?", b.ID); err != nil { + t.Fatal(err) + } + } + if err := series.LinkBook(ctx, s.ID, b.ID, r.position, true); err != nil { + t.Fatal(err) + } + ids = append(ids, b.ID) + } + return s.ID, ids + } + + // Covered: the whole is wanted but monitored, so the library is already + // looking for it. + wantedID, wanted := seed("wanted-whole", + row{"The Way of Kings", "1", models.BookStatusWanted, true, false, alice.ID}, + row{"The Way of Kings, Part 1", "1.1", models.BookStatusWanted, true, false, alice.ID}) + // Not covered: the whole is excluded. + _, excluded := seed("excluded-whole", + row{"The Way of Kings", "1", models.BookStatusImported, true, true, alice.ID}, + row{"The Way of Kings, Part 1", "1.1", models.BookStatusWanted, true, false, alice.ID}) + // Not covered: the whole is wanted and unmonitored, so nothing is being + // sought and the parts may be what the user wants. + _, unwanted := seed("unwanted-whole", + row{"The Way of Kings", "1", models.BookStatusWanted, false, false, alice.ID}, + row{"The Way of Kings, Part 1", "1.1", models.BookStatusWanted, true, false, alice.ID}) + // Not covered: alice owns the whole, the part is bob's. + _, crossUser := seed("cross-user", + row{"The Way of Kings", "1", models.BookStatusImported, true, false, alice.ID}, + row{"The Way of Kings, Part 1", "1.1", models.BookStatusWanted, true, false, bob.ID}) + // Not reported: the part is already imported. + _, importedPart := seed("imported-part", + row{"The Way of Kings", "1", models.BookStatusImported, true, false, alice.ID}, + row{"The Way of Kings, Part 1", "1.1", models.BookStatusImported, true, false, alice.ID}) + + parts, err := books.ListCoveredSplitEditionParts(ctx, 0) + if err != nil { + t.Fatal(err) + } + if len(parts) != 1 || parts[0].BookID != wanted[1] || parts[0].WholeBookID != wanted[0] || !parts[0].Monitored { + t.Fatalf("parts = %+v, want only book %d covered by %d", parts, wanted[1], wanted[0]) + } + for name, ids := range map[string][]int64{"excluded": excluded, "unwanted": unwanted, "cross user": crossUser, "imported part": importedPart} { + for _, p := range parts { + if p.BookID == ids[1] { + t.Errorf("%s case: part %d reported as covered", name, ids[1]) + } + } + } + + scoped, err := books.ListCoveredSplitEditionParts(ctx, wantedID+100) + if err != nil { + t.Fatal(err) + } + if len(scoped) != 0 { + t.Errorf("a series filter matched other series: %+v", scoped) + } + + n, err := books.UnmonitorNotImported(ctx, []int64{wanted[1], importedPart[1]}) + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Errorf("UnmonitorNotImported changed %d rows, want 1 (the imported part is left alone)", n) + } +} diff --git a/web/src/api/series.ts b/web/src/api/series.ts index a3ed92974..6431cbcb5 100644 --- a/web/src/api/series.ts +++ b/web/src/api/series.ts @@ -93,6 +93,17 @@ export interface Series { book?: Book }> hardcoverLink?: SeriesHardcoverLink + // Books in this series that are split edition parts of a whole the library + // already has (#3048). Fill and the wanted search skip them. + splitEditionPartBookIds?: number[] +} + +// What POST /series/{id}/fill reports. The skipped counts are absent on a +// single book add, which is never screened. +export interface SeriesFillResult { + queued: number + skippedByProfile?: number + skippedSplitParts?: number } // What a series merge does (#2554): POST /series/{id}/merge returns it, as a @@ -143,7 +154,7 @@ export const seriesApi = { setPrimarySeriesForBook: (id: number, bookId: number) => request(`/series/${id}/books/${bookId}/primary`, { method: 'PUT' }), fillSeries: (id: number, book?: SeriesFillBookRequest) => - request<{ queued: number }>(`/series/${id}/fill`, { + request(`/series/${id}/fill`, { method: 'POST', ...(book ? { body: JSON.stringify(book) } : {}), }), @@ -151,10 +162,14 @@ export const seriesApi = { // fillSeries(book), this carries no book selector — the backend expands the // whole Hardcover catalog — so the media type travels in its own body. fillSeriesAll: (id: number, mediaType?: MediaType) => - request<{ queued: number }>(`/series/${id}/fill`, { + request(`/series/${id}/fill`, { method: 'POST', ...(mediaType ? { body: JSON.stringify({ mediaType }) } : {}), }), + // Unmonitor the split edition parts of books already in the series (#3048). + // Nothing is deleted. + unmonitorSeriesSplitParts: (id: number) => + request<{ unmonitored: number }>(`/series/${id}/split-parts/unmonitor`, { method: 'POST' }), // Genre override (#1446, #1709): set + lock the genre list on every current // and subsequently added book in the series. applySeriesGenres: (id: number, genres: string[]) => diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index a0922038f..33cc68cdd 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -830,7 +830,11 @@ "nothing": "Nothing to fill", "queued_one": "{{count}} book queued", "queued_other": "{{count}} books queued", - "failed": "Failed" + "failed": "Failed", + "skippedByProfile_one": "{{count}} skipped by the metadata profile", + "skippedByProfile_other": "{{count}} skipped by the metadata profile", + "skippedSplitParts_one": "{{count}} split edition part skipped", + "skippedSplitParts_other": "{{count}} split edition parts skipped" }, "hardcover": { "searching": "Searching…", @@ -883,6 +887,17 @@ "confirmBody_one": "Merge {{count}} series into \"{{title}}\"? The merged series are deleted; this cannot be undone.", "confirmBody_other": "Merge {{count}} series into \"{{title}}\"? The merged series are deleted; this cannot be undone.", "failed": "Merge failed" + }, + "splitParts": { + "badge": "Split part", + "badgeHint": "One part of a split edition of a book already in this series. Fill and the wanted search skip it.", + "unmonitor_one": "Unmonitor {{count}} split part", + "unmonitor_other": "Unmonitor {{count}} split parts", + "unmonitorHint": "Stop searching for the split edition parts of books you already have. Nothing is deleted.", + "confirm": "Unmonitor the split edition parts of books already in this series? They stay in the library and leave Wanted, and you can monitor any of them again from its book page.", + "done_one": "{{count}} split part unmonitored", + "done_other": "{{count}} split parts unmonitored", + "failed": "Could not unmonitor the split parts" } }, "importHints": { diff --git a/web/src/pages/SeriesPage.test.tsx b/web/src/pages/SeriesPage.test.tsx index b05eaa5d1..744d1b150 100644 --- a/web/src/pages/SeriesPage.test.tsx +++ b/web/src/pages/SeriesPage.test.tsx @@ -37,6 +37,7 @@ vi.mock('../api/client', async importOriginal => { linkSeriesHardcover: vi.fn(), unlinkSeriesHardcover: vi.fn(), getSeriesHardcoverDiff: vi.fn(), + unmonitorSeriesSplitParts: vi.fn(), }, } }) @@ -320,6 +321,47 @@ describe('SeriesPage', () => { expect(await screen.findByText('Excluded')).toBeInTheDocument() }) + it('marks split edition parts, leaves them out of the missing count and unmonitors them (#3048)', async () => { + const book = (id: number, title: string, status: Book['status'], monitored: boolean): Book => ({ + id, foreignBookId: `book-${id}`, authorId: 5, title, description: '', imageUrl: '', + releaseDate: '2010-08-31', genres: [], monitored, status, filePath: '', mediaType: 'ebook', + ebookFilePath: '', audiobookFilePath: '', excluded: false, + }) + const stormlight: Series = { + id: 40, + foreignSeriesId: 'series-40', + title: 'The Stormlight Archive', + description: '', + monitored: true, + splitEditionPartBookIds: [302, 303], + books: [ + { seriesId: 40, bookId: 301, positionInSeries: '1', book: book(301, 'The Way of Kings', 'imported', true) }, + { seriesId: 40, bookId: 302, positionInSeries: '1.1', book: book(302, 'The Way of Kings, Part 1', 'wanted', true) }, + { seriesId: 40, bookId: 303, positionInSeries: '1.2', book: book(303, 'The Way of Kings, Part 2', 'wanted', true) }, + ], + } + vi.mocked(api.unmonitorSeriesSplitParts).mockResolvedValue({ unmonitored: 2 }) + renderSeriesPage([stormlight], { version: 'dev', commit: 'unknown', buildDate: '', enhancedHardcoverApi: false, hardcoverTokenConfigured: true }) + + const heading = await screen.findByRole('heading', { name: 'The Stormlight Archive' }) + // The two parts are not gaps: the whole is imported. + expect(screen.queryByText('2 missing')).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Fill gaps' })).not.toBeInTheDocument() + + fireEvent.click(heading) + expect(await screen.findAllByText('Split part')).toHaveLength(2) + + vi.mocked(api.listSeries).mockResolvedValue([{ + ...stormlight, + books: stormlight.books!.map(b => b.bookId === 301 ? b : { ...b, book: { ...b.book!, monitored: false } }), + }]) + fireEvent.click(screen.getByRole('button', { name: 'Unmonitor 2 split parts' })) + await acceptConfirm() + await waitFor(() => expect(api.unmonitorSeriesSplitParts).toHaveBeenCalledWith(40)) + expect(await screen.findByText('2 split parts unmonitored')).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Unmonitor 2 split parts' })).not.toBeInTheDocument() + }) + it('opens the Hardcover series link modal from the Search control', async () => { vi.mocked(api.autoLinkSeriesHardcover).mockResolvedValue({ linked: false, diff --git a/web/src/pages/SeriesPage.tsx b/web/src/pages/SeriesPage.tsx index 273bc4a22..29ee1c2b5 100644 --- a/web/src/pages/SeriesPage.tsx +++ b/web/src/pages/SeriesPage.tsx @@ -21,6 +21,13 @@ function parseSeriesFilter(raw: string | null): SeriesFilter { return SERIES_FILTERS.find(f => f === raw) ?? 'all' } +// How many of a series' split edition parts (#3048) are still monitored, which +// is what the unmonitor action would change. +function monitoredSplitParts(series: Series): number { + const parts = new Set(series.splitEditionPartBookIds ?? []) + return (series.books ?? []).filter(b => parts.has(b.bookId) && b.book?.monitored).length +} + // The counts behind a series card's "N missing" badge, and the only place they // are computed, so the Missing and Complete filters cannot disagree with the // badge the user is looking at. With enhanced Hardcover on, the badge can count @@ -31,8 +38,10 @@ function parseSeriesFilter(raw: string | null): SeriesFilter { function seriesMissingCounts(series: Series, enhancedHardcoverApi: boolean, diff?: SeriesHardcoverDiff) { const books = series.books ?? [] // Excluded books are not a gap: counting them showed a "missing" pill - // that Fill could not act on (#2324). - const gapCount = books.filter(b => b.book && b.book.status !== 'imported' && !b.book.excluded).length + // that Fill could not act on (#2324). Nor is a split edition part of a book + // already in the series, which Fill skips (#3048). + const splitParts = new Set(series.splitEditionPartBookIds ?? []) + const gapCount = books.filter(b => b.book && b.book.status !== 'imported' && !b.book.excluded && !splitParts.has(b.bookId)).length const hardcoverMissingEstimate = enhancedHardcoverApi ? Math.max(0, (series.hardcoverLink?.hardcoverBookCount ?? 0) - books.length) : 0 const hardcoverMissingCount = enhancedHardcoverApi ? (diff?.missingCount ?? hardcoverMissingEstimate) : 0 const displayMissingCount = Math.max(gapCount, hardcoverMissingCount) @@ -219,7 +228,12 @@ export default function SeriesPage() { ...(mediaType ? { mediaType } : {}), }) : await api.fillSeriesAll(series.id, mediaType) - setFillResult(prev => ({ ...prev, [series.id]: r.queued === 0 ? t('series.fill.nothing') : t('series.fill.queued', { count: r.queued }) })) + // Say what the fill left out and why, or a profile that filtered every + // missing book reads as a fill that did nothing (#2208, #3048). + const parts = [r.queued === 0 ? t('series.fill.nothing') : t('series.fill.queued', { count: r.queued })] + if (r.skippedByProfile) parts.push(t('series.fill.skippedByProfile', { count: r.skippedByProfile })) + if (r.skippedSplitParts) parts.push(t('series.fill.skippedSplitParts', { count: r.skippedSplitParts })) + setFillResult(prev => ({ ...prev, [series.id]: parts.join(', ') })) const list = await refreshSeriesList() const updated = list.find(s => s.id === series.id) if (enhancedHardcoverApi && updated?.hardcoverLink) { @@ -232,6 +246,26 @@ export default function SeriesPage() { } } + // One click cleanup for the split edition parts a fill created before the + // series diff learned to recognise them (#3048). Unmonitors, never deletes. + const unmonitorSplitParts = async (series: Series) => { + if (!await confirm({ + title: t('common.confirmTitle'), + body: t('series.splitParts.confirm'), + confirmLabel: t('series.splitParts.unmonitor', { count: monitoredSplitParts(series) }), + })) return + setFilling(series.id) + try { + const r = await api.unmonitorSeriesSplitParts(series.id) + setFillResult(prev => ({ ...prev, [series.id]: t('series.splitParts.done', { count: r.unmonitored }) })) + await refreshSeriesList() + } catch { + setFillResult(prev => ({ ...prev, [series.id]: t('series.splitParts.failed') })) + } finally { + setFilling(null) + } + } + const openHardcoverLink = async (series: Series) => { if (!enhancedHardcoverApi) return setLinkResult(prev => { @@ -391,6 +425,8 @@ export default function SeriesPage() { const diff = diffs[series.id] const { gapCount, hardcoverMissingCount, displayMissingCount } = seriesMissingCounts(series, enhancedHardcoverApi, diff) const fillNeeded = gapCount > 0 || hardcoverMissingCount > 0 + const splitPartIds = new Set(series.splitEditionPartBookIds ?? []) + const splitPartsToUnmonitor = monitoredSplitParts(series) const isOpen = expanded === series.id const sortedBooks = [...books].sort((a, b) => { const posA = parseFloat(a.positionInSeries) || 0 @@ -515,6 +551,16 @@ export default function SeriesPage() { {filling === series.id ? t('series.fill.queuing') : t('series.fill.button')} )} + {splitPartsToUnmonitor > 0 && ( + + )} {fillResult[series.id] && ( {fillResult[series.id]} )} @@ -570,6 +616,14 @@ export default function SeriesPage() { {t('series.excluded')} )} + {splitPartIds.has(entry.bookId) && ( + + {t('series.splitParts.badge')} + + )} ))} From 1c0bf1c09c059bd02f1d48a4761bfd3cb25168cb Mon Sep 17 00:00:00 2001 From: vavallee Date: Thu, 8 Oct 2026 00:49:15 -0300 Subject: [PATCH 3/3] 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 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- docs/Hardcover-Series-Wiki.md | 2 +- internal/api/profile_filters.go | 36 +++++++---- internal/api/profile_filters_test.go | 66 +++++++++++++++++++++ internal/api/series.go | 20 +++++-- internal/api/series_profile_filters_test.go | 21 ++++++- 5 files changed, 125 insertions(+), 20 deletions(-) create mode 100644 internal/api/profile_filters_test.go diff --git a/docs/Hardcover-Series-Wiki.md b/docs/Hardcover-Series-Wiki.md index 3f4a5235d..7b89ced87 100644 --- a/docs/Hardcover-Series-Wiki.md +++ b/docs/Hardcover-Series-Wiki.md @@ -91,7 +91,7 @@ The fill action may create new authors and books from Hardcover metadata when th **Fill gaps** and **add all** apply the author's metadata profile to every book they would create, the same filters an author catalogue sync applies: skip part books, skip missing release date, minimum editions, allowed languages, minimum pages and skip missing ISBN. A book a filter rejects is not created, and the result line says how many the profile skipped. An author new to the library gets the default profile. The single row **add** is an explicit pick of one book, so it is not filtered, the same way adding one work from an author page is not. A book that is already in the library is linked to the series whatever the profile says. -When fill re-queues the books already in the series, it also leaves out any that are 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. The other filters need a fresh provider lookup; use **More → Reconcile catalogue…** on the author to apply every filter to books already stored. +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. diff --git a/internal/api/profile_filters.go b/internal/api/profile_filters.go index e0c74556e..71356e08a 100644 --- a/internal/api/profile_filters.go +++ b/internal/api/profile_filters.go @@ -79,6 +79,7 @@ func storedBookProfileFilter(p *models.MetadataProfile, b *models.Book) string { // list rejects is checked once more against the provider's edition // evidence, which the author sync also consults, so a work whose display // language is foreign but which has an allowed language edition is kept. +// A blank language, or an evidence lookup that fails, passes. // - MinPages and SkipMissingISBN from an edition lookup, made only when the // profile turns either on. A failed lookup enforces neither. func catalogBookProfileFilter(ctx context.Context, meta *metadata.Aggregator, p *models.MetadataProfile, b *models.Book) string { @@ -91,10 +92,22 @@ func catalogBookProfileFilter(ctx context.Context, meta *metadata.Aggregator, p if p.MinEditionCount > 0 && b.EditionCount > 0 && b.EditionCount < p.MinEditionCount { return profileFilterMinEditionCount } - if allowed := models.ParseAllowedLanguages(p.AllowedLanguages); len(allowed) > 0 { - unknownFail := p.UnknownLanguageBehavior == models.UnknownLanguageFail - if !models.IsLanguageAllowed(b.Language, allowed, unknownFail) && !languageEvidenceAllows(ctx, meta, b, allowed) { - return profileFilterLanguage + // A blank language is unknown and passes here whatever the profile's + // unknown language behaviour says. Hardcover series catalogue entries + // carry no language at all, so honouring "fail" would reject every book + // of every series fill; the language is resolved later, on hydration or + // import. Same unknown passes rule as MinPages and SkipMissingISBN. + if allowed := models.ParseAllowedLanguages(p.AllowedLanguages); len(allowed) > 0 && strings.TrimSpace(b.Language) != "" { + if !models.IsLanguageAllowed(b.Language, allowed, false) { + ok, err := languageEvidenceAllows(ctx, meta, b, allowed) + if err != nil { + // No answer is not a "no": a lookup that failed (or was + // held back) does not reject the book. + slog.Debug("language evidence lookup failed; not enforcing the language filter for this book", + "title", b.Title, "foreignId", b.ForeignID, "error", err) + } else if !ok { + return profileFilterLanguage + } } } if (p.MinPages > 0 || p.SkipMissingISBN) && meta != nil && b.ForeignID != "" { @@ -115,17 +128,18 @@ func catalogBookProfileFilter(ctx context.Context, meta *metadata.Aggregator, p } // languageEvidenceAllows asks the provider whether any edition of b is in an -// allowed language. Only a definitive "allowed" answer counts; an error or an -// indeterminate answer leaves the scalar language's verdict standing. -func languageEvidenceAllows(ctx context.Context, meta *metadata.Aggregator, b *models.Book, allowed []string) bool { +// allowed language. Only a definitive "allowed" answer counts as true; an +// indeterminate answer, or a primary provider with no evidence to offer, +// leaves the book's own language verdict standing. A failed lookup is +// returned as an error so the caller can treat it as unknown. +func languageEvidenceAllows(ctx context.Context, meta *metadata.Aggregator, b *models.Book, allowed []string) (bool, error) { if meta == nil || b.ForeignID == "" { - return false + return false, nil } evidence, err := meta.GetAuthorWorkLanguageEvidence(ctx, []models.Book{*b}, allowed) if err != nil { - slog.Debug("language evidence lookup failed; using the book's own language", "title", b.Title, "error", err) - return false + return false, err } resolved, found := evidence[strings.TrimSpace(b.ForeignID)] - return found && resolved.State == metadata.AuthorWorkLanguageAllowed + return found && resolved.State == metadata.AuthorWorkLanguageAllowed, nil } diff --git a/internal/api/profile_filters_test.go b/internal/api/profile_filters_test.go new file mode 100644 index 000000000..81a2ff915 --- /dev/null +++ b/internal/api/profile_filters_test.go @@ -0,0 +1,66 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +// evidenceSeriesProvider is a primary provider that answers language evidence +// queries, the way Hardcover does, with a fixed state or a fixed error. +type evidenceSeriesProvider struct { + *stubSeriesProvider + state metadata.AuthorWorkLanguageEvidenceState + err error +} + +func (p *evidenceSeriesProvider) GetAuthorWorkLanguageEvidence(_ context.Context, books []models.Book, _ []string) (map[string]metadata.AuthorWorkLanguageEvidence, error) { + if p.err != nil { + return nil, p.err + } + out := make(map[string]metadata.AuthorWorkLanguageEvidence, len(books)) + for _, b := range books { + out[b.ForeignID] = metadata.AuthorWorkLanguageEvidence{State: p.state} + } + return out, nil +} + +// TestCatalogBookProfileFilterLanguage pins the language rules a series fill +// applies (#2208 review). Hardcover series catalogue entries carry no +// language, so a blank language must pass even under "unknown fails", or add +// all rejects every book whenever OpenLibrary is primary and no evidence can +// rescue it. A failed evidence lookup (an outage, or a held request) is no +// answer and must pass too; only a definitive "not allowed" rejects. +func TestCatalogBookProfileFilterLanguage(t *testing.T) { + ctx := context.Background() + profile := &models.MetadataProfile{AllowedLanguages: "eng", UnknownLanguageBehavior: models.UnknownLanguageFail} + openLibraryLike := metadata.NewAggregator(&stubSeriesProvider{}).WithAudnexClient(nil) + evidence := func(state metadata.AuthorWorkLanguageEvidenceState, err error) *metadata.Aggregator { + return metadata.NewAggregator(&evidenceSeriesProvider{stubSeriesProvider: &stubSeriesProvider{}, state: state, err: err}).WithAudnexClient(nil) + } + cases := []struct { + name string + meta *metadata.Aggregator + language string + want string + }{ + {"blank language, no evidence provider", openLibraryLike, "", ""}, + {"blank language, evidence lookup fails", evidence(0, errors.New("held")), "", ""}, + {"allowed language", openLibraryLike, "eng", ""}, + {"foreign language, no evidence provider", openLibraryLike, "spa", profileFilterLanguage}, + {"foreign language, evidence lookup fails", evidence(0, errors.New("held")), "spa", ""}, + {"foreign language, evidence says allowed", evidence(metadata.AuthorWorkLanguageAllowed, nil), "spa", ""}, + {"foreign language, evidence says not allowed", evidence(metadata.AuthorWorkLanguageNotAllowed, nil), "spa", profileFilterLanguage}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + b := &models.Book{ForeignID: "hc:book", Title: "A Book", Language: tc.language} + if got := catalogBookProfileFilter(ctx, tc.meta, profile, b); got != tc.want { + t.Fatalf("catalogBookProfileFilter = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/api/series.go b/internal/api/series.go index 10a87db63..92e68a06f 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -743,12 +743,20 @@ func (h *SeriesHandler) Fill(w http.ResponseWriter, r *http.Request) { // The same filters an author sync applies, judged from the // stored row (#2208). Without this, a fill put the very box // sets the profile screens out back on Wanted and monitored, - // including ones the user had unmonitored by hand. - if reason := storedBookProfileFilter(h.profileForAuthor(r.Context(), profiles, b.AuthorID), &b); reason != "" { - skippedByProfile++ - slog.Debug("series fill: not queueing a book the metadata profile filters out", - "seriesID", id, "bookID", b.ID, "title", b.Title, "filter", reason) - continue + // including ones the user had unmonitored by hand. Only an + // unmonitored row is screened: a monitored one is already + // being sought, very possibly because the user added or + // monitored it on purpose, and there is no stored signal that + // tells the two apart. Leaving it queued changes nothing the + // wanted sweep would not do anyway, and it is not reported as + // skipped on every fill. + if !b.Monitored { + if reason := storedBookProfileFilter(h.profileForAuthor(r.Context(), profiles, b.AuthorID), &b); reason != "" { + skippedByProfile++ + slog.Debug("series fill: not queueing a book the metadata profile filters out", + "seriesID", id, "bookID", b.ID, "title", b.Title, "filter", reason) + continue + } } } didQueue, queuedBook, err := h.queueSeriesBook(r.Context(), b) diff --git a/internal/api/series_profile_filters_test.go b/internal/api/series_profile_filters_test.go index 7adbacbd4..5fd7f6d4d 100644 --- a/internal/api/series_profile_filters_test.go +++ b/internal/api/series_profile_filters_test.go @@ -65,6 +65,9 @@ func profileSeriesFixture(t *testing.T, catalog *metadata.SeriesCatalog, searche } p.SkipPartBooks = true p.SkipMissingDate = true + // Hardcover series entries carry no language. Under "unknown fails" a + // fill must still create them; the language is resolved later. + p.UnknownLanguageBehavior = models.UnknownLanguageFail if err := profiles.Update(ctx, p); err != nil { t.Fatal(err) } @@ -105,6 +108,20 @@ func TestSeriesFillAppliesMetadataProfileFilters(t *testing.T) { if err := seriesRepo.LinkBook(ctx, series.ID, boxSet.ID, "4.5", true); err != nil { t.Fatal(err) } + // An undated novella the user added and monitored by hand. It is already + // being sought, so fill queues it as before and does not report it as + // skipped on every run. + handPicked := &models.Book{ + ForeignID: "hc:the-churn", AuthorID: author.ID, Title: "The Churn", SortTitle: "The Churn", + Status: models.BookStatusWanted, Monitored: true, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "hardcover", + } + if err := bookRepo.Create(ctx, handPicked); err != nil { + t.Fatal(err) + } + if err := seriesRepo.LinkBook(ctx, series.ID, handPicked.ID, "3.5", true); 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))) @@ -141,8 +158,8 @@ func TestSeriesFillAppliesMetadataProfileFilters(t *testing.T) { if stored.Monitored { t.Error("fill re-monitored a stored box set the metadata profile filters out") } - if body["queued"] != 2 || body["skippedByProfile"] != 3 { - t.Errorf("response = %+v, want queued 2 and skippedByProfile 3", body) + if body["queued"] != 3 || body["skippedByProfile"] != 3 { + t.Errorf("response = %+v, want queued 3 (two created, the hand picked novella) and skippedByProfile 3", body) } // The per row add names one book on purpose and is not screened.