LG-13657 Remove IPP FSM - set controller feature flag in test and update tests

[jennyverdeyen]

🎫 Ticket

Link to the relevant ticket:
LG-13657

🛠 Summary of changes

In preparation for turning on the in_person_state_id_controller_enabled flag in other environments, this PR turns it on in test and updates tests accordingly.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Verify that tests pass

[gina-yamada]

Other notes:

• All instances of complete_state_id_step being used in the code are in a context block where allow(IdentityConfig.store).to receive(:in_person_state_id_controller_enabled).and_return(false) is set - All instances that I'd expect to be updated have been updated to complete_state_id_controller.

[gina-yamada]

I am not sure the best practice here...

1. I think maybe we should enable the flag here to show new State ID controller is working with the Verify Info page. This would require setting in_person_state_id_controller_enabled to true and updating all instance of complete_state_id_step(user) to complete_state_id_controller(user) as you previously have done.

2. Copy/paste tests and then have an enabled and disabled context block of tests to show both. As I write this- I realize we are not doing this everyone (to show both) in all of the above tests.

After reading Feature Flags Test Coverage - I think we should go with 2. Maybe better in case there is a delay to error on the side of too much test coverage. Maybe we will need to go back and check if above specs have 2 versions to ensure we are testing everything.

Thoughts?

[jennyverdeyen]

Ah you are right, I didn't treat this test the same as others where I've switched the tests to use the controller helper instead of the step one. I can definitely take the second approach and make sure both versions are being tested here. And I'll look at doing the same for the other feature specs.

[jennyverdeyen]

I updated this spec to include both scenarios, the FSM one and the state id controller one.

However in looking at others, in particular in_person_spec.rb, I'm realizing that there are a few helper methods that call the state id step, and also more helper methods that use those methods that use the state id step. So since there's some nested method calls, that means I'll have to make an "FSM" version for a handful of helper methods to call on the old state id FSM "step", only to delete them later. I understand the benefit here is that it does allow us to continue to test the feature flag "false" state until we remove it but I'm not sure it's worth making a number of separate feature test helper methods.

Curious what you and others think @gina-yamada !

[jennyverdeyen]

If we agree that it's too much new code just to test the old feature flag, I will still be sure to keep the changes to this file and others consistent with testing at least the "true"/controller version. It was a good catch that I only flipped this one to expect "false" originally.

[gina-yamada]

Good read:- Lifecycle of a feature flag - I think we should have set default but this has been in development so long that I don't think that would have been a good idea so point this out for future Joy. (I know you were not here when this was started.)

[gina-yamada]

I had recommended adding more tests (when flag was enabled and disabled) rather than just changing the specs around when the flag is enabled. I think you mentioned having to update a lot because of the helper methods- you could also combine complete_state_id_controller and complete_state_id_step with a if block with the expect inside (to eval current path). I leaned towards copy/paste (and have a new block for enabled) rather than just enabling the tests that existed in case this epic got put on hold again just to have more test coverage. It looks like we test the flow for both cases enough in other specs. I think you are okay as is.