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

LG-13692 Use PendingProfileConcern to check for pending profiles during IdV #10867

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

jmhooper
Copy link
Member

The PendingProfileConcern uses the PendingProfilePolicy to check if a user has a pending profile and needs to be redirected to finish proofing. We have code in the IdvStepConcern that does the same, but does not factor all of the things that go into that function, such as whether the SP requested a biometric comparison and the pending profile had a biometric comparison.

This commit uses the PendingProfileConcern in the IdvStepConcern to ensure the pending profile logic is consistent.

@jmhooper jmhooper force-pushed the jmhooper-use-pending-profile-concern-in-idv branch from 6dcd40e to 08a5542 Compare July 1, 2024 13:55
@jmhooper jmhooper changed the base branch from main to jmhooper-remove-resend-from-the-request-letter-controller July 1, 2024 13:56
@jmhooper jmhooper marked this pull request as ready for review July 1, 2024 14:19
@jmhooper jmhooper requested a review from a team July 1, 2024 14:19
Base automatically changed from jmhooper-remove-resend-from-the-request-letter-controller to main July 1, 2024 14:58
…ring IdV

The `PendingProfileConcern` uses the `PendingProfilePolicy` to check if a user has a pending profile and needs to be redirected to finish proofing. We have code in the `IdvStepConcern` that does the same, but does not factor all of the things that go into that function, such as whether the SP requested a biometric comparison and the pending profile had a biometric comparison.

This commit uses the `PendingProfileConcern` in the `IdvStepConcern` to ensure the pending profile logic is consistent.

[skip changelog]
@jmhooper jmhooper force-pushed the jmhooper-use-pending-profile-concern-in-idv branch from 08a5542 to 561fb82 Compare July 1, 2024 14:59
return idv_please_call_url if current_user.fraud_review_pending?
idv_not_verified_url if current_user.fraud_rejection?
end

def user_has_pending_profile?
pending_profile_policy.user_has_pending_profile?
pending_profile_policy.user_has_pending_profile? && !user_failed_ipp_with_fraud_review_pending?
Copy link
Member

Choose a reason for hiding this comment

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

Should this !user_failed_ipp_with_fraud_review_pending be part of PendingProfilePolicy?

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 had some back and forth on this. It is a little tough to track exactly what it does. It appears that if a user failed IPP with fraud review pending then their pending_profile does not count. That implementation is, perhaps, questionable but I think that is a bigger change than this.

Copy link
Member

Choose a reason for hiding this comment

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

It feels like if we know they failed IPP we should cancel the profile

@matthinz
Copy link
Member

matthinz commented Jul 2, 2024

LGTM, pending adding back the feature spec showing what this fixes

expect(current_path).to eq(idv_verify_by_mail_enter_code_path)

# Cancelling redirects to IdV flow start
click_on t('idv.gpo.address_accordion.cta_link')
Copy link
Member

Choose a reason for hiding this comment

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

This is wholly irrelevant to your PR, but just looking at this translation string I would never guess it read "Clear your information and start over". I would expect a CTA to be enticing the user to start some action, not cancel and start over.

@jmhooper jmhooper merged commit b42e45b into main Jul 2, 2024
2 checks passed
@jmhooper jmhooper deleted the jmhooper-use-pending-profile-concern-in-idv branch July 2, 2024 18:26
WilliamBirdsall pushed a commit that referenced this pull request Jul 2, 2024
…ring IdV (#10867)

The `PendingProfileConcern` uses the `PendingProfilePolicy` to check if a user has a pending profile and needs to be redirected to finish proofing. We have code in the `IdvStepConcern` that does the same, but does not factor all of the things that go into that function, such as whether the SP requested a biometric comparison and the pending profile had a biometric comparison.

This commit uses the `PendingProfileConcern` in the `IdvStepConcern` to ensure the pending profile logic is consistent.

[skip changelog]
WilliamBirdsall pushed a commit that referenced this pull request Jul 2, 2024
…ring IdV (#10867)

The `PendingProfileConcern` uses the `PendingProfilePolicy` to check if a user has a pending profile and needs to be redirected to finish proofing. We have code in the `IdvStepConcern` that does the same, but does not factor all of the things that go into that function, such as whether the SP requested a biometric comparison and the pending profile had a biometric comparison.

This commit uses the `PendingProfileConcern` in the `IdvStepConcern` to ensure the pending profile logic is consistent.

[skip changelog]
@@ -14,9 +15,7 @@ module IdvStepConcern
before_action :confirm_personal_key_acknowledged_if_needed
before_action :confirm_idv_needed
before_action :confirm_letter_recently_enqueued
before_action :confirm_no_pending_gpo_profile
before_action :confirm_no_pending_in_person_enrollment
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jmhooper, for future PRs that affect in_person_enrollments, could you please tag @identity-joy-engineers for a review? It would help us track potential changes to our flow.

If you’re concerned about turnaround time, you can also post the PR in the #login-team-joy-eng channel and tag the @login-joy-engineers group. Thanks!

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