Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve feature toggle API to actually use default value #1822

Merged
merged 4 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Improve feature toggle API to actually use default value
  • Loading branch information
aitorvs committed Mar 22, 2022
commit f9183658c8b412ff0484560c5fad97c91629b1bb
Original file line number Diff line number Diff line change
Expand Up @@ -3945,7 +3945,7 @@ class BrowserTabViewModelTest {

private fun givenUrlCannotUseGpc(url: String) {
val exceptions = CopyOnWriteArrayList<GpcException>().apply { add(GpcException(url)) }
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName()), any())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName), any())).thenReturn(true)
whenever(mockGpcRepository.isGpcEnabled()).thenReturn(true)
whenever(mockGpcRepository.exceptions).thenReturn(exceptions)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class EmailInjectorJsTest {
fun setup() {
testee = EmailInjectorJs(mockEmailManager, DuckDuckGoUrlDetector(), mockDispatcherProvider, mockFeatureToggle, mockAutofill)

whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(true)
whenever(mockAutofill.isAnException(any())).thenReturn(false)
}

Expand Down Expand Up @@ -102,7 +102,7 @@ class EmailInjectorJsTest {
@SdkSuppress(minSdkVersion = 24)
fun whenInjectEmailAutofillJsAndUrlIsFromDuckDuckGoAndFeatureIsDisabledThenInjectJsCode() {
whenever(mockEmailManager.isSignedIn()).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName())).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(false)

val jsToEvaluate = getJsToEvaluate()
val webView = spy(WebView(InstrumentationRegistry.getInstrumentation().targetContext))
Expand Down Expand Up @@ -144,7 +144,7 @@ class EmailInjectorJsTest {
@Test
@SdkSuppress(minSdkVersion = 24)
fun whenInjectAddressAndFeatureIsDisabledThenJsCodeNotInjected() {
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName())).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(false)

val address = "address"
val webView = spy(WebView(InstrumentationRegistry.getInstrumentation().targetContext))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ class HttpsUpgraderTest {
@Before
fun before() {
whenever(mockHttpsBloomFilterFactory.create()).thenReturn(bloomFilter)
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName)).thenReturn(true)
testee = HttpsUpgraderImpl(mockHttpsBloomFilterFactory, mockBloomFalsePositiveListDao, mockUserAllowlistDao, mockFeatureToggle, mockHttps)
testee.reloadData()
}

@Test
fun whenFeatureIsDisableTheShouldNotUpgrade() {
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName())).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName)).thenReturn(false)
bloomFilter.add("www.local.url")
assertFalse(testee.shouldUpgrade(Uri.parse("http://www.local.url")))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class HttpsEmbeddedDataIntegrationTest {

@Before
fun before() {
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName)).thenReturn(true)

db = Room.inMemoryDatabaseBuilder(context, AppDatabase::class.java)
.allowMainThreadQueries()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class HttpsReferenceTest(private val testCase: TestCase) {
val isEnabled = httpsFeature?.state == "enabled"
val exceptionsUnprotectedTemporary = CopyOnWriteArrayList(config?.unprotectedTemporary ?: emptyList())

whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName(), isEnabled)).thenReturn(isEnabled)
whenever(mockFeatureToggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName, isEnabled)).thenReturn(isEnabled)
whenever(mockHttpsRepository.exceptions).thenReturn(CopyOnWriteArrayList(httpsExceptions))
whenever(mockUnprotectedTemporaryRepository.exceptions).thenReturn(exceptionsUnprotectedTemporary)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class BrokenSiteSubmitter(
) : BrokenSiteSender {

override fun submitBrokenSiteFeedback(brokenSite: BrokenSite) {
val isGpcEnabled = (featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName()) == true && gpc.isEnabled()).toString()
val isGpcEnabled = (featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName) == true && gpc.isEnabled()).toString()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We don't need == true anymore

val absoluteUrl = Uri.parse(brokenSite.siteUrl).absoluteString

appCoroutineScope.launch(dispatcherProvider.io()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class EmailInjectorJs(
}
}

private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName(), defaultValue = true) ?: false
private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName, defaultValue = true)

private fun isDuckDuckGoUrl(url: String?): Boolean = (url != null && urlDetector.isDuckDuckGoEmailUrl(url))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class EmailJavascriptInterface(
return (url != null && urlDetector.isDuckDuckGoEmailUrl(url))
}

private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName(), defaultValue = true) ?: false
private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName, defaultValue = true)

@JavascriptInterface
fun isSignedIn(): String {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class GlobalPrivacyControlViewModel(
init {
_viewState.value = ViewState(
globalPrivacyControlEnabled = gpc.isEnabled(),
globalPrivacyControlFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName(), true) == true
globalPrivacyControlFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName, true) == true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We don't need == true anymore

)
pixel.fire(SETTINGS_DO_NOT_SELL_SHOWN)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class HttpsUpgraderImpl @Inject constructor(
override fun shouldUpgrade(uri: Uri): Boolean {
val host = uri.host ?: return false

if (toggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName()) != true) {
if (toggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName) != true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This could now be if (!toggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName)) {

Timber.d("https is disabled in the remote config and so $host is not upgradable")
return false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class SettingsViewModel(
automaticallyClearData = AutomaticallyClearData(automaticallyClearWhat, automaticallyClearWhen, automaticallyClearWhenEnabled),
appIcon = settingsDataStore.appIcon,
selectedFireAnimation = settingsDataStore.selectedFireAnimation,
globalPrivacyControlEnabled = gpc.isEnabled() && featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName()) == true,
globalPrivacyControlEnabled = gpc.isEnabled() && featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName) == true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We don't need == true anymore

appLinksSettingType = getAppLinksSettingsState(settingsDataStore.appLinksEnabled, settingsDataStore.showAppLinksPrompt),
appTrackingProtectionEnabled = TrackerBlockingVpnService.isServiceRunning(appContext),
appTrackingProtectionWaitlistState = atpRepository.getState()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class EmailJavascriptInterfaceTest {
mockAutofill
) { counter++ }

whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(true)
whenever(mockAutofill.isAnException(any())).thenReturn(false)
}

Expand Down Expand Up @@ -130,7 +130,7 @@ class EmailJavascriptInterfaceTest {
@Test
fun whenShowTooltipAndFeatureDisabledThenLambdaNotCalled() {
whenever(mockWebView.url).thenReturn(NON_EMAIL_URL)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName())).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(false)

testee.showTooltip()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class SettingsViewModelTest {

@Test
fun whenStartIfGpcToggleDisabledAndGpcEnabledThenGpgDisabled() = runTest {
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName()), any())).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName), any())).thenReturn(false)
whenever(mockGpc.isEnabled()).thenReturn(true)

testee.start()
Expand All @@ -152,7 +152,7 @@ class SettingsViewModelTest {

@Test
fun whenStartIfGpcToggleEnabledAndGpcDisabledThenGpgDisabled() = runTest {
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName()), any())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName), any())).thenReturn(true)
whenever(mockGpc.isEnabled()).thenReturn(false)
testee.start()

Expand All @@ -164,7 +164,7 @@ class SettingsViewModelTest {

@Test
fun whenStartIfGpcToggleEnabledAndGpcEnabledThenGpgEnabled() = runTest {
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName()), any())).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(eq(PrivacyFeatureName.GpcFeatureName), any())).thenReturn(true)
whenever(mockGpc.isEnabled()).thenReturn(true)
testee.start()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ package com.duckduckgo.feature.toggles.api
interface FeatureToggle {
/**
* This method takes a [featureName] and optionally a default value.
* @return `true` if the feature is enabled, `false` if is not and `null` if the feature does
* not exist.
* @return `true` if the feature is enabled, `false` if is not
* @throws [IllegalArgumentException] if the feature is not implemented
*/
fun isFeatureEnabled(
featureName: FeatureName,
defaultValue: Boolean = true
): Boolean?
): Boolean
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@ class RealFeatureToggleImpl @Inject constructor(private val featureTogglesPlugin
override fun isFeatureEnabled(
featureName: FeatureName,
defaultValue: Boolean
): Boolean? {
): Boolean {
featureTogglesPluginPoint.getPlugins().forEach { plugin ->
plugin.isEnabled(featureName, defaultValue)?.let { return it }
}
return null

throw IllegalArgumentException("Unknown feature: ${featureName.value}")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ class RealFeatureToggleImplTest {
fun whenFeatureNameCanBeHandledByPluginThenReturnTheCorrectValue() {
val result = testee.isFeatureEnabled(TrueFeatureName(), false)
assertNotNull(result)
assertTrue(result!!)
assertTrue(result)
}

@Test
@Test(expected = IllegalArgumentException::class)
fun whenFeatureNameCannotBeHandledByAnyPluginThenReturnNull() {
aitorvs marked this conversation as resolved.
Show resolved Hide resolved
val result = testee.isFeatureEnabled(NullFeatureName(), false)
assertNull(result)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,20 @@ package com.duckduckgo.privacy.config.api
import com.duckduckgo.feature.toggles.api.FeatureName

/** List of [FeatureName] that belong to the Privacy Configuration */
sealed class PrivacyFeatureName(override val value: String) : FeatureName {
data class ContentBlockingFeatureName(override val value: String = "contentBlocking") :
PrivacyFeatureName(value)

data class GpcFeatureName(override val value: String = "gpc") : PrivacyFeatureName(value)
data class HttpsFeatureName(override val value: String = "https") : PrivacyFeatureName(value)
data class TrackerAllowlistFeatureName(override val value: String = "trackerAllowlist") :
PrivacyFeatureName(value)

data class DrmFeatureName(override val value: String = "eme") : PrivacyFeatureName(value)
data class AmpLinksFeatureName(override val value: String = "ampLinks") :
PrivacyFeatureName(value)
enum class PrivacyFeatureName(override val value: String) : FeatureName {
ContentBlockingFeatureName("contentBlocking"),
GpcFeatureName("gpc"),
HttpsFeatureName("https"),
TrackerAllowlistFeatureName("trackerAllowlist"),
DrmFeatureName("eme"),
AmpLinksFeatureName("ampLinks"),
TrackingParametersFeatureName("trackingParameters"),
AutofillFeatureName("autofill"),
}

data class TrackingParametersFeatureName(override val value: String = "trackingParameters") :
PrivacyFeatureName(value)
data class AutofillFeatureName(override val value: String = "autofill") : PrivacyFeatureName(value)
/**
* Convenience method to get the [PrivacyFeatureName] from its [String] value
*/
fun privacyFeatureValueOf(value: String): PrivacyFeatureName? {
return PrivacyFeatureName.values().find { it.value == value }
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.duckduckgo.privacy.config.impl
import androidx.annotation.WorkerThread
import com.duckduckgo.app.global.plugins.PluginPoint
import com.duckduckgo.di.scopes.AppScope
import com.duckduckgo.privacy.config.api.privacyFeatureValueOf
import com.duckduckgo.privacy.config.impl.models.JsonPrivacyConfig
import com.duckduckgo.privacy.config.impl.plugins.PrivacyFeaturePlugin
import com.duckduckgo.privacy.config.store.PrivacyConfig
Expand Down Expand Up @@ -58,7 +59,9 @@ class RealPrivacyConfigPersister @Inject constructor(
jsonPrivacyConfig.features.forEach { feature ->
feature.value?.let { jsonObject ->
privacyFeaturePluginPoint.getPlugins().forEach { plugin ->
plugin.store(feature.key, jsonObject.toString())
privacyFeatureValueOf(feature.key)?.let {
plugin.store(it, jsonObject.toString())
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ class AmpLinksPlugin @Inject constructor(
private val privacyFeatureTogglesRepository: PrivacyFeatureTogglesRepository
) : PrivacyFeaturePlugin {

override fun store(name: String, jsonString: String): Boolean {
if (name == featureName.value) {
override fun store(name: PrivacyFeatureName, jsonString: String): Boolean {
if (name == featureName) {
val moshi = Moshi.Builder().build()
val jsonAdapter: JsonAdapter<AmpLinksFeature> =
moshi.adapter(AmpLinksFeature::class.java)
Expand Down Expand Up @@ -64,5 +64,5 @@ class AmpLinksPlugin @Inject constructor(
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.AmpLinksFeatureName()
override val featureName: PrivacyFeatureName = PrivacyFeatureName.AmpLinksFeatureName
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class RealAmpLinks @Inject constructor(
}

override fun extractCanonicalFromAmpLink(url: String): AmpLinkType? {
if (featureToggle.isFeatureEnabled(PrivacyFeatureName.AmpLinksFeatureName()) == false) return null
if (featureToggle.isFeatureEnabled(PrivacyFeatureName.AmpLinksFeatureName) == false) return null
aitorvs marked this conversation as resolved.
Show resolved Hide resolved
if (url == lastExtractedUrl) return null

val extractedUrl = extractCanonical(url)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class AutofillPlugin @Inject constructor(
) : PrivacyFeaturePlugin {

override fun store(
name: String,
name: PrivacyFeatureName,
jsonString: String
): Boolean {
if (name == featureName.value) {
if (name == featureName) {
val autofillExceptions = mutableListOf<AutofillExceptionEntity>()
val moshi = Moshi.Builder().build()
val jsonAdapter: JsonAdapter<AutofillFeature> =
Expand All @@ -56,5 +56,5 @@ class AutofillPlugin @Inject constructor(
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.AutofillFeatureName()
override val featureName: PrivacyFeatureName = PrivacyFeatureName.AutofillFeatureName
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class ContentBlockingPlugin @Inject constructor(
) : PrivacyFeaturePlugin {

override fun store(
name: String,
name: PrivacyFeatureName,
jsonString: String
): Boolean {
if (name == featureName.value) {
if (name == featureName) {
val contentBlockingExceptions = mutableListOf<ContentBlockingExceptionEntity>()
val moshi = Moshi.Builder().build()
val jsonAdapter: JsonAdapter<ContentBlockingFeature> =
Expand All @@ -56,5 +56,5 @@ class ContentBlockingPlugin @Inject constructor(
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.ContentBlockingFeatureName()
override val featureName: PrivacyFeatureName = PrivacyFeatureName.ContentBlockingFeatureName
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class RealContentBlocking @Inject constructor(
) : ContentBlocking {

override fun isAnException(url: String): Boolean {
return if (featureToggle.isFeatureEnabled(PrivacyFeatureName.ContentBlockingFeatureName(), true) == true) {
return if (featureToggle.isFeatureEnabled(PrivacyFeatureName.ContentBlockingFeatureName, true) == true) {
unprotectedTemporary.isAnException(url) || matches(url)
} else {
false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class DrmPlugin @Inject constructor(
) : PrivacyFeaturePlugin {

override fun store(
name: String,
name: PrivacyFeatureName,
jsonString: String
): Boolean {
if (name == featureName.value) {
if (name == featureName) {
val drmExceptions = mutableListOf<DrmExceptionEntity>()
val moshi = Moshi.Builder().build()
val jsonAdapter: JsonAdapter<DrmFeature> =
Expand All @@ -56,5 +56,5 @@ class DrmPlugin @Inject constructor(
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.DrmFeatureName()
override val featureName: PrivacyFeatureName = PrivacyFeatureName.DrmFeatureName
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class RealDrm @Inject constructor(
}

private fun shouldEnableDrmForUri(uri: Uri): Boolean {
val isFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.DrmFeatureName(), defaultValue = true) ?: false
val isFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.DrmFeatureName, defaultValue = true)
return isFeatureEnabled && domainsThatAllowDrm(uri.baseHost)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ class GpcPlugin @Inject constructor(
) : PrivacyFeaturePlugin {

override fun store(
name: String,
name: PrivacyFeatureName,
jsonString: String
): Boolean {
if (name == featureName.value) {
if (name == featureName) {
val gpcExceptions = mutableListOf<GpcExceptionEntity>()
val gpcHeaders = mutableListOf<GpcHeaderEnabledSiteEntity>()
val moshi = Moshi.Builder().build()
Expand All @@ -61,5 +61,5 @@ class GpcPlugin @Inject constructor(
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.GpcFeatureName()
override val featureName: PrivacyFeatureName = PrivacyFeatureName.GpcFeatureName
}
Loading