For mozilla-mobile#12384 - Add optional filter for top frecent sites.

Alexandru2909 · Jun 28, 2022 · 8cad9f6 · 8cad9f6
1 parent 6401f09
commit 8cad9f6
Show file tree

Hide file tree

Showing 5 changed files with 175 additions and 29 deletions.
diff --git a/...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/...re/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt
@@ -8,7 +8,6 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
-import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.feature.top.sites.ext.hasUrl
 import mozilla.components.feature.top.sites.ext.toTopSite
 import mozilla.components.feature.top.sites.facts.emitTopSitesCountFact
@@ -86,7 +85,7 @@ class DefaultTopSitesStorage(
     @Suppress("ComplexCondition", "TooGenericExceptionCaught")
     override suspend fun getTopSites(
         totalSites: Int,
-        frecencyConfig: FrecencyThresholdOption?,
+        frecencyConfig: TopSitesFrecencyConfig?,
         providerConfig: TopSitesProviderConfig?
     ): List<TopSite> {
         val topSites = ArrayList<TopSite>()
@@ -114,13 +113,17 @@ class DefaultTopSitesStorage(
 
         topSites.addAll(pinnedSites)
 
-        if (frecencyConfig != null && numSitesRequired > 0) {
+        if (frecencyConfig?.frecencyTresholdOption != null && numSitesRequired > 0) {
             // Get 'totalSites' sites for duplicate entries with
             // existing pinned sites
             val frecentSites = historyStorage
-                .getTopFrecentSites(totalSites, frecencyConfig)
+                .getTopFrecentSites(totalSites, frecencyConfig.frecencyTresholdOption)
                 .map { it.toTopSite() }
-                .filter { !pinnedSites.hasUrl(it.url) && !providerTopSites.hasUrl(it.url) }
+                .filter {
+                    !pinnedSites.hasUrl(it.url) &&
+                        !providerTopSites.hasUrl(it.url) &&
+                        frecencyConfig.frecencyFilter?.invoke(it) ?: true
+                }
                 .take(numSitesRequired)
 
             topSites.addAll(frecentSites)

diff --git a/...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt b/...ts/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt
@@ -11,15 +11,14 @@ import mozilla.components.concept.storage.FrecencyThresholdOption
  * whether or not to include top frecent sites in the top sites feature.
  *
  * @property totalSites A total number of sites that will be displayed.
- * @property frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency
- * score above the given threshold will be returned. Otherwise, frecent top site results are
- * not included.
+ * @property frecencyConfig An instance of [TopSitesFrecencyConfig] that specifies which top
+ * frecent sites should be included.
  * @property providerConfig An instance of [TopSitesProviderConfig] that specifies whether or
  * not to fetch top sites from the [TopSitesProvider].
  */
 data class TopSitesConfig(
     val totalSites: Int,
-    val frecencyConfig: FrecencyThresholdOption? = null,
+    val frecencyConfig: TopSitesFrecencyConfig? = null,
     val providerConfig: TopSitesProviderConfig? = null
 )
 
@@ -36,3 +35,16 @@ data class TopSitesProviderConfig(
     val maxThreshold: Int = Int.MAX_VALUE,
     val providerFilter: ((TopSite) -> Boolean)? = null
 )
+
+/**
+ * Top sites frecency configuration used to specify which top frecent sites should be included.
+ *
+ * @property frecencyTresholdOption If [frecencyTresholdOption] is specified, only visited sites with a frecency
+ * score above the given threshold will be returned. Otherwise, frecent top site results are
+ * not included.
+ * @property frecencyFilter Optional function used to filter the top frecent sites.
+ */
+data class TopSitesFrecencyConfig(
+    val frecencyTresholdOption: FrecencyThresholdOption? = null,
+    val frecencyFilter: ((TopSite) -> Boolean)? = null
+)
diff --git a/...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt b/...s/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt
@@ -5,7 +5,6 @@
 package mozilla.components.feature.top.sites
 
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
-import mozilla.components.concept.storage.FrecencyThresholdOption
 import mozilla.components.support.base.observer.Observable
 
 /**
@@ -43,15 +42,14 @@ interface TopSitesStorage : Observable<TopSitesStorage.Observer> {
      * If `frecencyConfig` is specified, fill in any missing top sites with frecent top site results.
      *
      * @param totalSites A total number of sites that will be retrieve if possible.
-     * @param frecencyConfig If [frecencyConfig] is specified, only visited sites with a frecency
-     * score above the given threshold will be returned. Otherwise, frecent top site results are
-     * not included.
+     * @param frecencyConfig An instance of [TopSitesFrecencyConfig] that specifies which top
+     * frecent sites to be included.
      * @param providerConfig An instance of [TopSitesProviderConfig] that specifies whether or
      * not to fetch top sites from the [TopSitesProvider].
      */
     suspend fun getTopSites(
         totalSites: Int,
-        frecencyConfig: FrecencyThresholdOption? = null,
+        frecencyConfig: TopSitesFrecencyConfig? = null,
         providerConfig: TopSitesProviderConfig? = null
     ): List<TopSite>
 

diff --git a/...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt b/...op-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt
@@ -4,6 +4,7 @@
 
 package mozilla.components.feature.top.sites
 
+import android.net.Uri
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
@@ -522,7 +523,9 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -532,7 +535,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -544,7 +549,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -557,7 +564,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 3,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -571,7 +580,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -620,14 +631,18 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 0,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertTrue(topSites.isEmpty())
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 1,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertEquals(1, topSites.size)
@@ -636,7 +651,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 2,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertEquals(2, topSites.size)
@@ -646,7 +663,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertEquals(3, topSites.size)
@@ -667,7 +686,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertEquals(5, topSites.size)
@@ -690,7 +711,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         assertEquals(5, topSites.size)
@@ -755,7 +778,9 @@ class DefaultTopSitesStorageTest {
 
         val topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 5,
-            frecencyConfig = FrecencyThresholdOption.NONE
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            )
         )
 
         verify(historyStorage).getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.NONE)
@@ -830,7 +855,9 @@ class DefaultTopSitesStorageTest {
 
         var topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 3,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = providerConfig
         )
 
@@ -842,7 +869,9 @@ class DefaultTopSitesStorageTest {
 
         topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 4,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = providerConfig
         )
 
@@ -920,7 +949,9 @@ class DefaultTopSitesStorageTest {
 
         val topSites = defaultTopSitesStorage.getTopSites(
             totalSites = 10,
-            frecencyConfig = FrecencyThresholdOption.NONE,
+            frecencyConfig = TopSitesFrecencyConfig(
+                frecencyTresholdOption = FrecencyThresholdOption.NONE
+            ),
             providerConfig = TopSitesProviderConfig(
                 showProviderTopSites = true
             )
@@ -938,4 +969,103 @@ class DefaultTopSitesStorageTest {
         assertEquals("mozilla.com", frecentSiteWithNoTitle.toTopSite().title)
         assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
     }
+
+    @Test
+    fun `GIVEN frecencyFilter is set WHEN getTopSites is called THEN the frecent top sites are filtered`() = runTestOnMain {
+        val defaultTopSitesStorage = DefaultTopSitesStorage(
+            pinnedSitesStorage = pinnedSitesStorage,
+            historyStorage = historyStorage,
+            topSitesProvider = topSitesProvider,
+            coroutineContext = coroutineContext
+        )
+
+        val filterMethod: ((TopSite) -> Boolean) = { topSite ->
+            val uri = Uri.parse(topSite.url)
+            if (!uri.queryParameterNames.contains("key")) {
+                true
+            } else {
+                uri.getQueryParameter("key") != "value"
+            }
+        }
+
+        val filteredUrl = "https://test.com/?key=value"
+
+        val frecencyConfig = TopSitesFrecencyConfig(
+            frecencyTresholdOption = FrecencyThresholdOption.NONE,
+            frecencyFilter = filterMethod
+        )
+
+        val defaultSite = TopSite.Default(
+            id = 1,
+            title = "Firefox",
+            url = "https://firefox.com",
+            createdAt = 1
+        )
+        val pinnedSite = TopSite.Pinned(
+            id = 2,
+            title = "Test",
+            url = "https://test.com",
+            createdAt = 2
+        )
+
+        whenever(pinnedSitesStorage.getPinnedSites()).thenReturn(
+            listOf(
+                defaultSite,
+                pinnedSite
+            )
+        )
+
+        val providedFilteredSite = TopSite.Provided(
+            id = 3,
+            title = "Filtered",
+            url = "https://test.com",
+            clickUrl = "https://test.com/click",
+            imageUrl = "https://test.com/image2.jpg",
+            impressionUrl = "https://example.com",
+            createdAt = 3
+        )
+
+        whenever(topSitesProvider.getTopSites()).thenReturn(
+            listOf(
+                providedFilteredSite
+            )
+        )
+
+        val frecentSite = TopFrecentSiteInfo("https://getpocket.com", "Pocket")
+
+        val frecentFilteredSite = TopFrecentSiteInfo(filteredUrl, "testSearch")
+
+        whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(
+            listOf(
+                frecentSite,
+                frecentFilteredSite
+            )
+        )
+
+        var topSites = defaultTopSitesStorage.getTopSites(
+            totalSites = 4,
+            frecencyConfig = frecencyConfig,
+            providerConfig = TopSitesProviderConfig(showProviderTopSites = true)
+        )
+
+        assertEquals(4, topSites.size)
+        assertTrue(topSites.contains(frecentSite.toTopSite()))
+        assertTrue(topSites.contains(providedFilteredSite))
+        assertTrue(topSites.contains(defaultSite))
+        assertTrue(topSites.contains(pinnedSite))
+        assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
+
+        topSites = defaultTopSitesStorage.getTopSites(
+            totalSites = 5,
+            frecencyConfig = frecencyConfig,
+            providerConfig = TopSitesProviderConfig(showProviderTopSites = true)
+        )
+
+        assertEquals(4, topSites.size)
+        assertTrue(topSites.contains(frecentSite.toTopSite()))
+        assertTrue(topSites.contains(providedFilteredSite))
+        assertTrue(topSites.contains(defaultSite))
+        assertTrue(topSites.contains(pinnedSite))
+        assertEquals(defaultTopSitesStorage.cachedTopSites, topSites)
+    }
 }
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -11,6 +11,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)
 
+* **feature-top-sites**
+  * Replaced `frecencyConfig` from `TopSitesConfig` with `TopSitesFrecencyConfig`, which specifies the `FrecencyTresholdOption` and the frecency filter, an optional function used to filter the top frecent sites. [#12384] (https://github.com/mozilla-mobile/android-components/issues/12384)
+
 # 103.0.0
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v102.0.0...v103.0.0)
 * [Milestone](https://github.com/mozilla-mobile/android-components/milestone/150?closed=1)