Fix AttributeError causing Loan Notification script to fail. (#1698)

core/util/notifications.py

@@ -175,37 +175,45 @@ def send_holds_notifications(self, holds: list[Hold]) -> list[str]:
         _db = Session.object_session(holds[0])
         url = self.base_url
         for hold in holds:
-            tokens = self.notifiable_tokens(hold.patron)
-            self.log.info(
-                f"Notifying patron {hold.patron.authorization_identifier or hold.patron.username} for "
-                f"hold: {hold.work.title}. Patron has {len(tokens)} device tokens."
-            )
-            loans_api = f"{url}/{hold.patron.library.short_name}/loans"
-            work: Work = hold.work
-            identifier: Identifier = hold.license_pool.identifier
-            title = "Your hold is available!"
-            body = f'Your hold on "{work.title}" is available!'
-            data = dict(
-                title=title,
-                body=body,
-                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
-
-            resp = self.send_messages(
-                tokens, messaging.Notification(title=title, body=body), data
-            )
-            if len(resp) > 0:
-                # At least one notification succeeded
-                hold.patron_last_notified = utc_now().date()
-
-            responses.extend(resp)
+            try:
+                tokens = self.notifiable_tokens(hold.patron)
+                self.log.info(
+                    f"Notifying patron {hold.patron.authorization_identifier or hold.patron.username} for "
+                    f"hold: {hold.work.title}. Patron has {len(tokens)} device tokens."
+                )
+                loans_api = f"{url}/{hold.patron.library.short_name}/loans"
+                work: Work = hold.work
+                identifier: Identifier = hold.license_pool.identifier
+                title = "Your hold is available!"
+                body = f'Your hold on "{work.title}" is available!'
+                data = dict(
+                    title=title,
+                    body=body,
+                    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
+
+                resp = self.send_messages(
+                    tokens, messaging.Notification(title=title, body=body), data
+                )
+                if len(resp) > 0:
+                    # At least one notification succeeded
+                    hold.patron_last_notified = utc_now().date()
+
+                responses.extend(resp)
+            except AttributeError:
+                error = f"Failed to send notification for hold {hold.id}"
+                if hold.patron is not None:
+                    error += f" to patron {hold.patron.authorization_identifier or hold.patron.username}"
+                self.log.exception(error)
 
         return responses

tests/core/util/test_notifications.py

@@ -339,6 +339,52 @@ def assert_message_call(
             message_call3, "test-token-3", work2, hold2, include_auth_id=False
         )
 
+    def test_holds_notification_errors(
+        self,
+        push_notf_fixture: PushNotificationsFixture,
+        caplog: pytest.LogCaptureFixture,
+    ):
+        db = push_notf_fixture.db
+
+        # Hold lacking patron
+        hold1 = Hold()
+
+        # Hold with patron, but lacking other required data
+        hold2 = Hold()
+        hold2.patron = db.patron()
+        hold2.patron.authorization_identifier = "12345"
+
+        # Complete hold
+        work = db.work(with_license_pool=True)
+        license_pool = work.active_license_pool()
+        assert license_pool is not None
+        patron = db.patron()
+        DeviceToken.create(
+            db.session, DeviceTokenTypes.FCM_ANDROID, "test-token-1", patron
+        )
+        hold3, _ = license_pool.on_hold_to(patron, position=0)
+
+        # mock the send_messages method to avoid actual calls
+        mock_send_messages = MagicMock()
+        push_notf_fixture.notifications.send_messages = mock_send_messages
+
+        # We should log errors, but continue on and send the successful notifications
+        caplog.set_level(logging.INFO)
+        push_notf_fixture.notifications.send_holds_notifications([hold1, hold2, hold3])
+
+        error1, error2, success = caplog.records
+        assert error1.levelname == error2.levelname == "ERROR"
+        assert f"Failed to send notification for hold {hold1.id}" in error1.message
+        assert (
+            f"Failed to send notification for hold {hold2.id} to patron {hold2.patron.authorization_identifier}"
+            in error2.message
+        )
+
+        assert success.levelname == "INFO"
+        assert "Notifying patron" in success.message
+
+        assert mock_send_messages.call_count == 1
+
     def test_send_messages(
         self,
         push_notf_fixture: PushNotificationsFixture,