From 1d20914f8f4bbafd578e822b3fd641db46a38070 Mon Sep 17 00:00:00 2001 From: MatthewTighe Date: Fri, 8 Jul 2022 15:25:04 -0700 Subject: [PATCH] Fixes 24823: remove recent synced tab when logged out --- .../fenix/components/BackgroundServices.kt | 12 +++ .../org/mozilla/fenix/home/HomeFragment.kt | 20 +++-- .../RecentSyncedTabFeature.kt | 38 ++++++++-- .../RecentSyncedTabFeatureTest.kt | 76 ++++++++++++++----- 4 files changed, 108 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt index 2982cbabc279..b32d2840874a 100644 --- a/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt +++ b/app/src/main/java/org/mozilla/fenix/components/BackgroundServices.kt @@ -30,6 +30,8 @@ import mozilla.components.service.fxa.SyncEngine import mozilla.components.service.fxa.manager.FxaAccountManager import mozilla.components.service.fxa.manager.SCOPE_SESSION import mozilla.components.service.fxa.manager.SCOPE_SYNC +import mozilla.components.service.fxa.store.SyncStore +import mozilla.components.service.fxa.store.SyncStoreSupport import mozilla.components.service.fxa.sync.GlobalSyncableStoreProvider import mozilla.components.service.glean.private.NoExtras import mozilla.components.service.sync.autofill.AutofillCreditCardsAddressesStorage @@ -130,6 +132,12 @@ class BackgroundServices( val accountAbnormalities = AccountAbnormalities(context, crashReporter, strictMode) + val syncStore by lazyMonitored { + SyncStore() + } + + private lateinit var syncStoreSupport: SyncStoreSupport + val accountManager by lazyMonitored { makeAccountManager(context, serverConfig, deviceConfig, syncConfig, crashReporter) } @@ -183,6 +191,10 @@ class BackgroundServices( SyncedTabsIntegration(context, accountManager).launch() + syncStoreSupport = SyncStoreSupport(syncStore, lazyOf(accountManager)).also { + it.initialize() + } + MainScope().launch { accountManager.start() } 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 3381762d1038..bb0c4ce6873e 100644 --- a/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt @@ -154,16 +154,6 @@ class HomeFragment : Fragment() { } } - private val syncedTabFeature by lazy { - RecentSyncedTabFeature( - store = requireComponents.appStore, - context = requireContext(), - storage = requireComponents.backgroundServices.syncedTabsStorage, - accountManager = requireComponents.backgroundServices.accountManager, - lifecycleOwner = viewLifecycleOwner, - ) - } - private var _sessionControlInteractor: SessionControlInteractor? = null private val sessionControlInteractor: SessionControlInteractor get() = _sessionControlInteractor!! @@ -282,7 +272,15 @@ class HomeFragment : Fragment() { if (requireContext().settings().enableTaskContinuityEnhancements) { recentSyncedTabFeature.set( - feature = syncedTabFeature, + feature = RecentSyncedTabFeature( + appStore = requireComponents.appStore, + syncStore = requireComponents.backgroundServices.syncStore, + coroutineScope = viewLifecycleOwner.lifecycleScope, + context = requireContext(), + storage = requireComponents.backgroundServices.syncedTabsStorage, + accountManager = requireComponents.backgroundServices.accountManager, + lifecycleOwner = viewLifecycleOwner, + ), 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 e7da83ce0e31..f9f9149553bb 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 @@ -6,13 +6,20 @@ 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.manager.FxaAccountManager +import mozilla.components.service.fxa.store.SyncStatus +import mozilla.components.service.fxa.store.SyncStore import mozilla.components.support.base.feature.LifecycleAwareFeature +import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.components.appstate.AppAction import mozilla.telemetry.glean.GleanTimerId @@ -21,7 +28,9 @@ import org.mozilla.fenix.GleanMetrics.RecentSyncedTabs /** * Delegate to handle layout updates and dispatch actions related to the recent synced tab. * - * @property store Store to dispatch actions to when synced tabs are updated or errors encountered. + * @property appStore Store to dispatch actions to when synced tabs are updated or errors encountered. + * @property syncStore Store to observe Sync state from. + * @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. @@ -29,7 +38,9 @@ import org.mozilla.fenix.GleanMetrics.RecentSyncedTabs */ @Suppress("LongParameterList") class RecentSyncedTabFeature( - private val store: AppStore, + private val appStore: AppStore, + private val syncStore: SyncStore, + private val coroutineScope: CoroutineScope, accountManager: FxaAccountManager, context: Context, storage: SyncedTabsStorage, @@ -54,8 +65,8 @@ class RecentSyncedTabFeature( override fun startLoading() { syncStartId?.let { RecentSyncedTabs.recentSyncedTabTimeToLoad.cancel(it) } syncStartId = RecentSyncedTabs.recentSyncedTabTimeToLoad.start() - if (store.state.recentSyncedTabState == RecentSyncedTabState.None) { - store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) + if (appStore.state.recentSyncedTabState == RecentSyncedTabState.None) { + appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) } } @@ -74,7 +85,7 @@ class RecentSyncedTabFeature( ) } ?: return recordMetrics(syncedTab, lastSyncedTab, syncStartId) - store.dispatch( + appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(syncedTab)) ) lastSyncedTab = syncedTab @@ -87,18 +98,29 @@ class RecentSyncedTabFeature( */ override fun stopLoading() { if (lastSyncedTab == null) { - store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) + appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } override fun onError(error: SyncedTabsView.ErrorType) { - if (store.state.recentSyncedTabState == RecentSyncedTabState.Loading) { - store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) + 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() { 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 270cc49be750..1fe74a70183d 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 @@ -8,6 +8,12 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import io.mockk.every import io.mockk.mockk import io.mockk.verify +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain import mozilla.components.browser.storage.sync.SyncedDeviceTabs import mozilla.components.browser.storage.sync.Tab import mozilla.components.browser.storage.sync.TabEntry @@ -15,7 +21,11 @@ import mozilla.components.concept.sync.Device import mozilla.components.concept.sync.DeviceType import mozilla.components.feature.syncedtabs.view.SyncedTabsView import mozilla.components.service.fxa.manager.FxaAccountManager +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.glean.testing.GleanTestRule +import mozilla.components.support.test.libstate.ext.waitUntilIdle import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull @@ -68,19 +78,25 @@ class RecentSyncedTabFeatureTest { subscription = null ) - private val store: AppStore = mockk() - private val accountManager: FxaAccountManager = mockk() + private val appStore: AppStore = mockk() + private val accountManager: FxaAccountManager = mockk(relaxed = true) + + private val syncStore = SyncStore() private lateinit var feature: RecentSyncedTabFeature @Before fun setup() { - every { store.dispatch(any()) } returns mockk() + Dispatchers.setMain(StandardTestDispatcher()) + + every { appStore.dispatch(any()) } returns mockk() feature = RecentSyncedTabFeature( - store = store, + appStore = appStore, + syncStore = syncStore, + coroutineScope = TestScope(), accountManager = accountManager, - context = mockk(), + context = mockk(relaxed = true), storage = mockk(), lifecycleOwner = mockk(), ) @@ -88,20 +104,20 @@ class RecentSyncedTabFeatureTest { @Test fun `GIVEN that there is no current state WHEN loading is started THEN loading state is dispatched`() { - every { store.state } returns mockk { + every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } feature.startLoading() - verify { store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) } + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Loading)) } } @Test fun `WHEN empty synced tabs are displayed THEN no action is dispatched`() { feature.displaySyncedTabs(listOf()) - verify(exactly = 0) { store.dispatch(any()) } + verify(exactly = 0) { appStore.dispatch(any()) } } @Test @@ -114,7 +130,7 @@ class RecentSyncedTabFeatureTest { val expectedTab = tab.toRecentSyncedTab(deviceAccessed1) verify { - store.dispatch( + appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) ) } @@ -134,7 +150,7 @@ class RecentSyncedTabFeatureTest { val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) verify { - store.dispatch( + appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) ) } @@ -153,7 +169,7 @@ class RecentSyncedTabFeatureTest { val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1) verify { - store.dispatch( + appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) ) } @@ -173,7 +189,7 @@ class RecentSyncedTabFeatureTest { val expectedTab = secondTab.toRecentSyncedTab(deviceAccessed2) verify { - store.dispatch( + appStore.dispatch( AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab)) ) } @@ -190,7 +206,7 @@ class RecentSyncedTabFeatureTest { @Test fun `GIVEN that tab previously started loading WHEN synced tab displayed THEN load time metric recorded`() { - every { store.state } returns mockk { + every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } val tab = SyncedDeviceTabs(deviceAccessed1, listOf(createActiveTab())) @@ -226,7 +242,7 @@ class RecentSyncedTabFeatureTest { fun `GIVEN that no tab is displayed WHEN stopLoading is called THEN none state dispatched`() { feature.stopLoading() - verify { store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @Test @@ -236,28 +252,41 @@ class RecentSyncedTabFeatureTest { feature.displaySyncedTabs(listOf(tab)) feature.stopLoading() - verify(exactly = 0) { store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + 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`() { - every { store.state } returns mockk { + every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.None } feature.onError(SyncedTabsView.ErrorType.NO_TABS_AVAILABLE) - verify(exactly = 0) { store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + verify(exactly = 0) { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } @Test fun `GIVEN that feature is loading WHEN error received THEN dispatches NONE state`() { - every { store.state } returns mockk { + every { appStore.state } returns mockk { every { recentSyncedTabState } returns RecentSyncedTabState.Loading } feature.onError(SyncedTabsView.ErrorType.MULTIPLE_DEVICES_UNAVAILABLE) - verify { store.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } + } + + @Test + fun `WHEN LoggedOut is observed THEN tab state is dispatched as none`() = runTest { + every { appStore.state } returns mockk { + every { recentSyncedTabState } returns RecentSyncedTabState.None + } + + feature.start() + syncStore.setState(SyncStatus.LoggedOut) + runCurrent() + + verify { appStore.dispatch(AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.None)) } } private fun createActiveTab( @@ -278,4 +307,13 @@ class RecentSyncedTabFeatureTest { url = this.active().url, iconUrl = this.active().iconUrl ) + + private fun SyncStore.setState( + status: SyncStatus? = null, + ) { + status?.let { + this.dispatch(SyncAction.UpdateSyncStatus(status)) + } + this.waitUntilIdle() + } }