PP-483 Added last notified date to loan and hold models

alembic/versions/20231016_21a65b8f391d_loan_and_hold_notification_times.py

@@ -0,0 +1,34 @@
+"""Loan and hold notification times
+
+Revision ID: 21a65b8f391d
+Revises: 5d71a80073d5
+Create Date: 2023-10-16 09:46:58.743018+00:00
+
+"""
+import sqlalchemy as sa
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "21a65b8f391d"
+down_revision = "5d71a80073d5"
+branch_labels = None
+depends_on = None
+
+
+def upgrade() -> None:
+    # ### commands auto generated by Alembic ###
+    op.add_column(
+        "holds", sa.Column("patron_last_notified", sa.DateTime(), nullable=True)
+    )
+    op.add_column(
+        "loans", sa.Column("patron_last_notified", sa.DateTime(), nullable=True)
+    )
+    # ### end Alembic commands ###
+
+
+def downgrade() -> None:
+    # ### commands auto generated by Alembic ###
+    op.drop_column("loans", "patron_last_notified")
+    op.drop_column("holds", "patron_last_notified")
+    # ### end Alembic commands ###

core/jobs/holds_notification.py

@@ -2,11 +2,14 @@
 
 from typing import TYPE_CHECKING
 
+from sqlalchemy import or_
+
 from core.config import Configuration, ConfigurationConstants
 from core.model import Base
 from core.model.configuration import ConfigurationSetting
 from core.model.patron import Hold
 from core.monitor import SweepMonitor
+from core.util.datetime_helpers import utc_now
 from core.util.notifications import PushNotifications
 
 if TYPE_CHECKING:
@@ -38,7 +41,13 @@ def scope_to_collection(self, qu: Query, collection: Collection) -> Query:
 
     def item_query(self) -> Query:
         query = super().item_query()
-        query = query.filter(Hold.position == 0)
+        query = query.filter(
+            Hold.position == 0,
+            or_(
+                Hold.patron_last_notified != utc_now().date(),
+                Hold.patron_last_notified == None,
+            ),
+        )
         return query
 
     def process_items(self, items: list[Hold]) -> None:

core/model/patron.py

@@ -552,6 +552,7 @@ class Loan(Base, LoanAndHoldMixin):
     # Some distributors (e.g. Feedbooks) may have an identifier that can
     # be used to check the status of a specific Loan.
     external_identifier = Column(Unicode, unique=True, nullable=True)
+    patron_last_notified = Column(DateTime, nullable=True)
 
     __table_args__ = (UniqueConstraint("patron_id", "license_pool_id"),)
 
@@ -583,6 +584,7 @@ class Hold(Base, LoanAndHoldMixin):
     end = Column(DateTime(timezone=True), index=True)
     position = Column(Integer, index=True)
     external_identifier = Column(Unicode, unique=True, nullable=True)
+    patron_last_notified = Column(DateTime, nullable=True)
 
     patron: Mapped[Patron] = relationship(
         "Patron", back_populates="holds", lazy="joined"

core/scripts.py

@@ -11,7 +11,7 @@
 from enum import Enum
 from typing import Generator, Optional, Type
 
-from sqlalchemy import and_, exists, tuple_
+from sqlalchemy import and_, exists, or_, tuple_
 from sqlalchemy.orm import Query, Session, defer
 from sqlalchemy.orm.attributes import flag_modified
 from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound
@@ -2824,7 +2824,16 @@ def do_run(self):
             )
             return
 
-        _query = self._db.query(Loan).order_by(Loan.id)
+        _query = (
+            self._db.query(Loan)
+            .filter(
+                or_(
+                    Loan.patron_last_notified != utc_now().date(),
+                    Loan.patron_last_notified == None,
+                )
+            )
+            .order_by(Loan.id)
+        )
         last_loan_id = None
         processed_loans = 0
 
@@ -2863,9 +2872,6 @@ 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
-        # We assume this script runs ONCE A DAY
-        # else this will send notifications multiple times for
-        # the same day
         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})"

core/util/notifications.py

@@ -15,6 +15,7 @@
 from core.model.identifier import Identifier
 from core.model.patron import Hold, Loan, Patron
 from core.model.work import Work
+from core.util.datetime_helpers import utc_now
 from core.util.log import LoggerMixin
 
 
@@ -135,9 +136,13 @@
             f"Patron {loan.patron.authorization_identifier} has {len(tokens)} device tokens. "
             f"Sending loan expiry notification(s)."
         )
-        return cls.send_messages(
+        responses = cls.send_messages(
             tokens, messaging.Notification(title=title, body=body), data
         )
+        if len(responses) > 0:
+            # Atleast one notification succeeded
+            loan.patron_last_notified = utc_now().date()
+        return responses
 
     @classmethod
     def send_activity_sync_message(cls, patrons: list[Patron]) -> list[str]:
@@ -205,6 +210,10 @@
                 data["authorization_identifier"] = hold.patron.authorization_identifier
 
             resp = cls.send_messages(tokens, messaging.Notification(title=title), data)
+            if len(resp) > 0:
+                # Atleast one notification succeeded
+                hold.patron_last_notified = utc_now().date()
+
             responses.extend(resp)
 
         return responses

docker/services/cron/cron.d/circulation

@@ -111,8 +111,8 @@ HOME=/var/www/circulation
 
 # Notifications
 #
-10 12 * * * root core/bin/run loan_notifications >> /var/log/cron.log 2>&1
-15 12 * * * root core/bin/run hold_notifications >> /var/log/cron.log 2>&1
+10 */2 * * * root core/bin/run loan_notifications >> /var/log/cron.log 2>&1
+15 */2 * * * root core/bin/run hold_notifications >> /var/log/cron.log 2>&1
 0 1 * * * root core/bin/run patron_activity_sync_notifications >> /var/log/cron.log 2>&1
 
 # Audiobook playtimes

tests/core/test_holds_notifications.py

@@ -1,10 +1,12 @@
+import datetime
 from unittest.mock import call, patch
 
 import pytest
 
 from core.config import Configuration, ConfigurationConstants
 from core.jobs.holds_notification import HoldsNotificationMonitor
 from core.model.configuration import ConfigurationSetting
+from core.util.datetime_helpers import utc_now
 from tests.fixtures.database import DatabaseTransactionFixture
 
 
@@ -27,12 +29,16 @@ def test_item_query(self, holds_fixture: HoldsNotificationFixture):
         work2 = db.work(with_license_pool=True)
         work3 = db.work(with_license_pool=True)
         work4 = db.work(with_license_pool=True)
+        work5 = db.work(with_license_pool=True)
         hold1, _ = work1.active_license_pool().on_hold_to(patron1, position=1)
         hold2, _ = work2.active_license_pool().on_hold_to(patron1, position=0)
         hold3, _ = work3.active_license_pool().on_hold_to(patron1, position=0)
         hold4, _ = work4.active_license_pool().on_hold_to(patron1, position=None)
+        hold5, _ = work5.active_license_pool().on_hold_to(patron1, position=0)
+        hold5.patron_last_notified = utc_now().date()
+        hold2.patron_last_notified = utc_now().date() - datetime.timedelta(days=1)
 
-        # Only position 0 holds should be queried for
+        # Only position 0 holds, that haven't bene notified today, should be queried for
         assert holds_fixture.monitor.item_query().all() == [hold2, hold3]
 
     def test_script_run(self, holds_fixture: HoldsNotificationFixture):

tests/core/test_scripts.py

@@ -2523,32 +2523,50 @@ def test_loan_notification(self, db: DatabaseTransactionFixture):
         )
 
     def test_do_run(self, db: DatabaseTransactionFixture):
+        now = utc_now()
         self._setup_method(db)
         loan, _ = self.work.active_license_pool().loan_to(
             self.patron,
-            utc_now(),
-            utc_now() + datetime.timedelta(days=1, hours=1),
+            now,
+            now + datetime.timedelta(days=1, 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),
+            now,
+            now + datetime.timedelta(days=2, hours=1),
+        )
+
+        work3 = db.work(with_license_pool=True)
+        p = work3.active_license_pool()
+        loan3, _ = p.loan_to(
+            self.patron,
+            now,
+            now + datetime.timedelta(days=1, hours=1),
+        )
+        # loan 3 was notified today already, so should get skipped
+        loan3.patron_last_notified = now.date()
+
+        work4 = db.work(with_license_pool=True)
+        p = work4.active_license_pool()
+        loan4, _ = p.loan_to(
+            self.patron,
+            now,
+            now + datetime.timedelta(days=1, hours=1),
         )
+        # loan 4 was notified yesterday, so should NOT get skipped
+        loan4.patron_last_notified = now.date() - datetime.timedelta(days=1)
 
         self.script.process_loan = MagicMock()
         self.script.BATCH_SIZE = 1
         self.script.do_run()
 
-        assert self.script.process_loan.call_count == 2
+        assert self.script.process_loan.call_count == 3
         assert self.script.process_loan.call_args_list == [
-            call(
-                loan,
-            ),
-            call(
-                loan2,
-            ),
+            call(loan),
+            call(loan2),
+            call(loan4),
         ]
 
         # Sitewide notifications are turned off

tests/core/util/test_notifications.py

@@ -1,11 +1,13 @@
 import logging
 import re
+from datetime import datetime
 from typing import Generator
 from unittest import mock
 from unittest.mock import MagicMock
 
 import firebase_admin
 import pytest
+import pytz
 from firebase_admin.exceptions import FirebaseError
 from firebase_admin.messaging import UnregisteredError
 from google.auth import credentials
@@ -17,6 +19,7 @@
 from core.model.constants import NotificationConstants
 from core.model.devicetokens import DeviceToken, DeviceTokenTypes
 from core.model.work import Work
+from core.util.datetime_helpers import utc_now
 from core.util.notifications import PushNotifications
 from tests.fixtures.database import DatabaseTransactionFixture
 
@@ -90,6 +93,7 @@ def test_send_loan_notification(self, push_notf_fixture: PushNotificationsFixtur
             assert PushNotifications.send_loan_expiry_message(
                 loan, 1, [device_token]
             ) == ["mid-mock"]
+            assert loan.patron_last_notified == utc_now().date()
 
         with mock.patch("core.util.notifications.messaging") as messaging:
             PushNotifications.send_loan_expiry_message(loan, 1, [device_token])
@@ -123,6 +127,58 @@ def test_send_loan_notification(self, push_notf_fixture: PushNotificationsFixtur
                 {"dry_run": True, "app": push_notf_fixture.app},
             ]
 
+    def test_patron_last_notified_updated(
+        self, push_notf_fixture: PushNotificationsFixture
+    ):
+        db = push_notf_fixture.db
+        patron = db.patron(external_identifier="xyz1")
+        patron.authorization_identifier = "abc1"
+
+        device_token, _ = get_one_or_create(
+            db.session,
+            DeviceToken,
+            device_token="atoken",
+            token_type=DeviceTokenTypes.FCM_ANDROID,
+            patron=patron,
+        )
+        work: Work = db.work(with_license_pool=True)
+        loan, _ = work.active_license_pool().loan_to(patron)  # type: ignore
+        work2: Work = db.work(with_license_pool=True)
+        hold, _ = work2.active_license_pool().on_hold_to(patron)  # type: ignore
+
+        with mock.patch(
+            "core.util.notifications.PushNotifications.send_messages"
+        ) as mock_send, mock.patch("core.util.notifications.utc_now") as mock_now:
+            # Loan expiry
+            # No messages sent
+            mock_send.return_value = []
+            responses = PushNotifications.send_loan_expiry_message(
+                loan, 1, [device_token]
+            )
+            assert responses == []
+            assert loan.patron_last_notified == None
+
+            # One message sent
+            mock_now.return_value = datetime(2020, 1, 1, tzinfo=pytz.UTC)
+            mock_send.return_value = ["mock-mid"]
+            responses = PushNotifications.send_loan_expiry_message(
+                loan, 1, [device_token]
+            )
+            assert responses == ["mock-mid"]
+            # last notified gets updated
+            assert loan.patron_last_notified == datetime(2020, 1, 1).date()
+
+            # Now hold expiry
+            mock_send.return_value = []
+            responses = PushNotifications.send_holds_notifications([hold])
+            assert responses == []
+            assert hold.patron_last_notified == None
+
+            mock_send.return_value = ["mock-mid"]
+            responses = PushNotifications.send_holds_notifications([hold])
+            assert responses == ["mock-mid"]
+            assert hold.patron_last_notified == datetime(2020, 1, 1).date()
+
     def test_send_activity_sync(self, push_notf_fixture: PushNotificationsFixture):
         db = push_notf_fixture.db
         # Only patron 1 will get authorization identifiers
@@ -224,6 +280,9 @@ def test_holds_notification(self, push_notf_fixture: PushNotificationsFixture):
         with mock.patch("core.util.notifications.messaging") as messaging:
             PushNotifications.send_holds_notifications([hold1, hold2])
 
+        assert (
+            hold1.patron_last_notified == hold2.patron_last_notified == utc_now().date()
+        )
         loans_api = "http://localhost/default/loans"
         assert messaging.Message.call_count == 3
         assert messaging.Message.call_args_list == [