-
Notifications
You must be signed in to change notification settings - Fork 14
Fyst 1517 add ssn itin validation step to prior year access flow #5326
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
Fyst 1517 add ssn itin validation step to prior year access flow #5326
Conversation
Co-authored-by: Drew Proebstel <[email protected]>
…" (#5317) This reverts commit f1e5246. Co-authored-by: Drew Proebstel <[email protected]>
… into FYST-1516-add-email-validation-step-to-prior-year-access-flow
* Add last year verification email Co-authored-by: Em Barnard-Shao <[email protected]> Co-authored-by: Hugo Melo <[email protected]> * Rename previous year to archived intake Co-authored-by: Hugo Melo <[email protected]> * normalize Co-authored-by: Hugo Melo <[email protected]> --------- Co-authored-by: Em Barnard-Shao <[email protected]>
…il-validation-step-to-prior-year-access-flow
Heroku app: https://gyr-review-app-5326-483eb329f5d0.herokuapp.com/ |
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Tahsina Islam <[email protected]>
…n-validation-step-to-prior-year-access-flow Co-authored-by: Tahsina Islam <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
…ior-year-access-flow Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Tahsina Islam <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
…ior-year-access-flow Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
Co-authored-by: Anisha Ramnani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One hunch of a potential bug in the interaction between this story and the previous one that I'd like you to check out, and one request for changes to comments, but looks good other than that! LMK when you've gotten a chance to check out the potential interaction with failed verification code attempts & I'll approve.
# need to change to address controller | ||
else | ||
create_state_file_access_log("incorrect_ssn_challenge") | ||
current_request.increment_failed_attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're re-using the same mechanism for marking "failed attempts" here as on the verification code page. If the user mis-enters a verification code and then gets it right on their second try, will that count against their total allowed failed attempts? In other words, should we reset failed attempts to zero after a user succeeds verification code authorization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I thought we did this in both controllers when the form is valid by line 17: current_request.reset_failed_attempts! but i could be wrong would be down to discuss on tuesday!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right, I missed that! Thanks for pointing it out - looks good!
expect(intake_request.reload.failed_attempts).to eq(0) | ||
|
||
expect(response).to redirect_to(root_path) | ||
# need to change to address path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace these comments with a TODO that reference the story ID where the work to change the path will be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! good call out, i also left a note on the ticket referencing the todos.
Co-authored-by: Tahsina Islam <[email protected]>
# need to change to address controller | ||
else | ||
create_state_file_access_log("incorrect_ssn_challenge") | ||
current_request.increment_failed_attempts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right, I missed that! Thanks for pointing it out - looks good!
…ior-year-access-flow Co-authored-by: Anisha Ramnani <[email protected]>
…n-validation-step-to-prior-year-access-flow Co-authored-by: Tahsina Islam <[email protected]>
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
What was done?
How to test?
Screenshots (for visual changes)