Return FormResponse from IdV form submission#1517
Conversation
app/services/idv/step.rb
Outdated
There was a problem hiding this comment.
reek is complaining aobut this.
We could probably make it happy by doing:
form_validate(params).success?
There was a problem hiding this comment.
I just went ahead and fixed in 50f069e738d8fbf9c1fd901f11866ef324e6c0b6
app/forms/idv/finance_form.rb
Outdated
There was a problem hiding this comment.
What do you think about assigning valid? to a variable, like success, and then using the variable, to avoid calling valid? twice?
app/forms/idv/phone_form.rb
Outdated
There was a problem hiding this comment.
Same comment here about only calling valid? once.
spec/forms/idv/finance_form_spec.rb
Outdated
There was a problem hiding this comment.
Can we also add expect(result).to be_kind_of(FormResponse) here?
There was a problem hiding this comment.
Yeah, there's probably a few places we can look for this
There was a problem hiding this comment.
Pushed a commit to resolve all of these
There was a problem hiding this comment.
add expect(result).to be_kind_of(FormResponse) here?
spec/services/idv/phone_step_spec.rb
Outdated
There was a problem hiding this comment.
Add expect(result).to be_kind_of(FormResponse) here?
There was a problem hiding this comment.
Add expect(result).to be_kind_of(FormResponse) here?
There was a problem hiding this comment.
Was there a particular reason you changed these specs? These specs should still pass the way they were, right? Seems like these changes can be in a separate PR. They are a bit distracting from the purpose of this PR.
There was a problem hiding this comment.
I think this PR would be easier to review without these changes. Do you mind saving them for later?
There was a problem hiding this comment.
They did not pass because the step.submit would submit the form which would call FormResponse.new with different params than what the stub was looking for.
There was a problem hiding this comment.
I think this PR would be easier to review without these changes. Do you mind saving them for later?
Out of sync here. Agree but the specs don't pass w/o them
There was a problem hiding this comment.
I see the issue now. It's that the Step class is now calling #success? on the FormResponse object, and that method wasn't being stubbed. Your changes make sense now and I like what you did.
**Why**: As part of backgrounding vendor proofing, it will be helpful for the IdV form objects to behave like the rest of the form objects in the app so we can extract them from the `Idv::Step` subclasses and invoke them in the controller.
jmhooper
force-pushed
the
jmhooper-return-form-responses
branch
from
8144469 to
ba9cd3c
Compare
3 participants
Why: As part of backgrounding vendor proofing, it will be helpful for the IdV form objects to behave like the rest of the form objects in the app so we can extract them from the
Idv::Stepsubclasses and invoke them in the controller.