diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt index f24d576e06e1..e70fb7586bab 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt @@ -14,6 +14,7 @@ import mozilla.components.browser.state.selector.findNormalTab import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.selector.selectedNormalTab import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.SearchState import mozilla.components.browser.state.state.TabSessionState import mozilla.components.feature.search.ext.parseSearchTerms import mozilla.components.lib.state.Middleware @@ -141,40 +142,48 @@ class HistoryMetadataMiddleware( } } - private fun createHistoryMetadata(context: MiddlewareContext, tab: TabSessionState) { + @Suppress("ComplexMethod") + private fun createHistoryMetadata( + context: MiddlewareContext, + tab: TabSessionState + ) { val tabParent = tab.getParent(context.store) val previousUrlIndex = tab.content.history.currentIndex - 1 val tabMetadataHasSearchTerms = !tab.historyMetadata?.searchTerm.isNullOrBlank() // Obtain search terms and referrer url either from tab parent, from the history stack, or // from the tab itself. - // At a high level, there are two main cases here - 1) either the tab was opened as a 'new tab' - // via the search results page, or 2) a page was opened in the same tab as the search results page. - // Details about the New Tab case: - // - we obtain search terms via tab's parent (the search results page) - // - however, it's possible that parent changed (e.g. user navigated away from the search - // results page). - // - our approach below is to capture search terms from the parent within the tab.historyMetadata - // state on the first load of the tab, and then rely on this data for subsequent page loads on that tab. - // - this way, once a tab becomes part of the search group, it won't leave this search group + // + // At a high level, there are two main cases here: + // 1) The tab was opened as a 'new tab' via the search results page. In this case we obtain + // search terms via the tab's parent (the search results page). However, it's possible that + // the parent changed (e.g. user navigated away from the search results page). Our approach + // below is to capture search terms from the parent within the tab.historyMetadata state on + // the first load of the tab, and then rely on this data for subsequent page loads on that + // tab. This way, once a tab becomes part of the search group, it won't leave this group // unless a direct navigation event happens. + // + // 2) A page was opened in the same tab as the search results page (navigated to via content). val (searchTerm, referrerUrl) = when { - // Loading page opened in a New Tab for the first time. + // Page was opened in a new tab. Look for search terms in the parent tab. tabParent != null && !tabMetadataHasSearchTerms -> { - val searchTerms = tabParent.content.searchTerms.takeUnless { it.isEmpty() } - ?: context.state.search.parseSearchTerms(tabParent.content.url) + val searchTerms = findSearchTerms(tabParent, context.state.search) searchTerms to tabParent.content.url } - // We only want to inspect the previous url in history if the user navigated via - // web content i.e., they followed a link, not if the user navigated directly via - // toolbar. + // Page was navigated to via content i.e., the user followed a link. Look for search terms in tab history. !directLoadTriggered && previousUrlIndex >= 0 -> { // Once a tab is within the search group, only a direct load event (via the toolbar) can change that. + val previousUrl = tab.content.history.items[previousUrlIndex].uri val (searchTerms, referrerUrl) = if (tabMetadataHasSearchTerms) { - tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl + tab.historyMetadata?.searchTerm to previousUrl } else { - val previousUrl = tab.content.history.items[previousUrlIndex].uri - context.state.search.parseSearchTerms(previousUrl) to previousUrl + // Find search terms by checking if page is a SERP or a result opened from a SERP + val searchTerms = findSearchTerms(tab, context.state.search) + if (searchTerms != null) { + searchTerms to null + } else { + context.state.search.parseSearchTerms(previousUrl) to previousUrl + } } if (searchTerms != null) { @@ -190,10 +199,11 @@ class HistoryMetadataMiddleware( tabMetadataHasSearchTerms && !(directLoadTriggered && previousUrlIndex >= 0) -> { tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl } - // We had no search terms, no history stack, and no parent. - // This would be the case for any page loaded directly via the toolbar including - // a search results page itself. For now, the original search results page is not - // part of the search group: https://github.com/mozilla-mobile/fenix/issues/21659. + // A page was loaded directly or via content. Check if it is a search result page. + !tabMetadataHasSearchTerms -> { + findSearchTerms(tab, context.state.search) to null + } + // No search terms or referrer were detected. else -> null to null } @@ -218,4 +228,8 @@ class HistoryMetadataMiddleware( store.state.findTab(it) } } + + private fun findSearchTerms(tab: TabSessionState, searchState: SearchState): String? { + return tab.content.searchTerms.takeUnless { it.isEmpty() } ?: searchState.parseSearchTerms(tab.content.url) + } } diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt index 08ee477982a0..6050d0953ed0 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt @@ -114,6 +114,55 @@ class HistoryMetadataMiddlewareTest { } } + @Test + fun `GIVEN normal tab is a search engine result page WHEN history metadata is recorded THEN search terms are provided`() { + service = TestingMetadataService() + middleware = HistoryMetadataMiddleware(service) + val tab = createTab("about:blank") + store = BrowserStore( + middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()), + initialState = BrowserState( + tabs = listOf(tab) + ) + ) + setupGoogleSearchEngine() + + val serpUrl = "https://google.com?q=mozilla+website" + store.dispatch(EngineAction.LoadUrlAction(tab.id, serpUrl)).joinBlocking() + store.dispatch(ContentAction.UpdateUrlAction(tab.id, serpUrl)).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Google Search", serpUrl)), currentIndex = 0)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(1, this.count()) + assertEquals("https://google.com?q=mozilla+website", this[0].url) + assertEquals("mozilla website", this[0].searchTerm) + assertNull(this[0].referrerUrl) + } + } + + @Test + fun `GIVEN normal tab navigates to search engine result page WHEN history metadata is recorded THEN search terms are provided`() { + service = TestingMetadataService() + middleware = HistoryMetadataMiddleware(service) + val tab = createTab("https://google.com") + store = BrowserStore( + middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()), + initialState = BrowserState( + tabs = listOf(tab) + ) + ) + setupGoogleSearchEngine() + + val serpUrl = "https://google.com?q=mozilla+website" + store.dispatch(ContentAction.UpdateUrlAction(tab.id, serpUrl)).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Google Search", "https://google.com"), HistoryItem("Google Search", serpUrl)), currentIndex = 1)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(1, this.count()) + assertEquals("https://google.com?q=mozilla+website", this[0].url) + assertEquals("mozilla website", this[0].searchTerm) + assertNull(this[0].referrerUrl) + } + } + @Test fun `GIVEN tab opened as new tab from a search page WHEN search page navigates away THEN redirecting or navigating in tab retains original search terms`() { service = TestingMetadataService() @@ -132,7 +181,7 @@ class HistoryMetadataMiddlewareTest { with((service as TestingMetadataService).createdMetadata) { assertEquals(2, this.count()) assertEquals("https://google.com?q=mozilla+website", this[0].url) - assertNull(this[0].searchTerm) + assertEquals("mozilla website", this[0].searchTerm) assertNull(this[0].referrerUrl) assertEquals("https://google.com?url=https://mozilla.org", this[1].url) @@ -180,7 +229,7 @@ class HistoryMetadataMiddlewareTest { assertEquals(5, this.count()) assertEquals("https://mozilla.org/manifesto", this[4].url) assertEquals("mozilla website", this[4].searchTerm) - assertEquals("https://google.com?q=mozilla+website", this[4].referrerUrl) + assertEquals("https://mozilla.org", this[4].referrerUrl) } } @@ -202,7 +251,7 @@ class HistoryMetadataMiddlewareTest { with((service as TestingMetadataService).createdMetadata) { assertEquals(2, this.count()) assertEquals("https://google.com?q=mozilla+website", this[0].url) - assertNull(this[0].searchTerm) + assertEquals("mozilla website", this[0].searchTerm) assertNull(this[0].referrerUrl) assertEquals("https://google.com?url=https://mozilla.org", this[1].url) @@ -258,7 +307,7 @@ class HistoryMetadataMiddlewareTest { with((service as TestingMetadataService).createdMetadata) { assertEquals(2, this.count()) assertEquals("https://google.com?q=mozilla+website", this[0].url) - assertNull(this[0].searchTerm) + assertEquals("mozilla website", this[0].searchTerm) assertNull(this[0].referrerUrl) assertEquals("https://google.com?url=https://mozilla.org", this[1].url) @@ -298,7 +347,7 @@ class HistoryMetadataMiddlewareTest { @Test fun `GIVEN normal tab has parent WHEN url is the same THEN nothing happens`() { - val parentTab = createTab("https://mozilla.org", searchTerms = "mozilla website") + val parentTab = createTab("https://mozilla.org") val tab = createTab("https://mozilla.org", parent = parentTab) store.dispatch(TabListAction.AddTabAction(parentTab)).joinBlocking() store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()