From 327a5dcef14219a961df51ca205dd7046d5ded46 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 22 Sep 2026 13:58:26 -0700 Subject: [PATCH 01/10] Resolve suppress-work identifier lookup through equivalencies SuppressWorkForLibraryScript looked up a Work via Identifier.work, which only follows LicensePools whose own identifier matches exactly. Librarians typically supply an ISBN, but a LicensePool is often keyed on a vendor identifier with the ISBN linked only via identifier equivalency, so the script reported NOT_FOUND for titles that are clearly circulating. suppress_work now resolves the identifier through Work.from_identifiers, the same strict, high-confidence equivalency lookup used elsewhere in the codebase. If an identifier's equivalency network resolves to more than one distinct Work, the script reports a new AMBIGUOUS result and suppresses nothing rather than guessing. Co-Authored-By: Claude Sonnet 5 --- src/palace/manager/scripts/suppress.py | 41 +++++++++++- tests/manager/scripts/test_suppress.py | 90 ++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 25acef596c..7f010a14b9 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -13,12 +13,14 @@ from palace.manager.scripts.base import Script, _normalize_cmd_args from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.sqlalchemy.model.library import Library +from palace.manager.sqlalchemy.model.work import Work class SuppressResult(Enum): NEWLY_SUPPRESSED = auto() ALREADY_SUPPRESSED = auto() NOT_FOUND = auto() + AMBIGUOUS = auto() class SuppressWorkForLibraryScript(Script): @@ -142,21 +144,53 @@ def load_identifiers_from_file( raise PalaceValueError(f"CSV file not found: {file_path}") return identifiers + def load_works(self, identifier: Identifier) -> list[Work]: + """Find the Work(s) reachable from an identifier. + + This looks past LicensePools whose own identifier matches, to + also include LicensePools reachable through identifier + equivalency -- e.g. an ISBN a librarian has on hand is often + linked to a vendor's LicensePool via metadata equivalency + rather than being that LicensePool's own identifier. This uses + the same strict, high-confidence equivalency policy that + `Work.from_identifiers` applies by default everywhere else in + the codebase, so it won't walk into loosely-related works. + """ + query = Work.from_identifiers(self._db, [identifier]) + if query is None: + return [] + return cast(list[Work], query.distinct().all()) + def suppress_work( self, library: Library, identifier: Identifier, dry_run: bool = False, ) -> SuppressResult: - work = identifier.work - if not work: + works = self.load_works(identifier) + if not works: self.log.warning(f"No work found for {identifier}") return SuppressResult.NOT_FOUND + if len(works) > 1: + self.log.warning( + f"{identifier.type}/{identifier.identifier} resolves to " + f"{len(works)} different works via identifier equivalency; " + "skipping rather than guessing which one to suppress." + ) + return SuppressResult.AMBIGUOUS + + work = works[0] + if library in work.suppressed_for: return SuppressResult.ALREADY_SUPPRESSED if not dry_run: + # Suppression is scoped to exactly this one library. Resolving + # the work via identifier equivalency only changes *which work* + # we find -- it never changes *which libraries* it's suppressed + # for, since only the `library` argument passed in is ever + # appended to `suppressed_for`. work.suppressed_for.append(library) self.log.info( @@ -225,6 +259,7 @@ def _print_results( k for k, v in results.items() if v == SuppressResult.ALREADY_SUPPRESSED ] not_found = [k for k, v in results.items() if v == SuppressResult.NOT_FOUND] + ambiguous = [k for k, v in results.items() if v == SuppressResult.AMBIGUOUS] prefix = "[DRY RUN] " if dry_run else "" suppress_label = "Would suppress" if dry_run else "Newly suppressed" @@ -236,6 +271,7 @@ def _print_results( (suppress_label + ":", len(newly_suppressed)), ("Already suppressed:", len(already_suppressed)), ("Not found:", len(not_found)), + ("Ambiguous:", len(ambiguous)), ] col = max(len(label) for label, _ in summary_rows) print(f"\n{prefix}Suppression Results Summary:") @@ -249,6 +285,7 @@ def _print_results( ), SuppressResult.ALREADY_SUPPRESSED: "ALREADY SUPPRESSED", SuppressResult.NOT_FOUND: "NOT FOUND", + SuppressResult.AMBIGUOUS: "AMBIGUOUS", } for (id_type, id_value), result in results.items(): status = status_map[result] diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 11894cd955..0a4970fb40 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -8,6 +8,7 @@ import pytest from palace.manager.scripts.suppress import SuppressResult, SuppressWorkForLibraryScript +from palace.manager.sqlalchemy.model.datasource import DataSource from tests.fixtures.database import DatabaseTransactionFixture @@ -379,6 +380,76 @@ def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixtu assert result == SuppressResult.NOT_FOUND + def test_suppress_work_resolves_via_equivalent_identifier( + self, db: DatabaseTransactionFixture + ): + """A librarian will typically have an ISBN in hand, but the + LicensePool is often keyed on a vendor identifier (e.g. an + Overdrive ID) with the ISBN linked only via identifier + equivalency. The script must resolve the work through that + equivalency instead of requiring the ISBN to be the + LicensePool's own identifier.""" + test_library = db.library(short_name="test") + work = db.work(with_license_pool=True) + pool_identifier = work.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, pool_identifier, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result == SuppressResult.NEWLY_SUPPRESSED + assert work.suppressed_for == [test_library] + + def test_suppress_work_equivalent_identifier_only_affects_specified_library( + self, db: DatabaseTransactionFixture + ): + """Resolving the work via identifier equivalency must never + broaden *which libraries* get the work suppressed -- only the + library explicitly passed in should end up in + `work.suppressed_for`.""" + library_a = db.library(short_name="lib_a") + library_b = db.library(short_name="lib_b") + work = db.work(with_license_pool=True) + pool_identifier = work.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, pool_identifier, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(library_a, isbn) + + assert result == SuppressResult.NEWLY_SUPPRESSED + assert work.suppressed_for == [library_a] + assert library_b not in work.suppressed_for + + def test_suppress_work_ambiguous_equivalent_identifier( + self, db: DatabaseTransactionFixture + ): + """If an identifier resolves to more than one distinct Work via + equivalency, the script must not guess -- it should report + AMBIGUOUS and suppress nothing.""" + test_library = db.library(short_name="test") + work1 = db.work(with_license_pool=True) + work2 = db.work(with_license_pool=True) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result == SuppressResult.AMBIGUOUS + assert work1.suppressed_for == [] + assert work2.suppressed_for == [] + def test_suppress_work_dry_run(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") work = db.work(with_license_pool=True) @@ -439,6 +510,25 @@ def test_print_results_normal(self, db: DatabaseTransactionFixture, capsys): assert "[NOT FOUND] ISBN/333" in out assert "[DRY RUN]" not in out + def test_print_results_ambiguous(self, db: DatabaseTransactionFixture, capsys): + test_library = db.library(short_name="mylib", name="My Library") + script = SuppressWorkForLibraryScript(db.session) + results = { + ("ISBN", "111"): SuppressResult.AMBIGUOUS, + } + started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) + script._print_results( + results, + dry_run=False, + library=test_library, + started_at=started_at, + duration_seconds=1.23, + ) + + out = capsys.readouterr().out + assert re.search(r"Ambiguous:\s+1", out) + assert "[AMBIGUOUS] ISBN/111" in out + def test_print_results_dry_run(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) From ebb992718b8a19dfd12d4eaaec952036f2879064 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 22 Sep 2026 14:05:30 -0700 Subject: [PATCH 02/10] Include the work title in suppress-work script output suppress_work now returns a SuppressOutcome(result, title) instead of a bare SuppressResult, so the per-identifier output line and log messages can show which title was affected -- useful for confirming the right book was found, especially since identifiers now resolve through equivalencies. For AMBIGUOUS, all candidate titles are listed to help a human pick the right one. Co-Authored-By: Claude Sonnet 5 --- src/palace/manager/scripts/suppress.py | 52 +++++++++++++------- tests/manager/scripts/test_suppress.py | 67 +++++++++++++++++--------- 2 files changed, 79 insertions(+), 40 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 7f010a14b9..0ecff4f42d 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -3,7 +3,7 @@ from collections.abc import Sequence from datetime import datetime, timezone from enum import Enum, auto -from typing import cast +from typing import NamedTuple, cast from sqlalchemy import select from sqlalchemy.orm import Session @@ -23,6 +23,14 @@ class SuppressResult(Enum): AMBIGUOUS = auto() +class SuppressOutcome(NamedTuple): + result: SuppressResult + # The title of the resolved work, when there is exactly one. For + # AMBIGUOUS, this instead lists the titles of every candidate work, + # to help a human resolve the ambiguity. + title: str | None = None + + class SuppressWorkForLibraryScript(Script): """Suppress works from a library by identifier""" @@ -166,24 +174,25 @@ def suppress_work( library: Library, identifier: Identifier, dry_run: bool = False, - ) -> SuppressResult: + ) -> SuppressOutcome: works = self.load_works(identifier) if not works: self.log.warning(f"No work found for {identifier}") - return SuppressResult.NOT_FOUND + return SuppressOutcome(SuppressResult.NOT_FOUND) if len(works) > 1: + titles = "; ".join(w.title for w in works if w.title) self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " f"{len(works)} different works via identifier equivalency; " "skipping rather than guessing which one to suppress." ) - return SuppressResult.AMBIGUOUS + return SuppressOutcome(SuppressResult.AMBIGUOUS, titles or None) work = works[0] if library in work.suppressed_for: - return SuppressResult.ALREADY_SUPPRESSED + return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, work.title) if not dry_run: # Suppression is scoped to exactly this one library. Resolving @@ -198,7 +207,7 @@ def suppress_work( f"{identifier.type}/{identifier.identifier} (work id: {work.id}) " f"for {library.short_name}." ) - return SuppressResult.NEWLY_SUPPRESSED + return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, work.title) def do_run(self, cmd_args: list[str] | None = None) -> None: parsed = self.parse_command_line(self._db, cmd_args=cmd_args) @@ -223,15 +232,15 @@ def do_run(self, cmd_args: list[str] | None = None) -> None: ) pairs = unique_pairs - results: dict[tuple[str, str], SuppressResult] = {} + results: dict[tuple[str, str], SuppressOutcome] = {} try: for id_type, id_value in pairs: try: identifier = self.load_identifier(id_type, id_value) - result = self.suppress_work(library, identifier, dry_run=dry_run) + outcome = self.suppress_work(library, identifier, dry_run=dry_run) except PalaceValueError: - result = SuppressResult.NOT_FOUND - results[(id_type, id_value)] = result + outcome = SuppressOutcome(SuppressResult.NOT_FOUND) + results[(id_type, id_value)] = outcome if not dry_run: self._db.commit() @@ -246,20 +255,26 @@ def do_run(self, cmd_args: list[str] | None = None) -> None: def _print_results( self, - results: dict[tuple[str, str], SuppressResult], + results: dict[tuple[str, str], SuppressOutcome], dry_run: bool, library: Library, started_at: datetime, duration_seconds: float, ) -> None: newly_suppressed = [ - k for k, v in results.items() if v == SuppressResult.NEWLY_SUPPRESSED + k for k, v in results.items() if v.result == SuppressResult.NEWLY_SUPPRESSED ] already_suppressed = [ - k for k, v in results.items() if v == SuppressResult.ALREADY_SUPPRESSED + k + for k, v in results.items() + if v.result == SuppressResult.ALREADY_SUPPRESSED + ] + not_found = [ + k for k, v in results.items() if v.result == SuppressResult.NOT_FOUND + ] + ambiguous = [ + k for k, v in results.items() if v.result == SuppressResult.AMBIGUOUS ] - not_found = [k for k, v in results.items() if v == SuppressResult.NOT_FOUND] - ambiguous = [k for k, v in results.items() if v == SuppressResult.AMBIGUOUS] prefix = "[DRY RUN] " if dry_run else "" suppress_label = "Would suppress" if dry_run else "Newly suppressed" @@ -287,6 +302,7 @@ def _print_results( SuppressResult.NOT_FOUND: "NOT FOUND", SuppressResult.AMBIGUOUS: "AMBIGUOUS", } - for (id_type, id_value), result in results.items(): - status = status_map[result] - print(f" [{status}] {id_type}/{id_value}") + for (id_type, id_value), outcome in results.items(): + status = status_map[outcome.result] + title_suffix = f" -- {outcome.title}" if outcome.title else "" + print(f" [{status}] {id_type}/{id_value}{title_suffix}") diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 0a4970fb40..0df63e7556 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -7,7 +7,11 @@ import pytest -from palace.manager.scripts.suppress import SuppressResult, SuppressWorkForLibraryScript +from palace.manager.scripts.suppress import ( + SuppressOutcome, + SuppressResult, + SuppressWorkForLibraryScript, +) from palace.manager.sqlalchemy.model.datasource import DataSource from tests.fixtures.database import DatabaseTransactionFixture @@ -273,7 +277,9 @@ def test_do_run(self, db: DatabaseTransactionFixture, capsys): script = SuppressWorkForLibraryScript(db.session) suppress_work_mock = create_autospec(script.suppress_work) - suppress_work_mock.return_value = SuppressResult.NEWLY_SUPPRESSED + suppress_work_mock.return_value = SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Some Title" + ) script.suppress_work = suppress_work_mock args = [ "--library", @@ -295,7 +301,9 @@ def test_do_run_dry_run(self, db: DatabaseTransactionFixture, capsys): script = SuppressWorkForLibraryScript(db.session) suppress_work_mock = create_autospec(script.suppress_work) - suppress_work_mock.return_value = SuppressResult.NEWLY_SUPPRESSED + suppress_work_mock.return_value = SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Some Title" + ) script.suppress_work = suppress_work_mock args = [ "--library", @@ -355,7 +363,8 @@ def test_suppress_work(self, db: DatabaseTransactionFixture): test_library, work.presentation_edition.primary_identifier ) - assert result == SuppressResult.NEWLY_SUPPRESSED + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.title == work.title assert work.suppressed_for == [test_library] def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): @@ -368,7 +377,8 @@ def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): test_library, work.presentation_edition.primary_identifier ) - assert result == SuppressResult.ALREADY_SUPPRESSED + assert result.result == SuppressResult.ALREADY_SUPPRESSED + assert result.title == work.title assert work.suppressed_for == [test_library] def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixture): @@ -378,7 +388,8 @@ def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixtu script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, identifier) - assert result == SuppressResult.NOT_FOUND + assert result.result == SuppressResult.NOT_FOUND + assert result.title is None def test_suppress_work_resolves_via_equivalent_identifier( self, db: DatabaseTransactionFixture @@ -400,7 +411,8 @@ def test_suppress_work_resolves_via_equivalent_identifier( script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn) - assert result == SuppressResult.NEWLY_SUPPRESSED + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.title == work.title assert work.suppressed_for == [test_library] def test_suppress_work_equivalent_identifier_only_affects_specified_library( @@ -422,7 +434,7 @@ def test_suppress_work_equivalent_identifier_only_affects_specified_library( script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(library_a, isbn) - assert result == SuppressResult.NEWLY_SUPPRESSED + assert result.result == SuppressResult.NEWLY_SUPPRESSED assert work.suppressed_for == [library_a] assert library_b not in work.suppressed_for @@ -446,7 +458,10 @@ def test_suppress_work_ambiguous_equivalent_identifier( script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn) - assert result == SuppressResult.AMBIGUOUS + assert result.result == SuppressResult.AMBIGUOUS + assert result.title is not None + assert work1.title in result.title + assert work2.title in result.title assert work1.suppressed_for == [] assert work2.suppressed_for == [] @@ -461,7 +476,7 @@ def test_suppress_work_dry_run(self, db: DatabaseTransactionFixture): dry_run=True, ) - assert result == SuppressResult.NEWLY_SUPPRESSED + assert result.result == SuppressResult.NEWLY_SUPPRESSED assert work.suppressed_for == [] def test_suppress_work_dry_run_already_suppressed( @@ -478,15 +493,19 @@ def test_suppress_work_dry_run_already_suppressed( dry_run=True, ) - assert result == SuppressResult.ALREADY_SUPPRESSED + assert result.result == SuppressResult.ALREADY_SUPPRESSED def test_print_results_normal(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) results = { - ("ISBN", "111"): SuppressResult.NEWLY_SUPPRESSED, - ("ISBN", "222"): SuppressResult.ALREADY_SUPPRESSED, - ("ISBN", "333"): SuppressResult.NOT_FOUND, + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Book One" + ), + ("ISBN", "222"): SuppressOutcome( + SuppressResult.ALREADY_SUPPRESSED, "Book Two" + ), + ("ISBN", "333"): SuppressOutcome(SuppressResult.NOT_FOUND), } started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) script._print_results( @@ -505,8 +524,8 @@ def test_print_results_normal(self, db: DatabaseTransactionFixture, capsys): assert re.search(r"Newly suppressed:\s+1", out) assert re.search(r"Already suppressed:\s+1", out) assert re.search(r"Not found:\s+1", out) - assert "[SUPPRESSED] ISBN/111" in out - assert "[ALREADY SUPPRESSED] ISBN/222" in out + assert "[SUPPRESSED] ISBN/111 -- Book One" in out + assert "[ALREADY SUPPRESSED] ISBN/222 -- Book Two" in out assert "[NOT FOUND] ISBN/333" in out assert "[DRY RUN]" not in out @@ -514,7 +533,9 @@ def test_print_results_ambiguous(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) results = { - ("ISBN", "111"): SuppressResult.AMBIGUOUS, + ("ISBN", "111"): SuppressOutcome( + SuppressResult.AMBIGUOUS, "Book One; Book Two" + ), } started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) script._print_results( @@ -527,14 +548,16 @@ def test_print_results_ambiguous(self, db: DatabaseTransactionFixture, capsys): out = capsys.readouterr().out assert re.search(r"Ambiguous:\s+1", out) - assert "[AMBIGUOUS] ISBN/111" in out + assert "[AMBIGUOUS] ISBN/111 -- Book One; Book Two" in out def test_print_results_dry_run(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) results = { - ("ISBN", "111"): SuppressResult.NEWLY_SUPPRESSED, - ("ISBN", "222"): SuppressResult.NOT_FOUND, + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Book One" + ), + ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), } started_at = datetime(2026, 2, 26, 9, 30, 0, tzinfo=timezone.utc) script._print_results( @@ -552,7 +575,7 @@ def test_print_results_dry_run(self, db: DatabaseTransactionFixture, capsys): assert "0.05s" in out assert re.search(r"Would suppress:\s+1", out) assert re.search(r"Not found:\s+1", out) - assert "[WOULD SUPPRESS] ISBN/111" in out + assert "[WOULD SUPPRESS] ISBN/111 -- Book One" in out assert "[NOT FOUND] ISBN/222" in out def test_do_run_not_found_identifier(self, db: DatabaseTransactionFixture, capsys): @@ -690,5 +713,5 @@ def test_suppress_work_does_not_commit(self, db: DatabaseTransactionFixture): result = script.suppress_work( test_library, work.presentation_edition.primary_identifier ) - assert result == SuppressResult.NEWLY_SUPPRESSED + assert result.result == SuppressResult.NEWLY_SUPPRESSED mock_commit.assert_not_called() From 39bbbe6082329ba6686fcfe5152f0458da61017d Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 22 Sep 2026 14:45:12 -0700 Subject: [PATCH 03/10] Prefer a direct LicensePool match over equivalency ambiguity An identifier that owns a LicensePool directly is an exact match and needs no equivalency lookup. Previously load_works always consulted Work.from_identifiers, so an identifier with its own LicensePool that was also equivalent (e.g. via a Bibliotheca-style ISBN equivalency) to a different, unmerged Work would report AMBIGUOUS and suppress nothing -- a regression versus the pre-fix behavior, which correctly suppressed the exact match. Equivalency is now only consulted as a fallback when there's no direct match. Also add a docstring to suppress_work per project convention, and include each candidate work's id (and a "[no title]" placeholder) in the AMBIGUOUS output, since two distinct works can otherwise render as a single indistinguishable title. Co-Authored-By: Claude Sonnet 5 --- src/palace/manager/scripts/suppress.py | 41 ++++++++++++++++++-------- tests/manager/scripts/test_suppress.py | 28 ++++++++++++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 0ecff4f42d..581a607f76 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -26,8 +26,8 @@ class SuppressResult(Enum): class SuppressOutcome(NamedTuple): result: SuppressResult # The title of the resolved work, when there is exactly one. For - # AMBIGUOUS, this instead lists the titles of every candidate work, - # to help a human resolve the ambiguity. + # AMBIGUOUS, this instead lists the title and work id of every + # candidate work, to help a human resolve the ambiguity. title: str | None = None @@ -155,15 +155,21 @@ def load_identifiers_from_file( def load_works(self, identifier: Identifier) -> list[Work]: """Find the Work(s) reachable from an identifier. - This looks past LicensePools whose own identifier matches, to - also include LicensePools reachable through identifier - equivalency -- e.g. an ISBN a librarian has on hand is often - linked to a vendor's LicensePool via metadata equivalency - rather than being that LicensePool's own identifier. This uses - the same strict, high-confidence equivalency policy that - `Work.from_identifiers` applies by default everywhere else in - the codebase, so it won't walk into loosely-related works. + An identifier that owns a LicensePool directly is an exact match + and is returned on its own, with no need to consult equivalencies. + Only when there's no direct match do we look past LicensePools + whose own identifier matches, to also include LicensePools + reachable through identifier equivalency -- e.g. an ISBN a + librarian has on hand is often linked to a vendor's LicensePool + via metadata equivalency rather than being that LicensePool's own + identifier. This uses the same strict, high-confidence equivalency + policy that `Work.from_identifiers` applies by default everywhere + else in the codebase, so it won't walk into loosely-related works. """ + direct_work = identifier.work + if direct_work is not None: + return [direct_work] + query = Work.from_identifiers(self._db, [identifier]) if query is None: return [] @@ -175,19 +181,30 @@ def suppress_work( identifier: Identifier, dry_run: bool = False, ) -> SuppressOutcome: + """Suppress the work resolved from an identifier for a library. + + :param library: The library for which the resolved work should be suppressed. + :param identifier: The identifier used to resolve the work, either + directly (it owns a LicensePool) or through identifier equivalency. + :param dry_run: If true, report the outcome without changing suppression. + :return: The result of the suppression attempt, and the resolved + work's title(s) when available. + """ works = self.load_works(identifier) if not works: self.log.warning(f"No work found for {identifier}") return SuppressOutcome(SuppressResult.NOT_FOUND) if len(works) > 1: - titles = "; ".join(w.title for w in works if w.title) + titles = "; ".join( + f"{w.title or '[no title]'} (work id: {w.id})" for w in works + ) self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " f"{len(works)} different works via identifier equivalency; " "skipping rather than guessing which one to suppress." ) - return SuppressOutcome(SuppressResult.AMBIGUOUS, titles or None) + return SuppressOutcome(SuppressResult.AMBIGUOUS, titles) work = works[0] diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 0df63e7556..c7ef7b0b36 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -462,9 +462,37 @@ def test_suppress_work_ambiguous_equivalent_identifier( assert result.title is not None assert work1.title in result.title assert work2.title in result.title + assert f"work id: {work1.id}" in result.title + assert f"work id: {work2.id}" in result.title assert work1.suppressed_for == [] assert work2.suppressed_for == [] + def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( + self, db: DatabaseTransactionFixture + ): + """An identifier that owns a LicensePool directly is an exact + match and must win even if it's also equivalent (via messy + metadata) to a different, unrelated Work -- equivalency should + only be consulted as a fallback when there's no direct match, + never used to second-guess one.""" + test_library = db.library(short_name="test") + work = db.work(with_license_pool=True) + identifier = work.presentation_edition.primary_identifier + + other_work = db.work(with_license_pool=True) + other_identifier = other_work.presentation_edition.primary_identifier + + source = DataSource.lookup(db.session, DataSource.OCLC) + identifier.equivalent_to(source, other_identifier, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, identifier) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.title == work.title + assert work.suppressed_for == [test_library] + assert other_work.suppressed_for == [] + def test_suppress_work_dry_run(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") work = db.work(with_license_pool=True) From cf61f874efd9e0e6264acd5280df589d02d51aee Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 22 Sep 2026 15:05:00 -0700 Subject: [PATCH 04/10] Add --suppress-ambiguous to suppress across all ambiguous candidates An AMBIGUOUS result is often not bad data: non-open-access LicensePools never merge into a shared Work (LicensePool.calculate_work()'s own docstring says so), so a library running more than one collection for the same title (e.g. OverDrive + Bibliotheca) legitimately ends up with one ISBN equivalent, at full strength, to two permanently separate Works. An operator who already knows that's the situation has no way to suppress both without looking up each work's own identifier and running the script twice. --suppress-ambiguous opts into suppressing the identifier in every candidate work instead of refusing. The default stays conservative (refuse and report) since the same ambiguity shape can also mean mismatched/incorrect equivalency data, where auto-suppressing every candidate would silently pull an unrelated title. Co-Authored-By: Claude Sonnet 5 --- src/palace/manager/scripts/suppress.py | 89 +++++++++++---- tests/manager/scripts/test_suppress.py | 149 ++++++++++++++++++++++++- 2 files changed, 216 insertions(+), 22 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 581a607f76..59e3a1d9ea 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -76,6 +76,14 @@ def arg_parser(cls, _db: Session) -> argparse.ArgumentParser: help="Report what would be suppressed without making any changes.", action="store_true", ) + parser.add_argument( + "--suppress-ambiguous", + help="If an identifier resolves to more than one distinct work via " + "identifier equivalency (e.g. the same ISBN licensed through more " + "than one collection), suppress it in all of them instead of " + "skipping it.", + action="store_true", + ) return parser @classmethod @@ -175,18 +183,29 @@ def load_works(self, identifier: Identifier) -> list[Work]: return [] return cast(list[Work], query.distinct().all()) + @staticmethod + def _describe_works(works: list[Work]) -> str: + """Format a list of works for operator-facing output, e.g. for an + ambiguous match or when suppressing more than one work at once.""" + return "; ".join(f"{w.title or '[no title]'} (work id: {w.id})" for w in works) + def suppress_work( self, library: Library, identifier: Identifier, dry_run: bool = False, + suppress_ambiguous: bool = False, ) -> SuppressOutcome: - """Suppress the work resolved from an identifier for a library. + """Suppress the work(s) resolved from an identifier for a library. :param library: The library for which the resolved work should be suppressed. :param identifier: The identifier used to resolve the work, either directly (it owns a LicensePool) or through identifier equivalency. :param dry_run: If true, report the outcome without changing suppression. + :param suppress_ambiguous: If the identifier resolves to more than one + distinct work via equivalency, suppress it for the library in + every candidate work instead of refusing to guess which one is + meant. :return: The result of the suppression attempt, and the resolved work's title(s) when available. """ @@ -195,41 +214,66 @@ def suppress_work( self.log.warning(f"No work found for {identifier}") return SuppressOutcome(SuppressResult.NOT_FOUND) - if len(works) > 1: - titles = "; ".join( - f"{w.title or '[no title]'} (work id: {w.id})" for w in works + if len(works) == 1: + work = works[0] + + if library in work.suppressed_for: + return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, work.title) + + if not dry_run: + # Suppression is scoped to exactly this one library. Resolving + # the work via identifier equivalency only changes *which work* + # we find -- it never changes *which libraries* it's suppressed + # for, since only the `library` argument passed in is ever + # appended to `suppressed_for`. + work.suppressed_for.append(library) + + self.log.info( + f"{'[DRY RUN] Would suppress' if dry_run else 'Suppressing'} " + f"{identifier.type}/{identifier.identifier} (work id: {work.id}) " + f"for {library.short_name}." ) + return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, work.title) + + # More than one distinct Work is reachable from this identifier via + # equivalency (see load_works) -- e.g. the same ISBN licensed + # through more than one collection, each with its own permanent + # Work. Without --suppress-ambiguous, refuse to guess which one(s) + # the operator meant. + if not suppress_ambiguous: + titles = self._describe_works(works) self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " f"{len(works)} different works via identifier equivalency; " - "skipping rather than guessing which one to suppress." + "skipping rather than guessing which one to suppress. Pass " + "--suppress-ambiguous to suppress all of them for this library." ) return SuppressOutcome(SuppressResult.AMBIGUOUS, titles) - work = works[0] - - if library in work.suppressed_for: - return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, work.title) + newly_suppressed = 0 + for work in works: + if library in work.suppressed_for: + continue + if not dry_run: + work.suppressed_for.append(library) + newly_suppressed += 1 - if not dry_run: - # Suppression is scoped to exactly this one library. Resolving - # the work via identifier equivalency only changes *which work* - # we find -- it never changes *which libraries* it's suppressed - # for, since only the `library` argument passed in is ever - # appended to `suppressed_for`. - work.suppressed_for.append(library) + titles = self._describe_works(works) + if newly_suppressed == 0: + return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, titles) self.log.info( f"{'[DRY RUN] Would suppress' if dry_run else 'Suppressing'} " - f"{identifier.type}/{identifier.identifier} (work id: {work.id}) " - f"for {library.short_name}." + f"{identifier.type}/{identifier.identifier} in {newly_suppressed} " + f"of {len(works)} ambiguous work(s) for {library.short_name}." ) - return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, work.title) + return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, titles) def do_run(self, cmd_args: list[str] | None = None) -> None: parsed = self.parse_command_line(self._db, cmd_args=cmd_args) library = self.load_library(parsed.library) dry_run: bool = parsed.dry_run + suppress_ambiguous: bool = parsed.suppress_ambiguous started_at = datetime.now(tz=timezone.utc) if parsed.file: @@ -254,7 +298,12 @@ def do_run(self, cmd_args: list[str] | None = None) -> None: for id_type, id_value in pairs: try: identifier = self.load_identifier(id_type, id_value) - outcome = self.suppress_work(library, identifier, dry_run=dry_run) + outcome = self.suppress_work( + library, + identifier, + dry_run=dry_run, + suppress_ambiguous=suppress_ambiguous, + ) except PalaceValueError: outcome = SuppressOutcome(SuppressResult.NOT_FOUND) results[(id_type, id_value)] = outcome diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index c7ef7b0b36..939193317b 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -76,6 +76,24 @@ def test_parse_command_line_dry_run(self, db: DatabaseTransactionFixture): ) assert parsed.dry_run is True + def test_parse_command_line_suppress_ambiguous_default_false( + self, db: DatabaseTransactionFixture + ): + parsed = SuppressWorkForLibraryScript.parse_command_line( + db.session, + ["--library", "lib1", "--identifier", "123"], + ) + assert parsed.suppress_ambiguous is False + + def test_parse_command_line_suppress_ambiguous_flag( + self, db: DatabaseTransactionFixture + ): + parsed = SuppressWorkForLibraryScript.parse_command_line( + db.session, + ["--library", "lib1", "--identifier", "123", "--suppress-ambiguous"], + ) + assert parsed.suppress_ambiguous is True + def test_parse_command_line_file_and_identifier_mutually_exclusive( self, db: DatabaseTransactionFixture, capsys ): @@ -292,7 +310,7 @@ def test_do_run(self, db: DatabaseTransactionFixture, capsys): script.do_run(args) suppress_work_mock.assert_called_once_with( - test_library, test_identifier, dry_run=False + test_library, test_identifier, dry_run=False, suppress_ambiguous=False ) def test_do_run_dry_run(self, db: DatabaseTransactionFixture, capsys): @@ -317,7 +335,34 @@ def test_do_run_dry_run(self, db: DatabaseTransactionFixture, capsys): script.do_run(args) suppress_work_mock.assert_called_once_with( - test_library, test_identifier, dry_run=True + test_library, test_identifier, dry_run=True, suppress_ambiguous=False + ) + + def test_do_run_suppress_ambiguous_flag( + self, db: DatabaseTransactionFixture, capsys + ): + test_library = db.library(short_name="test") + test_identifier = db.identifier() + + script = SuppressWorkForLibraryScript(db.session) + suppress_work_mock = create_autospec(script.suppress_work) + suppress_work_mock.return_value = SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Some Title" + ) + script.suppress_work = suppress_work_mock + args = [ + "--library", + test_library.short_name, + "--identifier-type", + test_identifier.type, + "--identifier", + test_identifier.identifier, + "--suppress-ambiguous", + ] + script.do_run(args) + + suppress_work_mock.assert_called_once_with( + test_library, test_identifier, dry_run=False, suppress_ambiguous=True ) def test_do_run_with_file(self, db: DatabaseTransactionFixture, tmp_path, capsys): @@ -467,6 +512,106 @@ def test_suppress_work_ambiguous_equivalent_identifier( assert work1.suppressed_for == [] assert work2.suppressed_for == [] + def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( + self, db: DatabaseTransactionFixture + ): + """With --suppress-ambiguous, an identifier resolving to multiple + distinct works (e.g. the same ISBN licensed through more than one + collection) should suppress the work for the library in every + candidate, rather than refusing.""" + test_library = db.library(short_name="test") + work1 = db.work(with_license_pool=True) + work2 = db.work(with_license_pool=True) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.title is not None + assert work1.title in result.title + assert work2.title in result.title + assert work1.suppressed_for == [test_library] + assert work2.suppressed_for == [test_library] + + def test_suppress_work_suppress_ambiguous_already_suppressed_for_all( + self, db: DatabaseTransactionFixture + ): + test_library = db.library(short_name="test") + work1 = db.work(with_license_pool=True) + work2 = db.work(with_license_pool=True) + work1.suppressed_for.append(test_library) + work2.suppressed_for.append(test_library) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) + + assert result.result == SuppressResult.ALREADY_SUPPRESSED + # Not duplicated by a redundant append. + assert work1.suppressed_for == [test_library] + assert work2.suppressed_for == [test_library] + + def test_suppress_work_suppress_ambiguous_partial_already_suppressed( + self, db: DatabaseTransactionFixture + ): + """If some but not all candidates are already suppressed, the + overall result is NEWLY_SUPPRESSED since the run had an effect, + and every candidate ends up suppressed.""" + test_library = db.library(short_name="test") + work1 = db.work(with_license_pool=True) + work2 = db.work(with_license_pool=True) + work1.suppressed_for.append(test_library) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert work1.suppressed_for == [test_library] + assert work2.suppressed_for == [test_library] + + def test_suppress_work_suppress_ambiguous_dry_run_does_not_mutate( + self, db: DatabaseTransactionFixture + ): + test_library = db.library(short_name="test") + work1 = db.work(with_license_pool=True) + work2 = db.work(with_license_pool=True) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work( + test_library, isbn, dry_run=True, suppress_ambiguous=True + ) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert work1.suppressed_for == [] + assert work2.suppressed_for == [] + def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( self, db: DatabaseTransactionFixture ): From caa336eb0277428419c57c10379853cff7c9b294 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Tue, 22 Sep 2026 15:57:47 -0700 Subject: [PATCH 05/10] Scope equivalency resolution to the library, fix ambiguous reporting Four fixes from review: - load_works's equivalency fallback is now scoped to the target library's own active collections. Without this, a title licensed to two different libraries through two different collections (e.g. library A via OverDrive, library B via Bibliotheca) reported AMBIGUOUS when suppressing for either library alone, even though only one of the two candidate works was ever licensed to that library. The direct-match path is untouched, so identifiers that already resolved before this PR behave exactly as before. - A fully-covered set of candidates (every work already suppressed for the library) now short-circuits to ALREADY_SUPPRESSED before the ambiguity check runs, whether or not --suppress-ambiguous is passed. Previously, re-running an already-fully-suppressed identifier without the flag reported AMBIGUOUS, even though there was nothing left to decide. - --suppress-ambiguous's reported title now describes only the works that actually changed in this run, not every candidate. Previously a partially-already-suppressed run could read as though every candidate had just been suppressed. - The single-work success/already-suppressed path now includes the work id alongside the title (matching the ambiguous-path format), so an operator has what they need to look the work up in the admin UI regardless of which path resolved it. Co-Authored-By: Claude Sonnet 5 --- src/palace/manager/scripts/suppress.py | 76 +++++++++++----- tests/manager/scripts/test_suppress.py | 117 ++++++++++++++++++++----- 2 files changed, 149 insertions(+), 44 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 59e3a1d9ea..9499f60f35 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -13,6 +13,7 @@ from palace.manager.scripts.base import Script, _normalize_cmd_args from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.sqlalchemy.model.library import Library +from palace.manager.sqlalchemy.model.licensing import LicensePool from palace.manager.sqlalchemy.model.work import Work @@ -25,9 +26,10 @@ class SuppressResult(Enum): class SuppressOutcome(NamedTuple): result: SuppressResult - # The title of the resolved work, when there is exactly one. For - # AMBIGUOUS, this instead lists the title and work id of every - # candidate work, to help a human resolve the ambiguity. + # The title and work id of every work described by this outcome, + # formatted as " (work id: <id>)" and joined with "; " when + # there's more than one -- e.g. for AMBIGUOUS, or when + # --suppress-ambiguous affects more than one work at once. title: str | None = None @@ -160,11 +162,15 @@ def load_identifiers_from_file( raise PalaceValueError(f"CSV file not found: {file_path}") return identifiers - def load_works(self, identifier: Identifier) -> list[Work]: - """Find the Work(s) reachable from an identifier. + def load_works(self, identifier: Identifier, library: Library) -> list[Work]: + """Find the Work(s) reachable from an identifier for a library. An identifier that owns a LicensePool directly is an exact match - and is returned on its own, with no need to consult equivalencies. + and is returned on its own, with no need to consult equivalencies, + regardless of which library that LicensePool's collection belongs + to (this matches the identifier resolution the script has always + done, before this class started consulting equivalencies at all). + Only when there's no direct match do we look past LicensePools whose own identifier matches, to also include LicensePools reachable through identifier equivalency -- e.g. an ISBN a @@ -173,12 +179,29 @@ def load_works(self, identifier: Identifier) -> list[Work]: identifier. This uses the same strict, high-confidence equivalency policy that `Work.from_identifiers` applies by default everywhere else in the codebase, so it won't walk into loosely-related works. + + This equivalency fallback is scoped to `library`'s own collections. + Otherwise, a title licensed to two different libraries through two + different collections (e.g. one via OverDrive, one via Bibliotheca) + would look ambiguous when suppressing for either library alone, + even though only one of the two candidate works is actually + licensed to that library. """ direct_work = identifier.work if direct_work is not None: return [direct_work] - query = Work.from_identifiers(self._db, [identifier]) + collection_ids = [c.id for c in library.active_collections] + if not collection_ids: + return [] + + base_query = ( + self._db.query(Work) + .join(Work.license_pools) + .join(LicensePool.identifier) + .filter(LicensePool.collection_id.in_(collection_ids)) + ) + query = Work.from_identifiers(self._db, [identifier], base_query=base_query) if query is None: return [] return cast(list[Work], query.distinct().all()) @@ -209,16 +232,17 @@ def suppress_work( :return: The result of the suppression attempt, and the resolved work's title(s) when available. """ - works = self.load_works(identifier) + works = self.load_works(identifier, library) if not works: self.log.warning(f"No work found for {identifier}") return SuppressOutcome(SuppressResult.NOT_FOUND) if len(works) == 1: work = works[0] + description = self._describe_works(works) if library in work.suppressed_for: - return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, work.title) + return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, description) if not dry_run: # Suppression is scoped to exactly this one library. Resolving @@ -233,14 +257,24 @@ def suppress_work( f"{identifier.type}/{identifier.identifier} (work id: {work.id}) " f"for {library.short_name}." ) - return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, work.title) + return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, description) # More than one distinct Work is reachable from this identifier via # equivalency (see load_works) -- e.g. the same ISBN licensed - # through more than one collection, each with its own permanent - # Work. Without --suppress-ambiguous, refuse to guess which one(s) - # the operator meant. + # through more than one of this library's collections, each with + # its own permanent Work. + if all(library in work.suppressed_for for work in works): + # Every candidate already reflects the desired state, so there's + # nothing to decide or change -- this isn't really ambiguous in + # practice, just a no-op re-run (e.g. after an earlier + # --suppress-ambiguous run already covered every candidate). + return SuppressOutcome( + SuppressResult.ALREADY_SUPPRESSED, self._describe_works(works) + ) + if not suppress_ambiguous: + # Without --suppress-ambiguous, refuse to guess which one(s) the + # operator meant. titles = self._describe_works(works) self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " @@ -250,24 +284,24 @@ def suppress_work( ) return SuppressOutcome(SuppressResult.AMBIGUOUS, titles) - newly_suppressed = 0 + # At least one candidate isn't yet suppressed (the all-suppressed + # case was handled above), so this always changes at least one work. + changed_works: list[Work] = [] for work in works: if library in work.suppressed_for: continue if not dry_run: work.suppressed_for.append(library) - newly_suppressed += 1 - - titles = self._describe_works(works) - if newly_suppressed == 0: - return SuppressOutcome(SuppressResult.ALREADY_SUPPRESSED, titles) + changed_works.append(work) self.log.info( f"{'[DRY RUN] Would suppress' if dry_run else 'Suppressing'} " - f"{identifier.type}/{identifier.identifier} in {newly_suppressed} " + f"{identifier.type}/{identifier.identifier} in {len(changed_works)} " f"of {len(works)} ambiguous work(s) for {library.short_name}." ) - return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, titles) + return SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, self._describe_works(changed_works) + ) def do_run(self, cmd_args: list[str] | None = None) -> None: parsed = self.parse_command_line(self._db, cmd_args=cmd_args) diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 939193317b..932d7c05b8 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -409,7 +409,7 @@ def test_suppress_work(self, db: DatabaseTransactionFixture): ) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == work.title + assert result.title == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): @@ -423,7 +423,7 @@ def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): ) assert result.result == SuppressResult.ALREADY_SUPPRESSED - assert result.title == work.title + assert result.title == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixture): @@ -446,7 +446,8 @@ def test_suppress_work_resolves_via_equivalent_identifier( equivalency instead of requiring the ISBN to be the LicensePool's own identifier.""" test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) pool_identifier = work.presentation_edition.primary_identifier isbn = db.identifier(identifier_type="ISBN") @@ -457,7 +458,7 @@ def test_suppress_work_resolves_via_equivalent_identifier( result = script.suppress_work(test_library, isbn) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == work.title + assert result.title == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_equivalent_identifier_only_affects_specified_library( @@ -469,7 +470,8 @@ def test_suppress_work_equivalent_identifier_only_affects_specified_library( `work.suppressed_for`.""" library_a = db.library(short_name="lib_a") library_b = db.library(short_name="lib_b") - work = db.work(with_license_pool=True) + collection = db.collection(library=library_a) + work = db.work(with_license_pool=True, collection=collection) pool_identifier = work.presentation_edition.primary_identifier isbn = db.identifier(identifier_type="ISBN") @@ -490,8 +492,9 @@ def test_suppress_work_ambiguous_equivalent_identifier( equivalency, the script must not guess -- it should report AMBIGUOUS and suppress nothing.""" test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -505,13 +508,43 @@ def test_suppress_work_ambiguous_equivalent_identifier( assert result.result == SuppressResult.AMBIGUOUS assert result.title is not None - assert work1.title in result.title - assert work2.title in result.title - assert f"work id: {work1.id}" in result.title - assert f"work id: {work2.id}" in result.title + parts = result.title.split("; ") + assert f"{work1.title} (work id: {work1.id})" in parts + assert f"{work2.title} (work id: {work2.id})" in parts assert work1.suppressed_for == [] assert work2.suppressed_for == [] + def test_suppress_work_not_ambiguous_when_only_one_candidate_is_licensed_to_library( + self, db: DatabaseTransactionFixture + ): + """Two different libraries each carrying "the same" title through + their own collection (e.g. library A via OverDrive, library B via + Bibliotheca) legitimately produces two distinct works reachable + from one shared ISBN. Suppressing for library A alone must resolve + cleanly to A's own work rather than reporting AMBIGUOUS over a + candidate that isn't even licensed to A.""" + library_a = db.library(short_name="lib_a") + library_b = db.library(short_name="lib_b") + collection_a = db.collection(library=library_a) + collection_b = db.collection(library=library_b) + work_a = db.work(with_license_pool=True, collection=collection_a) + work_b = db.work(with_license_pool=True, collection=collection_b) + id_a = work_a.presentation_edition.primary_identifier + id_b = work_b.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id_a, 1) + isbn.equivalent_to(source, id_b, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(library_a, isbn) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.title == f"{work_a.title} (work id: {work_a.id})" + assert work_a.suppressed_for == [library_a] + assert work_b.suppressed_for == [] + def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( self, db: DatabaseTransactionFixture ): @@ -520,8 +553,9 @@ def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( collection) should suppress the work for the library in every candidate, rather than refusing.""" test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -535,8 +569,9 @@ def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( assert result.result == SuppressResult.NEWLY_SUPPRESSED assert result.title is not None - assert work1.title in result.title - assert work2.title in result.title + parts = result.title.split("; ") + assert f"{work1.title} (work id: {work1.id})" in parts + assert f"{work2.title} (work id: {work2.id})" in parts assert work1.suppressed_for == [test_library] assert work2.suppressed_for == [test_library] @@ -544,8 +579,9 @@ def test_suppress_work_suppress_ambiguous_already_suppressed_for_all( self, db: DatabaseTransactionFixture ): test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) work1.suppressed_for.append(test_library) work2.suppressed_for.append(test_library) id1 = work1.presentation_edition.primary_identifier @@ -564,15 +600,45 @@ def test_suppress_work_suppress_ambiguous_already_suppressed_for_all( assert work1.suppressed_for == [test_library] assert work2.suppressed_for == [test_library] + def test_suppress_work_all_already_suppressed_reports_already_suppressed_without_flag( + self, db: DatabaseTransactionFixture + ): + """Re-running against a fully-covered set of candidates (e.g. + after an earlier --suppress-ambiguous run) must be idempotent: it + should report ALREADY_SUPPRESSED, not AMBIGUOUS, since there's + nothing left to decide or change even without the flag.""" + test_library = db.library(short_name="test") + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) + work1.suppressed_for.append(test_library) + work2.suppressed_for.append(test_library) + id1 = work1.presentation_edition.primary_identifier + id2 = work2.presentation_edition.primary_identifier + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, id1, 1) + isbn.equivalent_to(source, id2, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result.result == SuppressResult.ALREADY_SUPPRESSED + assert work1.suppressed_for == [test_library] + assert work2.suppressed_for == [test_library] + def test_suppress_work_suppress_ambiguous_partial_already_suppressed( self, db: DatabaseTransactionFixture ): """If some but not all candidates are already suppressed, the overall result is NEWLY_SUPPRESSED since the run had an effect, - and every candidate ends up suppressed.""" + every candidate ends up suppressed, and the reported title + describes only the candidate that actually changed.""" test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) work1.suppressed_for.append(test_library) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -588,13 +654,18 @@ def test_suppress_work_suppress_ambiguous_partial_already_suppressed( assert result.result == SuppressResult.NEWLY_SUPPRESSED assert work1.suppressed_for == [test_library] assert work2.suppressed_for == [test_library] + # Only the newly-changed work2 is described -- work1 was already + # suppressed before this run, so it isn't reported as an action + # that just happened. + assert result.title == f"{work2.title} (work id: {work2.id})" def test_suppress_work_suppress_ambiguous_dry_run_does_not_mutate( self, db: DatabaseTransactionFixture ): test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -634,7 +705,7 @@ def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( result = script.suppress_work(test_library, identifier) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == work.title + assert result.title == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] assert other_work.suppressed_for == [] From 8e2e97dc06178ab64ced992eeae6fb17ba686387 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein <dabylon@gmail.com> Date: Tue, 22 Sep 2026 16:21:00 -0700 Subject: [PATCH 06/10] Make library scoping coherent across both resolution paths Follow-up to the library scoping added in f82085d, from review: - Scope collections with an EXISTS over the work's pools rather than a filter on the joined pool row. A Work can own pools in several collections (open-access pools merge across collections by permanent work id), in which case the pool carrying the equivalent identifier and the pool in this library's collection are different pools of the same Work. Filtering the joined row required one pool to satisfy both conditions, so those works resolved to NOT_FOUND -- the very failure this branch set out to fix. - Apply the same scope to the direct match: a direct match whose pool isn't in any of the library's collections now falls through to the equivalency lookup instead of being suppressed on that library's behalf. This matters for collections keyed on ISBN (ODL/OPDS), where an ISBN owned by another library's collection would otherwise be suppressed while this library's own equivalent work went unexamined. - Scope by associated_collections rather than active_collections. suppressed_for is a durable flag; it shouldn't depend on where today falls in a collection's subscription window. - Rename SuppressOutcome.title to description, since it holds "<title> (work id: <id>)" rather than a bare title. - Drop the unreachable from_identifiers None branch (it returns None only for an empty identifier list) and cover the reachable no-collections early return with a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- src/palace/manager/scripts/suppress.py | 71 +++++++------ tests/manager/scripts/test_suppress.py | 140 ++++++++++++++++++++----- 2 files changed, 151 insertions(+), 60 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 9499f60f35..efbd8c477b 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -30,7 +30,7 @@ class SuppressOutcome(NamedTuple): # formatted as "<title> (work id: <id>)" and joined with "; " when # there's more than one -- e.g. for AMBIGUOUS, or when # --suppress-ambiguous affects more than one work at once. - title: str | None = None + description: str | None = None class SuppressWorkForLibraryScript(Script): @@ -163,15 +163,16 @@ def load_identifiers_from_file( return identifiers def load_works(self, identifier: Identifier, library: Library) -> list[Work]: - """Find the Work(s) reachable from an identifier for a library. + """Find the Work(s) `library` carries for an identifier. - An identifier that owns a LicensePool directly is an exact match - and is returned on its own, with no need to consult equivalencies, - regardless of which library that LicensePool's collection belongs - to (this matches the identifier resolution the script has always - done, before this class started consulting equivalencies at all). + Only works the library actually licenses are considered, so that a + title carried by two libraries through two different collections + (e.g. one via OverDrive, one via Bibliotheca) resolves cleanly for + each of them instead of looking ambiguous to both. - Only when there's no direct match do we look past LicensePools + An identifier that owns a LicensePool in one of the library's + collections is an exact match and is returned on its own, with no + need to consult equivalencies. Otherwise we look past LicensePools whose own identifier matches, to also include LicensePools reachable through identifier equivalency -- e.g. an ISBN a librarian has on hand is often linked to a vendor's LicensePool @@ -180,30 +181,30 @@ def load_works(self, identifier: Identifier, library: Library) -> list[Work]: policy that `Work.from_identifiers` applies by default everywhere else in the codebase, so it won't walk into loosely-related works. - This equivalency fallback is scoped to `library`'s own collections. - Otherwise, a title licensed to two different libraries through two - different collections (e.g. one via OverDrive, one via Bibliotheca) - would look ambiguous when suppressing for either library alone, - even though only one of the two candidate works is actually - licensed to that library. + Collections are scoped by association rather than by whether + they're currently active: `suppressed_for` is a durable flag, not + something that should depend on where today falls in a + collection's subscription window. """ - direct_work = identifier.work - if direct_work is not None: - return [direct_work] - - collection_ids = [c.id for c in library.active_collections] + collection_ids = [ + c.id for c in library.associated_collections if c.id is not None + ] if not collection_ids: return [] - base_query = ( - self._db.query(Work) - .join(Work.license_pools) - .join(LicensePool.identifier) - .filter(LicensePool.collection_id.in_(collection_ids)) + direct_work = identifier.work + if direct_work is not None and any( + pool.collection_id in collection_ids for pool in direct_work.license_pools + ): + return [direct_work] + + # The collection scope is an EXISTS rather than a filter on the + # joined pool, because the pool carrying the equivalent identifier + # and the pool in this library's collection may be different pools + # of the same Work. + query = Work.from_identifiers(self._db, [identifier]).filter( + Work.license_pools.any(LicensePool.collection_id.in_(collection_ids)) ) - query = Work.from_identifiers(self._db, [identifier], base_query=base_query) - if query is None: - return [] return cast(list[Work], query.distinct().all()) @staticmethod @@ -223,14 +224,15 @@ def suppress_work( :param library: The library for which the resolved work should be suppressed. :param identifier: The identifier used to resolve the work, either - directly (it owns a LicensePool) or through identifier equivalency. + directly (it owns a LicensePool in one of the library's + collections) or through identifier equivalency. :param dry_run: If true, report the outcome without changing suppression. :param suppress_ambiguous: If the identifier resolves to more than one distinct work via equivalency, suppress it for the library in every candidate work instead of refusing to guess which one is meant. - :return: The result of the suppression attempt, and the resolved - work's title(s) when available. + :return: The result of the suppression attempt, and a description of + the work(s) it applies to, when any were resolved. """ works = self.load_works(identifier, library) if not works: @@ -275,14 +277,15 @@ def suppress_work( if not suppress_ambiguous: # Without --suppress-ambiguous, refuse to guess which one(s) the # operator meant. - titles = self._describe_works(works) self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " f"{len(works)} different works via identifier equivalency; " "skipping rather than guessing which one to suppress. Pass " "--suppress-ambiguous to suppress all of them for this library." ) - return SuppressOutcome(SuppressResult.AMBIGUOUS, titles) + return SuppressOutcome( + SuppressResult.AMBIGUOUS, self._describe_works(works) + ) # At least one candidate isn't yet suppressed (the all-suppressed # case was handled above), so this always changes at least one work. @@ -404,5 +407,5 @@ def _print_results( } for (id_type, id_value), outcome in results.items(): status = status_map[outcome.result] - title_suffix = f" -- {outcome.title}" if outcome.title else "" - print(f" [{status}] {id_type}/{id_value}{title_suffix}") + suffix = f" -- {outcome.description}" if outcome.description else "" + print(f" [{status}] {id_type}/{id_value}{suffix}") diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 932d7c05b8..f126b52f24 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -258,8 +258,9 @@ def test_do_run_deduplicates_and_warns( import logging test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -367,8 +368,9 @@ def test_do_run_suppress_ambiguous_flag( def test_do_run_with_file(self, db: DatabaseTransactionFixture, tmp_path, capsys): test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -399,7 +401,8 @@ def test_do_run_with_file(self, db: DatabaseTransactionFixture, tmp_path, capsys def test_suppress_work(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) assert work.suppressed_for == [] @@ -409,12 +412,13 @@ def test_suppress_work(self, db: DatabaseTransactionFixture): ) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == f"{work.title} (work id: {work.id})" + assert result.description == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) work.suppressed_for.append(test_library) script = SuppressWorkForLibraryScript(db.session) @@ -423,18 +427,35 @@ def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): ) assert result.result == SuppressResult.ALREADY_SUPPRESSED - assert result.title == f"{work.title} (work id: {work.id})" + assert result.description == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") + db.collection(library=test_library) identifier = db.identifier() script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, identifier) assert result.result == SuppressResult.NOT_FOUND - assert result.title is None + assert result.description is None + + def test_suppress_work_library_with_no_collections( + self, db: DatabaseTransactionFixture + ): + """A library that carries no collections carries no works, so + there is nothing for it to suppress.""" + test_library = db.library(short_name="test") + work = db.work(with_license_pool=True) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work( + test_library, work.presentation_edition.primary_identifier + ) + + assert result.result == SuppressResult.NOT_FOUND + assert work.suppressed_for == [] def test_suppress_work_resolves_via_equivalent_identifier( self, db: DatabaseTransactionFixture @@ -458,7 +479,7 @@ def test_suppress_work_resolves_via_equivalent_identifier( result = script.suppress_work(test_library, isbn) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == f"{work.title} (work id: {work.id})" + assert result.description == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] def test_suppress_work_equivalent_identifier_only_affects_specified_library( @@ -507,8 +528,8 @@ def test_suppress_work_ambiguous_equivalent_identifier( result = script.suppress_work(test_library, isbn) assert result.result == SuppressResult.AMBIGUOUS - assert result.title is not None - parts = result.title.split("; ") + assert result.description is not None + parts = result.description.split("; ") assert f"{work1.title} (work id: {work1.id})" in parts assert f"{work2.title} (work id: {work2.id})" in parts assert work1.suppressed_for == [] @@ -541,10 +562,71 @@ def test_suppress_work_not_ambiguous_when_only_one_candidate_is_licensed_to_libr result = script.suppress_work(library_a, isbn) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == f"{work_a.title} (work id: {work_a.id})" + assert result.description == f"{work_a.title} (work id: {work_a.id})" assert work_a.suppressed_for == [library_a] assert work_b.suppressed_for == [] + def test_suppress_work_work_with_pools_in_several_collections( + self, db: DatabaseTransactionFixture + ): + """A Work can own pools in more than one collection (open-access + pools are merged across collections by permanent work id). The + library's own pool and the pool carrying the equivalent identifier + may therefore be different pools of the same Work, so the + collection scope has to be independent of the equivalency match + rather than riding on the same joined row.""" + test_library = db.library(short_name="test") + library_collection = db.collection(library=test_library) + other_collection = db.collection() + + work = db.work(with_license_pool=True, collection=library_collection) + + # A second pool of the same work, in a collection this library + # doesn't carry. The ISBN is equivalent to *this* pool's identifier. + other_edition = db.edition() + db.licensepool(other_edition, collection=other_collection, work=work) + + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, other_edition.primary_identifier, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert work.suppressed_for == [test_library] + + def test_suppress_work_direct_match_outside_library_falls_through( + self, db: DatabaseTransactionFixture + ): + """An ISBN can be some *other* library's collection's own pool + identifier (ISBN-keyed ODL/OPDS collections are the common case). + That direct match isn't this library's work, so it must not be + suppressed on this library's behalf -- and the equivalency + fallback should still find the work this library does carry.""" + test_library = db.library(short_name="test") + library_collection = db.collection(library=test_library) + other_collection = db.collection() + + work = db.work(with_license_pool=True, collection=library_collection) + + # An ISBN that is another collection's own pool identifier. + isbn_edition = db.edition(identifier_type="ISBN") + other_work = db.work(with_license_pool=True, collection=other_collection) + db.licensepool(isbn_edition, collection=other_collection, work=other_work) + isbn = isbn_edition.primary_identifier + + source = DataSource.lookup(db.session, DataSource.OCLC) + isbn.equivalent_to(source, work.presentation_edition.primary_identifier, 1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.description == f"{work.title} (work id: {work.id})" + assert work.suppressed_for == [test_library] + assert other_work.suppressed_for == [] + def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( self, db: DatabaseTransactionFixture ): @@ -568,8 +650,8 @@ def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title is not None - parts = result.title.split("; ") + assert result.description is not None + parts = result.description.split("; ") assert f"{work1.title} (work id: {work1.id})" in parts assert f"{work2.title} (work id: {work2.id})" in parts assert work1.suppressed_for == [test_library] @@ -657,7 +739,7 @@ def test_suppress_work_suppress_ambiguous_partial_already_suppressed( # Only the newly-changed work2 is described -- work1 was already # suppressed before this run, so it isn't reported as an action # that just happened. - assert result.title == f"{work2.title} (work id: {work2.id})" + assert result.description == f"{work2.title} (work id: {work2.id})" def test_suppress_work_suppress_ambiguous_dry_run_does_not_mutate( self, db: DatabaseTransactionFixture @@ -692,10 +774,11 @@ def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( only be consulted as a fallback when there's no direct match, never used to second-guess one.""" test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) identifier = work.presentation_edition.primary_identifier - other_work = db.work(with_license_pool=True) + other_work = db.work(with_license_pool=True, collection=collection) other_identifier = other_work.presentation_edition.primary_identifier source = DataSource.lookup(db.session, DataSource.OCLC) @@ -705,13 +788,14 @@ def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( result = script.suppress_work(test_library, identifier) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.title == f"{work.title} (work id: {work.id})" + assert result.description == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [test_library] assert other_work.suppressed_for == [] def test_suppress_work_dry_run(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work( @@ -727,7 +811,8 @@ def test_suppress_work_dry_run_already_suppressed( self, db: DatabaseTransactionFixture ): test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) work.suppressed_for.append(test_library) script = SuppressWorkForLibraryScript(db.session) @@ -846,8 +931,9 @@ def test_do_run_commits_once_for_all_suppressions( self, db: DatabaseTransactionFixture, tmp_path, capsys ): test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -872,8 +958,9 @@ def test_do_run_rolls_back_all_on_commit_failure( self, db: DatabaseTransactionFixture, tmp_path ): test_library = db.library(short_name="test") - work1 = db.work(with_license_pool=True) - work2 = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work1 = db.work(with_license_pool=True, collection=collection) + work2 = db.work(with_license_pool=True, collection=collection) id1 = work1.presentation_edition.primary_identifier id2 = work2.presentation_edition.primary_identifier @@ -950,7 +1037,8 @@ def test_load_identifiers_from_file_not_found(self, db: DatabaseTransactionFixtu def test_suppress_work_does_not_commit(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") - work = db.work(with_license_pool=True) + collection = db.collection(library=test_library) + work = db.work(with_license_pool=True, collection=collection) script = SuppressWorkForLibraryScript(db.session) with patch.object(db.session, "commit") as mock_commit: From d7e850b0f986200aee5120e3639d9317071c7579 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein <dabylon@gmail.com> Date: Wed, 23 Sep 2026 09:15:23 -0700 Subject: [PATCH 07/10] Subject the direct-match path to the ambiguity guard too The direct-match path resolved through Identifier.work, which returns the first pool's work and stops. But one vendor identifier can be licensed separately by two of a library's own collections -- a consortium's OverDrive collection and that library's OverDrive Advantage collection both carry the same OverDrive id, and OverdriveAdvantageAccount.to_collection() creates the Advantage collection as a distinct child Collection. Each non-open-access pool gets its own permanent Work, so the script suppressed whichever work it happened to find first, reported [SUPPRESSED], and left the title circulating through the other collection. The direct set is now built from the identifier's pools in the library's collections, so a multi-collection match reaches the same AMBIGUOUS guard (and the same --suppress-ambiguous escape hatch) as the equivalency path. Verified that dropping the old single-work lookup loses nothing: an identifier is always a member of its own equivalent set -- fn_recursive_equivalents seeds the CTE with it at strength 1, independent of the threshold -- so a work whose pool for this identifier sits outside the library but which has another pool inside is still found by the equivalency query. There's a test for that case, and the two new multi-collection tests both fail against the old logic. Also from review: - Distinguish "this library doesn't carry it" from "no such work" with a NOT_IN_LIBRARY result, so an operator scanning a batch can tell a title in someone else's collection from a typo'd identifier. - Cache the library's collection ids for the life of the script rather than issuing the same SELECT once per identifier. - Order candidates by work id so operator-facing output is stable between runs. Messages that described multi-work matches as coming "via identifier equivalency" are reworded, since that's no longer the only way to get there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- src/palace/manager/scripts/suppress.py | 141 +++++++++++++++------ tests/manager/scripts/test_suppress.py | 168 ++++++++++++++++++++++++- 2 files changed, 266 insertions(+), 43 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index efbd8c477b..456db43164 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -21,6 +21,7 @@ class SuppressResult(Enum): NEWLY_SUPPRESSED = auto() ALREADY_SUPPRESSED = auto() NOT_FOUND = auto() + NOT_IN_LIBRARY = auto() AMBIGUOUS = auto() @@ -38,6 +39,8 @@ class SuppressWorkForLibraryScript(Script): BY_DATABASE_ID = "Database ID" + _collection_ids: dict[int, list[int]] + @classmethod def arg_parser(cls, _db: Session) -> argparse.ArgumentParser: parser = argparse.ArgumentParser() @@ -80,9 +83,9 @@ def arg_parser(cls, _db: Session) -> argparse.ArgumentParser: ) parser.add_argument( "--suppress-ambiguous", - help="If an identifier resolves to more than one distinct work via " - "identifier equivalency (e.g. the same ISBN licensed through more " - "than one collection), suppress it in all of them instead of " + help="If an identifier resolves to more than one distinct work " + "(e.g. the same title licensed through more than one of the " + "library's collections), suppress it in all of them instead of " "skipping it.", action="store_true", ) @@ -162,6 +165,27 @@ def load_identifiers_from_file( raise PalaceValueError(f"CSV file not found: {file_path}") return identifiers + def _library_collection_ids(self, library: Library) -> list[int]: + """The ids of the collections a library is associated with. + + Cached for the life of the script: `Library.associated_collections` + issues a fresh SELECT on every access, and this is consulted once + per identifier, so a `--file` run would otherwise repeat the same + query for every row. + + Collections are scoped by association rather than by whether + they're currently active: `suppressed_for` is a durable flag, not + something that should depend on where today falls in a + collection's subscription window. + """ + if not hasattr(self, "_collection_ids"): + self._collection_ids = {} + if library.id not in self._collection_ids: + self._collection_ids[library.id] = [ + c.id for c in library.associated_collections if c.id is not None + ] + return self._collection_ids[library.id] + def load_works(self, identifier: Identifier, library: Library) -> list[Work]: """Find the Work(s) `library` carries for an identifier. @@ -170,40 +194,53 @@ def load_works(self, identifier: Identifier, library: Library) -> list[Work]: (e.g. one via OverDrive, one via Bibliotheca) resolves cleanly for each of them instead of looking ambiguous to both. - An identifier that owns a LicensePool in one of the library's - collections is an exact match and is returned on its own, with no - need to consult equivalencies. Otherwise we look past LicensePools - whose own identifier matches, to also include LicensePools - reachable through identifier equivalency -- e.g. an ISBN a - librarian has on hand is often linked to a vendor's LicensePool - via metadata equivalency rather than being that LicensePool's own - identifier. This uses the same strict, high-confidence equivalency - policy that `Work.from_identifiers` applies by default everywhere - else in the codebase, so it won't walk into loosely-related works. - - Collections are scoped by association rather than by whether - they're currently active: `suppressed_for` is a durable flag, not - something that should depend on where today falls in a - collection's subscription window. + Pools for this exact identifier in one of the library's collections + are exact matches, and win outright over equivalency. There can be + more than one of them: each collection that carries a title + licenses it separately, and every non-open-access pool gets its own + permanent Work -- so a library holding both a consortium's + OverDrive collection and its own OverDrive Advantage collection has + two works for one OverDrive id. All of them are returned, so that + case meets the same ambiguity guard as the equivalency case below + rather than silently suppressing whichever was found first. + + Failing an exact match, we look past LicensePools whose own + identifier matches, to also include LicensePools reachable through + identifier equivalency -- e.g. an ISBN a librarian has on hand is + often linked to a vendor's LicensePool via metadata equivalency + rather than being that LicensePool's own identifier. This uses the + same strict, high-confidence equivalency policy that + `Work.from_identifiers` applies by default everywhere else in the + codebase, so it won't walk into loosely-related works. + + Exact matches deliberately don't get combined with equivalency + matches: two pools of the same identifier are certainly the same + title, whereas an equivalency edge is only ever a vendor's + assertion, so widening an exact hit with equivalent works could + suppress an unrelated title. """ - collection_ids = [ - c.id for c in library.associated_collections if c.id is not None - ] + collection_ids = self._library_collection_ids(library) if not collection_ids: return [] - direct_work = identifier.work - if direct_work is not None and any( - pool.collection_id in collection_ids for pool in direct_work.license_pools - ): - return [direct_work] + direct_works = { + pool.work.id: pool.work + for pool in identifier.licensed_through + if pool.work is not None and pool.collection_id in collection_ids + } + if direct_works: + return [direct_works[work_id] for work_id in sorted(direct_works)] # The collection scope is an EXISTS rather than a filter on the # joined pool, because the pool carrying the equivalent identifier # and the pool in this library's collection may be different pools # of the same Work. - query = Work.from_identifiers(self._db, [identifier]).filter( - Work.license_pools.any(LicensePool.collection_id.in_(collection_ids)) + query = ( + Work.from_identifiers(self._db, [identifier]) + .filter( + Work.license_pools.any(LicensePool.collection_id.in_(collection_ids)) + ) + .order_by(Work.id) ) return cast(list[Work], query.distinct().all()) @@ -213,6 +250,32 @@ def _describe_works(works: list[Work]) -> str: ambiguous match or when suppressing more than one work at once.""" return "; ".join(f"{w.title or '[no title]'} (work id: {w.id})" for w in works) + def _nothing_to_suppress( + self, identifier: Identifier, library: Library + ) -> SuppressOutcome: + """Report why an identifier resolved to none of the library's works. + + A title the library doesn't carry is a different problem from an + identifier that matches nothing at all -- the first means the + identifier is fine but belongs to someone else's collection, the + second usually means a typo -- so they get distinct results rather + than both landing in one "not found" bucket. + """ + elsewhere = ( + Work.from_identifiers(self._db, [identifier]).order_by(Work.id).first() + ) + if elsewhere is not None: + self.log.warning( + f"{identifier.type}/{identifier.identifier} resolves to a work " + f"that {library.short_name} does not carry in any of its collections." + ) + return SuppressOutcome( + SuppressResult.NOT_IN_LIBRARY, self._describe_works([elsewhere]) + ) + + self.log.warning(f"No work found for {identifier}") + return SuppressOutcome(SuppressResult.NOT_FOUND) + def suppress_work( self, library: Library, @@ -228,16 +291,14 @@ def suppress_work( collections) or through identifier equivalency. :param dry_run: If true, report the outcome without changing suppression. :param suppress_ambiguous: If the identifier resolves to more than one - distinct work via equivalency, suppress it for the library in - every candidate work instead of refusing to guess which one is - meant. + distinct work, suppress it for the library in every candidate + work instead of refusing to guess which one is meant. :return: The result of the suppression attempt, and a description of the work(s) it applies to, when any were resolved. """ works = self.load_works(identifier, library) if not works: - self.log.warning(f"No work found for {identifier}") - return SuppressOutcome(SuppressResult.NOT_FOUND) + return self._nothing_to_suppress(identifier, library) if len(works) == 1: work = works[0] @@ -261,10 +322,9 @@ def suppress_work( ) return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, description) - # More than one distinct Work is reachable from this identifier via - # equivalency (see load_works) -- e.g. the same ISBN licensed - # through more than one of this library's collections, each with - # its own permanent Work. + # This identifier resolves to more than one of the library's works + # (see load_works) -- e.g. the same title licensed through more than + # one of its collections, each pool with its own permanent Work. if all(library in work.suppressed_for for work in works): # Every candidate already reflects the desired state, so there's # nothing to decide or change -- this isn't really ambiguous in @@ -279,7 +339,7 @@ def suppress_work( # operator meant. self.log.warning( f"{identifier.type}/{identifier.identifier} resolves to " - f"{len(works)} different works via identifier equivalency; " + f"{len(works)} different works for {library.short_name}; " "skipping rather than guessing which one to suppress. Pass " "--suppress-ambiguous to suppress all of them for this library." ) @@ -375,6 +435,9 @@ def _print_results( not_found = [ k for k, v in results.items() if v.result == SuppressResult.NOT_FOUND ] + not_in_library = [ + k for k, v in results.items() if v.result == SuppressResult.NOT_IN_LIBRARY + ] ambiguous = [ k for k, v in results.items() if v.result == SuppressResult.AMBIGUOUS ] @@ -389,6 +452,7 @@ def _print_results( (suppress_label + ":", len(newly_suppressed)), ("Already suppressed:", len(already_suppressed)), ("Not found:", len(not_found)), + ("Not in this library:", len(not_in_library)), ("Ambiguous:", len(ambiguous)), ] col = max(len(label) for label, _ in summary_rows) @@ -403,6 +467,7 @@ def _print_results( ), SuppressResult.ALREADY_SUPPRESSED: "ALREADY SUPPRESSED", SuppressResult.NOT_FOUND: "NOT FOUND", + SuppressResult.NOT_IN_LIBRARY: "NOT IN THIS LIBRARY", SuppressResult.AMBIGUOUS: "AMBIGUOUS", } for (id_type, id_value), outcome in results.items(): diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index f126b52f24..3a755f2579 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -445,7 +445,8 @@ def test_suppress_work_library_with_no_collections( self, db: DatabaseTransactionFixture ): """A library that carries no collections carries no works, so - there is nothing for it to suppress.""" + there is nothing for it to suppress -- but the work does exist, + so this is reported as not-in-this-library rather than not-found.""" test_library = db.library(short_name="test") work = db.work(with_license_pool=True) @@ -454,7 +455,55 @@ def test_suppress_work_library_with_no_collections( test_library, work.presentation_edition.primary_identifier ) + assert result.result == SuppressResult.NOT_IN_LIBRARY + assert work.suppressed_for == [] + + def test_library_collection_ids_are_cached(self, db: DatabaseTransactionFixture): + """load_works consults this once per identifier, so a --file run + would otherwise repeat the same query for every row.""" + test_library = db.library(short_name="test") + collection = db.collection(library=test_library) + + script = SuppressWorkForLibraryScript(db.session) + first = script._library_collection_ids(test_library) + second = script._library_collection_ids(test_library) + + assert first == [collection.id] + assert first is second + + def test_suppress_work_pool_without_a_work(self, db: DatabaseTransactionFixture): + """A LicensePool can exist before its Work has been calculated + (work_id is nullable), so there may be nothing to suppress even + though the identifier is licensed by the library.""" + test_library = db.library(short_name="test") + collection = db.collection(library=test_library) + edition = db.edition() + db.licensepool(edition, collection=collection) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, edition.primary_identifier) + assert result.result == SuppressResult.NOT_FOUND + + def test_suppress_work_not_in_library_distinguished_from_not_found( + self, db: DatabaseTransactionFixture + ): + """An identifier belonging to some other library's collection is a + different problem from an identifier that matches nothing at all, + so the two get distinct results instead of both reading NOT FOUND.""" + test_library = db.library(short_name="test") + db.collection(library=test_library) + other_collection = db.collection() + work = db.work(with_license_pool=True, collection=other_collection) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work( + test_library, work.presentation_edition.primary_identifier + ) + + assert result.result == SuppressResult.NOT_IN_LIBRARY + # The work is named, so an operator can see what they don't carry. + assert result.description == f"{work.title} (work id: {work.id})" assert work.suppressed_for == [] def test_suppress_work_resolves_via_equivalent_identifier( @@ -528,10 +577,12 @@ def test_suppress_work_ambiguous_equivalent_identifier( result = script.suppress_work(test_library, isbn) assert result.result == SuppressResult.AMBIGUOUS - assert result.description is not None - parts = result.description.split("; ") - assert f"{work1.title} (work id: {work1.id})" in parts - assert f"{work2.title} (work id: {work2.id})" in parts + # Candidates are ordered by work id, so the operator-facing output + # is stable between runs on the same data. + assert result.description == ( + f"{work1.title} (work id: {work1.id}); " + f"{work2.title} (work id: {work2.id})" + ) assert work1.suppressed_for == [] assert work2.suppressed_for == [] @@ -596,6 +647,87 @@ def test_suppress_work_work_with_pools_in_several_collections( assert result.result == SuppressResult.NEWLY_SUPPRESSED assert work.suppressed_for == [test_library] + def test_suppress_work_same_identifier_in_two_of_the_librarys_collections( + self, db: DatabaseTransactionFixture + ): + """One vendor identifier can be licensed by two of a library's own + collections -- a consortium's OverDrive collection plus that + library's OverDrive Advantage collection, say. Each pool gets its + own permanent Work, so suppressing one and reporting success would + leave the title circulating through the other. Both must reach the + ambiguity guard.""" + test_library = db.library(short_name="test") + consortium = db.collection(library=test_library) + advantage = db.collection(library=test_library) + + edition = db.edition() + identifier = edition.primary_identifier + + # The same identifier, licensed separately by each collection. + consortium_work = db.work(with_license_pool=False) + advantage_work = db.work(with_license_pool=False) + db.licensepool(edition, collection=consortium, work=consortium_work) + db.licensepool(edition, collection=advantage, work=advantage_work) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, identifier) + + assert result.result == SuppressResult.AMBIGUOUS + assert consortium_work.suppressed_for == [] + assert advantage_work.suppressed_for == [] + + def test_suppress_work_same_identifier_in_two_collections_with_flag( + self, db: DatabaseTransactionFixture + ): + """--suppress-ambiguous covers every collection's copy, which is + what an operator wants once they know why there are two.""" + test_library = db.library(short_name="test") + consortium = db.collection(library=test_library) + advantage = db.collection(library=test_library) + + edition = db.edition() + identifier = edition.primary_identifier + + consortium_work = db.work(with_license_pool=False) + advantage_work = db.work(with_license_pool=False) + db.licensepool(edition, collection=consortium, work=consortium_work) + db.licensepool(edition, collection=advantage, work=advantage_work) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, identifier, suppress_ambiguous=True) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert consortium_work.suppressed_for == [test_library] + assert advantage_work.suppressed_for == [test_library] + + def test_suppress_work_identifier_pool_outside_library_other_pool_inside( + self, db: DatabaseTransactionFixture + ): + """The identifier's own pool may sit outside the library while the + same Work has another pool inside it. Resolution has to find that + work through the equivalency query -- which reaches it because an + identifier is always a member of its own equivalent set (the + fn_recursive_equivalents base case seeds the CTE with it).""" + test_library = db.library(short_name="test") + library_collection = db.collection(library=test_library) + other_collection = db.collection() + + # The work's pool for `identifier` is in a collection the library + # doesn't carry... + work = db.work(with_license_pool=True, collection=other_collection) + identifier = work.presentation_edition.primary_identifier + + # ...but the same work has another pool in a collection it does. + inside_edition = db.edition() + db.licensepool(inside_edition, collection=library_collection, work=work) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, identifier) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.description == f"{work.title} (work id: {work.id})" + assert work.suppressed_for == [test_library] + def test_suppress_work_direct_match_outside_library_falls_through( self, db: DatabaseTransactionFixture ): @@ -879,6 +1011,32 @@ def test_print_results_ambiguous(self, db: DatabaseTransactionFixture, capsys): assert re.search(r"Ambiguous:\s+1", out) assert "[AMBIGUOUS] ISBN/111 -- Book One; Book Two" in out + def test_print_results_not_in_library(self, db: DatabaseTransactionFixture, capsys): + test_library = db.library(short_name="mylib", name="My Library") + script = SuppressWorkForLibraryScript(db.session) + results = { + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NOT_IN_LIBRARY, "Book One (work id: 1)" + ), + ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), + } + started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) + script._print_results( + results, + dry_run=False, + library=test_library, + started_at=started_at, + duration_seconds=1.23, + ) + + out = capsys.readouterr().out + # The two misses are counted separately, so an operator can tell a + # title they don't carry from an identifier that matches nothing. + assert re.search(r"Not in this library:\s+1", out) + assert re.search(r"Not found:\s+1", out) + assert "[NOT IN THIS LIBRARY] ISBN/111 -- Book One (work id: 1)" in out + assert "[NOT FOUND] ISBN/222" in out + def test_print_results_dry_run(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) From 11e24c03dc54a6c8e446ff6ae3f926466f65b099 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein <dabylon@gmail.com> Date: Mon, 28 Sep 2026 10:04:32 -0700 Subject: [PATCH 08/10] Parameterize duplicative setup in the suppress-work tests From review: the file repeated the same setup in a lot of places. Merged into parametrized cases where the tests differed only in inputs and expectations: CSV parsing, the parse_command_line flags, do_run flag threading, the single-work already-suppressed/dry-run matrix, the two-collection pair, the with/without-flag idempotency pair, and the _print_results cases. Pulled the two setups that recur with genuinely different assertions into helpers instead, since parametrizing those would have meant branching on the parameter inside the test body: an ISBN linked by equivalency to N works, and two works plus a CSV naming them. Left the regression tests that build deliberately unusual topologies alone -- each documents a distinct shape in its docstring, and folding them into a shared harness would hide the thing they exist to show. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- tests/manager/scripts/test_suppress.py | 936 +++++++++++-------------- 1 file changed, 410 insertions(+), 526 deletions(-) diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 3a755f2579..1d32f14d18 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -3,6 +3,7 @@ import re import textwrap from datetime import datetime, timezone +from pathlib import Path from unittest.mock import create_autospec, patch import pytest @@ -13,9 +14,57 @@ SuppressWorkForLibraryScript, ) from palace.manager.sqlalchemy.model.datasource import DataSource +from palace.manager.sqlalchemy.model.identifier import Identifier +from palace.manager.sqlalchemy.model.library import Library +from palace.manager.sqlalchemy.model.work import Work from tests.fixtures.database import DatabaseTransactionFixture +def isbn_equivalent_to_works( + db: DatabaseTransactionFixture, library: Library, work_count: int = 2 +) -> tuple[Identifier, list[Work]]: + """An ISBN with no LicensePool of its own, linked by equivalency to + `work_count` distinct works licensed to `library`. + + This is the shape a librarian hits in practice: the ISBN they have in + hand isn't any pool's own identifier, and more than one work can hang + off it. + """ + collection = db.collection(library=library) + works = [ + db.work(with_license_pool=True, collection=collection) + for _ in range(work_count) + ] + isbn = db.identifier(identifier_type="ISBN") + source = DataSource.lookup(db.session, DataSource.OCLC) + for work in works: + isbn.equivalent_to(source, work.presentation_edition.primary_identifier, 1) + return isbn, works + + +def works_with_identifier_csv( + db: DatabaseTransactionFixture, + library: Library, + tmp_path: Path, + *, + repeat_first: bool = False, +) -> tuple[list[Work], str]: + """Two works licensed to `library`, plus the path to a CSV naming + their identifiers. `repeat_first` duplicates the first row.""" + collection = db.collection(library=library) + works = [db.work(with_license_pool=True, collection=collection) for _ in range(2)] + identifiers = [work.presentation_edition.primary_identifier for work in works] + if repeat_first: + identifiers.insert(1, identifiers[0]) + + csv_file = tmp_path / "ids.csv" + csv_file.write_text( + "identifier,identifier_type\n" + + "".join(f"{i.identifier},{i.type}\n" for i in identifiers) + ) + return works, str(csv_file) + + class TestSuppressWorkForLibraryScript: @pytest.mark.parametrize( "cmd_args", @@ -69,30 +118,34 @@ def test_parse_command_line_with_file(self, db: DatabaseTransactionFixture): assert parsed.file == "/tmp/ids.csv" assert parsed.identifier is None - def test_parse_command_line_dry_run(self, db: DatabaseTransactionFixture): - parsed = SuppressWorkForLibraryScript.parse_command_line( - db.session, - ["--library", "lib1", "--identifier", "123", "--dry-run"], - ) - assert parsed.dry_run is True - - def test_parse_command_line_suppress_ambiguous_default_false( - self, db: DatabaseTransactionFixture - ): - parsed = SuppressWorkForLibraryScript.parse_command_line( - db.session, - ["--library", "lib1", "--identifier", "123"], - ) - assert parsed.suppress_ambiguous is False - - def test_parse_command_line_suppress_ambiguous_flag( - self, db: DatabaseTransactionFixture + @pytest.mark.parametrize( + "extra_args,attribute,expected", + [ + pytest.param([], "dry_run", False, id="dry-run-default"), + pytest.param(["--dry-run"], "dry_run", True, id="dry-run-set"), + pytest.param( + [], "suppress_ambiguous", False, id="suppress-ambiguous-default" + ), + pytest.param( + ["--suppress-ambiguous"], + "suppress_ambiguous", + True, + id="suppress-ambiguous-set", + ), + ], + ) + def test_parse_command_line_flags( + self, + db: DatabaseTransactionFixture, + extra_args: list[str], + attribute: str, + expected: bool, ): parsed = SuppressWorkForLibraryScript.parse_command_line( db.session, - ["--library", "lib1", "--identifier", "123", "--suppress-ambiguous"], + ["--library", "lib1", "--identifier", "123", *extra_args], ) - assert parsed.suppress_ambiguous is True + assert getattr(parsed, attribute) is expected def test_parse_command_line_file_and_identifier_mutually_exclusive( self, db: DatabaseTransactionFixture, capsys @@ -138,117 +191,73 @@ def test_load_identifier(self, db: DatabaseTransactionFixture): with pytest.raises(ValueError): script.load_identifier("test", "test") - def test_load_identifiers_from_file(self, db: DatabaseTransactionFixture, tmp_path): - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - textwrap.dedent( + @pytest.mark.parametrize( + "csv_content,expected", + [ + pytest.param( """\ identifier,identifier_type 978-0-06-112008-4,ISBN 12345,Overdrive ID ,ISBN - """ - ) - ) - - script = SuppressWorkForLibraryScript(db.session) - pairs = script.load_identifiers_from_file(str(csv_file), "ISBN") - - assert pairs == [ - ("ISBN", "978-0-06-112008-4"), - ("Overdrive ID", "12345"), - ] - - def test_load_identifiers_from_file_no_type_column( - self, db: DatabaseTransactionFixture, tmp_path - ): - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - textwrap.dedent( + """, + [("ISBN", "978-0-06-112008-4"), ("Overdrive ID", "12345")], + id="row-without-an-identifier-is-skipped", + ), + pytest.param( """\ identifier 978-0-06-112008-4 12345 - """ - ) - ) - - script = SuppressWorkForLibraryScript(db.session) - pairs = script.load_identifiers_from_file(str(csv_file), "ISBN") - - assert pairs == [ - ("ISBN", "978-0-06-112008-4"), - ("ISBN", "12345"), - ] - - def test_load_identifiers_from_file_omitted_type_value_falls_back_to_default( - self, db: DatabaseTransactionFixture, tmp_path - ): - """When identifier_type column exists but a row omits the value (e.g. '12345' - instead of '12345,'), DictReader sets it to None. We must not call .strip() - on None.""" - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - textwrap.dedent( + """, + [("ISBN", "978-0-06-112008-4"), ("ISBN", "12345")], + id="no-type-column-falls-back-to-default", + ), + pytest.param( + # A row that omits the trailing comma entirely ('12345' rather + # than '12345,') leaves DictReader with None, not "", so the + # fallback must not call .strip() on it. """\ identifier,identifier_type 978-0-06-112008-4 12345,Overdrive ID - """ - ) - ) - - script = SuppressWorkForLibraryScript(db.session) - pairs = script.load_identifiers_from_file(str(csv_file), "ISBN") - - assert pairs == [ - ("ISBN", "978-0-06-112008-4"), - ("Overdrive ID", "12345"), - ] - - def test_load_identifiers_from_file_empty_type_falls_back_to_default( - self, db: DatabaseTransactionFixture, tmp_path - ): - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - textwrap.dedent( + """, + [("ISBN", "978-0-06-112008-4"), ("Overdrive ID", "12345")], + id="omitted-type-value-falls-back-to-default", + ), + pytest.param( """\ identifier,identifier_type 978-0-06-112008-4, 12345,Overdrive ID - """ - ) - ) - - script = SuppressWorkForLibraryScript(db.session) - pairs = script.load_identifiers_from_file(str(csv_file), "ISBN") - - assert pairs == [ - ("ISBN", "978-0-06-112008-4"), - ("Overdrive ID", "12345"), - ] - - def test_load_identifiers_from_file_with_duplicates( - self, db: DatabaseTransactionFixture, tmp_path - ): - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - textwrap.dedent( + """, + [("ISBN", "978-0-06-112008-4"), ("Overdrive ID", "12345")], + id="empty-type-value-falls-back-to-default", + ), + pytest.param( """\ identifier,identifier_type 978-0-06-112008-4,ISBN 978-0-06-112008-4,ISBN - """ - ) - ) + """, + [("ISBN", "978-0-06-112008-4"), ("ISBN", "978-0-06-112008-4")], + id="duplicates-are-preserved-for-do-run-to-dedupe", + ), + ], + ) + def test_load_identifiers_from_file( + self, + db: DatabaseTransactionFixture, + tmp_path, + csv_content: str, + expected: list[tuple[str, str]], + ): + csv_file = tmp_path / "ids.csv" + csv_file.write_text(textwrap.dedent(csv_content)) script = SuppressWorkForLibraryScript(db.session) - pairs = script.load_identifiers_from_file(str(csv_file), "ISBN") - assert pairs == [ - ("ISBN", "978-0-06-112008-4"), - ("ISBN", "978-0-06-112008-4"), - ] + assert script.load_identifiers_from_file(str(csv_file), "ISBN") == expected def test_do_run_deduplicates_and_warns( self, db: DatabaseTransactionFixture, tmp_path, caplog @@ -258,27 +267,17 @@ def test_do_run_deduplicates_and_warns( import logging test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - f"identifier,identifier_type\n" - f"{id1.identifier},{id1.type}\n" - f"{id1.identifier},{id1.type}\n" - f"{id2.identifier},{id2.type}\n" + works, csv_path = works_with_identifier_csv( + db, test_library, tmp_path, repeat_first=True ) caplog.set_level(logging.WARNING) script = SuppressWorkForLibraryScript(db.session) - script.do_run(["--library", test_library.short_name, "--file", str(csv_file)]) + script.do_run(["--library", test_library.short_name, "--file", csv_path]) assert "Removed 1 duplicate identifier(s) from input" in caplog.text - assert test_library in work1.suppressed_for - assert test_library in work2.suppressed_for + for work in works: + assert test_library in work.suppressed_for def test_load_identifiers_from_file_missing_identifier_column( self, db: DatabaseTransactionFixture, tmp_path @@ -290,31 +289,31 @@ def test_load_identifiers_from_file_missing_identifier_column( with pytest.raises(ValueError, match='must contain an "identifier" column'): script.load_identifiers_from_file(str(csv_file), "ISBN") - def test_do_run(self, db: DatabaseTransactionFixture, capsys): - test_library = db.library(short_name="test") - test_identifier = db.identifier() - - script = SuppressWorkForLibraryScript(db.session) - suppress_work_mock = create_autospec(script.suppress_work) - suppress_work_mock.return_value = SuppressOutcome( - SuppressResult.NEWLY_SUPPRESSED, "Some Title" - ) - script.suppress_work = suppress_work_mock - args = [ - "--library", - test_library.short_name, - "--identifier-type", - test_identifier.type, - "--identifier", - test_identifier.identifier, - ] - script.do_run(args) - - suppress_work_mock.assert_called_once_with( - test_library, test_identifier, dry_run=False, suppress_ambiguous=False - ) - - def test_do_run_dry_run(self, db: DatabaseTransactionFixture, capsys): + @pytest.mark.parametrize( + "extra_args,expected_kwargs", + [ + pytest.param( + [], {"dry_run": False, "suppress_ambiguous": False}, id="no-flags" + ), + pytest.param( + ["--dry-run"], + {"dry_run": True, "suppress_ambiguous": False}, + id="dry-run", + ), + pytest.param( + ["--suppress-ambiguous"], + {"dry_run": False, "suppress_ambiguous": True}, + id="suppress-ambiguous", + ), + ], + ) + def test_do_run_passes_flags_to_suppress_work( + self, + db: DatabaseTransactionFixture, + capsys, + extra_args: list[str], + expected_kwargs: dict[str, bool], + ): test_library = db.library(short_name="test") test_identifier = db.identifier() @@ -324,111 +323,87 @@ def test_do_run_dry_run(self, db: DatabaseTransactionFixture, capsys): SuppressResult.NEWLY_SUPPRESSED, "Some Title" ) script.suppress_work = suppress_work_mock - args = [ - "--library", - test_library.short_name, - "--identifier-type", - test_identifier.type, - "--identifier", - test_identifier.identifier, - "--dry-run", - ] - script.do_run(args) - - suppress_work_mock.assert_called_once_with( - test_library, test_identifier, dry_run=True, suppress_ambiguous=False - ) - - def test_do_run_suppress_ambiguous_flag( - self, db: DatabaseTransactionFixture, capsys - ): - test_library = db.library(short_name="test") - test_identifier = db.identifier() - script = SuppressWorkForLibraryScript(db.session) - suppress_work_mock = create_autospec(script.suppress_work) - suppress_work_mock.return_value = SuppressOutcome( - SuppressResult.NEWLY_SUPPRESSED, "Some Title" + script.do_run( + [ + "--library", + test_library.short_name, + "--identifier-type", + test_identifier.type, + "--identifier", + test_identifier.identifier, + *extra_args, + ] ) - script.suppress_work = suppress_work_mock - args = [ - "--library", - test_library.short_name, - "--identifier-type", - test_identifier.type, - "--identifier", - test_identifier.identifier, - "--suppress-ambiguous", - ] - script.do_run(args) suppress_work_mock.assert_called_once_with( - test_library, test_identifier, dry_run=False, suppress_ambiguous=True + test_library, test_identifier, **expected_kwargs ) def test_do_run_with_file(self, db: DatabaseTransactionFixture, tmp_path, capsys): test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - f"identifier,identifier_type\n" - f"{id1.identifier},{id1.type}\n" - f"{id2.identifier},{id2.type}\n" - ) + works, csv_path = works_with_identifier_csv(db, test_library, tmp_path) script = SuppressWorkForLibraryScript(db.session) - script.do_run( - [ - "--library", - test_library.short_name, - "--file", - str(csv_file), - ] - ) + script.do_run(["--library", test_library.short_name, "--file", csv_path]) - assert test_library in work1.suppressed_for - assert test_library in work2.suppressed_for + for work in works: + assert test_library in work.suppressed_for out = capsys.readouterr().out assert re.search(r"Newly suppressed:\s+2", out) assert re.search(r"Already suppressed:\s+0", out) assert re.search(r"Not found:\s+0", out) - def test_suppress_work(self, db: DatabaseTransactionFixture): - test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work = db.work(with_license_pool=True, collection=collection) - - assert work.suppressed_for == [] - - script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work( - test_library, work.presentation_edition.primary_identifier - ) - - assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.description == f"{work.title} (work id: {work.id})" - assert work.suppressed_for == [test_library] - - def test_suppress_work_already_suppressed(self, db: DatabaseTransactionFixture): + @pytest.mark.parametrize( + "already_suppressed,dry_run,expected_result,suppressed_after", + [ + pytest.param( + False, False, SuppressResult.NEWLY_SUPPRESSED, True, id="suppresses" + ), + pytest.param( + True, + False, + SuppressResult.ALREADY_SUPPRESSED, + True, + id="already-suppressed", + ), + pytest.param( + False, True, SuppressResult.NEWLY_SUPPRESSED, False, id="dry-run" + ), + pytest.param( + True, + True, + SuppressResult.ALREADY_SUPPRESSED, + True, + id="dry-run-already-suppressed", + ), + ], + ) + def test_suppress_work( + self, + db: DatabaseTransactionFixture, + already_suppressed: bool, + dry_run: bool, + expected_result: SuppressResult, + suppressed_after: bool, + ): test_library = db.library(short_name="test") collection = db.collection(library=test_library) work = db.work(with_license_pool=True, collection=collection) - work.suppressed_for.append(test_library) + if already_suppressed: + work.suppressed_for.append(test_library) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work( - test_library, work.presentation_edition.primary_identifier + test_library, + work.presentation_edition.primary_identifier, + dry_run=dry_run, ) - assert result.result == SuppressResult.ALREADY_SUPPRESSED + assert result.result == expected_result assert result.description == f"{work.title} (work id: {work.id})" - assert work.suppressed_for == [test_library] + assert work.suppressed_for == ([test_library] if suppressed_after else []) def test_suppress_work_no_work_for_identifier(self, db: DatabaseTransactionFixture): test_library = db.library(short_name="test") @@ -516,20 +491,58 @@ def test_suppress_work_resolves_via_equivalent_identifier( equivalency instead of requiring the ISBN to be the LicensePool's own identifier.""" test_library = db.library(short_name="test") + isbn, (work,) = isbn_equivalent_to_works(db, test_library, work_count=1) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, isbn) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert result.description == f"{work.title} (work id: {work.id})" + assert work.suppressed_for == [test_library] + + @pytest.mark.parametrize( + "strength,expected_result,expect_suppressed", + [ + pytest.param( + 1, SuppressResult.NEWLY_SUPPRESSED, True, id="full-confidence-resolves" + ), + pytest.param( + 0.85, SuppressResult.NOT_FOUND, False, id="below-threshold-ignored" + ), + ], + ) + def test_suppress_work_only_high_confidence_equivalencies_resolve( + self, + db: DatabaseTransactionFixture, + strength: float, + expected_result: SuppressResult, + expect_suppressed: bool, + ): + """Equivalency resolution uses `Work.from_identifiers`' strict + default policy (threshold 0.999), so only assertions a data source + is fully confident about can pull a work into a suppression. + + 0.85 isn't an arbitrary "low" number: it's the strength the + importer itself assigns when it links two identifiers purely + because their editions share a permanent work id + (`BibliographicData` in `data_layer/bibliographic.py`). Those + edges exist throughout production data, and a suppression must + not ride one into a work the librarian never named.""" + test_library = db.library(short_name="test") collection = db.collection(library=test_library) work = db.work(with_license_pool=True, collection=collection) - pool_identifier = work.presentation_edition.primary_identifier isbn = db.identifier(identifier_type="ISBN") source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, pool_identifier, 1) + isbn.equivalent_to( + source, work.presentation_edition.primary_identifier, strength + ) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn) - assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.description == f"{work.title} (work id: {work.id})" - assert work.suppressed_for == [test_library] + assert result.result == expected_result + assert work.suppressed_for == ([test_library] if expect_suppressed else []) def test_suppress_work_equivalent_identifier_only_affects_specified_library( self, db: DatabaseTransactionFixture @@ -540,13 +553,7 @@ def test_suppress_work_equivalent_identifier_only_affects_specified_library( `work.suppressed_for`.""" library_a = db.library(short_name="lib_a") library_b = db.library(short_name="lib_b") - collection = db.collection(library=library_a) - work = db.work(with_license_pool=True, collection=collection) - pool_identifier = work.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, pool_identifier, 1) + isbn, (work,) = isbn_equivalent_to_works(db, library_a, work_count=1) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(library_a, isbn) @@ -562,16 +569,7 @@ def test_suppress_work_ambiguous_equivalent_identifier( equivalency, the script must not guess -- it should report AMBIGUOUS and suppress nothing.""" test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) + isbn, (work1, work2) = isbn_equivalent_to_works(db, test_library) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn) @@ -647,58 +645,52 @@ def test_suppress_work_work_with_pools_in_several_collections( assert result.result == SuppressResult.NEWLY_SUPPRESSED assert work.suppressed_for == [test_library] + @pytest.mark.parametrize( + "suppress_ambiguous,expected_result,suppressed_after", + [ + pytest.param( + False, SuppressResult.AMBIGUOUS, False, id="refuses-without-flag" + ), + pytest.param( + True, SuppressResult.NEWLY_SUPPRESSED, True, id="covers-both-with-flag" + ), + ], + ) def test_suppress_work_same_identifier_in_two_of_the_librarys_collections( - self, db: DatabaseTransactionFixture + self, + db: DatabaseTransactionFixture, + suppress_ambiguous: bool, + expected_result: SuppressResult, + suppressed_after: bool, ): """One vendor identifier can be licensed by two of a library's own collections -- a consortium's OverDrive collection plus that library's OverDrive Advantage collection, say. Each pool gets its own permanent Work, so suppressing one and reporting success would - leave the title circulating through the other. Both must reach the - ambiguity guard.""" + leave the title circulating through the other: both must reach the + ambiguity guard, and --suppress-ambiguous must cover both.""" test_library = db.library(short_name="test") - consortium = db.collection(library=test_library) - advantage = db.collection(library=test_library) - edition = db.edition() - identifier = edition.primary_identifier # The same identifier, licensed separately by each collection. - consortium_work = db.work(with_license_pool=False) - advantage_work = db.work(with_license_pool=False) - db.licensepool(edition, collection=consortium, work=consortium_work) - db.licensepool(edition, collection=advantage, work=advantage_work) - - script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work(test_library, identifier) - - assert result.result == SuppressResult.AMBIGUOUS - assert consortium_work.suppressed_for == [] - assert advantage_work.suppressed_for == [] - - def test_suppress_work_same_identifier_in_two_collections_with_flag( - self, db: DatabaseTransactionFixture - ): - """--suppress-ambiguous covers every collection's copy, which is - what an operator wants once they know why there are two.""" - test_library = db.library(short_name="test") - consortium = db.collection(library=test_library) - advantage = db.collection(library=test_library) - - edition = db.edition() - identifier = edition.primary_identifier - - consortium_work = db.work(with_license_pool=False) - advantage_work = db.work(with_license_pool=False) - db.licensepool(edition, collection=consortium, work=consortium_work) - db.licensepool(edition, collection=advantage, work=advantage_work) + works = [] + for _ in range(2): + work = db.work(with_license_pool=False) + db.licensepool( + edition, collection=db.collection(library=test_library), work=work + ) + works.append(work) script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work(test_library, identifier, suppress_ambiguous=True) + result = script.suppress_work( + test_library, + edition.primary_identifier, + suppress_ambiguous=suppress_ambiguous, + ) - assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert consortium_work.suppressed_for == [test_library] - assert advantage_work.suppressed_for == [test_library] + assert result.result == expected_result + for work in works: + assert work.suppressed_for == ([test_library] if suppressed_after else []) def test_suppress_work_identifier_pool_outside_library_other_pool_inside( self, db: DatabaseTransactionFixture @@ -767,80 +759,44 @@ def test_suppress_work_suppress_ambiguous_suppresses_all_candidates( collection) should suppress the work for the library in every candidate, rather than refusing.""" test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) + isbn, (work1, work2) = isbn_equivalent_to_works(db, test_library) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert result.description is not None - parts = result.description.split("; ") - assert f"{work1.title} (work id: {work1.id})" in parts - assert f"{work2.title} (work id: {work2.id})" in parts - assert work1.suppressed_for == [test_library] - assert work2.suppressed_for == [test_library] - - def test_suppress_work_suppress_ambiguous_already_suppressed_for_all( - self, db: DatabaseTransactionFixture - ): - test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - work1.suppressed_for.append(test_library) - work2.suppressed_for.append(test_library) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) - - script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) - - assert result.result == SuppressResult.ALREADY_SUPPRESSED - # Not duplicated by a redundant append. + assert result.description == ( + f"{work1.title} (work id: {work1.id}); " + f"{work2.title} (work id: {work2.id})" + ) assert work1.suppressed_for == [test_library] assert work2.suppressed_for == [test_library] - def test_suppress_work_all_already_suppressed_reports_already_suppressed_without_flag( - self, db: DatabaseTransactionFixture + @pytest.mark.parametrize( + "suppress_ambiguous", + [pytest.param(False, id="without-flag"), pytest.param(True, id="with-flag")], + ) + def test_suppress_work_all_candidates_already_suppressed( + self, db: DatabaseTransactionFixture, suppress_ambiguous: bool ): """Re-running against a fully-covered set of candidates (e.g. after an earlier --suppress-ambiguous run) must be idempotent: it - should report ALREADY_SUPPRESSED, not AMBIGUOUS, since there's - nothing left to decide or change even without the flag.""" + should report ALREADY_SUPPRESSED rather than AMBIGUOUS whether or + not the flag is passed, since there's nothing left to decide or + change -- and no duplicate rows from a redundant append.""" test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - work1.suppressed_for.append(test_library) - work2.suppressed_for.append(test_library) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) + isbn, works = isbn_equivalent_to_works(db, test_library) + for work in works: + work.suppressed_for.append(test_library) script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work(test_library, isbn) + result = script.suppress_work( + test_library, isbn, suppress_ambiguous=suppress_ambiguous + ) assert result.result == SuppressResult.ALREADY_SUPPRESSED - assert work1.suppressed_for == [test_library] - assert work2.suppressed_for == [test_library] + for work in works: + assert work.suppressed_for == [test_library] def test_suppress_work_suppress_ambiguous_partial_already_suppressed( self, db: DatabaseTransactionFixture @@ -850,17 +806,8 @@ def test_suppress_work_suppress_ambiguous_partial_already_suppressed( every candidate ends up suppressed, and the reported title describes only the candidate that actually changed.""" test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) + isbn, (work1, work2) = isbn_equivalent_to_works(db, test_library) work1.suppressed_for.append(test_library) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work(test_library, isbn, suppress_ambiguous=True) @@ -877,16 +824,7 @@ def test_suppress_work_suppress_ambiguous_dry_run_does_not_mutate( self, db: DatabaseTransactionFixture ): test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - isbn = db.identifier(identifier_type="ISBN") - source = DataSource.lookup(db.session, DataSource.OCLC) - isbn.equivalent_to(source, id1, 1) - isbn.equivalent_to(source, id2, 1) + isbn, works = isbn_equivalent_to_works(db, test_library) script = SuppressWorkForLibraryScript(db.session) result = script.suppress_work( @@ -894,8 +832,8 @@ def test_suppress_work_suppress_ambiguous_dry_run_does_not_mutate( ) assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert work1.suppressed_for == [] - assert work2.suppressed_for == [] + for work in works: + assert work.suppressed_for == [] def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( self, db: DatabaseTransactionFixture @@ -924,146 +862,116 @@ def test_suppress_work_prefers_direct_match_over_ambiguous_equivalency( assert work.suppressed_for == [test_library] assert other_work.suppressed_for == [] - def test_suppress_work_dry_run(self, db: DatabaseTransactionFixture): - test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work = db.work(with_license_pool=True, collection=collection) - - script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work( - test_library, - work.presentation_edition.primary_identifier, - dry_run=True, - ) - - assert result.result == SuppressResult.NEWLY_SUPPRESSED - assert work.suppressed_for == [] - - def test_suppress_work_dry_run_already_suppressed( - self, db: DatabaseTransactionFixture - ): - test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work = db.work(with_license_pool=True, collection=collection) - work.suppressed_for.append(test_library) - - script = SuppressWorkForLibraryScript(db.session) - result = script.suppress_work( - test_library, - work.presentation_edition.primary_identifier, - dry_run=True, - ) - - assert result.result == SuppressResult.ALREADY_SUPPRESSED - - def test_print_results_normal(self, db: DatabaseTransactionFixture, capsys): - test_library = db.library(short_name="mylib", name="My Library") - script = SuppressWorkForLibraryScript(db.session) - results = { - ("ISBN", "111"): SuppressOutcome( - SuppressResult.NEWLY_SUPPRESSED, "Book One" + @pytest.mark.parametrize( + "dry_run,results,expected,absent", + [ + pytest.param( + False, + { + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Book One" + ), + ("ISBN", "222"): SuppressOutcome( + SuppressResult.ALREADY_SUPPRESSED, "Book Two" + ), + ("ISBN", "333"): SuppressOutcome(SuppressResult.NOT_FOUND), + }, + [ + "Suppression Results Summary", + "My Library (mylib)", + "2026-02-26 12:00:00 UTC", + "1.23s", + "Newly suppressed: 1", + "Already suppressed: 1", + "Not found: 1", + "[SUPPRESSED] ISBN/111 -- Book One", + "[ALREADY SUPPRESSED] ISBN/222 -- Book Two", + "[NOT FOUND] ISBN/333", + ], + ["[DRY RUN]"], + id="normal", ), - ("ISBN", "222"): SuppressOutcome( - SuppressResult.ALREADY_SUPPRESSED, "Book Two" + pytest.param( + True, + { + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NEWLY_SUPPRESSED, "Book One" + ), + ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), + }, + [ + "[DRY RUN] Suppression Results Summary", + "Would suppress: 1", + "Not found: 1", + "[WOULD SUPPRESS] ISBN/111 -- Book One", + "[NOT FOUND] ISBN/222", + ], + ["[SUPPRESSED]"], + id="dry-run", ), - ("ISBN", "333"): SuppressOutcome(SuppressResult.NOT_FOUND), - } - started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) - script._print_results( - results, - dry_run=False, - library=test_library, - started_at=started_at, - duration_seconds=1.23, - ) - - out = capsys.readouterr().out - assert "Suppression Results Summary" in out - assert "My Library (mylib)" in out - assert "2026-02-26 12:00:00 UTC" in out - assert "1.23s" in out - assert re.search(r"Newly suppressed:\s+1", out) - assert re.search(r"Already suppressed:\s+1", out) - assert re.search(r"Not found:\s+1", out) - assert "[SUPPRESSED] ISBN/111 -- Book One" in out - assert "[ALREADY SUPPRESSED] ISBN/222 -- Book Two" in out - assert "[NOT FOUND] ISBN/333" in out - assert "[DRY RUN]" not in out - - def test_print_results_ambiguous(self, db: DatabaseTransactionFixture, capsys): - test_library = db.library(short_name="mylib", name="My Library") - script = SuppressWorkForLibraryScript(db.session) - results = { - ("ISBN", "111"): SuppressOutcome( - SuppressResult.AMBIGUOUS, "Book One; Book Two" + pytest.param( + False, + { + ("ISBN", "111"): SuppressOutcome( + SuppressResult.AMBIGUOUS, "Book One; Book Two" + ), + }, + [ + "Ambiguous: 1", + "[AMBIGUOUS] ISBN/111 -- Book One; Book Two", + ], + [], + id="ambiguous", ), - } - started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) - script._print_results( - results, - dry_run=False, - library=test_library, - started_at=started_at, - duration_seconds=1.23, - ) - - out = capsys.readouterr().out - assert re.search(r"Ambiguous:\s+1", out) - assert "[AMBIGUOUS] ISBN/111 -- Book One; Book Two" in out - - def test_print_results_not_in_library(self, db: DatabaseTransactionFixture, capsys): - test_library = db.library(short_name="mylib", name="My Library") - script = SuppressWorkForLibraryScript(db.session) - results = { - ("ISBN", "111"): SuppressOutcome( - SuppressResult.NOT_IN_LIBRARY, "Book One (work id: 1)" + pytest.param( + False, + { + ("ISBN", "111"): SuppressOutcome( + SuppressResult.NOT_IN_LIBRARY, "Book One (work id: 1)" + ), + ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), + }, + # The two misses are counted separately, so an operator can + # tell a title they don't carry from an identifier that + # matches nothing. + [ + "Not in this library: 1", + "Not found: 1", + "[NOT IN THIS LIBRARY] ISBN/111 -- Book One (work id: 1)", + "[NOT FOUND] ISBN/222", + ], + [], + id="not-in-library", ), - ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), - } - started_at = datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc) - script._print_results( - results, - dry_run=False, - library=test_library, - started_at=started_at, - duration_seconds=1.23, - ) - - out = capsys.readouterr().out - # The two misses are counted separately, so an operator can tell a - # title they don't carry from an identifier that matches nothing. - assert re.search(r"Not in this library:\s+1", out) - assert re.search(r"Not found:\s+1", out) - assert "[NOT IN THIS LIBRARY] ISBN/111 -- Book One (work id: 1)" in out - assert "[NOT FOUND] ISBN/222" in out - - def test_print_results_dry_run(self, db: DatabaseTransactionFixture, capsys): + ], + ) + def test_print_results( + self, + db: DatabaseTransactionFixture, + capsys, + dry_run: bool, + results: dict[tuple[str, str], SuppressOutcome], + expected: list[str], + absent: list[str], + ): test_library = db.library(short_name="mylib", name="My Library") script = SuppressWorkForLibraryScript(db.session) - results = { - ("ISBN", "111"): SuppressOutcome( - SuppressResult.NEWLY_SUPPRESSED, "Book One" - ), - ("ISBN", "222"): SuppressOutcome(SuppressResult.NOT_FOUND), - } - started_at = datetime(2026, 2, 26, 9, 30, 0, tzinfo=timezone.utc) + script._print_results( results, - dry_run=True, + dry_run=dry_run, library=test_library, - started_at=started_at, - duration_seconds=0.05, + started_at=datetime(2026, 2, 26, 12, 0, 0, tzinfo=timezone.utc), + duration_seconds=1.23, ) - out = capsys.readouterr().out - assert "[DRY RUN] Suppression Results Summary" in out - assert "My Library (mylib)" in out - assert "2026-02-26 09:30:00 UTC" in out - assert "0.05s" in out - assert re.search(r"Would suppress:\s+1", out) - assert re.search(r"Not found:\s+1", out) - assert "[WOULD SUPPRESS] ISBN/111 -- Book One" in out - assert "[NOT FOUND] ISBN/222" in out + # Summary rows are column-padded, so compare against a + # whitespace-collapsed copy to keep the expectations readable. + out = re.sub(r"\s+", " ", capsys.readouterr().out) + for fragment in expected: + assert fragment in out + for fragment in absent: + assert fragment not in out def test_do_run_not_found_identifier(self, db: DatabaseTransactionFixture, capsys): test_library = db.library(short_name="test") @@ -1089,45 +997,21 @@ def test_do_run_commits_once_for_all_suppressions( self, db: DatabaseTransactionFixture, tmp_path, capsys ): test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - f"identifier,identifier_type\n" - f"{id1.identifier},{id1.type}\n" - f"{id2.identifier},{id2.type}\n" - ) + works, csv_path = works_with_identifier_csv(db, test_library, tmp_path) script = SuppressWorkForLibraryScript(db.session) with patch.object(db.session, "commit", wraps=db.session.commit) as mock_commit: - script.do_run( - ["--library", test_library.short_name, "--file", str(csv_file)] - ) + script.do_run(["--library", test_library.short_name, "--file", csv_path]) mock_commit.assert_called_once() - assert test_library in work1.suppressed_for - assert test_library in work2.suppressed_for + for work in works: + assert test_library in work.suppressed_for def test_do_run_rolls_back_all_on_commit_failure( self, db: DatabaseTransactionFixture, tmp_path ): test_library = db.library(short_name="test") - collection = db.collection(library=test_library) - work1 = db.work(with_license_pool=True, collection=collection) - work2 = db.work(with_license_pool=True, collection=collection) - id1 = work1.presentation_edition.primary_identifier - id2 = work2.presentation_edition.primary_identifier - - csv_file = tmp_path / "ids.csv" - csv_file.write_text( - f"identifier,identifier_type\n" - f"{id1.identifier},{id1.type}\n" - f"{id2.identifier},{id2.type}\n" - ) + _, csv_path = works_with_identifier_csv(db, test_library, tmp_path) script = SuppressWorkForLibraryScript(db.session) with ( @@ -1136,7 +1020,7 @@ def test_do_run_rolls_back_all_on_commit_failure( ): with pytest.raises(Exception, match="DB error"): script.do_run( - ["--library", test_library.short_name, "--file", str(csv_file)] + ["--library", test_library.short_name, "--file", csv_path] ) mock_rollback.assert_called_once() From c60db6a7222d95e1f4e81463aebca9dcdfede4fe Mon Sep 17 00:00:00 2001 From: Daniel Bernstein <dabylon@gmail.com> Date: Mon, 28 Sep 2026 10:07:32 -0700 Subject: [PATCH 09/10] Cover the equivalency confidence threshold and workless pools From review: - Nothing proved that a lower-confidence equivalency is excluded, even though the docstring claims the strict policy will not walk into loosely-related works. The new test pins both sides of the threshold. 0.85 is not an arbitrary low number: it is the strength the importer assigns when it links two identifiers purely because their editions share a permanent work id (BibliographicData), so those edges are all over production data and a suppression must not ride one. - A pool whose Work has not been calculated cannot be suppressed, since suppression is a Work/Library relation, so such a pool does not count as an exact match and resolution falls through to equivalency. That was unstated and untested; refusing instead would leave a title the library demonstrably carries unsuppressed because some other pool of the same identifier was mid-import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- src/palace/manager/scripts/suppress.py | 9 ++++++++ tests/manager/scripts/test_suppress.py | 31 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index 456db43164..e0c4c762d0 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -204,6 +204,15 @@ def load_works(self, identifier: Identifier, library: Library) -> list[Work]: case meets the same ambiguity guard as the equivalency case below rather than silently suppressing whichever was found first. + A pool whose Work hasn't been calculated yet counts for nothing + here: suppression is a Work/Library relation, so a pool without a + Work has nothing that *can* be suppressed. Such a pool therefore + doesn't make this an exact match, and resolution falls through to + equivalency rather than refusing -- otherwise a title the library + demonstrably carries under an equivalent identifier would go + unsuppressed because some other pool of the same identifier was + mid-import. + Failing an exact match, we look past LicensePools whose own identifier matches, to also include LicensePools reachable through identifier equivalency -- e.g. an ISBN a librarian has on hand is diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index 1d32f14d18..d6458ead45 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -460,6 +460,37 @@ def test_suppress_work_pool_without_a_work(self, db: DatabaseTransactionFixture) assert result.result == SuppressResult.NOT_FOUND + def test_suppress_work_pool_without_a_work_falls_through_to_equivalency( + self, db: DatabaseTransactionFixture + ): + """A pool whose Work hasn't been calculated can't be suppressed -- + there is no Work to attach the suppression to -- so it doesn't + count as an exact match and resolution falls through to + equivalency. Refusing instead would leave a title the library + demonstrably carries unsuppressed just because some other pool of + the same identifier was mid-import.""" + test_library = db.library(short_name="test") + collection = db.collection(library=test_library) + + # The identifier the librarian names is licensed here, but its + # pool has no Work yet. + workless_edition = db.edition() + db.licensepool(workless_edition, collection=collection) + identifier = workless_edition.primary_identifier + + # An equivalent identifier does have a Work in the same collection. + equivalent_work = db.work(with_license_pool=True, collection=collection) + source = DataSource.lookup(db.session, DataSource.OCLC) + identifier.equivalent_to( + source, equivalent_work.presentation_edition.primary_identifier, 1 + ) + + script = SuppressWorkForLibraryScript(db.session) + result = script.suppress_work(test_library, identifier) + + assert result.result == SuppressResult.NEWLY_SUPPRESSED + assert equivalent_work.suppressed_for == [test_library] + def test_suppress_work_not_in_library_distinguished_from_not_found( self, db: DatabaseTransactionFixture ): From 04e780240a666e09f32a37b4d4c9242c9fdbad12 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein <dabylon@gmail.com> Date: Mon, 28 Sep 2026 10:21:20 -0700 Subject: [PATCH 10/10] Correct the same-identifier multi-work rationale The docstring claimed a consortium OverDrive collection plus an Advantage collection gives two works for one OverDrive id. The model layer enforces the opposite: calculate_work requires all LicensePools with a given identifier to share a work, and points every pool in licensed_through at it. test_all_licensepools_with_same_identifier_get _same_work covers exactly this, two pools of one identifier in different collections, asserting one work. The earlier reasoning conflated two rules -- non-open-access pools are not merged across *different* identifiers by permanent work id, which is true, with pools of the *same* identifier not merging, which is not. The code is unchanged: returning every matching work still guards against inconsistent data, a state calculate_work itself warns about and repairs. Only the justification was wrong, so the docstring and the test that builds that state by hand now say what they actually cover. The legitimate multi-work case is different vendor identifiers sharing an ISBN, which goes through the equivalency path. Also create _collection_ids in __init__ rather than lazily behind hasattr, so the annotation is true for any reader rather than only through its accessor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- src/palace/manager/scripts/suppress.py | 35 +++++++++++++++----------- tests/manager/scripts/test_suppress.py | 21 ++++++++++------ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/src/palace/manager/scripts/suppress.py b/src/palace/manager/scripts/suppress.py index e0c4c762d0..2ddb6957a5 100644 --- a/src/palace/manager/scripts/suppress.py +++ b/src/palace/manager/scripts/suppress.py @@ -3,7 +3,7 @@ from collections.abc import Sequence from datetime import datetime, timezone from enum import Enum, auto -from typing import NamedTuple, cast +from typing import Any, NamedTuple, cast from sqlalchemy import select from sqlalchemy.orm import Session @@ -39,7 +39,9 @@ class SuppressWorkForLibraryScript(Script): BY_DATABASE_ID = "Database ID" - _collection_ids: dict[int, list[int]] + def __init__(self, *args: Any, **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self._collection_ids: dict[int, list[int]] = {} @classmethod def arg_parser(cls, _db: Session) -> argparse.ArgumentParser: @@ -178,8 +180,6 @@ def _library_collection_ids(self, library: Library) -> list[int]: something that should depend on where today falls in a collection's subscription window. """ - if not hasattr(self, "_collection_ids"): - self._collection_ids = {} if library.id not in self._collection_ids: self._collection_ids[library.id] = [ c.id for c in library.associated_collections if c.id is not None @@ -195,14 +195,21 @@ def load_works(self, identifier: Identifier, library: Library) -> list[Work]: each of them instead of looking ambiguous to both. Pools for this exact identifier in one of the library's collections - are exact matches, and win outright over equivalency. There can be - more than one of them: each collection that carries a title - licenses it separately, and every non-open-access pool gets its own - permanent Work -- so a library holding both a consortium's - OverDrive collection and its own OverDrive Advantage collection has - two works for one OverDrive id. All of them are returned, so that - case meets the same ambiguity guard as the equivalency case below - rather than silently suppressing whichever was found first. + are exact matches, and win outright over equivalency. With + consistent data there is at most one Work among them, because + `LicensePool.calculate_work` forces every pool sharing an + identifier onto the same Work -- so suppressing it already covers + every collection that identifier appears in, a consortium's + OverDrive collection and a library's own Advantage collection + alike. Every matching work is nonetheless returned rather than the + first one found, so that inconsistent data (a state + `calculate_work` itself warns about and repairs, logging that the + pools have "more than one Work between them") meets the ambiguity + guard instead of being silently resolved to one of them. + + The legitimate way one identifier reaches several works is + different vendor identifiers sharing an ISBN, and that goes + through the equivalency path below rather than this one. A pool whose Work hasn't been calculated yet counts for nothing here: suppression is a Work/Library relation, so a pool without a @@ -332,8 +339,8 @@ def suppress_work( return SuppressOutcome(SuppressResult.NEWLY_SUPPRESSED, description) # This identifier resolves to more than one of the library's works - # (see load_works) -- e.g. the same title licensed through more than - # one of its collections, each pool with its own permanent Work. + # (see load_works) -- usually different vendor identifiers sharing + # an ISBN, each with its own Work. if all(library in work.suppressed_for for work in works): # Every candidate already reflects the desired state, so there's # nothing to decide or change -- this isn't really ambiguous in diff --git a/tests/manager/scripts/test_suppress.py b/tests/manager/scripts/test_suppress.py index d6458ead45..70c773c3f7 100644 --- a/tests/manager/scripts/test_suppress.py +++ b/tests/manager/scripts/test_suppress.py @@ -687,23 +687,28 @@ def test_suppress_work_work_with_pools_in_several_collections( ), ], ) - def test_suppress_work_same_identifier_in_two_of_the_librarys_collections( + def test_suppress_work_same_identifier_pointing_at_inconsistent_works( self, db: DatabaseTransactionFixture, suppress_ambiguous: bool, expected_result: SuppressResult, suppressed_after: bool, ): - """One vendor identifier can be licensed by two of a library's own - collections -- a consortium's OverDrive collection plus that - library's OverDrive Advantage collection, say. Each pool gets its - own permanent Work, so suppressing one and reporting success would - leave the title circulating through the other: both must reach the - ambiguity guard, and --suppress-ambiguous must cover both.""" + """`LicensePool.calculate_work` forces every pool sharing an + identifier onto one Work, so two pools of one identifier pointing + at different Works is precisely the inconsistency it warns about + ("more than one Work between them") and repairs -- which is why + this state has to be built by hand here rather than through the + normal import path. + + The guard exists so that if the script does meet that state it + refuses, rather than suppressing one Work, reporting success, and + leaving the other circulating.""" test_library = db.library(short_name="test") edition = db.edition() - # The same identifier, licensed separately by each collection. + # Two pools of one identifier, deliberately pointed at different + # works: the inconsistent state calculate_work would repair. works = [] for _ in range(2): work = db.work(with_license_pool=False)