From fc7e707538c8d507c95aeceedc68d98b6ddc95ff Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Tue, 16 Nov 2021 23:20:32 -0800 Subject: [PATCH] [fenix] Closes https://github.com/mozilla-mobile/fenix/issues/22472: Filter out redirects from History search groups --- .../history/PagedHistoryProvider.kt | 28 ++- .../history/PagedHistoryProviderTest.kt | 160 ++++++++++++++++++ 2 files changed, 186 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt index fb17eebbcb..9f88bbb975 100644 --- a/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt +++ b/app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt @@ -50,6 +50,14 @@ class DefaultPagedHistoryProvider( VisitType.FRAMED_LINK, ) + /** + * All types of visits that aren't redirects. This is used for fetching only redirecting visits + * from the store so that we can filter them out. + */ + private val notRedirectTypes = VisitType.values().filterNot { + it == VisitType.REDIRECT_PERMANENT || it == VisitType.REDIRECT_TEMPORARY + } + @Volatile private var historyGroups: List? = null @Suppress("LongMethod") @@ -64,7 +72,7 @@ class DefaultPagedHistoryProvider( val history: List if (showHistorySearchGroups) { - // We need to refetch all the history metadata if the offset resets back at 0 + // We need to re-fetch all the history metadata if the offset resets back at 0 // in the case of a pull to refresh. if (historyGroups == null || offset == 0L) { historyGroups = historyStorage.getHistoryMetadataSince(Long.MIN_VALUE) @@ -134,6 +142,22 @@ class DefaultPagedHistoryProvider( ) .mapIndexed(transformVisitInfoToHistoryItem(offset.toInt())) + // We'll use this list to filter out redirects from metadata groups below. + val redirectsInThePage = if (history.isNotEmpty()) { + historyStorage.getDetailedVisits( + start = history.last().visitedAt, + end = history.first().visitedAt, + excludeTypes = notRedirectTypes + ).map { it.url } + } else { + // Edge-case this doesn't cover: if we only had redirects in the current page, + // we'd end up with an empty 'history' list since the redirects would have been + // filtered out above. One possible solution would be to look at redirects in all of + // history, but that's potentially quite expensive on large profiles, and introduces + // other problems (e.g. pages that were redirects a month ago may not be redirects today). + emptyList() + } + // History metadata items are recorded after their associated visited info, we add an // additional buffer time to the most recent visit to account for a history group // appearing as the most recent item. @@ -164,7 +188,7 @@ class DefaultPagedHistoryProvider( // url, but we don't have a use case for this currently in the history view. result.addAll( historyGroupsInOffset.map { group -> - group.copy(items = group.items.distinctBy { it.url }) + group.copy(items = group.items.distinctBy { it.url }.filterNot { redirectsInThePage.contains(it.url) }) } ) diff --git a/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt b/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt index 2b22481998..51e60fbc28 100644 --- a/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt @@ -89,6 +89,7 @@ class PagedHistoryProviderTest { ) coEvery { storage.getVisitsPaginated(any(), any(), any()) } returns listOf(visitInfo1, visitInfo2, visitInfo3) + coEvery { storage.getDetailedVisits(any(), any(), any()) } returns emptyList() coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1, historyEntry2, historyEntry3) var actualResults: List? = null @@ -175,6 +176,7 @@ class PagedHistoryProviderTest { ) coEvery { storage.getVisitsPaginated(any(), any(), any()) } returns listOf(visitInfo1) + coEvery { storage.getDetailedVisits(any(), any(), any()) } returns emptyList() coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) var actualResults: List? = null @@ -248,6 +250,7 @@ class PagedHistoryProviderTest { ) coEvery { storage.getVisitsPaginated(any(), any(), any()) } returns listOf(visitInfo1) + coEvery { storage.getDetailedVisits(any(), any(), any()) } returns emptyList() coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1) var actualResults: List? = null @@ -292,4 +295,161 @@ class PagedHistoryProviderTest { assertEquals(results, actualResults) } + + @Test + fun `redirects are filtered out from history metadata groups`() { + val provider = DefaultPagedHistoryProvider( + historyStorage = storage, + showHistorySearchGroups = true + ) + + val visitInfo1 = VisitInfo( + url = "http://www.mozilla.com", + title = "mozilla", + visitTime = 5, + visitType = VisitType.LINK, + previewImageUrl = null + ) + val visitInfo2 = VisitInfo( + url = "http://www.firefox.com", + title = "firefox", + visitTime = 2, + visitType = VisitType.LINK, + previewImageUrl = null + ) + val visitInfo3 = VisitInfo( + url = "http://www.google.com/link?url=http://www.firefox.com", + title = "", + visitTime = 1, + visitType = VisitType.REDIRECT_TEMPORARY, + previewImageUrl = null + ) + val visitInfo4 = VisitInfo( + url = "http://mozilla.com", + title = "", + visitTime = 1, + visitType = VisitType.REDIRECT_PERMANENT, + previewImageUrl = null + ) + + val historyMetadataKey1 = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null) + val historyEntry1 = HistoryMetadata( + key = historyMetadataKey1, + title = "mozilla", + createdAt = 1, + updatedAt = 10, + totalViewTime = 10, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + val historyMetadataKey2 = HistoryMetadataKey("http://www.firefox.com", "mozilla", null) + val historyEntry2 = HistoryMetadata( + key = historyMetadataKey2, + title = "firefox", + createdAt = 2, + updatedAt = 11, + totalViewTime = 20, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + val historyMetadataKey3 = HistoryMetadataKey("http://www.google.com/link?url=http://www.firefox.com", "mozilla", null) + val historyEntry3 = HistoryMetadata( + key = historyMetadataKey3, + title = "", + createdAt = 2, + updatedAt = 11, + totalViewTime = 0, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + val historyMetadataKey4 = HistoryMetadataKey("http://mozilla.com", "mozilla", null) + val historyEntry4 = HistoryMetadata( + key = historyMetadataKey4, + title = "", + createdAt = 2, + updatedAt = 11, + totalViewTime = 0, + documentType = DocumentType.Regular, + previewImageUrl = null + ) + + // Normal visits. + coEvery { + storage.getVisitsPaginated( + any(), any(), + eq( + listOf( + VisitType.NOT_A_VISIT, + VisitType.DOWNLOAD, + VisitType.REDIRECT_PERMANENT, + VisitType.REDIRECT_TEMPORARY, + VisitType.RELOAD, + VisitType.EMBED, + VisitType.FRAMED_LINK, + ) + ) + ) + } returns listOf(visitInfo1, visitInfo2) + // Redirects. + coEvery { + storage.getDetailedVisits( + any(), any(), + eq( + VisitType.values().filterNot { + it == VisitType.REDIRECT_PERMANENT || it == VisitType.REDIRECT_TEMPORARY + } + ) + ) + } returns listOf(visitInfo3, visitInfo4) + + coEvery { storage.getHistoryMetadataSince(any()) } returns listOf(historyEntry1, historyEntry2, historyEntry3, historyEntry4) + + var actualResults: List? = null + provider.getHistory(10L, 5) { + actualResults = it + } + + coVerify { + storage.getVisitsPaginated( + offset = 10L, + count = 5, + excludeTypes = listOf( + VisitType.NOT_A_VISIT, + VisitType.DOWNLOAD, + VisitType.REDIRECT_PERMANENT, + VisitType.REDIRECT_TEMPORARY, + VisitType.RELOAD, + VisitType.EMBED, + VisitType.FRAMED_LINK, + ) + ) + } + + val results = listOf( + History.Group( + id = historyEntry2.createdAt.toInt(), + title = historyEntry2.key.searchTerm!!, + visitedAt = historyEntry2.createdAt, + items = listOf( + History.Metadata( + id = historyEntry2.createdAt.toInt(), + title = historyEntry2.title!!, + url = historyEntry2.key.url, + visitedAt = historyEntry2.createdAt, + totalViewTime = historyEntry2.totalViewTime, + historyMetadataKey = historyMetadataKey2 + ), + History.Metadata( + id = historyEntry1.createdAt.toInt(), + title = historyEntry1.title!!, + url = historyEntry1.key.url, + visitedAt = historyEntry1.createdAt, + totalViewTime = historyEntry1.totalViewTime, + historyMetadataKey = historyMetadataKey1 + ), + ) + ) + ) + assertEquals(results, actualResults) + } }