Merge branch 'main' into feature/remove-unused-opds-coverage-provider

api/opds2.py

@@ -9,7 +9,7 @@
 from api.circulation import CirculationFulfillmentPostProcessor, FulfillmentInfo
 from api.circulation_exceptions import CannotFulfill
 from core.lane import Facets
-from core.model import ConfigurationSetting, ExternalIntegration
+from core.model import ConfigurationSetting, DataSource, ExternalIntegration
 from core.model.edition import Edition
 from core.model.identifier import Identifier
 from core.model.licensing import LicensePoolDeliveryMechanism
@@ -94,6 +94,10 @@ class TokenAuthenticationFulfillmentProcessor(CirculationFulfillmentPostProcesso
     def __init__(self, collection) -> None:
         pass
 
+    @classmethod
+    def logger(cls) -> logging.Logger:
+        return logging.getLogger(f"{cls.__module__}.{cls.__name__}")
+
     def fulfill(
         self,
         patron: Patron,
@@ -120,7 +124,9 @@ def fulfill(
         if not token_auth or token_auth.value is None:
             return fulfillment
 
-        token = self.get_authentication_token(patron, token_auth.value)
+        token = self.get_authentication_token(
+            patron, licensepool.data_source, token_auth.value
+        )
         if isinstance(token, ProblemDetail):
             raise CannotFulfill()
 
@@ -130,24 +136,18 @@ def fulfill(
 
     @classmethod
     def get_authentication_token(
-        cls, patron: Patron, token_auth_url: str
+        cls, patron: Patron, datasource: DataSource, token_auth_url: str
     ) -> ProblemDetail | str:
         """Get the authentication token for a patron"""
-        log = logging.getLogger("OPDS2API")
-
-        patron_id = patron.username if patron.username else patron.external_identifier
-        if patron_id is None:
-            log.error(
-                f"Could not authenticate the patron({patron.authorization_identifier}): "
-                f"both username and external_identifier are None."
-            )
-            return INVALID_CREDENTIALS
+        log = cls.logger()
 
+        patron_id = patron.identifier_to_remote_service(datasource)
         url = URITemplate(token_auth_url).expand(patron_id=patron_id)
         response = HTTP.get_with_timeout(url)
         if response.status_code != 200:
             log.error(
-                f"Could not authenticate the patron({patron_id}): {str(response.content)}"
+                f"Could not authenticate the patron (authorization identifier: '{patron.authorization_identifier}' "
+                f"external identifier: '{patron_id}'): {str(response.content)}"
             )
             return INVALID_CREDENTIALS
 

core/feed/annotator/base.py

@@ -93,9 +93,10 @@ def contributor(
             return None
 
         name = contributor.display_name or contributor.sort_name
-        name_key = name.lower()
-        if name_key in state[marc_role]:
-            # We've already credited this person with this
+        name_key = name and name.lower()
+        if not name_key or name_key in state[marc_role]:
+            # Either there is no valid name present or
+            # we've already credited this person with this
             # MARC role. Returning a tag would be redundant.
             return None
 
@@ -108,7 +109,6 @@ def contributor(
         # Record the fact that we credited this person with this role,
         # so that we don't do it again on a subsequent call.
         state[marc_role].add(name_key)
-
         return current_role, entry
 
     @classmethod

core/feed/serializer/opds.py

@@ -221,7 +221,9 @@ def serialize_work_entry(self, feed_entry: WorkEntryData) -> etree._Element:
             entry.append(rating_tag)
 
         for author in feed_entry.authors:
-            entry.append(self._serialize_author_tag("author", author))
+            # Author must at a minimum have a name
+            if author.name:
+                entry.append(self._serialize_author_tag("author", author))
         for contributor in feed_entry.contributors:
             entry.append(self._serialize_author_tag("contributor", contributor))
 

core/util/notifications.py

@@ -67,22 +67,26 @@ def send_loan_expiry_message(
         library_short_name = loan.library and loan.library.short_name
         title = f"Only {days_to_expiry} {'days' if days_to_expiry != 1 else 'day'} left on your loan!"
         body = f"Your loan on {edition.title} is expiring soon"
+        data = dict(
+            title=title,
+            body=body,
+            event_type=NotificationConstants.LOAN_EXPIRY_TYPE,
+            loans_endpoint=f"{url}/{loan.library.short_name}/loans",
+            type=identifier.type,
+            identifier=identifier.identifier,
+            library=library_short_name,
+            days_to_expiry=days_to_expiry,
+        )
+        if loan.patron.external_identifier:
+            data["external_identifier"] = loan.patron.external_identifier
+        if loan.patron.authorization_identifier:
+            data["authorization_identifier"] = loan.patron.authorization_identifier
+
         for token in tokens:
             msg = messaging.Message(
                 token=token.device_token,
                 notification=messaging.Notification(title=title, body=body),
-                data=dict(
-                    title=title,
-                    body=body,
-                    event_type=NotificationConstants.LOAN_EXPIRY_TYPE,
-                    loans_endpoint=f"{url}/{loan.library.short_name}/loans",
-                    external_identifier=loan.patron.external_identifier,
-                    authorization_identifier=loan.patron.authorization_identifier,
-                    identifier=identifier.identifier,
-                    type=identifier.type,
-                    library=library_short_name,
-                    days_to_expiry=days_to_expiry,
-                ),
+                data=data,
             )
             resp = messaging.send(msg, dry_run=cls.TESTING_MODE, app=cls.fcm_app())
             responses.append(resp)
@@ -102,15 +106,19 @@ def send_activity_sync_message(cls, patrons: list[Patron]) -> list[str]:
         for patron in patrons:
             tokens = cls.notifiable_tokens(patron)
             loans_api = f"{url}/{patron.library.short_name}/loans"
+            data = dict(
+                event_type=NotificationConstants.ACTIVITY_SYNC_TYPE,
+                loans_endpoint=loans_api,
+            )
+            if patron.external_identifier:
+                data["external_identifier"] = patron.external_identifier
+            if patron.authorization_identifier:
+                data["authorization_identifier"] = patron.authorization_identifier
+
             for token in tokens:
                 msg = messaging.Message(
                     token=token.device_token,
-                    data=dict(
-                        event_type=NotificationConstants.ACTIVITY_SYNC_TYPE,
-                        loans_endpoint=loans_api,
-                        external_identifier=patron.external_identifier,
-                        authorization_identifier=patron.authorization_identifier,
-                    ),
+                    data=data,
                 )
                 msgs.append(msg)
         batch: messaging.BatchResponse = messaging.send_all(
@@ -133,20 +141,24 @@ def send_holds_notifications(cls, holds: list[Hold]) -> list[str]:
             work: Work = hold.work
             identifier: Identifier = hold.license_pool.identifier
             title = f'Your hold on "{work.title}" is available!'
+            data = dict(
+                title=title,
+                event_type=NotificationConstants.HOLD_AVAILABLE_TYPE,
+                loans_endpoint=loans_api,
+                identifier=identifier.identifier,
+                type=identifier.type,
+                library=hold.patron.library.short_name,
+            )
+            if hold.patron.external_identifier:
+                data["external_identifier"] = hold.patron.external_identifier
+            if hold.patron.authorization_identifier:
+                data["authorization_identifier"] = hold.patron.authorization_identifier
+
             for token in tokens:
                 msg = messaging.Message(
                     token=token.device_token,
                     notification=messaging.Notification(title=title),
-                    data=dict(
-                        title=title,
-                        event_type=NotificationConstants.HOLD_AVAILABLE_TYPE,
-                        loans_endpoint=loans_api,
-                        external_identifier=hold.patron.external_identifier,
-                        authorization_identifier=hold.patron.authorization_identifier,
-                        identifier=identifier.identifier,
-                        type=identifier.type,
-                        library=hold.patron.library.short_name,
-                    ),
+                    data=data,
                 )
                 msgs.append(msg)
         batch: messaging.BatchResponse = messaging.send_all(

tests/api/feed/test_annotators.py

@@ -155,6 +155,27 @@ def test_appeals(self, annotators_fixture: TestAnnotatorsFixture):
         actual = [(x["term"], x["label"], x["ratingValue"]) for x in appeal_tags]
         assert set(expect) == set(actual)
 
+    def test_authors(self, annotators_fixture: TestAnnotatorsFixture):
+        db = annotators_fixture.db
+        edition = db.edition()
+        [c_orig] = list(edition.contributors)
+
+        c1 = edition.add_contributor("c1", Contributor.AUTHOR_ROLE, _sort_name="c1")
+        # No name contributor
+        c_none = edition.add_contributor("c2", Contributor.AUTHOR_ROLE)
+        c_none.display_name = ""
+        c_none._sort_name = ""
+
+        authors = Annotator.authors(edition)
+        # The default, c1 and c_none
+        assert len(edition.contributions) == 3
+        # Only default and c1 are used in the feed, because c_none has no name
+        assert len(authors["authors"]) == 2
+        assert set(map(lambda x: x.name, authors["authors"])) == {
+            c1.sort_name,
+            c_orig.sort_name,
+        }
+
     def test_detailed_author(self, annotators_fixture: TestAnnotatorsFixture):
         data, db, session = (
             annotators_fixture,

tests/api/test_opds2.py

@@ -175,7 +175,6 @@ class TestTokenAuthenticationFulfillmentProcessor:
     @patch("api.opds2.HTTP")
     def test_fulfill(self, mock_http, db: DatabaseTransactionFixture):
         patron = db.patron()
-        patron.username = "username"
         collection: Collection = db.collection(
             protocol=ExternalIntegration.OPDS2_IMPORT
         )
@@ -207,10 +206,14 @@ def test_fulfill(self, mock_http, db: DatabaseTransactionFixture):
         processor = TokenAuthenticationFulfillmentProcessor(collection)
         ff_info = processor.fulfill(patron, "", work.license_pools[0], None, ff_info)
 
+        patron_id = patron.identifier_to_remote_service(
+            work.license_pools[0].data_source
+        )
+
         assert mock_http.get_with_timeout.call_count == 1
         assert (
             mock_http.get_with_timeout.call_args[0][0]
-            == "http://example.org/token?userName=username"
+            == f"http://example.org/token?userName={patron_id}"
         )
 
         assert (
@@ -265,9 +268,9 @@ def test_get_authentication_token(self, mock_http, db: DatabaseTransactionFixtur
         resp.raw = io.BytesIO(b"plaintext-auth-token")
         mock_http.get_with_timeout.return_value = resp
         patron = db.patron()
-        patron.username = "test"
+        datasource = DataSource.lookup(db.session, "test", autocreate=True)
         token = TokenAuthenticationFulfillmentProcessor.get_authentication_token(
-            patron, "http://example.org/token"
+            patron, datasource, "http://example.org/token"
         )
 
         assert token == "plaintext-auth-token"
@@ -280,9 +283,9 @@ def test_get_authentication_token_errors(
         resp = Response()
         resp.status_code = 400
         mock_http.get_with_timeout.return_value = resp
-
+        datasource = DataSource.lookup(db.session, "test", autocreate=True)
         token = TokenAuthenticationFulfillmentProcessor.get_authentication_token(
-            db.patron(), "http://example.org/token"
+            db.patron(), datasource, "http://example.org/token"
         )
 
         assert token == INVALID_CREDENTIALS

tests/core/util/test_notifications.py

@@ -30,7 +30,8 @@ def push_notf_fixture(db: DatabaseTransactionFixture) -> PushNotificationsFixtur
 class TestPushNotifications:
     def test_send_loan_notification(self, push_notf_fixture: PushNotificationsFixture):
         db = push_notf_fixture.db
-        patron = db.patron()
+        patron = db.patron(external_identifier="xyz1")
+        patron.authorization_identifier = "abc1"
 
         device_token, _ = get_one_or_create(
             db.session,
@@ -78,7 +79,9 @@ def test_send_loan_notification(self, push_notf_fixture: PushNotificationsFixtur
 
     def test_send_activity_sync(self, push_notf_fixture: PushNotificationsFixture):
         db = push_notf_fixture.db
+        # Only patron 1 will get authorization identifiers
         patron1 = db.patron()
+        patron1.authorization_identifier = "auth1"
         patron2 = db.patron()
         patron3 = db.patron()
 
@@ -132,7 +135,6 @@ def test_send_activity_sync(self, push_notf_fixture: PushNotificationsFixture):
                         event_type=NotificationConstants.ACTIVITY_SYNC_TYPE,
                         loans_endpoint="http://localhost/default/loans",
                         external_identifier=patron2.external_identifier,
-                        authorization_identifier=patron2.authorization_identifier,
                     ),
                 ),
                 mock.call(
@@ -141,7 +143,6 @@ def test_send_activity_sync(self, push_notf_fixture: PushNotificationsFixture):
                         event_type=NotificationConstants.ACTIVITY_SYNC_TYPE,
                         loans_endpoint="http://localhost/default/loans",
                         external_identifier=patron2.external_identifier,
-                        authorization_identifier=patron2.authorization_identifier,
                     ),
                 ),
             ]
@@ -150,7 +151,9 @@ def test_send_activity_sync(self, push_notf_fixture: PushNotificationsFixture):
 
     def test_holds_notification(self, push_notf_fixture: PushNotificationsFixture):
         db = push_notf_fixture.db
+        # Only patron1 will get an identifier
         patron1 = db.patron()
+        patron1.authorization_identifier = "auth1"
         patron2 = db.patron()
         DeviceToken.create(
             db.session, DeviceTokenTypes.FCM_ANDROID, "test-token-1", patron1
@@ -220,7 +223,6 @@ def test_holds_notification(self, push_notf_fixture: PushNotificationsFixture):
                     event_type=NotificationConstants.HOLD_AVAILABLE_TYPE,
                     loans_endpoint=loans_api,
                     external_identifier=hold2.patron.external_identifier,
-                    authorization_identifier=hold2.patron.authorization_identifier,
                     identifier=hold2.license_pool.identifier.identifier,
                     type=hold2.license_pool.identifier.type,
                     library=hold2.patron.library.short_name,