Skip to content

LG-14749 Cancel enrollments on profile encryption error#11585

Merged
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-14749
Dec 6, 2024
Merged

LG-14749 Cancel enrollments on profile encryption error#11585
shanechesnutt-ft merged 1 commit intomainfrom
sc/LG-14749

Conversation

@shanechesnutt-ft
Copy link
Copy Markdown
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-14748

🛠 Summary of changes

Cancel in-person enrollments when a profile is deactivated with reason encryption_error.

📜 Testing Plan

Scenario: User has a pending in-person enrollment and resets their password

  • Login through the oidc sinatra application selecting the Identity Verified level of service.
  • Create a new account
  • Complete the ID-IPP flow reaching the ready to verify page.
  • Logout
  • Reset the password of the user.
  • Login through the oidc sinatra application selecting the Identity Verified level of service.
  • Ensure user is navigated to the welcome page.
  • Ensure the in-person enrollment is update to have a status of cancelled.
  • Ensure the user is able to complete the ID-IPP flow reaching the ready to verify page.

@shanechesnutt-ft shanechesnutt-ft changed the title LG-14748 Cancel enrollments on profile encryption error LG-14749 Cancel enrollments on profile encryption error Dec 3, 2024
@shanechesnutt-ft shanechesnutt-ft requested review from a team and eileen-nava December 3, 2024 22:02
Comment on lines 204 to 205
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would there be any harm in doing the canceling of the enrollment in the background job? Like when we go to check on the status of the enrollment, notice the profile is deactivated, and deactivate the enrollment. If the profile is out of sync with the enrollment in the time between when the profile is deactivated and we next check on the enrollment, is that bad?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is logic in place that handles cancelling enrollments with profiles that are deactivated in the background job. The issue with it is that users are able to attempt to schedule an enrollment before the pending enrollment is cancelled because of the 30 min / 1 hour window of the background job. If it doesn't make sense to cancel it at the point of encryption error, maybe it would be better to have a check during the ID-IPP workflow to cancel any existing pending enrollments like we do with existing establishing enrollments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The profile and in-person enrollment models are tightly coupled which is why I thought it is important that we try to keep them in sync. So when one becomes in a dead state the other should also be in a dead state.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha, that makes sense to me

@gina-yamada
Copy link
Copy Markdown
Contributor

gina-yamada commented Dec 4, 2024

Testing Notes

  • Login through the oidc sinatra application selecting the Identity Verified level of service.
  • Create a new account
  • Complete the ID-IPP flow reaching the ready to verify page.
  • Logout
  • Reset the password of the user.
  • Login through the oidc sinatra application selecting the Identity Verified level of service.
  • Ensure user is navigated to the welcome page.
  • Ensure the in-person enrollment is update to have a status of cancelled. I also checked the profile was not active and had a deactivation reason set to encryption error. (I checked on view /verify/welcome) (I did check before I signed in but no updates to profile and enrollments were made, this is as I'd expect.)
  • Ensure the user is able to complete the ID-IPP flow reaching the ready to verify page.
  • Additionally, I ran through the above steps but checked that I could successfully complete remote flow

@gina-yamada gina-yamada self-requested a review December 4, 2024 18:28
@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/LG-14749 branch 2 times, most recently from 6df2218 to 8506ef1 Compare December 5, 2024 14:18
Copy link
Copy Markdown
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.

I'm not sure what it means to approve a draft PR, but this all looks good to me!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Irrelevant mumblings: my brain always reads this as though it means be_somewhat, like be_kind_of(:confusing) or such. It obviously doesn't make any sense that that would be a matcher, though.

I was going to comment that I prefer be_a_kind_of which reads much better, but in searching the code, be_kind_of is much more common so we should stick with it. (It's kind_of(:a_bummer) though.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 for adding tests for this class!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 for adding thorough tests to this class that previously had none. 😃

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
context 'when the active profile was activated before the pending profile as created' do
context 'when the active profile was activated before the pending profile was created' do

Is that how this is supposed to read?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I will update that!

Copy link
Copy Markdown
Contributor

@matthinz matthinz left a comment

Choose a reason for hiding this comment

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

Ok, we had a little back and forth about whether or not to clear in_person_proofing_verification_at when canceling. Where we ended up was to keep the timestamp in place so that we're consistent with what we do in other cases when we cancel profiles.

This all LGTM, with the caveat I haven't actually tested it--I'll leave the IPP portions to y'all.

@shanechesnutt-ft shanechesnutt-ft marked this pull request as ready for review December 5, 2024 17:03
@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/LG-14749 branch 2 times, most recently from 4f68c6a to 054139f Compare December 6, 2024 15:17
Copy link
Copy Markdown
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

Hi, as I noted in slack here, I am not clear on the benefit of leaving the in_person_verification_pending_at timestamp populated. The main stated benefit seems to be that leaving the timestamp populated indicates what kind of profile it was before cancellation. However, that information is already available from the idv_level field.

I am still approving the PR. That being said, I’d appreciate clarity from Ada Engineering on the benefit of leaving the in_person_verification_pending_at timestamp populated. Thanks!

P.S. Shane, great work adding tests! 👏🏻

changelog: Internal, In-person Proofing, Cancel in-person enrollments when profiles are deactivated due to encryption error.
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