[Issue #3536] Add saved opportunity notifications to backend job #3639
Conversation
… of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3527-modify-load-opp-logic-never-delete
Co-authored-by: Michael Chouinard <[email protected]>
Co-authored-by: Michael Chouinard <[email protected]>
Co-authored-by: Michael Chouinard <[email protected]>
… of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3527-modify-load-opp-logic-never-delete
… of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3527-modify-load-opp-logic-never-delete
… of https://github.com/HHS/simpler-grants-gov into mikehgrantsgov/3527-modify-load-opp-logic-never-delete
| log_records = [r for r in caplog.records if "Would send notification to user" in r.message] | ||
| assert len(log_records) == 1 | ||
| extra = log_records[0].__dict__ |
There was a problem hiding this comment.
If we want tests that are a bit less log-focused at the moment, I did add the notification table which we'll use as a sort of auditing table. Could start populating that with something.
There was a problem hiding this comment.
Added some new tests here to and adding logs in generate_notifications.py
| Opportunity, | ||
| UserSavedOpportunity.opportunity_id == Opportunity.opportunity_id, | ||
| ) | ||
| .where(Opportunity.updated_at > UserSavedOpportunity.last_notified_at) |
There was a problem hiding this comment.
Should this be compared against the timestamp from the updated_at column in the opportunity change log table from your other PR?
There was a problem hiding this comment.
Updated this, thanks
| select(User.user_id, UserSavedOpportunity.opportunity_id) | ||
| .join( | ||
| UserSavedOpportunity, | ||
| User.user_id == UserSavedOpportunity.user_id, |
There was a problem hiding this comment.
What if this was select(UserSavedOpportunity)... with similar joins. You'd avoid needing to use mappings/specific columns since the UserSavedOpportunity record is the user_id + opportunity_id.
We should make the user_notification_map opportunity IDs instead just be this class object. Might also simplify the updates later?
There was a problem hiding this comment.
I didn't totally follow the second part of this comment, but take a look and let me know what you think. I think the queries are simpler this way and I think corresponds to what you're going after.
There was a problem hiding this comment.
Yes, you adjusted it like I was thinking
mikehgrantsgov
requested review from
acouch,
andycochran,
btabaska,
coilysiren,
doug-s-nava,
mdragon and
widal001
as code owners
| # Create notification log entry | ||
| # TODO: Use enum for notification reason? | ||
| notification_log = UserNotificationLog( | ||
| user_id=user_id, | ||
| notification_reason="opportunity_updates", | ||
| notification_sent=True, | ||
| ) |
There was a problem hiding this comment.
For the notification reason, I wasn't sure how notifications might evolve over time so left it pretty freeform. For this PR and the later one to add notifications for search, I'd probably just suggest adding some constants at the top of the file like:
class NotificationConstants:
OPPORTUNITY_UPDATES = "opportunity_updates"
At least organizes it slightly kinda like an enum
mikehgrantsgov
deleted the
mikehgrantsgov/3536-add-saved-notifications-to-job
branch
Summary
Fixes #3536
Time to review: 15 mins
Changes proposed
Add
last_notified_attouser_saved_opportunityCollect opportunity notifications, log them out and test.
Context for reviewers
Can be merged, now that #3527 is wrapped up.
Additional information
See attached unit tests