PP-483 Added last notified date to loan and hold models #1464
Conversation
This allows us to run the notification scripts multiple times a day without bombarding the same patron multiple times with the same content
RishiDiwanTT
added
DB migration
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1464 +/- ##
=======================================
Coverage 90.38% 90.38%
=======================================
Files 233 233
Lines 29700 29710 +10
Branches 6787 6793 +6
=======================================
+ Hits 26843 26853 +10
Misses 1824 1824
Partials 1033 1033
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
A couple minor comments on this one:
- Maybe we should set the notified field after we've successfully sent a notification, so that we gracefully retry in the case of a transient failure from firebase.
- Since the ticket is to run the scripts more often, we should probably update the cron schedule in this PR as well.
core/util/notifications.py
Outdated
| @@ -135,6 +136,7 @@ def send_loan_expiry_message( | |||
| f"Patron {loan.patron.authorization_identifier} has {len(tokens)} device tokens. " | |||
| f"Sending loan expiry notification(s)." | |||
| ) | |||
| loan.patron_last_notified = utc_now().date() | |||
There was a problem hiding this comment.
If firebase gives us a transient exception in send_messages, this might cause the notification to be dropped, instead of retried on the next script run. Since we set the notification date before we know that we sent a message.
There was a problem hiding this comment.
I've updated both instances of the code to only update the patron_last_notified if at least one notification was sent successfully.
core/util/notifications.py
Outdated
| @@ -204,6 +206,7 @@ def send_holds_notifications(cls, holds: list[Hold]) -> list[str]: | |||
| if hold.patron.authorization_identifier: | |||
| data["authorization_identifier"] = hold.patron.authorization_identifier | |||
|
|
|||
| hold.patron_last_notified = utc_now().date() | |||
There was a problem hiding this comment.
Same comment about when we set the notified date as above.
Description
This allows us to run the notification scripts multiple times a day without bombarding the same patron multiple times with the same content.
The notifications code updates the timestamp every time a notification is sent out, and the script queries incorporate the new column.
Motivation and Context
Currently, the push notification scripts only run once per day.
This is because additional runs would send out duplicate notifications.
We need to add some way of disallowing the notification from going out twice.
JIRA
How Has This Been Tested?
Updated the unit tests to cover the new cases.
Checklist