Clean up some AB Test bucket code by zachmargolis · Pull Request #6994 · 18F/identity-idp

zachmargolis · 2022-09-20T23:27:42Z

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

- 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

zachmargolis · 2022-09-20T23:29:12Z

config/initializers/ab_tests.rb

+      native_camera_only: IdentityConfig.store.idv_native_camera_a_b_testing_enabled ?
+        IdentityConfig.store.idv_native_camera_a_b_testing_percent :
+        nil,


In my opinion, we shouldn't need to have both an "enabled" feature flag as well as a percent feature flag... we just set the percent to zero (or -1) and boom, everything goes to the default.

I figured that change would require changes to the workflow of whoever manages these flags, so I opted not to change them here

Agreed, that would work for cases like this with only one alternative (which is likely to be the norm).

maybe we could update AbTestBucket to skip categories with weight = 0, and then that's how we can disable them

Well, we also may want to suppress event logging if the test is "disabled"? That doesn't happen in AbTestBucket (though maybe it should?)

I think that we should be logging which buckets get assigned in the particular events, because with Cloudwatch today, we can't join in two separate events easily, so my vote is that AbTestBucket should not do logging itself

zachmargolis · 2022-09-20T23:31:05Z

app/services/doc_auth_router.rb

    end
-
-    max_sha = (16 ** 64) - 1
-    user_value = Digest::SHA256.hexdigest(discriminator).to_i(16).to_f / max_sha * 100


Since the AbTestBucket code injects the experiment name into the percent function, technically this will change the buckets from what is currently deployed.

However, since these are per-session UUID, the users are getting a free new chance every time they proof, so it's not like the same users should be getting the exact same behavior, so this shouldn't affect users significantly

zachmargolis · 2022-09-20T23:31:29Z

spec/services/ab_test_bucket_spec.rb

-    # A slow test that runs the above 10000 times and reports the number of times it fails to pass
-    # due to the random nature of the test. With acceptable_delta == 5, the above spec succeeded
-    # 99.78% of the time.
-    xit 'succeessfully sorts data to within an acceptable delta of the desired percentage' do


disabled test = remove it in my book

zachmargolis · 2022-09-20T23:31:52Z

spec/services/doc_auth_router_spec.rb

-      let(:doc_auth_vendor) { 'test1' }
-      let(:doc_auth_vendor_randomize) { true }
-      let(:doc_auth_vendor_randomize_alternate_vendor) { 'test2' }
-      let(:iterations) { 1000 }


🔥 no more randomized tests, only deterministic ones

zachmargolis · 2022-09-20T23:37:54Z

spec/config/initializers/ab_tests_spec.rb

+    before do
+      allow(IdentityConfig.store).to receive(:idv_native_camera_a_b_testing_enabled).
+        and_return(true)
+      allow(IdentityConfig.store).to receive(:idv_native_camera_a_b_testing_percent).
+        and_return(percent)
+
+      reload_ab_test_initializer!
+    end
+
+    after do
+      allow(IdentityConfig.store).to receive(:idv_native_camera_a_b_testing_enabled).
+        and_call_original
+      allow(IdentityConfig.store).to receive(:idv_native_camera_a_b_testing_percent).
+        and_call_original
+
+      reload_ab_test_initializer!
+    end


This is the one downside to moving these to an initializer, when we want to test the parameters, we have to resort to haxx to reload the objects and reload the initializer

However, I think there are a few reasons why maybe we shouldn't need to reload

The AbTestBucket class tests the behavior and bucketing, most tests should be assigning buckets directly

I think we can reduce the number of config variables and reduce our complexity overall see comment

solipet

Some comments and suggestions, but overall really appreciate that you sorted out how to get them to work as initializers.

app/services/idv/steps/document_capture_step.rb

solipet · 2022-09-21T00:29:17Z

config/initializers/ab_tests.rb

+    }.compact,
+  )
+
+  DOC_AUTH_VENDOR = AbTestBucket.new(


I hadn't converted the doc auth vendor selection to use the AbTestBucket since it was used less as a test between two possibilities and more as a way to gracefully transition from one to the other. We're now permanently keeping it at 95-TrueID, 5-Acuant going forward so we can ensure that we don't break the Acuant path in case we ever need to switch back.

I view A/B tests as more of a short-lived experiment to test which of two presentations to the user result in a better, measurable, outcome.

Having written that, this is all just semantics and this does dry up the code so I'm OK either way.

lib/ab_test_bucket.rb

solipet · 2022-09-21T00:40:35Z

spec/services/doc_auth_router_spec.rb

-      let(:doc_auth_vendor) { 'test1' }
-      let(:doc_auth_vendor_randomize) { true }
-      let(:doc_auth_vendor_randomize_alternate_vendor) { 'test2' }
-      let(:iterations) { 1000 }


Co-authored-by: Doug Price <douglas.price@gsa.gov>

* 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>

Clean up some AB Test bucket code

7503387

- 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

zachmargolis requested review from aduth and solipet September 20, 2022 23:27

zachmargolis commented Sep 20, 2022

View reviewed changes

zachmargolis requested a review from jmhooper September 20, 2022 23:29

zachmargolis commented Sep 20, 2022

View reviewed changes

solipet approved these changes Sep 21, 2022

View reviewed changes

zachmargolis and others added 5 commits September 21, 2022 08:34

Update app/services/idv/steps/document_capture_step.rb

c435c7f

Co-authored-by: Doug Price <douglas.price@gsa.gov>

Update lib/ab_test_bucket.rb

fa65b72

Co-authored-by: Doug Price <douglas.price@gsa.gov>

Update lib/ab_test_bucket.rb

2af0bbe

Co-authored-by: Doug Price <douglas.price@gsa.gov>

Update document_capture_step spec and create new FakeAbTestBucket

60d8467

[skip changelog]

d0f52cd

zachmargolis merged commit e8cd6b3 into main Sep 21, 2022

zachmargolis deleted the margolis-ab-test-cleanup branch September 21, 2022 17:17

solipet mentioned this pull request Sep 22, 2022

Scope the NativeCameraABTest to the Idv module #6989

Merged

zachmargolis mentioned this pull request Oct 5, 2022

Add FakeABTestBucket#assign_all #7093

Merged

1 task

Conversation

zachmargolis commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solipet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants