Separate idv_form from Idv::Step classes#1520
Conversation
There was a problem hiding this comment.
@monfresh I'd love your feedback here
To keep the changeset as small as possible, I had the two form submissions use the same event name which resulted in hacks like this expect(...).twice.
My guess is that we'll want to split this into two events, one for the form submit and one for the vendor submit? Do you think we should address this now, or in a follow-up PR?
There was a problem hiding this comment.
Good question. I think 2 events makes sense. If it doesn't add too much to this PR, I would say do it now, but I'll leave it up to you.
There was a problem hiding this comment.
I'll address that in this PR, thanks!
There was a problem hiding this comment.
Updated in dd4270d98e911f5ce4a187757da461089a5a9514, PTAL!
There was a problem hiding this comment.
Why define this in the controllers instead of in the IdvFailuerConcern where it is invoked?
There was a problem hiding this comment.
Well each form has a different name (ex idv_profile_form, idv_phone_form, idv_finance_form) ... so I guess I could just rename those to idv_form and then make it consistent again
Thanks for pointing this out, I'm fixing it now
There was a problem hiding this comment.
Updated in ff881b7bfe89aae16a8beea0e993c33ed7c50ec4
There was a problem hiding this comment.
Since this action doesn't communicate with the vendor, what do you think about replacing render_failure with @view_model = view_model since the first 2 conditions in render_failure will always be false here.
There was a problem hiding this comment.
Sure, fixed in 34d939adde4b413ca33f558f37068cbcfb593dfb
There was a problem hiding this comment.
Same comment here about using @view_model = view_model.
There was a problem hiding this comment.
Now that we've separated the form submission from the vendor validation, should we make sure we have a controller or integration test that makes sure the attempts count is only incremented if the form submission passed?
There was a problem hiding this comment.
Good idea! Added some expectations to the controller spec in ea7de2b3a02d0491ef726c530d21116d837d2f54
app/services/idv/financials_step.rb
Outdated
There was a problem hiding this comment.
CC says this is not tested. I confirmed locally.
There was a problem hiding this comment.
You're right! It's not called ever, I think I can remove it.
app/services/idv/phone_step.rb
Outdated
There was a problem hiding this comment.
CC says this is not tested. I confirmed locally.
There was a problem hiding this comment.
Ditto, removing it
zachmargolis
force-pushed
the
margolis-remove-idv-form
branch
from
ea7de2b to
d459ca4
Compare
|
@monfresh thanks for the review! I just squashed the commits, if it LGTM can you hit the "approve" button? |
**Why**: This will let us break submission in to two steps and into two controller actions which will further help us when we move the call to our vendors to background jobs.
3 participants
Why:
This will let us break submission in to two steps and
into two controller actions which will further help us
when we move the call to our vendors to background
jobs.
--
Before: the step classes combined results from
idv_formas well asvendor_validator(which calls out to the vendor).Now: the step classes just wrap the vendor validator and the associated side effects.
Next:
vendor_resultas input#createactions into#createand#showwhere the#showaction can poll for the result and then submit to theStepclasses if readyOpen questions: