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.

Sign up for GitHub

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

Remove api dependencies from privacy-config-api and vpn-api modules #2103

Merged
merged 4 commits into from
Jul 28, 2022
Merged

Remove api dependencies from privacy-config-api and vpn-api modules #2103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5949712
Remove api dependencies from privacy-config-api and vpn-api modules
marcosholgado Jul 27, 2022
13a3f59
refactor
marcosholgado Jul 27, 2022
49e7cec
Fix test
marcosholgado Jul 28, 2022
58411a6
Fix test
marcosholgado Jul 28, 2022
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
Remove api dependencies from privacy-config-api and vpn-api modules
  • Loading branch information
@marcosholgado
marcosholgado committed Jul 27, 2022
commit 59497120b29888afc73062da2de6f312f9c97719
2 changes: 1 addition & 1 deletion app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4188,7 +4188,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.value), any())).thenReturn(true)
whenever(mockGpcRepository.isGpcEnabled()).thenReturn(true)
whenever(mockGpcRepository.exceptions).thenReturn(exceptions)
}
Expand Down
8 changes: 4 additions & 4 deletions app/src/androidTest/java/com/duckduckgo/app/email/EmailInjectorJsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class EmailInjectorJsTest {
testee =
EmailInjectorJs(mockEmailManager, DuckDuckGoUrlDetector(), mockDispatcherProvider, mockFeatureToggle, javascriptInjector, mockAutofill)

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

Expand All @@ -68,7 +68,7 @@ class EmailInjectorJsTest {
@Test
@SdkSuppress(minSdkVersion = 24)
fun whenInjectAddressAndFeatureIsDisabledThenJsCodeNotInjected() {
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName.value)).thenReturn(false)

val address = "address"
val webView = spy(WebView(InstrumentationRegistry.getInstrumentation().targetContext))
Expand Down Expand Up @@ -123,7 +123,7 @@ class EmailInjectorJsTest {
@SdkSuppress(minSdkVersion = 24)
fun whenNotifyWebAppSignEventAndUrlIsFromDuckDuckGoAndFeatureIsDisabledAndEmailIsNotSignedInThenDoNotEvaluateJsCode() {
whenever(mockEmailManager.isSignedIn()).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName.value)).thenReturn(false)

val jsToEvaluate = getNotifySignOutJsToEvaluate()
val webView = spy(WebView(InstrumentationRegistry.getInstrumentation().targetContext))
Expand All @@ -138,7 +138,7 @@ class EmailInjectorJsTest {
@SdkSuppress(minSdkVersion = 24)
fun whenNotifyWebAppSignEventAndUrlIsFromDuckDuckGoAndFeatureIsEnabledAndEmailIsNotSignedInThenEvaluateJsCode() {
whenever(mockEmailManager.isSignedIn()).thenReturn(false)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName)).thenReturn(true)
whenever(mockFeatureToggle.isFeatureEnabled(AutofillFeatureName.value)).thenReturn(true)

val jsToEvaluate = getNotifySignOutJsToEvaluate()
val webView = spy(WebView(InstrumentationRegistry.getInstrumentation().targetContext))
Expand Down
3 changes: 1 addition & 2 deletions app/src/androidTest/java/com/duckduckgo/app/fakes/FeatureToggleFake.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,11 @@

package com.duckduckgo.app.fakes

import com.duckduckgo.feature.toggles.api.FeatureName
import com.duckduckgo.feature.toggles.api.FeatureToggle

class FeatureToggleFake : FeatureToggle {
override fun isFeatureEnabled(
featureName: FeatureName,
featureName: String,
defaultValue: Boolean
): Boolean = true
}
4 changes: 2 additions & 2 deletions app/src/androidTest/java/com/duckduckgo/app/httpsupgrade/HttpsUpgraderTest.kt
View file
Open in desktop
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.value)).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.value)).thenReturn(false)
bloomFilter.add("www.local.url")
assertFalse(testee.shouldUpgrade(Uri.parse("http://www.local.url")))
}
Expand Down
2 changes: 1 addition & 1 deletion app/src/androidTest/java/com/duckduckgo/app/referencetests/HttpsReferenceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,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.value, isEnabled)).thenReturn(isEnabled)
whenever(mockHttpsRepository.exceptions).thenReturn(CopyOnWriteArrayList(httpsExceptions))
whenever(mockUnprotectedTemporaryRepository.exceptions).thenReturn(exceptionsUnprotectedTemporary)

Expand Down
2 changes: 1 addition & 1 deletion ...c/androidTestPlay/java/com/duckduckgo/app/integration/HttpsEmbeddedDataIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class HttpsEmbeddedDataIntegrationTest {

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

db = Room.inMemoryDatabaseBuilder(context, AppDatabase::class.java)
.allowMainThreadQueries()
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/brokensite/api/BrokenSiteSender.kt
View file
Open in desktop
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) && gpc.isEnabled()).toString()
val isGpcEnabled = (featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName.value) && gpc.isEnabled()).toString()
val absoluteUrl = Uri.parse(brokenSite.siteUrl).absoluteString

appCoroutineScope.launch(dispatcherProvider.io()) {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/browser/useragent/UserAgentProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class UserAgentProvider constructor(

val isDomainInUserAllowList = isHostInUserAllowedList(host)

if (isDomainInUserAllowList || !toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName) || shouldUseDefaultUserAgent) {
if (isDomainInUserAllowList || !toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName.value) || shouldUseDefaultUserAgent) {
return if (isDesktop) {
defaultUserAgent.get().replace(AgentRegex.platform, desktopPrefix)
} else {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/email/EmailInjector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class EmailInjectorJs(
}
}

private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName, defaultValue = true)
private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName.value, defaultValue = true)

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

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/email/EmailJavascriptInterface.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class EmailJavascriptInterface(
return (url != null && urlDetector.isDuckDuckGoEmailUrl(url))
}

private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName, defaultValue = true)
private fun isFeatureEnabled() = featureToggle.isFeatureEnabled(PrivacyFeatureName.AutofillFeatureName.value, defaultValue = true)

@JavascriptInterface
fun isSignedIn(): String {
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/com/duckduckgo/app/globalprivacycontrol/ui/GlobalPrivacyControlViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class GlobalPrivacyControlViewModel @Inject constructor(
init {
_viewState.value = ViewState(
globalPrivacyControlEnabled = gpc.isEnabled(),
globalPrivacyControlFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName, true)
globalPrivacyControlFeatureEnabled = featureToggle.isFeatureEnabled(PrivacyFeatureName.GpcFeatureName.value, true)
)
pixel.fire(SETTINGS_DO_NOT_SELL_SHOWN)
}
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/httpsupgrade/HttpsUpgrader.kt
View file
Open in desktop
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)) {
if (!toggle.isFeatureEnabled(PrivacyFeatureName.HttpsFeatureName.value)) {
Timber.d("https is disabled in the remote config and so $host is not upgradable")
return false
}
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/com/duckduckgo/app/settings/SettingsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class SettingsViewModel @Inject constructor(
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.value),
appLinksSettingType = getAppLinksSettingsState(settingsDataStore.appLinksEnabled, settingsDataStore.showAppLinksPrompt),
appTrackingProtectionEnabled = TrackerBlockingVpnService.isServiceRunning(appContext),
appTrackingProtectionWaitlistState = atpRepository.getState(),
Expand Down
4 changes: 2 additions & 2 deletions app/src/test/java/com/duckduckgo/app/browser/useragent/UserAgentProviderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class UserAgentProviderTest {
@Before
fun before() {
whenever(deviceInfo.majorAppVersion).thenReturn("5")
whenever(toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName)).thenReturn(true)
whenever(toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName.value)).thenReturn(true)
whenever(userAgentRepository.defaultExceptions).thenReturn(defaultExceptions)
whenever(userAgentRepository.omitApplicationExceptions).thenReturn(applicationExceptions)
whenever(userAgentRepository.omitVersionExceptions).thenReturn(versionExceptions)
Expand Down Expand Up @@ -174,7 +174,7 @@ class UserAgentProviderTest {

@Test
fun whenFeatureIsDisabledThenUaIsDefault() {
whenever(toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName)).thenReturn(false)
whenever(toggle.isFeatureEnabled(PrivacyFeatureName.UserAgentFeatureName.value)).thenReturn(false)
testee = getUserAgentProvider(Agent.DEFAULT, deviceInfo)
val actual = testee.userAgent(DOMAIN)
assertTrue("$actual does not match expected regex", ValidationRegex.default.matches(actual))
Expand Down
4 changes: 2 additions & 2 deletions app/src/test/java/com/duckduckgo/app/email/EmailJavascriptInterfaceTest.kt
View file
Open in desktop
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.value)).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.value)).thenReturn(false)

testee.showTooltip()

Expand Down
6 changes: 3 additions & 3 deletions app/src/test/java/com/duckduckgo/app/settings/SettingsViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ class SettingsViewModelTest {

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

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

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

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

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

Expand Down
30 changes: 1 addition & 29 deletions ...les/feature-toggles-api/src/main/java/com/duckduckgo/feature/toggles/api/FeatureToggle.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,35 +24,7 @@ interface FeatureToggle {
* @throws [IllegalArgumentException] if the feature is not implemented
*/
fun isFeatureEnabled(
featureName: FeatureName,
featureName: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some doubts about this change:
I liked using a specific type instead of a primitive here. So I wonder if we should see this from a different perspective instead of finding a way to break the api dependency.

How I see it:

  • FeatureName restricts the possible value we pass into that function
  • FeatureName clearly indicates that we have a set of features that can be enabled/disabled
  • We had some method interfaces that used FeatureName as arguments.
  • Then I see, that different modules can enable/disable features. I expect those modules to contribute to the app values that implement FeatureName
  • Maybe this is a different type of api. Saying this because I see a difference between: modules that own some logic and we are only consumers, vs modules that we can contribute to.
  • In this case I think we are contributors in a way.

I see this very similar to how we consume statistics module for Pixels. We have a bunch of modules that are extending Pixel.PixelName interface, and I think that's correct.

Happy to know more about the approach here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The rational (trade-off) behind is is the following (assuming we don't want to break the api not depending in api rule):

  • The reason to have a the FeatureName type is to have type-safe code as much as possible
  • The PrivacyFeaturePlugin plugins are internally not type-safe anyway because they have to do FeatureName.value. So in that sense, a String instead of a FeatureName doesn't make a difference
  • So now it comes to the caller of the isFeatureEnabled
  • In this case, the idea is that even though that isFeatureEnabled is now taking a String as a parameter, every feature-api module will expose an enum listing all its supported features
  • Callers using that enum are still as type-safe as before
  • The only nuance is that, having a FeatureName type in the isFeatureEnable signature makes clear(er) in the that the caller should look for a FeatureName (extended) type
  • And that's the only thing we're giving up with this trade-off
  • Which I think it's easily solvable with just Javadocs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What @aitorvs said plus... you could also do FeatureName { "something" } before so it wasn't safe at all :). In any of the cases the app will crash if you use a featureName that doesn't exist but that's explained in the Javadocs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree "it's solvable", and that FeatureName is replaceable by String with the current logic. The other arguments about type-safe are implementation details and I wouldn't focus too much there.

My point is that we are going to have similar scenarios in the future where we contribute to a module rather than just consume the module. As I said, Statistics module is a similar example.

So I'm not advocating we should allow depending on other modules api, I'm advocating we should have another type of module (not sure if we consider Statistics as an api module or not). And feature-toggles should be that type of module where others can depend on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my head the only way you contribute to a module is usually done in the impl module. In your statistics example, the macOS waitlist needs that module, but the dependency happens in the macos-impl module rather than the macos-api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PrivacyFeaturePlugin is defined in the privacy-config-api module so it cannot have FeatureName because that was part of the feature-toggles-api module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm sorry I see my comment was confusing. I wanted to ask if we could have moved to PrivacyFeatureName instead of String when replacing FeatureName

Copy link
Contributor Author

@marcosholgado marcosholgado Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with using PrivacyFeatureName in the PrivacyFeaturePlugin would be the same. We have modules (i.e. vpn and soon ad_click) that contribute to the plugin but by enforcing to use PrivacyFeatureName we would end up with the same api dependency if we wanted to expose their specific feature names (i.e. AppTpFeatureName)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was assuming that we will only "contribute" from those modules (vpn and ad_click), and never expose anything from api module.

In any case, it was just a question to see if I was getting the big picture here, no worries. Thanks for your time explaining this in more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that was the case then yes, you could use PrivacyFeatureName in the plugin and extend in the impl modules to create more specific feature names but we want to expose those features (like AppFeatureName) from the different api modules.

defaultValue: Boolean = true
): Boolean
}

/**
* Each feature toggle created needs a [FeatureName] which can be implemented using this interface
*/
interface FeatureName {
val value: String

companion object {
/**
* Utility function to create a [FeatureName] from the passed in [block] lambda
* instead of using the anonymous `object : FeatureName` syntax.
*
* Usage:
*
* ```kotlin
* val feature = FeatureName {
*
* }
* ```
*/
inline operator fun invoke(crossinline block: () -> String): FeatureName {
return object : FeatureName {
override val value: String
get() = block()
}
}
}
}
2 changes: 1 addition & 1 deletion ...ture-toggles-api/src/main/java/com/duckduckgo/feature/toggles/api/FeatureTogglesPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface FeatureTogglesPlugin {
* know the featureName. [defaultValue] if the plugin knows featureName but is not set
*/
fun isEnabled(
featureName: FeatureName,
featureName: String,
defaultValue: Boolean
): Boolean?
}
5 changes: 2 additions & 3 deletions ...-toggles-impl/src/main/java/com/duckduckgo/feature/toggles/impl/RealFeatureTogglesImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import com.duckduckgo.app.global.plugins.PluginPoint
import com.duckduckgo.di.scopes.AppScope
import com.duckduckgo.feature.toggles.api.FeatureToggle
import com.duckduckgo.feature.toggles.api.FeatureTogglesPlugin
import com.duckduckgo.feature.toggles.api.FeatureName
import com.squareup.anvil.annotations.ContributesBinding
import javax.inject.Inject
import dagger.SingleInstanceIn
Expand All @@ -32,14 +31,14 @@ class RealFeatureToggleImpl @Inject constructor(private val featureTogglesPlugin
FeatureToggle {

override fun isFeatureEnabled(
featureName: FeatureName,
featureName: String,
defaultValue: Boolean
): Boolean {
featureTogglesPluginPoint.getPlugins().forEach { plugin ->
plugin.isEnabled(featureName, defaultValue)?.let { return it }
}

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

Expand Down
3 changes: 0 additions & 3 deletions privacy-config/privacy-config-api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ java {
}

dependencies {

implementation project(path: ':feature-toggles-api')

implementation Google.dagger
implementation Kotlin.stdlib.jdk7
}
Expand Down
6 changes: 2 additions & 4 deletions .../privacy-config-api/src/main/java/com/duckduckgo/privacy/config/api/PrivacyFeatureName.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@

package com.duckduckgo.privacy.config.api

import com.duckduckgo.feature.toggles.api.FeatureName

/** List of [FeatureName] that belong to the Privacy Configuration */
enum class PrivacyFeatureName(override val value: String) : FeatureName {
/** List of [PrivacyFeatureName] that belong to the Privacy Configuration */
enum class PrivacyFeatureName(val value: String) {
ContentBlockingFeatureName("contentBlocking"),
GpcFeatureName("gpc"),
HttpsFeatureName("https"),
Expand Down
10 changes: 4 additions & 6 deletions ...rivacy-config-api/src/main/java/com/duckduckgo/privacy/config/api/PrivacyFeaturePlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package com.duckduckgo.privacy.config.api

import com.duckduckgo.feature.toggles.api.FeatureName

/**
* Implement this interface and contribute it as a multibinding to get called upon downloading remote privacy config
*
Expand All @@ -32,15 +30,15 @@ import com.duckduckgo.feature.toggles.api.FeatureName
*/
interface PrivacyFeaturePlugin {
/**
* @return `true` when the feature config was stored, otherwise `false`
* @return `true` when the feature was stored, otherwise `false`
*/
fun store(
name: FeatureName,
featureName: String,
jsonString: String
): Boolean

/**
* @return the [FeatureName] of this feature
* @return the [featureName] of this feature
*/
val featureName: FeatureName
val featureName: String
}
5 changes: 2 additions & 3 deletions ...cy-config-impl/src/main/java/com/duckduckgo/privacy/config/impl/PrivacyConfigPersister.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ 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.feature.toggles.api.FeatureName
import com.duckduckgo.privacy.config.impl.models.JsonPrivacyConfig
import com.duckduckgo.privacy.config.api.PrivacyFeaturePlugin
import com.duckduckgo.privacy.config.store.PrivacyConfig
Expand Down Expand Up @@ -58,8 +57,8 @@ class RealPrivacyConfigPersister @Inject constructor(
unprotectedTemporaryRepository.updateAll(jsonPrivacyConfig.unprotectedTemporary)
jsonPrivacyConfig.features.forEach { feature ->
feature.value?.let { jsonObject ->
privacyFeaturePluginPoint.getPlugins().firstOrNull { feature.key == it.featureName.value }?.let { featurePlugin ->
featurePlugin.store(FeatureName { feature.key }, jsonObject.toString())
privacyFeaturePluginPoint.getPlugins().firstOrNull { feature.key == it.featureName }?.let { featurePlugin ->
featurePlugin.store(featurePlugin.featureName, jsonObject.toString())
}
}
}
Expand Down
12 changes: 5 additions & 7 deletions ...impl/src/main/java/com/duckduckgo/privacy/config/impl/features/amplinks/AmpLinksPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.duckduckgo.privacy.config.impl.features.amplinks

import com.duckduckgo.di.scopes.AppScope
import com.duckduckgo.feature.toggles.api.FeatureName
import com.duckduckgo.privacy.config.api.PrivacyFeatureName
import com.duckduckgo.privacy.config.impl.features.privacyFeatureValueOf
import com.duckduckgo.privacy.config.api.PrivacyFeaturePlugin
Expand All @@ -34,10 +33,9 @@ class AmpLinksPlugin @Inject constructor(
private val privacyFeatureTogglesRepository: PrivacyFeatureTogglesRepository
) : PrivacyFeaturePlugin {

override fun store(name: FeatureName, jsonString: String): Boolean {
@Suppress("NAME_SHADOWING")
val name = privacyFeatureValueOf(name.value)
if (name == featureName) {
override fun store(featureName: String, jsonString: String): Boolean {
val privacyFeature = privacyFeatureValueOf(featureName) ?: return false
if (privacyFeature.value == this.featureName) {
val moshi = Moshi.Builder().build()
val jsonAdapter: JsonAdapter<AmpLinksFeature> =
moshi.adapter(AmpLinksFeature::class.java)
Expand All @@ -62,11 +60,11 @@ class AmpLinksPlugin @Inject constructor(

ampLinksRepository.updateAll(exceptions, ampLinkFormats, ampKeywords)
val isEnabled = ampLinksFeature?.state == "enabled"
privacyFeatureTogglesRepository.insert(PrivacyFeatureToggles(name, isEnabled, ampLinksFeature?.minSupportedVersion))
privacyFeatureTogglesRepository.insert(PrivacyFeatureToggles(this.featureName, isEnabled, ampLinksFeature?.minSupportedVersion))
return true
}
return false
}

override val featureName: PrivacyFeatureName = PrivacyFeatureName.AmpLinksFeatureName
override val featureName: String = PrivacyFeatureName.AmpLinksFeatureName.value
}
Loading