LG-7602 Pull VA IP Data and Store It In Session (Form refactor) 1 of 3

[gangelo]

https://cm-jira.usa.gov/browse/LG-7602

1 of 3.

This is simply an existing Form refactor that splits off a FormBase class.

This is a supporting change for the LG-7602 feature for Inherited Proofing (IP) where user PII needs to be stored in session for IP flow steps and subsequent IDV workflow flow steps that need this information for (example) Lexis Phone Finder look ups.

This change exposes a #user_pii method that retuns a Hash suitable to be stored in the idv_session and flow_session[:pii_from_user] (minus :uuid). The data mapped to the user PII returned from this method is Service Provider (SP)-specific in that the SP API returning the user PII may be returned having data keys that differ from what we need; consequently, create a mapper service for this, but that would disconnect what the SP service returns hash-wise. The Form knows about this, so it may be better suited "as is".

changelog: Upcoming Features, Inherited Proofing, Pull VA IP Data and Store It In Session (LG-7602)

[jmhooper]

I'm not sure I understand why we would introduce inheritance here while there is only one subclass? It seems like it introduces indirection and I'm not sure I see the benefit. This class looks a little prettier but there is complexity hiding in the superclass. Now a reader has to reference both places to understand what is going on.

[gangelo]

I hear you. I needed to split this up per the PR comment above. TLDR: the user PII is (#user_pii) is service provider-specific and I didn't want a user to have to refactor what would certainly be a very heavy lift later on, since we are trying to keep our inherited proofing code somewhat service-provider agnostic. The inheritance chain will never go beyond one generation, and the benefit will certainly outweigh the cost in the end IMHO. This is only the first of several changes I need to make. I have the 2nd of 3 changes I am going to push in a little bit, I think it will make more sense.

[gangelo]

For example:

https://github.com/18F/identity-idp/pull/7012/files#diff-3316a6462ad5cc81ddc835c7d5a86652545ed8cb44e674e7ddcae5c85ca02379

The service provider-specific form returned from #inherited_proofing_form will be #submit_ted_ and the #user_pii obtained from the form used to populate flow_session[:pii_from_user] in one of our flow steps prior to handoff to the IDV workflow.

[jmhooper]

I still don't think I see it. Since we only have 1 service provider providing inherited proofing at the moment we have to make assumptions about how future service providers will work. That makes me think we will likely need to refactor this no matter how we build it. With that in mind I'm suspicious of adding the indirection that comes with trying to generalize it through inheritance.

I can approve this but with the note that I foresee the complexity here being a problem in the future.

I also added a comment to the change you linked about because I don't think I understand the direction there.

[gangelo]

@jmhooper that's fine for now, thank you. The only assumption I am trying to make with our Form codez is that we cannot make assumptions about how our future service providers, at least by way of what they will return to us; this is why I used a little meta programming to build/return the hash passed from the Service class.

I'd like to understand more about your concerns about "how future service providers will work"; what exactly are you concerned about in regards to the way we coded our Form class, because all it is doing is accepting a hash from our Service class? If you could take the time to answer at some point, I'd appreciate understanding this more because I don't want to be blind to anything.