From b72938292db7b16f7061d3013fc7d8f50b03a160 Mon Sep 17 00:00:00 2001 From: Francis Rath Date: Wed, 7 Oct 2026 08:30:21 +0200 Subject: [PATCH 01/17] feat(series): keep a book out of a series the user removed it from (#2554) A refresh re-adds whatever series a provider reports and never removes one, so a book taken out of a series came back with the next refresh. Removing a book from a series now records it in book_series_exclusions, keyed by the series' foreign id so it holds through a merge, and LinkBookIfMissing, which every automatic path goes through, skips an excluded pair inside its INSERT. Adding the book back by hand clears it. The table is migration 102. UpsertBookLink, the manual add, now also demotes the book's other series when it files it as primary, so a book keeps one primary series (#2525). Co-Authored-By: Claude Opus 5.5 Signed-off-by: Francis Rath --- internal/api/series.go | 4 +- internal/api/series_merge_refresh_test.go | 32 +++++++ .../migrations/102_book_series_exclusions.sql | 15 +++ internal/db/series.go | 86 +++++++++++++++-- internal/db/series_aliases_test.go | 4 +- internal/db/series_exclusions_test.go | 92 +++++++++++++++++++ 6 files changed, 221 insertions(+), 12 deletions(-) create mode 100644 internal/db/migrations/102_book_series_exclusions.sql create mode 100644 internal/db/series_exclusions_test.go diff --git a/internal/api/series.go b/internal/api/series.go index c95c8423f..c7d215fce 100644 --- a/internal/api/series.go +++ b/internal/api/series.go @@ -408,7 +408,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 } 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..f8236a6d0 --- /dev/null +++ b/internal/db/migrations/102_book_series_exclusions.sql @@ -0,0 +1,15 @@ +-- +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. +CREATE TABLE book_series_exclusions ( + book_id INTEGER NOT NULL REFERENCES books(id) ON DELETE CASCADE, + series_foreign_id TEXT NOT NULL, + 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..6e0a73c68 100644 --- a/internal/db/series.go +++ b/internal/db/series.go @@ -803,6 +803,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 +815,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 +857,84 @@ 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() }() + 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.ExecContext(ctx, `INSERT OR IGNORE INTO book_series_exclusions (book_id, series_foreign_id) + SELECT ?, foreign_id FROM series WHERE id = ? AND foreign_id != ''`, bookID, seriesID); err != nil { + return false, fmt.Errorf("record book %d left series %d: %w", bookID, seriesID, err) + } + 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..4ac8af7eb --- /dev/null +++ b/internal/db/series_exclusions_test.go @@ -0,0 +1,92 @@ +package db + +import ( + "fmt" + "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) + } +} + +// 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) + } +} From e7dfe1d7767ebfe8fe314ef3672cc75e2ea818bf Mon Sep 17 00:00:00 2001 From: Francis Rath Date: Wed, 7 Oct 2026 08:32:41 +0200 Subject: [PATCH 02/17] feat(web): set a book's series and position in Edit metadata (#2554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edit metadata gets a Series row: a dropdown of the library's series, New series… to create one, and a position. It edits the book's primary series: picking another files the book there as primary and takes it out of the old one, which the server remembers so a refresh does not put it back; clearing it takes the book out; changing only the position updates it in place. Nothing is sent for the series unless it changed, and the book page reloads its series after a save. If the series cannot be loaded the row stays hidden and the other fields still save. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Francis Rath --- web/src/components/EditBookModal.test.tsx | 88 ++++++++++++++++++ web/src/components/EditBookModal.tsx | 108 ++++++++++++++++++++-- web/src/i18n/locales/en.json | 7 +- web/src/pages/BookDetailPage.tsx | 1 + 4 files changed, 196 insertions(+), 8 deletions(-) diff --git a/web/src/components/EditBookModal.test.tsx b/web/src/components/EditBookModal.test.tsx index 75aee4b16..0458960a9 100644 --- a/web/src/components/EditBookModal.test.tsx +++ b/web/src/components/EditBookModal.test.tsx @@ -19,6 +19,11 @@ vi.mock('react-i18next', () => ({ vi.mock('../api/client', () => ({ api: { updateBook: vi.fn(), + listSeries: vi.fn(), + listAuthorSeries: vi.fn(), + createSeries: vi.fn(), + linkBookToSeries: vi.fn(), + removeBookFromSeries: vi.fn(), }, })) @@ -43,6 +48,8 @@ describe('EditBookModal (#1237, #1446)', () => { beforeEach(() => { vi.clearAllMocks() vi.mocked(api.updateBook).mockResolvedValue({ ...BOOK, title: 'Changed' } as never) + vi.mocked(api.listSeries).mockResolvedValue([]) + vi.mocked(api.listAuthorSeries).mockResolvedValue([]) }) it('sends only the fields the user changed', async () => { @@ -87,4 +94,85 @@ describe('EditBookModal (#1237, #1446)', () => { await waitFor(() => expect(api.updateBook).toHaveBeenCalled()) expect(vi.mocked(api.updateBook).mock.calls[0]).toEqual([7, { lockedFields: [] }]) }) + + describe('series (#2554)', () => { + const IN_SERIES = { ...BOOK, authorId: 3 } as unknown as Book + const A = { id: 1, title: 'Fjellserien', books: [{ seriesId: 1, bookId: 7, positionInSeries: '1', primarySeries: true }] } + const B = { id: 2, title: 'Havserien', books: [] } + const onSeriesSaved = vi.fn() + + beforeEach(() => { + vi.mocked(api.listSeries).mockResolvedValue([A, B] as never) + vi.mocked(api.listAuthorSeries).mockResolvedValue([A] as never) + vi.mocked(api.linkBookToSeries).mockResolvedValue({} as never) + vi.mocked(api.removeBookFromSeries).mockResolvedValue(undefined as never) + vi.mocked(api.createSeries).mockResolvedValue({ id: 9, title: 'Ny serie' } as never) + }) + + const open = async () => { + render() + const select = await screen.findByLabelText('Series') as HTMLSelectElement + await waitFor(() => expect(select.value).toBe('1')) + return select + } + const save = () => fireEvent.click(screen.getByRole('button', { name: 'Save' })) + + it('shows the primary series and position, and saving untouched makes no series call', async () => { + await open() + expect((screen.getByLabelText('Position') as HTMLInputElement).value).toBe('1') + save() + await waitFor(() => expect(onClose).toHaveBeenCalled()) + expect(api.linkBookToSeries).not.toHaveBeenCalled() + expect(api.removeBookFromSeries).not.toHaveBeenCalled() + expect(onSeriesSaved).not.toHaveBeenCalled() + }) + + it('moves the book: files it under the new series and takes it out of the old one', async () => { + const select = await open() + fireEvent.change(select, { target: { value: '2' } }) + fireEvent.change(screen.getByLabelText('Position'), { target: { value: ' 3 ' } }) + save() + await waitFor(() => expect(api.removeBookFromSeries).toHaveBeenCalledWith(1, 7)) + expect(api.linkBookToSeries).toHaveBeenCalledWith(2, { bookId: 7, positionInSeries: '3', primarySeries: true }) + expect(api.updateBook).not.toHaveBeenCalled() + expect(onSeriesSaved).toHaveBeenCalled() + }) + + it('changes only the position in the same series', async () => { + await open() + fireEvent.change(screen.getByLabelText('Position'), { target: { value: '2' } }) + save() + await waitFor(() => expect(api.linkBookToSeries).toHaveBeenCalledWith(1, { bookId: 7, positionInSeries: '2', primarySeries: true })) + expect(api.removeBookFromSeries).not.toHaveBeenCalled() + }) + + it('creates a new series and files the book there', async () => { + const select = await open() + fireEvent.change(select, { target: { value: 'new' } }) + expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled() + fireEvent.change(screen.getByLabelText('New series name'), { target: { value: ' Ny serie ' } }) + save() + await waitFor(() => expect(api.createSeries).toHaveBeenCalledWith({ title: 'Ny serie' })) + expect(api.linkBookToSeries).toHaveBeenCalledWith(9, { bookId: 7, positionInSeries: '1', primarySeries: true }) + expect(api.removeBookFromSeries).toHaveBeenCalledWith(1, 7) + }) + + it('takes the book out of its series when cleared', async () => { + const select = await open() + fireEvent.change(select, { target: { value: '' } }) + expect(screen.getByLabelText('Position')).toBeDisabled() + save() + await waitFor(() => expect(api.removeBookFromSeries).toHaveBeenCalledWith(1, 7)) + expect(api.linkBookToSeries).not.toHaveBeenCalled() + }) + + it('hides the series row when the series cannot be loaded, and other edits still save', async () => { + vi.mocked(api.listSeries).mockRejectedValue(new Error('down')) + render() + fireEvent.change(screen.getByLabelText('Title'), { target: { value: 'Changed' } }) + save() + await waitFor(() => expect(api.updateBook).toHaveBeenCalledWith(7, { title: 'Changed' })) + expect(screen.queryByLabelText('Series')).toBeNull() + }) + }) }) diff --git a/web/src/components/EditBookModal.tsx b/web/src/components/EditBookModal.tsx index 46e62508b..ec17d6324 100644 --- a/web/src/components/EditBookModal.tsx +++ b/web/src/components/EditBookModal.tsx @@ -1,18 +1,26 @@ -import { useState } from 'react' +import { useEffect, useMemo, useState } from 'react' import { useTranslation } from 'react-i18next' -import { api, Book } from '../api/client' +import { api, Book, Series } from '../api/client' import { useModal } from './useModal' interface Props { book: Book onClose: () => void onSaved: (book: Book) => void + // Called after the book's series changed, so the page reloads its series. + onSeriesSaved?: () => void } +// The series a book is filed under for naming: its primary membership, or its +// only one. +interface Membership { id: number; position: string } + +const NEW_SERIES = 'new' + // Manual metadata editor (#1237, #1446). Only fields the user actually // changed are sent — the backend locks every submitted field against // metadata refresh, so sending unchanged values would spuriously lock them. -export default function EditBookModal({ book, onClose, onSaved }: Props) { +export default function EditBookModal({ book, onClose, onSaved, onSeriesSaved }: Props) { const { t } = useTranslation() const [title, setTitle] = useState(book.title) const [description, setDescription] = useState(book.description || '') @@ -21,6 +29,39 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { const [releaseDate, setReleaseDate] = useState(book.releaseDate ? book.releaseDate.slice(0, 10) : '') const [saving, setSaving] = useState(false) const [error, setError] = useState(null) + // Series (#2554): the book's primary series and its position, edited here + // so one book can be fixed without going through the series page. null + // until loaded; the row stays hidden if loading fails, and editing the + // other fields still works. + const [allSeries, setAllSeries] = useState(null) + const [membership, setMembership] = useState(null) + const [seriesChoice, setSeriesChoice] = useState('') + const [newSeriesName, setNewSeriesName] = useState('') + const [position, setPosition] = useState('') + + useEffect(() => { + let active = true + Promise.all([api.listSeries(), book.authorId ? api.listAuthorSeries(book.authorId) : Promise.resolve([] as Series[])]) + .then(([list, authorSeries]) => { + if (!active) return + const mine = authorSeries.flatMap(s => + (s.books ?? []).filter(b => b.bookId === book.id).map(b => ({ id: s.id, position: b.positionInSeries, primary: b.primarySeries === true }))) + const current = mine.find(m => m.primary) ?? (mine.length === 1 ? mine[0] : null) + setAllSeries([...list].sort((a, b) => a.title.localeCompare(b.title))) + setMembership(current ? { id: current.id, position: current.position } : null) + setSeriesChoice(current ? String(current.id) : '') + setPosition(current?.position ?? '') + }) + .catch(() => { /* the series row stays hidden */ }) + return () => { active = false } + }, [book.authorId, book.id]) + + const seriesChanged = useMemo(() => { + if (allSeries === null) return false + const was = membership ? String(membership.id) : '' + if (seriesChoice !== was) return true + return seriesChoice !== '' && position.trim() !== (membership?.position ?? '') + }, [allSeries, membership, seriesChoice, position]) const locked = book.lockedFields ?? [] const { titleId, panelProps } = useModal({ onClose, canClose: !saving }) @@ -34,15 +75,20 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { if (language.trim() !== (book.language || '')) patch.language = language.trim() const origDate = book.releaseDate ? book.releaseDate.slice(0, 10) : '' if (releaseDate !== origDate) patch.releaseDate = releaseDate - if (Object.keys(patch).length === 0) { + if (Object.keys(patch).length === 0 && !seriesChanged) { onClose() return } setSaving(true) setError(null) try { - const updated = await api.updateBook(book.id, patch as Partial) - onSaved(updated) + if (seriesChanged) { + await saveSeries() + onSeriesSaved?.() + } + if (Object.keys(patch).length > 0) { + onSaved(await api.updateBook(book.id, patch as Partial)) + } onClose() } catch (e) { setError(e instanceof Error ? e.message : t('bookDetail.edit.saveFailed', 'Save failed')) @@ -51,6 +97,25 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { } } + // Files the book under the chosen series as its primary one, at the given + // position, and takes it out of the series it was in before. The server + // demotes the book's other series and remembers the removal, so a refresh + // does not put it back (#2554). + const saveSeries = async () => { + let target: number | null = null + if (seriesChoice === NEW_SERIES) { + target = (await api.createSeries({ title: newSeriesName.trim() })).id + } else if (seriesChoice !== '') { + target = Number(seriesChoice) + } + if (target !== null) { + await api.linkBookToSeries(target, { bookId: book.id, positionInSeries: position.trim(), primarySeries: true }) + } + if (membership && membership.id !== target) { + await api.removeBookFromSeries(membership.id, book.id) + } + } + const unlockAll = async () => { setSaving(true) setError(null) @@ -137,6 +202,35 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { setReleaseDate(e.target.value)} className={inputCls} /> + {allSeries !== null && ( +
+
+ + +
+
+ + setPosition(e.target.value)} + disabled={seriesChoice === ''} className={inputCls} /> +
+
+ )} + {seriesChoice === NEW_SERIES && ( +
+ + setNewSeriesName(e.target.value)} className={inputCls} /> +
+ )} {error &&

{error}

} @@ -159,7 +253,7 @@ export default function EditBookModal({ book, onClose, onSaved }: Props) { diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index c79cf68eb..4f386df95 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -2063,7 +2063,7 @@ "fieldReleaseDate": "Release date", "lockedHint": "Manually edited — metadata refresh will not overwrite this field", "unlockAll": "Unlock all fields", - "unlockAllHint": "Let metadata refresh manage every field again", + "unlockAllHint": "Let metadata refresh manage every field and the book's series again", "saveFailed": "Save failed", "fieldSeries": "Series", "seriesNone": "No series", @@ -2156,7 +2156,11 @@ "removeBody1": "This takes the book out of", "removeBody2": "It does not delete the book, its files, or the series itself.", "setPrimaryFailed": "Could not change the series used for naming", - "removeFailed": "Could not remove the book from that series" + "removeFailed": "Could not remove the book from that series", + "keptOutHeading": "Kept out of", + "keptOutExplainer": "You took this book out of these series, so refreshes leave it out. Restore puts it back where it was.", + "restore": "Restore", + "restoreFailed": "Could not put the book back in that series" }, "calibreChip": { "pending": "Waiting for Calibre", diff --git a/web/src/pages/BookDetailPage.test.tsx b/web/src/pages/BookDetailPage.test.tsx index b74266406..03f0b86b6 100644 --- a/web/src/pages/BookDetailPage.test.tsx +++ b/web/src/pages/BookDetailPage.test.tsx @@ -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(), @@ -232,6 +234,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) }, @@ -1076,6 +1079,38 @@ 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() + + 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('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 d6cc493bb..30435cfe0 100644 --- a/web/src/pages/BookDetailPage.tsx +++ b/web/src/pages/BookDetailPage.tsx @@ -1,7 +1,7 @@ import { useEffect, 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' @@ -332,6 +332,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) { @@ -381,9 +384,30 @@ 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) + } + } + const restorable = 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. @@ -1000,8 +1024,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 => ( @@ -1038,6 +1063,26 @@ function BookDetailPageInner() { ))}
+ )} + {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} + + +
    • + ))} +
+
+ )}
)} @@ -1458,6 +1503,7 @@ function BookDetailPageInner() { onClose={() => setShowEdit(false)} onSaved={updated => setBook(updated)} onSeriesSaved={() => setSeriesNonce(n => n + 1)} + seriesExclusions={keptOut.length} /> )} {showRebind && ( From d35f383f1dad2ce1286321aa155e871e2cba2c90 Mon Sep 17 00:00:00 2001 From: Francis Rath Date: Wed, 7 Oct 2026 09:08:38 +0200 Subject: [PATCH 07/17] fix(web): regroup an author's books by series after a refresh The author page loaded the series grouping once per author, so after a Refresh it reloaded the books but grouped them by the series from before it: a book the refresh filed under a series stayed under Standalone until a full page reload. The grouping now loads again with the reload a finished Refresh triggers, keeping the current grouping on screen while it fetches. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Francis Rath --- web/src/pages/AuthorDetailPage.test.tsx | 23 +++++++++++++++++++++++ web/src/pages/AuthorDetailPage.tsx | 16 +++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/web/src/pages/AuthorDetailPage.test.tsx b/web/src/pages/AuthorDetailPage.test.tsx index 36f8bbd63..cfbc9849c 100644 --- a/web/src/pages/AuthorDetailPage.test.tsx +++ b/web/src/pages/AuthorDetailPage.test.tsx @@ -1195,6 +1195,29 @@ describe('AuthorDetailPage: manual refresh', () => { expect(vi.mocked(api.getAuthor).mock.calls.length).toBe(calls) }) + it('regroups by series after a refresh moves a book into one (#2554)', async () => { + // Grouped by series: before the refresh the book is in none, the refresh + // files it under one, and the page must regroup without a reload. + localStorage.setItem('bindery.group.author-detail.series', 'true') + const book = makeBook({ id: 7, title: 'Fjellvinden', status: 'imported' }) + renderAuthorDetailPage([book], 'table') + await screen.findByRole('heading', { name: /Standalone/ }) + + vi.useFakeTimers() + const finishAt = Date.now() + 3000 + const synced = () => Date.now() >= finishAt + vi.mocked(api.getAuthor).mockImplementation(async () => (synced() ? author : { ...author, syncInProgress: true })) + vi.mocked(api.listAuthorSeries).mockImplementation(async () => (synced() + ? [{ id: 3, foreignSeriesId: 's:3', title: 'Fjellserien', description: '', monitored: false, books: [{ seriesId: 3, bookId: 7, positionInSeries: '1' }] }] + : [])) + vi.mocked(api.refreshAuthor).mockResolvedValue(undefined) + + fireEvent.click(screen.getByRole('button', { name: 'Refresh' })) + await advance(4000) + expect(screen.getByRole('heading', { name: /Fjellserien/ })).toBeInTheDocument() + expect(screen.queryByRole('heading', { name: /Standalone/ })).toBeNull() + }) + it('gives up waiting after a minute and shows whatever the server has', async () => { renderAuthorDetailPage(oldBooks, 'grid', { description: 'Old bio' }) await screen.findByRole('heading', { name: 'Ancillary Justice' }) diff --git a/web/src/pages/AuthorDetailPage.tsx b/web/src/pages/AuthorDetailPage.tsx index d1f30934d..7e841d874 100644 --- a/web/src/pages/AuthorDetailPage.tsx +++ b/web/src/pages/AuthorDetailPage.tsx @@ -150,7 +150,10 @@ export default function AuthorDetailPage() { // mounted across Previous/Next (only authorId changes), so a plain // "already loaded" check on authorSeries.length kept the previous // author's series and grouped the new author's books against them. - const loadedSeriesAuthorId = useRef(null) + // Which author and reload the series were loaded for: a manual Refresh + // reloads through reloadKey, and its sync can change the series a book is + // in, so the grouping is fetched again then too. + const loadedSeries = useRef<{ authorId: number; reloadKey: number } | null>(null) useEffect(() => { try { localStorage.setItem('bindery.group.author-detail.series', String(groupBySeries)) } catch { /* ignore */ } @@ -160,16 +163,19 @@ export default function AuthorDetailPage() { // the default flat view never pays for the extra round trip. Failures fall // back to an empty set — every book then lands in the Standalone group. useEffect(() => { - if (!groupBySeries || loadedSeriesAuthorId.current === authorId) return + const loaded = loadedSeries.current + if (!groupBySeries || (loaded?.authorId === authorId && loaded.reloadKey === reloadKey)) return // Drop the previous author's series before fetching, so neither the // in flight window nor a failed fetch groups these books against them. - setAuthorSeries([]) + // The same author's stay up while a refresh reload fetches, so the + // grouping does not flash every book into Standalone. + if (loaded?.authorId !== authorId) setAuthorSeries([]) let cancelled = false api.listAuthorSeries(authorId) - .then(s => { if (!cancelled) { setAuthorSeries(s); loadedSeriesAuthorId.current = authorId } }) + .then(s => { if (!cancelled) { setAuthorSeries(s); loadedSeries.current = { authorId, reloadKey } } }) .catch(() => { /* leave empty: books fall into Standalone */ }) return () => { cancelled = true } - }, [groupBySeries, authorId]) + }, [groupBySeries, authorId, reloadKey]) // Filter / sort state — persisted to localStorage under page-scoped keys const [typeFilter, setTypeFilter] = useState(() => { From 68515c71164a798c503fba54731fa1a90664f532 Mon Sep 17 00:00:00 2001 From: Francis Rath Date: Wed, 7 Oct 2026 09:08:38 +0200 Subject: [PATCH 08/17] test(series): cover failures and single-book edge cases (#2554) UpsertBookLink and RemoveBookFromSeries roll back as a whole when a step fails; a failed exclusion read or clear is an error; Unlock all and Remove answer 500 when their write fails; the exclusions endpoint 404s for an unknown book. On the web side: a book in no series filed under one, a single non-primary membership preselected, and a failed Restore shown. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Francis Rath --- internal/api/book_series_exclusions_test.go | 71 ++++++++++++++++++++- internal/db/series_exclusions_test.go | 69 ++++++++++++++++++++ web/src/components/EditBookModal.test.tsx | 23 +++++++ web/src/pages/BookDetailPage.test.tsx | 12 ++++ 4 files changed, 174 insertions(+), 1 deletion(-) diff --git a/internal/api/book_series_exclusions_test.go b/internal/api/book_series_exclusions_test.go index 920b3aa9d..dcd0bf727 100644 --- a/internal/api/book_series_exclusions_test.go +++ b/internal/api/book_series_exclusions_test.go @@ -3,6 +3,7 @@ package api import ( "bytes" "context" + "database/sql" "encoding/json" "net/http" "net/http/httptest" @@ -15,7 +16,10 @@ import ( // 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). -func TestBookSeriesExclusionsEndpointAndUnlockAll(t *testing.T) { +// 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) @@ -33,6 +37,11 @@ func TestBookSeriesExclusionsEndpointAndUnlockAll(t *testing.T) { 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) @@ -79,3 +88,63 @@ func TestBookSeriesExclusionsEndpointAndUnlockAll(t *testing.T) { 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() + h.Update(rec, withURLParam(httptest.NewRequest(http.MethodPut, "/api/v1/book/"+id, bytes.NewBufferString(`{"lockedFields":[]}`)), "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/db/series_exclusions_test.go b/internal/db/series_exclusions_test.go index 105d7d3c3..8d80598fa 100644 --- a/internal/db/series_exclusions_test.go +++ b/internal/db/series_exclusions_test.go @@ -1,6 +1,7 @@ package db import ( + "context" "fmt" "testing" ) @@ -134,3 +135,71 @@ func TestListBookSeriesExclusions(t *testing.T) { 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") + } +} diff --git a/web/src/components/EditBookModal.test.tsx b/web/src/components/EditBookModal.test.tsx index 792dae77f..f842adb26 100644 --- a/web/src/components/EditBookModal.test.tsx +++ b/web/src/components/EditBookModal.test.tsx @@ -173,6 +173,29 @@ describe('EditBookModal (#1237, #1446)', () => { expect(api.linkBookToSeries).not.toHaveBeenCalled() }) + it('files a book that is in no series under the one picked', async () => { + vi.mocked(api.listAuthorSeries).mockResolvedValue([{ ...A, books: [] }, B] as never) + render() + const select = await screen.findByLabelText('Series') as HTMLSelectElement + await waitFor(() => expect(select.options.length).toBe(4)) + expect(select.value).toBe('') + fireEvent.change(select, { target: { value: '2' } }) + fireEvent.change(screen.getByLabelText('Position'), { target: { value: '5' } }) + save() + await waitFor(() => expect(api.linkBookToSeries).toHaveBeenCalledWith(2, { bookId: 7, positionInSeries: '5', primarySeries: true })) + expect(api.removeBookFromSeries).not.toHaveBeenCalled() + }) + + it('preselects the only series a book is in, even when it is not flagged primary', async () => { + vi.mocked(api.listAuthorSeries).mockResolvedValue([ + { ...A, books: [{ seriesId: 1, bookId: 7, positionInSeries: '4', primarySeries: false }] }, B, + ] as never) + render() + const select = await screen.findByLabelText('Series') as HTMLSelectElement + await waitFor(() => expect(select.value).toBe('1')) + expect((screen.getByLabelText('Position') as HTMLInputElement).value).toBe('4') + }) + it('hides the series row when the series cannot be loaded, and other edits still save', async () => { vi.mocked(api.listAuthorSeries).mockRejectedValue(new Error('down')) render() diff --git a/web/src/pages/BookDetailPage.test.tsx b/web/src/pages/BookDetailPage.test.tsx index 03f0b86b6..1fb105179 100644 --- a/web/src/pages/BookDetailPage.test.tsx +++ b/web/src/pages/BookDetailPage.test.tsx @@ -1111,6 +1111,18 @@ describe('BookDetailPage — header', () => { await waitFor(() => expect(api.getBookSeriesExclusions).toHaveBeenCalledTimes(2)) }) + 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. From 4bdaa80beb09d793931f923782f4fdf13df0d647 Mon Sep 17 00:00:00 2001 From: Francis Rath Date: Wed, 7 Oct 2026 09:36:02 +0200 Subject: [PATCH 09/17] fix(web): make the series position optional when adding a book (#2554) The server links a book to a series without a position, and unnumbered series and omnibus volumes have none, but the Add Book dialog refused to save without one and showed a "1" placeholder that looked like a value. The field is now labelled optional and Add only needs a book. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Francis Rath --- .../components/AddSeriesBookModal.test.tsx | 34 +++++++++++++++++++ web/src/components/AddSeriesBookModal.tsx | 11 +++--- web/src/pages/SeriesPage.test.tsx | 2 +- 3 files changed, 39 insertions(+), 8 deletions(-) create mode 100644 web/src/components/AddSeriesBookModal.test.tsx 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) - + {/* 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 1fb105179..1945fe1ef 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' @@ -67,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}, })) @@ -1111,6 +1122,26 @@ describe('BookDetailPage — header', () => { 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' }, diff --git a/web/src/pages/BookDetailPage.tsx b/web/src/pages/BookDetailPage.tsx index 30435cfe0..044e5c759 100644 --- a/web/src/pages/BookDetailPage.tsx +++ b/web/src/pages/BookDetailPage.tsx @@ -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' @@ -406,7 +407,10 @@ function BookDetailPageInner() { setSeriesBusy(false) } } - const restorable = keptOut.filter(e => e.seriesId > 0) + // 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 @@ -1042,7 +1046,7 @@ function BookDetailPageInner() { {s.primary ? ( {t('bookDetail.series.namesFiles')} - ) : ( + ) : isAdmin && ( + {isAdmin && ( + + )} ))} diff --git a/web/src/pages/SeriesPage.test.tsx b/web/src/pages/SeriesPage.test.tsx index 75af18567..3f7d50f5b 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' @@ -41,6 +41,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) @@ -737,6 +744,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: [], + 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 273bc4a22..0a5687d5b 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' @@ -41,6 +42,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([]) @@ -328,12 +331,14 @@ export default function SeriesPage() { {filter === 'all' ? t('series.count', { count: seriesList.length }) : t('series.countFiltered', { shown: filteredSeries.length, total: seriesList.length })} - + {isAdmin && ( + + )}
@@ -429,6 +434,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 @@ -522,6 +528,7 @@ export default function SeriesPage() { {linkResult[series.id]} )}
+ )} {isOpen && bookCount > 0 && (
@@ -606,7 +613,7 @@ export default function SeriesPage() { )}
- {(diff?.missingCount ?? 0) > 0 && ( + {isAdmin && (diff?.missingCount ?? 0) > 0 && (