diff --git a/changelog.d/2554-book-series.md b/changelog.d/2554-book-series.md new file mode 100644 index 000000000..21c8ac743 --- /dev/null +++ b/changelog.d/2554-book-series.md @@ -0,0 +1,10 @@ +### Added +- **Fix a book's series from the book** (#2554). Edit metadata has a Series field, limited to the author's series, with a position and New series… to create one, so a book filed under the wrong series or missing its number can be fixed without going through the series page. No series takes the book out of every series it is in. Admin only. +- **Removed stays removed** (#2554). A book taken out of a series is no longer put back by the next refresh. The book page lists the series it was taken out of, with Restore, and Unlock all fields hands the book's series back to refresh. + +### Fixed +- **One primary series per book** (#2525). Adding a book to a series as primary now makes it the book's only primary series. +- **Position is optional** when adding a book to a series, as it already was on the server. +- **Series controls for other users.** Users who aren't admins no longer see series buttons that the server refuses (add, rename, merge, delete, fill, Hardcover link, a book's series in Edit metadata). They still see every series and what is missing. +- **Series grouping after a refresh.** The author page regroups its books by series after a refresh, without a reload. +- **Add on a removed book** (#2554). Clicking Add on a book you took out of a series, in the Hardcover missing list, now puts it back in that series. Before, it said it queued the book and left it missing. Fill all still leaves removed books out. diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index f3a63ce0f..2b02ea2cc 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -1059,6 +1059,7 @@ func main() { r.Delete("/book/{id}", bookHandler.Delete) r.Delete("/book/{id}/file", bookHandler.DeleteFile) r.Put("/book/{id}/exclude", bookHandler.ToggleExcluded) + r.Get("/book/{id}/series-exclusions", bookHandler.SeriesExclusions) r.Post("/book/{id}/rebind", bookHandler.Rebind) r.Post("/book/{id}/enrich-audiobook", bookHandler.EnrichAudiobook) r.Post("/book/{id}/search", indexerHandler.SearchBook) diff --git a/docs/API.md b/docs/API.md index c775f81f4..488b0e588 100644 --- a/docs/API.md +++ b/docs/API.md @@ -290,6 +290,7 @@ PUT /api/v1/book/{id} update monitor / status / meta DELETE /api/v1/book/{id} remove from library DELETE /api/v1/book/{id}/file delete imported file(s) on disk (`?format=ebook|audiobook` scopes to one format; `?path=…` deregisters one tracked path WITHOUT deleting anything on disk) PUT /api/v1/book/{id}/exclude exclude from future searches +GET /api/v1/book/{id}/series-exclusions the series the book was taken out of: [{seriesForeignId, seriesId, seriesTitle, position}]; seriesId is 0 when nothing resolves the id any more POST /api/v1/book/{id}/rebind re-link to a different metadata record POST /api/v1/book/{id}/enrich-audiobook pull narrator/duration/cover from Audnex POST /api/v1/book/{id}/search manual indexer search @@ -306,8 +307,17 @@ POST /api/v1/series/{id}/fill add the series' missing books 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) +POST /api/v1/series/{id}/books add a book or update its position: {"bookId":1,"positionInSeries":"2","primarySeries":true}; positionInSeries is optional (admin) +DELETE /api/v1/series/{id}/books/{bookId} take a book out of the series, and keep it out on refresh (admin) ``` +Taking a book out of a series is remembered (#2554): a refresh that still +reports the book in that series, under its own provider id or one merged into +it, leaves it out. Adding the book back with `POST /series/{id}/books` forgets +that, and so does an admin unlocking all of the book's fields (`PUT /book/{id}` +with `"lockedFields": []`; another user's unlock leaves the book's series alone). Adding a book as primary makes it the book's only +primary series. `GET /book/{id}/series-exclusions` lists what was taken out. + `GET /series` returns the bare array it always has. Pagination is opt-in (#2345): pass `limit` and/or `offset` and you get an `{items, total, limit, offset}` envelope instead, which is what a large catalogue wants, since the diff --git a/docs/Hardcover-Series-Wiki.md b/docs/Hardcover-Series-Wiki.md index 4fd778be8..1a20c44ef 100644 --- a/docs/Hardcover-Series-Wiki.md +++ b/docs/Hardcover-Series-Wiki.md @@ -111,6 +111,12 @@ The dialog previews every change, including books whose positions differ, before Undoing an import (ABS, Calibre) made before a merge does not take the merged books back out of the series you kept. +## Fixing One Book's Series + +When one book is filed under the wrong series, or is missing its number, fix it from the book: **Edit metadata** has a **Series** field with the author's series, a position, and **New series…** to create one. Picking a series makes it the book's primary series and takes the book out of the one it was filed under; **No series** takes it out of every series it is in. A position is optional. + +A refresh does not put a book back into a series you took it out of, here or from the series page, even when the provider still lists it there or the series was merged into another. The book page lists those series with **Restore**, and **Unlock all fields** in Edit metadata hands the book's series back to refresh. Changing series is for admins. + ## Known Behavior - Hardcover-backed controls require outbound HTTPS access to Hardcover. diff --git a/internal/api/book_series_exclusions.go b/internal/api/book_series_exclusions.go new file mode 100644 index 000000000..f4a4e691a --- /dev/null +++ b/internal/api/book_series_exclusions.go @@ -0,0 +1,23 @@ +package api + +import "net/http" + +// SeriesExclusions lists the series the user took this book out of (#2554): +// GET /book/{id}/series-exclusions. Each names the series as it is now, and +// the position the book had, so the book page can show it and offer Restore. +func (h *BookHandler) SeriesExclusions(w http.ResponseWriter, r *http.Request) { + book, ok := h.loadOwnedBook(w, r) + if !ok { + return + } + if h.series == nil { + writeJSON(w, http.StatusOK, []any{}) + return + } + list, err := h.series.ListBookSeriesExclusions(r.Context(), book.ID) + if err != nil { + writeServerError(w, r, err) + return + } + writeJSON(w, http.StatusOK, list) +} diff --git a/internal/api/book_series_exclusions_test.go b/internal/api/book_series_exclusions_test.go new file mode 100644 index 000000000..1e621b1b3 --- /dev/null +++ b/internal/api/book_series_exclusions_test.go @@ -0,0 +1,159 @@ +package api + +import ( + "bytes" + "context" + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/vavallee/bindery/internal/auth" + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +// The book page lists the series a book was taken out of, and Unlock all +// fields forgets them; any other edit leaves them alone (#2554). +// exclusionsEnv is a book handler with series wired, one book and the +// database, for tests that inject failures with a temporary trigger. +func exclusionsEnv(t *testing.T) (*BookHandler, *db.SeriesRepo, *models.Book, *sql.DB, context.Context) { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + books, series := db.NewBookRepo(database), db.NewSeriesRepo(database) + author := &models.Author{ForeignID: "OL1A", Name: "Kari Nordmann", SortName: "Nordmann, Kari", MetadataProvider: "openlibrary"} + if err := db.NewAuthorRepo(database).Create(ctx, author); err != nil { + t.Fatal(err) + } + h := NewBookHandler(books, nil, db.NewHistoryRepo(database), nil).WithSeries(series) + book := &models.Book{ForeignID: "B1", AuthorID: author.ID, Title: "Fjellvinden", SortTitle: "fjellvinden", + Status: "wanted", Genres: []string{}, MetadataProvider: "openlibrary"} + if err := books.Create(ctx, book); err != nil { + t.Fatal(err) + } + return h, series, book, database, ctx +} + +func TestBookSeriesExclusionsEndpointAndUnlockAll(t *testing.T) { + h, series, book, _, ctx := exclusionsEnv(t) + s := &models.Series{ForeignID: "s:fjell", Title: "Fjellserien"} + if err := series.CreateOrGet(ctx, s); err != nil { + t.Fatal(err) + } + if err := series.LinkBook(ctx, s.ID, book.ID, "2", true); err != nil { + t.Fatal(err) + } + if _, err := series.RemoveBookFromSeries(ctx, s.ID, book.ID); err != nil { + t.Fatal(err) + } + id := strconv.FormatInt(book.ID, 10) + + list := func() []db.BookSeriesExclusion { + t.Helper() + rec := httptest.NewRecorder() + h.SeriesExclusions(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/book/"+id+"/series-exclusions", nil), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("list: %d %s", rec.Code, rec.Body.String()) + } + var out []db.BookSeriesExclusion + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatal(err) + } + return out + } + update := func(role, body string) { + t.Helper() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPut, "/api/v1/book/"+id, bytes.NewBufferString(body)) + h.Update(rec, withURLParam(req.WithContext(auth.WithUserRole(req.Context(), role)), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("update %s: %d %s", body, rec.Code, rec.Body.String()) + } + } + + if got := list(); len(got) != 1 || got[0].SeriesID != s.ID || got[0].SeriesTitle != "Fjellserien" || got[0].Position != "2" { + t.Fatalf("exclusions = %+v, want Fjellserien at 2", got) + } + update(auth.RoleAdmin, `{"title":"Fjellvinden (ny)"}`) + if got := list(); len(got) != 1 { + t.Fatalf("an ordinary edit cleared the exclusions: %+v", got) + } + // Series changes are admin only: another user's Unlock all unlocks the + // fields and leaves the book's series as they are. + update(auth.RoleUser, `{"lockedFields":[]}`) + if got := list(); len(got) != 1 { + t.Fatalf("a non admin's Unlock all cleared the exclusions: %+v", got) + } + update(auth.RoleAdmin, `{"lockedFields":[]}`) + if got := list(); len(got) != 0 { + t.Errorf("after Unlock all = %+v, want none", got) + } +} + +func TestBookSeriesExclusionsEdges(t *testing.T) { + h, _, book, _, _ := exclusionsEnv(t) + get := func(h *BookHandler, id string) *httptest.ResponseRecorder { + rec := httptest.NewRecorder() + h.SeriesExclusions(rec, withURLParam(httptest.NewRequest(http.MethodGet, "/api/v1/book/"+id+"/series-exclusions", nil), "id", id)) + return rec + } + if rec := get(h, "999"); rec.Code != http.StatusNotFound { + t.Errorf("unknown book: %d, want 404", rec.Code) + } + // A handler without series wiring has nothing to report. + bare := NewBookHandler(h.books, nil, nil, nil) + if rec := get(bare, strconv.FormatInt(book.ID, 10)); rec.Code != http.StatusOK || rec.Body.String() != "[]\n" { + t.Errorf("no series repo: %d %q, want an empty list", rec.Code, rec.Body.String()) + } +} + +// When forgetting the exclusions fails, Unlock all reports it instead of +// answering as if the book had been handed back to refresh. +func TestUnlockAllReportsAFailedExclusionClear(t *testing.T) { + h, _, book, database, _ := exclusionsEnv(t) + if _, err := database.Exec(`CREATE TEMP TRIGGER fail_clear BEFORE DELETE ON book_series_exclusions BEGIN SELECT RAISE(ABORT, 'injected'); END`); err != nil { + t.Fatal(err) + } + if _, err := database.Exec(`INSERT INTO book_series_exclusions (book_id, series_foreign_id) VALUES (?, 's:x')`, book.ID); err != nil { + t.Fatal(err) + } + id := strconv.FormatInt(book.ID, 10) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPut, "/api/v1/book/"+id, bytes.NewBufferString(`{"lockedFields":[]}`)) + h.Update(rec, withURLParam(req.WithContext(auth.WithUserRole(req.Context(), auth.RoleAdmin)), "id", id)) + if rec.Code != http.StatusInternalServerError { + t.Errorf("status = %d %s, want 500", rec.Code, rec.Body.String()) + } +} + +// A removal that fails is a 500, and leaves the book in the series. +func TestRemoveBookReportsAFailedRemoval(t *testing.T) { + _, series, book, database, ctx := exclusionsEnv(t) + s := &models.Series{ForeignID: "s:fjell", Title: "Fjellserien"} + if err := series.CreateOrGet(ctx, s); err != nil { + t.Fatal(err) + } + if err := series.LinkBook(ctx, s.ID, book.ID, "1", true); err != nil { + t.Fatal(err) + } + if _, err := database.Exec(`CREATE TEMP TRIGGER fail_unlink BEFORE DELETE ON series_books BEGIN SELECT RAISE(ABORT, 'injected'); END`); err != nil { + t.Fatal(err) + } + sh := NewSeriesHandler(series, db.NewBookRepo(database), db.NewAuthorRepo(database), nil, nil) + sid, bid := strconv.FormatInt(s.ID, 10), strconv.FormatInt(book.ID, 10) + rec := httptest.NewRecorder() + sh.RemoveBook(rec, withURLParams(httptest.NewRequest(http.MethodDelete, "/api/v1/series/"+sid+"/books/"+bid, nil), map[string]string{"id": sid, "bookId": bid})) + if rec.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want 500", rec.Code) + } + if ids, _ := series.GetSeriesIDsForBook(ctx, book.ID); len(ids) != 1 { + t.Errorf("series ids = %v, want the book still in its series", ids) + } +} diff --git a/internal/api/books.go b/internal/api/books.go index 34adee7f8..2b027d125 100644 --- a/internal/api/books.go +++ b/internal/api/books.go @@ -596,6 +596,16 @@ func (h *BookHandler) Update(w http.ResponseWriter, r *http.Request) { writeServerError(w, r, err) return } + // Unlocking every field hands the book back to metadata refresh, and that + // includes its series: forget the series the user took it out of (#2554). + // Series changes are admin only, so another user's unlock leaves them be. + if req.LockedFields != nil && len(book.LockedFields) == 0 && h.series != nil && + auth.UserRoleFromContext(r.Context()) == auth.RoleAdmin { + if err := h.series.ClearBookSeriesExclusions(r.Context(), book.ID); err != nil { + writeServerError(w, r, err) + return + } + } // Fire an immediate indexer search when this write leaves the book wanted // and monitored and it was not both before (e.g. "Delete file" flips diff --git a/internal/api/series.go b/internal/api/series.go index c2d7fbd36..e2fdfb0fa 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -499,7 +499,9 @@ func (h *SeriesHandler) RemoveBook(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusNotFound, map[string]string{"error": "series not found"}) return } - if err := h.series.UnlinkBook(r.Context(), id, bookID); err != nil { + // The user is taking the book out of this series, so it is recorded: a + // refresh that still reports the book in it leaves it out (#2554). + if _, err := h.series.RemoveBookFromSeries(r.Context(), id, bookID); err != nil { writeServerError(w, r, err) return } @@ -1902,7 +1904,7 @@ func (h *SeriesHandler) createMissingHardcoverBooks(ctx context.Context, seriesI if !ok { continue } - if _, err := h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, format, true); err != nil { + if _, err := h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, format, true, false); err != nil { if errors.Is(err, errSeriesBookFilteredByProfile) { filtered++ continue @@ -1960,7 +1962,7 @@ func (h *SeriesHandler) createMissingHardcoverBook(ctx context.Context, seriesID if !ok { return nil, errSeriesCatalogBookNotFound } - return h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, selector.format(), false) + return h.ensureHardcoverCatalogBook(ctx, series, catalog.AuthorName, catalogBook, selector.format(), false, true) } return nil, errSeriesCatalogBookNotFound } @@ -1999,10 +2001,25 @@ func findCatalogBook(books []metadata.SeriesCatalogBook, foreignID, position str // 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) { +// +// explicit marks the user asking for this one book (the Add button), as +// opposed to a bulk fill. It forgets a record of the user having taken the +// book out of this series before linking it, the way adding by hand does; +// without it the exclusion silently refused the link and the book stayed +// missing for good (#2554). A bulk fill keeps honouring the exclusion. +func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series *models.Series, fallbackAuthor string, catalogBook metadata.SeriesCatalogBook, format requestedFormat, applyProfile, explicit bool) (*models.Book, error) { if series == nil { return nil, errSeriesNotFound } + link := func(target *models.Book) error { + if explicit { + if err := h.series.ClearBookSeriesExclusion(ctx, series.ID, target.ID); err != nil { + return err + } + } + _, err := h.series.LinkBookPreservingPrimary(ctx, series.ID, target.ID, catalogBook.Position) + return err + } // A series catalog is a metadata ingestion source like an author // catalogue, so the unconditional bundle prune (#1780) has to apply here // too. It did not, and a fill created " 3 Books Collection Set" @@ -2041,8 +2058,7 @@ func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series * "matchedForeignID", existing.ForeignID, "matchedStatus", existing.Status, "matchedBy", "identity") - _, err := h.series.LinkBookPreservingPrimary(ctx, series.ID, existing.ID, catalogBook.Position) - return existing, err + return existing, link(existing) } author := book.Author @@ -2206,8 +2222,7 @@ func (h *SeriesHandler) ensureHardcoverCatalogBook(ctx context.Context, series * "matchedStatus", best.Status, "matchedBy", "title", "score", bestScore) - _, err := h.series.LinkBookPreservingPrimary(ctx, series.ID, best.ID, catalogBook.Position) - return best, err + return best, link(best) } if blockedByExcludedTitle { return nil, nil diff --git a/internal/api/series_fill_excluded_test.go b/internal/api/series_fill_excluded_test.go new file mode 100644 index 000000000..cd33d1389 --- /dev/null +++ b/internal/api/series_fill_excluded_test.go @@ -0,0 +1,101 @@ +package api + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +// seriesFillExcludedFixture files the Way of Kings under a Hardcover linked +// series, takes it out the way the book page does, and returns what a test +// needs to fill the series again (#2554). +func seriesFillExcludedFixture(t *testing.T) (*SeriesHandler, *db.SeriesRepo, *models.Series, *models.Book) { + t.Helper() + catalog := stormlightCatalog() + h, seriesRepo, authorRepo, bookRepo := seriesFixtureWithProvider(t, &stubSeriesProvider{ + catalogs: map[string]*metadata.SeriesCatalog{catalog.ForeignID: catalog}, + }, newMockBookSearcher()) + ctx := context.Background() + series := &models.Series{ForeignID: "ol-series:stormlight", Title: "The Stormlight Archive"} + if err := seriesRepo.Create(ctx, series); err != nil { + t.Fatal(err) + } + if err := seriesRepo.UpsertHardcoverLink(ctx, &models.SeriesHardcoverLink{ + SeriesID: series.ID, HardcoverSeriesID: catalog.ForeignID, HardcoverProviderID: catalog.ProviderID, + HardcoverTitle: catalog.Title, HardcoverAuthorName: catalog.AuthorName, HardcoverBookCount: catalog.BookCount, + Confidence: 1, LinkedBy: "manual", + }); err != nil { + t.Fatal(err) + } + 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) + } + book := &models.Book{ + ForeignID: "hc:the-way-of-kings", AuthorID: author.ID, Title: "The Way of Kings", SortTitle: "The Way of Kings", + Status: models.BookStatusImported, Monitored: true, Genres: []string{}, MetadataProvider: "hardcover", + } + if err := bookRepo.Create(ctx, book); err != nil { + t.Fatal(err) + } + if _, err := seriesRepo.LinkBookIfMissing(ctx, series.ID, book.ID, "1", true); err != nil { + t.Fatal(err) + } + if removed, err := seriesRepo.RemoveBookFromSeries(ctx, series.ID, book.ID); err != nil || !removed { + t.Fatalf("remove from series: removed=%v err=%v", removed, err) + } + return h, seriesRepo, series, book +} + +func seriesFillMembers(t *testing.T, repo *db.SeriesRepo, seriesID int64) int { + t.Helper() + books, err := repo.ListBooksInSeries(context.Background(), seriesID) + if err != nil { + t.Fatal(err) + } + return len(books) +} + +// An explicit Add of one book the user took out of the series is the user +// asking for it back, so it rejoins the series and the exclusion is gone. +// The bulk fill must keep honouring the exclusion (#2554). +func TestSeriesFillSingleBookAddRejoinsExcludedBook(t *testing.T) { + h, seriesRepo, series, book := seriesFillExcludedFixture(t) + id := strconv.FormatInt(series.ID, 10) + + rec := httptest.NewRecorder() + h.Fill(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/1/fill", bytes.NewBufferString(`{"foreignBookId":"hc:the-way-of-kings"}`)), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if got := seriesFillMembers(t, seriesRepo, series.ID); got != 1 { + t.Fatalf("explicit Add left the book out of the series (members=%d, response %s)", got, rec.Body.String()) + } + if left, err := seriesRepo.ListBookSeriesExclusions(context.Background(), book.ID); err != nil || len(left) != 0 { + t.Fatalf("exclusion survived an explicit Add: %+v err=%v", left, err) + } +} + +func TestSeriesFillBulkKeepsExcludedBookOut(t *testing.T) { + h, seriesRepo, series, book := seriesFillExcludedFixture(t) + id := strconv.FormatInt(series.ID, 10) + + rec := httptest.NewRecorder() + h.Fill(rec, withURLParam(httptest.NewRequest(http.MethodPost, "/api/v1/series/1/fill", nil), "id", id)) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if got := seriesFillMembers(t, seriesRepo, series.ID); got != 0 { + t.Fatalf("bulk fill put an excluded book back (members=%d)", got) + } + if left, err := seriesRepo.ListBookSeriesExclusions(context.Background(), book.ID); err != nil || len(left) != 1 { + t.Fatalf("bulk fill cleared the exclusion: %+v err=%v", left, err) + } +} diff --git a/internal/api/series_merge_refresh_test.go b/internal/api/series_merge_refresh_test.go index e147162b1..e8821e8c6 100644 --- a/internal/api/series_merge_refresh_test.go +++ b/internal/api/series_merge_refresh_test.go @@ -2,6 +2,9 @@ package api import ( "context" + "net/http" + "net/http/httptest" + "strconv" "testing" "github.com/vavallee/bindery/internal/models" @@ -91,3 +94,32 @@ func TestCatalogueSync_MonitorsBooksReportedUnderAMergedSeriesID(t *testing.T) { t.Errorf("links = %+v, want only Fjellserien", links) } } + +// A book the user takes out of a series stays out: a refresh that still +// reports it in that series does not put it back (#2554). +func TestCatalogueSync_KeepsABookTheUserRemovedFromASeriesOut(t *testing.T) { + f := newSeriesLinkFixture(t, false) + ctx := context.Background() + book := f.addImportedBook(t, "OL1W", "Fjellvinden") + s := &models.Series{ForeignID: "OL-S-IMPERIAL-RADCH", Title: "Fjellserien"} + if err := f.series.CreateOrGet(ctx, s); err != nil { + t.Fatal(err) + } + if err := f.series.LinkBook(ctx, s.ID, book.ID, "1", true); err != nil { + t.Fatal(err) + } + h := NewSeriesHandler(f.series, f.books, f.authors, nil, nil) + id := strconv.FormatInt(s.ID, 10) + rec := httptest.NewRecorder() + h.RemoveBook(rec, withURLParams(httptest.NewRequest(http.MethodDelete, "/api/v1/series/"+id+"/books/"+strconv.FormatInt(book.ID, 10), nil), + map[string]string{"id": id, "bookId": strconv.FormatInt(book.ID, 10)})) + if rec.Code != http.StatusNoContent { + t.Fatalf("remove: %d %s", rec.Code, rec.Body.String()) + } + + // seriesWork reports the book in that series again. + refreshCatalogue(t, f.handler(&stubMetaProvider{works: []models.Book{seriesWork("OL1W", "Fjellvinden", "1")}}), f.author) + if links := linksForBook(t, f, book.ID); len(links) != 0 { + t.Errorf("links = %+v, want the book kept out of the series it was removed from", links) + } +} diff --git a/internal/db/migrations/102_book_series_exclusions.sql b/internal/db/migrations/102_book_series_exclusions.sql new file mode 100644 index 000000000..486709b34 --- /dev/null +++ b/internal/db/migrations/102_book_series_exclusions.sql @@ -0,0 +1,21 @@ +-- +migrate Up +-- A book the user took out of a series. Keyed by the series' provider foreign +-- id rather than its row, so the record holds when that series is merged away +-- (its id becomes an alias) or deleted and recreated by a later refresh. +-- Linking the book back by hand clears it, and so does unlocking all of the +-- book's fields. The position the book had is kept so it can be restored. +-- +-- IF NOT EXISTS so the table can be created again without failing: this +-- migration ran on custom builds before it merged, and should it be +-- renumbered, the renumbered copy must not stop those databases starting. +CREATE TABLE IF NOT EXISTS book_series_exclusions ( + book_id INTEGER NOT NULL REFERENCES books(id) ON DELETE CASCADE, + series_foreign_id TEXT NOT NULL, + position_in_series TEXT NOT NULL DEFAULT '', + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (book_id, series_foreign_id) +); + +-- +migrate Down + +DROP TABLE IF EXISTS book_series_exclusions; diff --git a/internal/db/series.go b/internal/db/series.go index 1f7dd21db..b4b1f98d9 100644 --- a/internal/db/series.go +++ b/internal/db/series.go @@ -788,6 +788,64 @@ func (r *SeriesRepo) AliasForeignIDs(ctx context.Context, seriesIDs []int64) ([] return out, rows.Err() } +// BookSeriesExclusion is a series the user took a book out of (#2554), with +// the series it names now (following merge aliases; 0 and "" when it no +// longer exists) and the position the book had in it. +type BookSeriesExclusion struct { + SeriesForeignID string `json:"seriesForeignId"` + SeriesID int64 `json:"seriesId"` + SeriesTitle string `json:"seriesTitle"` + Position string `json:"position"` +} + +// ListBookSeriesExclusions lists the series a book is kept out of. +func (r *SeriesRepo) ListBookSeriesExclusions(ctx context.Context, bookID int64) ([]BookSeriesExclusion, error) { + rows, err := r.exec.QueryContext(ctx, ` + SELECT e.series_foreign_id, COALESCE(s.id, 0), COALESCE(s.title, ''), e.position_in_series + FROM book_series_exclusions e + LEFT JOIN series s ON s.id = ( + SELECT id FROM series WHERE foreign_id = e.series_foreign_id + UNION ALL SELECT series_id FROM series_aliases WHERE foreign_id = e.series_foreign_id + LIMIT 1) + WHERE e.book_id = ? + ORDER BY COALESCE(s.title, e.series_foreign_id)`, bookID) + if err != nil { + return nil, fmt.Errorf("list series exclusions for book %d: %w", bookID, err) + } + defer rows.Close() + out := []BookSeriesExclusion{} + for rows.Next() { + var e BookSeriesExclusion + if err := rows.Scan(&e.SeriesForeignID, &e.SeriesID, &e.SeriesTitle, &e.Position); err != nil { + return nil, err + } + out = append(out, e) + } + return out, rows.Err() +} + +// ClearBookSeriesExclusions forgets every series the user took the book out +// of, so refreshes file it as the provider reports again. +func (r *SeriesRepo) ClearBookSeriesExclusions(ctx context.Context, bookID int64) error { + if _, err := r.exec.ExecContext(ctx, `DELETE FROM book_series_exclusions WHERE book_id = ?`, bookID); err != nil { + return fmt.Errorf("clear series exclusions for book %d: %w", bookID, err) + } + return nil +} + +// ClearBookSeriesExclusion forgets that the user took the book out of this one +// series (by its foreign id or any id merged into it), so the next link is not +// refused. It is for an explicit request to put the book back; every +// automatic writer leaves the exclusion alone (#2554). +func (r *SeriesRepo) ClearBookSeriesExclusion(ctx context.Context, seriesID, bookID int64) error { + if _, err := r.exec.ExecContext(ctx, `DELETE FROM book_series_exclusions WHERE book_id = ? AND series_foreign_id IN ( + SELECT foreign_id FROM series WHERE id = ? + UNION SELECT foreign_id FROM series_aliases WHERE series_id = ?)`, bookID, seriesID, seriesID); err != nil { + return fmt.Errorf("clear series exclusion for book %d in series %d: %w", bookID, seriesID, err) + } + return nil +} + // ErrSeriesAlias reports a foreign id that names a series through a merge // alias and so cannot be given to another series. var ErrSeriesAlias = errors.New("series foreign id is an alias") @@ -803,6 +861,11 @@ func (r *SeriesRepo) LinkBook(ctx context.Context, seriesID, bookID int64, posit // LinkBookIfMissing inserts a series_books row and reports whether it created // the membership. Callers that record rollback ownership should only claim // ownership when this returns true. +// +// It is how every automatic path (author sync and refresh, imports, list +// sync, series fill) files a book under a series, so it is also where a +// book the user took out of that series stays out (#2554): the exclusion is +// checked inside the INSERT, by the series' foreign id and its merge aliases. func (r *SeriesRepo) LinkBookIfMissing(ctx context.Context, seriesID, bookID int64, position string, primary bool) (bool, error) { primaryInt := 0 if primary { @@ -810,8 +873,9 @@ func (r *SeriesRepo) LinkBookIfMissing(ctx context.Context, seriesID, bookID int } result, err := r.db.ExecContext(ctx, `INSERT OR IGNORE INTO series_books (series_id, book_id, position_in_series, primary_series) - VALUES (?, ?, ?, ?)`, - seriesID, bookID, position, primaryInt) + SELECT ?, ?, ?, ? + WHERE NOT EXISTS (`+bookExcludedFromSeriesSQL+`)`, + seriesID, bookID, position, primaryInt, seriesID, bookID) if err != nil { return false, fmt.Errorf("link book %d to series %d: %w", bookID, seriesID, err) } @@ -851,24 +915,88 @@ func (r *SeriesRepo) UpdateBookLinkPosition(ctx context.Context, seriesID, bookI return nil } +// bookExcludedFromSeriesSQL is true when the user took the book out of the +// series (#2554), recorded under the series' foreign id or any id merged into +// it. Takes the series id, then the book id. +const bookExcludedFromSeriesSQL = `SELECT 1 FROM book_series_exclusions e + JOIN series s ON s.id = ? + WHERE e.book_id = ? + AND (e.series_foreign_id = s.foreign_id + OR e.series_foreign_id IN (SELECT a.foreign_id FROM series_aliases a WHERE a.series_id = s.id))` + +// UpsertBookLink files a book under a series at a position because the user +// said so: adding it on the series page, or setting its series in the edit +// dialog. It clears any record of the user having taken the book out of that +// series, and when the link is primary it demotes the book's other series, +// so the book keeps exactly one primary series (#2525). func (r *SeriesRepo) UpsertBookLink(ctx context.Context, seriesID, bookID int64, position string, primary bool) error { - primaryInt := 0 - if primary { - primaryInt = 1 + tx, err := r.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("upsert book %d in series %d: %w", bookID, seriesID, err) } - _, err := r.db.ExecContext(ctx, ` - INSERT INTO series_books (series_id, book_id, position_in_series, primary_series) + defer func() { _ = tx.Rollback() }() + steps := []struct { + query string + args []any + }{ + {`DELETE FROM book_series_exclusions WHERE book_id = ? AND series_foreign_id IN ( + SELECT foreign_id FROM series WHERE id = ? + UNION SELECT foreign_id FROM series_aliases WHERE series_id = ?)`, []any{bookID, seriesID, seriesID}}, + {`INSERT INTO series_books (series_id, book_id, position_in_series, primary_series) VALUES (?, ?, ?, ?) ON CONFLICT(series_id, book_id) DO UPDATE SET position_in_series = excluded.position_in_series, - primary_series = excluded.primary_series`, - seriesID, bookID, strings.TrimSpace(position), primaryInt) - if err != nil { + primary_series = excluded.primary_series`, []any{seriesID, bookID, strings.TrimSpace(position), boolToInt(primary)}}, + } + if primary { + steps = append(steps, struct { + query string + args []any + }{`UPDATE series_books SET primary_series = 0 WHERE book_id = ? AND series_id != ?`, []any{bookID, seriesID}}) + } + for _, st := range steps { + if _, err := tx.ExecContext(ctx, st.query, st.args...); err != nil { + return fmt.Errorf("upsert book %d in series %d: %w", bookID, seriesID, err) + } + } + if err := tx.Commit(); err != nil { return fmt.Errorf("upsert book %d in series %d: %w", bookID, seriesID, err) } return nil } +// RemoveBookFromSeries takes a book out of a series because the user said so, +// and records that under the series' foreign id (#2554), so a refresh or an +// import that still reports the book in that series does not put it back. +// Rollbacks and rebinds use UnlinkBook, which records nothing. Reports false +// when the book was not in the series. +func (r *SeriesRepo) RemoveBookFromSeries(ctx context.Context, seriesID, bookID int64) (bool, error) { + tx, err := r.db.BeginTx(ctx, nil) + if err != nil { + return false, fmt.Errorf("remove book %d from series %d: %w", bookID, seriesID, err) + } + defer func() { _ = tx.Rollback() }() + // Record the exclusion, with the position the book had, before the + // membership it is read from goes. + if _, err := tx.ExecContext(ctx, `INSERT OR REPLACE INTO book_series_exclusions (book_id, series_foreign_id, position_in_series) + SELECT sb.book_id, s.foreign_id, sb.position_in_series + FROM series_books sb JOIN series s ON s.id = sb.series_id + WHERE sb.series_id = ? AND sb.book_id = ? AND s.foreign_id != ''`, seriesID, bookID); err != nil { + return false, fmt.Errorf("record book %d left series %d: %w", bookID, seriesID, err) + } + res, err := tx.ExecContext(ctx, `DELETE FROM series_books WHERE series_id = ? AND book_id = ?`, seriesID, bookID) + if err != nil { + return false, fmt.Errorf("remove book %d from series %d: %w", bookID, seriesID, err) + } + if n, _ := res.RowsAffected(); n == 0 { + return false, nil + } + if err := tx.Commit(); err != nil { + return false, fmt.Errorf("remove book %d from series %d: %w", bookID, seriesID, err) + } + return true, nil +} + func (r *SeriesRepo) UnlinkBook(ctx context.Context, seriesID, bookID int64) error { _, err := r.exec.ExecContext(ctx, `DELETE FROM series_books WHERE series_id = ? AND book_id = ?`, seriesID, bookID) if err != nil { diff --git a/internal/db/series_aliases_test.go b/internal/db/series_aliases_test.go index 73179a543..e409d3bb6 100644 --- a/internal/db/series_aliases_test.go +++ b/internal/db/series_aliases_test.go @@ -126,7 +126,9 @@ func TestSeriesForeignIDLookupsGoThroughResolver(t *testing.T) { literal := regexp.MustCompile("(?s)`[^`]*`|\"(?:[^\"\\\\]|\\\\.)*\"") seriesTable := regexp.MustCompile(`(?i)\b(?:FROM|JOIN|UPDATE|INTO)\s+series(?:[\s),]|$)`) assignment := regexp.MustCompile(`(?i)\bSET\s+foreign_id\s*=`) - compares := regexp.MustCompile(`(?i)\bforeign_id\s*(?:=|!=|<>|\bIN\b|\bLIKE\b|\bIS\b)`) + // A lookup is equality, IN or LIKE; foreign_id != '' only filters out + // series without one. + compares := regexp.MustCompile(`(?i)\bforeign_id\s*(?:=|\bIN\b|\bLIKE\b)`) files, err := filepath.Glob("*.go") if err != nil { diff --git a/internal/db/series_exclusions_test.go b/internal/db/series_exclusions_test.go new file mode 100644 index 000000000..c9e9e9d58 --- /dev/null +++ b/internal/db/series_exclusions_test.go @@ -0,0 +1,272 @@ +package db + +import ( + "context" + "fmt" + "strings" + "testing" +) + +// A book the user took out of a series stays out: no automatic path files it +// back under that series (#2554), until the user adds it back by hand. +func TestBookSeriesExclusion(t *testing.T) { + f := newMergeFixture(t, 1) + s := f.newSeries("s:fjell", "Fjellserien") + f.link(s, 1, "1", true) + + removed, err := f.series.RemoveBookFromSeries(f.ctx, s, f.books[0]) + if err != nil || !removed { + t.Fatalf("RemoveBookFromSeries = %v, %v", removed, err) + } + created, err := f.series.LinkBookIfMissing(f.ctx, s, f.books[0], "1", true) + if err != nil || created { + t.Fatalf("an automatic link after the user removed the book = %v, %v; want skipped", created, err) + } + if created, err := f.series.LinkBookPreservingPrimary(f.ctx, s, f.books[0], "1"); err != nil || created { + t.Fatalf("LinkBookPreservingPrimary = %v, %v; want skipped", created, err) + } + if got := f.membership(s); len(got) != 0 { + t.Fatalf("membership = %v, want the book kept out", got) + } + + if err := f.series.UpsertBookLink(f.ctx, s, f.books[0], "2", true); err != nil { + t.Fatal(err) + } + if got := f.membership(s); fmt.Sprint(got) != "map[1:2/P]" { + t.Errorf("after adding it back by hand = %v, want the book at 2", got) + } + if n := f.count(`SELECT COUNT(*) FROM book_series_exclusions`); n != 0 { + t.Errorf("exclusions = %d after adding back, want 0", n) + } +} + +// The exclusion is by provider id, so it holds when the series is merged +// away: its id becomes an alias of the target, and the book stays out of it. +func TestBookSeriesExclusionFollowsMerge(t *testing.T) { + f := newMergeFixture(t, 1) + target, src := f.newSeries("s:t", "T"), f.newSeries("s:s", "S") + f.link(src, 1, "1", true) + if _, err := f.series.RemoveBookFromSeries(f.ctx, src, f.books[0]); err != nil { + t.Fatal(err) + } + if _, err := f.series.Merge(f.ctx, target, []int64{src}, ""); err != nil { + t.Fatal(err) + } + if created, err := f.series.LinkBookIfMissing(f.ctx, target, f.books[0], "1", true); err != nil || created { + t.Errorf("link into the merge target = %v, %v; want skipped, the book was taken out of the merged series", created, err) + } +} + +// A book taken out of a duplicate but still in the series it is merged into +// is in that series: the merge drops the exclusion instead of leaving the +// book listed as kept out of a series it is in. One kept out of the +// duplicate only still stays out. +func TestMergeDropsExclusionsForBooksInTheTarget(t *testing.T) { + f := newMergeFixture(t, 2) + target, src := f.newSeries("s:t", "T"), f.newSeries("s:s", "S") + f.link(target, 1, "1", true) + f.link(src, 1, "1", true) + f.link(src, 2, "2", true) + for _, b := range f.books { + if _, err := f.series.RemoveBookFromSeries(f.ctx, src, b); err != nil { + t.Fatal(err) + } + } + if _, err := f.series.Merge(f.ctx, target, []int64{src}, ""); err != nil { + t.Fatal(err) + } + if got, err := f.series.ListBookSeriesExclusions(f.ctx, f.books[0]); err != nil || len(got) != 0 { + t.Errorf("book in the target: exclusions = %+v, %v; want none", got, err) + } + if got, err := f.series.ListBookSeriesExclusions(f.ctx, f.books[1]); err != nil || len(got) != 1 || got[0].SeriesID != target { + t.Errorf("book only in the duplicate: exclusions = %+v, %v; want it kept out of the target", got, err) + } +} + +// A hand-made series' id is not kept as an alias, so what was kept out of it +// goes with it. +func TestMergeDropsAHandMadeSeriesExclusions(t *testing.T) { + f := newMergeFixture(t, 1) + target := f.newSeries("s:t", "T") + manual, err := f.series.CreateManual(f.ctx, "Handmade") + if err != nil { + t.Fatal(err) + } + f.link(manual.ID, 1, "1", true) + if _, err := f.series.RemoveBookFromSeries(f.ctx, manual.ID, f.books[0]); err != nil { + t.Fatal(err) + } + if _, err := f.series.Merge(f.ctx, target, []int64{manual.ID}, ""); err != nil { + t.Fatal(err) + } + if n := f.count(`SELECT COUNT(*) FROM book_series_exclusions`); n != 0 { + t.Errorf("exclusions = %d, want 0", n) + } +} + +// Removing a book that is not in the series records nothing. +func TestRemoveBookFromSeriesNotMember(t *testing.T) { + f := newMergeFixture(t, 1) + s := f.newSeries("s:fjell", "Fjellserien") + if removed, err := f.series.RemoveBookFromSeries(f.ctx, s, f.books[0]); err != nil || removed { + t.Fatalf("RemoveBookFromSeries = %v, %v; want false", removed, err) + } + if n := f.count(`SELECT COUNT(*) FROM book_series_exclusions`); n != 0 { + t.Errorf("exclusions = %d, want 0", n) + } +} + +// Filing a book under a series as primary by hand demotes its other series, +// so it keeps one primary (#2525). +func TestUpsertBookLinkKeepsOnePrimary(t *testing.T) { + f := newMergeFixture(t, 1) + a, b := f.newSeries("s:a", "A"), f.newSeries("s:b", "B") + f.link(a, 1, "1", true) + if err := f.series.UpsertBookLink(f.ctx, b, f.books[0], "3", true); err != nil { + t.Fatal(err) + } + if got := f.membership(a); got[1] != "1" { + t.Errorf("A = %v, want book 1 kept but no longer primary", got) + } + if got := f.membership(b); got[1] != "3/P" { + t.Errorf("B = %v, want book 1 primary at 3", got) + } + // Not primary leaves the existing primary alone. + if err := f.series.UpsertBookLink(f.ctx, a, f.books[0], "1", false); err != nil { + t.Fatal(err) + } + if n := f.count(`SELECT COUNT(*) FROM series_books WHERE book_id = ? AND primary_series = 1`, f.books[0]); n != 1 { + t.Errorf("primary series = %d, want 1", n) + } +} + +// The exclusion keeps the position the book had, and lists under the series' +// current name: after a merge, the series it was merged into. +func TestListBookSeriesExclusions(t *testing.T) { + f := newMergeFixture(t, 1) + target, src := f.newSeries("s:t", "Fjellserien"), f.newSeries("s:s", "Serien om fjellet") + gone := f.newSeries("s:gone", "Borte") + f.link(src, 1, "4", true) + f.link(gone, 1, "", false) + for _, s := range []int64{src, gone} { + if _, err := f.series.RemoveBookFromSeries(f.ctx, s, f.books[0]); err != nil { + t.Fatal(err) + } + } + if _, err := f.series.Merge(f.ctx, target, []int64{src}, ""); err != nil { + t.Fatal(err) + } + if err := f.series.Delete(f.ctx, gone); err != nil { + t.Fatal(err) + } + + got, err := f.series.ListBookSeriesExclusions(f.ctx, f.books[0]) + if err != nil { + t.Fatal(err) + } + // By name; a series that no longer exists sorts by its id. + want := []BookSeriesExclusion{ + {SeriesForeignID: "s:s", SeriesID: target, SeriesTitle: "Fjellserien", Position: "4"}, + {SeriesForeignID: "s:gone", SeriesID: 0, SeriesTitle: "", Position: ""}, + } + if fmt.Sprint(got) != fmt.Sprint(want) { + t.Errorf("exclusions = %+v, want %+v", got, want) + } + + if err := f.series.ClearBookSeriesExclusions(f.ctx, f.books[0]); err != nil { + t.Fatal(err) + } + if got, _ := f.series.ListBookSeriesExclusions(f.ctx, f.books[0]); len(got) != 0 { + t.Errorf("after clearing = %+v, want none", got) + } + if created, err := f.series.LinkBookIfMissing(f.ctx, target, f.books[0], "4", true); err != nil || !created { + t.Errorf("automatic link after clearing = %v, %v; want linked again", created, err) + } +} + +// UpsertBookLink is one transaction: when its last step (demoting the book's +// other series) fails, the exclusion it cleared and the link it wrote are +// rolled back too. +func TestUpsertBookLinkIsAtomic(t *testing.T) { + f := newMergeFixture(t, 1) + a, b := f.newSeries("s:a", "A"), f.newSeries("s:b", "B") + f.link(a, 1, "1", true) + f.link(b, 1, "2", false) + if _, err := f.series.RemoveBookFromSeries(f.ctx, b, f.books[0]); err != nil { + t.Fatal(err) + } + if _, err := f.db.Exec(`CREATE TEMP TRIGGER fail_demote BEFORE UPDATE OF primary_series ON series_books + WHEN NEW.primary_series = 0 BEGIN SELECT RAISE(ABORT, 'injected'); END`); err != nil { + t.Fatal(err) + } + if err := f.series.UpsertBookLink(f.ctx, b, f.books[0], "2", true); err == nil { + t.Fatal("UpsertBookLink succeeded although its last step failed") + } + if got := f.membership(b); len(got) != 0 { + t.Errorf("B = %v, want the link rolled back", got) + } + if got := f.membership(a); got[1] != "1/P" { + t.Errorf("A = %v, want book 1 still primary", got) + } + if n := f.count(`SELECT COUNT(*) FROM book_series_exclusions`); n != 1 { + t.Errorf("exclusions = %d, want the cleared one restored", n) + } +} + +// RemoveBookFromSeries is one transaction: when the removal fails, no +// exclusion is left behind for a book still in the series. +func TestRemoveBookFromSeriesIsAtomic(t *testing.T) { + f := newMergeFixture(t, 1) + s := f.newSeries("s:a", "A") + f.link(s, 1, "1", true) + if _, err := f.db.Exec(`CREATE TEMP TRIGGER fail_unlink BEFORE DELETE ON series_books BEGIN SELECT RAISE(ABORT, 'injected'); END`); err != nil { + t.Fatal(err) + } + if _, err := f.series.RemoveBookFromSeries(f.ctx, s, f.books[0]); err == nil { + t.Fatal("RemoveBookFromSeries succeeded although the removal failed") + } + if got := f.membership(s); got[1] != "1/P" { + t.Errorf("membership = %v, want the book still in the series", got) + } + if n := f.count(`SELECT COUNT(*) FROM book_series_exclusions`); n != 0 { + t.Errorf("exclusions = %d, want none for a book still in the series", n) + } +} + +// Read and clear failures are errors, not an empty list or a silent no-op. +func TestBookSeriesExclusionFailures(t *testing.T) { + f := newMergeFixture(t, 1) + ctx, cancel := context.WithCancel(f.ctx) + cancel() + if got, err := f.series.ListBookSeriesExclusions(ctx, f.books[0]); err == nil { + t.Errorf("list on a cancelled context = %v, want an error", got) + } + if _, err := f.db.Exec(`CREATE TEMP TRIGGER fail_clear BEFORE DELETE ON book_series_exclusions BEGIN SELECT RAISE(ABORT, 'injected'); END`); err != nil { + t.Fatal(err) + } + if _, err := f.db.Exec(`INSERT INTO book_series_exclusions (book_id, series_foreign_id) VALUES (?, 's:x')`, f.books[0]); err != nil { + t.Fatal(err) + } + if err := f.series.ClearBookSeriesExclusions(f.ctx, f.books[0]); err == nil { + t.Error("clear succeeded although the delete failed") + } +} + +// Migration 102 ran on custom builds before it merged. Should it be +// renumbered, running it again over the table it created must not fail +// startup. +func TestMigration102IsSafeToRunAgain(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + content, err := migrationsFS.ReadFile("migrations/102_book_series_exclusions.sql") + if err != nil { + t.Fatal(err) + } + up, _, _ := strings.Cut(string(content), "-- +migrate Down") + if _, err := database.Exec(up); err != nil { + t.Fatalf("running 102 over an existing table: %v", err) + } +} diff --git a/internal/db/series_merge.go b/internal/db/series_merge.go index aafd94f73..127848990 100644 --- a/internal/db/series_merge.go +++ b/internal/db/series_merge.go @@ -295,8 +295,19 @@ func (r *SeriesRepo) applyMerge(ctx context.Context, plan *SeriesMergePlan) erro add("delete the merged series", `DELETE FROM series WHERE id = ?`, src.ID) if keepAsAlias(src.ForeignID) { add("alias the merged series' id", `INSERT INTO series_aliases (foreign_id, series_id) VALUES (?, ?)`, src.ForeignID, t) + } else if src.ForeignID != "" { + // Nothing resolves the id any more, so what was kept out of it + // has nothing left to stay out of. + add("drop the merged series' exclusions", `DELETE FROM book_series_exclusions WHERE series_foreign_id = ?`, src.ForeignID) } } + // A book the user took out of one series and that is in the target, or + // joined it now, is in the series the exclusion resolves to: forget it, + // or the book page lists it as kept out of a series it is in. + add("drop exclusions the merge made stale", `DELETE FROM book_series_exclusions + WHERE book_id IN (SELECT book_id FROM series_books WHERE series_id = ?) + AND (series_foreign_id = (SELECT foreign_id FROM series WHERE id = ?) + OR series_foreign_id IN (SELECT foreign_id FROM series_aliases WHERE series_id = ?))`, t, t, t) add("update the series", `UPDATE series SET title = ?, monitored = ? WHERE id = ?`, plan.Title, boolToInt(plan.Monitored), t) for _, st := range steps { if _, err := r.exec.ExecContext(ctx, st.query, st.args...); err != nil { diff --git a/web/src/api/books.coverage.test.ts b/web/src/api/books.coverage.test.ts index 71cbfed98..8337767a9 100644 --- a/web/src/api/books.coverage.test.ts +++ b/web/src/api/books.coverage.test.ts @@ -106,6 +106,7 @@ describe('booksApi request shapes', () => { await booksApi.getLastSearchDebug() await booksApi.enrichAudiobook(4) await booksApi.toggleExcluded(4) + await booksApi.getBookSeriesExclusions(4) await booksApi.rebindBook(4, 'hardcover', 'hc:1') await booksApi.rebindBook(4, 'openlibrary', 'OL1W', true) expect(seen.map(s => [s.method, s.path + s.query, s.body])).toEqual([ @@ -118,6 +119,7 @@ describe('booksApi request shapes', () => { ['GET', '/search/last-debug', undefined], ['POST', '/book/4/enrich-audiobook', undefined], ['PUT', '/book/4/exclude', undefined], + ['GET', '/book/4/series-exclusions', undefined], ['POST', '/book/4/rebind', { provider: 'hardcover', foreign_id: 'hc:1', force: false }], ['POST', '/book/4/rebind', { provider: 'openlibrary', foreign_id: 'OL1W', force: true }], ]) diff --git a/web/src/api/books.ts b/web/src/api/books.ts index 570553e5c..374696a53 100644 --- a/web/src/api/books.ts +++ b/web/src/api/books.ts @@ -166,6 +166,15 @@ export interface ReassignPreview { message?: string } +// A series the user took a book out of (#2554), named as it is now (0 and '' +// when the series no longer exists), with the position the book had. +export interface BookSeriesExclusion { + seriesForeignId: string + seriesId: number + seriesTitle: string + position: string +} + export const booksApi = { // Metadata search searchAuthors: (term: string) => request(`/search/author?term=${encodeURIComponent(term)}`), @@ -234,6 +243,7 @@ export const booksApi = { }, getBook: (id: number) => request(`/book/${id}`), updateBook: (id: number, data: Partial) => request(`/book/${id}`, { method: 'PUT', body: JSON.stringify(data) }), + getBookSeriesExclusions: (id: number) => request(`/book/${id}/series-exclusions`), deleteBook: (id: number, deleteFiles = false) => request(`/book/${id}${deleteFiles ? '?deleteFiles=true' : ''}`, { method: 'DELETE' }), deleteBookFile: (id: number, queryParams = '') => request(`/book/${id}/file${queryParams}`, { method: 'DELETE' }), diff --git a/web/src/auth/AuthContext.tsx b/web/src/auth/AuthContext.tsx index 9a446565a..d8fcbcc20 100644 --- a/web/src/auth/AuthContext.tsx +++ b/web/src/auth/AuthContext.tsx @@ -87,6 +87,15 @@ export function useAuth() { return ctx } +// useIsAdmin gates controls whose endpoints are admin only, such as every +// series change (#468), so other users are not offered a button that answers +// 403. Like useIsRequester it also serves components rendered outside the +// provider (isolated tests), where the answer is true: the full controls they +// were written for. The server enforces the role either way. +export function useIsAdmin(): boolean { + return useContext(AuthContext)?.isAdmin ?? true +} + // useIsRequester is for components that also render outside the provider // (in isolated tests, for one): with no provider the answer is false, which // keeps the full add flow they were written for. diff --git a/web/src/components/AddSeriesBookModal.test.tsx b/web/src/components/AddSeriesBookModal.test.tsx new file mode 100644 index 000000000..b66375fc0 --- /dev/null +++ b/web/src/components/AddSeriesBookModal.test.tsx @@ -0,0 +1,34 @@ +import { describe, expect, it, vi } from 'vitest' +import { fireEvent, render, screen, waitFor } from '@testing-library/react' +import AddSeriesBookModal from './AddSeriesBookModal' +import { api } from '../api/client' +import type { Book, Series } from '../api/client' + +vi.mock('../api/client', async importOriginal => { + const actual = await importOriginal() + return { + ...actual, + api: { + ...actual.api, + listAllBooks: vi.fn(async () => [{ id: 7, title: 'Fjellvinden', authorId: 1 } as Book]), + listAllAuthors: vi.fn(async () => []), + linkBookToSeries: vi.fn(async () => series), + }, + } +}) + +const series: Series = { id: 3, title: 'Fjellserien', foreignSeriesId: 's:3', description: '', monitored: false, books: [] } + +describe('AddSeriesBookModal', () => { + // The server takes a book without a position (unnumbered series, omnibus + // volumes), so the dialog does too. + it('adds a book without a position', async () => { + const onLinked = vi.fn() + render( {}} onLinked={onLinked} />) + fireEvent.click(await screen.findByRole('radio', { name: /Fjellvinden/ })) + expect(screen.getByLabelText('Position (optional)')).toHaveValue('') + fireEvent.click(screen.getByRole('button', { name: 'Add' })) + await waitFor(() => expect(onLinked).toHaveBeenCalled()) + expect(api.linkBookToSeries).toHaveBeenCalledWith(3, { bookId: 7, positionInSeries: '', primarySeries: true }) + }) +}) diff --git a/web/src/components/AddSeriesBookModal.tsx b/web/src/components/AddSeriesBookModal.tsx index 0f117ea21..be8e4b3f7 100644 --- a/web/src/components/AddSeriesBookModal.tsx +++ b/web/src/components/AddSeriesBookModal.tsx @@ -67,11 +67,9 @@ export default function AddSeriesBookModal({ series, onClose, onLinked }: Props) setError('Select a book') return } + // A position is optional, as on the server: unnumbered series and + // omnibus volumes have none. const trimmedPosition = position.trim() - if (!trimmedPosition) { - setError('Position is required') - return - } setSaving(true) setError(null) try { @@ -160,12 +158,11 @@ export default function AddSeriesBookModal({ series, onClose, onLinked }: Props)
@@ -193,7 +190,7 @@ export default function AddSeriesBookModal({ series, onClose, onLinked }: Props)
+ {allSeries !== null && ( +
+
+ + +
+
+ + setPosition(e.target.value)} + disabled={seriesChoice === ''} className={inputCls} /> +
+
+ )} + {seriesChoice === NEW_SERIES && ( +
+ + setNewSeriesName(e.target.value)} + onKeyDown={e => { if (e.key === 'Enter' && canSave) void save() }} className={inputCls} autoFocus /> +
+ )} {error &&

{error}

}
- {locked.length > 0 && ( + {(locked.length > 0 || seriesExclusions > 0) && ( @@ -159,7 +287,7 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { - + {/* Creating a series is admin only (#468). */} + {isAdmin && ( + + )} {/* Below `sm` the five buttons wrap and the primary action used to land on the second row, under Refresh all metadata, Merge and Add Book. `order-first` pulls it to the front of the wrapped group on a diff --git a/web/src/pages/BookDetailPage.test.tsx b/web/src/pages/BookDetailPage.test.tsx index 09dcfbc1c..4078b3c99 100644 --- a/web/src/pages/BookDetailPage.test.tsx +++ b/web/src/pages/BookDetailPage.test.tsx @@ -1,5 +1,5 @@ import { useEffect } from 'react' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, onTestFinished, vi } from 'vitest' import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react' import { MemoryRouter, Route, Routes, useLocation } from 'react-router' import BookDetailPage, { SearchResultsSection } from './BookDetailPage' @@ -58,6 +58,8 @@ vi.mock('../api/client', async importOriginal => { toggleExcluded: vi.fn(), enrichAudiobook: vi.fn(), listAuthorSeries: vi.fn(), + getBookSeriesExclusions: vi.fn(), + linkBookToSeries: vi.fn(), bookCalibreState: vi.fn().mockResolvedValue({ state: 'off' }), setPrimarySeriesForBook: vi.fn(), removeBookFromSeries: vi.fn(), @@ -65,6 +67,17 @@ vi.mock('../api/client', async importOriginal => { } }) +// Series changes are admin only (#468); a test switches to another user. +const authState = { isAdmin: true } +vi.mock('../auth/AuthContext', async importOriginal => ({ + ...await importOriginal(), + useIsAdmin: () => authState.isAdmin, +})) +const asAnotherUser = () => { + authState.isAdmin = false + onTestFinished(() => { authState.isAdmin = true }) +} + vi.mock('../components/MediaBadge', () => ({ default: ({ type }: { type?: string }) => {type}, })) @@ -232,6 +245,7 @@ beforeEach(() => { vi.mocked(api.toggleExcluded).mockImplementation(async () => makeBook({ excluded: true })) vi.mocked(api.enrichAudiobook).mockImplementation(async () => makeBook()) vi.mocked(api.listAuthorSeries).mockResolvedValue([]) + vi.mocked(api.getBookSeriesExclusions).mockResolvedValue([]) // jsdom has no clipboard by default. Object.defineProperty(navigator, 'clipboard', { value: { writeText: vi.fn().mockResolvedValue(undefined) }, @@ -1231,6 +1245,72 @@ describe('BookDetailPage — header', () => { expect(screen.queryByText(resolveKey('bookDetail.series.heading')!)).toBeNull() }) + it('lists the series the book was taken out of and restores one where it was (#2554)', async () => { + vi.mocked(api.listAuthorSeries).mockResolvedValue([ + { + id: 7, + foreignSeriesId: 'ol:s7', + title: 'Discworld', + description: '', + monitored: true, + books: [{ seriesId: 7, bookId: 42, positionInSeries: '3', primarySeries: true }], + }, + ] as unknown as Awaited>) + vi.mocked(api.getBookSeriesExclusions).mockResolvedValue([ + { seriesForeignId: 'ol:s8', seriesId: 8, seriesTitle: 'Rincewind', position: '2' }, + // A series that no longer exists has nothing to restore to. + { seriesForeignId: 'ol:gone', seriesId: 0, seriesTitle: '', position: '' }, + ]) + vi.mocked(api.linkBookToSeries).mockResolvedValue({} as never) + + renderBookDetailPage() + // One membership, so the section is there only for the kept-out series. + expect(await screen.findByText(resolveKey('bookDetail.series.keptOutExplainer')!)).toBeInTheDocument() + expect(screen.queryByText(resolveKey('bookDetail.series.explainer')!)).toBeNull() + const keptOut = screen.getByRole('list', { name: resolveKey('bookDetail.series.keptOutHeading')! }) + expect(within(keptOut).getAllByRole('listitem')).toHaveLength(1) + expect(within(keptOut).getByText('Rincewind #2')).toBeInTheDocument() + + // Under 44px on its own, so it carries the touch hit area. + expect(within(keptOut).getByRole('button', { name: resolveKey('bookDetail.series.restore')! }).className.split(/\s+/)).toContain('touch-target') + fireEvent.click(within(keptOut).getByRole('button', { name: resolveKey('bookDetail.series.restore')! })) + // The book already has a primary series, so the restored one is not. + await waitFor(() => expect(api.linkBookToSeries).toHaveBeenCalledWith(8, { bookId: 42, positionInSeries: '2', primarySeries: false })) + await waitFor(() => expect(api.getBookSeriesExclusions).toHaveBeenCalledTimes(2)) + }) + + it('shows another user the book\'s series without the controls that need an admin', async () => { + asAnotherUser() + vi.mocked(api.listAuthorSeries).mockResolvedValue([ + { id: 7, foreignSeriesId: 'ol:s7', title: 'Fjellserien', description: '', monitored: true, + books: [{ seriesId: 7, bookId: 42, positionInSeries: '1', primarySeries: true }] }, + { id: 8, foreignSeriesId: 'ol:s8', title: 'Havserien', description: '', monitored: true, + books: [{ seriesId: 8, bookId: 42, positionInSeries: '', primarySeries: false }] }, + ] as unknown as Awaited>) + vi.mocked(api.getBookSeriesExclusions).mockResolvedValue([ + { seriesForeignId: 'ol:s9', seriesId: 9, seriesTitle: 'Skogserien', position: '2' }, + ]) + + renderBookDetailPage() + const heading = await screen.findByText(resolveKey('bookDetail.series.heading')!) + const section = heading.closest('section')! + expect(within(section).getAllByRole('listitem')).toHaveLength(2) + expect(within(section).queryAllByRole('button')).toHaveLength(0) + expect(screen.queryByText(resolveKey('bookDetail.series.keptOutExplainer')!)).toBeNull() + }) + + it('says so when restoring a series fails', async () => { + vi.mocked(api.getBookSeriesExclusions).mockResolvedValue([ + { seriesForeignId: 'ol:s8', seriesId: 8, seriesTitle: 'Rincewind', position: '2' }, + ]) + vi.mocked(api.linkBookToSeries).mockRejectedValue(new Error('series mutations are admin only')) + + renderBookDetailPage() + const keptOut = await screen.findByRole('list', { name: resolveKey('bookDetail.series.keptOutHeading')! }) + fireEvent.click(within(keptOut).getByRole('button', { name: resolveKey('bookDetail.series.restore')! })) + expect(await screen.findByText('series mutations are admin only')).toBeInTheDocument() + }) + it('names the series the renamer uses and lets you change it', async () => { // #2525: two primary rows meant the renamer picked one by query-plan order // and nothing on the page said which, or let the user say otherwise. diff --git a/web/src/pages/BookDetailPage.tsx b/web/src/pages/BookDetailPage.tsx index 98b031eb3..3dabd0387 100644 --- a/web/src/pages/BookDetailPage.tsx +++ b/web/src/pages/BookDetailPage.tsx @@ -1,7 +1,7 @@ import { useEffect, useRef, useState } from 'react' import { Link, useLocation, useNavigate, useParams } from 'react-router' import { useTranslation } from 'react-i18next' -import { api, BINDERY_BASE, Book, HistoryEvent, MediaType, SearchResult, SearchDebug, Series } from '../api/client' +import { api, BINDERY_BASE, Book, BookSeriesExclusion, HistoryEvent, MediaType, SearchResult, SearchDebug, Series } from '../api/client' import SearchDebugPanel from '../components/SearchDebugPanel' import CoverPlaceholder from '../components/CoverPlaceholder' import MarkdownDescription from '../components/MarkdownDescription' @@ -9,6 +9,7 @@ import MoreMenu from '../components/MoreMenu' import Section from '../components/Section' import Switch from '../components/Switch' import { btn, btnSize, dangerLink } from '../components/buttons' +import { useIsAdmin } from '../auth/AuthContext' import MediaBadge from '../components/MediaBadge' import CalibreDeliveryChip from '../components/CalibreDeliveryChip' import { bookStatusBadge } from '../components/bookStatus' @@ -328,6 +329,9 @@ function BookDetailPageInner() { const [seriesBusy, setSeriesBusy] = useState(false) const [removeSeries, setRemoveSeries] = useState<{ id: number; title: string } | null>(null) const [seriesNonce, setSeriesNonce] = useState(0) + // Series the user took this book out of (#2554). Refreshes leave it out + // of these; Restore puts it back where it was. + const [keptOut, setKeptOut] = useState([]) useEffect(() => { if (book?.title) { @@ -395,9 +399,33 @@ function BookDetailPageInner() { setSeries(mine) }) .catch(() => { /* no series row */ }) + api.getBookSeriesExclusions(id) + .then(list => { if (!cancelled) setKeptOut(list) }) + .catch(() => { /* nothing to show */ }) return () => { cancelled = true } }, [book?.authorId, book?.id, seriesNonce]) + // Puts the book back in a series it was taken out of, at the position it + // had, as its primary series only when it has none. Linking by hand clears + // the exclusion on the server. + const restoreSeries = async (e: BookSeriesExclusion) => { + if (!book) return + setSeriesBusy(true) + setError(null) + try { + await api.linkBookToSeries(e.seriesId, { bookId: book.id, positionInSeries: e.position, primarySeries: !series.some(s => s.primary) }) + setSeriesNonce(n => n + 1) + } catch (err) { + setError(err instanceof Error ? err.message : t('bookDetail.series.restoreFailed')) + } finally { + setSeriesBusy(false) + } + } + // Series changes are admin only (#468): other users see the book's series + // without the controls, and nothing to restore. + const isAdmin = useIsAdmin() + const restorable = isAdmin ? keptOut.filter(e => e.seriesId > 0) : [] + // #2525: the renamer reads one series per book. When a book sits in both its // real series and an umbrella "Universe" one, these are how the user says // which, and how they leave the one that should never have been linked. @@ -1058,8 +1086,9 @@ function BookDetailPageInner() { Only when a book is in more than one series. With a single membership there is nothing to choose and nothing to correct, so the meta row above already says everything. */} - {series.length > 1 && ( + {(series.length > 1 || restorable.length > 0) && (
+ {series.length > 1 && (<>

{t('bookDetail.series.explainer')}

    {series.map(s => ( @@ -1075,7 +1104,7 @@ function BookDetailPageInner() { {s.primary ? ( {t('bookDetail.series.namesFiles')} - ) : ( + ) : isAdmin && ( + {isAdmin && ( + + )} ))}
+ )} + {restorable.length > 0 && ( +
1 ? 'mt-4' : ''}> +

{t('bookDetail.series.keptOutExplainer')}

+
    + {restorable.map(e => ( +
  • + + {e.position + ? t('bookDetail.seriesPosition', { series: e.seriesTitle, position: e.position, defaultValue: '{{series}} #{{position}}' }) + : e.seriesTitle} + + +
    • + ))} +
+
+ )}
)} @@ -1519,6 +1570,8 @@ function BookDetailPageInner() { book={book} onClose={() => setShowEdit(false)} onSaved={updated => setBook(updated)} + onSeriesSaved={() => setSeriesNonce(n => n + 1)} + seriesExclusions={keptOut.length} /> )} {showRebind && ( diff --git a/web/src/pages/SeriesPage.test.tsx b/web/src/pages/SeriesPage.test.tsx index e6cdded76..9225669c7 100644 --- a/web/src/pages/SeriesPage.test.tsx +++ b/web/src/pages/SeriesPage.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach, onTestFinished } from 'vitest' import { act, render, screen, fireEvent, waitFor, within } from '@testing-library/react' import { MemoryRouter, Route, Routes, useLocation } from 'react-router' import { ModalHistoryProvider } from '../components/useModal' @@ -42,6 +42,13 @@ vi.mock('../api/client', async importOriginal => { } }) +// Series changes are admin only (#468); one test switches to another user. +const authState = { isAdmin: true } +vi.mock('../auth/AuthContext', async importOriginal => ({ + ...await importOriginal(), + useIsAdmin: () => authState.isAdmin, +})) + function renderSeriesPage(series: Series[], status: SystemStatus = { version: 'dev', commit: 'unknown', buildDate: '', enhancedHardcoverApi: true, hardcoverTokenConfigured: true }) { vi.mocked(api.listSeries).mockResolvedValue(series) vi.mocked(api.status).mockResolvedValue(status) @@ -769,7 +776,7 @@ describe('SeriesPage', () => { expect(within(dialog).queryByText('Dune')).not.toBeInTheDocument() fireEvent.click(await within(dialog).findByLabelText(/Dune Messiah/)) - fireEvent.change(within(dialog).getByLabelText('Position'), { target: { value: '2' } }) + fireEvent.change(within(dialog).getByLabelText('Position (optional)'), { target: { value: '2' } }) fireEvent.click(within(dialog).getByRole('button', { name: 'Add' })) await waitFor(() => expect(api.linkBookToSeries).toHaveBeenCalledWith(30, { @@ -780,6 +787,29 @@ describe('SeriesPage', () => { expect(await screen.findByRole('link', { name: /Dune Messiah/ })).toHaveAttribute('href', '/book/102') }) + it('shows another user the series and what is missing, without the controls that need an admin', async () => { + authState.isAdmin = false + onTestFinished(() => { authState.isAdmin = true }) + const hardcoverLink = { + id: 1, seriesId: 41, hardcoverSeriesId: 'hc-series:7', hardcoverProviderId: '7', hardcoverTitle: 'Fjellserien', + hardcoverAuthorName: 'Kari Nordmann', hardcoverBookCount: 2, confidence: 1, linkedBy: 'manual', + linkedAt: '2026-01-01T00:00:00Z', createdAt: '2026-01-01T00:00:00Z', updatedAt: '2026-01-01T00:00:00Z', + } + vi.mocked(api.getSeriesHardcoverDiff).mockResolvedValue({ + seriesId: 41, link: hardcoverLink, present: [], covered: [], + missing: [{ foreignBookId: 'hc:fjellvinden', providerId: '8', title: 'Fjellvinden', position: '2', authorName: 'Kari Nordmann' }], + localOnly: [], uncertain: [], presentCount: 0, missingCount: 1, + }) + renderSeriesPage([{ id: 41, foreignSeriesId: 'series-41', title: 'Fjellserien', description: '', monitored: false, books: [], hardcoverLink }]) + + fireEvent.click(await screen.findByRole('heading', { name: 'Fjellserien' })) + expect(await screen.findByText('Fjellvinden')).toBeInTheDocument() + for (const name of [/add series/i, /rename/i, /merge/i, /delete/i, /^add$/i, /add all/i, /add book/i]) { + expect(screen.queryByRole('button', { name })).not.toBeInTheDocument() + } + expect(screen.queryByRole('switch')).not.toBeInTheDocument() + }) + it('adds only the selected Hardcover missing book from its row', async () => { const hardcoverLink = { id: 1, diff --git a/web/src/pages/SeriesPage.tsx b/web/src/pages/SeriesPage.tsx index ffd193297..46bd8a756 100644 --- a/web/src/pages/SeriesPage.tsx +++ b/web/src/pages/SeriesPage.tsx @@ -9,6 +9,7 @@ import HardcoverSeriesLinkModal from '../components/HardcoverSeriesLinkModal' import SeriesNameModal from '../components/SeriesNameModal' import MergeSeriesModal from '../components/MergeSeriesModal' import { btn, btnSize } from '../components/buttons' +import { useIsAdmin } from '../auth/AuthContext' import Switch from '../components/Switch' import { useConfirmDialog } from '../components/useConfirmDialog' @@ -50,6 +51,8 @@ function seriesMissingCounts(series: Series, enhancedHardcoverApi: boolean, diff export default function SeriesPage() { const { t } = useTranslation() + // Every series change is admin only (#468); other users browse. + const isAdmin = useIsAdmin() const { confirm, confirmDialog } = useConfirmDialog() const location = useLocation() const [seriesList, setSeriesList] = useState([]) @@ -362,12 +365,14 @@ export default function SeriesPage() { {filter === 'all' ? t('series.count', { count: seriesList.length }) : t('series.countFiltered', { shown: filteredSeries.length, total: seriesList.length })} - + {isAdmin && ( + + )}
@@ -465,6 +470,7 @@ export default function SeriesPage() { {/* Actions row */} + {isAdmin && (
e.stopPropagation()}> {/* This flag is a shortlist marker, not a schedule. Nothing reads series.monitored except this page: no job checks a @@ -568,6 +574,7 @@ export default function SeriesPage() { {linkResult[series.id]} )}
+ )} {isOpen && bookCount > 0 && (
@@ -660,7 +667,7 @@ export default function SeriesPage() { )}
- {(diff?.missingCount ?? 0) > 0 && ( + {isAdmin && (diff?.missingCount ?? 0) > 0 && (