Skip to content
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-10206 Move STEP_INDICATOR_STEP constants out of InPersonFlow #11607

Conversation

jennyverdeyen
Copy link
Member

🎫 Ticket

Link to the relevant ticket:
LG-10206

🛠 Summary of changes

Moves STEP_INDICATOR_STEPS from InPersonFlow into StepIndicatorConcern. InPersonFlow is FSM-related and will get deleted in a future clean-up PR, so these constants need to be moved as they are still used for the IPP step indicator. References to the old Idv::Flows::InPersonFlow::STEP_INDICATOR_STEPS are updated to reference the new constants.

Note: For 50/50 state management, the old constants in the InPersonFlow are not being removed in this PR.

📜 Testing Plan

No new features or behavior was added - regression testing would be good:

  • Perform manual regression testing of the IPP flow, as well as hybrid and remote flows to be thorough.
  • Ensure tests pass

@jennyverdeyen jennyverdeyen requested review from a team and KeithNava December 6, 2024 17:07
@@ -83,11 +83,7 @@ def pii_locked?
end

def step_indicator_steps
if in_person_proofing?
Copy link
Member Author

Choose a reason for hiding this comment

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

This was probably just missed in the IPP GPO removal, I did some manual testing and this condition should never be true. There is no longer a need for IPP GPO steps so I updated this block.

changelog: Internal, In-person proofing, Move STEP_INDICATOR_STEP constants out of InPersonFlow
@jennyverdeyen jennyverdeyen force-pushed the jverdeyen/LG-10206-move-step-indicator-constants-fsm-removal branch from 492f001 to 246f36a Compare December 16, 2024 21:53
Copy link
Contributor

@eileen-nava eileen-nava left a comment

Choose a reason for hiding this comment

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

I tested manually. On the state id page, I killed the server while it was running on main, switched to main, and was able to successfully move forward in the app. Caveat: I didn't manually test the hybrid flow. Also, I tested ipp, but not gpo.

else
StepIndicatorConcern::STEP_INDICATOR_STEPS_GPO
end
StepIndicatorConcern::STEP_INDICATOR_STEPS_GPO
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻 Thanks for investigating this after our slack conversation.

@gina-yamada gina-yamada self-requested a review December 18, 2024 15:06
Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

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

I think we talked about this before the hackathon- you will delete STEP_INDICATOR_STEPS in InPersonFlow (and probably STEP_INDICATOR_STEPS_GPO) in a follow up PR because of 50/50 state.

This looks good to me. I don't see other instances of Idv::Flows::InPersonFlow::STEP_INDICATOR_STEPS being used in the code. I walked through IPP, GPO, Remote, and hybrid. Steps in step indicator are what I'd expect and I move through flow as I'd expect.

Nice work Jenny 🚀

@jennyverdeyen jennyverdeyen merged commit 2296d09 into main Dec 18, 2024
2 checks passed
@jennyverdeyen jennyverdeyen deleted the jverdeyen/LG-10206-move-step-indicator-constants-fsm-removal branch December 18, 2024 17:41
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