Skip to content

LG-10287: Add Acuant SDK 11.9.1 files#8846

Merged
eileen-nava merged 20 commits intomainfrom
em/10287-acuant-sdk-upgrade
Aug 10, 2023
Merged

LG-10287: Add Acuant SDK 11.9.1 files#8846
eileen-nava merged 20 commits intomainfrom
em/10287-acuant-sdk-upgrade

Conversation

@eileen-nava
Copy link
Contributor

@eileen-nava eileen-nava commented Jul 24, 2023

🎫 Ticket

LG-10287

🛠 Summary of changes

  • Include Acuant SDK 11.9.1 files. This is necessary before we can do A/B testing.

📜 Testing Plan

As instructed in the runbook, I tested this on my droid and had a coworker test it on their iphone.

@soniaconnolly
Copy link
Contributor

I tried this out with my iPhone, and it told me to "try again with a driver's license" with both my Oregon and California Real ID drivers licenses. I retried with the current sdk and it accepted the images as it has in the past.

@eileen-nava
Copy link
Contributor Author

I tried this out with my iPhone, and it told me to "try again with a driver's license" with both my Oregon and California Real ID drivers licenses. I retried with the current sdk and it accepted the images as it has in the past.

I saw the same behavior, @soniaconnolly. It's because this version has a fix for the cardtype/cardType bug discussed in PR #8783 and PR #8789. I changed our code to expect cardType and was able to proceed.

@eileen-nava eileen-nava marked this pull request as ready for review August 3, 2023 20:58
@eileen-nava eileen-nava changed the title Em/10287 acuant sdk upgrade LG-10287: Add Acuant SDK 11.9.1 files Aug 3, 2023
@eileen-nava eileen-nava requested review from a team, aduth, dawei-nava and zachmargolis and removed request for a team August 3, 2023 21:02
Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

Overall looks good! Couple of comments, and I'm about to test on my iPhone.


type AcuantCameraUIStart = (
callbacks: AcuantCameraUICallbacks,
onFailure: AcuantFailureCallback,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it still work for the older version to remove this? Same on line 308 below.

Copy link
Contributor Author

@eileen-nava eileen-nava Aug 9, 2023

Choose a reason for hiding this comment

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

Good point. I addressed this in my latest commits, with lots of help from Matt! 🙏🏻

Comment on lines 308 to 314
onError('iOS 15 sequence break', code);
onFailure('iOS 15 sequence break', code);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have two versions of these specs, one for each value of the SDK feature flag, to confirm the code still works for both values?

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 idea! I added more tests. 👍🏻

@soniaconnolly
Copy link
Contributor

Tried it on my iPhone with both versions locally, and SDK loaded and was able to capture images. 👍

To clarify this comment, I don't know how to trigger those failure callbacks, and I'm wondering if the old version still works the way the code is now.

@gina-yamada
Copy link
Contributor

I tried locally with an android. I snapped terrible pictures- blur w/ flash- and I am still successfully moving on to the SSN page.

return window.AcuantCameraUI;
// evaluate the arguments the function start takes
// if the second argument is not a function, it is the current start method, so just return the AcuantCameraUIInterface as is
if (window.AcuantCameraUI && typeof window.AcuantCameraUI.start.arguments[1] !== 'function') {
Copy link
Contributor

@matthinz matthinz Aug 8, 2023

Choose a reason for hiding this comment

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

I don't think this will work, since JS does not augment functions with argument type information like this.

Looking at the diff between 11.8 and 11.9, it looks like they went from:

start(objectWithCallbacks, errorCallback, options)

to:

start(objectWithCallbacksIncludingErrorCallback, options)

I think you want to build a set of 3 arguments that you can pass to both versions of the code. This means:

  • Put the error callback on the first argument
  • Augment the error callback with options and pass that as the second argument
  • Pass the options as the third argument to support the 11.8 version

So something like:

const callbacks = {
  onError() { /* ... */ },
  onCropped(response) { /* ... */ },
} 

const options = { /* ... */ }

const errorCallbackMushedTogetherWithOptions = (...args) => {
  return callbacks.onError(...args)
}
Object.keys(options).forEach(key => {
  errorCallbackMushedTogetherWithOptions[key] = options[key];
});

// Support 11.8 or 11.9
AcuantCameraUI.start(callbacks, errorCallbackMushedTogetherWithOptions, options)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really helpful, thank you! I will try this approach.


it('shows error if capture fails', async () => {
it('shows error if capture fails: legacy version of Acuant SDK', async () => {
const trackEvent = sinon.spy();
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 clarify how this is selecting the legacy version? I'm not seeing version number differences in the two specs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we look at line 275, we can see that the second argument for the start function is the onFailure callback. If this were the more recent version of Acuant SDK, the onFailure callback would be included in the first argument for the start function, which are the AcuantCameraUICallbacks.

To give a more general answer, I decided to test that both version 11.8.2 and version 11.9.1 are supported by stubbing the start function as it would be called for the two different versions. I didn't put any direct references to version number in the specs.

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

@eileen-nava eileen-nava merged commit 7c6ce30 into main Aug 10, 2023
@eileen-nava eileen-nava deleted the em/10287-acuant-sdk-upgrade branch August 10, 2023 14:51
@jmdembe jmdembe mentioned this pull request Aug 15, 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.

6 participants