Ensured the optional attributes of a patron are vetted before being u…

…sed in a notification (#1392)
ThePalaceProject · Sep 19, 2023 · cf04c5c · cf04c5c
1 parent f01807e
commit cf04c5c
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 32 deletions.
diff --git a/core/util/notifications.py b/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(

diff --git a/tests/core/util/test_notifications.py b/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,