Skip to content

Commit

Permalink
Migrate variantManager to remote config (#3758)
Browse files Browse the repository at this point in the history
Task/Issue URL: https://app.asana.com/0/0/1205644869911729/f

### Description
This PR introduces the capability to download experiment variants from
remote config, replacing the previous hardcoding approach. Additionally,
it includes a refactoring of the old `getVariant()` method, which
previously served as both a setter and a getter for user variants, into
separate setter and getter methods for improved clarity and
maintainability.

### Steps to test this PR

_Replace `PRIVACY_REMOTE_CONFIG_URL` endpoint for
https://www.jsonblob.com/api/1164161108453220352 so you can edit remote
variants for each test_

_Experiment Variants Allocation_
- In remote config add "ma/weight 1.0/ no filters", "mb/weight 1.0/no
filters", "mc/weight 1.0/no filters"
- Fresh install from branch
- [x] Check any of the 3 variants is allocated to the user

_Experiment Variants with weight 0.0_
- In remote config add "ma/weight 0.0/ no filters" & "mb/weight 0.0/no
filters"
- Fresh install from branch
- [x] Check default variant is allocated (_empty string_)

_Experiment Variants with matching locale filter_
- In remote config add "ma/weight 1.0/ locale ["en_US"]" & "mb/weight
1.0/filters: locale ["en_US"]""
- Set your device to English (United States)
- Fresh install from branch
- [x] Check any of the variants have been allocated

_Experiment Variants without matching locale filter_
- Set your device language to English (United States)
- In remote config add "ma/weight 1.0/ locale ["de_DE"]" & "mb/weight
1.0/filters: locale ["de_DE"]""
- Fresh install from branch
- [x] Check default variant is allocated (_empty string_)

_Experiment Variants Update - Disable experiment_
- Make sure you have already a variant allocated (_"ma"_ or _"mb"_)
- In remote config change weight to 0.0 "ma/weight 0.0/no filters" &
"mb/weight 0.0/no filters"
- Install from branch
- [x] Check variant is still allocated to the user

_Experiment Variants Update - Remove experiment_
- Make sure you have already a variant allocated (_"ma"_ or _"mb"_)
- In remote config remove variant allocated to the user
- Install from branch
- [x] Check user variant is updated to default variant (_empty string_)

_Experiment Variants Update - Add new experiment_
- Make sure you have already the default variant allocated (_empty
string_)
- In remote config add a new experiment with variants "md/weight 1.0/ no
filters" & "me/weight 1.0/no filters"
- Install from branch
- [x] Check user variant is not updated (ℹ️ experiments work for new
installs only)

_getVariantKey() should NOT update variant value_
- Install from branch
- Perform an action which would call 'getVariantKey()' -> _e.g._ Send
any pixel
- [x] Check we only get the variant and no setting the value like we
used to do


### No UI changes

---------

Co-authored-by: Cris Barreiro <cbarreiro@duckduckgo.com>
Co-authored-by: Aitor Viana <aitorvs@gmail.com>
  • Loading branch information
3 people authored Nov 21, 2023
1 parent b289c87 commit 40495c5
Show file tree
Hide file tree
Showing 86 changed files with 1,890 additions and 734 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,17 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
.build(),
)
.addParameter("appBuildConfig", appBuildConfig.asClassName(module))
.addParameter("variantManager", variantManager.asClassName(module))
.addCode(
"""
return %T.Builder()
.store(toggleStore)
.appVersionProvider({ appBuildConfig.versionCode })
.flavorNameProvider({ appBuildConfig.flavor.name })
.featureName(%S)
.appVariantProvider({ appBuildConfig.variantName })
// save empty variants will force the default variant to be set
.forceDefaultVariantProvider({ variantManager.saveVariants(emptyList()) })
.build()
.create(%T::class.java)
""".trimIndent(),
Expand Down Expand Up @@ -256,6 +260,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
)
.addParameter("feature", dagger.Lazy::class.asClassName().parameterizedBy(boundType.asClassName()))
.addParameter("appBuildConfig", appBuildConfig.asClassName(module))
.addParameter("variantManager", variantManager.asClassName(module))
.addParameter("context", context.asClassName(module))
.build(),
)
Expand Down Expand Up @@ -289,6 +294,11 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
.initializer("appBuildConfig")
.build(),
)
.addProperty(
PropertySpec.builder("variantManager", variantManager.asClassName(module), KModifier.PRIVATE)
.initializer("variantManager")
.build(),
)
.addProperty(
PropertySpec.builder("context", context.asClassName(module), KModifier.PRIVATE)
.initializer("context")
Expand All @@ -309,6 +319,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
.addFunction(createInvokeMethod(boundType))
.addType(createJsonRolloutDataClass(generatedPackage, module))
.addType(createJsonRolloutStepDataClass(generatedPackage, module))
.addType(createJsonToggleTargetDataClass(generatedPackage, module))
.addType(createJsonToggleDataClass(generatedPackage, module))
.addType(createJsonFeatureDataClass(generatedPackage, module))
.addType(createJsonExceptionDataClass(generatedPackage, module))
Expand Down Expand Up @@ -416,6 +427,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
remoteEnableState = isEnabled,
enable = isEnabled,
minSupportedVersion = feature.minSupportedVersion,
targets = emptyList(),
)
)
Expand All @@ -432,13 +444,19 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
val previousRolloutStep = previousState?.rolloutStep
val newStateValue = (jsonToggle.state == "enabled" || (appBuildConfig.flavor == %T && jsonToggle.state == "internal"))
val targets = jsonToggle?.targets?.map { target ->
Toggle.State.Target(
variantKey = target.variantKey,
)
} ?: emptyList()
this.feature.get().invokeMethod(subfeature.key).setEnabled(
Toggle.State(
remoteEnableState = newStateValue,
enable = previousStateValue,
minSupportedVersion = jsonToggle.minSupportedVersion?.toInt(),
rollout = jsonToggle?.rollout?.steps?.map { it.percent },
rolloutStep = previousRolloutStep,
targets = targets,
),
)
} catch(e: Throwable) {
Expand Down Expand Up @@ -613,6 +631,22 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
.build()
}

private fun createJsonToggleTargetDataClass(
generatedPackage: String,
module: ModuleDescriptor,
): TypeSpec {
return TypeSpec.classBuilder(FqName("$generatedPackage.JsonToggleTarget").asClassName(module))
.addModifiers(KModifier.PRIVATE)
.addModifiers(KModifier.DATA)
.primaryConstructor(
FunSpec.constructorBuilder()
.addParameter("variantKey", String::class.asClassName())
.build(),
)
.addProperty(PropertySpec.builder("variantKey", String::class.asClassName()).initializer("variantKey").build())
.build()
}

private fun createJsonToggleDataClass(
generatedPackage: String,
module: ModuleDescriptor,
Expand All @@ -634,6 +668,10 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
"rollout",
FqName("JsonToggleRollout").asClassName(module).copy(nullable = true),
)
.addParameter(
"targets",
List::class.asClassName().parameterizedBy(FqName("JsonToggleTarget").asClassName(module)),
)
.build(),
)
.addProperty(
Expand All @@ -654,6 +692,12 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
.initializer("rollout")
.build(),
)
.addProperty(
PropertySpec
.builder("targets", List::class.asClassName().parameterizedBy(FqName("JsonToggleTarget").asClassName(module)))
.initializer("targets")
.build(),
)
.build()
}

Expand Down Expand Up @@ -944,6 +988,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
private val context = FqName("android.content.Context")
private val privacyFeaturePlugin = FqName("com.duckduckgo.privacy.config.api.PrivacyFeaturePlugin")
private val appBuildConfig = FqName("com.duckduckgo.appbuildconfig.api.AppBuildConfig")
private val variantManager = FqName("com.duckduckgo.experiments.api.VariantManager")
private val buildFlavorInternal = FqName("com.duckduckgo.appbuildconfig.api.BuildFlavor.INTERNAL")
private val moshi = FqName("com.squareup.moshi.Moshi")
private val jsonObjectAdapter = FqName("JSONObjectAdapter")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ interface AppBuildConfig {
val model: String
val deviceLocale: Locale
val isDefaultVariantForced: Boolean

/**
* You should call [variantName] in a background thread
*/
val variantName: String?
}

enum class BuildFlavor {
Expand Down
2 changes: 2 additions & 0 deletions app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ dependencies {
implementation project(':app-build-config-api')
implementation project(':browser-api')
implementation project(':statistics')
implementation project(':experiments-api')
implementation project(':experiments-impl')
implementation project(':common-utils')
implementation project(':app-store')
implementation project(':common-ui')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,19 @@ package com.duckduckgo.app.integration
import androidx.test.filters.LargeTest
import androidx.test.platform.app.InstrumentationRegistry
import com.duckduckgo.app.getDaggerComponent
import com.duckduckgo.app.statistics.Variant
import com.duckduckgo.app.statistics.VariantManager
import com.duckduckgo.app.statistics.api.RefreshRetentionAtbPlugin
import com.duckduckgo.app.statistics.api.StatisticsRequester
import com.duckduckgo.app.statistics.api.StatisticsService
import com.duckduckgo.app.statistics.model.Atb
import com.duckduckgo.app.statistics.store.StatisticsDataStore
import com.duckduckgo.app.statistics.store.StatisticsSharedPreferences
import com.duckduckgo.autofill.api.email.EmailManager
import com.duckduckgo.common.test.CoroutineTestRule
import com.duckduckgo.common.test.InstantSchedulersRule
import com.duckduckgo.common.utils.plugins.PluginPoint
import com.duckduckgo.experiments.api.VariantManager
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.TestScope
import org.junit.Assert.*
import org.junit.Before
import org.junit.Rule
Expand All @@ -44,6 +46,7 @@ import org.mockito.kotlin.whenever
* Would normally have separate tests for each assertion, but these tests are relatively expensive to run.
*/
@LargeTest
@ExperimentalCoroutinesApi
class AtbIntegrationTest {

private lateinit var mockVariantManager: VariantManager
Expand All @@ -55,21 +58,32 @@ class AtbIntegrationTest {
@get:Rule
val schedulers = InstantSchedulersRule()

@get:Rule
val coroutineTestRule: CoroutineTestRule = CoroutineTestRule()

@Before
fun before() {
mockVariantManager = mock()
statisticsStore = StatisticsSharedPreferences(InstrumentationRegistry.getInstrumentation().targetContext)
statisticsStore.clearAtb()

whenever(mockVariantManager.getVariant()).thenReturn(Variant("ma", 100.0, filterBy = { true }))
whenever(mockVariantManager.getVariantKey()).thenReturn("ma")
service = getDaggerComponent().retrofit().create(StatisticsService::class.java)

val plugins = object : PluginPoint<RefreshRetentionAtbPlugin> {
override fun getPlugins(): Collection<RefreshRetentionAtbPlugin> {
return listOf()
}
}
testee = StatisticsRequester(statisticsStore, service, mockVariantManager, plugins, emailManager)
testee = StatisticsRequester(
statisticsStore,
service,
mockVariantManager,
plugins,
emailManager,
TestScope(),
coroutineTestRule.testDispatcherProvider,
)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,22 @@ import com.duckduckgo.app.notification.model.NotificationPlugin
import com.duckduckgo.app.notification.model.SchedulableNotificationPlugin
import com.duckduckgo.app.pixels.AppPixelName
import com.duckduckgo.app.settings.db.SettingsDataStore
import com.duckduckgo.app.statistics.VariantManager
import com.duckduckgo.app.statistics.VariantManager.Companion.DEFAULT_VARIANT
import com.duckduckgo.app.statistics.pixels.Pixel
import com.duckduckgo.appbuildconfig.api.AppBuildConfig
import com.duckduckgo.common.test.CoroutineTestRule
import com.duckduckgo.common.utils.plugins.PluginPoint
import com.duckduckgo.experiments.api.VariantManager
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.TestScope
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.mockito.kotlin.*
import org.mockito.kotlin.any
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever

@ExperimentalCoroutinesApi
class NotificationRegistrarTest {
Expand All @@ -54,7 +58,7 @@ class NotificationRegistrarTest {

@Before
fun before() {
whenever(mockVariantManager.getVariant(any())).thenReturn(DEFAULT_VARIANT)
whenever(mockVariantManager.getVariantKey()).thenReturn("DEFAULT_VARIANT")
whenever(appBuildConfig.sdkInt).thenReturn(30)
testee = NotificationRegistrar(
TestScope(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ import androidx.lifecycle.LifecycleOwner
import androidx.room.Room
import androidx.test.platform.app.InstrumentationRegistry
import com.duckduckgo.app.global.db.AppDatabase
import com.duckduckgo.app.statistics.Variant
import com.duckduckgo.app.statistics.VariantManager
import com.duckduckgo.app.statistics.api.RxPixelSenderTest.TestPixels.TEST
import com.duckduckgo.app.statistics.config.StatisticsLibraryConfig
import com.duckduckgo.app.statistics.model.Atb
Expand All @@ -32,6 +30,7 @@ import com.duckduckgo.app.statistics.store.PendingPixelDao
import com.duckduckgo.app.statistics.store.StatisticsDataStore
import com.duckduckgo.common.test.InstantSchedulersRule
import com.duckduckgo.common.utils.device.DeviceInfo
import com.duckduckgo.experiments.api.VariantManager
import io.reactivex.Completable
import java.util.concurrent.TimeoutException
import org.junit.After
Expand All @@ -46,11 +45,9 @@ import org.mockito.kotlin.*
class RxPixelSenderTest {

@get:Rule
@Suppress("unused")
var instantTaskExecutorRule = InstantTaskExecutorRule()

@get:Rule
@Suppress("unused")
val schedulers = InstantSchedulersRule()

@Mock
Expand Down Expand Up @@ -98,7 +95,7 @@ class RxPixelSenderTest {
fun whenPixelFiredThenPixelServiceCalledWithCorrectAtbAndVariant() {
givenPixelApiSucceeds()
givenAtbVariant(Atb("atb"))
givenVariant(Variant("variant", filterBy = { true }))
givenVariant("variant")
givenFormFactor(DeviceInfo.FormFactor.PHONE)

testee.sendPixel(TEST.pixelName, emptyMap(), emptyMap())
Expand Down Expand Up @@ -130,7 +127,7 @@ class RxPixelSenderTest {
fun whenPixelFiredWithAdditionalParametersThenPixelServiceCalledWithDefaultAndAdditionalParameters() {
givenPixelApiSucceeds()
givenAtbVariant(Atb("atb"))
givenVariant(Variant("variant", filterBy = { true }))
givenVariant("variant")
givenFormFactor(DeviceInfo.FormFactor.PHONE)
givenAppVersion("1.0.0")

Expand All @@ -145,7 +142,7 @@ class RxPixelSenderTest {
fun whenPixelFiredWithoutAdditionalParametersThenPixelServiceCalledWithOnlyDefaultParameters() {
givenPixelApiSucceeds()
givenAtbVariant(Atb("atb"))
givenVariant(Variant("variant", filterBy = { true }))
givenVariant("variant")
givenFormFactor(DeviceInfo.FormFactor.PHONE)
givenAppVersion("1.0.0")

Expand All @@ -158,7 +155,7 @@ class RxPixelSenderTest {
@Test
fun whenPixelEnqueuedWitAdditionalParametersThenPixelEnqueuedWithParameters() {
givenAtbVariant(Atb("atb"))
givenVariant(Variant("variant", filterBy = { true }))
givenVariant("variant")
givenFormFactor(DeviceInfo.FormFactor.PHONE)
givenAppVersion("1.0.0")
val params = mapOf("param1" to "value1", "param2" to "value2")
Expand All @@ -183,7 +180,7 @@ class RxPixelSenderTest {
@Test
fun whenPixelEnqueuedWithoutAdditionalParametersThenPixelEnqueuedWithOnlyDefaultParameters() {
givenAtbVariant(Atb("atb"))
givenVariant(Variant("variant", filterBy = { true }))
givenVariant("variant")
givenFormFactor(DeviceInfo.FormFactor.PHONE)
givenAppVersion("1.0.0")

Expand Down Expand Up @@ -304,8 +301,8 @@ class RxPixelSenderTest {
whenever(api.fire(any(), any(), any(), any(), any(), any())).thenReturn(Completable.complete())
}

private fun givenVariant(variant: Variant) {
whenever(mockVariantManager.getVariant()).thenReturn(variant)
private fun givenVariant(variantKey: String) {
whenever(mockVariantManager.getVariantKey()).thenReturn(variantKey)
}

private fun givenAtbVariant(atb: Atb) {
Expand Down
Loading

0 comments on commit 40495c5

Please sign in to comment.