-
Notifications
You must be signed in to change notification settings - Fork 130
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-14231: Users can't access accounts after enrollment expires #11105
LG-14231: Users can't access accounts after enrollment expires #11105
Conversation
…sProofingResultsJob
…gets locked out of account after in_person_enrollment expires
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle existing expired in-person enrollments?
Can we add a test for the identity verification view that had been raising exceptions, to include a context
case that would fail on main
for the expired enrollment?
@@ -196,6 +196,14 @@ def deactivate_due_to_gpo_expiration | |||
) | |||
end | |||
|
|||
def deactivate_due_to_ipp_expiration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that this largely follows from the equivalent GPO behavior, would we have a similar need to track the "expired at" value as is done with GPO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to see when an in_person_enrollment
was expired, we could inspect the in_person_enrollment
's status_check_completed_at
field. It's set in the GetUspsProofingResultsJob
when we expire an enrollment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for parity with other profile features we should get an ipp_enrollment_expired_at
field added to Profile
, then use the status_check_completed_at
to seed those values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(That does not need to block this PR though)
@@ -259,6 +259,12 @@ | |||
|
|||
before do | |||
enrollment_records = InPersonEnrollment.where(id: pending_enrollments.map(&:id)) | |||
# Below sets in_person_verification_pending_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be done in the factory trait for pending in-person enrollments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a ticket for refactoring the in_person_enrollments
factory, LG-14237. I agree that this isn't the ideal set-up, but due to the time-sensitive nature of the bug, this was the approach we took.
@@ -196,6 +196,14 @@ def deactivate_due_to_gpo_expiration | |||
) | |||
end | |||
|
|||
def deactivate_due_to_ipp_expiration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might recommend writing tests for this in spec/models/profile_spec.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 554c8f6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since I think it helps stem future issues, but still curious about existing ones.
🎫 Ticket
LG-14231: Expired enrollments put users in broken account state
🛠 Summary of changes
in_person_verification_pending_at
field as nil and mark the profile as inactive.Related PR: Bug fix for PR10847 - Verified pending states
📜 Testing Plan
Then, examine the profile with the below command
enrollment.profile
Confirm that