search

cds-snc / notification-planning

Project planning for GC Notify Team
5 stars 0 forks source link

Scheduled Jobs add to daily limit counter #1479

Closed jzbahrai closed 6 months ago

jzbahrai commented 8 months ago

Describe the bug

  1. Set a scheduled send, somehow it gets included in the counter for the daily limit, even if the messages aren't sent
  2. https://cds-snc.freshdesk.com/a/tickets/16350
  3. We need to find a way to delete from the daily limit if the scheduled send is cancelled (potentially) https://github.com/cds-snc/notification-api/blob/main/app/job/rest.py#L66

    To Reproduce

I am unsure of the reasons why this is happening. Needs investigation

whabanks commented 8 months ago

Began investigation, the root of the issue seems to be because we don't check if the notification(s) are scheduled before we increment in Redis in post_notifications and send_one_off_notification. Initial thoughts on solutions are below.

~Extend Redis capabilities~

~1) Extend increment_todays_email_count to allow creation and incrementation of future keys.~ ~2) Check, in send_one_off_notification and post_notifications, if the notifications are scheduled, and increment the future keys instead of current keys.~ ~3) Add a method to decrement a key in the event that a user cancels scheduled notification(s)~

~Perform the increments later in the process if dealing with scheduled notifications~

~1) If the notifications are scheduled, don't increment in send_one_off_notification or post_notifications~ ~2) Delegate the increment to the task that processes the scheduled notifications, therefore incrementing the Redis keys on the day of the send.~

~Currently leaning toward the second solution, we wouldn't need to perform any decrementing if the user cancels a scheduled notification as nothing would have been incremented yet, and it doesn't require any changes to the existing Redis infrastructure that's currently in place - less of a chance to introduce more bugs while fixing a bug.~

whabanks commented 7 months ago

Met with Andrew and Jumana regarding solutions for this one. We currently refresh email daily limit counts in Redis from the DB every 2 hours. The underlying query does not currently include counts from schedule jobs.

The plan:

  1. We will no longer count scheduled jobs toward the daily email count if the job is scheduled for > the current day
  2. Add decrementing functionality so that, when a user cancels a job scheduled for the current day, we can update their daily email limit key in Redis to reflect the change.
  3. Change the underlying query, that refreshes the Redis daily count key from the DB, to pick up and count notifications from scheduled jobs toward their daily limit.
foudamo commented 7 months ago

Should be wrapped up today

whabanks commented 7 months ago

Code complete, tested manually, uncovered and fixed some bugs. Fixed most tests but many are failing with the changes. Will wrap it up today!

whabanks commented 7 months ago

PR's are up and ready for review.

https://github.com/cds-snc/notification-utils/pull/275 https://github.com/cds-snc/notification-api/pull/2112

Our linters refuse methods that exceed a cyclomatic complexity of 15, which create_job in app/job/rest.py exceeded after implementing this fix. Some additional time was needed to refactor, fix tests, and regression/sanity test to ensure that functionality is intact post-refactor.

amazingphilippe commented 7 months ago

@andrewleith to review.

whabanks commented 7 months ago

Code has been merged into staging, QA would be good to do on this one as it changes how we handle daily limits - we want to ensure that the daily limit is still enforced correctly.

QA Steps

Jobs scheduled after today do not block sending

  1. Set your email daily limit to something low, like 10
  2. Schedule a bulk email send, of 10 notifications, for any time after today
  3. Send a one off email to yourself
  4. Note that it was successful and you weren't blocked due to the daily limit

Canceling a job scheduled for today unblocks sending when at the daily limit

  1. Bump your email limit up to 20
  2. Schedule a bulk send of 10 emails for today - this puts the service at it's daily limit
  3. Try to send a 1 off notification, you should be blocked from sending
  4. Cancel the job you just scheduled
  5. Send another one off notification, it should succeed
andrewleith commented 7 months ago

✅ Tested - works as designed 🎉