Skip to content

LG-9439: Verify info controller refactor#8356

Merged
tomas-nava merged 6 commits intomainfrom
tomas/lg-9439-verify-info-controller-refactor
May 8, 2023
Merged

LG-9439: Verify info controller refactor#8356
tomas-nava merged 6 commits intomainfrom
tomas/lg-9439-verify-info-controller-refactor

Conversation

@tomas-nava
Copy link
Contributor

🎫 Ticket

LG-9439

🛠 Summary of changes

Moves methods shared by the two verify info controllers into the shared VerifyInfoConcern, refactors the InPerson::VerifyInfoController to use the IdvStepConcern implementation of confirm_verify_info_step_needed.

@tomas-nava tomas-nava requested review from a team, allthesignals and soniaconnolly May 8, 2023 14:45
Copy link
Contributor

@svalexander svalexander left a comment

Choose a reason for hiding this comment

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

LGTM


private

def confirm_ssn_step_complete
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been putting these methods that check flow state into IdvStepConcern. I think that would be a better place for confirm_ssn_step_complete.

It's more debatable for delete_pii, but we have pii_from_doc in there, so it could move too. We're eventually going to consolidate the flow_session code and use idv_session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I moved confirm_ssn_step_complete into IdvStepConcern in 47914a6

I think I'll leave delete_pii where it is, because it's only used by move_applicant_to_idv_session, also in the Idv::VerifyInfoConcern.

and make it and all the other 'confirm' before methods private
@tomas-nava tomas-nava merged commit 804fb3c into main May 8, 2023
@tomas-nava tomas-nava deleted the tomas/lg-9439-verify-info-controller-refactor branch May 8, 2023 19:18
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