Skip to content

LG-9515: Refactor capture_secondary_id_enabled#8344

Merged
night-jellyfish merged 2 commits intomainfrom
brittany/lg-9515-refactor-capture-secondary-id-enabled
May 8, 2023
Merged

LG-9515: Refactor capture_secondary_id_enabled#8344
night-jellyfish merged 2 commits intomainfrom
brittany/lg-9515-refactor-capture-secondary-id-enabled

Conversation

@night-jellyfish
Copy link
Contributor

@night-jellyfish night-jellyfish commented May 4, 2023

🎫 Ticket

LG-9515

🛠 Summary of changes

Refactored the capture_secondary_id_enabled method to be defined once

Because these classes all inherit from DocAuthBaseStep and define the
method in the exact same way, we can DRY it up by removing the method
from the descendant classes.

📜 Testing Plan

with capture_secondary_id_enabled

  • enable in_person_capture_secondary_id_enabled
  • follow the flow to the State ID step
  • note that the radio button for same address as id is on the State ID page, as is the initial address entry
  • follow through to the verify page
  • expect that underneath the Verify your information heading, you see subheadings of State-issued ID, Residential address, and Social Security Number

without capture_secondary_id_enabled

  • disable in_person_capture_secondary_id_enabled
  • follow the flow past the State ID step to the address step
  • note that the radio button for same address as id is on the Address page
  • follow through to the verify page
  • expect the only heading to be Verify your information

night-jellyfish and others added 2 commits May 4, 2023 17:25
Because these classes all inherit from `DocAuthBaseStep` and define the
method in the exact same way, we can DRY it up by removing the method
from the descendant classes.
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Will In-Person Proofing be moving away from FlowStateMachine (the "step" implementations) like what is currently happening with Unsupervised Proofing? If it were, maybe it'd be better to move the common behavior into a concern/mixin rather than the base step, which would be more portable to a controller once DocAuthBaseStep goes away.

@night-jellyfish night-jellyfish requested review from a team and svalexander May 5, 2023 17:54
@JackRyan1989
Copy link
Contributor

@aduth that's a really good call out. We are planning on making that change in the future, though we have a ways to go. Since it's a single method, I'm not sure it makes sense to create a new mixin/concern to house it at this exact moment, but I don't have a strong opinion.

@tomas-nava
Copy link
Contributor

Will In-Person Proofing be moving away from FlowStateMachine (the "step" implementations) like what is currently happening with Unsupervised Proofing? If it were, maybe it'd be better to move the common behavior into a concern/mixin rather than the base step, which would be more portable to a controller once DocAuthBaseStep goes away.

We are planning to move these pages out of the FSM at the same time they're being consolidated into a single-page form, in LG-9443 and related tickets. That's a major refactor so I think it's fine to wait until then to worry about where this method lives.

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. It's nice to be able to delete stuff!

Copy link
Contributor

@JackRyan1989 JackRyan1989 left a comment

Choose a reason for hiding this comment

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

LGTM!

@night-jellyfish night-jellyfish merged commit 549bef0 into main May 8, 2023
@night-jellyfish night-jellyfish deleted the brittany/lg-9515-refactor-capture-secondary-id-enabled branch May 8, 2023 16:26
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