Record IAL2 proofing components (LG-5324)#5721
Conversation
zachmargolis
force-pushed
the
margolis-assign-proofing-components
branch
from
9a614d8 to
83430f8
Compare
zachmargolis
requested review from
aduth,
mitchellhenke,
nathanberg and
stevegsa
| expect(proofing_components['document_check']).to be_present | ||
| expect(proofing_components['document_type']).to be_present |
There was a problem hiding this comment.
so the bug that triggered this ticket was that the liveness_required wasn't being recorded, so it would make sense to just include that assertion here
however, a lot of the step helpers to get this done aren't set up for js: true and so we'd need to add in extra expect(page).to have_path('/something', wait: 5) in a bunch of random places, was unsure if it was worth it
| def update_analytics(client_response) | ||
| add_costs(client_response) | ||
| update_funnel(client_response) | ||
| save_proofing_components |
There was a problem hiding this comment.
I'm fine with this approach and this does seem to fit in what I might consider "analytics", but as an alternative, I notice that in other places we add proofing components, it's at the time where we call extract_pii_from_doc. By calling it in the step when we handle the stored result, we could avoid having to reimplement save_proofing_components, and maybe even refactor a bit to combine parts of post_images_and_handle_result and handle_stored_result.
There was a problem hiding this comment.
And actually, looking at this code, I wonder if this implementation could cause duplicate proofing components to be added if it's handled both by the hybrid-mobile API call, and then also by the hybrid-desktop's redirect?
identity-idp/app/services/idv/steps/link_sent_step.rb
Lines 19 to 21 in d3de31e
There was a problem hiding this comment.
So proofing components is a misleading name, there's a unique index so a user only has one proofing_component row at a time, so updating in place with the same data should be ok?
There was a problem hiding this comment.
So proofing components is a misleading name, there's a unique index so a user only has one
proofing_componentrow at a time, so updating in place with the same data should be ok?
Ah, okay. I didn't dig too deep, but had seen in the changes below that we "add" (Db::ProofingComponent::Add) a proofing component, so it wasn't immediately obvious if uniqueness was considered.
There was a problem hiding this comment.
Yeah I just broke out this PR to clean that up, take a quick look and it might explain some of the model more: #5737
But to your original point, yes I'll look at seeing if I can re-implement it in the step to avoid duplication
There was a problem hiding this comment.
Much simpler in 820ae85 ! thanks for the reminder
* Save proofing components in synchronous upload via JS
2 participants
(this is a PR with just a failing test, I was having some problems running this locally so I wanted to see the result in CI)