Fix book suppression links in admin feeds (PP-1318, PP-1348) (#1902)

* Fix admin annotator for book details feed.

* Raise LIBRARY_NOT_FOUND ProblemDetailException when no library.
- As discussed on #1872
  - #1872 (comment)

src/palace/manager/api/admin/controller/work_editor.py

@@ -45,14 +45,15 @@
 from palace.manager.sqlalchemy.model.datasource import DataSource
 from palace.manager.sqlalchemy.model.edition import Edition
 from palace.manager.sqlalchemy.model.lane import Lane
+from palace.manager.sqlalchemy.model.library import Library
 from palace.manager.sqlalchemy.model.licensing import RightsStatus
 from palace.manager.sqlalchemy.model.measurement import Measurement
 from palace.manager.sqlalchemy.model.resource import Hyperlink
 from palace.manager.sqlalchemy.presentation import PresentationCalculationPolicy
 from palace.manager.sqlalchemy.util import create, get_one, get_one_or_create
 from palace.manager.util.datetime_helpers import strptime_utc, utc_now
 from palace.manager.util.languages import LanguageCodes
-from palace.manager.util.problem_detail import ProblemDetail
+from palace.manager.util.problem_detail import ProblemDetail, ProblemDetailException
 
 
 class WorkController(CirculationManagerController, AdminPermissionsControllerMixin):
@@ -375,10 +376,9 @@ def suppress(
     ) -> Response | ProblemDetail:
         """Suppress a book at the level of a library."""
 
-        library = flask.request.library  # type: ignore[attr-defined]
-
+        library: Library | None = getattr(flask.request, "library")
         if library is None:
-            return self.no_library_response()
+            raise ProblemDetailException(LIBRARY_NOT_FOUND)
 
         self.require_library_manager(library)
 
@@ -402,10 +402,9 @@ def unsuppress(
     ) -> Response | ProblemDetail:
         """Remove a book suppression from a book at the level of a library"""
 
-        library = flask.request.library  # type: ignore[attr-defined]
-
+        library: Library | None = getattr(flask.request, "library")
         if library is None:
-            return self.no_library_response()
+            raise ProblemDetailException(LIBRARY_NOT_FOUND)
 
         self.require_library_manager(library)
 
@@ -738,8 +737,3 @@ def custom_lists(self, identifier_type, identifier):
                 lane.update_size(self._db, search_engine=self.search_engine)
 
             return Response(str(_("Success")), 200)
-
-    def no_library_response(self) -> ProblemDetail:
-        return LIBRARY_NOT_FOUND.detailed(
-            "A library must be specified in the request. No library specified."
-        )

src/palace/manager/api/admin/routes.py

@@ -204,7 +204,7 @@ def edit(identifier_type, identifier):
 @returns_problem_detail
 @requires_admin
 @requires_csrf_token
-def suppress(identifier_type, identifier):
+def suppress_for_library(identifier_type, identifier):
     return app.manager.admin_work_controller.suppress(identifier_type, identifier)
 
 
@@ -215,7 +215,7 @@ def suppress(identifier_type, identifier):
 @returns_problem_detail
 @requires_admin
 @requires_csrf_token
-def unsuppress(identifier_type, identifier):
+def unsuppress_for_library(identifier_type, identifier):
     return app.manager.admin_work_controller.unsuppress(identifier_type, identifier)
 
 

src/palace/manager/feed/annotator/admin.py

@@ -10,19 +10,37 @@
 
 
 class AdminAnnotator(LibraryAnnotator):
+    REL_SUPPRESS_FOR_LIBRARY = "http://palaceproject.io/terms/rel/suppress-for-library"
+    REL_UNSUPPRESS_FOR_LIBRARY = (
+        "http://palaceproject.io/terms/rel/unsuppress-for-library"
+    )
+
+    # We do not currently support un/suppressing at the collection level via the API.
+    # These are the link `rels` that we used, in case we want to support them in the future.
+    # REL_SUPPRESS_FOR_COLLECTION = "http://librarysimplified.org/terms/rel/hide"
+    # REL_UNSUPPRESS_FOR_COLLECTION = "http://librarysimplified.org/terms/rel/restore"
+
     def __init__(self, circulation: CirculationAPI | None, library: Library) -> None:
         super().__init__(circulation, None, library)
 
     def annotate_work_entry(
         self, entry: WorkEntry, updated: datetime | None = None
     ) -> None:
+        """Annotate a work entry for the admin client feed.
+
+        This annotator supports links for un/suppressing works at the
+        library level, but not at the collection level. If a work is
+        already suppressed at the collection level, we don't add any
+        per-library un/suppression links to the feed.
+        """
         super().annotate_work_entry(entry)
         if not entry.computed:
             return
         VerboseAnnotator.add_ratings(entry)
 
         identifier = entry.identifier
         active_license_pool = entry.license_pool
+        work = entry.work
 
         # Find staff rating and add a tag for it.
         for measurement in identifier.measurements:
@@ -35,30 +53,33 @@ def annotate_work_entry(
                     self.rating(measurement.quantity_measured, measurement.value)
                 )
 
-        if active_license_pool and active_license_pool.suppressed:
-            entry.computed.other_links.append(
-                Link(
-                    href=self.url_for(
-                        "unsuppress",
-                        identifier_type=identifier.type,
-                        identifier=identifier.identifier,
-                        _external=True,
-                    ),
-                    rel="http://librarysimplified.org/terms/rel/restore",
+        if active_license_pool and not active_license_pool.suppressed:
+            if self.library in work.suppressed_for:
+                entry.computed.other_links.append(
+                    Link(
+                        href=self.url_for(
+                            "unsuppress_for_library",
+                            identifier_type=identifier.type,
+                            identifier=identifier.identifier,
+                            library_short_name=self.library.short_name,
+                            _external=True,
+                        ),
+                        rel=self.REL_UNSUPPRESS_FOR_LIBRARY,
+                    )
                 )
-            )
-        else:
-            entry.computed.other_links.append(
-                Link(
-                    href=self.url_for(
-                        "suppress",
-                        identifier_type=identifier.type,
-                        identifier=identifier.identifier,
-                        _external=True,
-                    ),
-                    rel="http://librarysimplified.org/terms/rel/hide",
+            else:
+                entry.computed.other_links.append(
+                    Link(
+                        href=self.url_for(
+                            "suppress_for_library",
+                            identifier_type=identifier.type,
+                            identifier=identifier.identifier,
+                            library_short_name=self.library.short_name,
+                            _external=True,
+                        ),
+                        rel=self.REL_SUPPRESS_FOR_LIBRARY,
+                    )
                 )
-            )
 
         entry.computed.other_links.append(
             Link(

tests/manager/api/admin/controller/test_work_editor.py

@@ -1,6 +1,8 @@
 import json
 from collections.abc import Generator
+from typing import Any
 
+import feedparser
 import flask
 import pytest
 from werkzeug.datastructures import ImmutableMultiDict
@@ -20,7 +22,9 @@
     UNKNOWN_MEDIUM,
     UNKNOWN_ROLE,
 )
+from palace.manager.api.problem_details import LIBRARY_NOT_FOUND
 from palace.manager.core.classifier import SimplifiedGenreClassifier
+from palace.manager.feed.annotator.admin import AdminAnnotator
 from palace.manager.sqlalchemy.constants import IdentifierType
 from palace.manager.sqlalchemy.model.admin import AdminRole
 from palace.manager.sqlalchemy.model.classification import (
@@ -36,7 +40,7 @@
 from palace.manager.sqlalchemy.model.licensing import RightsStatus
 from palace.manager.sqlalchemy.util import create
 from palace.manager.util.datetime_helpers import datetime_utc
-from palace.manager.util.problem_detail import ProblemDetail
+from palace.manager.util.problem_detail import ProblemDetail, ProblemDetailException
 from tests.fixtures.api_admin import AdminControllerFixture
 from tests.fixtures.api_controller import ControllerFixture
 from tests.mocks.flask import add_request_context
@@ -75,6 +79,80 @@ def work_fixture(
 
 
 class TestWorkController:
+    def test_details(self, work_fixture: WorkFixture):
+        def _links_for_rel(entry, rel: str) -> list[dict[str, Any]]:
+            return [link for link in entry["links"] if link["rel"] == rel]
+
+        [lp] = work_fixture.english_1.license_pools
+        work = lp.work
+        library = work_fixture.ctrl.db.default_library()
+
+        lp.suppressed = False
+        assert library not in work.suppressed_for
+
+        with work_fixture.request_context_with_library_and_admin("/"):
+            response = work_fixture.manager.admin_work_controller.details(
+                lp.identifier.type, lp.identifier.identifier
+            )
+            assert 200 == response.status_code
+            feed = feedparser.parse(response.get_data())
+            [entry] = feed["entries"]
+            hide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_SUPPRESS_FOR_LIBRARY
+            )
+            unhide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_UNSUPPRESS_FOR_LIBRARY
+            )
+            assert 1 == len(hide_for_library)
+            assert 0 == len(unhide_for_library)
+            assert lp.identifier.identifier in hide_for_library[0]["href"]
+
+        work.suppressed_for.append(library)
+        with work_fixture.request_context_with_library_and_admin("/"):
+            response = work_fixture.manager.admin_work_controller.details(
+                lp.identifier.type, lp.identifier.identifier
+            )
+            assert 200 == response.status_code
+            feed = feedparser.parse(response.get_data())
+            [entry] = feed["entries"]
+            hide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_SUPPRESS_FOR_LIBRARY
+            )
+            unhide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_UNSUPPRESS_FOR_LIBRARY
+            )
+            assert 0 == len(hide_for_library)
+            assert 1 == len(unhide_for_library)
+            assert lp.identifier.identifier in unhide_for_library[0]["href"]
+
+        lp.suppressed = True
+        with work_fixture.request_context_with_library_and_admin("/"):
+            response = work_fixture.manager.admin_work_controller.details(
+                lp.identifier.type, lp.identifier.identifier
+            )
+            assert 200 == response.status_code
+            feed = feedparser.parse(response.get_data())
+            [entry] = feed["entries"]
+            hide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_SUPPRESS_FOR_LIBRARY
+            )
+            unhide_for_library = _links_for_rel(
+                entry, AdminAnnotator.REL_UNSUPPRESS_FOR_LIBRARY
+            )
+            assert 0 == len(hide_for_library)
+            assert 0 == len(unhide_for_library)
+
+        work_fixture.admin.remove_role(
+            AdminRole.LIBRARIAN, work_fixture.ctrl.db.default_library()
+        )
+        with work_fixture.request_context_with_library_and_admin("/"):
+            pytest.raises(
+                AdminNotAuthorized,
+                work_fixture.manager.admin_work_controller.details,
+                lp.identifier.type,
+                lp.identifier.identifier,
+            )
+
     def test_roles(self, work_fixture: WorkFixture):
         roles = work_fixture.manager.admin_work_controller.roles()
         assert Contributor.ILLUSTRATOR_ROLE in list(roles.values())
@@ -670,15 +748,6 @@ def test_suppress(self, work_fixture: WorkFixture):
             )
             assert isinstance(response, ProblemDetail)
 
-        # test no library
-        with work_fixture.request_context_with_library_and_admin("/"):
-            flask.request.library = None  # type: ignore[attr-defined]
-            response = work_fixture.manager.admin_work_controller.suppress(
-                lp.identifier.type, lp.identifier.identifier
-            )
-            assert 404 == response.status_code
-            assert "No library specified" in str(response.detail)  # type: ignore[union-attr]
-
         # test unauthorized
         work_fixture.admin.remove_role(AdminRole.LIBRARY_MANAGER, library=library)
         with work_fixture.request_context_with_library_and_admin("/"):
@@ -689,6 +758,15 @@ def test_suppress(self, work_fixture: WorkFixture):
                 lp.identifier.identifier,
             )
 
+        # test no library
+        with work_fixture.request_context_with_library_and_admin("/"):
+            flask.request.library = None  # type: ignore[attr-defined]
+            with pytest.raises(ProblemDetailException) as exc:
+                work_fixture.manager.admin_work_controller.suppress(
+                    lp.identifier.type, lp.identifier.identifier
+                )
+            assert exc.value.problem_detail == LIBRARY_NOT_FOUND
+
     def test_unsuppress(self, work_fixture: WorkFixture):
         work = work_fixture.english_1
         [lp] = work.license_pools
@@ -713,17 +791,8 @@ def test_unsuppress(self, work_fixture: WorkFixture):
             )
             assert isinstance(response, ProblemDetail)
 
-        # test no library
-        with work_fixture.request_context_with_library_and_admin("/"):
-            flask.request.library = None  # type: ignore[attr-defined]
-            response = work_fixture.manager.admin_work_controller.unsuppress(
-                lp.identifier.type, lp.identifier.identifier
-            )
-            assert 404 == response.status_code
-            assert "No library specified" in str(response.detail)  # type: ignore[union-attr]
-
+        # test unauthorized
         work_fixture.admin.remove_role(AdminRole.LIBRARY_MANAGER, library=library)
-
         with work_fixture.request_context_with_library_and_admin("/"):
             pytest.raises(
                 AdminNotAuthorized,
@@ -732,6 +801,15 @@ def test_unsuppress(self, work_fixture: WorkFixture):
                 lp.identifier.identifier,
             )
 
+        # test no library
+        with work_fixture.request_context_with_library_and_admin("/"):
+            flask.request.library = None  # type: ignore[attr-defined]
+            with pytest.raises(ProblemDetailException) as exc:
+                work_fixture.manager.admin_work_controller.unsuppress(
+                    lp.identifier.type, lp.identifier.identifier
+                )
+            assert exc.value.problem_detail == LIBRARY_NOT_FOUND
+
     def test_refresh_metadata(self, work_fixture: WorkFixture):
         wrangler = DataSource.lookup(
             work_fixture.ctrl.db.session, DataSource.METADATA_WRANGLER