Skip to content

LG-12445: Attention to Barcode does not override a failed portrait match#10075

Merged
amirbey merged 4 commits intomainfrom
amirbey/remove-all-passed
Feb 15, 2024
Merged

LG-12445: Attention to Barcode does not override a failed portrait match#10075
amirbey merged 4 commits intomainfrom
amirbey/remove-all-passed

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Feb 12, 2024

🎫 Ticket

LG-12445

🛠 Summary of changes

Remove all_passed which is redundant
Update result_success so that Attention to Barcode does not override portrait match failure.

📜 Testing Plan

Unit tests added

@amirbey amirbey changed the title remove all passed from TrueIDREsponse remove all passed from TrueIDResponse Feb 12, 2024
@amirbey amirbey changed the title remove all passed from TrueIDResponse LG-12370: remove all passed from TrueIDResponse Feb 12, 2024
@amirbey amirbey changed the title LG-12370: remove all passed from TrueIDResponse LG-12370: remove all passed from TrueIDResponse refactor Feb 12, 2024
Copy link
Contributor

@night-jellyfish night-jellyfish left a comment

Choose a reason for hiding this comment

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

I had one clarifying question.

Otherwise, I looked through the code and couldn't find any other uses of all_passed?. I ran spec/services/doc_auth/lexis_nexis/responses/true_id_response_spec.rb locally and it all still passed. Looking at the specs there, I couldn't think of another test that should be added due to this change. So pending CI passing this looks good to me.

Edit: CI did pass! I just didn't realize it ran on drafts so didn't look closely enough for it.

@amirbey amirbey marked this pull request as ready for review February 13, 2024 18:56
Copy link
Contributor

@night-jellyfish night-jellyfish left a comment

Choose a reason for hiding this comment

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

LGTM!

@amirbey amirbey force-pushed the amirbey/remove-all-passed branch from 35714d3 to 61780e5 Compare February 14, 2024 00:03
@amirbey amirbey self-assigned this Feb 14, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm sorry to wave back on my approval @amirbey, after reviewing a comment by @eileen-nava on the previous iteration of this code, I'm not sure this change would cover the attention_with_barcode? case (tagged you in the conversation on the doc).

I did find when changing the logic to all_passed? to account for selfie, that this successful_result? didn't have tests. I added them for the selfie, but I wonder if now with all these questions on the logic, if we really need to add tests for the other cases (ie, no selfie but attention with barcode, etc).

I wasn't sure if I should change my review to "request changes", but I'm not sure if that should / could be a separate ticket.

@amirbey amirbey changed the title LG-12370: remove all passed from TrueIDResponse refactor LG-12445: Attn to Barcode does not override a failed portrait match Feb 14, 2024
@amirbey amirbey changed the title LG-12445: Attn to Barcode does not override a failed portrait match LG-12445: Attention to Barcode does not override a failed portrait match Feb 14, 2024
Copy link
Contributor

@night-jellyfish night-jellyfish left a comment

Choose a reason for hiding this comment

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

Thank you for adding the tests!

I tested using the original testing plan and after talking with Amir, realized we found a FE issue regarding the error show. We will need to make a ticket for that issue. But as that issue is not because of this work, it's not a blocker.

Re-approving!

@amirbey amirbey force-pushed the amirbey/remove-all-passed branch from 682598d to f59c312 Compare February 15, 2024 02:00
@amirbey amirbey merged commit 6ea1c96 into main Feb 15, 2024
@amirbey amirbey deleted the amirbey/remove-all-passed branch February 15, 2024 02:19
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.

2 participants