LG-7152: A/B testing native camera only by solipet · Pull Request #6915 · 18F/identity-idp

solipet · 2022-09-06T12:33:44Z

As a Login user, I want the best possible experience when choosing images for doc auth so I have the best chance at success.

Acceptance criteria

Run an A/B comparison where 10% of sessions reaching document capture do not load the Acuant SDK at all
Log whether a session is offered Acuant SDK or native camera app/upload
Query exists that identifies count of sessions in each test bucket (“native” or “SDK”) and success/fail outcome of doc auth capture

Notes

This is an attempt to understand our bias/reliance on using the Acuant SDK. We generally believe it outperforms manual uploads, but have never tested this.

app/javascript/packages/document-capture/context/native-camera-a-b-test.tsx

app/services/idv/steps/document_capture_step.rb

spec/services/ab_test_bucket_spec.rb

app/services/idv/steps/document_capture_step.rb

app/services/ab_test_bucket.rb

app/javascript/packages/document-capture/components/acuant-capture.tsx

app/services/ab_test_bucket.rb

app/services/idv/native_camera_a_b_test.rb

changelog: Internal, Document Capture, Set up A/B testing for native camera vs Acuant SDK * Rename max failures flag

Log the bucket in the image upload vendor submitted event.

aduth · 2022-09-16T18:38:24Z

app/javascript/packages/document-capture/components/acuant-capture.tsx

        });
      }

+      if (nativeCameraABTestingEnabled) {


Should we include additional checks like what exist in the condition immediately preceding this one?

isAcuantCaptureCapable would limit the event logging to devices which would be candidates for using Acuant (i.e. mobile, not desktop)

isEnvironmentCapture would limit the event logging to environmental capture (i.e. phone back camera, not selfie capture)

Suggested change

if (nativeCameraABTestingEnabled) {

if (isAcuantCaptureCapable && isEnvironmentCapture && nativeCameraABTestingEnabled) {

Great point! I went with if (shouldStartAcuantCapture && nativeCameraABTestEnabled) since that accurately encapsulates what I'm trying to test, i.e., if the user would otherwise be shown the Acuant SDK, check to see if we want to skip it for our test. Sound good?

See 26a75be

aduth · 2022-09-16T18:40:43Z

app/javascript/packages/document-capture/context/native-camera-a-b-test.tsx

+interface NativeCameraABTestContextProviderProps {
+  children: ReactNode;
+  nativeCameraABTestingEnabled: boolean;
+  nativeCameraOnly: boolean;
+}
+
+function NativeCameraABTestContextProvider({
+  children,
+  nativeCameraABTestingEnabled,
+  nativeCameraOnly,
+}: NativeCameraABTestContextProviderProps) {
+  return (
+    <NativeCameraABTestContext.Provider
+      value={{
+        nativeCameraOnly,
+        nativeCameraABTestingEnabled,
+      }}
+    >
+      {children}
+    </NativeCameraABTestContext.Provider>
+  );
+}


You probably don't need this custom provider component, since you could just render NativeCameraABTestContext.Provider from the point where you're currently rendering NativeCameraABTestContextProvider. The main difference would be specifying props as value={{ nativeCameraOnly: ... }} vs. nativeCameraOnly={...}.

As I write it, that could be convenience enough to be worthwhile I suppose. Most of the other context implementations which implement a custom Provider component tend to do so because they'll do a bit more setup in the component.

Eric and I paired a bit on this last Friday and I couldn't (easily) get that change working? I'm not terribly concerned since this will be a short lived context - we will revert this code once the test is complete - so unless you have objections, I'm going to leave it as is.

Yeah, mostly a simplification refactoring, but if this is temporary, not worth concerning too much over.

aduth

One question, but otherwise LGTM 👍

aduth · 2022-09-20T13:55:45Z

app/services/idv/native_camera_a_b_test.rb

+module Idv
+  class NativeCameraABTest < AbTestBucket
+    def initialize
+      buckets = {}


I just want to double-check: Is the absence of a :default key going to cause any trouble here? I see it's included in the default value for the base class, but not here. Some test coverage for this class would help provide extra assurance.

Conversely, if it's not needed, should we bother having it in the default value in the base class?

Good call - I added a spec for the NativeCameraABTest and remove the default: 100 from the base class. See f013dd0

* Replace id_token_hint with client_id in OIDC Logout (#6936) Resolves LG-7433 **Why:** We don't want partners sending us ID tokens as query parameters. We initially permit both client_id and id_token_hint, but also include two feature flags so that we can extend the rollout of both support for client_id as well as the deprecation of id_token_hint through the sandbox. changelog: Bug Fixes, Authentication, Replace id_token_hint with client_id in OIDC logout * Fix a typo in the step indicator constants for inherited proofing (#6985) [skip changelog] * Implement basic Please Verify page UI for Inherited Proofing (#6988) Why: Inherited Proofing users will need to verify that the information we receive from the partner organization is correct changelog: Internal, Inherited Proofing, Adding basic Please Verify UI * LG-7152: A/B testing native camera only (#6915) * LG-7152: Setting up A/B testing for native camera vs Acuant SDK changelog: Internal, Document Capture, Set up A/B testing for native camera vs Acuant SDK * include a feature flag to enable/disable test completely * first cut at AbTestBucket * flesh out AbTestBucket * apply the AbTestBucket to the DocumentCaptureStep * Pull the specifics around this A/B test into its own class. * Log the bucket in the image upload vendor submitted event. * use a fully deterministic spec to test bucket distribution * check for nativeCameraOnly as part of shouldStartAcuantCapture * adds the name of the experiment to the percent generator * better logic on when to block SDK for A/B test * adds a spec for the native camera A/B test * LG-7123 Normalize arguments for enrollments (#6987) **Why:** - We were sometimes passing Pii::Attribute structs and other times passing hashes to this function. While it wasn't causing a problem now it is confusing changelog: Upcoming Features, In-person proofing, Normalize arguments for creating an enrollment * LG-7195 | log_reproof_event is now reachable (#6982) changelog: Internal, Attempts API, Fixes log_reproof_event * Scope the NativeCameraABTest to the Idv module (#6989) [skip changelog] * LG-7364 Return specific attributes that fail from LexisNexis proofer (#6956) * LG-7364 Return specific attributes that fail from LexisNexis proofer This commit aims to refactor the LexisNexis proofer to user a plain old ruby object and to have it return specifically which attributes fail if only certain attributes fail * i can't even write psuedocode * still cannot code * add failing specs for the proofer * put resolution job back the way we found it for now * [skip changelog] * make the lexisnexis proofer look like the phonefinder one * get started on the mock proofer * get mock proofer resolution client passing * start mapping checks to attributes * User proofer_result directly * Punt on merging with state_id_proofer result * Punt on mutating the callback_log_data result * Punt on context field and other proofer results * Group transaction_id and reference with other fields and mark TODO field * Test expected fields in turn * Group fields and mark TODO context field * Test fields in turn * Group and mark TODO * Group and mark TODO * Test fields in turn * Test fields in turn * Consolidate different result hash logic * use match instead of eq * some things passing and some things failing * example of how to fix nomethoderror * Defer when resolution result is only a proofer result * Implement methods on proofing result class * Test result fields in turn * Rename local variable name * Rename threatmetrix entities * Add back resolution tests * Test result fields in turn * Test expected value directly * Test threatmetrix disabled * Test lexisnexis failure response * Restore threatmetrix nil response test * Consolidate logic * Improve format * Improve format * Test against result hash methods * delint * spec cleanup * state id result is not quite ready * clean up agent spec * Define first_name on pii test object * delint Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> * Log timing info for TMX inside ResolutionProofingJob (#6991) [skip changelog] * LG-7469 Standardize naming conventions (#6992) changelog: Internal, Attempts API, Standardize events name * Allow logging of emailage fields including confidence scores (#6993) * Allow logging of emailage fields including confidence scores * changelog: Internal, ThreatMetrix API, allow non-PII fields * Only fetch all email addresses when requested for OIDC user info (#6999) changelog: Internal, Performance, Only fetch all email addresses when requested for OIDC user info * Add ESLint enforcement of awaited userEvent interaction (#6995) * Add ESLint enforcement of awaited userEvent **Why**: Avoid developer confusion associated with race conditions caused by not properly awaiting the completion of a userEvent interaction. changelog: Internal, Automated Testing, Improve developer experience for writing interaction tests * Refactor password reset button spec to avoid Mocha "done" API * Refactor PasswordResetButton spec to use Chai promise helprs * Clean up some AB Test bucket code (#6994) - Move to initializer so we're not constantly re-allocating and checking - Remove ActiveModel::Model, it was only half-used - Update DocAuthRouter to use buckets * Update document_capture_step spec and create new FakeAbTestBucket [skip changelog] Co-authored-by: Doug Price <douglas.price@gsa.gov> * Update Rails (#7000) * Update Rails changelog: Internal, Dependencies, Update Rails * Fix patched behavior for redirects and unsafe redirects * Cache phone_configuration queries during OTP authentication (#6998) changelog: Internal, Performance, Cache phone_configuration queries during OTP authentication * Handle zip+0 at GPO verification letter export (#6970) * Handle zip+0 at GPO verification letter export [skip changelog] * Fix short-circuiting in OTP confirmation (#7002) [skip changelog] * Revert strscan version upgrade (#7003) [skip changelog] Co-authored-by: Oren Kanner <oren.kanner@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Melissa Miller <melissa.miller@gsa.gov> Co-authored-by: Doug Price <douglas.price@gsa.gov> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Matt Wagner <mattwagner@navapbc.com> Co-authored-by: Kimball Bighorse <kbighorse@yahoo.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: olatifflexion <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov>

aduth reviewed Sep 6, 2022

View reviewed changes

app/javascript/packages/document-capture/context/native-camera-a-b-test.tsx Outdated Show resolved Hide resolved

solipet force-pushed the dprice-lg-7152-ab-testing-native-camera branch 3 times, most recently from f0c42c0 to e319b89 Compare September 9, 2022 14:28

zachmargolis reviewed Sep 9, 2022

View reviewed changes

app/services/idv/steps/document_capture_step.rb Outdated Show resolved Hide resolved

solipet force-pushed the dprice-lg-7152-ab-testing-native-camera branch 4 times, most recently from d0cb604 to faf9875 Compare September 14, 2022 18:47

solipet commented Sep 14, 2022

View reviewed changes

spec/services/ab_test_bucket_spec.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 14, 2022

View reviewed changes

app/services/idv/steps/document_capture_step.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 14, 2022

View reviewed changes

app/services/ab_test_bucket.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 14, 2022

View reviewed changes

app/services/ab_test_bucket.rb Outdated Show resolved Hide resolved

aduth reviewed Sep 15, 2022

View reviewed changes

app/javascript/packages/document-capture/components/acuant-capture.tsx Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 15, 2022

View reviewed changes

app/services/ab_test_bucket.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Sep 15, 2022

View reviewed changes

app/services/idv/native_camera_a_b_test.rb Outdated Show resolved Hide resolved

solipet marked this pull request as ready for review September 15, 2022 18:44

solipet requested a review from a team September 15, 2022 18:45

solipet force-pushed the dprice-lg-7152-ab-testing-native-camera branch 2 times, most recently from 9b3a258 to d130d0c Compare September 15, 2022 20:50

solipet added 10 commits September 16, 2022 14:26

LG-7152: Setting up A/B testing for native camera vs Acuant SDK

19476b9

changelog: Internal, Document Capture, Set up A/B testing for native camera vs Acuant SDK * Rename max failures flag

first cut at A/B test context

437f54c

spec for the NativeCameraABTestContext

6486331

include a feature flag to enable/disable test completely

8d716b0

determine test group on server

bc3fa78

lints and specs

406d5ef

first cut at AbTestBucket

18f8d4f

flesh out AbTestBucket

90a47bb

apply the AbTestBucket to the DocumentCaptureStep

9d67f27

Pull the specifics around this A/B test into its own class.

ab623d6

Log the bucket in the image upload vendor submitted event.

solipet added 5 commits September 16, 2022 14:31

use a fully deterministic spec to test bucket distribution

816b87c

check for nativeCameraOnly as part of shouldStartAcuantCapture

831a48b

fixed the specs

2035998

adds the name of the experiment to the percent generator

c9af31c

fix spec

33e71ff

solipet force-pushed the dprice-lg-7152-ab-testing-native-camera branch from 2eae558 to 33e71ff Compare September 16, 2022 18:32

aduth reviewed Sep 16, 2022

View reviewed changes

better logic on when to block SDK for A/B test

26a75be

solipet requested a review from aduth September 19, 2022 18:30

aduth approved these changes Sep 20, 2022

View reviewed changes

adds a spec for the native camera A/B test

f013dd0

jmhooper approved these changes Sep 20, 2022

View reviewed changes

solipet merged commit 1b5d39e into main Sep 20, 2022

solipet deleted the dprice-lg-7152-ab-testing-native-camera branch September 20, 2022 15:36

solipet mentioned this pull request Sep 20, 2022

Scope the NativeCameraABTest to the Idv module #6989

Merged

mitchellhenke mentioned this pull request Sep 22, 2022

Deploy RC 211 to Production #6997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-7152: A/B testing native camera only#6915

LG-7152: A/B testing native camera only#6915
solipet merged 17 commits intomainfrom
dprice-lg-7152-ab-testing-native-camera

solipet commented Sep 6, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth Sep 16, 2022

Uh oh!

solipet Sep 19, 2022 •

edited

Loading

Uh oh!

aduth Sep 16, 2022

Uh oh!

solipet Sep 19, 2022

Uh oh!

aduth Sep 20, 2022

Uh oh!

aduth left a comment

Uh oh!

aduth Sep 20, 2022

Uh oh!

solipet Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (nativeCameraABTestingEnabled) {
	if (isAcuantCaptureCapable && isEnvironmentCapture && nativeCameraABTestingEnabled) {

Conversation

solipet commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

solipet Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

solipet Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

aduth Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

solipet Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

solipet commented Sep 6, 2022 •

edited

Loading

solipet Sep 19, 2022 •

edited

Loading