Skip to content

LG-10255: Capture cardtype in sdk#8783

Merged
eileen-nava merged 5 commits intomainfrom
em/10255-capture-cardtype-in-sdk
Jul 17, 2023
Merged

LG-10255: Capture cardtype in sdk#8783
eileen-nava merged 5 commits intomainfrom
em/10255-capture-cardtype-in-sdk

Conversation

@eileen-nava
Copy link
Contributor

🎫 Ticket

Check ID 'card type' when using SDK

🛠 Summary of changes

  • Correctly capture acuant document types during sdk image capture. Previously, all document types were captured as "undefined" and logged as "none."
  • Update the logs to display when a value other than "none," "id," or "passport" is captured.
  • Update spec file to use the key "cardtype" instead of "cardType"
  • Write a test for the new log option

📜 Testing Plan

  • Run automated tests.
  • N.B. I didn't notice this bug until I made an actual request to acuant, rather than using our mock client. As a result, I think acceptance testing done in staging will be more effective than manual testing with the mock client.

👀 Screenshots

Here's a screenshot debugger, which shows the cardType/cardtype bug.

Screenshot: cardtypeDebuggerScreenshot

P.S. This seems like it might be a bug in the Acuant SDK, because their documentation indicates that the response from the onCropped callback should use cardType, not cardtype.

@eileen-nava eileen-nava requested a review from aduth July 14, 2023 16:30
@eileen-nava eileen-nava changed the title Capture cardtype in sdk LG-10255: Capture cardtype in sdk Jul 14, 2023
@aduth
Copy link
Contributor

aduth commented Jul 14, 2023

Not blocking this, but could be worth flagging this with Acuant support folks, due to the inconsistency in the documentation. It looks like they have at least one internal reference to the wrong property name in their own code as well, which might be similarly broken:

https://github.com/Acuant/JavascriptWebSDKV11/blob/11.8.1/webSdk/AcuantCamera.js#L1468

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 31 to 34
const ACUANT_CAPTURE_CARDTYPE_ERROR = { ...ACUANT_CAPTURE_SUCCESS_RESULT };

delete ACUANT_CAPTURE_CARDTYPE_ERROR.cardtype;
ACUANT_CAPTURE_CARDTYPE_ERROR.cardType = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely clear why we're setting the casing here this way, since the code never references it. To exercise the new default case, would we want to use the correct casing on the property, but a value that's not accounted for (e.g. 3, 4, or undefined explicitly?).

Suggested change
const ACUANT_CAPTURE_CARDTYPE_ERROR = { ...ACUANT_CAPTURE_SUCCESS_RESULT };
delete ACUANT_CAPTURE_CARDTYPE_ERROR.cardtype;
ACUANT_CAPTURE_CARDTYPE_ERROR.cardType = 2;
const ACUANT_CAPTURE_CARDTYPE_ERROR = { ...ACUANT_CAPTURE_SUCCESS_RESULT, cardtype: undefined };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that works! I changed this and did the setup in the test itself.

@eileen-nava
Copy link
Contributor Author

Not blocking this, but could be worth flagging this with Acuant support folks, due to the inconsistency in the documentation. It looks like they have at least one internal reference to the wrong property name in their own code as well, which might be similarly broken:

https://github.com/Acuant/JavascriptWebSDKV11/blob/11.8.1/webSdk/AcuantCamera.js#L1468

Yeah, I agree that it would be good to flag to Acuant support folks. Should I reach out to a specific person at Acuant who works with GSA? Or post an issue in the acuant repo?

@aduth
Copy link
Contributor

aduth commented Jul 14, 2023

Yeah, I agree that it would be good to flag to Acuant support folks. Should I reach out to a specific person at Acuant who works with GSA? Or post an issue in the acuant repo?

I'll follow-up on Slack!

@eileen-nava eileen-nava merged commit dbdef13 into main Jul 17, 2023
@eileen-nava eileen-nava deleted the em/10255-capture-cardtype-in-sdk branch July 17, 2023 14:41
@aduth aduth mentioned this pull request Jul 17, 2023
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