From 690f3e72f52a0272c553b43f6c70b9a21c36f988 Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Wed, 25 Aug 2021 12:55:31 -0400 Subject: [PATCH] [fenix] Dedupe history metadata in groups based on url --- .../historymetadata/HistoryMetadataFeature.kt | 16 ++ .../HistoryMetadataFeatureTest.kt | 187 ++++++++++++++---- 2 files changed, 162 insertions(+), 41 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeature.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeature.kt index 6d079ed320..be2d59287c 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeature.kt @@ -15,6 +15,7 @@ import mozilla.components.support.base.feature.LifecycleAwareFeature import org.mozilla.fenix.home.HomeFragment import org.mozilla.fenix.home.HomeFragmentAction import org.mozilla.fenix.home.HomeFragmentStore +import kotlin.math.max /** * View-bound feature that retrieves a list of history metadata and dispatches updates to the @@ -41,6 +42,21 @@ class HistoryMetadataFeature( val historyMetadata = historyMetadataStorage.getHistoryMetadataSince(Long.MIN_VALUE) .filter { it.totalViewTime > 0 && it.key.searchTerm != null } .groupBy { it.key.searchTerm!! } + .mapValues { group -> + // Within a group, we dedupe entries based on their url so we don't display + // a page multiple times in the same group, and we sum up the total view time + // of deduped entries while making sure to keep the latest updatedAt value. + val metadataInGroup = group.value + val metadataUrlGroups = metadataInGroup.groupBy { metadata -> metadata.key.url } + metadataUrlGroups.map { metadata -> + metadata.value.reduce { acc, elem -> + acc.copy( + totalViewTime = acc.totalViewTime + elem.totalViewTime, + updatedAt = max(acc.updatedAt, elem.updatedAt) + ) + } + } + } .map { (title, data) -> HistoryMetadataGroup( title = title, diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeatureTest.kt index f7dde4bb36..b6a4d85785 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataFeatureTest.kt @@ -18,7 +18,6 @@ import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.support.test.libstate.ext.waitUntilIdle import mozilla.components.support.test.middleware.CaptureActionsMiddleware import mozilla.components.support.test.rule.MainCoroutineRule -import org.junit.After import org.junit.Assert.assertEquals import org.junit.Before import org.junit.Rule @@ -34,20 +33,6 @@ class HistoryMetadataFeatureTest { private val middleware = CaptureActionsMiddleware() private val homeStore = HomeFragmentStore(middlewares = listOf(middleware)) - - private val historyEntry = HistoryMetadata( - key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), - title = "mozilla", - createdAt = System.currentTimeMillis(), - updatedAt = System.currentTimeMillis(), - totalViewTime = 10, - documentType = DocumentType.Regular - ) - private val historyGroup = HistoryMetadataGroup( - title = "mozilla", - historyMetadata = listOf(historyEntry) - ) - private val testDispatcher = TestCoroutineDispatcher() @get:Rule @@ -56,42 +41,162 @@ class HistoryMetadataFeatureTest { @Before fun setup() { historyMetadataStorage = mockk(relaxed = true) - - coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { - listOf( - historyEntry - ) - } - } - - @After - fun cleanUp() { - testDispatcher.cleanupTestCoroutines() } @Test fun `GIVEN no history metadata WHEN feature starts THEN fetch history metadata and notify store`() = testDispatcher.runBlockingTest { - val feature = HistoryMetadataFeature( - homeStore, - historyMetadataStorage, - CoroutineScope(testDispatcher), - testDispatcher + val historyEntry = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular + ) + val expectedHistoryGroup = HistoryMetadataGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry) ) - assertEquals(emptyList(), homeStore.state.historyMetadata) - - feature.start() - - testDispatcher.advanceUntilIdle() - homeStore.waitUntilIdle() - - coVerify { - historyMetadataStorage.getHistoryMetadataSince(any()) + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry + ) } + startHistoryMetadataFeature() + middleware.assertLastAction(HomeFragmentAction.HistoryMetadataChange::class) { - assertEquals(listOf(historyGroup), it.historyMetadata) + assertEquals(listOf(expectedHistoryGroup), it.historyMetadata) } } + + @Test + fun `GIVEN history metadata WHEN group contains multiple entries with same url THEN entries are deduped`() = + testDispatcher.runBlockingTest { + val historyEntry1 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = 1, + totalViewTime = 10, + documentType = DocumentType.Regular + ) + + val historyEntry2 = HistoryMetadata( + key = HistoryMetadataKey("http://firefox.com", "mozilla", null), + title = "firefox", + createdAt = System.currentTimeMillis(), + updatedAt = 2, + totalViewTime = 20, + documentType = DocumentType.Regular + ) + + val historyEntry3 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = 3, + totalViewTime = 30, + documentType = DocumentType.Regular + ) + + val expectedHistoryGroup = HistoryMetadataGroup( + title = "mozilla", + historyMetadata = listOf( + // Expected total view time to be summed up for deduped entries + historyEntry1.copy( + totalViewTime = historyEntry1.totalViewTime + historyEntry3.totalViewTime, + updatedAt = historyEntry3.updatedAt + ), + historyEntry2 + ) + ) + + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry1, historyEntry2, historyEntry3 + ) + } + + startHistoryMetadataFeature() + + middleware.assertLastAction(HomeFragmentAction.HistoryMetadataChange::class) { + assertEquals(listOf(expectedHistoryGroup), it.historyMetadata) + } + } + + @Test + fun `GIVEN history metadata WHEN different groups contain entries with same url THEN entries are not deduped`() = + testDispatcher.runBlockingTest { + val historyEntry1 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "mozilla", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 10, + documentType = DocumentType.Regular + ) + + val historyEntry2 = HistoryMetadata( + key = HistoryMetadataKey("http://firefox.com", "mozilla", null), + title = "firefox", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 20, + documentType = DocumentType.Regular + ) + + val historyEntry3 = HistoryMetadata( + key = HistoryMetadataKey("http://www.mozilla.com", "firefox", null), + title = "mozilla", + createdAt = System.currentTimeMillis(), + updatedAt = System.currentTimeMillis(), + totalViewTime = 30, + documentType = DocumentType.Regular + ) + + val expectedHistoryGroup1 = HistoryMetadataGroup( + title = "mozilla", + historyMetadata = listOf(historyEntry1, historyEntry2) + ) + + val expectedHistoryGroup2 = HistoryMetadataGroup( + title = "firefox", + historyMetadata = listOf(historyEntry3) + ) + + coEvery { historyMetadataStorage.getHistoryMetadataSince(any()) }.coAnswers { + listOf( + historyEntry1, historyEntry2, historyEntry3 + ) + } + + startHistoryMetadataFeature() + + middleware.assertLastAction(HomeFragmentAction.HistoryMetadataChange::class) { + assertEquals(listOf(expectedHistoryGroup1, expectedHistoryGroup2), it.historyMetadata) + } + } + + private fun startHistoryMetadataFeature() { + val feature = HistoryMetadataFeature( + homeStore, + historyMetadataStorage, + CoroutineScope(testDispatcher), + testDispatcher + ) + + assertEquals(emptyList(), homeStore.state.historyMetadata) + + feature.start() + + testDispatcher.advanceUntilIdle() + homeStore.waitUntilIdle() + + coVerify { + historyMetadataStorage.getHistoryMetadataSince(any()) + } + } }