From 75e7cd3c64d0a5716751dde6c6121f54fc5dca15 Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Fri, 28 Aug 2020 11:02:07 -0700 Subject: [PATCH] =?UTF-8?q?FNX-14583=20=E2=81=83=20Extract=20and=20test=20?= =?UTF-8?q?preference=20helpers=20for=20Settings=20(#13402)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../components/settings/CounterPreference.kt | 31 ++++ .../settings/FeatureFlagPreference.kt | 25 ++++ .../fenix/utils/FeatureFlagPreference.kt | 29 ---- .../java/org/mozilla/fenix/utils/Settings.kt | 133 ++++++------------ .../settings/CounterPreferenceTest.kt | 63 +++++++++ .../settings/FeatureFlagPreferenceTest.kt | 67 +++++++++ .../org/mozilla/fenix/utils/SettingsTest.kt | 8 +- 7 files changed, 231 insertions(+), 125 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/components/settings/CounterPreference.kt create mode 100644 app/src/main/java/org/mozilla/fenix/components/settings/FeatureFlagPreference.kt delete mode 100644 app/src/main/java/org/mozilla/fenix/utils/FeatureFlagPreference.kt create mode 100644 app/src/test/java/org/mozilla/fenix/components/settings/CounterPreferenceTest.kt create mode 100644 app/src/test/java/org/mozilla/fenix/components/settings/FeatureFlagPreferenceTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/components/settings/CounterPreference.kt b/app/src/main/java/org/mozilla/fenix/components/settings/CounterPreference.kt new file mode 100644 index 000000000..c4722cb2f --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/components/settings/CounterPreference.kt @@ -0,0 +1,31 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components.settings + +import androidx.core.content.edit +import mozilla.components.support.ktx.android.content.PreferencesHolder + +class CounterPreference( + private val holder: PreferencesHolder, + private val key: String, + private val maxCount: Int +) { + + val value get() = holder.preferences.getInt(key, 0) + + fun underMaxCount() = value < maxCount + + fun increment() { + holder.preferences.edit { + putInt(key, value + 1) + } + } +} + +/** + * Property delegate for getting and an int shared preference and incrementing it. + */ +fun PreferencesHolder.counterPreference(key: String, maxCount: Int = -1) = + CounterPreference(this, key, maxCount) diff --git a/app/src/main/java/org/mozilla/fenix/components/settings/FeatureFlagPreference.kt b/app/src/main/java/org/mozilla/fenix/components/settings/FeatureFlagPreference.kt new file mode 100644 index 000000000..0df122c5f --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/components/settings/FeatureFlagPreference.kt @@ -0,0 +1,25 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components.settings + +import mozilla.components.support.ktx.android.content.PreferencesHolder +import mozilla.components.support.ktx.android.content.booleanPreference +import kotlin.properties.ReadWriteProperty +import kotlin.reflect.KProperty + +private class DummyProperty : ReadWriteProperty { + override fun getValue(thisRef: PreferencesHolder, property: KProperty<*>) = false + override fun setValue(thisRef: PreferencesHolder, property: KProperty<*>, value: Boolean) = Unit +} + +/** + * Property delegate for getting and setting a boolean shared preference gated by a feature flag. + */ +fun featureFlagPreference(key: String, default: Boolean, featureFlag: Boolean) = + if (featureFlag) { + booleanPreference(key, default) + } else { + DummyProperty() + } diff --git a/app/src/main/java/org/mozilla/fenix/utils/FeatureFlagPreference.kt b/app/src/main/java/org/mozilla/fenix/utils/FeatureFlagPreference.kt deleted file mode 100644 index d0830bd73..000000000 --- a/app/src/main/java/org/mozilla/fenix/utils/FeatureFlagPreference.kt +++ /dev/null @@ -1,29 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package org.mozilla.fenix.utils - -import mozilla.components.support.ktx.android.content.PreferencesHolder -import kotlin.properties.ReadWriteProperty -import kotlin.reflect.KProperty - -fun featureFlagPreference( - key: String, - default: Boolean, - featureFlag: Boolean -): ReadWriteProperty = - FeatureFlagPreferencePreference(key, default, featureFlag) - -private class FeatureFlagPreferencePreference( - private val key: String, - private val default: Boolean, - private val featureFlag: Boolean -) : ReadWriteProperty { - - override fun getValue(thisRef: PreferencesHolder, property: KProperty<*>): Boolean = - featureFlag && thisRef.preferences.getBoolean(key, default) - - override fun setValue(thisRef: PreferencesHolder, property: KProperty<*>, value: Boolean) = - thisRef.preferences.edit().putBoolean(key, value).apply() -} diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 677fdb2f6..e08622d0b 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -30,6 +30,8 @@ import org.mozilla.fenix.FeatureFlags import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.metrics.MozillaProductDetector +import org.mozilla.fenix.components.settings.counterPreference +import org.mozilla.fenix.components.settings.featureFlagPreference import org.mozilla.fenix.components.toolbar.ToolbarPosition import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.getPreferenceKey @@ -50,14 +52,9 @@ private const val AUTOPLAY_USER_SETTING = "AUTOPLAY_USER_SETTING" class Settings(private val appContext: Context) : PreferencesHolder { companion object { - const val showLoginsSecureWarningSyncMaxCount = 1 - const val showLoginsSecureWarningMaxCount = 1 - const val trackingProtectionOnboardingMaximumCount = 1 - const val pwaVisitsToShowPromptMaxCount = 3 const val topSitesMaxCount = 16 const val FENIX_PREFERENCES = "fenix_preferences" - private const val showSearchWidgetCFRMaxCount = 3 private const val BLOCKED_INT = 0 private const val ASK_TO_ALLOW_INT = 1 private const val ALLOWED_INT = 2 @@ -177,38 +174,26 @@ class Settings(private val appContext: Context) : PreferencesHolder { true ) - private val activeSearchCount by intPreference( - appContext.getPreferenceKey(R.string.pref_key_search_count), - default = 0 + private val activeSearchCount = counterPreference( + appContext.getPreferenceKey(R.string.pref_key_search_count) ) - fun incrementActiveSearchCount() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_search_count), - activeSearchCount + 1 - ).apply() - } + fun incrementActiveSearchCount() = activeSearchCount.increment() private val isActiveSearcher: Boolean - get() = activeSearchCount > 2 + get() = activeSearchCount.value > 2 fun shouldDisplaySearchWidgetCFR(): Boolean = isActiveSearcher && - searchWidgetCFRDismissCount < showSearchWidgetCFRMaxCount && + searchWidgetCFRDismissCount.underMaxCount() && !searchWidgetInstalled && !searchWidgetCFRManuallyDismissed - private val searchWidgetCFRDisplayCount by intPreference( - appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_display_count), - default = 0 + private val searchWidgetCFRDisplayCount = counterPreference( + appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_display_count) ) - fun incrementSearchWidgetCFRDisplayed() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_display_count), - searchWidgetCFRDisplayCount + 1 - ).apply() - } + fun incrementSearchWidgetCFRDisplayed() = searchWidgetCFRDisplayCount.increment() private val searchWidgetCFRManuallyDismissed by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_manually_dismissed), @@ -222,17 +207,12 @@ class Settings(private val appContext: Context) : PreferencesHolder { ).apply() } - private val searchWidgetCFRDismissCount by intPreference( + private val searchWidgetCFRDismissCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_dismiss_count), - default = 0 + maxCount = 3 ) - fun incrementSearchWidgetCFRDismissed() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_search_widget_cfr_dismiss_count), - searchWidgetCFRDismissCount + 1 - ).apply() - } + fun incrementSearchWidgetCFRDismissed() = searchWidgetCFRDismissCount.increment() val isInSearchWidgetExperiment by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_is_in_search_widget_experiment), @@ -298,17 +278,16 @@ class Settings(private val appContext: Context) : PreferencesHolder { val shouldShowTrackingProtectionOnboarding: Boolean get() = !isOverrideTPPopupsForPerformanceTest && - (trackingProtectionOnboardingCount < trackingProtectionOnboardingMaximumCount && + (trackingProtectionOnboardingCount.underMaxCount() && !trackingProtectionOnboardingShownThisSession) var showSecretDebugMenuThisSession = false - var showNotificationsSetting = Build.VERSION.SDK_INT >= Build.VERSION_CODES.O val shouldShowSecurityPinWarningSync: Boolean - get() = loginsSecureWarningSyncCount < showLoginsSecureWarningSyncMaxCount + get() = loginsSecureWarningSyncCount.underMaxCount() val shouldShowSecurityPinWarning: Boolean - get() = loginsSecureWarningCount < showLoginsSecureWarningMaxCount + get() = loginsSecureWarningCount.underMaxCount() var shouldUseLightTheme by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_light_theme), @@ -544,12 +523,6 @@ class Settings(private val appContext: Context) : PreferencesHolder { return touchExplorationIsEnabled || switchServiceIsEnabled } - val toolbarSettingString: String - get() = when { - shouldUseBottomToolbar -> appContext.getString(R.string.preference_bottom_toolbar) - else -> appContext.getString(R.string.preference_top_toolbar) - } - fun getDeleteDataOnQuit(type: DeleteBrowsingDataOnQuitType): Boolean = preferences.getBoolean(type.getPreferenceKey(appContext), false) @@ -571,30 +544,20 @@ class Settings(private val appContext: Context) : PreferencesHolder { ).apply() @VisibleForTesting(otherwise = PRIVATE) - internal val loginsSecureWarningSyncCount by intPreference( + internal val loginsSecureWarningSyncCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_logins_secure_warning_sync), - default = 0 + maxCount = 1 ) @VisibleForTesting(otherwise = PRIVATE) - internal val loginsSecureWarningCount by intPreference( + internal val loginsSecureWarningCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_logins_secure_warning), - default = 0 + maxCount = 1 ) - fun incrementShowLoginsSecureWarningCount() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_logins_secure_warning), - loginsSecureWarningCount + 1 - ).apply() - } + fun incrementShowLoginsSecureWarningCount() = loginsSecureWarningCount.increment() - fun incrementShowLoginsSecureWarningSyncCount() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_logins_secure_warning_sync), - loginsSecureWarningSyncCount + 1 - ).apply() - } + fun incrementShowLoginsSecureWarningSyncCount() = loginsSecureWarningSyncCount.increment() val shouldShowSearchSuggestions by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_show_search_suggestions), @@ -616,21 +579,16 @@ class Settings(private val appContext: Context) : PreferencesHolder { default = false ) - fun incrementVisitedInstallableCount() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_install_pwa_visits), - pwaInstallableVisitCount + 1 - ).apply() - } + fun incrementVisitedInstallableCount() = pwaInstallableVisitCount.increment() @VisibleForTesting(otherwise = PRIVATE) - internal val pwaInstallableVisitCount by intPreference( + internal val pwaInstallableVisitCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_install_pwa_visits), - default = 0 + maxCount = 3 ) private val userNeedsToVisitInstallableSites: Boolean - get() = pwaInstallableVisitCount < pwaVisitsToShowPromptMaxCount + get() = pwaInstallableVisitCount.underMaxCount() val shouldShowPwaOnboarding: Boolean get() { @@ -639,9 +597,8 @@ class Settings(private val appContext: Context) : PreferencesHolder { // ShortcutManager::pinnedShortcuts is only available on Oreo+ if (!userKnowsAboutPwas && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - val alreadyHavePwaInstalled = - appContext.getSystemService(ShortcutManager::class.java) - .pinnedShortcuts.size > 0 + val manager = appContext.getSystemService(ShortcutManager::class.java) + val alreadyHavePwaInstalled = manager != null && manager.pinnedShortcuts.size > 0 // Users know about PWAs onboarding if they already have PWAs installed. userKnowsAboutPwas = alreadyHavePwaInstalled @@ -656,17 +613,14 @@ class Settings(private val appContext: Context) : PreferencesHolder { ) @VisibleForTesting(otherwise = PRIVATE) - internal val trackingProtectionOnboardingCount by intPreference( + internal val trackingProtectionOnboardingCount = counterPreference( appContext.getPreferenceKey(R.string.pref_key_tracking_protection_onboarding), - 0 + maxCount = 1 ) fun incrementTrackingProtectionOnboardingCount() { trackingProtectionOnboardingShownThisSession = true - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_tracking_protection_onboarding), - trackingProtectionOnboardingCount + 1 - ).apply() + trackingProtectionOnboardingCount.increment() } fun getSitePermissionsPhoneFeatureAction( @@ -703,7 +657,7 @@ class Settings(private val appContext: Context) : PreferencesHolder { default: Int ) = preferences.getInt(AUTOPLAY_USER_SETTING, default) - fun getSitePermissionsPhoneFeatureAutoplayAction( + private fun getSitePermissionsPhoneFeatureAutoplayAction( feature: PhoneFeature, default: AutoplayAction = AutoplayAction.BLOCKED ) = preferences.getInt(feature.getPreferenceKey(appContext), default.toInt()).toAutoplayAction() @@ -785,23 +739,16 @@ class Settings(private val appContext: Context) : PreferencesHolder { 0 ) - fun incrementNumTimesPrivateModeOpened() { - preferences.edit().putInt( - appContext.getPreferenceKey(R.string.pref_key_private_mode_opened), - numTimesPrivateModeOpened + 1 - ).apply() - } + fun incrementNumTimesPrivateModeOpened() = numTimesPrivateModeOpened.increment() private var showedPrivateModeContextualFeatureRecommender by booleanPreference( appContext.getPreferenceKey(R.string.pref_key_showed_private_mode_cfr), default = false ) - private val numTimesPrivateModeOpened: Int - get() = preferences.getInt( - appContext.getPreferenceKey(R.string.pref_key_private_mode_opened), - 0 - ) + private val numTimesPrivateModeOpened = counterPreference( + appContext.getPreferenceKey(R.string.pref_key_private_mode_opened) + ) val showPrivateModeContextualFeatureRecommender: Boolean get() { @@ -809,9 +756,11 @@ class Settings(private val appContext: Context) : PreferencesHolder { .getInstalledMozillaProducts(appContext as Application) .contains(MozillaProductDetector.MozillaProducts.FOCUS.productName) - val showCondition = - (numTimesPrivateModeOpened == CFR_COUNT_CONDITION_FOCUS_INSTALLED && focusInstalled) || - (numTimesPrivateModeOpened == CFR_COUNT_CONDITION_FOCUS_NOT_INSTALLED && !focusInstalled) + val showCondition = if (focusInstalled) { + numTimesPrivateModeOpened.value == CFR_COUNT_CONDITION_FOCUS_INSTALLED + } else { + numTimesPrivateModeOpened.value == CFR_COUNT_CONDITION_FOCUS_NOT_INSTALLED + } if (showCondition && !showedPrivateModeContextualFeatureRecommender) { showedPrivateModeContextualFeatureRecommender = true diff --git a/app/src/test/java/org/mozilla/fenix/components/settings/CounterPreferenceTest.kt b/app/src/test/java/org/mozilla/fenix/components/settings/CounterPreferenceTest.kt new file mode 100644 index 000000000..cee183392 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/settings/CounterPreferenceTest.kt @@ -0,0 +1,63 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components.settings + +import android.content.SharedPreferences +import io.mockk.MockKAnnotations +import io.mockk.Runs +import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.just +import io.mockk.verify +import mozilla.components.support.ktx.android.content.PreferencesHolder +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +class CounterPreferenceTest { + + @MockK private lateinit var prefs: SharedPreferences + @MockK private lateinit var editor: SharedPreferences.Editor + + @Before + fun setup() { + MockKAnnotations.init(this) + every { prefs.getInt("key", 0) } returns 0 + every { prefs.edit() } returns editor + every { editor.putInt("key", any()) } returns editor + every { editor.apply() } just Runs + } + + @Test + fun `update value after increment`() { + val holder = CounterHolder() + + assertEquals(0, holder.property.value) + holder.property.increment() + + verify { editor.putInt("key", 1) } + } + + @Test + fun `check if value is under max count`() { + val holder = CounterHolder(maxCount = 2) + + every { prefs.getInt("key", 0) } returns 0 + assertEquals(0, holder.property.value) + assertTrue(holder.property.underMaxCount()) + + every { prefs.getInt("key", 0) } returns 2 + assertEquals(2, holder.property.value) + assertFalse(holder.property.underMaxCount()) + } + + private inner class CounterHolder(maxCount: Int = -1) : PreferencesHolder { + override val preferences = prefs + + val property = counterPreference("key", maxCount) + } +} diff --git a/app/src/test/java/org/mozilla/fenix/components/settings/FeatureFlagPreferenceTest.kt b/app/src/test/java/org/mozilla/fenix/components/settings/FeatureFlagPreferenceTest.kt new file mode 100644 index 000000000..764a7e383 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/settings/FeatureFlagPreferenceTest.kt @@ -0,0 +1,67 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components.settings + +import android.content.SharedPreferences +import io.mockk.Called +import io.mockk.MockKAnnotations +import io.mockk.Runs +import io.mockk.every +import io.mockk.impl.annotations.MockK +import io.mockk.just +import io.mockk.verify +import mozilla.components.support.ktx.android.content.PreferencesHolder +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +class FeatureFlagPreferenceTest { + + @MockK private lateinit var prefs: SharedPreferences + @MockK private lateinit var editor: SharedPreferences.Editor + + @Before + fun setup() { + MockKAnnotations.init(this) + every { prefs.getBoolean("key", false) } returns true + every { prefs.edit() } returns editor + every { editor.putBoolean("key", any()) } returns editor + every { editor.apply() } just Runs + } + + @Test + fun `acts like boolean preference if feature flag is true`() { + val holder = FeatureFlagHolder(featureFlag = true) + + assertTrue(holder.property) + verify { prefs.getBoolean("key", false) } + + holder.property = false + verify { editor.putBoolean("key", false) } + } + + @Test + fun `no-op if feature flag is false`() { + val holder = FeatureFlagHolder(featureFlag = false) + + assertFalse(holder.property) + holder.property = true + holder.property = false + + verify { prefs wasNot Called } + verify { editor wasNot Called } + } + + private inner class FeatureFlagHolder(featureFlag: Boolean) : PreferencesHolder { + override val preferences = prefs + + var property by featureFlagPreference( + "key", + default = false, + featureFlag = featureFlag + ) + } +} diff --git a/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt b/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt index 6e1e9a51d..37d82cbef 100644 --- a/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt +++ b/app/src/test/java/org/mozilla/fenix/utils/SettingsTest.kt @@ -128,13 +128,13 @@ class SettingsTest { fun showLoginsDialogWarningSync() { // When just created // Then - assertEquals(0, settings.loginsSecureWarningSyncCount) + assertEquals(0, settings.loginsSecureWarningSyncCount.value) // When settings.incrementShowLoginsSecureWarningSyncCount() // Then - assertEquals(1, settings.loginsSecureWarningSyncCount) + assertEquals(1, settings.loginsSecureWarningSyncCount.value) } @Test @@ -154,13 +154,13 @@ class SettingsTest { fun showLoginsDialogWarning() { // When just created // Then - assertEquals(0, settings.loginsSecureWarningCount) + assertEquals(0, settings.loginsSecureWarningCount.value) // When settings.incrementShowLoginsSecureWarningCount() // Then - assertEquals(1, settings.loginsSecureWarningCount) + assertEquals(1, settings.loginsSecureWarningCount.value) } @Test