[ABTesting] Cache variations of logged out context experiments #8806

selanthiraiyan · 2023-02-02T03:44:13Z

Closes: #8782

⚠️ Kindly review Experiment Variation - Return optional instead of control Automattic/Automattic-Tracks-iOS#247

Description

Raising this PR based on the solution we reached after discussions in this PR - #8788

We are caching the variation values of logged-out experiments to try to address the high crossovers that we see in Explat.

As this PR targets the release branch, I have cherry-picked commits from this PR #8769. This is done to import ABTestVariationProvider

Testing instructions

Install and launch the app. Log out if needed.
Launch and skip onboarding.
If you don't see the "Continue With WordPress.com" button to log in you have been assigned treatment variation for the woocommerceios_login_rest_api_project_202301_v2 experiment and you can test this PR (If not, you need to use a sandbox to force a variation. You can follow the instructions from this PR to assign a variation to your device.)
Login into the app using site creds.
Go to settings and log out.
You will be navigated to the login screen and ensure that you still don't see the "Continue With WordPress.com" button. i.e. You are still assigned the treatment variation.

Screenshots

NA

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

… for reading ABTest.

…ading ABTest.

…rovider`.

…ntication is turned on.

…to disk.

…VariationProvider`

wpmobilebot · 2023-02-02T04:12:32Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8806-d6df1c6 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

itsmeichigo · 2023-02-02T04:52:09Z

Experiments/Experiments/CachedABTestVariationProvider.swift

+    }
+
+    public func variation(for abTest: ABTest) -> Variation {
+        guard abTest.context == .loggedOut else {


Nit: should we leave a comment on why we are caching only for the logged-out context.

I'm not sure if we need to restrict to logged-out experiments only 🤔 We are requesting only experiments for specific contexts so it should be safe to apply this for all contexts right?

We are requesting only experiments for specific contexts so it should be safe to apply this for all contexts right?

I hope you mean that we only request experiments for current contexts from the server. Yes, I agree.

But, I would still prefer only to cache logged-out experiments to minimize the impact of this change. Also, we don't face any crossover issues with logged-in experiments.

WDYT?

Makes sense, please go ahead with this solution 👍

itsmeichigo · 2023-02-02T05:00:06Z

Experiments/ExperimentsTests/VariationCacheTests.swift

+        let cache = VariationCache(userDefaults: userDefaults)
+
+        // When
+        try cache.assign(variation: .treatment, for: .applicationPasswordAuthentication)


Nit: should we create mock tests instead to avoid having to update this test when the experiment is removed?

Good point. Done in e0d9ee5 and 737ee32

itsmeichigo · 2023-02-02T05:00:30Z

Experiments/ExperimentsTests/VariationCacheTests.swift

+        let cache = VariationCache(userDefaults: userDefaults)
+
+        // When
+        XCTAssertThrowsError(try cache.assign(variation: .treatment, for: .aaTestLoggedIn))


Nit: Same comment as above about mocking A/B test.

Done in e0d9ee5 and 737ee32

itsmeichigo · 2023-02-02T05:21:47Z

Experiments/Experiments/CachedABTestVariationProvider.swift

+        if let cachedVariation = cache.variation(for: abTest) {
+            return cachedVariation
+        } else if let variation = abTest.variation {
+            try? cache.assign(variation: variation, for: abTest)
+            return variation
+        } else {
+            return .control
+        }


I think we should prioritize fresh values, i.e. getting the variation from abTest and only use cache as the fallback. That way we always get the latest values:

Suggested change

if let cachedVariation = cache.variation(for: abTest) {

return cachedVariation

} else if let variation = abTest.variation {

try? cache.assign(variation: variation, for: abTest)

return variation

} else {

return .control

}

if let variation = abTest.variation {

try? cache.assign(variation: variation, for: abTest)

return variation

} else if let cachedVariation = cache.variation(for: abTest) {

return cachedVariation

} else {

return .control

}

Done in c0708bf

Earlier, I prioritised cached value to display that we return the cached value if it is available. Because, for a logged-out experiment, it is perfectly valid to get the variation value only once and use it without refresh.

itsmeichigo

🚀

peril-woocommerce · 2023-02-03T05:45:12Z

	Warnings
⚠️	This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

selanthiraiyan added 17 commits February 1, 2023 17:49

Introduce ABTestVariationProvider for reading ABTest from Woo layer.

5a9608e

AuthenticationManager - Inject ABTestVariationProvider and use it…

b8868f1

… for reading ABTest.

AppCoordinator - Inject ABTestVariationProvider and use it for re…

3813dc4

…ading ABTest.

AppCoordinator - Inject ABTestVariationProvider and use it for re…

1c5fdc8

…ading ABTest.

Inject ABTest variation from unit tests.

c3414ad

Stop having variation as public to force usage of `ABTestVariationP…

66a0321

…rovider`.

Unit test the REST API AB test tracks event value.

0033332

Move MockABTestVariationProvider to a separate file for reusability.

4e7535e

Update unit test name to reflect the actual test.

5ae2cda

Add unit tests to check the scenarios when application password authe…

b4b1582

…ntication is turned on.

Make ABTest confirm to Codable to store it to disk

0c32dd1

Introduce VariationCache to store the ABTest variation information …

7c5a5cf

…to disk.

Add cached implementation of ABTestVariationProvider

ada4aa6

Point Automattic tracks pod to beta version.

f9107f3

ABTest - Return nil instead of assuming control as variation.

99b0953

Add ExperimentsTests to unit test plan.

dcca964

Start using CachedABTestVariationProvider instead of `DefaultABTest…

9803a25

…VariationProvider`

selanthiraiyan added type: bug A confirmed bug. feature: login Related to any part of the log in or sign in flow, or authentication. labels Feb 2, 2023

selanthiraiyan mentioned this pull request Feb 2, 2023

Experiment Variation - Return optional instead of control Automattic/Automattic-Tracks-iOS#247

Merged

1 task

Point tracks pod to latest beta commit.

8908527

selanthiraiyan marked this pull request as ready for review February 2, 2023 04:30

selanthiraiyan requested review from itsmeichigo and jaclync February 2, 2023 04:30

itsmeichigo self-assigned this Feb 2, 2023

itsmeichigo reviewed Feb 2, 2023

View reviewed changes

selanthiraiyan added 3 commits February 2, 2023 11:42

Use Mock ABTest values for unit testing.

e0d9ee5

Explain why we cache only logged out experiments.

c4ce4ca

Avoid sending mock ABTest cases to server.

737ee32

Give preference to remote value over cached value.

c0708bf

selanthiraiyan requested a review from itsmeichigo February 2, 2023 06:57

itsmeichigo approved these changes Feb 2, 2023

View reviewed changes

selanthiraiyan added 2 commits February 2, 2023 13:16

Point Automattic-Tracks-iOS to latest beta version.

40c08c3

Update podfile.lock.

527373f

selanthiraiyan added this to the 12.1 ❄️ milestone Feb 2, 2023

toupper mentioned this pull request Feb 2, 2023

Test draft branch for CI #8812

Closed

1 task

mokagio mentioned this pull request Feb 3, 2023

Bump version from 2.0.0-beta.1 to 2.0.0 stable Automattic/Automattic-Tracks-iOS#250

Merged

Point tracks pod to 2.0.0-beta.1

d6df1c6

selanthiraiyan merged commit a20f9b7 into release/12.1 Feb 3, 2023

selanthiraiyan deleted the bugfix/8782-cache-logged-out-ab-tests branch February 3, 2023 08:51

spencertransier mentioned this pull request Feb 3, 2023

Merge 12.1 final into trunk #8825

Merged

[ABTesting] Cache variations of logged out context experiments #8806

[ABTesting] Cache variations of logged out context experiments #8806

Uh oh!

Conversation

selanthiraiyan commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itsmeichigo Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

itsmeichigo Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

selanthiraiyan Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

itsmeichigo Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

itsmeichigo Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

selanthiraiyan Feb 2, 2023 • 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!

itsmeichigo Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

selanthiraiyan Feb 2, 2023 • 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!

itsmeichigo Feb 2, 2023 • 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!

selanthiraiyan Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

itsmeichigo left a comment

Choose a reason for hiding this comment

Uh oh!

peril-woocommerce bot commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

selanthiraiyan commented Feb 2, 2023 •

edited

Loading

wpmobilebot commented Feb 2, 2023 •

edited

Loading

selanthiraiyan Feb 2, 2023 •

edited

Loading

selanthiraiyan Feb 2, 2023 •

edited

Loading

itsmeichigo Feb 2, 2023 •

edited

Loading