Skip to content

LG-12080: Send selfie_status to FE#10073

Merged
charleyf merged 12 commits intomainfrom
charley/lg-12080-send-selfie-success-to-fe
Feb 14, 2024
Merged

LG-12080: Send selfie_status to FE#10073
charleyf merged 12 commits intomainfrom
charley/lg-12080-send-selfie-success-to-fe

Conversation

@charleyf
Copy link
Contributor

@charleyf charleyf commented Feb 12, 2024

🎫 Ticket

https://cm-jira.usa.gov/browse/LG-12080

🛠 Summary of changes

This PR sends the selfie_status from the BE to the FE so we can show appropriate error messages (see this PR).

📜 Testing Plan

  • Make sure tests pass in CI
  • Run your localhost server
  • Get to the front/back/selfie page
    • Upload the test_selfie_with_face_match_fail.yml for all three of front/back/selfie
    • Start watching the logs (in a new terminal) with make watch_events
  • Click the submit button
  • Look at the logs (from make watch_events or in cloudwatch)
    • Find the "name": "IdV: doc auth image upload vendor submitted" event
    • In that event this line should appear: "selfie_status": "fail",`
  • You can also redo these steps with a passing image.
    • You should get "selfie_status": "success" when the selfie passes
    • You should get no "selfie_status" line when the selfie is not required.

@charleyf charleyf marked this pull request as ready for review February 12, 2024 20:40
@dawei-nava
Copy link
Contributor

I think we also need add liveness_enabled: selfie_required entry to the extra hash

extra: {
doc_auth_result: doc_auth_result,
portrait_match_results: portrait_match_results,
billed: true,
classification_info: classification_info,
}.compact,
)

And also test for the value in spec.

expect(response.extra[:liveness_enabled]).to eq(false/true)

attention_with_barcode: attention_with_barcode? || other.attention_with_barcode?,
doc_type_supported: doc_type_supported? || other.doc_type_supported?,
selfie_status: selfie_status,
selfie_live: selfie_live?,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response to this:
I think that confusingly we already have selfie_status tests in various specs. Search for "xpect(response.selfie_status).to eq to find a few.

Copy link
Contributor

@dawei-nava dawei-nava Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charleyf, my point is that expect(response.extra[:liveness_checking_required]).to eq(false/true) should work properly for result_response, unless we add it to extra of result_response , the following method in presenter will always be false for mock client

  def show_selfie_failures?
    @form_response.extra[:liveness_checking_required] == true
  end

I think variable name liveness_enabled changed to liveness_checking_required

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but I think what @dawei-nava is saying in the linked comment is that extra hash in the mock proofer should match the extra in the real acuant response.

But, I might not quite be understanding that right. From what I can see the liveness_enabled is included in the Mock Proofer's response here. But I could see an argument for adding the selfie status.

Maybe I'm way off base here though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and while I was typing out my response I see Dawei replied! I'm still wrapping my head around it but I think maybe my comment was wrong, and I misunderstood.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this following basically is the contract between the presenter and response(mock or trueid etc). The presenter expects it can fetch @form_response.extra[:liveness_checking_required]

 def show_selfie_failures?
    @form_response.extra[:liveness_checking_required] == true
  end

Copy link
Contributor

@dawei-nava dawei-nava Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify what I meant, I just pushed a check to the result_response_spec.rb to show it does not contains the information the presenter expected.

Copy link
Contributor Author

@charleyf charleyf Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for clarifying and thanks for adding that test Dawei.

I think variable name liveness_enabled changed to liveness_checking_required

I'm not sure what happened, but I do think that I'm going to address it by changing liveness_enabled to liveness_checking_required in the mock proofer.

Copy link
Contributor

@dawei-nava dawei-nava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I assume that the selfie_status is already fetched in FE.

@charleyf charleyf merged commit 0f8b27c into main Feb 14, 2024
@charleyf charleyf deleted the charley/lg-12080-send-selfie-success-to-fe branch February 14, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants