Skip to content

Add ensure_user_stays_in_vendor_bucket to all the controllers.#11425

Merged
jmax-gsa merged 6 commits intomainfrom
jmax/LG-14658-redirect-lost-user
Nov 7, 2024
Merged

Add ensure_user_stays_in_vendor_bucket to all the controllers.#11425
jmax-gsa merged 6 commits intomainfrom
jmax/LG-14658-redirect-lost-user

Conversation

@jmax-gsa
Copy link
Contributor

@jmax-gsa jmax-gsa commented Oct 30, 2024

🎫 Ticket

Link to the relevant ticket:
LG-14658

🛠 Summary of changes

Added a before action to all of the document capture controllers to make sure the user stays in their assigned vendor bucket. i.e., if we send them to Socure, they can't use LN by manually entering the Socure document capture URL, and vice-versa.

📜 Testing Plan

** IN PROGRESS **

Bring up the app running locally, with access from your phone via WiFi
HOST=0.0.0.0 make run

  • Set your application.yml file to use Lexis Nexis (see below). Begin IdV on your computer, and select hybrid handoff. When you reach the document capture page, verify that the page you are on is /idv/hybrid_mobile/document_capture
  • Manually enter the path for Socure document capture /idv/hybrid_mobile/socure/document_capture. Verify that you are redirected back to the same page as on the previous step.
  • Cancel IdV.
  • Begin IdV on your phone (i.e. do not use hybrid handoff). When you reach the document capture page, verify that the page you are on is /idv/document_capture
  • Manually enter the page for Socure document capture /idv/socure/document_capture. Verify that you are redirected back to the same page as on the previous step
  • Cancel IdV
  • Set your application.yml file to use Socure (see below). Begin IdV on your computer, and select hybrid handoff. When you reach the document capture page, verify that the page you are on is /idv/hybrid_mobile/socure/document_capture
  • Manually enter the path for Lexis Nexis document capture /idv/hybrid_mobile/document_capture. Verify that you are redirected back to the same page as on the previous step.
  • Cancel IdV.
  • Begin IdV on your phone (i.e. do not use hybrid handoff). When you reach the document capture page, verify that the page you are on is /idv/socure/document_capture
  • Manually enter the page for Lexis Nexis document capture /idv/document_capture. Verify that you are redirected back to the same page as above.
  • Cancel IdV

The two relevant application.yml values to use LN are:

doc_auth_vendor: 'lexis_nexis'
doc_auth_vendor_default: 'lexis_nexis'

The two relevant application.yml values to use Socure are:

doc_auth_vendor: 'lexis_socure'
doc_auth_vendor_default: 'lexis_socure'

Copy link
Contributor

@mitchellhenke mitchellhenke Oct 30, 2024

Choose a reason for hiding this comment

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

This seems like it could be kind of brittle, can we do a comparison on the expected doc_auth_vendor and the calculated doc auth vendor instead closer to the controller actions and use the context there to pass in the relevant variables?

before_action ->{ redirect_to_correct_vendor(Idp::Constants::Vendors::SOCURE, true) }, only: [:relevant_action]
before_action ->{ redirect_to_correct_vendor(Idp::Constants::Vendors::SOCURE, false) }, only: [:other_relevant_action]
before_action ->{ redirect_to_correct_vendor(Idp::Constants::Vendors::LEXIS_NEXIS, true) }, only: [:relevant_action]
before_action ->{ redirect_to_correct_vendor(Idp::Constants::Vendors::LEXIS_NEXIS, false) }, only: [:other_relevant_action]

# concern.rb
def redirect_to_correct_vendor(vendor, in_hybrid_mobile)
      expected_doc_auth_vendor = doc_auth_vendor
      return if vendor == expected_doc_auth_vendor
      path = case expected_doc_auth_vendor
      when Idp::Constants::Vendors::SOCURE
        in_hybrid_mobile ? idv_hybrid_mobile_socure_document_capture_path
                         : idv_socure_document_capture_path
      when Idp::Constants::Vendors::LEXIS_NEXIS, Idp::Constants::Vendors::MOCK
        in_hybrid_mobile ? idv_hybrid_mobile_document_capture_path
                         : idv_document_capture_path
      end
   redirect_to path
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.

Better. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving the actions clauses out, because based on conversation with @amirbey when refining this, he really does mean it to check all the actions.

Copy link
Contributor

@mitchellhenke mitchellhenke Oct 30, 2024

Choose a reason for hiding this comment

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

In reading through the controllers, there's only one display action it seems? Is there value in redirecting the update request that should never happen? Should that respond with an error?

One of the controllers already has the following, which might conflict?

      case doc_auth_vendor
      when Idp::Constants::Vendors::SOCURE
        redirect_to idv_socure_document_capture_url
      when Idp::Constants::Vendors::LEXIS_NEXIS, Idp::Constants::Vendors::MOCK
        render :show, locals: extra_view_variables
      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.

All of these are "should never happen" conditions; I don't see that any one has priority over the others.

Copy link
Contributor Author

@jmax-gsa jmax-gsa Oct 30, 2024

Choose a reason for hiding this comment

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

That code was the result of addressing similar concerns during the initial Socure implementation. Removed, as it is now redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of these are "should never happen" conditions; I don't see that any one has priority over the others.

Ah, with the understanding that this is more about handling circumvention, is there a way to add these checks to step_info so that confirm_step_allowed can handle it automatically?

Copy link
Contributor

Choose a reason for hiding this comment

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

(apologies, unresolved this thread following the change in my understanding)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Will look into. Implemented your suggestion above; it took a little tweaking to allow for the mock proofer.
But putting it in confirm_step_allowed is an interesting thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hit the timebox for doing this with the step_info and confirm_step_allowed approach. It's a good idea, and we have a ticket for it: LG-14990

@jmax-gsa jmax-gsa force-pushed the jmax/LG-14658-redirect-lost-user branch from 78e1927 to 825944d Compare October 31, 2024 17:01
@jmax-gsa jmax-gsa force-pushed the jmax/LG-14658-redirect-lost-user branch from 825944d to 0b16314 Compare November 1, 2024 14:11
@jmax-gsa jmax-gsa marked this pull request as ready for review November 6, 2024 15:49
@jmax-gsa jmax-gsa force-pushed the jmax/LG-14658-redirect-lost-user branch from 6bc36d5 to 0b16314 Compare November 6, 2024 17:09
Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

Works as expected ... LGTM 👍🏿

one small request in comment section 😄

Comment on lines +51 to +52
visit idv_hybrid_mobile_socure_document_capture_url
expect(page).to have_current_path(idv_hybrid_mobile_document_capture_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this quick test to the other document capture feature tests (standard, standard socure, and hybrid socure)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to the standard flow feature spec. Will bring up Socure feature specs as a 16th minute today.

@jmax-gsa jmax-gsa merged commit 4d2e903 into main Nov 7, 2024
@jmax-gsa jmax-gsa deleted the jmax/LG-14658-redirect-lost-user branch November 7, 2024 17:24
@aduth aduth mentioned this pull request Nov 12, 2024
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