LG-12041: Prevent user from resubmitting a selfie that already failed portrait matching

[amirbey]

🎫 Ticket

LG-12041

🛠 Summary of changes

• add selfie_failed_images to DocumentCaptureSessionResult
• store selfie image in selfie_failed_images when portrait match fails
• add failed selfie images to failed_selfie_image_fingerprints when failing in vendor response

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Submit doc auth with invalid selfie
• Click Try Again
• Submit doc auth with invalid selfie
• Verify error preventing resubmission of same selfie

[charleyf]

Curious if the work around selfie_status_from_response can be moved to a different PR? It seems distinct and the fingerprint work seems like it can stand alone? I may be missing something that ties the two things together?

[charleyf]

It seems odd to me that we have failed_front_fingerprint and failed_back_fingerprint then extra_attributes[:selfie_image_fingerprint]. I'm not saying that using extra_attributes is wrong, more that I'd expect we're doing the same things with all three images and I'm not sure why the selfie is different?

[amirbey]

hey @charleyf 👋🏿 - the doc auth result errors exist across 2 granular parts (front and back image) and the front and failed_(front|back)_fingerprints are defined conditionally based on doc auth response errors that may exist for the front or back images. here we are are always passing the fingerprint to the the document capture session

[charleyf]

Ah. The reason I was asking is that it felt weird to me that we needed semi-complex conditional logic for the front and back and not the selfie.

The more I think about this, the more I'm pretty sure that the conditional logic here needs to change to reflect how we want to handle selfie_fingerprint errors.

My reasoning:

• if client_response && !client_response.success? && !client_response.network_error? is checking "Did the client response fail in a way where we should store the image fingerprints as 'failed' ?"
  • This means that when selfie is enabled, we'll always store the selfie fingerprint, regardless if it was the selfie that failed. This is where I'd expect a check on the :selfie_status === :Fail
• elsif doc_pii_response && !doc_pii_response.success?
  • This says "If the doc_pii_response fails"
  • selfie_image_fingerprint: extra_attributes[:selfie_image_fingerprint], then mark the selfie as failed.
  • I don't think that in this case we do actually want to fail the selfie? The PII is not affected by the selfie?

[amirbey]

@charleyf - here we are always passing the selfie fingerprint but conditionally store the selfie_fingerprint

[charleyf]

Interesting, I missed that. Does it make sense to match failed_front_fingerprint and back_image_fingerprint and do something like this in store_failed_images?

      failed_selfie_fingerprint = nil
      if client_response && client_response.selfie_status == :Fail
          failed_selfie_fingerprint = extra_attributes[:selfie_image_fingerprint]
      end

[amirbey]

yes, i did think about that 🤔

my reasoning for putting this condition in the DocumentCaptureSession was that if we call store_failed_auth_data in more places in the future it would require us to remember and duplicate this condition/code. however, in regards to the front and back fingerprint values, the business logic for storing front and back images is pretty specific to the errors here and also can happen if doc auth result fails while we have no errors returned from the vendor.

[charleyf]

That makes sense, I would still expect the conditionals that determine when we store fingerprints to be in one place: either store_failed_images or DocumentCaptureSession, but I may be misunderstanding a benefit here.

Either of these would make more sense to me in the session 🤷

session_result.add_failed_front_image!(front_image_fingerprint)  if front_image_fingerprint
session_result.add_failed_back_image!(back_image_fingerprint)  if back_image_fingerprint
session_result.add_failed_selfie_image!(selfie_image_fingerprint) if selfie_image_fingerprint

session_result.add_failed_front_image!(front_image_fingerprint)
session_result.add_failed_back_image!(back_image_fingerprint)
session_result.add_failed_selfie_image!(selfie_image_fingerprint)

[charleyf]

I'm running through some test cases now, will likely approve after that.

[charleyf]

👍 Interesting, I didn't know about this fixtureing file. I think I just saw some test files that can benefit from this.

[charleyf]

That makes sense, I would still expect the conditionals that determine when we store fingerprints to be in one place: either store_failed_images or DocumentCaptureSession, but I may be misunderstanding a benefit here.

Either of these would make more sense to me in the session 🤷

session_result.add_failed_front_image!(front_image_fingerprint)  if front_image_fingerprint
session_result.add_failed_back_image!(back_image_fingerprint)  if back_image_fingerprint
session_result.add_failed_selfie_image!(selfie_image_fingerprint) if selfie_image_fingerprint

session_result.add_failed_front_image!(front_image_fingerprint)
session_result.add_failed_back_image!(back_image_fingerprint)
session_result.add_failed_selfie_image!(selfie_image_fingerprint)

[charleyf]

I ran through a few testing scenarios and this seems to work.

Notes:

• The mock proofer makes it hard to test this in dev, staging is a much better test.
• There are a bunch of analytics events in analytics_events.rb with front_image_fingerprint and back_image_fingerprint that haven't had selfie_image_fingerprint added.

[dawei-nava]

LGTM.