Skip to content

LG-13005: use TransactionStatus to determine if document passed TrueID#10427

Merged
amirbey merged 37 commits intomainfrom
amirbey/spike-LG-13005
May 22, 2024
Merged

LG-13005: use TransactionStatus to determine if document passed TrueID#10427
amirbey merged 37 commits intomainfrom
amirbey/spike-LG-13005

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Apr 12, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13005

🛠 Summary of changes

Use TrueIdResponse transaction status value (passed/failed) to determine if the document was successfully authenticated.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • DocAuth succeeds after submitting a yml document which includes transaction_status: passed
  • DocAuth fails after submitting a yml document which includes transaction_status: failed
  • DocAuth succeeds after submitting a yml document with transaction_status: passed and 2D Barcode Read error after user confirms name and DOB

@amirbey amirbey changed the title [SPIKE] doc auth pass if transaction status passes [SPIKE] doc auth success reflect TrueID transaction status Apr 15, 2024
@amirbey amirbey force-pushed the amirbey/spike-LG-13005 branch from 90e9a1b to dfd28c5 Compare April 16, 2024 16:49
@amirbey amirbey force-pushed the amirbey/spike-LG-13005 branch 2 times, most recently from 2ad7495 to 77d0f4e Compare April 30, 2024 15:35
@amirbey amirbey force-pushed the amirbey/spike-LG-13005 branch from 77d0f4e to a7db0c5 Compare May 14, 2024 14:49
@amirbey amirbey changed the title [SPIKE] doc auth success reflect TrueID transaction status LG-13005 doc auth success reflect TrueID transaction status May 14, 2024
@amirbey amirbey self-assigned this May 14, 2024
@amirbey amirbey force-pushed the amirbey/spike-LG-13005 branch 2 times, most recently from 2c9f414 to b501648 Compare May 16, 2024 21:02
@amirbey amirbey marked this pull request as ready for review May 16, 2024 22:54
@amirbey amirbey changed the title LG-13005 doc auth success reflect TrueID transaction status LG-13005: use TransactionStatus to determine if document passed TrueID May 16, 2024
"ExecutedStepName": "True_ID_Step",
"ProductConfigurationName": "AndreV3_TrueID_Flow",
"ProductStatus": "pass",
"ProductStatus": "fail",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say more about these changes (also in some of the other fixtures)? I could be mistaken but I thought these were direct responses from TrueID and this PR is to change what field we rely on from TrueID, and not that the TrueID response itself has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@night-jellyfish - yes, you are correct ... this change is about relying on a different field (transaction_status)! 👍🏿
while we will not be evaluating product_status ... in our prod events.log, it is consistent that product status and transaction status either both pass or both fail. however, it seems there is a discrepancy in many of our fixtures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! I guess I was worrying a little that I thought our fixtures were taken directly from requests to the API (and then anonymized). So that would imply to me that maybe the values could be opposing. But maybe I'm wrong about how they were created and this isn't a concern.

@amirbey amirbey requested a review from night-jellyfish May 20, 2024 16:04
return false unless id_type_supported?
return false if transaction_status_from_uploaded_file == 'failed'
return true if transaction_status_from_uploaded_file == 'passed'
return false if doc_auth_result_from_uploaded_file == 'Failed'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line in case there are yml files out there that haven't implemented transaction_status?

If so, it does seem like it could be worthwhile enforcing folks to include that, rather than keeping this method for old code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point .. currently we do not force folks to include doc_auth_result ... so replicating this behavior with transaction_status

@@ -166,15 +183,23 @@ def classification_info
end

def doc_auth_result_from_success
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we still need this method? It seems possible that we could refactor this away if we aren't using the doc_auth_result for any logic.


def all_doc_capture_values_passing?(doc_auth_result, id_type_supported)
doc_auth_result == 'Passed' &&
def transaction_status_from_success
Copy link
Contributor

Choose a reason for hiding this comment

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

this name is confusing to me - I'm not sure I understand the from_success piece.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

continuing the pattern used in doc_auth_result_from_success this is if the document is authenticated successfully

@amirbey amirbey requested a review from night-jellyfish May 21, 2024 15:29
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.

Tested out manually as per the testing instructions, and it all worked! 🎉

There are still some comments unresolved from me, but I don't think there are any that are blocking.

changelog: Internal, Document Authentication, TrueIDReponse successful if transaction status passes
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