Extract Idv::ProoferValidator#result#1515
Conversation
zachmargolis
force-pushed
the
margolis-extract-vendor-result
branch
from
c7f5caa to
9ea5c4f
Compare
There was a problem hiding this comment.
Do we change the behavior of #to_json somewhere, or is this just verifying that it serializes normalized_applicant correctly?
There was a problem hiding this comment.
We're not changing the behavior anywhere, do you think we should remove the spec?
There was a problem hiding this comment.
Hmm, maybe it the purpose is to check that it correctly serializes things that don't translate to JSON primitives, it could be something like:
it 'serializes normalized_applicant correctly'There was a problem hiding this comment.
Good idea! Will update
jmhooper
left a comment
jmhooper
left a comment
There was a problem hiding this comment.
One question for my understanding.
Looks good 🥇
zachmargolis
force-pushed
the
margolis-extract-vendor-result
branch
2 times, most recently
from
203b6cc to
0ff17d2
Compare
**Why**: Extracting this into a serializable object will make it easier to refactor these classes into background jobs.
| success | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I agree with Reek's TooManyInstanceVariables offense. How about making this class only accept the result as an argument, and then the class can define public method for success, errors, reasons, session_id, and normalized_applicant?
There was a problem hiding this comment.
Just to state my goals clearly, the end result that I want is an object that can be can easily be serialized and passed around (from the background process) so we can continue working on the results of the proofing process outside of a background job. In this case I settled on creating Idv::VendorResult.
If we chose to make this class accept result as you suggest, then @result would be its only instance variable, and it would not serialize cleanly. The result (which is an Proofer::Resolution) has result.vendor_resp which is kind of a free-form field and so we have no way of easily deserializing it (it has different classes and values depending on which method we call on the client).
I am satisfied with the choice here to ignore reek's warning.
There was a problem hiding this comment.
I get it now. Sorry for jumping the gun. It wasn't clear at the beginning what this was doing since new_from_json is not being used yet.
There was a problem hiding this comment.
No worries, thanks for checking! 👍
| describe '#success?' do | ||
| it 'returns Proofer::Confirmation#success?' do | ||
| describe '#result' do | ||
| it 'has success' do |
There was a problem hiding this comment.
is successful? instead of has success?
There was a problem hiding this comment.
Or maybe it 'responds to success?', and it 'responds to errors'
| describe '#success?' do | ||
| it 'returns Proofer::Confirmation#success?' do | ||
| describe '#result' do | ||
| it 'has success' do |
There was a problem hiding this comment.
Same comment as above about the test description.
3 participants
Why:
Extracting this into a serializable object will make it
easier to refactor these classes into background jobs.
Follow-up to #1513