Calculate job percent complete only when the job has rows in the notification table #1664
Conversation
… complete - Display 0% complete if the job is scheduled instead of calclating to avoid a division by zero exception
🧪 Review environmenthttps://pewjqi44kua72gdsxmihy5xqla0ijirz.lambda-url.ca-central-1.on.aws/ |
| @@ -429,7 +429,9 @@ def get_job_partials(job, template): | |||
| "notifications_header": render_template( | |||
| "partials/jobs/notifications_header.html", | |||
| notifications=list(add_preview_of_content_to_notifications(notifications["notifications"])), | |||
| percentage_complete=(job["notifications_requested"] / job["notification_count"] * 100), | |||
There was a problem hiding this comment.
So if a job is still in a scheduled state then the job["notification_count"] is zero? I would have thought that would be the (eventual) total # of notifications?
There was a problem hiding this comment.
I tend to agree that the eventual # of notifications should be displayed on this page, but it looks like the job stats come from here in API and at a glance they don't seem to include scheduled notifications. I think that change might be a separate PR for a related bug (i.e. that scheduled notifications aren't part of the stats)
In terms of a guard against this page crashing, this change seems to fit the bill - I'm not sure we could do much more on the admin side if we're given a 0 for the notification_count. Although maybe the check should be if job["notification_count"] is 0 rather than if the job is scheduled?
Out of scope for this PR, but for consistency sake, do scheduled notifications similarly count against the daily limit I wonder?
There was a problem hiding this comment.
Good point regarding the bug where scheduled notifications aren't part of the stats. When I tested locally and in staging the notifications for scheduled jobs only get created once the scheduled_for time has elapsed. So until the send occurs it seems we won't be able to include those notifications in the stats. It looks like this is the expected behaviour - I came across a test that indicates no stats should be returned if a job has no associated rows in the notifications table.
That said, checking the notification_count instead of if the job is scheduled makes more sense as eventually there will be notifications available in the DB for the percent complete calculation. I appreciate the discussion it's sparked a few ideas to flesh out unit tests in this area of the code! ❤️
To answer your question about scheduled notifications counting against the daily limit, I think the answer is yes. When we create a job we increment the daily count in Redis.
There was a problem hiding this comment.
Looking in the jobs table for a job I just created on staging and scheduled for 48 hours in the future:
notification_count is the number of notifications that will be eventually sent. @whabanks if you are able to schedule a job and you see a notification_count of 0 that would be a bug. I think we still need to figure out how to reproduce that before we make any code changes.
whabanks
changed the title
|
@whabanks do you know how to reproduce the original bug? I have not been able to reproduce it so I'm not sure how to test if this PR fixes the bug. |
I tested and debugged this with the following steps:
|
|
I followed those steps on Staging and I still wasn't able to reproduce the bug:
|
|
I clicked the wrong button and closed this by accident! Sorry. |
🧪 Review environmenthttps://f6bivkakrsnkxeozkwagke4uwe0wqmsb.lambda-url.ca-central-1.on.aws/ |
Summary | Résumé
This PR adds a defensive check to
get_job_partialsto prevent aDivisionByZeroErrorthat occurred when attempting to view a scheduled job. This issue was uncovered when investigating an alarmTest instructions | Instructions pour tester la modification
Scheduled Jobssection