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

Jump to bottom

#91156 [10-10EZR]: Update submission failure notifications and add user initials to successful submissions #18348

Merged
merged 9 commits into from
Sep 10, 2024
Merged

#91156 [10-10EZR]: Update submission failure notifications and add user initials to successful submissions #18348

merged 9 commits into from
Sep 10, 2024

Conversation

JoshingYou1
Copy link
Contributor

@JoshingYou1 JoshingYou1 commented Sep 6, 2024 •
edited
Loading

Summary

  • This work is behind a feature toggle (flipper): NO
  • This PR adds code that allows the 10-10 team to differentiate between failures that are still retrying and exhausted failures. It also adds user initials to successful submissions in order to more easily discern when failure retries have finally succeeded.
  • I work for the 10-10 health apps team

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Prior to the change, all submission failures were considered exhausted failures. This code separates failures that are still retrying and exhausted failures.

Screenshots

Note: Optional

What areas of the site does it impact?

10-10EZR form

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 17:45 Inactive
JoshingYou1
JoshingYou1 commented Sep 6, 2024
View reviewed changes
lib/form1010_ezr/service.rb Outdated
@@ -60,24 +64,22 @@ def submit_form(parsed_form)
log_and_raise_error(e, parsed_form)
end

def log_submission_failure(parsed_form)
StatsD.increment("#{Form1010Ezr::Service::STATSD_KEY_PREFIX}.failed_wont_retry")
def log_submission_failure(parsed_form, retries_exhausted: false)
Copy link
Contributor Author

@JoshingYou1 JoshingYou1 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Adding this boolean will allow us to determine if the failure was the last retry attempt or not.

JoshingYou1
JoshingYou1 commented Sep 6, 2024
View reviewed changes
lib/form1010_ezr/service.rb Outdated
middle_initial: parsed_form.dig('veteranFullName', 'middle')&.chr || 'no initial provided',
last_initial: parsed_form.dig('veteranFullName', 'last')&.chr || 'no initial provided'
}
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRY'd up this piece of code.

@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 18:58 Inactive
@JoshingYou1 JoshingYou1 changed the title #91156 [10-10EZR]: Update submission failure notification in order to differentiate between failures and total failures #91156 [10-10EZR]: Update submission failure notifications and add user initials to successful submissions Sep 6, 2024
@JoshingYou1 JoshingYou1 marked this pull request as ready for review September 6, 2024 19:29
@JoshingYou1 JoshingYou1 requested review from a team as code owners September 6, 2024 19:29
@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 19:40 Inactive
coope93
coope93 previously requested changes Sep 6, 2024
View reviewed changes
lib/form1010_ezr/service.rb Outdated Show resolved Hide resolved
lib/form1010_ezr/service.rb Outdated Show resolved Hide resolved
lib/form1010_ezr/service.rb Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 6, 2024
View reviewed changes
lib/form1010_ezr/service.rb Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 6, 2024
View reviewed changes
lib/form1010_ezr/service.rb

if parsed_form.present?
PersonalInformationLog.create!(
data: parsed_form,

Check failure

Code scanning / CodeQL

Clear-text storage of sensitive information High

This stores sensitive data returned by
a write to veteranDateOfBirth
Loading
as clear text.
This stores sensitive data returned by
a write to veteranSocialSecurityNumber
Loading
as clear text.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please confirm this is intended and is not a security issue?

@JoshingYou1 JoshingYou1 requested a review from coope93 September 6, 2024 20:33
@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 20:37 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 22:23 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 22:30 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 91156_1010_ezr_update_failure_notifications/main/main September 6, 2024 22:36 Inactive
@platform-support-slack-integration platform-support-slack-integration bot mentioned this pull request Sep 10, 2024
Closed
@github-actions GitHub Actions
Copy link

Backend-review-group approval confirmed.

@rjohnson2011 rjohnson2011 self-requested a review September 10, 2024 17:40
@dellerbie dellerbie removed the request for review from coope93 September 10, 2024 19:33
@platform-support-slack-integration platform-support-slack-integration bot mentioned this pull request Sep 10, 2024
Closed
@JoshingYou1 JoshingYou1 merged commit b42d6b3 into master Sep 10, 2024
28 of 29 checks passed
@JoshingYou1 JoshingYou1 deleted the 91156_1010_ezr_update_failure_notifications branch September 10, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coope93 coope93 coope93 left review comments

@dellerbie dellerbie dellerbie approved these changes

@rjohnson2011 rjohnson2011 Awaiting requested review from rjohnson2011

Assignees
No one assigned
Labels
require-backend-approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JoshingYou1 @dellerbie @coope93 @rjohnson2011 @va-vfs-bot