diff --git a/CLAUDE.md b/CLAUDE.md index 62f1c495..3c8c2b6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1076,6 +1076,13 @@ future metadata site can attach enrichment without reshaping the schema. The repick records only books with a new ASIN), so a clean library leaves no items. A refresh proposal names the identifier it was found by (`found_by`, "asin:..."/"isbn:..."), and applying it is a no-op for a book that no longer has it (matched again since). + Likewise a match run's book that has an ASIN or ISBN now (`matchedSince`: matched by hand + from the review or anywhere, or a rescan found one) has no changes, reads `matched_since` + in the review and is skipped with detail `matched_since`, under any scope; a match whose + fields went in before a stop cut its cover short stays to apply, detail `cover_pending`, its + ASIN the run's own rather than one matched since, so the next apply takes the cover. The review's + items page best match first (`score DESC, id`), the cursor still an item id whose score + the next page reads on from. Each scope's change carries `series` {from, to} (main first, position 0 = none) when it changes the book's series, which the review shows as "old -> new". A retired work slug is a redirect the client follows, so the names written are the merged record's. diff --git a/admin-ui/src/api/types.ts b/admin-ui/src/api/types.ts index a837ea52..ba443032 100644 --- a/admin-ui/src/api/types.ts +++ b/admin-ui/src/api/types.ts @@ -1994,12 +1994,17 @@ export interface MatchRunItem { proposal: MatchProposal; /** What applying did: '' (not yet) | applied | skipped | failed. */ applied: '' | 'applied' | 'skipped' | 'failed'; - /** Why: book_gone | nothing_to_change | edit_failed | cover_failed | metadata_unavailable | identifier_unknown. */ + /** Why: book_gone | nothing_to_change | matched_since | edit_failed | cover_failed | metadata_unavailable | identifier_unknown; cover_pending while still to apply (a stop cut its cover short). */ detail?: string; /** The book as it is now. */ book: { title: string; author: string }; /** Nothing is indexed at the path any more. */ gone?: boolean; + /** + * A match run's book matched since the run (it has an ASIN or ISBN now: by hand, + * or elsewhere): an apply leaves it alone, and it has no changes. + */ + matched_since?: boolean; /** What each scope would write: fields (in display order) and the cover. */ changes: Partial>; } diff --git a/admin-ui/src/features/book/book-page.tsx b/admin-ui/src/features/book/book-page.tsx index 4a3ca448..87b7f49e 100644 --- a/admin-ui/src/features/book/book-page.tsx +++ b/admin-ui/src/features/book/book-page.tsx @@ -196,7 +196,12 @@ function BookView({ detail, matchOnOpen }: { detail: AdminBookDetail; matchOnOpe onSaved={reset} onRefused={(field, message) => setRefused((r) => ({ ...r, [field]: message }))} /> - + (s.length > n ? `${s.slice(0, n - 3)}...` * unticked, and so does the community cover for a book with art of its own. * Covers are fetched by the server (the console's CSP loads no other host's * images). Each opening starts afresh, at the search. + * + * It reads the book itself, only while open (on the book page that's the page's + * own query, already loaded); opened elsewhere (the bulk match's review) it waits + * for it, or says why it didn't load. `onMatched` hears that something was + * written to the book. */ export function MatchDialog({ open, onOpenChange, - detail, + libraryId, + path, + onMatched, }: { open: boolean; onOpenChange: (open: boolean) => void; - detail: AdminBookDetail; + libraryId: number; + path: string; + onMatched?: () => void; }) { const { t } = useTranslation(); const [step, setStep] = useState('search'); @@ -94,11 +105,15 @@ export function MatchDialog({ } > {open ? ( - onOpenChange(false)} + onDone={() => { + onMatched?.(); + onOpenChange(false); + }} /> ) : null} @@ -106,6 +121,41 @@ export function MatchDialog({ ); } +/** The book the dialog matches, loaded (or why it didn't), then the dialog's body. */ +function MatchLoader({ + libraryId, + path, + ...body +}: { + libraryId: number; + path: string; + step: Step; + onStep: (step: Step) => void; + onDone: () => void; +}) { + const { t } = useTranslation(); + const book = useAdminBook(libraryId, path); + if (book.data) return ; + if (book.isError) { + return ( + + void book.refetch()} + /> + + ); + } + return ( + + {[0, 1, 2].map((i) => ( +
+ ))} + + ); +} + type Step = 'search' | 'compare'; function MatchBody({ @@ -117,6 +167,7 @@ function MatchBody({ detail: AdminBookDetail; step: Step; onStep: (step: Step) => void; + /** Closes the dialog once something was written to the book. */ onDone: () => void; }) { const { t } = useTranslation(); diff --git a/admin-ui/src/features/health/bulk-match-model.test.ts b/admin-ui/src/features/health/bulk-match-model.test.ts index 50ac3401..6ace03a5 100644 --- a/admin-ui/src/features/health/bulk-match-model.test.ts +++ b/admin-ui/src/features/health/bulk-match-model.test.ts @@ -7,6 +7,7 @@ import { bulkPhase, isChosen, isPickable, + leaveOut, runFraction, togglePick, } from './bulk-match-model'; @@ -115,6 +116,16 @@ describe('picks', () => { expect(applyCount(r, { exclude: new Set([1, 2]), include: new Set([9]) })).toBe(2); expect(applyCount(r, { exclude: new Set([1, 2, 3, 4]), include: new Set() })).toBe(0); }); + + it('leaves a book matched by hand out of the apply, and never offers one matched since', () => { + const picks = leaveOut(leaveOut(togglePick(NO_PICKS, doubtful), doubtful), confident); + expect(isChosen(confident, picks)).toBe(false); + expect(isChosen(doubtful, picks)).toBe(false); + expect(applyRequest('fill', picks)).toEqual({ scope: 'fill', exclude: [1], include: [] }); + expect(isPickable(item({ matched_since: true }))).toBe(false); + // One already applied isn't pending, so leaving it out doesn't lower the count. + expect(leaveOut(NO_PICKS, item({ applied: 'applied' }))).toBe(NO_PICKS); + }); }); describe('regions', () => { diff --git a/admin-ui/src/features/health/bulk-match-model.ts b/admin-ui/src/features/health/bulk-match-model.ts index 674457cb..b787ec0c 100644 --- a/admin-ui/src/features/health/bulk-match-model.ts +++ b/admin-ui/src/features/health/bulk-match-model.ts @@ -44,30 +44,53 @@ export interface Picks { export const NO_PICKS: Picks = { exclude: new Set(), include: new Set() }; +const withId = (set: ReadonlySet, id: number) => new Set(set).add(id); +const withoutId = (set: ReadonlySet, id: number) => { + const next = new Set(set); + next.delete(id); + return next; +}; + /** Whether an apply takes the item: confident unless left out, a review one only if put in. */ export function isChosen(item: MatchRunItem, picks: Picks): boolean { if (!isPickable(item)) return false; return item.outcome === 'auto' ? !picks.exclude.has(item.id) : picks.include.has(item.id); } -/** Whether the item has a checkbox at all: a candidate, not applied yet, its book still there. */ +/** + * Whether the item has a checkbox at all: a candidate, not applied yet, its book + * still there and not matched since the run (an apply leaves that one alone). + */ export function isPickable(item: MatchRunItem): boolean { - return (item.outcome === 'auto' || item.outcome === 'review') && !item.applied && !item.gone; + return ( + (item.outcome === 'auto' || item.outcome === 'review') && + !item.applied && + !item.gone && + !item.matched_since + ); } /** Flips one item's pick. */ export function togglePick(picks: Picks, item: MatchRunItem): Picks { - const flip = (set: ReadonlySet) => { - const next = new Set(set); - if (next.has(item.id)) next.delete(item.id); - else next.add(item.id); - return next; - }; + const flip = (set: ReadonlySet) => + set.has(item.id) ? withoutId(set, item.id) : withId(set, item.id); return item.outcome === 'auto' ? { ...picks, exclude: flip(picks.exclude) } : { ...picks, include: flip(picks.include) }; } +/** + * Leaves an item out of the apply, as a book matched by hand from the review is + * (the server leaves it alone anyway: this keeps the apply's count true). One + * already applied is no pick at all (applyCount counts only pending ones). + */ +export function leaveOut(picks: Picks, item: MatchRunItem): Picks { + if (item.applied) return picks; + return item.outcome === 'auto' + ? { ...picks, exclude: withId(picks.exclude, item.id) } + : { ...picks, include: withoutId(picks.include, item.id) }; +} + /** How many books an apply writes to: the confident ones still pending, less left out, plus put in. */ export function applyCount(run: MatchRun, picks: Picks): number { return Math.max(0, run.counts.pending - picks.exclude.size) + picks.include.size; diff --git a/admin-ui/src/features/health/bulk-match.test.tsx b/admin-ui/src/features/health/bulk-match.test.tsx index fe8ed586..e0181648 100644 --- a/admin-ui/src/features/health/bulk-match.test.tsx +++ b/admin-ui/src/features/health/bulk-match.test.tsx @@ -4,6 +4,7 @@ import { setToken } from '@/api/token'; import type { MatchRun, MatchRunItem } from '@/api/types'; import { mockFetch, type MockRoute } from '@/test/fetch-mock'; import { issuesSummary } from '@/test/fixtures'; +import { bookDetail, books } from '@/test/library-fixtures'; import { renderApp } from '@/test/render-app'; import { signedInRoutes } from '@/test/routes'; @@ -301,6 +302,116 @@ describe('bulk matching', () => { await waitFor(() => expect(issuesAsked()).toBeGreaterThan(before)); }); + // The match dialog over the review: one candidate, no recordings (so no cover). + const byHandRoutes: Record = { + 'GET /admin/libraries/1/book/match': { + body: { + candidates: [ + { + work_id: 'the-martian', + title: 'The Martian', + authors: [{ id: 'a1', name: 'Andy Weir' }], + first_published: '2011', + series: [], + web_url: 'https://meta.audiosilo.app/works/the-martian', + recordings: [], + score: 100, + }, + ], + }, + }, + 'POST /admin/meta/covers': { body: { covers: [''] } }, + }; + + /** Matches the review's confident book by hand, accepting the default ticks. */ + async function matchByHand(user: ReturnType, row: HTMLElement) { + await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); + const match = await screen.findByRole('dialog', { name: 'Match with community metadata' }); + await within(match).findByRole('radio', { name: /The Martian/ }); + await user.click(within(match).getByRole('button', { name: 'Compare fields' })); + const compare = await screen.findByRole('dialog', { name: 'Compare with community metadata' }); + await user.click(within(compare).getByRole('button', { name: /^Accept \d+ field/ })); + await waitFor(() => + expect(screen.queryByRole('dialog', { name: 'Compare with community metadata' })).toBeNull(), + ); + } + + it('matches a book by hand over the review, which keeps its place and leaves it out', async () => { + const detail = bookDetail(); + const calls = mockFetch( + routes([run()], { + 'GET /admin/libraries/1/book': { body: detail }, + 'PATCH /admin/libraries/1/book': { body: detail }, + ...byHandRoutes, + }), + ); + const user = userEvent.setup(); + renderApp('/health?issue=unmatched'); + await user.click(await screen.findByRole('button', { name: 'Review and apply' })); + const review = await screen.findByRole('dialog', { name: 'Review matches' }); + const row = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + // The book page is a new tab away, for a closer look without leaving the review. + expect( + within(row).getByRole('link', { name: 'Open the book page for The Martian in a new tab' }), + ).toHaveAttribute('target', '_blank'); + + // Escape closes the match, not the review under it. + await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); + expect( + await screen.findByRole('dialog', { name: 'Match with community metadata' }), + ).toBeInTheDocument(); + await user.keyboard('{Escape}'); + await waitFor(() => + expect(screen.queryByRole('dialog', { name: 'Match with community metadata' })).toBeNull(), + ); + expect(screen.getByRole('dialog', { name: 'Review matches' })).toBeInTheDocument(); + + await matchByHand(user, row); + expect(calls.some((c) => c.method === 'PATCH' && c.path === '/admin/libraries/1/book')).toBe( + true, + ); + + // Back on the review: the book reads as matched since the run, and the apply leaves it out. + const after = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + expect( + within(after).getByText('Matched since this run: an apply leaves it as it is'), + ).toBeInTheDocument(); + expect(within(after).queryByRole('checkbox')).toBeNull(); + expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); + }); + + it('asks the server again for a book matched by hand that took no ASIN or ISBN', async () => { + const unmatched = bookDetail({ book: { ...books[0], asin: '' } }); + const calls = mockFetch( + routes([run()], { + 'GET /admin/libraries/1/book': { body: unmatched }, + 'PATCH /admin/libraries/1/book': { body: unmatched }, + ...byHandRoutes, + }), + ); + const autoAsked = () => + calls.filter( + (c) => c.path === '/admin/match-runs/5/items' && c.query.get('outcome') === 'auto', + ).length; + const issuesAsked = () => calls.filter((c) => c.path === '/admin/issues').length; + const user = userEvent.setup(); + renderApp('/health?issue=unmatched'); + await user.click(await screen.findByRole('button', { name: 'Review and apply' })); + const review = await screen.findByRole('dialog', { name: 'Review matches' }); + const row = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + const [items, issues] = [autoAsked(), issuesAsked()]; + await matchByHand(user, row); + + // Still unmatched to the server, so not "matched since": the review asks again, + // the Health page under it too, and the book stays left out of the apply. + await waitFor(() => expect(autoAsked()).toBeGreaterThan(items)); + await waitFor(() => expect(issuesAsked()).toBeGreaterThan(issues)); + const after = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + expect(within(after).queryByText(/Matched since this run/)).toBeNull(); + expect(within(after).getByRole('checkbox')).not.toBeChecked(); + expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); + }); + it("shows a gone book's path once, as its title", async () => { const gone: MatchRunItem = { ...confident, diff --git a/admin-ui/src/features/health/bulk-match.tsx b/admin-ui/src/features/health/bulk-match.tsx index cb481ee3..4eaaf982 100644 --- a/admin-ui/src/features/health/bulk-match.tsx +++ b/admin-ui/src/features/health/bulk-match.tsx @@ -1,10 +1,11 @@ import { useEffect, useId, useRef, useState } from 'react'; import { Link } from '@tanstack/react-router'; -import { useQueryClient } from '@tanstack/react-query'; +import { useQueryClient, type InfiniteData } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; -import { ArrowRight, Check, LoaderCircle, Sparkles } from 'lucide-react'; +import { ArrowRight, Check, ExternalLink, LoaderCircle, Sparkles } from 'lucide-react'; import { api } from '@/api/client'; import { + invalidateIssues, invalidateMatches, keys, matchRunActive, @@ -14,9 +15,11 @@ import { } from '@/api/hooks'; import { MATCH_SCOPES, + type AdminBookDetail, type MatchOutcome, type MatchRun, type MatchRunItem, + type MatchRunItemPage, type MatchRunMode, type MatchScope, type SeriesRef, @@ -37,6 +40,7 @@ import { } from '@/components/ui/dialog'; import { RadioCards } from '@/components/ui/radio-cards'; import { SegmentedControl } from '@/components/ui/segmented-control'; +import { MatchDialog } from '@/features/book/match-dialog'; import { scoreTone } from '@/features/book/match-model'; import { moreSeriesRefs, seriesRef } from '@/features/book/more-series'; import { bookRoute } from '@/lib/book-route'; @@ -60,6 +64,7 @@ import { bulkPhase, isChosen, isPickable, + leaveOut, runFraction, runProgress, togglePick, @@ -343,6 +348,9 @@ function ReviewDialog({ const [picks, setPicks] = useState(NO_PICKS); const [scope, setScope] = useState('fill'); const [busy, setBusy] = useState(false); + // The book being matched by hand, in a dialog over the review (which keeps its + // tab, picks and loaded pages underneath). + const [byHand, setByHand] = useState(); // Each opening starts afresh (the run may have changed since). const [wasOpen, setWasOpen] = useState(open); if (open !== wasOpen) { @@ -351,8 +359,39 @@ function ReviewDialog({ setTab(firstTab); setPicks(NO_PICKS); setScope('fill'); + setByHand(undefined); } } + // A book matched by hand leaves the apply, and the Health page under the review + // reads its issues again. A match run's book that took an ASIN or ISBN is one + // the server now calls matched since (matchrun.matchedSince): its row reads so + // at once, without asking again for every page the review has loaded. Any + // other (another mode, an applied item, a hand match that took no identifier) + // the server says what applying would do now. + const onMatchedByHand = (item: MatchRunItem) => { + setPicks((p) => leaveOut(p, item)); + invalidateIssues(qc); + const book = qc.getQueryData(keys.book(item.library_id, item.path))?.book; + const itemsKey = keys.matchRunItems(run.id, item.outcome); + if (run.mode !== 'match' || item.applied || !(book?.asin || book?.isbn)) { + void qc.invalidateQueries({ queryKey: itemsKey }); + return; + } + const now = { title: book.title, author: book.author }; + qc.setQueryData>( + itemsKey, + (data) => + data && { + ...data, + pages: data.pages.map((page) => ({ + ...page, + items: (page.items ?? []).map((it) => + it.id === item.id ? { ...it, matched_since: true, changes: {}, book: now } : it, + ), + })), + }, + ); + }; const count = applyCount(run, picks); const country = regionName(run.region, lang); const headings: Record = { @@ -446,6 +485,7 @@ function ReviewDialog({ scope={repick ? 'ids' : scope} picks={picks} onToggle={(item) => setPicks((p) => togglePick(p, item))} + onMatchByHand={setByHand} /> @@ -457,6 +497,16 @@ function ReviewDialog({ {t('health.bulkMatch.apply', counted(count, lang))} + {byHand ? ( + setByHand(undefined)} + libraryId={byHand.library_id} + path={byHand.path} + onMatched={() => onMatchedByHand(byHand)} + /> + ) : null} ) : null} @@ -471,12 +521,14 @@ function ItemList({ scope, picks, onToggle, + onMatchByHand, }: { run: MatchRun; outcome: MatchOutcome; scope: MatchScope; picks: Picks; onToggle: (item: MatchRunItem) => void; + onMatchByHand: (item: MatchRunItem) => void; }) { const { t } = useTranslation(); const list = useMatchRunItems(run.id, outcome); @@ -521,6 +573,7 @@ function ItemList({ scope={scope} chosen={isChosen(item, picks)} onToggle={() => onToggle(item)} + onMatchByHand={() => onMatchByHand(item)} /> ))} @@ -545,6 +598,7 @@ function ItemRow({ scope, chosen, onToggle, + onMatchByHand, }: { item: MatchRunItem; /** The library's folder: the book's whole path is the path's tooltip. */ @@ -552,12 +606,17 @@ function ItemRow({ scope: MatchScope; chosen: boolean; onToggle: () => void; + onMatchByHand: () => void; }) { const { t, i18n } = useTranslation(); const lang = i18n.resolvedLanguage ?? 'en'; const p = item.proposal; const title = item.book.title || item.path; const change = item.changes[scope]; + // What became of the book: applied, skipped or failed, or matched since the run + // (an apply leaves it alone, and says so). + const status = + item.matched_since || item.detail === 'matched_since' ? 'matchedSince' : item.applied; const changed = change ? [ ...(change.cover ? [t('book.match.cover')] : []), @@ -605,8 +664,11 @@ function ItemRow({ aria-label={t('health.bulkMatch.pick', { title })} aria-describedby={pathLine} /> - ) : item.applied === 'applied' ? ( - + ) : status === 'applied' || status === 'matchedSince' ? ( + ) : null} @@ -670,9 +732,9 @@ function ItemRow({ {t('health.bulkMatch.item.gone')} - ) : item.applied ? ( + ) : status ? ( - {t(`health.bulkMatch.item.${item.applied}`)} + {t(`health.bulkMatch.item.${status}`)} ) : candidate ? ( @@ -694,16 +756,29 @@ function ItemRow({ ) : null} {item.gone ? null : ( - - {t('health.bulkMatch.matchByHandShort')} -
diff --git a/admin-ui/src/i18n/locales/de.json b/admin-ui/src/i18n/locales/de.json index 23f66d0e..32d147ed 100644 --- a/admin-ui/src/i18n/locales/de.json +++ b/admin-ui/src/i18n/locales/de.json @@ -1183,8 +1183,10 @@ "health.bulkMatch.item.identifierUnknown": "Die Community kennt die ASIN oder ISBN dieses Buchs nicht", "health.bulkMatch.item.error": "Der Community-Dienst hat für dieses Buch nicht geantwortet", "health.bulkMatch.item.gone": "Nicht mehr in der Bibliothek", + "health.bulkMatch.item.matchedSince": "Seit diesem Lauf zugeordnet: Das Übernehmen lässt es, wie es ist", "health.bulkMatch.matchByHand": "{{title}} von Hand zuordnen", "health.bulkMatch.matchByHandShort": "Von Hand", + "health.bulkMatch.openBook": "Buchseite von {{title}} in einem neuen Tab öffnen", "health.reason.no_chapters": "Über {{hours}} Stunden ohne Kapitelmarken", "health.reason.detailed_chapters_one": "Hat {{count}} Kapitel; die feineren der Community passen zu diesem Exemplar", "health.reason.detailed_chapters_other": "Hat {{count}} Kapitel; die feineren der Community passen zu diesem Exemplar", diff --git a/admin-ui/src/i18n/locales/en.json b/admin-ui/src/i18n/locales/en.json index 8c8719fc..dc1d625b 100644 --- a/admin-ui/src/i18n/locales/en.json +++ b/admin-ui/src/i18n/locales/en.json @@ -1183,8 +1183,10 @@ "health.bulkMatch.item.identifierUnknown": "The community doesn't know this book's ASIN or ISBN", "health.bulkMatch.item.error": "The community service didn't answer for this book", "health.bulkMatch.item.gone": "No longer in the library", + "health.bulkMatch.item.matchedSince": "Matched since this run: an apply leaves it as it is", "health.bulkMatch.matchByHand": "Match {{title}} by hand", "health.bulkMatch.matchByHandShort": "Match by hand", + "health.bulkMatch.openBook": "Open the book page for {{title}} in a new tab", "health.reason.no_chapters": "Over {{hours}} hours with no chapter marks", "health.reason.detailed_chapters_one": "Has {{count}} chapter; the community’s finer ones fit this copy", "health.reason.detailed_chapters_other": "Has {{count}} chapters; the community’s finer ones fit this copy", diff --git a/admin-ui/src/i18n/locales/es.json b/admin-ui/src/i18n/locales/es.json index c118b7a7..d45d5641 100644 --- a/admin-ui/src/i18n/locales/es.json +++ b/admin-ui/src/i18n/locales/es.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La comunidad no conoce el ASIN o ISBN de este libro", "health.bulkMatch.item.error": "El servicio de la comunidad no respondió para este libro", "health.bulkMatch.item.gone": "Ya no está en la biblioteca", + "health.bulkMatch.item.matchedSince": "Asociado después de esta búsqueda: aplicar lo deja como está", "health.bulkMatch.matchByHand": "Asociar {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", + "health.bulkMatch.openBook": "Abrir la página de {{title}} en una pestaña nueva", "health.reason.no_chapters": "Más de {{hours}} horas sin marcas de capítulo", "health.reason.detailed_chapters_one": "Tiene {{count}} capítulo; los más finos de la comunidad encajan con esta copia", "health.reason.detailed_chapters_other": "Tiene {{count}} capítulos; los más finos de la comunidad encajan con esta copia", diff --git a/admin-ui/src/i18n/locales/fr.json b/admin-ui/src/i18n/locales/fr.json index 7b08b62a..8275774f 100644 --- a/admin-ui/src/i18n/locales/fr.json +++ b/admin-ui/src/i18n/locales/fr.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La communauté ne connaît pas l'ASIN ou l'ISBN de ce livre", "health.bulkMatch.item.error": "Le service communautaire n'a pas répondu pour ce livre", "health.bulkMatch.item.gone": "N'est plus dans la bibliothèque", + "health.bulkMatch.item.matchedSince": "Associé depuis cette recherche : l'application le laisse tel quel", "health.bulkMatch.matchByHand": "Associer {{title}} à la main", "health.bulkMatch.matchByHandShort": "À la main", + "health.bulkMatch.openBook": "Ouvrir la page de {{title}} dans un nouvel onglet", "health.reason.no_chapters": "Plus de {{hours}} heures sans marques de chapitre", "health.reason.detailed_chapters_one": "{{count}} chapitre ; ceux, plus fins, de la communauté conviennent à cet exemplaire", "health.reason.detailed_chapters_other": "{{count}} chapitres ; ceux, plus fins, de la communauté conviennent à cet exemplaire", diff --git a/admin-ui/src/i18n/locales/it.json b/admin-ui/src/i18n/locales/it.json index 4eed4e29..e9b7e530 100644 --- a/admin-ui/src/i18n/locales/it.json +++ b/admin-ui/src/i18n/locales/it.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La community non conosce l'ASIN o l'ISBN di questo libro", "health.bulkMatch.item.error": "Il servizio della community non ha risposto per questo libro", "health.bulkMatch.item.gone": "Non è più nella libreria", + "health.bulkMatch.item.matchedSince": "Associato dopo questa ricerca: l'applicazione lo lascia com'è", "health.bulkMatch.matchByHand": "Associa {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", + "health.bulkMatch.openBook": "Apri la pagina di {{title}} in una nuova scheda", "health.reason.no_chapters": "Oltre {{hours}} ore senza segni di capitolo", "health.reason.detailed_chapters_one": "Ha {{count}} capitolo; quelli più fini della community sono adatti a questa copia", "health.reason.detailed_chapters_other": "Ha {{count}} capitoli; quelli più fini della community sono adatti a questa copia", diff --git a/admin-ui/src/i18n/locales/pt.json b/admin-ui/src/i18n/locales/pt.json index c735e9ab..d0bc8a64 100644 --- a/admin-ui/src/i18n/locales/pt.json +++ b/admin-ui/src/i18n/locales/pt.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "A comunidade não conhece o ASIN ou ISBN deste livro", "health.bulkMatch.item.error": "O serviço da comunidade não respondeu para este livro", "health.bulkMatch.item.gone": "Já não está na biblioteca", + "health.bulkMatch.item.matchedSince": "Associado depois desta pesquisa: aplicar deixa-o como está", "health.bulkMatch.matchByHand": "Associar {{title}} manualmente", "health.bulkMatch.matchByHandShort": "Manualmente", + "health.bulkMatch.openBook": "Abrir a página de {{title}} num novo separador", "health.reason.no_chapters": "Mais de {{hours}} horas sem marcas de capítulo", "health.reason.detailed_chapters_one": "Tem {{count}} capítulo; os mais finos da comunidade ajustam-se a esta cópia", "health.reason.detailed_chapters_other": "Tem {{count}} capítulos; os mais finos da comunidade ajustam-se a esta cópia", diff --git a/internal/api/handlers_match_runs.go b/internal/api/handlers_match_runs.go index aca468d0..b3738206 100644 --- a/internal/api/handlers_match_runs.go +++ b/internal/api/handlers_match_runs.go @@ -145,9 +145,10 @@ func (a *API) handleGetMatchRun(w http.ResponseWriter, r *http.Request) { } // handleMatchRunItems serves GET /admin/match-runs/{id}/items: a page of the -// run's books (?outcome=auto|review|none|error, ?after=, ?limit=), each -// with the best candidate, the book as it is now and what each scope would change; -// next_after reads the next page (0 = the last). +// run's books, the best match first (?outcome=auto|review|none|error, +// ?after=, ?limit=), each with the best candidate, the book as it is now +// and what each scope would change; next_after reads the next page (0 = the +// last). func (a *API) handleMatchRunItems(w http.ResponseWriter, r *http.Request) { if a.metadataOff(w) { return diff --git a/internal/catalog/matchruns.go b/internal/catalog/matchruns.go index d3cc2410..9fd85e76 100644 --- a/internal/catalog/matchruns.go +++ b/internal/catalog/matchruns.go @@ -314,13 +314,19 @@ func scanMatchItem(rows *sql.Rows, it *MatchRunItem) error { const matchItemCols = `id, run_id, library_id, path, outcome, score, runner_up, proposal, applied, detail` -// ListMatchRunItems returns a page of a run's items in id order: of one outcome -// ("" = all), after the item `after`, at most limit, and the id the next page -// reads after (0 when this is the last). +// ListMatchRunItems returns a page of a run's items, the best match first (score +// descending, then id): of one outcome ("" = all), after the item `after`, at most +// limit, and the id the next page reads after (0 when this is the last). The +// cursor stays an item id: the page reads on from that item's score, which never +// changes once recorded, so a review paged 50 at a time sees the whole outcome in +// one order rather than each page sorted on its own. An `after` that isn't one of +// the run's items reads nothing (the row it compares with is all NULL). func (c *Catalog) ListMatchRunItems(ctx context.Context, runID int64, outcome string, after int64, limit int) ([]MatchRunItem, int64, error) { items, err := queryRows(ctx, c.db, scanMatchItem, `SELECT `+matchItemCols+` FROM match_run_items - WHERE run_id = ? AND (? = '' OR outcome = ?) AND id > ? ORDER BY id LIMIT ?`, - runID, outcome, outcome, after, limit+1) + WHERE run_id = ? AND (? = '' OR outcome = ?) + AND (? = 0 OR (score, -id) < (SELECT score, -id FROM match_run_items WHERE id = ? AND run_id = ?)) + ORDER BY score DESC, id LIMIT ?`, + runID, outcome, outcome, after, after, runID, limit+1) if err != nil { return nil, 0, err } diff --git a/internal/catalog/matchruns_test.go b/internal/catalog/matchruns_test.go index ea1f2c1e..9450f28d 100644 --- a/internal/catalog/matchruns_test.go +++ b/internal/catalog/matchruns_test.go @@ -2,6 +2,7 @@ package catalog import ( "errors" + "fmt" "strings" "testing" ) @@ -43,6 +44,62 @@ func TestRecordMatchItemDeletedLibrary(t *testing.T) { } } +// TestListMatchRunItemsBestFirst: a run's items page best match first (score +// descending, a tie in the order they were recorded) across pages, so a review read +// a page at a time sees one order; an outcome reads only its own items, and an +// `after` from another run reads nothing. +func TestListMatchRunItemsBestFirst(t *testing.T) { + t.Parallel() + c, ctx := newTestCatalog(t) + lib, _ := c.CreateLibrary(ctx, Library{Name: "One", Root: "/tmp/one"}) + run, err := c.StartMatchRun(ctx, MatchRun{Mode: MatchModeMatch, Total: 7}) + if err != nil { + t.Fatal(err) + } + other, err := c.StartMatchRun(ctx, MatchRun{Mode: MatchModeMatch, Total: 1}) + if err != nil { + t.Fatal(err) + } + for i, score := range []int{70, 95, 70, 60, 95, 80} { + item := &MatchRunItem{LibraryID: lib.ID, Path: fmt.Sprintf("A/%d-%d", i, score), Outcome: OutcomeReview, Score: score} + if err := c.RecordMatchItem(ctx, run, item); err != nil { + t.Fatal(err) + } + } + if err := c.RecordMatchItem(ctx, run, &MatchRunItem{LibraryID: lib.ID, Path: "A/none", Outcome: OutcomeNone}); err != nil { + t.Fatal(err) + } + if err := c.RecordMatchItem(ctx, other, &MatchRunItem{LibraryID: lib.ID, Path: "B/other", Outcome: OutcomeReview, Score: 99}); err != nil { + t.Fatal(err) + } + + var got []string + var after int64 + for range 10 { + items, next, err := c.ListMatchRunItems(ctx, run, OutcomeReview, after, 2) + if err != nil { + t.Fatal(err) + } + for _, it := range items { + got = append(got, it.Path) + } + if after = next; after == 0 { + break + } + } + if want := "A/1-95,A/4-95,A/5-80,A/0-70,A/2-70,A/3-60"; strings.Join(got, ",") != want { + t.Errorf("pages = %s, want %s", strings.Join(got, ","), want) + } + + foreign, _, err := c.ListMatchRunItems(ctx, other, OutcomeReview, 0, 10) + if err != nil || len(foreign) != 1 { + t.Fatalf("other run = %d items %v, want its one", len(foreign), err) + } + if items, next, err := c.ListMatchRunItems(ctx, run, OutcomeReview, foreign[0].ID, 10); err != nil || len(items) != 0 || next != 0 { + t.Errorf("after another run's item = %d items next %d %v, want none", len(items), next, err) + } +} + // TestClearCommunityMatches: clearing one library removes its community overrides // and covers and rebuilds its books (the scanned values back, an admin's own edit, // upload and the manager's enrichment kept), drops its match runs and its books' diff --git a/internal/matchrun/plan.go b/internal/matchrun/plan.go index a5f9147c..c2c74e65 100644 --- a/internal/matchrun/plan.go +++ b/internal/matchrun/plan.go @@ -281,11 +281,13 @@ func planSeries(scope string, st *catalog.MatchState, p catalog.MatchProposal, w } } -// planFor is what applying p to a book in state st writes for a run in mode: a -// repick's ASIN (whatever the scope), else Plan's (a refresh's proposal carries -// no identifier, so its Plan writes none: refreshBook), and for a refresh only -// while the book still has the identifier it was found by. -func planFor(mode, scope string, st *catalog.MatchState, p catalog.MatchProposal) Writes { +// planFor is what applying item it (its proposal) to a book in state st writes for a +// run in mode: a repick's ASIN (whatever the scope), else Plan's (a refresh's +// proposal carries no identifier, so its Plan writes none: refreshBook), for a +// refresh only while the book still has the identifier it was found by, and for +// a match only while the book is still unmatched (matchedSince). +func planFor(mode, scope string, st *catalog.MatchState, it *catalog.MatchRunItem) Writes { + p := it.Proposal switch mode { case catalog.MatchModeRepick: return Writes{Set: PlanRepick(st, p)} @@ -293,10 +295,28 @@ func planFor(mode, scope string, st *catalog.MatchState, p catalog.MatchProposal if !stillFoundBy(st, p.FoundBy) { return Writes{} } + case catalog.MatchModeMatch: + if matchedSince(st, it) { + return Writes{} + } } return Plan(scope, st, p) } +// matchedSince reports whether a match run's book has an ASIN or ISBN now. The +// run took only books with neither (UnmatchedBooks), so one has been matched +// since: by hand (from the review or the book page), by another admin or the +// manager, or by a rescan reading one from the tags. The run's match never +// writes over it, whatever the scope. An item whose own apply a stop cut short +// after its fields went in (DetailCoverPending) holds the run's identifier, not +// one matched since. +func matchedSince(st *catalog.MatchState, it *catalog.MatchRunItem) bool { + if it.Detail == DetailCoverPending { + return false + } + return st.Fields[catalog.FieldASIN].Value != "" || st.Fields[catalog.FieldISBN].Value != "" +} + // stillFoundBy reports whether the book still has the identifier a refresh // found its record by (Proposal.FoundBy): a book matched again since (by hand, // to another record) is not overwritten with the old one's details. diff --git a/internal/matchrun/runner.go b/internal/matchrun/runner.go index e7a53c8c..d1ef9326 100644 --- a/internal/matchrun/runner.go +++ b/internal/matchrun/runner.go @@ -35,6 +35,11 @@ const ( // know the book's ASIN or ISBN. const DetailIdentifierUnknown = "identifier_unknown" +// DetailCoverPending is a match item's detail while its fields went in but a stop +// cut its cover short: still to apply, so the next apply takes the cover, the +// ASIN or ISBN the book has now being the run's own (not matchedSince). +const DetailCoverPending = "cover_pending" + // ErrBusy is a start or apply asked while a run is working: one at a time. var ErrBusy = errors.New("a match run is already working") @@ -413,10 +418,17 @@ func (r *Runner) apply(ctx context.Context, run *catalog.MatchRun, items []catal pool.Each(ctx, workers, items, enabled, func(it *catalog.MatchRunItem) { applied, detail := r.applyItem(ctx, run.Mode, it, o) // Stopped mid-item: what the stop may have cut short (a failure, a cover - // not taken) is not marked, so the next apply takes it again; what went in - // is, so it counts as applied rather than coming back as nothing to change. + // not taken) stays to apply, so the next apply takes it again; what went in + // is marked, so it counts as applied rather than coming back as nothing to + // change. if ctx.Err() != nil && (applied == catalog.ItemFailed || detail == "cover_failed") { - return + if applied == catalog.ItemFailed || run.Mode != catalog.MatchModeMatch { + return + } + // A match's fields went in, its cover didn't: still to apply, marked + // as the run's own, as the ASIN the book has now would otherwise read + // as matched since and the cover would never be taken. + applied, detail = "", DetailCoverPending } if err := r.cat.MarkMatchItem(context.WithoutCancel(ctx), run.ID, it.ID, applied, detail); err != nil { r.log.Error("mark match item failed", "run", run.ID, "item", it.ID, "err", err) @@ -446,7 +458,10 @@ func (r *Runner) applyItem(ctx context.Context, mode string, it *catalog.MatchRu } return catalog.ItemFailed, "edit_failed" } - w := planFor(mode, o.Scope, st, it.Proposal) + if mode == catalog.MatchModeMatch && matchedSince(st, it) { + return catalog.ItemSkipped, "matched_since" + } + w := planFor(mode, o.Scope, st, it) if w.Empty() { return catalog.ItemSkipped, "nothing_to_change" } @@ -516,9 +531,12 @@ func changeOf(st *catalog.MatchState, w Writes) Change { // nothing is indexed at its path any more) and what each scope would change. type ItemView struct { catalog.MatchRunItem - Book ItemBook `json:"book"` - Gone bool `json:"gone,omitempty"` - Changes map[string]Change `json:"changes"` + Book ItemBook `json:"book"` + Gone bool `json:"gone,omitempty"` + // MatchedSince: a match run's book matched since the run (matchedSince), + // which an apply leaves alone; its changes are none. + MatchedSince bool `json:"matched_since,omitempty"` + Changes map[string]Change `json:"changes"` } // ItemBook is the book an item is for, as it is now. @@ -555,8 +573,9 @@ func (r *Runner) Items(ctx context.Context, run *catalog.MatchRun, outcome strin return nil, 0, err default: v.Book = ItemBook{Title: st.Title, Author: st.Author} + v.MatchedSince = run.Mode == catalog.MatchModeMatch && it.Applied == "" && matchedSince(st, &it) for _, scope := range scopes { - v.Changes[scope] = changeOf(st, planFor(run.Mode, scope, st, it.Proposal)) + v.Changes[scope] = changeOf(st, planFor(run.Mode, scope, st, &it)) } } out[i] = v diff --git a/internal/matchrun/runner_test.go b/internal/matchrun/runner_test.go index 7134e8bb..4d063052 100644 --- a/internal/matchrun/runner_test.go +++ b/internal/matchrun/runner_test.go @@ -255,6 +255,54 @@ func TestApplyExcludesAndSkips(t *testing.T) { } } +// TestApplyLeavesABookMatchedSince: a match run's book matched since the run (by +// hand, here to another work) reads as matched since in the review, with nothing +// to change under any scope, and an apply, even one that overwrites, leaves it +// alone while it matches the rest. +func TestApplyLeavesABookMatchedSince(t *testing.T) { + e := newEnv(t) + ctx := context.Background() + e.book(t, "A/One", "One", "", nil) + e.book(t, "A/Two", "Two", "", nil) + for _, title := range []string{"One", "Two"} { + e.matcher.byTitle[title] = []meta.MatchCandidate{cand("the-martian", 97, "")} + } + run := e.start(t, catalog.MatchModeMatch, "") + if err := e.cat.EditBook(ctx, e.lib, "A/Two", catalog.BookEdit{ + Set: map[string]string{catalog.FieldASIN: "B0HAND0001", catalog.FieldTitle: "By Hand"}, + Source: catalog.SourceCommunity, + }); err != nil { + t.Fatal(err) + } + for _, it := range e.items(t, run, catalog.OutcomeAuto) { + hand := it.Path == "A/Two" + if it.MatchedSince != hand { + t.Errorf("%s matched since = %v, want %v", it.Path, it.MatchedSince, hand) + } + if hand && len(it.Changes[ScopeOverwrite].Fields) != 0 { + t.Errorf("matched since changes = %+v, want none", it.Changes) + } + } + + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeOverwrite}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + for _, it := range e.items(t, run, catalog.OutcomeAuto) { + want := map[string]string{"A/One": catalog.ItemApplied + "/", "A/Two": catalog.ItemSkipped + "/matched_since"}[it.Path] + if got := it.Applied + "/" + it.Detail; got != want { + t.Errorf("%s = %s, want %s", it.Path, got, want) + } + if it.MatchedSince { + t.Errorf("%s still reads as matched since once applied", it.Path) + } + } + st, _ := e.cat.BookMatchState(ctx, e.lib, "A/Two") + if st.Fields[catalog.FieldASIN].Value != "B0HAND0001" || st.Fields[catalog.FieldTitle].Value != "By Hand" { + t.Fatalf("the hand match was overwritten: %+v", st.Fields) + } +} + func TestRunStopsWhenTheServiceIsDown(t *testing.T) { e := newEnv(t) for i := range 12 { @@ -521,6 +569,40 @@ func TestCancelledApplyMarksWhatWentIn(t *testing.T) { } } +// TestStoppedCoverIsTakenAgain: a match whose fields went in before a stop cut +// its cover short stays to apply, not matched since (the ASIN the book has now +// is the run's own), and the next apply takes the cover. +func TestStoppedCoverIsTakenAgain(t *testing.T) { + e := newEnv(t) + ctx := context.Background() + e.book(t, "A/a", "Book", "", no) + e.matcher.byTitle["Book"] = []meta.MatchCandidate{cand("the-martian", 97, "https://c/rec.jpg")} + run := e.start(t, catalog.MatchModeMatch, "") + save := e.runner.saveCover + e.runner.saveCover = func(ctx context.Context, _ int64, _, _ string, _ int64) error { + e.runner.Cancel(run.ID) + return ctx.Err() + } + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeFill}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + it := e.items(t, run, catalog.OutcomeAuto)[0] + if it.Applied != "" || it.MatchedSince || !it.Changes[ScopeFill].Cover { + t.Fatalf("item after the stop = %+v, want still to apply, its cover to take", it) + } + + e.runner.saveCover = save + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeFill}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + it = e.items(t, run, catalog.OutcomeAuto)[0] + if it.Applied != catalog.ItemApplied || it.Detail != "" || e.covers.Load() != 1 { + t.Fatalf("item = %+v, covers %d; want applied with the cover taken", it.MatchRunItem, e.covers.Load()) + } +} + func TestApplyCoverFailureAndInterruptedApply(t *testing.T) { e := newEnv(t) ctx := context.Background()