feat(series): fix one book's series, and keep removed books out (#2554)

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.

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)

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

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.

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)
+}

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)
+	}
+}

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

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 "<Series> 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