Skip to content

Commit 16f1de6

Browse files
aitorvsaitorvs
authored
Check minSupportedVersion and remote enable state before assigning cohort (#6050)
Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1210218578062547?focus=true ### Description Check minSupportedVersion and remote feature flag state before assigning cohort. Cohort is only assign if: * min supported version matches AND * feature remote state is enabled (this includes state=`internal`) ### Steps to test this PR New 2e2 tests should pass.
1 parent 2788dca commit 16f1de6

File tree

2 files changed

+344
-6
lines changed

2 files changed

+344
-6
lines changed

‎feature-toggles/feature-toggles-api/src/main/java/com/duckduckgo/feature/toggles/api/FeatureToggles.kt

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -388,12 +388,21 @@ internal class ToggleImpl constructor(
388388
val cohortDefaultValue = false
389389

390390
return store.get(key)?.let { state ->
391-
// we assign cohorts if it hasn't been assigned before or if the cohort was removed from the remote config
392-
val updatedState = if (state.assignedCohort == null || !state.cohorts.map { it.name }.contains(state.assignedCohort.name)) {
393-
state.copy(assignedCohort = assignCohortRandomly(state.cohorts, state.targets)).also {
394-
it.assignedCohort?.let { cohort ->
395-
callback?.onCohortAssigned(this.featureName().name, cohort.name, cohort.enrollmentDateET!!)
391+
// appVersion should be above or equal to minSupportedVersion for the cohort to be assigned
392+
// we do not assign a cohort if the feature flag is not enabled remotely
393+
val updatedState = if (
394+
appVersionProvider.invoke() >= (state.minSupportedVersion ?: 0) &&
395+
state.remoteEnableState == true
396+
) {
397+
// we assign cohorts if it hasn't been assigned before or if the cohort was removed from the remote config
398+
if (state.assignedCohort == null || !state.cohorts.map { it.name }.contains(state.assignedCohort.name)) {
399+
state.copy(assignedCohort = assignCohortRandomly(state.cohorts, state.targets)).also {
400+
it.assignedCohort?.let { cohort ->
401+
callback?.onCohortAssigned(this.featureName().name, cohort.name, cohort.enrollmentDateET!!)
402+
}
396403
}
404+
} else {
405+
state
397406
}
398407
} else {
399408
state

‎feature-toggles/feature-toggles-impl/src/test/java/com/duckduckgo/feature/toggles/codegen/ContributesRemoteFeatureCodeGeneratorTest.kt

Lines changed: 330 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package com.duckduckgo.feature.toggles.codegen
1818

19+
import android.annotation.SuppressLint
1920
import android.content.Context
2021
import androidx.test.ext.junit.runners.AndroidJUnit4
2122
import androidx.test.platform.app.InstrumentationRegistry
@@ -59,6 +60,7 @@ import org.mockito.kotlin.mock
5960
import org.mockito.kotlin.whenever
6061

6162
@RunWith(AndroidJUnit4::class)
63+
@SuppressLint("DenyListedApi")
6264
class ContributesRemoteFeatureCodeGeneratorTest {
6365

6466
private val context: Context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext
@@ -2845,7 +2847,7 @@ class ContributesRemoteFeatureCodeGeneratorTest {
28452847
val feature = generatedFeatureNewInstance()
28462848

28472849
val privacyPlugin = (feature as PrivacyFeaturePlugin)
2848-
whenever(appBuildConfig.versionCode).thenReturn(1)
2850+
whenever(appBuildConfig.versionCode).thenReturn(2)
28492851

28502852
assertTrue(
28512853
privacyPlugin.store(
@@ -2906,9 +2908,336 @@ class ContributesRemoteFeatureCodeGeneratorTest {
29062908
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
29072909
assertNotNull(rawState?.assignedCohort)
29082910
assertNotNull(testFeature.fooFeature().getCohort())
2911+
assertTrue(testFeature.fooFeature().isEnabled(CONTROL))
2912+
assertTrue(testFeature.fooFeature().isEnrolled())
2913+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
2914+
assertTrue(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
2915+
}
2916+
2917+
@Test
2918+
fun `test cohort not assigned when remote feature is enabled and minSupportedVersion not matching`() {
2919+
val feature = generatedFeatureNewInstance()
2920+
2921+
val privacyPlugin = (feature as PrivacyFeaturePlugin)
2922+
whenever(appBuildConfig.versionCode).thenReturn(1)
2923+
2924+
assertTrue(
2925+
privacyPlugin.store(
2926+
"testFeature",
2927+
"""
2928+
{
2929+
"hash": "1",
2930+
"state": "disabled",
2931+
"features": {
2932+
"fooFeature": {
2933+
"state": "enabled",
2934+
"minSupportedVersion": 2,
2935+
"cohorts": [
2936+
{
2937+
"name": "control",
2938+
"weight": 1
2939+
},
2940+
{
2941+
"name": "blue",
2942+
"weight": 0
2943+
}
2944+
]
2945+
}
2946+
}
2947+
}
2948+
""".trimIndent(),
2949+
),
2950+
)
2951+
2952+
// we haven't called isEnabled yet, so cohorts should not be yet assigned
2953+
var rawState = testFeature.fooFeature().getRawStoredState()
2954+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
2955+
assertNull(rawState?.assignedCohort)
2956+
assertFalse(testFeature.fooFeature().isEnrolled())
2957+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
2958+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
2959+
2960+
// we call isEnabled() without cohort, cohort should not be assigned either
2961+
testFeature.fooFeature().isEnabled()
2962+
rawState = testFeature.fooFeature().getRawStoredState()
2963+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
2964+
assertNull(rawState?.assignedCohort)
2965+
assertNull(testFeature.fooFeature().getCohort())
2966+
assertFalse(testFeature.fooFeature().isEnrolled())
2967+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
2968+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
2969+
2970+
// we call isEnabled(cohort), then we should assign cohort
2971+
testFeature.fooFeature().isEnabled(BLUE)
2972+
rawState = testFeature.fooFeature().getRawStoredState()
2973+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
2974+
assertNull(rawState?.assignedCohort)
2975+
assertNull(testFeature.fooFeature().getCohort())
2976+
assertFalse(testFeature.fooFeature().isEnabled(CONTROL))
2977+
assertFalse(testFeature.fooFeature().isEnrolled())
2978+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
2979+
assertFalse(testFeature.fooFeature().isEnabled())
2980+
}
2981+
2982+
@Test
2983+
fun `test cohort not assigned when remote feature is disabled and minSupportedVersion is matching`() {
2984+
val feature = generatedFeatureNewInstance()
2985+
2986+
val privacyPlugin = (feature as PrivacyFeaturePlugin)
2987+
whenever(appBuildConfig.versionCode).thenReturn(2)
2988+
2989+
assertTrue(
2990+
privacyPlugin.store(
2991+
"testFeature",
2992+
"""
2993+
{
2994+
"hash": "1",
2995+
"state": "disabled",
2996+
"features": {
2997+
"fooFeature": {
2998+
"state": "disabled",
2999+
"minSupportedVersion": 2,
3000+
"cohorts": [
3001+
{
3002+
"name": "control",
3003+
"weight": 1
3004+
},
3005+
{
3006+
"name": "blue",
3007+
"weight": 0
3008+
}
3009+
]
3010+
}
3011+
}
3012+
}
3013+
""".trimIndent(),
3014+
),
3015+
)
3016+
3017+
// we haven't called isEnabled yet, so cohorts should not be yet assigned
3018+
var rawState = testFeature.fooFeature().getRawStoredState()
3019+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3020+
assertNull(rawState?.assignedCohort)
3021+
assertFalse(testFeature.fooFeature().isEnrolled())
3022+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3023+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3024+
3025+
// we call isEnabled() without cohort, cohort should not be assigned either
3026+
testFeature.fooFeature().isEnabled()
3027+
rawState = testFeature.fooFeature().getRawStoredState()
3028+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3029+
assertNull(rawState?.assignedCohort)
3030+
assertNull(testFeature.fooFeature().getCohort())
3031+
assertFalse(testFeature.fooFeature().isEnrolled())
3032+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3033+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3034+
3035+
// we call isEnabled(cohort), then we should assign cohort
3036+
testFeature.fooFeature().isEnabled(BLUE)
3037+
rawState = testFeature.fooFeature().getRawStoredState()
3038+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3039+
assertNull(rawState?.assignedCohort)
3040+
assertNull(testFeature.fooFeature().getCohort())
3041+
assertFalse(testFeature.fooFeature().isEnabled(CONTROL))
3042+
assertFalse(testFeature.fooFeature().isEnrolled())
3043+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3044+
assertFalse(testFeature.fooFeature().isEnabled())
3045+
}
3046+
3047+
@Test
3048+
fun `test cohort is assigned when remote feature is enabled and minSupportedVersion is matching`() {
3049+
val feature = generatedFeatureNewInstance()
3050+
3051+
val privacyPlugin = (feature as PrivacyFeaturePlugin)
3052+
whenever(appBuildConfig.versionCode).thenReturn(2)
3053+
3054+
assertTrue(
3055+
privacyPlugin.store(
3056+
"testFeature",
3057+
"""
3058+
{
3059+
"hash": "1",
3060+
"state": "disabled",
3061+
"features": {
3062+
"fooFeature": {
3063+
"state": "enabled",
3064+
"minSupportedVersion": 2,
3065+
"cohorts": [
3066+
{
3067+
"name": "control",
3068+
"weight": 1
3069+
},
3070+
{
3071+
"name": "blue",
3072+
"weight": 0
3073+
}
3074+
]
3075+
}
3076+
}
3077+
}
3078+
""".trimIndent(),
3079+
),
3080+
)
3081+
3082+
// we haven't called isEnabled yet, so cohorts should not be yet assigned
3083+
var rawState = testFeature.fooFeature().getRawStoredState()
3084+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3085+
assertNull(rawState?.assignedCohort)
3086+
assertFalse(testFeature.fooFeature().isEnrolled())
3087+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3088+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3089+
3090+
// we call isEnabled() without cohort, cohort should not be assigned either
3091+
testFeature.fooFeature().isEnabled()
3092+
rawState = testFeature.fooFeature().getRawStoredState()
3093+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3094+
assertNull(rawState?.assignedCohort)
3095+
assertNull(testFeature.fooFeature().getCohort())
3096+
assertFalse(testFeature.fooFeature().isEnrolled())
3097+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3098+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3099+
3100+
// we call isEnabled(cohort), then we should assign cohort
3101+
testFeature.fooFeature().isEnabled(BLUE)
3102+
rawState = testFeature.fooFeature().getRawStoredState()
3103+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3104+
assertNotNull(rawState?.assignedCohort)
3105+
assertNotNull(testFeature.fooFeature().getCohort())
3106+
assertTrue(testFeature.fooFeature().isEnabled(CONTROL))
3107+
assertTrue(testFeature.fooFeature().isEnrolled())
3108+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3109+
assertTrue(testFeature.fooFeature().isEnabled())
3110+
}
3111+
3112+
@Test
3113+
fun `test cohort is not assigned when flavor not matching is enabled and minSupportedVersion is matching`() {
3114+
val feature = generatedFeatureNewInstance()
3115+
3116+
val privacyPlugin = (feature as PrivacyFeaturePlugin)
3117+
whenever(appBuildConfig.versionCode).thenReturn(2)
3118+
3119+
assertTrue(
3120+
privacyPlugin.store(
3121+
"testFeature",
3122+
"""
3123+
{
3124+
"hash": "1",
3125+
"state": "disabled",
3126+
"features": {
3127+
"fooFeature": {
3128+
"state": "internal",
3129+
"minSupportedVersion": 2,
3130+
"cohorts": [
3131+
{
3132+
"name": "control",
3133+
"weight": 1
3134+
},
3135+
{
3136+
"name": "blue",
3137+
"weight": 0
3138+
}
3139+
]
3140+
}
3141+
}
3142+
}
3143+
""".trimIndent(),
3144+
),
3145+
)
3146+
3147+
// we haven't called isEnabled yet, so cohorts should not be yet assigned
3148+
var rawState = testFeature.fooFeature().getRawStoredState()
3149+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3150+
assertNull(rawState?.assignedCohort)
3151+
assertFalse(testFeature.fooFeature().isEnrolled())
3152+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3153+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3154+
3155+
// we call isEnabled() without cohort, cohort should not be assigned either
3156+
testFeature.fooFeature().isEnabled()
3157+
rawState = testFeature.fooFeature().getRawStoredState()
3158+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3159+
assertNull(rawState?.assignedCohort)
3160+
assertNull(testFeature.fooFeature().getCohort())
3161+
assertFalse(testFeature.fooFeature().isEnrolled())
3162+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3163+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3164+
3165+
// we call isEnabled(cohort), then we should assign cohort
3166+
testFeature.fooFeature().isEnabled(BLUE)
3167+
rawState = testFeature.fooFeature().getRawStoredState()
3168+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3169+
assertNull(rawState?.assignedCohort)
3170+
assertNull(testFeature.fooFeature().getCohort())
29093171
assertFalse(testFeature.fooFeature().isEnabled(CONTROL))
3172+
assertFalse(testFeature.fooFeature().isEnrolled())
3173+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3174+
assertFalse(testFeature.fooFeature().isEnabled())
3175+
}
3176+
3177+
@Test
3178+
fun `test cohort is assigned when flavor is matching is enabled and minSupportedVersion is matching`() {
3179+
val feature = generatedFeatureNewInstance()
3180+
3181+
val privacyPlugin = (feature as PrivacyFeaturePlugin)
3182+
whenever(appBuildConfig.versionCode).thenReturn(2)
3183+
whenever(appBuildConfig.flavor).thenReturn(INTERNAL)
3184+
3185+
assertTrue(
3186+
privacyPlugin.store(
3187+
"testFeature",
3188+
"""
3189+
{
3190+
"hash": "1",
3191+
"state": "disabled",
3192+
"features": {
3193+
"fooFeature": {
3194+
"state": "internal",
3195+
"minSupportedVersion": 2,
3196+
"cohorts": [
3197+
{
3198+
"name": "control",
3199+
"weight": 1
3200+
},
3201+
{
3202+
"name": "blue",
3203+
"weight": 0
3204+
}
3205+
]
3206+
}
3207+
}
3208+
}
3209+
""".trimIndent(),
3210+
),
3211+
)
3212+
3213+
// we haven't called isEnabled yet, so cohorts should not be yet assigned
3214+
var rawState = testFeature.fooFeature().getRawStoredState()
3215+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3216+
assertNull(rawState?.assignedCohort)
3217+
assertFalse(testFeature.fooFeature().isEnrolled())
3218+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3219+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3220+
3221+
// we call isEnabled() without cohort, cohort should not be assigned either
3222+
testFeature.fooFeature().isEnabled()
3223+
rawState = testFeature.fooFeature().getRawStoredState()
3224+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3225+
assertNull(rawState?.assignedCohort)
3226+
assertNull(testFeature.fooFeature().getCohort())
3227+
assertFalse(testFeature.fooFeature().isEnrolled())
3228+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(CONTROL))
3229+
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3230+
3231+
// we call isEnabled(cohort), then we should assign cohort
3232+
testFeature.fooFeature().isEnabled(BLUE)
3233+
rawState = testFeature.fooFeature().getRawStoredState()
3234+
assertNotEquals(emptyList<Cohort>(), rawState?.cohorts)
3235+
assertNotNull(rawState?.assignedCohort)
3236+
assertNotNull(testFeature.fooFeature().getCohort())
3237+
assertTrue(testFeature.fooFeature().isEnabled(CONTROL))
29103238
assertTrue(testFeature.fooFeature().isEnrolled())
29113239
assertFalse(testFeature.fooFeature().isEnrolledAndEnabled(BLUE))
3240+
assertTrue(testFeature.fooFeature().isEnabled())
29123241
}
29133242

29143243
@Test

0 commit comments

Comments
 (0)