Skip to content

LG-8190: A/B Testing of Acuant SDK Upgrade #7392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eric-gade merged 21 commits into from
Nov 29, 2022
Merged

LG-8190: A/B Testing of Acuant SDK Upgrade #7392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f149c93
Initial commit of Acuant SDK a/b testing
Nov 23, 2022
6b5c1e9
Adding a/b test react context spec
Nov 23, 2022
8e3bd42
Fixing IdentityStore config typos
Nov 25, 2022
c3102b1
Adding test case to document capture step spec
Nov 25, 2022
7ff9ee7
Adding analytics method
Nov 25, 2022
c313395
Update config/initializers/ab_tests.rb
eric-gade Nov 25, 2022
ae046aa
Fixing lints
Nov 25, 2022
3afcaab
Merge branch 'eric-lg-8190' of github-tts:18F/identity-idp into eric-…
Nov 25, 2022
fea12c2
Adding acuant sdk ab test info to doc auth submitted tracking
Nov 25, 2022
3a34d28
Fixing lints
Nov 25, 2022
f032cf6
Fixing template variable issues
Nov 25, 2022
f9a4f79
Update app/views/idv/shared/_document_capture.html.erb
eric-gade Nov 25, 2022
b2192a1
Fixing image uploads spec to assume sdk ab testing is off
Nov 28, 2022
61ea1f1
Using suggested doc-capture pack version of trackEvent
Nov 28, 2022
71f6713
Switching acuant sdk frontend test to typescript
Nov 28, 2022
82b3bef
Fixing lints
Nov 28, 2022
df1b92c
Fixing sdkSrc and cameraSrc props 2b forced when present
Nov 28, 2022
6c1a5e1
Removing React Context for Acuant version A/B Test
Nov 28, 2022
24361ad
Fixing bad acuant camera path in template
Nov 28, 2022
4774b99
Fixing lints
Nov 29, 2022
9f7b4d4
Switching ab test to off by default
Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/forms/idv/api_image_upload_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ def update_analytics(client_response)
client_image_metrics: image_metadata,
async: false,
flow_path: params[:flow_path],
).merge(native_camera_ab_test_data),
).merge(native_camera_ab_test_data, acuant_sdk_upgrade_ab_test_data),
)
pii_from_doc = client_response.pii_from_doc || {}
store_encrypted_images_if_required
Expand Down Expand Up @@ -250,6 +250,14 @@ def native_camera_ab_test_data
}
end

def acuant_sdk_upgrade_ab_test_data
return {} unless IdentityConfig.store.idv_acuant_sdk_upgrade_a_b_testing_enabled
{
acuant_sdk_upgrade_ab_test_bucket:
AbTests::ACUANT_SDK.bucket(document_capture_session.uuid),
}
end

def acuant_sdk_capture?
image_metadata.dig(:front, :source) == Idp::Constants::Vendors::ACUANT &&
image_metadata.dig(:back, :source) == Idp::Constants::Vendors::ACUANT
Expand Down
4 changes: 4 additions & 0 deletions app/javascript/packages/document-capture/context/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,7 @@ export {
default as NativeCameraABTestContext,
Provider as NativeCameraABTestContextProvider,
} from './native-camera-a-b-test';
export {
default as AcuantSdkUpgradeABTestContext,
Provider as AcuantSdkUpgradeABTestContextProvider,
} from './native-camera-a-b-test';
17 changes: 15 additions & 2 deletions app/javascript/packs/document-capture.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ interface AppRootData {
maxAttemptsBeforeNativeCamera: string;
nativeCameraABTestingEnabled: string;
nativeCameraOnly: string;
acuantSdkUpgradeABTestingEnabled: string;
useNewerSdk: string;
acuantVersion: string;
flowPath: FlowPath;
cancelUrl: string;
idvInPersonUrl?: string;
Expand Down Expand Up @@ -67,8 +70,15 @@ function getMetaContent(name): string | null {
const device: DeviceContextValue = { isMobile: isCameraCapableMobile() };

const trackEvent: typeof baseTrackEvent = (event, payload) => {
const { flowPath } = appRoot.dataset;
return baseTrackEvent(event, { ...payload, flow_path: flowPath });
const { flowPath, acuantSdkUpgradeABTestingEnabled, useNewerSdk, acuantVersion } =
appRoot.dataset;
return baseTrackEvent(event, {
...payload,
flow_path: flowPath,
acuant_sdk_upgrade_a_b_testing_enabled: acuantSdkUpgradeABTestingEnabled,
use_newer_sdk: useNewerSdk,
Copy link
Copy Markdown
Contributor

@aduth aduth Nov 28, 2022

Choose a reason for hiding this comment

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

Should we consider populating the context with the Acuant version, and using that as part of the analytics event, as well as in place of the constants in acuant-sdk-upgrade-a-b-test.tsx? Or, maybe we don't even need an extra context provider, and can just use AcuantContext in the way it was designed to receive the sdkSrc?

(Short on time to elaborate at the moment, but will follow-up with more detail on what I had in mind)

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 28, 2022

Choose a reason for hiding this comment

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

What I was thinking is maybe the frontend would provide an SDK version if and only if A/B testing is enabled, and then that version (if given) is used to derive a source string to be given directly to the existing AcuantContext, so that we don't need a new context, and so that AcuantContext wouldn't require many (or any) additional changes.

In packs/document-capture.tsx

interface AppRootData {
  // ...
  acuantVersion?: string;
}

const {
  // ...
  acuantVersion,
} = appRoot.dataset;

const isAcuantSDKUpgradeABTestingEnabled = typeof acuantVersion === 'string';

const trackEvent: typeof baseTrackEvent = (event, payload) => {
  // ...
  return baseTrackEvent(event, {
    ...payload,
    // ...
    acuant_sdk_upgrade_a_b_testing_enabled: isAcuantSDKUpgradeABTestingEnabled,
    acuant_version: acuantVersion,
  });
};

const App = composeComponents(
  // ...
  [
    AcuantContextProvider,
    {
      // ...
      sdkSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantJavascriptWebSdk.min.js`,

Copy link
Copy Markdown
Contributor Author

@eric-gade eric-gade Nov 28, 2022

Choose a reason for hiding this comment

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

Makes sense. The one amendment I would suggest is to also have the actual acuant_sdk_upgrade_a_b_testing_enabled value also be on the AppRoot, because this way we can specify the acuant version to use regardless of the AB test (ie, let's always specify a version to use). Otherwise we would have hard coded values for the version still present in the acuant context

Copy link
Copy Markdown
Contributor Author

@eric-gade eric-gade Nov 28, 2022

Choose a reason for hiding this comment

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

I just pushed 6c1a5e1 which should implement most of your suggestions, along with some minor differences.

To me the outstanding issues are -- as I mentioned in the commit message -- whether/where to store the version numbers as globally accessible values, and whether or not we track an event for when the acuant sdk successfully loads or not.

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 29, 2022

Choose a reason for hiding this comment

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

whether/where to store the version numbers as globally accessible values

Do we need to access them anywhere else? If not, I don't think we'd need to do anything.

If we do, we can probably expose / access AcuantConfig.acuantVersion which would give us the version detail after the script is loaded.

whether or not we track an event for when the acuant sdk successfully loads or not.

What questions are we trying to answer with such an event? Are we concerned that the scripts might not load correctly?

Could we indirectly infer that from this event?

identity-idp/app/javascript/packages/document-capture/context/acuant.tsx

Lines 253 to 256 in 8c40e37

trackEvent('IdV: Acuant SDK loaded', {
success: true,
isCameraSupported: nextIsCameraSupported,
});

We could add the AcuantConfig.acuantVersion in there if we wanted?

Copy link
Copy Markdown
Contributor Author

@eric-gade eric-gade Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could we indirectly infer that from this event?

Yeah for sure, since we wrapped the default trackEvent in document capture and add all that extra information (in the pack). I am not so savvy on Cloudwatch queries so I don't know how much of a pain that is, however. If you think it's GTG let's just roll with it.

Do we need to access them anywhere else?

I am only thinking of the version numbers present in the default paths of the AcuantContext provider here. All other version information is defined on the Rails side now.

But again, if you think this is GTG for the moment let's push on

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 29, 2022

Choose a reason for hiding this comment

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

Oh, right, I forgot we added it in the base trackEvent, so it should be easy to distinguish in CloudWatch by e.g. filter properties.event_properties.acuant_version = '11.7.0', etc.

acuant_version: acuantVersion,
});
};

(async () => {
Expand Down Expand Up @@ -110,6 +120,7 @@ const trackEvent: typeof baseTrackEvent = (event, payload) => {
maxSubmissionAttemptsBeforeNativeCamera,
nativeCameraABTestingEnabled,
nativeCameraOnly,
acuantVersion,
appName,
flowPath,
cancelUrl: cancelURL,
Expand All @@ -125,6 +136,8 @@ const trackEvent: typeof baseTrackEvent = (event, payload) => {
[
AcuantContextProvider,
{
sdkSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantJavascriptWebSdk.min.js`,
cameraSrc: acuantVersion && `/acuant/${acuantVersion}/AcuantCamera.min.js`,
credentials: getMetaContent('acuant-sdk-initialization-creds'),
endpoint: getMetaContent('acuant-sdk-initialization-endpoint'),
glareThreshold,
Expand Down
13 changes: 12 additions & 1 deletion app/services/idv/steps/document_capture_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def extra_view_variables
image_type: 'back',
transaction_id: flow_session[:document_capture_session_uuid],
),
}.merge(native_camera_ab_testing_variables)
}.merge(native_camera_ab_testing_variables, acuant_sdk_upgrade_a_b_testing_variables)
end

private
Expand All @@ -44,6 +44,17 @@ def native_camera_ab_testing_variables
}
end

def acuant_sdk_upgrade_a_b_testing_variables
bucket = AbTests::ACUANT_SDK.bucket(flow_session[:document_capture_session_uuid])
acuant_version = (bucket == :use_newer_sdk) ? '11.7.1' : '11.7.0'
{
acuant_sdk_upgrade_a_b_testing_enabled:
IdentityConfig.store.idv_acuant_sdk_upgrade_a_b_testing_enabled,
use_newer_sdk: (bucket == :use_newer_sdk),
acuant_version: acuant_version,
}
end

def handle_stored_result
if stored_result&.success?
save_proofing_components
Expand Down
3 changes: 3 additions & 0 deletions app/views/idv/capture_doc/document_capture.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@
back_image_upload_url: back_image_upload_url,
native_camera_a_b_testing_enabled: native_camera_a_b_testing_enabled,
native_camera_only: native_camera_only,
acuant_sdk_upgrade_a_b_testing_enabled: acuant_sdk_upgrade_a_b_testing_enabled,
use_newer_sdk: use_newer_sdk,
acuant_version: acuant_version,
) %>
3 changes: 3 additions & 0 deletions app/views/idv/doc_auth/document_capture.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@
back_image_upload_url: back_image_upload_url,
native_camera_a_b_testing_enabled: native_camera_a_b_testing_enabled,
native_camera_only: native_camera_only,
acuant_sdk_upgrade_a_b_testing_enabled: acuant_sdk_upgrade_a_b_testing_enabled,
use_newer_sdk: use_newer_sdk,
acuant_version: acuant_version,
) %>
3 changes: 3 additions & 0 deletions app/views/idv/shared/_document_capture.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
max_submission_attempts_before_native_camera: IdentityConfig.store.doc_auth_max_submission_attempts_before_native_camera,
native_camera_a_b_testing_enabled: native_camera_a_b_testing_enabled,
native_camera_only: native_camera_only,
acuant_sdk_upgrade_a_b_testing_enabled: acuant_sdk_upgrade_a_b_testing_enabled,
use_newer_sdk: use_newer_sdk,
acuant_version: acuant_version,
sp_name: sp_name,
flow_path: flow_path,
cancel_url: idv_cancel_path,
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ idv_max_attempts: 5
idv_min_age_years: 13
idv_native_camera_a_b_testing_enabled: false
idv_native_camera_a_b_testing_percent: 10
idv_acuant_sdk_upgrade_a_b_testing_enabled: false
idv_acuant_sdk_upgrade_a_b_testing_percent: 50
idv_send_link_attempt_window_in_minutes: 10
idv_send_link_max_attempts: 5
ie11_support_end_date: '2022-12-31'
Expand Down
7 changes: 7 additions & 0 deletions config/initializers/ab_tests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,11 @@ module AbTests
nil,
}.compact,
)

ACUANT_SDK = AbTestBucket.new(
experiment_name: 'Acuant SDK Upgrade',
buckets: {
use_newer_sdk: IdentityConfig.store.idv_acuant_sdk_upgrade_a_b_testing_percent,
},
)
end
2 changes: 2 additions & 0 deletions lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ def self.build_store(config_map)
config.add(:idv_min_age_years, type: :integer)
config.add(:idv_native_camera_a_b_testing_enabled, type: :boolean)
config.add(:idv_native_camera_a_b_testing_percent, type: :integer)
config.add(:idv_acuant_sdk_upgrade_a_b_testing_enabled, type: :boolean)
config.add(:idv_acuant_sdk_upgrade_a_b_testing_percent, type: :integer)
config.add(:idv_send_link_attempt_window_in_minutes, type: :integer)
config.add(:idv_send_link_max_attempts, type: :integer)
config.add(:idv_sp_required, type: :boolean)
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/idv/image_uploads_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

before do
Funnel::DocAuth::RegisterStep.new(user.id, '').call('welcome', :view, true)
allow(IdentityConfig.store).to receive(:idv_acuant_sdk_upgrade_a_b_testing_enabled).
and_return(false)
end

context 'when fields are missing' do
Expand Down
2 changes: 2 additions & 0 deletions spec/features/idv/analytics_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@
and_return(fake_analytics)
allow(IdentityConfig.store).to receive(:idv_native_camera_a_b_testing_enabled).
and_return(false)
allow(IdentityConfig.store).to receive(:idv_acuant_sdk_upgrade_a_b_testing_enabled).
and_return(false)
end

context 'Happy path' do
Expand Down
10 changes: 5 additions & 5 deletions spec/javascripts/packages/document-capture/components/acuant-capture-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,16 +958,16 @@ describe('document-capture/components/acuant-capture', () => {
const upload = getByText('Upload');
fireEvent.click(upload);

expect(trackEvent).to.have.been.calledThrice();
expect(trackEvent.getCall(0)).to.have.been.calledWith('IdV: test image clicked', {
expect(trackEvent.callCount).to.be.at.least(3);
expect(trackEvent).to.have.been.calledWith('IdV: test image clicked', {
source: 'placeholder',
isDrop: false,
});
expect(trackEvent.getCall(1)).to.have.been.calledWith('IdV: test image clicked', {
expect(trackEvent).to.have.been.calledWith('IdV: test image clicked', {
source: 'button',
isDrop: false,
});
expect(trackEvent.getCall(2)).to.have.been.calledWith('IdV: test image clicked', {
expect(trackEvent).to.have.been.calledWith('IdV: test image clicked', {
source: 'upload',
isDrop: false,
});
Expand All @@ -986,7 +986,7 @@ describe('document-capture/components/acuant-capture', () => {
const input = getByLabelText('Image');
fireEvent.drop(input);

expect(trackEvent.getCall(0)).to.have.been.calledWith('IdV: test image clicked', {
expect(trackEvent).to.have.been.calledWith('IdV: test image clicked', {
source: 'placeholder',
isDrop: true,
});
Expand Down
42 changes: 42 additions & 0 deletions spec/services/idv/steps/document_capture_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,47 @@
expect(subject.extra_view_variables[:native_camera_only]).to eq(true)
end
end

context 'with acuant sdk upgrade A/B testing enabled' do
let(:session_uuid) { SecureRandom.uuid }

before do
allow(IdentityConfig.store).
to receive(:idv_acuant_sdk_upgrade_a_b_testing_enabled).
and_return(true)

flow.flow_session[:document_capture_session_uuid] = session_uuid
end

context 'and A/B test specifies the newer acuant version' do
before do
stub_const(
'AbTests::ACUANT_SDK',
FakeAbTestBucket.new.tap { |ab| ab.assign(session_uuid => :use_newer_sdk) },
)
end

it 'passes correct variables and acuant version when newer is specified' do
expect(subject.extra_view_variables[:acuant_sdk_upgrade_a_b_testing_enabled]).to eq(true)
expect(subject.extra_view_variables[:use_newer_sdk]).to eq(true)
expect(subject.extra_view_variables[:acuant_version]).to eq('11.7.1')
end
end

context 'and A/B test specifies the older acuant version' do
before do
stub_const(
'AbTests::ACUANT_SDK',
FakeAbTestBucket.new.tap { |ab| ab.assign(session_uuid => 0) },
)
end

it 'passes correct variables and acuant version when older is specified' do
expect(subject.extra_view_variables[:acuant_sdk_upgrade_a_b_testing_enabled]).to eq(true)
expect(subject.extra_view_variables[:use_newer_sdk]).to eq(false)
expect(subject.extra_view_variables[:acuant_version]).to eq('11.7.0')
end
end
end
end
end
6 changes: 6 additions & 0 deletions spec/views/idv/shared/_document_capture.html.erb_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
let(:back_image_upload_url) { nil }
let(:native_camera_a_b_testing_enabled) { false }
let(:native_camera_only) { false }
let(:acuant_sdk_upgrade_a_b_testing_enabled) { false }
let(:use_newer_sdk) { false }
let(:acuant_version) { '11.7.1' }

before do
decorated_session = instance_double(
Expand Down Expand Up @@ -48,6 +51,9 @@
back_image_upload_url: back_image_upload_url,
native_camera_a_b_testing_enabled: native_camera_a_b_testing_enabled,
native_camera_only: native_camera_only,
acuant_sdk_upgrade_a_b_testing_enabled: acuant_sdk_upgrade_a_b_testing_enabled,
use_newer_sdk: use_newer_sdk,
acuant_version: acuant_version,
}
end

Expand Down