WIP - all existing test passing.

src/palace/manager/api/lanes.py

@@ -1529,7 +1529,7 @@ def __init__(self, library, facets):
         # Add one or more WorkLists for every active collection for the
         # library, so that a client can test borrowing a book from
         # any of them.
-        for collection in sorted(library.associated_collections, key=lambda x: x.name):
+        for collection in sorted(library.active_collections, key=lambda x: x.name):
             for medium in Edition.FULFILLABLE_MEDIA:
                 # Give each Worklist a name that is distinctive
                 # and easy for a client to parse.

src/palace/manager/service/redis/models/patron_activity.py

@@ -379,10 +379,9 @@ def collections_ready_for_sync(
         patron activity sync. This indicates that the collection is ready to be
         synced.
         """
-        # TODO: What should happen to loans that are in a collection that is no longer active?
-        #  For now, we'll treat the loans as if they are still in an active collection by using
-        #  all `associated_collections`, rather than just the `active_collections`.
-        collections = patron.library.associated_collections
+        # TODO: What should happen to loans that are in a collection that is not active?
+        #  For now, we'll handle loans only for active collections.
+        collections = patron.library.active_collections
         keys = [
             cls._get_key(redis_client, patron.id, collection.id)
             for collection in collections

src/palace/manager/sqlalchemy/model/collection.py

@@ -21,6 +21,7 @@
 from sqlalchemy.orm.session import Session
 from sqlalchemy.sql import Select
 from sqlalchemy.sql.expression import and_, or_
+from sqlalchemy.sql.functions import count
 
 from palace.manager.core.exceptions import BasePalaceException
 from palace.manager.integration.goals import Goals
@@ -277,9 +278,18 @@ def by_protocol(
 
         return qu
 
-    @staticmethod
+    @property
+    def is_active(self) -> bool:
+        """Return True if the collection is active, False otherwise."""
+        active_query = self.active_collections_filter(sa_select=select(count())).where(
+            Collection.id == self.id
+        )
+        _db = Session.object_session(self)
+        return _db.execute(active_query).scalar() > 0
+
+    @classmethod
     def active_collections_filter(
-        *, sa_select: Select | None = None, today: datetime.date | None = None
+        cls, *, sa_select: Select | None = None, today: datetime.date | None = None
     ) -> Select:
         """Filter to select from only collections that are considered active.
 
@@ -294,33 +304,49 @@ def active_collections_filter(
         sa_select = sa_select if sa_select is not None else select()
         if today is None:
             today = datetime.date.today()
-        return (
-            sa_select.select_from(Collection)
-            .join(IntegrationConfiguration)
-            .where(
-                or_(
-                    not_(
-                        IntegrationConfiguration.settings_dict.has_key(
-                            "subscription_activation_date"
-                        )
-                    ),
-                    IntegrationConfiguration.settings_dict[
+        return cls._filter_active_collections(
+            sa_select=(sa_select.select_from(Collection).join(IntegrationConfiguration))
+        )
+
+    @staticmethod
+    def _filter_active_collections(
+        *, sa_select: Select, today: datetime.date | None = None
+    ) -> Select:
+        """Constrain to only active collections.
+
+        A collection is considered active if it either:
+            - has no activation/expiration settings; or
+            - meets the criteria specified by the activation/expiration settings.
+
+        :param sa_select: A SQLAlchemy Select object.
+        :param today: The date to use as the current date. Defaults to today.
+        :return: A filtered SQLAlchemy Select object.
+        """
+        if today is None:
+            today = datetime.date.today()
+        return sa_select.where(
+            or_(
+                not_(
+                    IntegrationConfiguration.settings_dict.has_key(
                         "subscription_activation_date"
-                    ].astext.cast(Date)
-                    <= today,
+                    )
                 ),
-                or_(
-                    not_(
-                        IntegrationConfiguration.settings_dict.has_key(
-                            "subscription_expiration_date"
-                        )
-                    ),
-                    IntegrationConfiguration.settings_dict[
+                IntegrationConfiguration.settings_dict[
+                    "subscription_activation_date"
+                ].astext.cast(Date)
+                <= today,
+            ),
+            or_(
+                not_(
+                    IntegrationConfiguration.settings_dict.has_key(
                         "subscription_expiration_date"
-                    ].astext.cast(Date)
-                    >= today,
+                    )
                 ),
-            )
+                IntegrationConfiguration.settings_dict[
+                    "subscription_expiration_date"
+                ].astext.cast(Date)
+                >= today,
+            ),
         )
 
     @property

src/palace/manager/sqlalchemy/model/lane.py

@@ -1438,7 +1438,7 @@ def initialize(
             self.library_id = library.id
             if self.collection_ids is None:
                 self.collection_ids = [
-                    collection.id for collection in library.associated_collections_ids
+                    collection.id for collection in library.active_collections
                 ]
         self.display_name = display_name
         if genres:

src/palace/manager/sqlalchemy/model/licensing.py

@@ -24,6 +24,7 @@
 from sqlalchemy.orm import Mapped, lazyload, relationship
 from sqlalchemy.orm.session import Session
 
+from palace.manager.api.circulation_exceptions import CannotHold, CannotLoan
 from palace.manager.core.exceptions import BasePalaceException
 from palace.manager.sqlalchemy.constants import (
     DataSourceConstants,
@@ -1079,13 +1080,27 @@ def loan_to(
     ) -> tuple[Loan, bool]:
         _db = Session.object_session(patron)
         kwargs = dict(start=start or utc_now(), end=end)
-        loan, is_new = get_one_or_create(
-            _db,
-            Loan,
-            patron=patron,
-            license_pool=self,
-            create_method_kwargs=kwargs,
-        )
+
+        # We can make new loans on active collections, but not on inactive ones.
+        # But we can look up already-existing loans on inactive collections.
+        if self.collection.is_active:
+            loan, is_new = get_one_or_create(
+                _db,
+                Loan,
+                patron=patron,
+                license_pool=self,
+                create_method_kwargs=kwargs,
+            )
+        else:
+            loan = get_one(
+                _db,
+                Loan,
+                patron=patron,
+                license_pool=self,
+            )
+            if not loan:
+                raise CannotLoan("Cannot create a new loan on an inactive collection.")
+            is_new = False
 
         if fulfillment:
             loan.fulfillment = fulfillment
@@ -1108,8 +1123,17 @@ def on_hold_to(
         if not patron.library.settings.allow_holds:
             raise PolicyException("Holds are disabled for this library.")
         start = start or utc_now()
-        hold, new = get_one_or_create(_db, Hold, patron=patron, license_pool=self)
-        hold.update(start, end, position)
+
+        # We can create new holds on active collections, but not on inactive ones.
+        # But we can look up already-existing holds in inactive collections.
+        if self.collection.is_active:
+            hold, new = get_one_or_create(_db, Hold, patron=patron, license_pool=self)
+            hold.update(start, end, position)
+        else:
+            hold = get_one(_db, Hold, patron=patron, license_pool=self)
+            if not hold:
+                raise CannotHold("Cannot create a new hold on an inactive collection.")
+            new = False
         return hold, new
 
     class _LicensePriority(IntEnum):

tests/fixtures/database.py

@@ -428,11 +428,8 @@ def make_default_library_with_collections(self) -> None:
         inactive_collection = self.collection(
             "Default Inactive Collection",
             protocol=OPDSAPI,
-            settings=self.opds_settings(
-                data_source="OPDS",
-                subscription_activation_date=datetime.date(2000, 12, 31),
-                subscription_expiration_date=datetime.date(1999, 1, 1),
-            ),
+            settings=self.opds_settings(data_source="OPDS"),
+            inactive=True,
         )
         inactive_collection.associated_libraries.append(library)
 
@@ -607,6 +604,7 @@ def collection(
         protocol: type[BaseCirculationAPI[Any, Any]] | str = OPDSAPI,
         settings: BaseCirculationApiSettings | dict[str, Any] | None = None,
         library: Library | None = None,
+        inactive: bool = False,
     ) -> Collection:
         name = name or self.fresh_str()
         protocol_str = (
@@ -634,8 +632,23 @@ def collection(
 
         if library and library not in collection.associated_libraries:
             collection.associated_libraries.append(library)
+
+        if inactive:
+            self.make_collection_inactive(collection)
         return collection
 
+    @staticmethod
+    def make_collection_inactive(collection: Collection):
+        """Make a collection inactive using some settings that will make it so."""
+        collection.integration_configuration.settings_dict.update(
+            {
+                "subscription_activation_date": datetime.date(2000, 12, 31),
+                "subscription_expiration_date": datetime.date(1999, 1, 1),
+            }
+        )
+        flag_modified(collection.integration_configuration, "settings_dict")
+        assert not collection.is_active
+
     def work(
         self,
         title=None,

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

@@ -96,6 +96,7 @@ def test_collections_get_with_no_collections(
     ) -> None:
         # Delete any existing collections created by the test setup.
         db.session.delete(db.default_collection())
+        db.session.delete(db.default_inactive_collection())
 
         response = controller.process_get()
         assert isinstance(response, Response)
@@ -108,13 +109,24 @@ def test_collections_get_with_no_collections(
         expected_names = {k for k, v in controller.registry}
         assert names == expected_names
 
+    @pytest.mark.parametrize(
+        "is_inactive",
+        (
+            pytest.param(True, id="inactive collection"),
+            pytest.param(False, id="active collection"),
+        ),
+    )
     def test_collections_get_collections_with_multiple_collections(
         self,
         controller: CollectionSettingsController,
         flask_app_fixture: FlaskAppFixture,
         db: DatabaseTransactionFixture,
+        is_inactive: bool,
     ) -> None:
-        [c1] = db.default_library().associated_collections
+        default_library = db.library(short_name="default", name="Default Library")
+        c1 = db.collection(
+            library=default_library, name="Default Collection", inactive=is_inactive
+        )
 
         c2 = db.collection(
             name="Collection 2",
@@ -137,7 +149,7 @@ def test_collections_get_collections_with_multiple_collections(
         c3.parent = c2
 
         l1 = db.library(short_name="L1")
-        c3.associated_libraries += [l1, db.default_library()]
+        c3.associated_libraries += [l1, default_library]
         db.integration_library_configuration(
             c3.integration_configuration,
             l1,
@@ -192,10 +204,11 @@ def test_collections_get_collections_with_multiple_collections(
         )
         assert "L1" == coll3_l1.get("short_name")
         assert 14 == coll3_l1.get("ebook_loan_duration")
-        assert db.default_library().short_name == coll3_default.get("short_name")
+        assert default_library.short_name == coll3_default.get("short_name")
 
+        # A librarian only sees collections associated with their library
+        # (including inactive ones).
         with flask_app_fixture.test_request_context("/", admin=l1_librarian):
-            # A librarian only sees collections associated with their library.
             response2 = controller.process_collections()
         assert isinstance(response2, Response)
         assert response2.status_code == 200

tests/manager/api/controller/test_base.py

@@ -2,6 +2,7 @@
 from unittest.mock import MagicMock, patch
 
 import flask
+import pytest
 from flask import Response
 from werkzeug.datastructures import Authorization
 
@@ -193,10 +194,24 @@ def test_authentication_sends_proper_headers(
             response = circulation_fixture.controller.authenticate()
             assert None == response.headers.get("WWW-Authenticate")
 
-    def test_load_licensepools(self, circulation_fixture: CirculationControllerFixture):
+    @pytest.mark.parametrize(
+        "is_inactive",
+        (
+            pytest.param(True, id="inactive collection"),
+            pytest.param(False, id="active collection"),
+        ),
+    )
+    def test_load_licensepools(
+        self, circulation_fixture: CirculationControllerFixture, is_inactive: bool
+    ):
         # Here's a Library that has two Collections.
         library = circulation_fixture.library
-        [c1] = library.associated_collections
+        assert len(library.associated_collections) == 2
+        c1 = (
+            circulation_fixture.db.default_inactive_collection()
+            if is_inactive
+            else circulation_fixture.db.default_collection()
+        )
         c2 = circulation_fixture.db.collection()
         c2.associated_libraries.append(library)
 

tests/manager/api/controller/test_crawlfeed.py

@@ -57,6 +57,8 @@ def test_crawlable_library_feed(
         # a library.
         controller = circulation_fixture.manager.opds_feeds
         library = circulation_fixture.db.default_library()
+        active_collection = circulation_fixture.db.default_collection()
+        inactive_collection = circulation_fixture.db.default_inactive_collection()
         with circulation_fixture.request_context_with_library("/"):
             with self.mock_crawlable_feed(circulation_fixture):
                 response = controller.crawlable_library_feed()
@@ -79,11 +81,16 @@ def test_crawlable_library_feed(
         assert OPDSAcquisitionFeed == kwargs.pop("feed_class")
 
         # A CrawlableCollectionBasedLane has been set up to show
-        # everything in any of the requested library's collections.
+        # everything in any of the requested library's active collections.
         lane = kwargs.pop("worklist")
         assert isinstance(lane, CrawlableCollectionBasedLane)
         assert library.id == lane.library_id
-        assert [x.id for x in library.associated_collections] == lane.collection_ids
+        assert library.associated_collections == [
+            active_collection,
+            inactive_collection,
+        ]
+        assert library.active_collections == [active_collection]
+        assert set(lane.collection_ids) == {x.id for x in library.active_collections}
         assert {} == kwargs
 
     def test_crawlable_collection_feed(

tests/manager/api/controller/test_loan.py

@@ -1417,14 +1417,14 @@ def test_active_loans(
         assert not "<entry>" in response.get_data(as_text=True)
         assert response.headers["Cache-Control"].startswith("private,")
 
-        # We queued up a sync_patron_activity task to go sync the patrons information
+        # We queued up a sync_patron_activity task to go sync the patrons information,
+        # Only active collections are synced.
         assert isinstance(patron, Patron)
-        assert sync_task.apply_async.call_count == len(
-            patron.library.associated_collections
-        )
-        for library in patron.library.associated_collections:
+        patron_collections = patron.library.active_collections
+        assert sync_task.apply_async.call_count == len(patron_collections)
+        for collection in patron_collections:
             sync_task.apply_async.assert_any_call(
-                (library.id, patron.id, loan_fixture.valid_credentials["password"]),
+                (collection.id, patron.id, loan_fixture.valid_credentials["password"]),
             )
 
         # Set up some loans and holds