2990 - stuck files notification #3195

jtimpe · 2024-09-20T16:02:24Z

Summary of Changes

Pull request closes #2990

How to Test

Make sure the SENDGRID_API_KEY env var is set, reach out if you need a valid key

Spin up your environment

cd tdrs-backend && docker compose up
cd tdrs-frontend && docker compose up --build

Sign in and change your user role to OFA System Admin
Create some submissions. Using shell_plus, change the created_at to >1hr ago.
Reparse some submissions. Using shell_plus, change the timeout_at to some time in the past. Change finished and success to False.
Run docker compose exec web python manage.py find_pending_submissions.

Try a lot of combinations. If you can get it to fire in real life stuck-file scenarios, that's a plus.

Example of email

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

Notification occurs if a newly submitted file has been in pending state for 1+ hours
Notification content matches spec Notification content must include:
- STT, Program Type, Fiscal Period, Section Name, Submission Date/time
- Dynamic link that passes data file ID to the django URL string
- Confirm these email events should be captured in logs
All sys-admin users receive the notification
lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

Are all areas of code introduced in this PR meaningfully tested?
- If this PR introduces backend code changes, are they meaningfully tested?
- If this PR introduces frontend code changes, are they meaningfully tested?
Are code coverage minimums met?
- Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
- Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

Are backend code style checks passing on CircleCI?
Are frontend code style checks passing on CircleCI?
Are code maintainability principles being followed?

Deliverable 4: Accessible

Does this PR complete the epic?
Are links included to any other gov-approved PRs associated with epic?
Does PR include documentation for Raft's a11y review?
Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Does this PR provide background for why coding decisions were made?
If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces dependencies, are their licenses documented?
Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

Does the OWASP Scan pass on CircleCI?
Do manual code review and manual testing detect any new security issues?
If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

the purpose of the research
methods used to conduct the research
who participated in the research
what was tested and how
impact of research on TDP
(if applicable) final design mockups produced for TDP development

codecov · 2024-09-23T15:55:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.66%. Comparing base (c15cb7c) to head (1f6756b).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3195   +/-   ##
========================================
  Coverage    92.66%   92.66%           
========================================
  Files           47       47           
  Lines         1009     1009           
  Branches       169      169           
========================================
  Hits           935      935           
  Misses          42       42           
  Partials        32       32

Flag	Coverage Δ
dev-frontend	`92.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fab247d...1f6756b. Read the comment docs.

andrew-jameson · 2024-09-23T17:31:57Z

tdrs-backend/tdpservice/email/templates/stuck-file-single.html

+<p>The system has detected stuck <b>{{ section }}</b> data submitted by <b>{{ stt }}</b> in <b>{{ program_type }}</b> for Fiscal Year <b>{{ fiscal_year }}</b> that was submitted on <b>{{ submission_date }}</b>.</p>
+
+<p>
+  <a class="button" href="https://tdp-frontend-a11y.app.cloud.gov/" style="background-color:#336a90;border-radius:4px;color:#ffffff;display:inline-block;font-family:sans-serif;font-size:18px;font-weight:bold;line-height:60px;text-align:center;text-decoration:none;width: auto; padding-left: 24px; padding-right: 24px;-webkit-text-size-adjust:none;">


Does the link to a11y environment need to be a variable?

i deleted this whole template since it wasn't used

andrew-jameson · 2024-09-23T17:37:37Z

tdrs-backend/tdpservice/data_files/tasks.py

+@shared_task
+def notify_stuck_files():
+    """Find files stuck in 'Pending' and notify SysAdmins."""
+    recipients = User.objects.filter(


A minor nitpick, would it make sense to first check the stuck_files count before compiling the recipients list? It's minor but if we're not going to send an email, we don't need to do those actions.

stuck_files = get_stuck_files() if stuck_files.count() > 0: recipients = <...> send_stuck_file_email(...)

andrew-jameson · 2024-09-23T17:39:37Z

tdrs-backend/tdpservice/email/templates/stuck-file-list.html

+      <td style="padding: 4px; border-style: solid; border-color: #000000; border-width: 1px;">{{ file.section }}</td>
+      <td style="padding: 4px; border-style: solid; border-color: #000000; border-width: 1px;">{{ file.fiscal_year }}</td>
+      <td style="padding: 4px; border-style: solid; border-color: #000000; border-width: 1px;">{{ file.created_at }} {{ file.created_time_ago }}</td>
+      <td style="padding: 4px; border-style: solid; border-color: #000000; border-width: 1px;"><a href="">View in Admin Console</a></td>


Missing variable/link in href="" ?

andrew-jameson · 2024-09-23T17:42:35Z

tdrs-backend/tdpservice/data_files/test/test_stuck_files.py

+    )
+
+    stuck_files = get_stuck_files()
+    assert stuck_files.count() == 0


Should we have more asserts between each DataFile creation so that it is clear and debuggable which scenario failed?

tdrs-backend/tdpservice/data_files/test/test_stuck_files.py

elipe17 · 2024-09-25T18:31:28Z

tdrs-backend/tdpservice/data_files/tasks.py

+        # reparse submissions past the timeout, where the reparse did not complete
+        Q(
+            reparse_count__gt=0,
+            reparse_meta_models__timeout_at__lte=datetime.now(tz=timezone.utc),


Instead of datetime can we use Django's builtin timezone? That way we can guarantee the same timezone since Django should be managing it.

elipe17 · 2024-09-25T19:06:46Z

tdrs-backend/tdpservice/data_files/test/test_stuck_files.py

+
+
+def _time_ago(hours=0, minutes=0, seconds=0):
+    return datetime.now(tz=timezone.utc) - timedelta(hours=hours, minutes=minutes, seconds=seconds)


Should use Django timezone here too if possible.

ADPennington · 2024-09-30T19:56:46Z

@jtimpe some unit tests are failing on this branch since the reparsing work was merged in.

jtimpe · 2024-10-01T20:06:48Z

@ADPennington should be fixed!

ADPennington · 2024-10-02T12:41:09Z

@jtimpe @victoriaatraft -- quick question -- was the design mockup here intended for this ticket or a follow-on?

raftmsohani · 2024-10-02T12:46:01Z

tdrs-backend/tdpservice/data_files/models.py

@@ -206,6 +207,10 @@ def submitted_by(self):
        """Return the author as a string for this data file."""
        return self.user.get_full_name()

+    def admin_link(self):
+        """Return a link to the admin console for this file."""
+        return f"{settings.FRONTEND_BASE_URL}/admin/data_files/datafile/?id={self.pk}"


you shouldn't need to hardcode the whole URL, you should either use reverse form django or reverse_actions from DRF as stated here: https://www.django-rest-framework.org/api-guide/viewsets/#reversing-action-urls

jtimpe · 2024-10-02T12:53:16Z

@jtimpe @victoriaatraft -- quick question -- was the design mockup here intended for this ticket or a follow-on?

@ADPennington it was, but per this comment i modified the template to display a list of stuck files rather than a singular.

ADPennington · 2024-10-02T12:58:29Z

@jtimpe @victoriaatraft -- quick question -- was the design mockup here intended for this ticket or a follow-on?

@ADPennington it was, but per this comment i modified the template to display a list of stuck files rather than a singular.

ahh thank you! Appreciate the reference @jtimpe

ADPennington · 2024-10-02T20:41:27Z

testing update:

received email notification with list of pending files.
ran re-parsing command in qasp environment. no more pending files, so no email notification
successfully triggered pending status via resubmitting large files in sequence. will re-deploy with new subject line tonight.

tdrs-backend/tdpservice/email/helpers/data_file.py

Co-authored-by: Alex P. <63075587+ADPennington@users.noreply.github.com>

ADPennington

looks good @jtimpe 🚀 cc: ttran-hub

testing steps here

email (final copy)

logentries

jtimpe and others added 12 commits September 18, 2024 09:33

stuck file templates

568a00a

send stuck file notif

71b16f7

improve table readability

e2388d2

handle edge cases

77af04f

lint

46d18ad

only send if files stuck

0552872

convert to celery task

cc32905

fix test

65e9a4f

Merge branch 'develop' into 2990-stuck-files-notification

d2740e4

fix test

9638a81

unused import

2184c32

Merge branch 'develop' into 2990-stuck-files-notification

24063d7

jtimpe self-assigned this Sep 23, 2024

jtimpe added the raft review This issue is ready for raft review label Sep 23, 2024

jtimpe requested review from andrew-jameson, raftmsohani and elipe17 September 23, 2024 15:46

andrew-jameson reviewed Sep 23, 2024

View reviewed changes

tdrs-backend/tdpservice/data_files/test/test_stuck_files.py Show resolved Hide resolved

jtimpe and others added 4 commits September 23, 2024 14:10

Merge branch 'develop' into 2990-stuck-files-notification

74e6ff0

rm stuck-file-single

1dec46a

insert admin link

1486a1b

task efficiency

dda1831

jtimpe requested a review from andrew-jameson September 25, 2024 12:17

elipe17 reviewed Sep 25, 2024

View reviewed changes

jtimpe added the QASP Review label Sep 27, 2024

jtimpe requested a review from ADPennington September 27, 2024 16:24

ADPennington added 3 commits September 30, 2024 11:10

Merge branch 'develop' into 2990-stuck-files-notification

d8ec28b

Merge branch 'develop' into 2990-stuck-files-notification

eb3ba5e

Merge branch 'develop' into 2990-stuck-files-notification

78364c8

ADPennington added the Blocked Label for Pull Requests that are currently blocked by a dependency label Oct 1, 2024

fix test

e1a1f55

jtimpe force-pushed the 2990-stuck-files-notification branch from 53999bb to e1a1f55 Compare October 1, 2024 19:11

jtimpe and others added 2 commits October 1, 2024 15:12

Merge branch 'develop' into 2990-stuck-files-notification

072c8f6

lint

6c8c814

ADPennington added Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI and removed Blocked Label for Pull Requests that are currently blocked by a dependency labels Oct 1, 2024

raftmsohani reviewed Oct 2, 2024

View reviewed changes

ADPennington reviewed Oct 2, 2024

View reviewed changes

tdrs-backend/tdpservice/email/helpers/data_file.py Outdated Show resolved Hide resolved

jtimpe and others added 2 commits October 2, 2024 16:52

Update tdrs-backend/tdpservice/email/helpers/data_file.py

c189e62

Co-authored-by: Alex P. <63075587+ADPennington@users.noreply.github.com>

Merge branch 'develop' into 2990-stuck-files-notification

1f6756b

ADPennington added Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Oct 2, 2024

ADPennington approved these changes Oct 2, 2024

View reviewed changes

ADPennington added Ready to Merge and removed QASP Review Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Oct 2, 2024

andrew-jameson merged commit 30513b6 into develop Oct 3, 2024
29 checks passed

andrew-jameson deleted the 2990-stuck-files-notification branch October 3, 2024 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2990 - stuck files notification #3195

2990 - stuck files notification #3195

jtimpe commented Sep 20, 2024 •

edited by ADPennington

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

andrew-jameson Sep 23, 2024

jtimpe Sep 25, 2024

andrew-jameson Sep 23, 2024

jtimpe Sep 25, 2024

andrew-jameson Sep 23, 2024

jtimpe Sep 25, 2024

andrew-jameson Sep 23, 2024

elipe17 Sep 25, 2024

elipe17 Sep 25, 2024

ADPennington commented Sep 30, 2024

jtimpe commented Oct 1, 2024

ADPennington commented Oct 2, 2024

raftmsohani Oct 2, 2024

jtimpe commented Oct 2, 2024

ADPennington commented Oct 2, 2024

ADPennington commented Oct 2, 2024 •

edited

Loading

ADPennington left a comment



		def _time_ago(hours=0, minutes=0, seconds=0):
		return datetime.now(tz=timezone.utc) - timedelta(hours=hours, minutes=minutes, seconds=seconds)

2990 - stuck files notification #3195

2990 - stuck files notification #3195

Conversation

jtimpe commented Sep 20, 2024 • edited by ADPennington Loading

Summary of Changes

How to Test

Deliverables

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADPennington commented Sep 30, 2024

jtimpe commented Oct 1, 2024

ADPennington commented Oct 2, 2024

Choose a reason for hiding this comment

jtimpe commented Oct 2, 2024

ADPennington commented Oct 2, 2024

ADPennington commented Oct 2, 2024 • edited Loading

ADPennington left a comment

Choose a reason for hiding this comment

jtimpe commented Sep 20, 2024 •

edited by ADPennington

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

ADPennington commented Oct 2, 2024 •

edited

Loading