Skip to content

Add InPerson::AddressController to FlowPolicy#9794

Merged
soniaconnolly merged 8 commits intomainfrom
yamada/address-controller-add-flow-policy
Dec 20, 2023
Merged

Add InPerson::AddressController to FlowPolicy#9794
soniaconnolly merged 8 commits intomainfrom
yamada/address-controller-add-flow-policy

Conversation

@soniaconnolly
Copy link
Contributor

@soniaconnolly soniaconnolly commented Dec 19, 2023

🛠 Summary of changes

Add InPerson::AddressController to FlowPolicy. This controller is being developed behind a feature flag to replace the in person AddressStep which is still in the Flow State Machine. It requires an addition to the FlowPolicy code to accommodate it: allow clear_future_steps! in IdvStepConcern to specify a future controller, since for AddressController we do not want to clear the SSN if the user has already entered it.

In Idv::Session#ipp_document_capture_complete? it checks whether pii_from_user[:address1] is present, not just whether pii_from_user is present.

Note that this PR does not fix back button issues on the in person flow. It just adds the new AddressController to FlowPolicy.

📜 Testing Plan

  • In application.yml, set in_person_residential_address_controller_enabled: true and restart server
  • Create account, start IdV
  • Complete in person proofing up to submitting VerifyInfo
  • Try the back button, confirm that any unexpected behavior is also present on main
  • In application.yml, set in_person_residential_address_controller_enabled: false and restart server
  • Complete in person proofing up to submitting VerifyInfo
  • Try the back button, confirm that any unexpected behavior is also present on main

Setting :pii_from_user in flow_policy_spec is a temporary workaround while the in person flow
still has steps in the Flow State Machine.

def clear_future_steps!
flow_policy.undo_future_steps_from_controller!(controller: self.class)
def clear_future_steps!(controller: self.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would vote for this to be a new method, clear_future_steps_from!(controller:) and have clear_future_steps! call that with self.class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 37a127c

allow(subject).to receive(:pii_from_user).and_return(pii_from_user)
allow(subject).to receive(:flow_session).and_return(flow_session)
stub_sign_in(user)
subject.user_session['idv/in_person'] = flow_session
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're removing references to flow_session below, can we get rid of the let as well and just initialize this:

Suggested change
subject.user_session['idv/in_person'] = flow_session
subject.user_session['idv/in_person'] = { pii_from_user: pii_from_user }

stub_sign_in(user)
subject.user_session['idv/in_person'] = flow_session
subject.idv_session.welcome_visited = true
subject.idv_session.idv_consent_given = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this use stub_up_to?

end

def update
# don't clear the ssn when updating address, clear after SsnController
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 leaving comment- I understand b/c we talked about this but think it will be helpful to future devs

end

# update Idv::DocumentCaptureController.step_info.next_steps to include
# :ipp_address instead of :ipp_ssn in delete PR
Copy link
Contributor

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 but want to check that my understanding is correct. next_step should be the first non-FSM step. So currently- that is SSN. Upon delete- it will be address.

But still can't remember why not just keep it ipp_ssn b/c it is the next step in the flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The in person flow has DocumentCapture -> (several React steps) -> State ID -> Address (optional) -> SSN -> Verify Info. Eventually FlowPolicy will have :document_capture next_steps include :ipp_state_id when that's out of the FSM. We might need to wait to update it until then, actually, since Address is optional. It's going to take some fiddling to get all that right.

Comment on lines +58 to +62
flow_session[:pii_from_user][:address1] = nil
flow_session[:pii_from_user][:address2] = nil
flow_session[:pii_from_user][:city] = nil
flow_session[:pii_from_user][:zipcode] = nil
flow_session[:pii_from_user][:state] = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svalexander, since the state_id step also sets these fields, I wonder if we shouldn't clear them here. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's ok for now, especially since we're not yet including this step as a next step anywhere, so undo_step won't be called. And then if we always clear starting after SsnController for the in person steps that edit pii_from_user and ssn, it won't be a problem. But we'll have to give this more thought and see if we can come up with a better solution.

@gina-yamada gina-yamada self-requested a review December 20, 2023 02:32
@soniaconnolly soniaconnolly merged commit c2180f3 into main Dec 20, 2023
@soniaconnolly soniaconnolly deleted the yamada/address-controller-add-flow-policy branch December 20, 2023 21:22
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