fix(importer): suggest every book by the matched author when adopting a file (#2879)

changelog.d/2879-adoption-suggestion-pool.md

@@ -0,0 +1,2 @@
+### Fixed
+- **Import suggestions now offer every book by the matched author** (#2879): an untracked "The Matsumoto - Sarah K. L. Wilson.epub" was offered three other books by her instead of "The Matsumoto", because suggestions on **Import → In your library** only came from books still waiting for a file. A book you had skipped, or one already imported with a file somewhere else, was never offered, however exact the title. Suggestions now come from all of the author's books, an exact title comes first, and each suggestion shows its status. The scan still only attaches files on its own to books that are wanted or missing a file, so nothing changes without you choosing it. A book that already has its files or was skipped is never a one click Confirm, and adopting into a book that already has a file adds the new one alongside it and says so. The author's spelling with or without a space between initials was already treated as the same name. Thanks bhaveman for the report.

docs/User-Guide-Wiki.md

@@ -533,6 +533,12 @@ How to work through the list:
 - **Possible match** means the title is only similar, or the author differs.
   Click the suggested title to check it in the editor, where it is already
   selected, and adopt it from there.
+- Suggestions come from **every book by the author the scan matched**,
+  whatever its status, and each shows that status. A book you skipped or one
+  that is already **Imported** is never a one click Confirm; adopting into a
+  book that already has a file adds the new file alongside it (#2879).
+  Suggestions are worked out by the scan, so a book added since the last scan
+  appears after the next one; until then, search for it in the editor.
 - **Choose book** opens the row in place: the suggestions with their scores,
   a search of your library (prefilled from the file), and a collapsed
   **Search metadata**. Metadata providers are only asked when you press Search

internal/api/adoption_adopt.go

@@ -84,7 +84,8 @@ func (h *AdoptionHandler) Adopt(w http.ResponseWriter, r *http.Request) {
 	// The request context may be cancelled once work has started; releasing
 	// the claim and compensating must still happen.
 	work := context.WithoutCancel(ctx)
-	if err := h.adopt(work, id, token, req); err != nil {
+	message, err := h.adopt(work, id, token, req)
+	if err != nil {
 		var unfinished *unfinishedReversalError
 		if errors.As(err, &unfinished) {
 			// The claim and its record stay, so stale claim recovery can finish
@@ -100,42 +101,51 @@ func (h *AdoptionHandler) Adopt(w http.ResponseWriter, r *http.Request) {
 		h.writeAdoptionError(w, r, err)
 		return
 	}
-	h.writeUnit(w, r, id)
+	h.writeUnitWithMessage(w, r, id, message)
 }
 
-func (h *AdoptionHandler) adopt(ctx context.Context, id int64, token string, req adoptRequest) error {
+// alreadyHasFileMessage explains adopting into a book that already has a file
+// of the same format on disk. Suggestions offer such books (#2879), because an
+// untracked copy of a book already imported is common. The adopted files are
+// added alongside: book_files is additive, the file the book already shows
+// keeps showing, and nothing is deleted. Undo removes only what was added.
+const alreadyHasFileMessage = "This book already had a file of this format. The adopted files were added alongside it, and the book still shows its existing file."
+
+// adopt does the work of Adopt. The message it returns, when not empty,
+// explains an outcome that is not the obvious one.
+func (h *AdoptionHandler) adopt(ctx context.Context, id int64, token string, req adoptRequest) (string, error) {
 	unit, err := h.units.Get(ctx, id)
 	if err != nil {
-		return err
+		return "", err
 	}
 	if unit == nil {
-		return refuse(http.StatusNotFound, "unmatched book not found")
+		return "", refuse(http.StatusNotFound, "unmatched book not found")
 	}
 	if req.Format != "" && req.Format != unit.Format {
-		return refuse(http.StatusBadRequest, "These files are "+unit.Format+" files, so they can only be adopted as "+unit.Format+".")
+		return "", refuse(http.StatusBadRequest, "These files are "+unit.Format+" files, so they can only be adopted as "+unit.Format+".")
 	}
 	format := unit.Format
 	paths, err := h.registrationPaths(ctx, unit)
 	if err != nil {
-		return err
+		return "", err
 	}
 	// Refuse before any side effect when a file already belongs to a book.
 	if err := h.checkOwnership(ctx, append(paths, unit.MemberPaths...), 0); err != nil {
-		return err
+		return "", err
 	}
 
 	var book *models.Book
 	var created addBookResult
 	if req.BookID > 0 {
 		if book, err = h.books.GetByID(ctx, req.BookID); err != nil {
-			return err
+			return "", err
 		}
 		if book == nil {
-			return refuse(http.StatusNotFound, "That book is no longer in your library.")
+			return "", refuse(http.StatusNotFound, "That book is no longer in your library.")
 		}
 	} else {
 		if h.adder == nil {
-			return refuse(http.StatusServiceUnavailable, "Adding books is not available.")
+			return "", refuse(http.StatusServiceUnavailable, "Adding books is not available.")
 		}
 		unmonitored := false
 		created, err = h.adder.addBookCore(ctx, addBookParams{
@@ -153,12 +163,23 @@ func (h *AdoptionHandler) adopt(ctx context.Context, id int64, token string, req
 			// Already in the library: adopt into that row, as if picked.
 			book = inLibrary.Book
 		case err != nil:
-			return err
+			return "", err
 		default:
 			book = created.Book
 		}
 	}
 
+	message := ""
+	if !created.BookCreated {
+		had, err := h.hasLiveFile(ctx, book.ID, format)
+		if err != nil {
+			return "", err
+		}
+		if had {
+			message = alreadyHasFileMessage
+		}
+	}
+
 	rec := db.AdoptionRecord{BookID: book.ID}
 	if created.BookCreated {
 		rec.CreatedBookID = book.ID
@@ -168,7 +189,7 @@ func (h *AdoptionHandler) adopt(ctx context.Context, id int64, token string, req
 	}
 	// The created rows are on record before any file is registered.
 	if err := h.progress(ctx, id, token, rec); err != nil {
-		return h.failAdopt(ctx, id, rec, err)
+		return "", h.failAdopt(ctx, id, rec, err)
 	}
 
 	regErr := h.register(ctx, id, token, book.ID, format, paths, unit.MemberPaths, &rec)
@@ -191,18 +212,34 @@ func (h *AdoptionHandler) adopt(ctx context.Context, id int64, token string, req
 		}
 	}
 	if regErr != nil {
-		return h.failAdopt(ctx, id, rec, regErr)
+		return "", h.failAdopt(ctx, id, rec, regErr)
 	}
 	done, err := h.units.CompleteAdoption(ctx, id, token, rec)
 	if err != nil || !done {
 		if err == nil {
 			err = refuse(http.StatusConflict, "This book changed while it was being adopted. Try again.")
 		}
-		return h.failAdopt(ctx, id, rec, err)
+		return "", h.failAdopt(ctx, id, rec, err)
 	}
 	slog.Info("adoption: registered files in place", "unit", id, "book_id", book.ID,
-		"files", len(rec.Registered), "book_created", rec.CreatedBookID > 0, "author_created", rec.CreatedAuthorID > 0)
-	return nil
+		"files", len(rec.Registered), "book_created", rec.CreatedBookID > 0, "author_created", rec.CreatedAuthorID > 0,
+		"added_alongside_existing", message != "")
+	return message, nil
+}
+
+// hasLiveFile reports whether the book has a registered file of format that
+// still exists on disk.
+func (h *AdoptionHandler) hasLiveFile(ctx context.Context, bookID int64, format string) (bool, error) {
+	files, err := h.books.ListFiles(ctx, bookID)
+	if err != nil {
+		return false, err
+	}
+	for _, f := range files {
+		if f.Format == format && db.BookFilePathResolves(f.Path) {
+			return true, nil
+		}
+	}
+	return false, nil
 }
 
 // failAdopt reverses a failed adopt's writes. If the reversal itself fails

internal/api/adoption_test.go

@@ -515,3 +515,68 @@ func TestAdoptionList_NoProviderCallsAndOneHydrationQuery(t *testing.T) {
 		t.Fatalf("provider calls during list = %d, want 0", n)
 	}
 }
+
+// TestAdopt_IntoABookThatAlreadyHasItsFile is the outcome #2879 makes
+// reachable from a suggestion: the scan now offers a book already Imported
+// with a file, because an untracked copy of it is common. Adopting there adds
+// the copy alongside the existing file, keeps the book Imported and still
+// showing that file, says so in the response, and Undo removes only the copy.
+func TestAdopt_IntoABookThatAlreadyHasItsFile(t *testing.T) {
+	f := newAdoptionFixture(t, &stubMetaProvider{name: "openlibrary"})
+	ctx := context.Background()
+	book := f.seedBook(t, "The Matsumoto")
+	existing := f.write(t, "Sarah K.L. Wilson/The Matsumoto (13110)/The Matsumoto - Sarah K.L. Wilson.epub")
+	if err := f.books.AddBookFile(ctx, book.ID, models.MediaTypeEbook, existing); err != nil {
+		t.Fatal(err)
+	}
+	copyPath := f.write(t, "Sarah K. L. Wilson/The Matsumoto (13110)/The Matsumoto - Sarah K. L. Wilson.epub")
+	id := f.seedUnit(t, db.UnmatchedUnitScan{UnitPath: copyPath, MemberPaths: []string{copyPath}})
+
+	rec := f.post(t, fmt.Sprintf("/library/unmatched/%d/adopt", id), map[string]any{"bookId": book.ID})
+	if rec.Code != http.StatusOK {
+		t.Fatalf("adopt = %d %s", rec.Code, rec.Body.String())
+	}
+	it := decodeItem(t, rec)
+	if it.State != db.UnmatchedStateAdopted || it.Message != alreadyHasFileMessage {
+		t.Fatalf("adopted item = state %s message %q, want adopted with the already had a file message", it.State, it.Message)
+	}
+	if got := filePaths(t, f.books, book.ID); len(got) != 2 || got[0] != existing || got[1] != copyPath {
+		t.Fatalf("book files = %v, want the existing file then the adopted copy", got)
+	}
+	after, _ := f.books.GetByID(ctx, book.ID)
+	if after.Status != models.BookStatusImported || after.EbookFilePath != existing {
+		t.Fatalf("book after adopt = status %s showing %q, want imported still showing %s", after.Status, after.EbookFilePath, existing)
+	}
+
+	rec = f.post(t, fmt.Sprintf("/library/unmatched/%d/undo", id), nil)
+	if rec.Code != http.StatusOK {
+		t.Fatalf("undo = %d %s", rec.Code, rec.Body.String())
+	}
+	if got := filePaths(t, f.books, book.ID); len(got) != 1 || got[0] != existing {
+		t.Fatalf("book files after undo = %v, want only the existing file", got)
+	}
+	if b, _ := f.books.GetByID(ctx, book.ID); b.Status != models.BookStatusImported {
+		t.Fatalf("book after undo = status %s, want imported", b.Status)
+	}
+	for _, p := range []string{existing, copyPath} {
+		if _, err := os.Stat(p); err != nil {
+			t.Fatalf("%s touched on disk: %v", p, err)
+		}
+	}
+}
+
+// TestAdopt_IntoAWantedBookSaysNothingExtra: the message is only for a book
+// that already had a file, so an ordinary adoption stays quiet.
+func TestAdopt_IntoAWantedBookSaysNothingExtra(t *testing.T) {
+	f := newAdoptionFixture(t, &stubMetaProvider{name: "openlibrary"})
+	book := f.seedBook(t, "Ancillary Sword")
+	path := f.write(t, "Ann Leckie/Ancillary Sword.epub")
+	id := f.seedUnit(t, db.UnmatchedUnitScan{UnitPath: path, MemberPaths: []string{path}})
+	rec := f.post(t, fmt.Sprintf("/library/unmatched/%d/adopt", id), map[string]any{"bookId": book.ID})
+	if rec.Code != http.StatusOK {
+		t.Fatalf("adopt = %d %s", rec.Code, rec.Body.String())
+	}
+	if it := decodeItem(t, rec); it.Message != "" {
+		t.Fatalf("message = %q, want none", it.Message)
+	}
+}

internal/importer/scanner.go

@@ -2842,6 +2842,16 @@ type scanBook struct {
 	book      *models.Book
 	normTitle string
 	normLen   int
+	// reconcilable is isReconcileCandidate for the book: the scan may claim
+	// a file for it on its own. Only the suggestion pool holds books without
+	// it (#2879).
+	reconcilable bool
+}
+
+func newScanBook(b *models.Book, reconcilable bool) scanBook {
+	sb := scanBook{book: b, normTitle: normalizeTitle(b.Title), reconcilable: reconcilable}
+	sb.normLen = len(sb.normTitle)
+	return sb
 }
 
 // cleanLayoutTitle strips bracket/paren annotations from a book-folder name —
@@ -3205,8 +3215,7 @@ func (s *Scanner) scanLibrary(ctx context.Context) {
 		if !isReconcileCandidate(b) {
 			continue
 		}
-		sb := scanBook{book: b, normTitle: normalizeTitle(b.Title)}
-		sb.normLen = len(sb.normTitle)
+		sb := newScanBook(b, true)
 		idx := len(wantedBooks)
 		wantedBooks = append(wantedBooks, sb)
 		booksByAuthor[b.AuthorID] = append(booksByAuthor[b.AuthorID], idx)
@@ -3751,11 +3760,22 @@ func (s *Scanner) scanLibrary(ctx context.Context) {
 		"reconciled", reconciled, "unmatched", unmatched, "tagReadFailed", tagReadFailed)
 
 	// Suggestions come from the catalogue already in memory, ranked once per
-	// unit rather than per file.
+	// unit rather than per file. A person confirms a suggestion, so for a
+	// resolved author they are drawn from every book of that author, not only
+	// the ones the scan may claim by itself: a Skipped book, or one already
+	// Imported with a file elsewhere, is often exactly what an untracked copy
+	// is (#2879). Built on first use, so a scan with nothing unmatched pays
+	// nothing for it. Excluded books are not in allBooks and so are never
+	// offered.
+	var catalogue []scanBook
+	var catalogueByAuthor map[int64][]int
 	units := s.recordUnmatchedUnits(ctx, &unmatchedFiles, scanRoots, rootsWithFiles, scanStartedAt,
 		func(title, layoutTitle, author, layoutAuthor string) []db.UnmatchedCandidate {
+			if catalogueByAuthor == nil {
+				catalogue, catalogueByAuthor = suggestionCatalogue(allBooks, wantedBooks)
+			}
 			authorSet, _ := resolveAuthors(author, layoutAuthor)
-			return rankCandidates(title, layoutTitle, wantedBooks, booksByAuthor, authorSet)
+			return rankCandidates(title, layoutTitle, wantedBooks, catalogue, catalogueByAuthor, authorSet)
 		})
 
 	s.writeScanResult(ctx, len(foundFiles), reconciled, unmatched, alreadyTracked, tagReadFailed, units)

internal/importer/unmatched_units.go

@@ -332,24 +332,55 @@ func eligibleUnmatched(files []unmatchedScanFile, roots []string) []unmatchedSca
 	return out
 }
 
+// suggestionCatalogue indexes every book the scan loaded by author, for
+// suggestions only. The reconcile's own candidates (wanted) are marked so a
+// tie goes to the book still waiting for a file; they are taken from the set
+// the scan already built rather than asking isReconcileCandidate again, which
+// stats files on disk.
+func suggestionCatalogue(books []models.Book, wanted []scanBook) ([]scanBook, map[int64][]int) {
+	reconcilable := make(map[int64]bool, len(wanted))
+	for i := range wanted {
+		reconcilable[wanted[i].book.ID] = true
+	}
+	out := make([]scanBook, 0, len(books))
+	byAuthor := make(map[int64][]int)
+	for i := range books {
+		b := &books[i]
+		byAuthor[b.AuthorID] = append(byAuthor[b.AuthorID], len(out))
+		out = append(out, newScanBook(b, reconcilable[b.ID]))
+	}
+	return out, byAuthor
+}
+
 // rankCandidates returns up to maxCandidates catalogue books whose title
 // scores at least candidateThreshold against normParsed, best first. It uses
-// the reconcile's own measure (Jaro-Winkler over normalizeTitle) and its own
-// candidate set: the books of the authors the file's author resolved to, or
-// every reconcile candidate when the file named no author. In that last case
-// titles of wildly different length are skipped, the same cheap gate the
-// reconcile uses. No provider is asked anything.
+// the reconcile's own measure (Jaro-Winkler over normalizeTitle). When the
+// file's author resolved, the candidates are every catalogue book of those
+// authors, whatever its status (#2879): the reconcile only claims books still
+// waiting for a file, but a person confirming a suggestion may well be
+// pointing an untracked copy at a book that is Skipped or already has one.
+// When the file named no author they are the reconcile's own candidates
+// (wanted), and titles of wildly different length are skipped, the same
+// cheap gate the reconcile uses; the whole library is too wide a net without
+// an author. No provider is asked anything.
+//
+// An exact title scores 1, so it ranks first; between equal scores a book the
+// reconcile could claim goes ahead of one that already has its files.
 //
 // A book that is provably another volume of the same series is never
 // suggested, by the same rule the reconcile applies (libraryVolumeConflict):
 // volume 1's folder scores 0.983 against a wanted volume 17, and offering it
 // as the top suggestion would invite the user to adopt it there (#2860).
-func rankCandidates(title, layoutTitle string, wanted []scanBook, byAuthor map[int64][]int, authorSet map[int64]bool) []db.UnmatchedCandidate {
+func rankCandidates(title, layoutTitle string, wanted, catalogue []scanBook, catalogueByAuthor map[int64][]int, authorSet map[int64]bool) []db.UnmatchedCandidate {
 	normParsed := normalizeTitle(title)
 	if normParsed == "" {
 		return nil
 	}
-	var out []db.UnmatchedCandidate
+	type scored struct {
+		db.UnmatchedCandidate
+		reconcilable bool
+	}
+	var out []scored
 	consider := func(sb *scanBook) {
 		score := textutil.JaroWinkler(sb.normTitle, normParsed)
 		if score < candidateThreshold {
@@ -358,7 +389,7 @@ func rankCandidates(title, layoutTitle string, wanted []scanBook, byAuthor map[i
 		if libraryVolumeConflict(title, layoutTitle, sb.book.Title) {
 			return
 		}
-		out = append(out, db.UnmatchedCandidate{BookID: sb.book.ID, Score: score})
+		out = append(out, scored{db.UnmatchedCandidate{BookID: sb.book.ID, Score: score}, sb.reconcilable})
 	}
 	if authorSet == nil {
 		for i := range wanted {
@@ -378,24 +409,35 @@ func rankCandidates(title, layoutTitle string, wanted []scanBook, byAuthor map[i
 		}
 		slices.Sort(ids)
 		for _, id := range ids {
-			for _, idx := range byAuthor[id] {
-				consider(&wanted[idx])
+			for _, idx := range catalogueByAuthor[id] {
+				consider(&catalogue[idx])
 			}
 		}
 	}
-	slices.SortStableFunc(out, func(a, b db.UnmatchedCandidate) int {
+	slices.SortStableFunc(out, func(a, b scored) int {
 		switch {
 		case a.Score > b.Score:
 			return -1
 		case a.Score < b.Score:
 			return 1
+		case a.reconcilable && !b.reconcilable:
+			return -1
+		case b.reconcilable && !a.reconcilable:
+			return 1
 		}
 		return 0
 	})
 	if len(out) > maxCandidates {
 		out = out[:maxCandidates]
 	}
-	return out
+	if len(out) == 0 {
+		return nil
+	}
+	res := make([]db.UnmatchedCandidate, len(out))
+	for i := range out {
+		res[i] = out[i].UnmatchedCandidate
+	}
+	return res
 }
 
 // unitCounts is what the scan result blob reports about stored units.