diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt index b6bdcfab177..3457f0a4310 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/DefaultTopSitesStorage.kt @@ -86,6 +86,7 @@ class DefaultTopSitesStorage( @Suppress("TooGenericExceptionCaught") override suspend fun getTopSites( totalSites: Int, + fetchProvidedTopSites: Boolean, frecencyConfig: FrecencyThresholdOption? ): List { val topSites = ArrayList() @@ -94,9 +95,9 @@ class DefaultTopSitesStorage( topSites.addAll(pinnedSites) - topSitesProvider?.let { provider -> + if (fetchProvidedTopSites && topSitesProvider != null) { try { - val providerTopSites = provider.getTopSites() + val providerTopSites = topSitesProvider.getTopSites() topSites.addAll(providerTopSites.take(numSitesRequired)) numSitesRequired -= providerTopSites.size } catch (e: Exception) { diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt index 13af6d7949a..278a4036ac3 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesConfig.kt @@ -11,11 +11,13 @@ 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 fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider]. * @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. */ data class TopSitesConfig( val totalSites: Int, + val fetchProvidedTopSites: Boolean, val frecencyConfig: FrecencyThresholdOption? ) diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt index 4cd6601ac4b..155fdc90463 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/TopSitesStorage.kt @@ -4,6 +4,7 @@ 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 @@ -42,12 +43,14 @@ interface TopSitesStorage : Observable { * 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 fetchProvidedTopSites Whether or not to fetch top sites from the [TopSitesProvider]. * @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. */ suspend fun getTopSites( totalSites: Int, + fetchProvidedTopSites: Boolean, frecencyConfig: FrecencyThresholdOption? ): List diff --git a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt index 0e94184b2bb..da3ed0ea745 100644 --- a/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt +++ b/components/feature/top-sites/src/main/java/mozilla/components/feature/top/sites/presenter/DefaultTopSitesPresenter.kt @@ -46,6 +46,7 @@ internal class DefaultTopSitesPresenter( scope.launch { val topSites = storage.getTopSites( innerConfig.totalSites, + innerConfig.fetchProvidedTopSites, innerConfig.frecencyConfig ) diff --git a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt index 40cba459d44..055b4222afb 100644 --- a/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt +++ b/components/feature/top-sites/src/test/java/mozilla/components/feature/top/sites/DefaultTopSitesStorageTest.kt @@ -28,6 +28,7 @@ class DefaultTopSitesStorageTest { private val pinnedSitesStorage: PinnedSiteStorage = mock() private val historyStorage: PlacesHistoryStorage = mock() + private val topSitesProvider: TopSitesProvider = mock() @Test fun `default top sites are added to pinned site storage on init`() = runBlockingTest { @@ -54,6 +55,7 @@ class DefaultTopSitesStorageTest { defaultTopSites = listOf(), coroutineContext = coroutineContext ) + defaultTopSitesStorage.addTopSite("Mozilla", "https://mozilla.com", isDefault = false) verify(pinnedSitesStorage).addPinnedSite( @@ -78,6 +80,7 @@ class DefaultTopSitesStorageTest { url = "https://mozilla.com", createdAt = 1 ) + defaultTopSitesStorage.removeTopSite(frecentSite) verify(historyStorage).deleteVisitsFor(frecentSite.url) @@ -88,6 +91,7 @@ class DefaultTopSitesStorageTest { url = "https://firefox.com", createdAt = 2 ) + defaultTopSitesStorage.removeTopSite(pinnedSite) verify(pinnedSitesStorage).removePinnedSite(pinnedSite) @@ -99,6 +103,7 @@ class DefaultTopSitesStorageTest { url = "https://wikipedia.com", createdAt = 3 ) + defaultTopSitesStorage.removeTopSite(defaultSite) verify(pinnedSitesStorage).removePinnedSite(defaultSite) @@ -120,6 +125,7 @@ class DefaultTopSitesStorageTest { url = "https://firefox.com", createdAt = 1 ) + defaultTopSitesStorage.updateTopSite(defaultSite, "Mozilla Firefox", "https://mozilla.com") verify(pinnedSitesStorage).updatePinnedSite(defaultSite, "Mozilla Firefox", "https://mozilla.com") @@ -130,6 +136,7 @@ class DefaultTopSitesStorageTest { url = "https://wikipedia.com", createdAt = 2 ) + defaultTopSitesStorage.updateTopSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki") verify(pinnedSitesStorage).updatePinnedSite(pinnedSite, "Wiki", "https://en.wikipedia.org/wiki/Wiki") @@ -140,6 +147,7 @@ class DefaultTopSitesStorageTest { url = "https://mozilla.com", createdAt = 1 ) + defaultTopSitesStorage.updateTopSite(frecentSite, "Moz", "") verify(pinnedSitesStorage, never()).updatePinnedSite(frecentSite, "Moz", "") @@ -166,6 +174,7 @@ class DefaultTopSitesStorageTest { url = "https://wikipedia.com", createdAt = 2 ) + whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( defaultSite, @@ -174,26 +183,229 @@ class DefaultTopSitesStorageTest { ) whenever(pinnedSitesStorage.getPinnedSitesCount()).thenReturn(2) - var topSites = defaultTopSitesStorage.getTopSites(0, frecencyConfig = null) + var topSites = defaultTopSitesStorage.getTopSites( + totalSites = 0, + fetchProvidedTopSites = false, + frecencyConfig = null + ) + assertTrue(topSites.isEmpty()) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites(1, frecencyConfig = null) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 1, + fetchProvidedTopSites = false, + frecencyConfig = null + ) + assertEquals(1, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites(2, frecencyConfig = null) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 2, + fetchProvidedTopSites = false, + frecencyConfig = null + ) + + assertEquals(2, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = false, + frecencyConfig = null + ) + assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + } + + @Test + fun `GIVEN fetchProvidedTopSites is true WHEN getTopSites is called THEN default, pinned and provided top sites are returned`() = runBlockingTest { + val defaultTopSitesStorage = DefaultTopSitesStorage( + pinnedSitesStorage = pinnedSitesStorage, + historyStorage = historyStorage, + topSitesProvider = topSitesProvider, + defaultTopSites = listOf(), + coroutineContext = coroutineContext + ) + + val defaultSite = TopSite.Default( + id = 1, + title = "Firefox", + url = "https://firefox.com", + createdAt = 1 + ) + val pinnedSite = TopSite.Pinned( + id = 2, + title = "Wikipedia", + url = "https://wikipedia.com", + createdAt = 2 + ) + val providedSite = TopSite.Provided( + id = 3, + title = "Mozilla", + url = "https://mozilla.com", + clickUrl = "https://mozilla.com/click", + imageUrl = "https://test.com/image2.jpg", + impressionUrl = "https://example.com", + position = 1, + createdAt = 3 + ) + + whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( + listOf( + defaultSite, + pinnedSite + ) + ) + whenever(topSitesProvider.getTopSites()).thenReturn(listOf(providedSite)) + + var topSites = defaultTopSitesStorage.getTopSites( + totalSites = 0, + fetchProvidedTopSites = false, + frecencyConfig = null + ) + + assertTrue(topSites.isEmpty()) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 1, + fetchProvidedTopSites = true, + frecencyConfig = null + ) + + assertEquals(1, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 2, + fetchProvidedTopSites = true, + frecencyConfig = null + ) + + assertEquals(2, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = true, + frecencyConfig = null + ) + + assertEquals(3, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(providedSite, topSites[2]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + } + + @Test + fun `GIVEN fetchProvidedTopSites is true and frecencyConfig is specified WHEN getTopSites is called THEN default, pinned, provided and frecent top sites are returned`() = runBlockingTest { + val defaultTopSitesStorage = DefaultTopSitesStorage( + pinnedSitesStorage = pinnedSitesStorage, + historyStorage = historyStorage, + topSitesProvider = topSitesProvider, + defaultTopSites = listOf(), + coroutineContext = coroutineContext + ) + + val defaultSite = TopSite.Default( + id = 1, + title = "Firefox", + url = "https://firefox.com", + createdAt = 1 + ) + val pinnedSite = TopSite.Pinned( + id = 2, + title = "Wikipedia", + url = "https://wikipedia.com", + createdAt = 2 + ) + val providedSite = TopSite.Provided( + id = 3, + title = "Mozilla", + url = "https://mozilla.com", + clickUrl = "https://mozilla.com/click", + imageUrl = "https://test.com/image2.jpg", + impressionUrl = "https://example.com", + position = 1, + createdAt = 3 + ) + + whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( + listOf( + defaultSite, + pinnedSite + ) + ) + whenever(topSitesProvider.getTopSites()).thenReturn(listOf(providedSite)) + + val frecentSite1 = TopFrecentSiteInfo("https://mozilla.com", "Mozilla") + whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(listOf(frecentSite1)) + + var topSites = defaultTopSitesStorage.getTopSites( + totalSites = 0, + fetchProvidedTopSites = true, + frecencyConfig = FrecencyThresholdOption.NONE + ) + + assertTrue(topSites.isEmpty()) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 1, + fetchProvidedTopSites = true, + frecencyConfig = FrecencyThresholdOption.NONE + ) + + assertEquals(1, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 2, + fetchProvidedTopSites = true, + frecencyConfig = FrecencyThresholdOption.NONE + ) - topSites = defaultTopSitesStorage.getTopSites(5, frecencyConfig = null) assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 3, + fetchProvidedTopSites = true, + frecencyConfig = FrecencyThresholdOption.NONE + ) + + assertEquals(3, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(providedSite, topSites[2]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) + + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = true, + frecencyConfig = FrecencyThresholdOption.NONE + ) + + assertEquals(4, topSites.size) + assertEquals(defaultSite, topSites[0]) + assertEquals(pinnedSite, topSites[1]) + assertEquals(providedSite, topSites[2]) + assertEquals(frecentSite1.toTopSite(), topSites[3]) + assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) } @Test @@ -217,6 +429,7 @@ class DefaultTopSitesStorageTest { url = "https://wikipedia.com", createdAt = 2 ) + whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( defaultSite, @@ -228,21 +441,41 @@ class DefaultTopSitesStorageTest { val frecentSite1 = TopFrecentSiteInfo("https://mozilla.com", "Mozilla") whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn(listOf(frecentSite1)) - var topSites = defaultTopSitesStorage.getTopSites(0, frecencyConfig = FrecencyThresholdOption.NONE) + var topSites = defaultTopSitesStorage.getTopSites( + totalSites = 0, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertTrue(topSites.isEmpty()) - topSites = defaultTopSitesStorage.getTopSites(1, frecencyConfig = FrecencyThresholdOption.NONE) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 1, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertEquals(1, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites(2, frecencyConfig = FrecencyThresholdOption.NONE) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 2, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertEquals(2, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) assertEquals(defaultTopSitesStorage.cachedTopSites, topSites) - topSites = defaultTopSitesStorage.getTopSites(5, frecencyConfig = FrecencyThresholdOption.NONE) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertEquals(3, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) @@ -259,7 +492,12 @@ class DefaultTopSitesStorageTest { ) ) - topSites = defaultTopSitesStorage.getTopSites(5, frecencyConfig = FrecencyThresholdOption.NONE) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertEquals(5, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) @@ -278,7 +516,12 @@ class DefaultTopSitesStorageTest { ) ) - topSites = defaultTopSitesStorage.getTopSites(5, frecencyConfig = FrecencyThresholdOption.NONE) + topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) + assertEquals(5, topSites.size) assertEquals(defaultSite, topSites[0]) assertEquals(pinnedSite, topSites[1]) @@ -315,6 +558,7 @@ class DefaultTopSitesStorageTest { url = "https://example.com", createdAt = 3 ) + whenever(pinnedSitesStorage.getPinnedSites()).thenReturn( listOf( defaultSiteFirefox, @@ -328,6 +572,7 @@ class DefaultTopSitesStorageTest { val frecentSiteFirefox = TopFrecentSiteInfo("https://firefox.com", "Firefox") val frecentSite1 = TopFrecentSiteInfo("https://getpocket.com", "Pocket") val frecentSite2 = TopFrecentSiteInfo("https://www.example.com", "Example") + whenever(historyStorage.getTopFrecentSites(anyInt(), any())).thenReturn( listOf( frecentSiteWithNoTitle, @@ -337,7 +582,11 @@ class DefaultTopSitesStorageTest { ) ) - val topSites = defaultTopSitesStorage.getTopSites(5, frecencyConfig = FrecencyThresholdOption.NONE) + val topSites = defaultTopSitesStorage.getTopSites( + totalSites = 5, + fetchProvidedTopSites = false, + frecencyConfig = FrecencyThresholdOption.NONE + ) verify(historyStorage).getTopFrecentSites(5, frecencyThreshold = FrecencyThresholdOption.NONE) diff --git a/docs/changelog.md b/docs/changelog.md index c4df9969992..51fb0406f7d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -29,7 +29,8 @@ permalink: /changelog/ * **feature-top-sites** * ⚠️ **This is a breaking change**: The existing data class `TopSite` has been converted into a sealed class. [#11483](https://github.com/mozilla-mobile/android-components/issues/11483) * Extend `DefaultTopSitesStorage` to accept a `TopSitesProvider` for fetching top sites. [#11483](https://github.com/mozilla-mobile/android-components/issues/11483) - + * ⚠️ **This is a breaking change**: Added a new parameter `fetchProvidedTopSites` to `TopSitesConfig` to specify whether or not to fetch top sites from the `TopSitesProvider`. [#11605](https://github.com/mozilla-mobile/android-components/issues/11605) + * **service-contile** * Adds a `ContileTopSitesProvider` that implements `TopSitesProvider` for returning top sites from the Contile services API. [#11483](https://github.com/mozilla-mobile/android-components/issues/11483)