Skip to content

LG-8770 TMX rejected users page#8055

Merged
theabrad merged 16 commits intomainfrom
abrad-lg-8770-tmx-reject-page
Mar 29, 2023
Merged

LG-8770 TMX rejected users page#8055
theabrad merged 16 commits intomainfrom
abrad-lg-8770-tmx-reject-page

Conversation

@theabrad
Copy link
Contributor

@theabrad theabrad commented Mar 22, 2023

🎫 Ticket

LG-8770

🛠 Summary of changes

Created a new page for users who are fraud rejected.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Create a user and go through IdV and select 'reject' on SSN screen.
  • Get the user uuid
  • Run rake users:review:reject
  • Log in through a service provider.
  • Check to see if you are redirected to the verify_errors page.

👀 Screenshots

Screenshot 2023-03-22 at 4 47 59 PM

@theabrad theabrad requested a review from a team March 22, 2023 20:49
redirect_to_fraud_review if fraud_review_pending?
end

def handle_fraud_rejection
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be automatically wired up as a before_action if the concern is included:

included do
  before_action :handle_fraud_rejection
end

Taking a quick look over where this concern is included, I don't see a downside? We're having a proliferation of controllers under Idv and it'd be nice for them all to be able to easily redirect users due to being rejected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Idv::SetupErrorsController should get this before_action as well--if you refresh the page after being rejected, it should catch that and redirect you to the new messaging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true. yeah that will fix up some DRY.

Comment on lines +8 to +21
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: decorated_session.sp_name),
return_to_sp_failure_to_proof_path(
step: 'verify_info',
location: request.params[:action],
),
class: 'usa-link--external',
) %>
<% else %>
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: APP_NAME),
account_path,
class: 'usa-link--external',
) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically only use external links in combination with opening a new tab, which we're not doing here. I think a case could be made to include it here, since it is technically an external site, though we also link back to the SP in many other places without it. The second link here is definitely not external though.

For the first link, I'd suggest either (a) removing the external styling since it opens in the same tab or (b) make the link open in a new tab.

Suggested change
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: decorated_session.sp_name),
return_to_sp_failure_to_proof_path(
step: 'verify_info',
location: request.params[:action],
),
class: 'usa-link--external',
) %>
<% else %>
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: APP_NAME),
account_path,
class: 'usa-link--external',
) %>
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: decorated_session.sp_name),
return_to_sp_failure_to_proof_path(
step: 'verify_info',
location: request.params[:action],
),
) %>
<% else %>
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: APP_NAME),
account_path,
) %>

Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

Tried it out, saw the right views. Noted that the rake task gave me an error, but that's not directly in this PR. Added a couple of code comments.

<p>
<% if decorated_session.sp_name.present? %>
<%= link_to(
t('idv.failure.verify.fail_link_html', sp_name: decorated_session.sp_name),
Copy link
Contributor

@soniaconnolly soniaconnolly Mar 23, 2023

Choose a reason for hiding this comment

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

The text says this is a help link, but it returns to the same failure to proof path as the Exit Login.gov button. I see idv.troubleshooting.options.get_help_at_sp in other places, maybe that's the right link?

Actually, looks like that's the usual text and we always send people to the "failure to proof" link to "get help at their sp"? That seems like bait & switch! I thought we would have an sp help page link there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could bring this up with UX and ask

Choose a reason for hiding this comment

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

@theabrad - I like the idea of talking w/ UX about what we might want to accomplish here, I myself am learning a lot about these URLs/links in some of the work around "rate limit" screens!

@@ -0,0 +1,9 @@
module Idv
class VerifyErrorsController < ApplicationController
before_action :confirm_two_factor_authenticated
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would make sense to combine this controller and setup_errors_controller, since they're both really small and show related views, like the session_errors_controller that shows several different views.

Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

LGTM. I like how the not_verified controller name reads!

@theabrad theabrad merged commit bec1873 into main Mar 29, 2023
@theabrad theabrad deleted the abrad-lg-8770-tmx-reject-page branch March 29, 2023 16:03
@aduth aduth mentioned this pull request Mar 29, 2023
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.

5 participants