Extract a synchronous proofing job#1524
Conversation
|
Looks like you forgot to pass in the |
There was a problem hiding this comment.
Did you mean perform_later?
There was a problem hiding this comment.
Nope! I wanted to make this synchronous because then we don't have to touch the UI in this PR. Next PR will be asynchronous
There was a problem hiding this comment.
Can we find a better name for this, or split it up into multiple classes if necessary? Adding "Service" to the name of a class is a smell. It's implied that it's a service, so the name of the class should still make sense if you drop "Service" from its name, but VendorValidator would not be an appropriate name for what this class is doing.
There was a problem hiding this comment.
Instead of VendorValidatorService#submit_job, how about a class called SubmitIdvJob with a call method?
There was a problem hiding this comment.
Sure, I'll go with SubmitIdvJob and turn the other into VendorValidatorResultStorage
There was a problem hiding this comment.
Fixed in e58e2bdf2bc3d1cbccd49ac2bb64bff0e5143bb7
app/services/idv/vendor_result.rb
Outdated
There was a problem hiding this comment.
ABC size is "assignment branch condition" complexity and this method scored too high so I just disabled it, because I thought it was more important to have an equality method we could use in specs to compare things
There was a problem hiding this comment.
I put the comment on the wrong line. I meant what does the method do? Sounds like it's mainly to facilitate testing? Is this method used anywhere besides tests?
There was a problem hiding this comment.
Nope, it's just used in tests, I wanted a shorthand for comparing a VendorResult that we reconstructed from redis to compare to the one we put in redis (VendorResultStorage)
There was a problem hiding this comment.
I'm not a big fan of having public methods that are only used in tests. Can this be done using a custom RSpec matcher?
There was a problem hiding this comment.
I'll just remove it and check the attributes manually
There was a problem hiding this comment.
Removed in 83af72b1cca3849a97f5e619d1587814c64f63ff
There was a problem hiding this comment.
Does this TTL get updated or only set once when first called? I'm thinking about the scenario where the user starts proofing at 12:00, this TTL is then set to expire at 12:08, but the user takes longer than that to finish proofing. For example, after they submit the first profile page, let's say the vendor takes a minute to respond. Then, the user stays idle for 7 minutes on the financial info page because they had to go find their credit card. It's now 12:08, but the session is still active because their last request was only 7 minutes ago. So then they enter their credit card, but now the Redis TTL has expired. Will this cause any issues?
There was a problem hiding this comment.
This is the TTL for the result value of one job.
So for the 3 steps, they will have 3 separate result objects that will have 3 separate expirations.
When this is asynchronous, the result is created after hitting submit and hearing back from the vendor. So as long as the user's browser takes less than this time to refresh the page (which it definitely will, I think we'll set refresh interval to something like 10-15 seconds) then the controller will pick up the result from redis and use it for what it needs to be used and move on to the next step.
zachmargolis
force-pushed
the
margolis-proofing-job
branch
2 times, most recently
from
a3c4e92 to
f758ecb
Compare
**Why**: One last small refactor step before making the job async
2 participants
Why: One last small refactor step before making the job async