Closes #23517: history items are not being removed after deletion

app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt

@@ -16,7 +16,6 @@ import org.mozilla.fenix.library.history.History
 import org.mozilla.fenix.library.history.HistoryItemTimeGroup
 import org.mozilla.fenix.perf.runBlockingIncrement
 import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES
-import kotlin.math.abs
 
 private const val BUFFER_TIME = 15000 /* 15 seconds in ms */
 
@@ -155,37 +154,17 @@ class DefaultPagedHistoryProvider(
      * Removes [group] and any corresponding history visits.
      */
     suspend fun deleteMetadataSearchGroup(group: History.Group) {
+        // The intention is to delete items from history for good.
+        // Corresponding metadata items would also be removed,
+        // because of ON DELETE CASCADE relation in DB schema.
         for (historyMetadata in group.items) {
-            getMatchingHistory(historyMetadata)?.let {
-                historyStorage.deleteVisit(
-                    url = it.url,
-                    timestamp = it.visitTime
-                )
-            }
+            historyStorage.deleteVisitsFor(historyMetadata.url)
         }
 
-        historyStorage.deleteHistoryMetadata(
-            searchTerm = group.title
-        )
-
         // Force a re-fetch of the groups next time we go through #getHistory.
         historyGroups = null
     }
 
-    /**
-     * Returns the [History.Regular] corresponding to the given [History.Metadata] item.
-     */
-    private suspend fun getMatchingHistory(historyMetadata: History.Metadata): VisitInfo? {
-        val history = historyStorage.getDetailedVisits(
-            start = historyMetadata.visitedAt - BUFFER_TIME,
-            end = historyMetadata.visitedAt + BUFFER_TIME,
-            excludeTypes = excludedVisitTypes
-        )
-        return history
-            .filter { it.url == historyMetadata.url }
-            .minByOrNull { abs(historyMetadata.visitedAt - it.visitTime) }
-    }
-
     @Suppress("MagicNumber")
     private suspend fun getHistoryAndSearchGroups(
         offset: Int,

app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt

@@ -367,7 +367,7 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler {
                         analytics.metrics.track(Event.HistoryItemRemoved)
 
                         when (item) {
-                            is History.Regular -> core.historyStorage.deleteVisit(item.url, item.visitedAt)
+                            is History.Regular -> core.historyStorage.deleteVisitsFor(item.url)
                             is History.Group -> {
                                 // NB: If we have non-search groups, this logic needs to be updated.
                                 historyProvider.deleteMetadataSearchGroup(item)

app/src/main/java/org/mozilla/fenix/library/historymetadata/HistoryMetadataGroupFragment.kt

@@ -26,6 +26,7 @@ import org.mozilla.fenix.ext.nav
 import org.mozilla.fenix.ext.requireComponents
 import org.mozilla.fenix.ext.setTextColor
 import org.mozilla.fenix.ext.showToolbar
+import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.library.LibraryPageFragment
 import org.mozilla.fenix.library.history.History
 import org.mozilla.fenix.library.historymetadata.controller.DefaultHistoryMetadataGroupController
@@ -72,7 +73,8 @@ class HistoryMetadataGroupFragment :
 
         interactor = DefaultHistoryMetadataGroupInteractor(
             controller = DefaultHistoryMetadataGroupController(
-                activity = activity as HomeActivity,
+                historyStorage = (activity as HomeActivity).components.core.historyStorage,
+                browserStore = (activity as HomeActivity).components.core.store,
                 store = historyMetadataGroupStore,
                 selectOrAddUseCase = requireComponents.useCases.tabsUseCases.selectOrAddTab,
                 metrics = requireComponents.analytics.metrics,

app/src/main/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupController.kt

@@ -7,13 +7,14 @@ package org.mozilla.fenix.library.historymetadata.controller
 import androidx.navigation.NavController
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.launch
+import mozilla.components.browser.state.action.HistoryMetadataAction
+import mozilla.components.browser.state.store.BrowserStore
+import mozilla.components.browser.storage.sync.PlacesHistoryStorage
 import mozilla.components.concept.engine.prompt.ShareData
 import mozilla.components.feature.tabs.TabsUseCases
-import org.mozilla.fenix.HomeActivity
 import org.mozilla.fenix.components.metrics.Event
 import org.mozilla.fenix.components.metrics.MetricController
 import org.mozilla.fenix.R
-import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.library.history.History
 import org.mozilla.fenix.library.historymetadata.HistoryMetadataGroupFragmentAction
 import org.mozilla.fenix.library.historymetadata.HistoryMetadataGroupFragmentDirections
@@ -75,7 +76,8 @@ interface HistoryMetadataGroupController {
  * The default implementation of [HistoryMetadataGroupController].
  */
 class DefaultHistoryMetadataGroupController(
-    private val activity: HomeActivity,
+    private val historyStorage: PlacesHistoryStorage,
+    private val browserStore: BrowserStore,
     private val store: HistoryMetadataGroupFragmentStore,
     private val selectOrAddUseCase: TabsUseCases.SelectOrAddUseCase,
     private val metrics: MetricController,
@@ -117,18 +119,31 @@ class DefaultHistoryMetadataGroupController(
 
     override fun handleDelete(items: Set<History.Metadata>) {
         scope.launch {
+            val isDeletingLastItem = store.state.items.size == items.size
             items.forEach {
                 store.dispatch(HistoryMetadataGroupFragmentAction.Delete(it))
-                activity.components.core.historyStorage.deleteHistoryMetadata(it.historyMetadataKey)
+                historyStorage.deleteVisitsFor(it.url)
                 metrics.track(Event.HistorySearchTermGroupRemoveTab)
             }
+            // The method is called for both single and multiple items.
+            // In case all items have been deleted, we have to disband the search group.
+            if (isDeletingLastItem) {
+                browserStore.dispatch(
+                    HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = searchTerm)
+                )
+            }
         }
     }
 
     override fun handleDeleteAll() {
         scope.launch {
             store.dispatch(HistoryMetadataGroupFragmentAction.DeleteAll)
-            activity.components.core.historyStorage.deleteHistoryMetadata(searchTerm)
+            store.state.items.forEach {
+                historyStorage.deleteVisitsFor(it.url)
+            }
+            browserStore.dispatch(
+                HistoryMetadataAction.DisbandSearchGroupAction(searchTerm = searchTerm)
+            )
             metrics.track(Event.HistorySearchTermGroupRemoveAll)
         }
     }

app/src/test/java/org/mozilla/fenix/library/historymetadata/controller/HistoryMetadataGroupControllerTest.kt

@@ -11,6 +11,7 @@ import io.mockk.mockk
 import io.mockk.verify
 import kotlinx.coroutines.test.TestCoroutineScope
 import kotlinx.coroutines.test.runBlockingTest
+import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.browser.storage.sync.PlacesHistoryStorage
 import mozilla.components.concept.engine.prompt.ShareData
 import mozilla.components.concept.storage.HistoryMetadataKey
@@ -46,6 +47,7 @@ class HistoryMetadataGroupControllerTest {
 
     private val activity: HomeActivity = mockk(relaxed = true)
     private val store: HistoryMetadataGroupFragmentStore = mockk(relaxed = true)
+    private val browserStore: BrowserStore = mockk(relaxed = true)
     private val selectOrAddUseCase: TabsUseCases.SelectOrAddUseCase = mockk(relaxed = true)
     private val metrics: MetricController = mockk(relaxed = true)
     private val navController: NavController = mockk(relaxed = true)
@@ -74,10 +76,13 @@ class HistoryMetadataGroupControllerTest {
 
     private lateinit var controller: DefaultHistoryMetadataGroupController
 
+    private fun getMetadataItemsList() = listOf(mozillaHistoryMetadataItem, firefoxHistoryMetadataItem)
+
     @Before
     fun setUp() {
         controller = DefaultHistoryMetadataGroupController(
-            activity = activity,
+            historyStorage = historyStorage,
+            browserStore = browserStore,
             store = store,
             selectOrAddUseCase = selectOrAddUseCase,
             metrics = metrics,
@@ -87,6 +92,7 @@ class HistoryMetadataGroupControllerTest {
         )
 
         every { activity.components.core.historyStorage } returns historyStorage
+        every { store.state.items } returns getMetadataItemsList()
     }
 
     @After
@@ -99,7 +105,10 @@ class HistoryMetadataGroupControllerTest {
         controller.handleOpen(mozillaHistoryMetadataItem)
 
         verify {
-            selectOrAddUseCase.invoke(mozillaHistoryMetadataItem.url, mozillaHistoryMetadataItem.historyMetadataKey)
+            selectOrAddUseCase.invoke(
+                mozillaHistoryMetadataItem.url,
+                mozillaHistoryMetadataItem.historyMetadataKey
+            )
             navController.navigate(R.id.browserFragment)
             metrics.track(Event.HistorySearchTermGroupOpenTab)
         }
@@ -158,13 +167,13 @@ class HistoryMetadataGroupControllerTest {
 
     @Test
     fun handleDelete() = testDispatcher.runBlockingTest {
-        controller.handleDelete(setOf(mozillaHistoryMetadataItem, firefoxHistoryMetadataItem))
+        controller.handleDelete(getMetadataItemsList().toSet())
 
         coVerify {
-            store.dispatch(HistoryMetadataGroupFragmentAction.Delete(mozillaHistoryMetadataItem))
-            store.dispatch(HistoryMetadataGroupFragmentAction.Delete(firefoxHistoryMetadataItem))
-            historyStorage.deleteHistoryMetadata(mozillaHistoryMetadataItem.historyMetadataKey)
-            historyStorage.deleteHistoryMetadata(firefoxHistoryMetadataItem.historyMetadataKey)
+            getMetadataItemsList().forEach {
+                store.dispatch(HistoryMetadataGroupFragmentAction.Delete(it))
+                historyStorage.deleteVisitsFor(it.url)
+            }
             metrics.track(Event.HistorySearchTermGroupRemoveTab)
         }
     }
@@ -175,7 +184,9 @@ class HistoryMetadataGroupControllerTest {
 
         coVerify {
             store.dispatch(HistoryMetadataGroupFragmentAction.DeleteAll)
-            historyStorage.deleteHistoryMetadata(searchTerm)
+            getMetadataItemsList().forEach {
+                historyStorage.deleteVisitsFor(it.url)
+            }
             metrics.track(Event.HistorySearchTermGroupRemoveAll)
         }
     }