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.

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

Feat: system enrollment error - 500 error during /token #2089

Merged
merged 4 commits into from
May 22, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented May 13, 2024

Part of #2032

  • 500-level error that occurs during /token returns a JSON response that is used to send user to /enrollment/error
  • /enrollment/error shows system enrollment error template
  • Sentry notification is sent
  • Analytic event is sent (failed access token request with status_code in event_properties)

Reviewing / testing

Similar to review steps for #2088, you can add some code locally to simulate a 500 error.
To /token, add:

        try:
+          class response:
+             def __init__(self):
+                self.status_code = 500
+
+          raise HTTPError(response=response())
           response = client.request_card_tokenization_access()
        except Exception as e:

Copy link

github-actions bot commented May 13, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  benefits/enrollment
  analytics.py 45
  views.py 59
Project Total  

This report was generated by python-coverage-comment-action

@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. and removed tests Related to automated testing (unit, UI, integration, etc.) labels May 13, 2024
@angela-tran angela-tran changed the title refactor: show system enrollment error page if 500 during /token Chore: system enrollment error - 500 error during /token May 13, 2024
@angela-tran angela-tran changed the title Chore: system enrollment error - 500 error during /token Feat: system enrollment error - 500 error during /token May 14, 2024
@thekaveman thekaveman force-pushed the chore/system-enrollment-error--500-link branch 3 times, most recently from fa77a8e to 95ec106 Compare May 20, 2024 21:59
Base automatically changed from chore/system-enrollment-error--500-link to dev May 20, 2024 22:03
@thekaveman thekaveman force-pushed the chore/system-enrollment-error--500-token branch from 4b7bb60 to 17de9a1 Compare May 20, 2024 22:06
@thekaveman
Copy link
Member

Rebased on dev to incorporate changes from #2088

@angela-tran angela-tran force-pushed the chore/system-enrollment-error--500-token branch 3 times, most recently from 0077b6a to 384910a Compare May 21, 2024 22:37
@angela-tran angela-tran force-pushed the chore/system-enrollment-error--500-token branch from 384910a to 3769a00 Compare May 21, 2024 22:41
@angela-tran angela-tran marked this pull request as ready for review May 21, 2024 22:49
@angela-tran angela-tran requested a review from a team as a code owner May 21, 2024 22:49
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

This looks great. I made the suggested modification in the view and was sent to the expected error page when trying to acquire the access token.

if (data.redirect) {
// https://stackoverflow.com/a/42469170
// use 'assign' because 'replace' was giving strange Back button behavior
window.location.assign(data.redirect);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not worried about this as blocking this PR, but...

I wonder if window.location.replace is the right method here instead? Do we want to support the back button when the user is supposed to wait and not try again immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #2119 to follow up on this

@angela-tran angela-tran merged commit c238834 into dev May 22, 2024
13 checks passed
@angela-tran angela-tran deleted the chore/system-enrollment-error--500-token branch May 22, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-end Django views, sessions, middleware, models, migrations etc. front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants