Loan notification once, three days before expiration. (PP-927) (#1675)

core/scripts.py

@@ -2587,8 +2587,8 @@ class LoanNotificationsScript(Script):
     """Notifications must be sent to Patrons based on when their current loans
     are expiring"""
 
-    # Days before on which to send out a notification
-    LOAN_EXPIRATION_DAYS = [5, 1]
+    # List of days before expiration on which we will send out notifications.
+    DEFAULT_LOAN_EXPIRATION_DAYS = [3]
     BATCH_SIZE = 100
 
     def __init__(
@@ -2597,9 +2597,13 @@ def __init__(
         services: Services | None = None,
         notifications: PushNotifications | None = None,
         *args,
+        loan_expiration_days: list[int] | None = None,
         **kwargs,
     ):
         super().__init__(_db, services, *args, **kwargs)
+        self.loan_expiration_days = (
+            loan_expiration_days or self.DEFAULT_LOAN_EXPIRATION_DAYS
+        )
         self.notifications = notifications or PushNotifications(
             self.services.config.sitewide.base_url()
         )
@@ -2657,7 +2661,7 @@ def process_loan(self, loan: Loan):
             self.log.warning(f"Loan: {loan.id} has no end date, skipping")
             return
         delta: datetime.timedelta = loan.end - now
-        if delta.days in self.LOAN_EXPIRATION_DAYS:
+        if delta.days in self.loan_expiration_days:
             self.log.info(
                 f"Patron {patron.authorization_identifier} has an expiring loan on ({loan.license_pool.identifier.urn})"
             )

tests/core/test_scripts.py

@@ -2264,10 +2264,29 @@ def test_repopulate_state(
 
 
 class TestLoanNotificationsScript:
+    TEST_NOTIFICATION_DAYS = [5, 3]
+    PER_DAY_NOTIFICATION_EXPECTATIONS = (
+        # These days should NOT trigger a notification.
+        (7, False),
+        (6, False),
+        (4, False),
+        (2, False),
+        (1, False),
+        # These days SHOULD trigger a notification.
+        (5, True),
+        (3, True),
+    )
+    PARAMETRIZED_POSSIBLE_NOTIFICATION_DAYS = (
+        "days_remaining, is_notification_expected",
+        PER_DAY_NOTIFICATION_EXPECTATIONS,
+    )
+
     def _setup_method(self, db: DatabaseTransactionFixture):
         self.mock_notifications = create_autospec(PushNotifications)
         self.script = LoanNotificationsScript(
-            _db=db.session, notifications=self.mock_notifications
+            _db=db.session,
+            notifications=self.mock_notifications,
+            loan_expiration_days=self.TEST_NOTIFICATION_DAYS,
         )
         self.patron: Patron = db.patron()
         self.work: Work = db.work(with_license_pool=True)
@@ -2279,32 +2298,78 @@ def _setup_method(self, db: DatabaseTransactionFixture):
             device_token="atesttoken",
         )
 
-    def test_loan_notification(self, db: DatabaseTransactionFixture):
+    @pytest.mark.parametrize(*PARAMETRIZED_POSSIBLE_NOTIFICATION_DAYS)
+    def test_loan_notification(
+        self,
+        db: DatabaseTransactionFixture,
+        days_remaining: int,
+        is_notification_expected: bool,
+    ):
         self._setup_method(db)
         p = self.work.active_license_pool()
+
+        # `mypy` thinks `p` is an `Optional[LicensePool]`, so let's clear that up.
+        assert isinstance(p, LicensePool)
+
         loan, _ = p.loan_to(
             self.patron,
             utc_now(),
-            utc_now() + datetime.timedelta(days=1, hours=1),
+            utc_now() + datetime.timedelta(days=days_remaining, hours=1),
         )
-
-        work2 = db.work(with_license_pool=True)
-        loan2, _ = work2.active_license_pool().loan_to(
-            self.patron,
-            utc_now(),
-            utc_now() + datetime.timedelta(days=2, hours=1),
-        )  # Should not get notified
-
         self.script.process_loan(loan)
-        self.script.process_loan(loan2)
 
-        assert self.mock_notifications.send_loan_expiry_message.call_count == 1
-        assert self.mock_notifications.send_loan_expiry_message.call_args[0] == (
-            loan,
-            1,
-            [self.device_token],
+        expected_call_count = 1 if is_notification_expected else 0
+        expected_call_args = (
+            [(loan, days_remaining, [self.device_token])]
+            if is_notification_expected
+            else None
         )
 
+        assert (
+            self.mock_notifications.send_loan_expiry_message.call_count
+            == expected_call_count
+        ), f"Unexpected call count for {days_remaining} day(s) remaining."
+        assert (
+            self.mock_notifications.send_loan_expiry_message.call_args
+            == expected_call_args
+        ), f"Unexpected call args for {days_remaining} day(s) remaining."
+
+    def test_send_all_notifications(self, db: DatabaseTransactionFixture):
+        self._setup_method(db)
+        p = self.work.active_license_pool()
+
+        # `mypy` thinks `p` is an `Optional[LicensePool]`, so let's clear that up.
+        assert isinstance(p, LicensePool)
+
+        loan_start_time = utc_now()
+        loan_end_time = loan_start_time + datetime.timedelta(days=21)
+        loan, _ = p.loan_to(self.patron, loan_start_time, loan_end_time)
+
+        # Simulate multiple days of notification checks on a single loan, counting down to loan expiration.
+        # This needs to happen within the same test, so that we use the same loan each time.
+        for days_remaining, expect_notification in sorted(
+            self.PER_DAY_NOTIFICATION_EXPECTATIONS, reverse=True
+        ):
+            with freeze_time(loan_end_time - datetime.timedelta(days=days_remaining)):
+                self.mock_notifications.send_loan_expiry_message.reset_mock()
+                self.script.process_loan(loan)
+
+                expected_call_count = 1 if expect_notification else 0
+                expected_call_args = (
+                    [(loan, days_remaining, [self.device_token])]
+                    if expect_notification
+                    else None
+                )
+
+                assert (
+                    self.mock_notifications.send_loan_expiry_message.call_count
+                    == expected_call_count
+                ), f"Unexpected call count for {days_remaining} day(s) remaining."
+                assert (
+                    self.mock_notifications.send_loan_expiry_message.call_args
+                    == expected_call_args
+                ), f"Unexpected call args for {days_remaining} day(s) remaining."
+
     def test_do_run(self, db: DatabaseTransactionFixture):
         now = utc_now()
         self._setup_method(db)
@@ -2364,6 +2429,23 @@ def test_constructor(
                 db.session, services=services_fixture.services
             )
         assert script.BATCH_SIZE == 100
+        assert (
+            script.loan_expiration_days
+            == LoanNotificationsScript.DEFAULT_LOAN_EXPIRATION_DAYS
+        )
+        assert script.notifications == mock_notifications.return_value
+        mock_notifications.assert_called_once_with("http://test-circulation-manager")
+
+        with patch(
+            "core.scripts.PushNotifications", autospec=True
+        ) as mock_notifications:
+            script = LoanNotificationsScript(
+                db.session,
+                services=services_fixture.services,
+                loan_expiration_days=[-2, 0, 220],
+            )
+        assert script.BATCH_SIZE == 100
+        assert script.loan_expiration_days == [-2, 0, 220]
         assert script.notifications == mock_notifications.return_value
         mock_notifications.assert_called_once_with("http://test-circulation-manager")