Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculate job percent complete only when the job has rows in the notification table #1664

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+28 −2
Draft

Calculate job percent complete only when the job has rows in the notification table #1664

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
683cd57
Check if the job is currently scheduled before calculating percentage…
whabanks Sep 12, 2023
cd35e0f
Check notification count rather than if job is scheduled
whabanks Sep 13, 2023
31b2a6e
Test that percentage_complete is not calc'd for scheduled jobs
whabanks Sep 13, 2023
8b55902
Merge branch 'main' into fix/div-by-zero-scheduled-jobs
whabanks Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions app/main/views/jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Collaborator

@sastels sastels Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@whabanks whabanks Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking in the jobs table for a job I just created on staging and scheduled for 48 hours in the future:
Screenshot 2023-09-21 at 4 36 11 PM

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.

percentage_complete=0
if job["job_status"] == "scheduled"
else (job["notifications_requested"] / job["notification_count"] * 100),
download_link=url_for(
".view_job_csv",
service_id=current_service.id,
Expand All @@ -448,7 +450,9 @@ def get_job_partials(job, template):
"partials/jobs/notifications.html",
notifications=list(add_preview_of_content_to_notifications(notifications["notifications"])),
more_than_one_page=bool(notifications.get("links", {}).get("next")),
percentage_complete=(job["notifications_requested"] / job["notification_count"] * 100),
percentage_complete=0
if job["job_status"] == "scheduled"
else (job["notifications_requested"] / job["notification_count"] * 100),
download_link=url_for(
".view_job_csv",
service_id=current_service.id,
Expand Down