diff --git a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt index bb0c4ce6873e..a0e288d83f55 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -275,11 +275,9 @@ class HomeFragment : Fragment() { feature = RecentSyncedTabFeature( appStore = requireComponents.appStore, syncStore = requireComponents.backgroundServices.syncStore, - coroutineScope = viewLifecycleOwner.lifecycleScope, - context = requireContext(), storage = requireComponents.backgroundServices.syncedTabsStorage, accountManager = requireComponents.backgroundServices.accountManager, - lifecycleOwner = viewLifecycleOwner, + coroutineScope = viewLifecycleOwner.lifecycleScope, ), owner = viewLifecycleOwner, view = binding.root diff --git a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt index f9f9149553bb..1a83909350f2 100644 --- a/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt +++ b/app/src/main/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeature.kt @@ -4,20 +4,18 @@ package org.mozilla.fenix.home.recentsyncedtabs -import android.content.Context -import androidx.lifecycle.LifecycleOwner import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach -import mozilla.components.browser.storage.sync.SyncedDeviceTabs import mozilla.components.concept.sync.DeviceType -import mozilla.components.feature.syncedtabs.SyncedTabsFeature import mozilla.components.feature.syncedtabs.storage.SyncedTabsStorage -import mozilla.components.feature.syncedtabs.view.SyncedTabsView import mozilla.components.lib.state.ext.flow +import mozilla.components.service.fxa.SyncEngine import mozilla.components.service.fxa.manager.FxaAccountManager +import mozilla.components.service.fxa.manager.ext.withConstellation import mozilla.components.service.fxa.store.SyncStatus import mozilla.components.service.fxa.store.SyncStore +import mozilla.components.service.fxa.sync.SyncReason import mozilla.components.support.base.feature.LifecycleAwareFeature import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged import org.mozilla.fenix.components.AppStore @@ -29,40 +27,61 @@ import org.mozilla.fenix.GleanMetrics.RecentSyncedTabs * Delegate to handle layout updates and dispatch actions related to the recent synced tab. * * @property appStore Store to dispatch actions to when synced tabs are updated or errors encountered. - * @property syncStore Store to observe Sync state from. + * @property syncStore Store to observe for changes to Sync and account status. + * @property storage Storage layer for synced tabs. + * @property accountManager Account manager to initiate Syncs and refresh devices. * @property coroutineScope The scope to collect Sync state Flow updates in. - * @param accountManager Account manager used to retrieve synced tab state. - * @param context [Context] used for retrieving the sync engine storage state. - * @param storage Storage layer for synced tabs. - * @param lifecycleOwner View lifecycle owner to determine start/stop state for feature. */ -@Suppress("LongParameterList") class RecentSyncedTabFeature( private val appStore: AppStore, private val syncStore: SyncStore, + private val storage: SyncedTabsStorage, + private val accountManager: FxaAccountManager, private val coroutineScope: CoroutineScope, - accountManager: FxaAccountManager, - context: Context, - storage: SyncedTabsStorage, - lifecycleOwner: LifecycleOwner, -) : SyncedTabsView, LifecycleAwareFeature { - private val syncedTabsFeature by lazy { - SyncedTabsFeature( - view = this, - context = context, - storage = storage, - accountManager = accountManager, - lifecycleOwner = lifecycleOwner, - onTabClicked = {} - ) - } - - override var listener: SyncedTabsView.Listener? = null +) : LifecycleAwareFeature { private var syncStartId: GleanTimerId? = null private var lastSyncedTab: RecentSyncedTab? = null - override fun startLoading() { + override fun start() { + collectAccountUpdates() + collectStatusUpdates() + } + + override fun stop() = Unit + + private fun collectAccountUpdates() { + syncStore.flow() + .ifChanged { state -> + state.account != null + }.onEach { state -> + if (state.account != null) { + dispatchLoading() + // Sync tabs storage will fail to retrieve tabs aren't refreshed, as that action + // is what populates the device constellation state + accountManager.withConstellation { refreshDevices() } + accountManager.syncNow(SyncReason.User, customEngineSubset = listOf(SyncEngine.Tabs)) + } + }.launchIn(coroutineScope) + } + + private fun collectStatusUpdates() { + syncStore.flow() + .ifChanged { state -> + state.status + }.onEach { state -> + when (state.status) { + SyncStatus.Idle -> dispatchSyncedTabs() + SyncStatus.Error -> onError() + SyncStatus.LoggedOut -> appStore.dispatch( + AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None) + ) + else -> Unit + } + }.launchIn(coroutineScope) + } + + private fun dispatchLoading() { syncStartId?.let { RecentSyncedTabs.recentSyncedTabTimeToLoad.cancel(it) } syncStartId = RecentSyncedTabs.recentSyncedTabTimeToLoad.start() if (appStore.state.recentSyncedTabState == RecentSyncedTabState.None) { @@ -70,8 +89,8 @@ class RecentSyncedTabFeature( } } - override fun displaySyncedTabs(syncedTabs: List) { - val syncedTab = syncedTabs + private suspend fun dispatchSyncedTabs() { + val syncedTab = storage.getSyncedDeviceTabs() .filterNot { it.device.isCurrentDevice || it.tabs.isEmpty() } .maxByOrNull { it.device.lastAccessTime ?: 0 } ?.let { @@ -91,42 +110,12 @@ class RecentSyncedTabFeature( lastSyncedTab = syncedTab } - /** - * Note: This is called in success cases as well, but the state should only change if there - * isn't a tab displayed. The store's state isn't updated in time to rely on it for this - * condition, so local state is used instead. - */ - override fun stopLoading() { - if (lastSyncedTab == null) { - appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) - } - } - - override fun onError(error: SyncedTabsView.ErrorType) { + private fun onError() { if (appStore.state.recentSyncedTabState == RecentSyncedTabState.Loading) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } - override fun start() { - syncedTabsFeature.start() - syncStore.flow() - .ifChanged { state -> state.status } - .onEach { state -> - when (state.status) { - SyncStatus.LoggedOut -> appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None) - ) - else -> Unit - } - } - .launchIn(coroutineScope) - } - - override fun stop() { - syncedTabsFeature.stop() - } - private fun recordMetrics( tab: RecentSyncedTab, lastSyncedTab: RecentSyncedTab?, diff --git a/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt b/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt index 1fe74a70183d..653901477295 100644 --- a/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt +++ b/app/src/test/java/org/mozilla/fenix/home/recentsyncedtabs/RecentSyncedTabFeatureTest.kt @@ -5,6 +5,8 @@ package org.mozilla.fenix.home.recentsyncedtabs import androidx.test.ext.junit.runners.AndroidJUnit4 +import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import io.mockk.verify @@ -19,11 +21,15 @@ import mozilla.components.browser.storage.sync.Tab import mozilla.components.browser.storage.sync.TabEntry import mozilla.components.concept.sync.Device import mozilla.components.concept.sync.DeviceType -import mozilla.components.feature.syncedtabs.view.SyncedTabsView +import mozilla.components.feature.syncedtabs.storage.SyncedTabsStorage +import mozilla.components.service.fxa.SyncEngine import mozilla.components.service.fxa.manager.FxaAccountManager +import mozilla.components.service.fxa.manager.ext.withConstellation +import mozilla.components.service.fxa.store.Account import mozilla.components.service.fxa.store.SyncAction import mozilla.components.service.fxa.store.SyncStatus import mozilla.components.service.fxa.store.SyncStore +import mozilla.components.service.fxa.sync.SyncReason import mozilla.components.service.glean.testing.GleanTestRule import mozilla.components.support.test.libstate.ext.waitUntilIdle import mozilla.components.support.test.robolectric.testContext @@ -80,6 +86,7 @@ class RecentSyncedTabFeatureTest { private val appStore: AppStore = mockk() private val accountManager: FxaAccountManager = mockk(relaxed = true) + private val storage: SyncedTabsStorage = mockk() private val syncStore = SyncStore() @@ -94,61 +101,94 @@ class RecentSyncedTabFeatureTest { feature = RecentSyncedTabFeature( appStore = appStore, syncStore = syncStore, - coroutineScope = TestScope(), accountManager = accountManager, - context = mockk(relaxed = true), - storage = mockk(), - lifecycleOwner = mockk(), + storage = storage, + coroutineScope = TestScope(), ) } @Test - fun `GIVEN that there is no current state WHEN loading is started THEN loading state is dispatched`() { + fun `GIVEN account is not available WHEN started THEN nothing is dispatched`() { + feature.start() + + verify(exactly = 0) { appStore.dispatch(any()) } + } + + @Test + fun `GIVEN current tab state is none WHEN account becomes available THEN loading state is dispatched, devices are refreshed, and a sync is started`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } - feature.startLoading() + feature.start() + runCurrent() verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) } + coVerify { accountManager.withConstellation { refreshDevices() } } + coVerify { accountManager.syncNow(reason = SyncReason.User, debounce = false, customEngineSubset = listOf(SyncEngine.Tabs)) } } @Test - fun `WHEN empty synced tabs are displayed THEN no action is dispatched`() { - feature.displaySyncedTabs(listOf()) + fun `GIVEN current tab state is not none WHEN account becomes available THEN loading state is not dispatched`() = runTest { + val account = mockk() + syncStore.setState(account = account) - verify(exactly = 0) { appStore.dispatch(any()) } + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } + + feature.start() + runCurrent() + + verify(exactly = 0) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) } } @Test - fun `WHEN displaying synced tabs THEN first active tab is used`() { - val tab = createActiveTab("title", "https://mozilla.org", null) - val displayedTabs = listOf(SyncedDeviceTabs(deviceAccessed1, listOf(tab))) - - feature.displaySyncedTabs(displayedTabs) + fun `GIVEN synced tabs WHEN status becomes idle THEN recent synced tab is dispatched`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } + val activeTab = createActiveTab() + coEvery { storage.getSyncedDeviceTabs() } returns listOf( + SyncedDeviceTabs( + device = deviceAccessed1, + tabs = listOf(activeTab) + ) + ) - val expectedTab = tab.toRecentSyncedTab(deviceAccessed1) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() - verify { - appStore.dispatch( - AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) - ) - } + val expected = activeTab.toRecentSyncedTab(deviceAccessed1) + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } } @Test - fun `WHEN displaying synced tabs THEN current device is filtered out`() { + fun `GIVEN tabs from remote and current devices WHEN dispatching recent synced tab THEN current device is filtered out of dispatch`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } val localTab = createActiveTab("local", "https://local.com", null) val remoteTab = createActiveTab("remote", "https://mozilla.org", null) - val displayedTabs = listOf( + val syncedTabs = listOf( SyncedDeviceTabs(currentDevice, listOf(localTab)), SyncedDeviceTabs(deviceAccessed1, listOf(remoteTab)) ) + coEvery { storage.getSyncedDeviceTabs() } returns syncedTabs - feature.displaySyncedTabs(displayedTabs) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) - verify { appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) @@ -157,17 +197,24 @@ class RecentSyncedTabFeatureTest { } @Test - fun `WHEN displaying synced tabs THEN any devices with empty tabs list are filtered out`() { + fun `GIVEN there are devices with empty tabs list WHEN dispatching recent synced tab THEN devices with empty tabs list are filtered out`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } val remoteTab = createActiveTab("remote", "https://mozilla.org", null) - val displayedTabs = listOf( + val syncedTabs = listOf( SyncedDeviceTabs(deviceAccessed2, listOf()), SyncedDeviceTabs(deviceAccessed1, listOf(remoteTab)) ) + coEvery { storage.getSyncedDeviceTabs() } returns syncedTabs - feature.displaySyncedTabs(displayedTabs) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) - verify { appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) @@ -176,18 +223,25 @@ class RecentSyncedTabFeatureTest { } @Test - fun `WHEN displaying synced tabs THEN most recently accessed device is used`() { + fun `GIVEN tabs from different remote devices WHEN dispatching recent synced tab THEN most recently accessed device is used`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } val firstTab = createActiveTab("first", "https://local.com", null) val secondTab = createActiveTab("remote", "https://mozilla.org", null) - val displayedTabs = listOf( + val syncedTabs = listOf( SyncedDeviceTabs(deviceAccessed1, listOf(firstTab)), SyncedDeviceTabs(deviceAccessed2, listOf(secondTab)) ) + coEvery { storage.getSyncedDeviceTabs() } returns syncedTabs - feature.displaySyncedTabs(displayedTabs) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() val expectedTab = secondTab.toRecentSyncedTab(deviceAccessed2) - verify { appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) @@ -196,96 +250,131 @@ class RecentSyncedTabFeatureTest { } @Test - fun `WHEN synced tab displayed THEN labeled counter metric recorded with device type`() { + fun `WHEN synced tab dispatched THEN labeled counter metric recorded with device type`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.Loading + } val tab = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) + coEvery { storage.getSyncedDeviceTabs() } returns listOf(tab) - feature.displaySyncedTabs(listOf(tab)) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() assertEquals(1, RecentSyncedTabs.recentSyncedTabShown["desktop"].testGetValue()) } @Test - fun `GIVEN that tab previously started loading WHEN synced tab displayed THEN load time metric recorded`() { + fun `WHEN synced tab dispatched THEN load time metric recorded`() = runTest { + val account = mockk() + syncStore.setState(account = account) every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } val tab = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) + coEvery { storage.getSyncedDeviceTabs() } returns listOf(tab) - feature.startLoading() - feature.displaySyncedTabs(listOf(tab)) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() assertNotNull(RecentSyncedTabs.recentSyncedTabTimeToLoad.testGetValue()) } @Test - fun `GIVEN that the displayed tab was the last displayed tab WHEN displayed THEN recorded as stale`() { + fun `GIVEN that the dispatched tab was the last dispatched tab WHEN dispatched THEN recorded as stale`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.None + } val tab = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) + coEvery { storage.getSyncedDeviceTabs() } returns listOf(tab) - feature.displaySyncedTabs(listOf(tab)) - feature.displaySyncedTabs(listOf(tab)) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() + syncStore.setState(status = SyncStatus.Started) + runCurrent() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() assertEquals(1, RecentSyncedTabs.latestSyncedTabIsStale.testGetValue()) } @Test - fun `GIVEN that the displayed tab was not the last displayed tab WHEN displayed THEN not recorded as stale`() { - val tab1 = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) - val tab2 = SyncedDeviceTabs(deviceAccessed2, listOf(createActiveTab())) + fun `GIVEN that the dispatched tab was not the last dispatched tab WHEN dispatched THEN not recorded as stale`() = runTest { + val account = mockk() + syncStore.setState(account = account) + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.None + } + val tabs1 = listOf(SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab()))) + val tabs2 = listOf(SyncedDeviceTabs(deviceAccessed2, listOf(createActiveTab()))) + coEvery { storage.getSyncedDeviceTabs() } returnsMany listOf(tabs1, tabs2) - feature.displaySyncedTabs(listOf(tab1)) - feature.displaySyncedTabs(listOf(tab2)) + feature.start() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() + syncStore.setState(status = SyncStatus.Started) + runCurrent() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() assertNull(RecentSyncedTabs.latestSyncedTabIsStale.testGetValue()) } @Test - fun `GIVEN that no tab is displayed WHEN stopLoading is called THEN none state dispatched`() { - feature.stopLoading() - - verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } - } - - @Test - fun `GIVEN that a tab is displayed WHEN stopLoading is called THEN nothing dispatched`() { - val tab = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) - - feature.displaySyncedTabs(listOf(tab)) - feature.stopLoading() - - verify(exactly = 0) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } - } - - @Test - fun `GIVEN that feature is not loading WHEN error received THEN does not dispatch NONE state`() { + fun `GIVEN current tab state is loading WHEN error is observed THEN tab state is dispatched as none`() = runTest { + val account = mockk() + syncStore.setState(account = account) every { appStore.state } returns mockk { - every { recentSyncedTabState } returns RecentSyncedTabState.None + every { recentSyncedTabState } returnsMany listOf( + RecentSyncedTabState.None, + RecentSyncedTabState.Loading + ) } - feature.onError(SyncedTabsView.ErrorType.NO_TABS_AVAILABLE) - verify(exactly = 0) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + feature.start() + runCurrent() + syncStore.setState(status = SyncStatus.Error) + runCurrent() + + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @Test - fun `GIVEN that feature is loading WHEN error received THEN dispatches NONE state`() { - every { appStore.state } returns mockk { - every { recentSyncedTabState } returns RecentSyncedTabState.Loading - } - - feature.onError(SyncedTabsView.ErrorType.MULTIPLE_DEVICES_UNAVAILABLE) + fun `GIVEN current tab state is not loading WHEN error is observed THEN nothing is dispatched`() = runTest { + feature.start() + syncStore.setState(status = SyncStatus.Error) + runCurrent() - verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + verify(exactly = 0) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @Test - fun `WHEN LoggedOut is observed THEN tab state is dispatched as none`() = runTest { + fun `GIVEN that a tab has been dispatched WHEN LoggedOut is observed THEN tab state is dispatched as none`() = runTest { + val account = mockk() + syncStore.setState(account = account) every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } + val tab = createActiveTab() + coEvery { storage.getSyncedDeviceTabs() } returns listOf( + SyncedDeviceTabs(deviceAccessed1, listOf(tab)) + ) feature.start() - syncStore.setState(SyncStatus.LoggedOut) + runCurrent() + syncStore.setState(status = SyncStatus.Idle) + runCurrent() + syncStore.setState(status = SyncStatus.LoggedOut) runCurrent() + val expected = tab.toRecentSyncedTab(deviceAccessed1) + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expected))) } verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @@ -310,10 +399,14 @@ class RecentSyncedTabFeatureTest { private fun SyncStore.setState( status: SyncStatus? = null, + account: Account? = null, ) { status?.let { this.dispatch(SyncAction.UpdateSyncStatus(status)) } + account?.let { + this.dispatch(SyncAction.UpdateAccount(account)) + } this.waitUntilIdle() } }