Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Fixes #24823: remove recent synced tab when logged out #25956

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -183,6 +191,10 @@ class BackgroundServices(

SyncedTabsIntegration(context, accountManager).launch()

syncStoreSupport = SyncStoreSupport(syncStore, lazyOf(accountManager)).also {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I get a TLDR with what SyncStoreSupport does, since it looks like it's on the AC side?

Copy link
Contributor Author

@MatthewTighe MatthewTighe Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

The SyncStore in general is meant as an abstraction over the FxaAccountManager and some other Sync components. We had two options here:

  • Dig into the internals of FxaAccountManager and dispatch store updates from there.
  • Create a sidecar implementation that could follow the existing patterns of observation and dispatch store updates from there.

We went with the second because it seemed more easy, understandable, and extensible. That implementation is SyncStoreSupport

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, thank you!

it.initialize()
}

MainScope().launch {
accountManager.start()
}
Expand Down
20 changes: 9 additions & 11 deletions app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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!!
Expand Down Expand Up @@ -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
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -21,15 +28,19 @@ 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.
* @param lifecycleOwner View lifecycle owner to determine start/stop state for feature.
*/
@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,
Expand All @@ -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))
}
}

Expand All @@ -74,7 +85,7 @@ class RecentSyncedTabFeature(
)
} ?: return
recordMetrics(syncedTab, lastSyncedTab, syncStartId)
store.dispatch(
appStore.dispatch(
AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(syncedTab))
)
lastSyncedTab = syncedTab
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,24 @@ 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
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
Expand Down Expand Up @@ -68,40 +78,46 @@ 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(),
)
}

@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
Expand All @@ -114,7 +130,7 @@ class RecentSyncedTabFeatureTest {
val expectedTab = tab.toRecentSyncedTab(deviceAccessed1)

verify {
store.dispatch(
appStore.dispatch(
AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab))
)
}
Expand All @@ -134,7 +150,7 @@ class RecentSyncedTabFeatureTest {
val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1)

verify {
store.dispatch(
appStore.dispatch(
AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab))
)
}
Expand All @@ -153,7 +169,7 @@ class RecentSyncedTabFeatureTest {
val expectedTab = remoteTab.toRecentSyncedTab(deviceAccessed1)

verify {
store.dispatch(
appStore.dispatch(
AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab))
)
}
Expand All @@ -173,7 +189,7 @@ class RecentSyncedTabFeatureTest {
val expectedTab = secondTab.toRecentSyncedTab(deviceAccessed2)

verify {
store.dispatch(
appStore.dispatch(
AppAction.RecentSyncedTabStateChange(RecentSyncedTabState.Success(expectedTab))
)
}
Expand All @@ -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()))
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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()
}
}