Skip to content

LG-12172 implement tmx rejection screen and refactor please call code#10119

Merged
JackRyan1989 merged 4 commits intomainfrom
jryan/lg-12172-implement-tmx-rejection-screen
Feb 23, 2024
Merged

LG-12172 implement tmx rejection screen and refactor please call code#10119
JackRyan1989 merged 4 commits intomainfrom
jryan/lg-12172-implement-tmx-rejection-screen

Conversation

@JackRyan1989
Copy link
Contributor

@JackRyan1989 JackRyan1989 commented Feb 20, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12172

🛠 Summary of changes

This simplifies and extends the logic used to implement LG-11995. The reason I could simplify is that we're targeting both reject and review methods instead of just one. So there's no need to isolate as much code.

📜 Testing Plan

  • Fire up the local server
  • Create a new user - make sure to hang on to the mock login info you use to create the account
  • Proceed through the IPP flow
  • On SSN page use the drop down to select Reject
  • Proceed through the rest of the flow to the barcode page to create the enrollment
  • Code comment out enrollment_outcomes[:enrollments_passed] += 1 from app/jobs/get_usps_proofing_results_job.rb
  • Open the rails console: rails c
  • Save the latest enrollment you just created: e = InPersonEnrollment.last
  • Create a new mock JSON blob: json = UspsInPersonProofing::Mock::Fixtures.request_passed_proofing_results_response
  • Save that response as a response: res = JSON.parse(json)
  • Update the user's profile: e.profile.update!(fraud_pending_reason: 2)
  • Initiate job: GetUspsProofingResultsJob.new.send(:process_enrollment_response, e, res)
  • Update the user: User.last.pending_profile.reject_for_fraud(notify_user: false)
  • Reload the enrollment: e.reload
  • Login again, and attempt to navigate past the /account page
  • You should end up on the /not_verified page

Test with a failed enrollment:

  • Fire up the local server
  • Create a new user - make sure to hang on to the mock login info you use to create the account
  • Proceed through the IPP flow
  • On SSN page use the drop down to select Reject
  • Proceed through the rest of the flow to the barcode page to create the enrollment
  • Code comment out enrollment_outcomes[:enrollments_failed] += 1 from app/jobs/get_usps_proofing_results_job.rb
  • Open the rails console: rails c
  • Save the latest enrollment you just created: e = InPersonEnrollment.last
  • Create a new mock JSON blob: json = UspsInPersonProofing::Mock::Fixtures.request_failed_proofing_results_response
  • Save that response as a response: res = JSON.parse(json)
  • Update the user's profile: e.profile.update!(fraud_pending_reason: 2)
  • Initiate job: GetUspsProofingResultsJob.new.send(:process_enrollment_response, e, res)
  • Update the user: User.last.pending_profile.reject_for_fraud(notify_user: false)
  • Reload the enrollment: e.reload
  • Login again, and attempt to navigate past the /account page
  • You should not hit the not verified page with a failed enrollment

Screenshots:

Completed design: Screenshot 2024-02-22 at 1 44 00 PM Screenshot 2024-02-22 at 1 44 12 PM Screenshot 2024-02-22 at 1 44 22 PM

@JackRyan1989 JackRyan1989 requested review from a team and gina-yamada February 20, 2024 21:46
@JackRyan1989 JackRyan1989 self-assigned this Feb 21, 2024
@gina-yamada
Copy link
Contributor

gina-yamada commented Feb 21, 2024

✅ Code changes look good and includes testing
✅ I tested with in_person_proofing_enforce_tmx set to true in config/application.yml, went into the flow via Sinatra, and followed the testing plans above. I observed the expected behavior as stated in testing plan for both flows/situations
✅ The prevent fraud redirection is only being used when in_person_proofing_enforce_tmx is true so it is behind the flag so should not affect existing/current behavior
✅ Looked at Spanish and French views -- see screenshots below
❌ Users who select the "Get help at " link must be directed to the failure_to_proof_url link. I was taken to http://localhost:9292/?error=access_denied- this might be because we are going from 3000 to 9292?
❌ I don't see this button on the page- AC: Users who select the "Exit Login.gov" button must be directed to the return_to_sp_url link. -- see screenshots below

Update: I don't see the button on not_verified html. (I didn't dig to see if this is a partial view and it could be somewhere else.) I feel like this ticket was expecting that button to already be on that template. I think if more work is needed, we can remove that from the AC and create a new ticket to handle the Exit button. I also wonder if it should be added to the template for all or if it needs to be conditionally rendered?

Testing locally- I don't see the button
Screenshot 2024-02-21 at 1 45 24 PM
Here is the mock image
Screenshot 2024-02-21 at 1 50 05 PM

Translations

Screenshot 2024-02-22 at 9 21 55 AM

Screenshot 2024-02-22 at 9 21 17 AM

@JackRyan1989 JackRyan1989 requested review from a team and rutvigupta-design and removed request for a team February 22, 2024 21:56
heading: t('idv.failure.verify.heading'),
action: { text: t('idv.failure.verify.exit', app_name: APP_NAME), url: :return_to_sp_failure_to_proof, method: :get },
) do %>
<p>
Copy link
Contributor

@gina-yamada gina-yamada Feb 23, 2024

Choose a reason for hiding this comment

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

@JackRyan1989 I now observe the Exit button on the page.

Screenshot 2024-02-23 at 8 16 28 AM

Screenshot 2024-02-23 at 8 37 15 AM

Screenshot 2024-02-23 at 8 37 25 AM

When I click the Exit button, I am taken to the Sinatra app but it reads that I have not signed out.

Screenshot 2024-02-23 at 8 23 35 AM

On Sinatra, if you click the Log out button (see the screenshot above) you are taken to the screen below.

Screenshot 2024-02-23 at 8 23 46 AM

The Yes, sign out of Login.gov button appears to sign the user out and redirect you back to Sinatra. What do you think about keeping that same behavior (sign the user out and redirect) for the button? Or should the user be directed to this confirmation page from your button? Do either sound reasonable? I know the mocks are a little different but maybe best practice to sign the user out as well. Consider soliciting others opinions before implementing in case we don't want to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gina-yamada thanks for confirming! So I'm going by the note in the Figma file which states to use the return_to_sp link.

I found this route in the routes.rb file and am taking advantage of it here:

get '/redirect/return_to_sp/failure_to_proof' => 'redirect/return_to_sp#failure_to_proof',
        as: :return_to_sp_failure_to_proof

That's line 315 of the routes.rb file.

@rutvigupta-design
Copy link

LGTM based on our conversation in Slack!

Copy link

@rutvigupta-design rutvigupta-design left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

Nice! I feel like the scope on this grew, and yet you implemented it with relatively few lines of code.

I don't have a good answer for where the "Exit" button should go. I think Gina raises a good point about how it would be reasonable to log people out, but it sounds like your implementation is based on what was proposed in the Figma design. I'm approving this PR because I think either approach is reasonable.

@@ -0,0 +1,52 @@
require 'rails_helper'

RSpec.describe 'idv/not_verified/show.html.erb' do
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 for adding a test for this.

@gina-yamada gina-yamada self-requested a review February 23, 2024 20:47
@JackRyan1989 JackRyan1989 merged commit 4ee767d into main Feb 23, 2024
@JackRyan1989 JackRyan1989 deleted the jryan/lg-12172-implement-tmx-rejection-screen branch February 23, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants