Support settings for top-level and sub-feature and remove sub-feature…

… config (#5269) Task/Issue URL: https://app.asana.com/0/1198194956794324/1208740919623939/f ### Description We want to support `settings` for all top-level and sub-features because we can define schemas and validate them in the remote config As a result we can remove the sub-feature `config` ### Steps to test this PR _Test_ - [ ] test pass - [ ] smoke test abn - [ ] smoke test autofill --------- Co-authored-by: Marcos <[email protected]> Co-authored-by: Craig Russell <[email protected]>
duckduckgo · Nov 14, 2024 · 3a877a3 · 3a877a3
1 parent d13de5b
commit 3a877a3
Show file tree

Hide file tree

Showing 17 changed files with 315 additions and 186 deletions.
diff --git a/...il-annotations/src/main/java/com/duckduckgo/anvil/annotations/ContributesRemoteFeature.kt b/...il-annotations/src/main/java/com/duckduckgo/anvil/annotations/ContributesRemoteFeature.kt
@@ -55,6 +55,7 @@ annotation class ContributesRemoteFeature(
     val featureName: String,
 
     /** The class that implements the [FeatureSettings.Store] interface */
+    @Deprecated("Not needed anymore. Settings is now supported in top-level and sub-features and Toggle#getSettings returns it")
     val settingsStore: KClass<*> = Unit::class,
 
     /** The class that implements the [FeatureExceptions.Store] interface */

diff --git a/...iler/src/main/java/com/duckduckgo/anvil/compiler/ContributesRemoteFeatureCodeGenerator.kt b/...iler/src/main/java/com/duckduckgo/anvil/compiler/ContributesRemoteFeatureCodeGenerator.kt
@@ -498,6 +498,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
                             minSupportedVersion = feature.minSupportedVersion,
                             targets = emptyList(),
                             cohorts = emptyList(),
+                            settings = feature.settings?.toString(),
                         )
                     )
         
@@ -528,7 +529,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
                                         weight = cohort.weight,
                                     )
                                 } ?: emptyList()
-                                val config = jsonToggle?.config ?: emptyMap()
+                                val settings = jsonToggle?.settings?.toString()
                                 this.feature.get().invokeMethod(subfeature.key).setRawStoredState(
                                     Toggle.State(
                                         remoteEnableState = newStateValue,
@@ -539,7 +540,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
                                         assignedCohort = previousAssignedCohort,
                                         targets = targets,
                                         cohorts = cohorts,
-                                        config = config,                                        
+                                        settings = settings,                                        
                                     ),
                                 )
                             } catch(e: Throwable) {
@@ -781,10 +782,7 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
                         "cohorts",
                         List::class.asClassName().parameterizedBy(FqName("JsonToggleCohort").asClassName(module)),
                     )
-                    .addParameter(
-                        "config",
-                        Map::class.asClassName().parameterizedBy(String::class.asClassName(), String::class.asClassName()),
-                    )
+                    .addParameter("settings", FqName("org.json.JSONObject").asClassName(module).copy(nullable = true))
                     .build(),
             )
             .addProperty(
@@ -819,8 +817,8 @@ class ContributesRemoteFeatureCodeGenerator : CodeGenerator {
             )
             .addProperty(
                 PropertySpec
-                    .builder("config", Map::class.asClassName().parameterizedBy(String::class.asClassName(), String::class.asClassName()))
-                    .initializer("config")
+                    .builder("settings", FqName("org.json.JSONObject").asClassName(module).copy(nullable = true))
+                    .initializer("settings")
                     .build(),
             )
             .build()

diff --git a/app/src/main/java/com/duckduckgo/app/trackerdetection/blocklist/BlockList.kt b/app/src/main/java/com/duckduckgo/app/trackerdetection/blocklist/BlockList.kt
@@ -70,10 +70,7 @@ interface BlockList {
     }
 
     companion object {
-        const val EXPERIMENT_PREFIX = "tds"
-        const val TREATMENT_URL = "treatmentUrl"
-        const val CONTROL_URL = "controlUrl"
-        const val NEXT_URL = "nextUrl"
+        internal const val EXPERIMENT_PREFIX = "tds"
     }
 }
 

diff --git a/.../main/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListInterceptorApiPlugin.kt b/.../main/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListInterceptorApiPlugin.kt
@@ -20,12 +20,12 @@ import com.duckduckgo.app.global.api.ApiInterceptorPlugin
 import com.duckduckgo.app.trackerdetection.api.TDS_BASE_URL
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.CONTROL
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.TREATMENT
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.CONTROL_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.NEXT_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.TREATMENT_URL
 import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.feature.toggles.api.FeatureTogglesInventory
 import com.squareup.anvil.annotations.ContributesMultibinding
+import com.squareup.moshi.JsonAdapter
+import com.squareup.moshi.Moshi
+import com.squareup.moshi.Types
 import javax.inject.Inject
 import kotlinx.coroutines.runBlocking
 import okhttp3.Interceptor
@@ -38,8 +38,12 @@ import okhttp3.Response
 )
 class BlockListInterceptorApiPlugin @Inject constructor(
     private val inventory: FeatureTogglesInventory,
+    private val moshi: Moshi,
 ) : Interceptor, ApiInterceptorPlugin {
 
+    private val jsonAdapter: JsonAdapter<Map<String, String>> by lazy {
+        moshi.adapter(Types.newParameterizedType(Map::class.java, String::class.java, String::class.java))
+    }
     override fun intercept(chain: Chain): Response {
         val request = chain.request().newBuilder()
         val url = chain.request().url
@@ -51,11 +55,16 @@ class BlockListInterceptorApiPlugin @Inject constructor(
         }
 
         return activeExperiment?.let {
+            val config = activeExperiment.getSettings()?.let {
+                runCatching {
+                    jsonAdapter.fromJson(it)
+                }.getOrDefault(emptyMap())
+            } ?: emptyMap()
             val path = when {
-                activeExperiment.isEnabled(TREATMENT) -> activeExperiment.getConfig()[TREATMENT_URL]
-                activeExperiment.isEnabled(CONTROL) -> activeExperiment.getConfig()[CONTROL_URL]
-                else -> activeExperiment.getConfig()[NEXT_URL]
-            } ?: chain.proceed(request.build())
+                activeExperiment.isEnabled(TREATMENT) -> config["treatmentUrl"]
+                activeExperiment.isEnabled(CONTROL) -> config["controlUrl"]
+                else -> config["nextUrl"]
+            } ?: return chain.proceed(request.build())
             chain.proceed(request.url("$TDS_BASE_URL$path").build())
         } ?: chain.proceed(request.build())
     }

diff --git a/app/src/test/java/com/duckduckgo/app/brokensite/api/BrokenSiteSubmitterTest.kt b/app/src/test/java/com/duckduckgo/app/brokensite/api/BrokenSiteSubmitterTest.kt
@@ -11,8 +11,6 @@ import com.duckduckgo.app.statistics.pixels.Pixel
 import com.duckduckgo.app.statistics.pixels.Pixel.PixelType.Count
 import com.duckduckgo.app.statistics.store.StatisticsDataStore
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.TREATMENT
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.CONTROL_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.TREATMENT_URL
 import com.duckduckgo.app.trackerdetection.blocklist.FakeFeatureTogglesInventory
 import com.duckduckgo.app.trackerdetection.blocklist.TestBlockListFeature
 import com.duckduckgo.app.trackerdetection.db.TdsMetadataDao
@@ -43,6 +41,7 @@ import com.duckduckgo.privacy.config.api.PrivacyConfigData
 import com.duckduckgo.privacy.config.api.PrivacyFeatureName
 import com.duckduckgo.privacy.config.api.UnprotectedTemporary
 import com.duckduckgo.privacyprotectionspopup.api.PrivacyProtectionsPopupExperimentExternalPixels
+import com.squareup.moshi.Moshi
 import java.time.ZoneId
 import java.time.ZonedDateTime
 import java.time.temporal.ChronoUnit
@@ -68,6 +67,15 @@ class BrokenSiteSubmitterTest {
     @get:Rule
     var coroutineRule = CoroutineTestRule()
 
+    private val moshi = Moshi.Builder().build()
+
+    private data class Config(
+        val treatmentUrl: String? = null,
+        val controlUrl: String? = null,
+        val nextUrl: String? = null,
+    )
+    private val configAdapter = moshi.adapter(Config::class.java)
+
     private val mockPixel: Pixel = mock()
 
     private val mockVariantManager: VariantManager = mock()
@@ -605,10 +613,7 @@ class BrokenSiteSubmitterTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "treatmentUrl",
-                    CONTROL_URL to "controlUrl",
-                ),
+                settings = configAdapter.toJson(Config(treatmentUrl = "treatmentUrl", controlUrl = "controlUrl")),
                 assignedCohort = State.Cohort(name = TREATMENT.cohortName, weight = 1, enrollmentDateET = enrollmentDateET),
             ),
         )

diff --git a/app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt b/app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt
@@ -10,8 +10,6 @@ import com.duckduckgo.app.pixels.AppPixelName
 import com.duckduckgo.app.statistics.pixels.Pixel
 import com.duckduckgo.app.statistics.pixels.Pixel.PixelType.Daily
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.TREATMENT
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.CONTROL_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.TREATMENT_URL
 import com.duckduckgo.app.trackerdetection.blocklist.BlockListPixelsPlugin
 import com.duckduckgo.app.trackerdetection.blocklist.FakeFeatureTogglesInventory
 import com.duckduckgo.app.trackerdetection.blocklist.TestBlockListFeature
@@ -24,6 +22,7 @@ import com.duckduckgo.feature.toggles.api.FeatureToggles
 import com.duckduckgo.feature.toggles.api.FeatureTogglesInventory
 import com.duckduckgo.feature.toggles.api.Toggle.State
 import com.duckduckgo.feature.toggles.impl.RealFeatureTogglesInventory
+import com.squareup.moshi.Moshi
 import java.time.ZoneId
 import java.time.ZonedDateTime
 import java.time.temporal.ChronoUnit
@@ -48,6 +47,15 @@ class RefreshPixelSenderTest {
     @get:Rule
     var coroutineTestRule = CoroutineTestRule()
 
+    private val moshi = Moshi.Builder().build()
+
+    private data class Config(
+        val treatmentUrl: String? = null,
+        val controlUrl: String? = null,
+        val nextUrl: String? = null,
+    )
+    private val configAdapter = moshi.adapter(Config::class.java)
+
     private lateinit var db: AppDatabase
     private lateinit var refreshDao: RefreshDao
     private val mockPixel: Pixel = mock()
@@ -304,10 +312,7 @@ class RefreshPixelSenderTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "treatmentUrl",
-                    CONTROL_URL to "controlUrl",
-                ),
+                settings = configAdapter.toJson(Config(treatmentUrl = "treatmentUrl", controlUrl = "controlUrl")),
                 assignedCohort = State.Cohort(name = TREATMENT.cohortName, weight = 1, enrollmentDateET = enrollmentDateET),
             ),
         )

diff --git a/...t/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListInterceptorApiPluginTest.kt b/...t/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListInterceptorApiPluginTest.kt
@@ -22,9 +22,6 @@ import com.duckduckgo.app.trackerdetection.api.TDS_BASE_URL
 import com.duckduckgo.app.trackerdetection.api.TDS_PATH
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.CONTROL
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.TREATMENT
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.CONTROL_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.NEXT_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.TREATMENT_URL
 import com.duckduckgo.common.test.CoroutineTestRule
 import com.duckduckgo.common.test.api.FakeChain
 import com.duckduckgo.feature.toggles.api.FakeToggleStore
@@ -34,6 +31,7 @@ import com.duckduckgo.feature.toggles.api.Toggle
 import com.duckduckgo.feature.toggles.api.Toggle.DefaultValue
 import com.duckduckgo.feature.toggles.api.Toggle.State
 import com.duckduckgo.feature.toggles.impl.RealFeatureTogglesInventory
+import com.squareup.moshi.Moshi
 import org.junit.Assert.*
 import org.junit.Before
 import org.junit.Rule
@@ -49,6 +47,14 @@ class BlockListInterceptorApiPluginTest {
     private lateinit var testBlockListFeature: TestBlockListFeature
     private lateinit var inventory: FeatureTogglesInventory
     private lateinit var interceptor: BlockListInterceptorApiPlugin
+    private val moshi = Moshi.Builder().build()
+
+    private data class Config(
+        val treatmentUrl: String? = null,
+        val controlUrl: String? = null,
+        val nextUrl: String? = null,
+    )
+    private val configAdapter = moshi.adapter(Config::class.java)
 
     @Before
     fun setup() {
@@ -69,7 +75,7 @@ class BlockListInterceptorApiPluginTest {
             coroutineRule.testDispatcherProvider,
         )
 
-        interceptor = BlockListInterceptorApiPlugin(inventory)
+        interceptor = BlockListInterceptorApiPlugin(inventory, moshi)
     }
 
     @Test
@@ -78,9 +84,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "treatmentUrl",
-                    CONTROL_URL to "controlUrl",
+                settings = configAdapter.toJson(
+                    Config(treatmentUrl = "treatmentUrl", controlUrl = "controlUrl"),
                 ),
                 cohorts = listOf(
                     State.Cohort(name = CONTROL.cohortName, weight = 0),
@@ -92,9 +97,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "anotherTreatmentUrl",
-                    CONTROL_URL to "anotherControlUrl",
+                settings = configAdapter.toJson(
+                    Config(treatmentUrl = "anotherTreatmentUrl", controlUrl = "anotherControlUrl"),
                 ),
                 cohorts = listOf(
                     State.Cohort(name = CONTROL.cohortName, weight = 0),
@@ -114,9 +118,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "anotherTreatmentUrl",
-                    CONTROL_URL to "anotherControlUrl",
+                settings = configAdapter.toJson(
+                    Config(treatmentUrl = "anotherTreatmentUrl", controlUrl = "anotherControlUrl"),
                 ),
                 cohorts = listOf(
                     State.Cohort(name = CONTROL.cohortName, weight = 0),
@@ -136,9 +139,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "anotherTreatmentUrl",
-                    CONTROL_URL to "anotherControlUrl",
+                settings = configAdapter.toJson(
+                    Config(treatmentUrl = "anotherTreatmentUrl", controlUrl = "anotherControlUrl"),
                 ),
                 cohorts = listOf(
                     State.Cohort(name = CONTROL.cohortName, weight = 1),
@@ -158,8 +160,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    NEXT_URL to "nextUrl",
+                settings = configAdapter.toJson(
+                    Config(nextUrl = "nextUrl"),
                 ),
             ),
         )
@@ -182,9 +184,8 @@ class BlockListInterceptorApiPluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "anotherTreatmentUrl",
-                    CONTROL_URL to "anotherControlUrl",
+                settings = configAdapter.toJson(
+                    Config(treatmentUrl = "anotherTreatmentUrl", controlUrl = "anotherControlUrl"),
                 ),
                 cohorts = listOf(
                     State.Cohort(name = CONTROL.cohortName, weight = 1),

diff --git a/...st/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListPrivacyTogglePluginTest.kt b/...st/java/com/duckduckgo/app/trackerdetection/blocklist/BlockListPrivacyTogglePluginTest.kt
@@ -3,8 +3,6 @@ package com.duckduckgo.app.trackerdetection.blocklist
 import android.annotation.SuppressLint
 import com.duckduckgo.app.statistics.pixels.Pixel
 import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Cohorts.TREATMENT
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.CONTROL_URL
-import com.duckduckgo.app.trackerdetection.blocklist.BlockList.Companion.TREATMENT_URL
 import com.duckduckgo.common.test.CoroutineTestRule
 import com.duckduckgo.feature.toggles.api.FakeToggleStore
 import com.duckduckgo.feature.toggles.api.FeatureToggles
@@ -14,6 +12,7 @@ import com.duckduckgo.feature.toggles.impl.RealFeatureTogglesInventory
 import com.duckduckgo.privacy.dashboard.api.PrivacyToggleOrigin.BREAKAGE_FORM
 import com.duckduckgo.privacy.dashboard.api.PrivacyToggleOrigin.DASHBOARD
 import com.duckduckgo.privacy.dashboard.api.PrivacyToggleOrigin.MENU
+import com.squareup.moshi.Moshi
 import java.time.ZoneId
 import java.time.ZonedDateTime
 import java.time.temporal.ChronoUnit
@@ -31,6 +30,15 @@ class BlockListPrivacyTogglePluginTest {
     @get:Rule
     var coroutineRule = CoroutineTestRule()
 
+    private val moshi = Moshi.Builder().build()
+
+    private data class Config(
+        val treatmentUrl: String? = null,
+        val controlUrl: String? = null,
+        val nextUrl: String? = null,
+    )
+    private val configAdapter = moshi.adapter(Config::class.java)
+
     private val pixel: Pixel = mock()
     private lateinit var testBlockListFeature: TestBlockListFeature
     private lateinit var inventory: FeatureTogglesInventory
@@ -88,10 +96,7 @@ class BlockListPrivacyTogglePluginTest {
             State(
                 remoteEnableState = true,
                 enable = true,
-                config = mapOf(
-                    TREATMENT_URL to "treatmentUrl",
-                    CONTROL_URL to "controlUrl",
-                ),
+                settings = configAdapter.toJson(Config(treatmentUrl = "treatmentUrl", controlUrl = "controlUrl")),
                 assignedCohort = State.Cohort(name = TREATMENT.cohortName, weight = 1, enrollmentDateET = enrollmentDateET),
             ),
         )

diff --git a/autofill/autofill-impl/build.gradle b/autofill/autofill-impl/build.gradle
@@ -74,6 +74,7 @@ dependencies {
     implementation "net.zetetic:android-database-sqlcipher:_"
 
     // Testing dependencies
+    testImplementation project(':common-test')
     testImplementation "org.mockito.kotlin:mockito-kotlin:_"
     testImplementation Testing.junit4
     testImplementation AndroidX.core