LG-14273: Ensure the in-person verification profile has an enrollment record

[h-m-m]

🎫 Ticket

LG-14273: Profile factory in_person_verification_pending does not create InPersonEnrollment

🛠 Summary of changes

In theory, it should never happen that the profile is pending verification and the enrollment record does not exist. A previous discussion in a meeting that I missed brought up the point that the factory does not address this properly.

This is essentially pulling @aduth's work out of #11129 , one of the smallest pieces of that PR that is still useful to the development team on its own. In fact, in pairing with @eileen-nava on this, I basically retraced much of what @aduth had done before @eileen-nava pointed out the repetition.

We looked at pulling in more from #11129 , and doing so looked like it might double the level of effort

List of changes by factory:

• Update pending trait in the in_person_enrollment factory.
• Add the in_person_verified trait to the profile factory.
• Modify the in_person_verification_pending trait in the profile factory to use the idv_level in_person.
• Refactor the with_pending_in_person_enrollment trait in the user factory.
• Refactor the proofed_in_person_enrollment trait in the user factory to use the new profile trait, in_person_verified.

📜 Testing Plan

At @eileen-nava's suggestion when we were working together, we walked through inserting a binding.pry in a test that uses this factory and made sure that we got appropriate results back. I'm grateful to Eileen for walking me through a complicated process that I didn't understand and pointing out which details should be checked. For example:

From: /Users/hmiller/workspace/identity-idp/spec/models/profile_spec.rb:1053 :

    1048:   describe '#deactivate_due_to_in_person_verification_cancelled' do
    1049:     let(:profile) { create(:profile, :in_person_verification_pending) }
    1050:     context 'when the profile does not have a deactivation reason' do
    1051:       it 'updates the profile and sets the deactivation reason to "verification_cancelled"' do
    1052:         binding.pry
 => 1053:         expect(profile.deactivation_reason).to be_nil

[1] pry(#<RSpec::ExampleGroups::Profile::DeactivateDueToInPersonVerificationCancelled::WhenTheProfileDoesNotHaveADeactivationReason>)> profile.user.has_establishing_in_person_enrollment?
=> false
[2] pry(#<RSpec::ExampleGroups::Profile::DeactivateDueToInPersonVerificationCancelled::WhenTheProfileDoesNotHaveADeactivationReason>)> profile.in_person_verification_pending?
=> true
[3] pry(#<RSpec::ExampleGroups::Profile::DeactivateDueToInPersonVerificationCancelled::WhenTheProfileDoesNotHaveADeactivationReason>)> profile.user.has_in_person_enrollment?
=> true

as well as checking other relational data.

In addition, our test suite is thorough enough that I have high confidence if the full test suite passes with this change. While making only some of these changes, many tests failed loudly.

[zachmargolis]

Hi there, looks like this PR hasn't seen any updates in a few months. Is it still needed? Or if not, can we close it out?

[eileen-nava]

Hi @zachmargolis, thanks for following up. I was working on this today. I had planned on working on it more during RSpec League meetings, but the group hasn't met as much since we started this PR. My goal is to have a PR in the review process by the end of the week. If I don't make that goal, I am fine with closing this PR.

[eileen-nava]

Hi @zachmargolis, thanks again for following up. This PR is now ready for review.

[zachmargolis]

it would be nice if we could do this as a build() instead of a create() but I'm guessing we rely on DB associations and scopes for these, so easier to just always create

[eileen-nava]

Could you say more about the reason for preferring build() here? Is it because you’d expect build() to be faster than create()? Or is it about wanting to avoid calling create() inside of an after build hook?

[zachmargolis]

1. yes, build() should be faster than create() because build is in-memory only
2. the other reason is that after_build means that if an upstream caller of this constructor only builds, intending to be in-memory only, now by calling create this will hit the DB, which may be unexpected and unintended

[eileen-nava]

I tried making the suggested change, but it broke a spec in the delete account controller spec. I am going to merge the PR as is.

[n1zyy]

I agree with Zach on a formatting nit, but this is a nice cleanup.

[n1zyy]

Did this just not exist previously? 😮

[eileen-nava]

It did not.