-
Notifications
You must be signed in to change notification settings - Fork 120
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-12534 FSM clean up #11578
base: main
Are you sure you want to change the base?
LG-12534 FSM clean up #11578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file removal is a long time coming! Excited to see it happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same! Go @jennyverdeyen!
f9d621b
to
7174c83
Compare
b8e2b3b
to
2162849
Compare
…on controller changelog: Internal, In-person proofing, cleaning up unused FSM code
…s breaking after removing the dynamic FSM /:step route
6b536d8
to
4386652
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested locally and used telephony to test the hybrid flow. Things worked well. Approved.
:set_usps_form_presenter, | ||
:redirect_unless_enrollment, | ||
:initialize_in_person_session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see this added. 💪🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask if there's a benefit to increasing test coverage for the functionality of theinitialize_in_person_session
method, but I realize it's tricky to test private methods. I'm curious what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same! Go @jennyverdeyen!
step = local_assigns[:action] || local_assigns[:step] | ||
path = (step_url && step) ? send(step_url, step: step) : go_back_path | ||
path = go_back_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩
step_url = local_assigns[:step_url] || @step_url | ||
step = local_assigns[:action] || local_assigns[:step] | ||
path = (step_url && step) ? send(step_url, step: step) : go_back_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the code documentation above to remove action
and step_url
if they're no longer relevant?
🎫 Ticket
Link to the relevant ticket:
LG-12534
🛠 Summary of changes
Removes remaining Flow State Machine related files and references
📜 Testing Plan
No new functionality is being added, but manual regression testing is needed to ensure nothing has been adversely affected.