Skip to content

LG-7167: Verify establishing enrollment for in-person flow#6703

Merged
aduth merged 6 commits intomainfrom
aduth-verify-ipp-enrollment
Aug 9, 2022
Merged

LG-7167: Verify establishing enrollment for in-person flow#6703
aduth merged 6 commits intomainfrom
aduth-verify-ipp-enrollment

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Aug 8, 2022

Why: Since a user will need to select a location as part of establishing an enrollment before proceeding with the in-person flow.

aduth added 5 commits August 8, 2022 13:04
**Why**: Since a user will need to select a location as part of establishing an enrollment before proceeding with the in-person flow.

changelog: Upcoming Features, In-person proofing, Verify establishing enrollment for in-person flow
Avoid ambiguity between the two implementations
Seems to cause issues with replacing an existing session values, leading to session being empty at points-in-time where it's not expected to be, purely by referencing idv_session (e.g. in DocAuthController#redirect_if_session_applicant)
Copy link
Contributor

@sheldon-b sheldon-b left a comment

Choose a reason for hiding this comment

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

This all makes sense to me!

Not blocking for this PR but I think that the base steps (verify base and doc auth base) and the idv session files are pretty challenging to grok as a newcomer. They have layers of checks and redirects and tweaks to the session that are hard to follow or understand without tracking down why they were added. I don't have any particular suggestions right now (other than to stop inheriting from doc auth base classes in the in-person flow) but it'd be nice to try and improve them after the pilot

Copy link
Contributor

@tomas-nava tomas-nava left a comment

Choose a reason for hiding this comment

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

approve!

I'll echo Sheldon's comments on the opacity of the logic here, but definitely something to be addressed in future PRs.

@aduth
Copy link
Contributor Author

aduth commented Aug 9, 2022

Yeah, I agree. Some of that also comes back to the desired goals of FSMv2, which included the ability to implement steps as simple controllers, rather than extending step classes, or mixing in FSM concerns.

@aduth aduth merged commit ac46304 into main Aug 9, 2022
@aduth aduth deleted the aduth-verify-ipp-enrollment branch August 9, 2022 12:19
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.

3 participants