diff --git a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt index 188be699e..808cd37da 100644 --- a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt +++ b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt @@ -8,14 +8,14 @@ package org.mozilla.fenix.collections import androidx.annotation.VisibleForTesting import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.launch import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager import mozilla.components.feature.tab.collections.TabCollection -import org.mozilla.fenix.components.Analytics import org.mozilla.fenix.components.TabCollectionStorage import org.mozilla.fenix.components.metrics.Event +import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.home.Tab interface CollectionCreationController { @@ -65,10 +65,10 @@ fun List.toSessionBundle(sessionManager: SessionManager): List { class DefaultCollectionCreationController( private val store: CollectionCreationStore, private val dismiss: () -> Unit, - private val analytics: Analytics, + private val metrics: MetricController, private val tabCollectionStorage: TabCollectionStorage, private val sessionManager: SessionManager, - private val viewLifecycleScope: CoroutineScope + private val scope: CoroutineScope ) : CollectionCreationController { companion object { @@ -79,26 +79,31 @@ class DefaultCollectionCreationController( override fun saveCollectionName(tabs: List, name: String) { dismiss() - val sessionBundle = tabs.toList().toSessionBundle(sessionManager) - viewLifecycleScope.launch(Dispatchers.IO) { + val sessionBundle = tabs.toSessionBundle(sessionManager) + scope.launch(IO) { tabCollectionStorage.createCollection(name, sessionBundle) } - analytics.metrics.track( + metrics.track( Event.CollectionSaved(normalSessionSize(sessionManager), sessionBundle.size) ) } override fun renameCollection(collection: TabCollection, name: String) { dismiss() - viewLifecycleScope.launch(Dispatchers.IO) { + scope.launch(IO) { tabCollectionStorage.renameCollection(collection, name) - analytics.metrics.track(Event.CollectionRenamed) } + metrics.track(Event.CollectionRenamed) } override fun backPressed(fromStep: SaveCollectionStep) { - handleBackPress(fromStep) + val newStep = stepBack(fromStep) + if (newStep != null) { + store.dispatch(CollectionCreationAction.StepChanged(newStep)) + } else { + dismiss() + } } override fun selectAllTabs() { @@ -116,12 +121,12 @@ class DefaultCollectionCreationController( override fun selectCollection(collection: TabCollection, tabs: List) { dismiss() val sessionBundle = tabs.toList().toSessionBundle(sessionManager) - viewLifecycleScope.launch(Dispatchers.IO) { + scope.launch(IO) { tabCollectionStorage .addTabsToCollection(collection, sessionBundle) } - analytics.metrics.track( + metrics.track( Event.CollectionTabsAdded(normalSessionSize(sessionManager), sessionBundle.size) ) } @@ -171,25 +176,15 @@ class DefaultCollectionCreationController( store.dispatch(CollectionCreationAction.TabRemoved(tab)) } - private fun handleBackPress(backFromStep: SaveCollectionStep) { - val newStep = stepBack(backFromStep) - if (newStep != null) { - store.dispatch(CollectionCreationAction.StepChanged(newStep)) - } else { - dismiss() - } - } - + /** + * Will return the next valid state according to this diagram. + * + * Name Collection -> Select Collection -> Select Tabs -> (dismiss fragment) <- Rename Collection + */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun stepBack( backFromStep: SaveCollectionStep ): SaveCollectionStep? { - /* - Will return the next valid state according to this diagram. - - Name Collection -> Select Collection -> Select Tabs -> (dismiss fragment) <- Rename Collection - */ - val tabCollectionCount = store.state.tabCollections.size val tabCount = store.state.tabs.size diff --git a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationFragment.kt b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationFragment.kt index 4937d076e..8c32f3cb0 100644 --- a/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationFragment.kt @@ -74,10 +74,10 @@ class CollectionCreationFragment : DialogFragment() { DefaultCollectionCreationController( collectionCreationStore, ::dismiss, - requireComponents.analytics, + requireComponents.analytics.metrics, requireComponents.core.tabCollectionStorage, requireComponents.core.sessionManager, - viewLifecycleOwner.lifecycleScope + scope = lifecycleScope ) ) collectionCreationView = CollectionCreationView( diff --git a/app/src/test/java/org/mozilla/fenix/collections/DefaultCollectionCreationControllerTest.kt b/app/src/test/java/org/mozilla/fenix/collections/DefaultCollectionCreationControllerTest.kt index 4148868c9..af33df467 100644 --- a/app/src/test/java/org/mozilla/fenix/collections/DefaultCollectionCreationControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/collections/DefaultCollectionCreationControllerTest.kt @@ -5,48 +5,167 @@ import io.mockk.every import io.mockk.impl.annotations.MockK import io.mockk.mockk import io.mockk.verify +import io.mockk.verifyAll import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineScope +import kotlinx.coroutines.test.runBlockingTest import mozilla.components.browser.session.Session import mozilla.components.browser.session.SessionManager +import mozilla.components.browser.state.state.MediaState import mozilla.components.feature.tab.collections.TabCollection -import mozilla.components.feature.tabs.TabsUseCases import org.junit.Assert.assertEquals import org.junit.Assert.assertNull import org.junit.Before import org.junit.Test -import org.mozilla.fenix.components.Analytics import org.mozilla.fenix.components.TabCollectionStorage +import org.mozilla.fenix.components.metrics.Event +import org.mozilla.fenix.components.metrics.MetricController +import org.mozilla.fenix.home.Tab @ExperimentalCoroutinesApi class DefaultCollectionCreationControllerTest { private val testCoroutineScope = TestCoroutineScope() - + private lateinit var state: CollectionCreationState private lateinit var controller: DefaultCollectionCreationController @MockK(relaxed = true) private lateinit var store: CollectionCreationStore @MockK(relaxed = true) private lateinit var dismiss: () -> Unit - @MockK(relaxed = true) private lateinit var analytics: Analytics - @MockK private lateinit var tabCollectionStorage: TabCollectionStorage - @MockK private lateinit var tabsUseCases: TabsUseCases + @MockK(relaxUnitFun = true) private lateinit var metrics: MetricController + @MockK(relaxUnitFun = true) private lateinit var tabCollectionStorage: TabCollectionStorage @MockK private lateinit var sessionManager: SessionManager - @MockK private lateinit var state: CollectionCreationState @Before fun before() { MockKAnnotations.init(this) - every { store.state } returns state - every { state.tabCollections } returns emptyList() - every { state.tabs } returns emptyList() + state = CollectionCreationState( + tabCollections = emptyList(), + tabs = emptyList() + ) + every { store.state } answers { state } controller = DefaultCollectionCreationController( - store, dismiss, analytics, + store, dismiss, metrics, tabCollectionStorage, sessionManager, testCoroutineScope ) } + @Test + fun `GIVEN tab list WHEN saveCollectionName is called THEN collection should be created`() { + val session = mockSession(sessionId = "session-1") + val sessions = listOf( + session, + mockSession(sessionId = "session-2") + ) + every { sessionManager.findSessionById("session-1") } returns session + every { sessionManager.findSessionById("null-session") } returns null + every { sessionManager.sessions } returns sessions + val tabs = listOf( + Tab("session-1", "", "", "", mediaState = MediaState.State.NONE), + Tab("null-session", "", "", "", mediaState = MediaState.State.NONE) + ) + + controller.saveCollectionName(tabs, "name") + + verify { dismiss() } + verify { tabCollectionStorage.createCollection("name", listOf(session)) } + verify { metrics.track(Event.CollectionSaved(2, 1)) } + } + + @Test + fun `GIVEN name collection WHEN backPressed is called THEN next step should be dispatched`() { + state = state.copy(tabCollections = listOf(mockk())) + controller.backPressed(SaveCollectionStep.NameCollection) + verify { store.dispatch(CollectionCreationAction.StepChanged(SaveCollectionStep.SelectCollection)) } + + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk(), mockk())) + controller.backPressed(SaveCollectionStep.NameCollection) + verify { store.dispatch(CollectionCreationAction.StepChanged(SaveCollectionStep.SelectTabs)) } + + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk())) + controller.backPressed(SaveCollectionStep.NameCollection) + verify { dismiss() } + } + + @Test + fun `GIVEN select collection WHEN backPressed is called THEN next step should be dispatched`() { + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk(), mockk())) + controller.backPressed(SaveCollectionStep.SelectCollection) + verify { store.dispatch(CollectionCreationAction.StepChanged(SaveCollectionStep.SelectTabs)) } + + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk())) + controller.backPressed(SaveCollectionStep.SelectCollection) + verify { dismiss() } + } + + @Test + fun `GIVEN last step WHEN backPressed is called THEN dismiss should be called`() { + controller.backPressed(SaveCollectionStep.SelectTabs) + verify { dismiss() } + + controller.backPressed(SaveCollectionStep.RenameCollection) + verify { dismiss() } + } + + @Test + fun `GIVEN collection WHEN renameCollection is called THEN collection should be renamed`() = testCoroutineScope.runBlockingTest { + val collection = mockk() + + controller.renameCollection(collection, "name") + advanceUntilIdle() + + verifyAll { + dismiss() + tabCollectionStorage.renameCollection(collection, "name") + metrics.track(Event.CollectionRenamed) + } + } + + @Test + fun `WHEN select all is called THEN add all should be dispatched`() { + controller.selectAllTabs() + verify { store.dispatch(CollectionCreationAction.AddAllTabs) } + + controller.deselectAllTabs() + verify { store.dispatch(CollectionCreationAction.RemoveAllTabs) } + + controller.close() + verify { dismiss() } + } + + @Test + fun `WHEN select tab is called THEN add tab should be dispatched`() { + val tab = mockk() + + controller.addTabToSelection(tab) + verify { store.dispatch(CollectionCreationAction.TabAdded(tab)) } + + controller.removeTabFromSelection(tab) + verify { store.dispatch(CollectionCreationAction.TabRemoved(tab)) } + } + + @Test + fun `WHEN selectCollection is called THEN add tabs should be added to collection`() { + val session = mockSession(sessionId = "session-1") + val sessions = listOf( + session, + mockSession(sessionId = "session-2") + ) + every { sessionManager.findSessionById("session-1") } returns session + every { sessionManager.sessions } returns sessions + val tabs = listOf( + Tab("session-1", "", "", "", mediaState = MediaState.State.NONE) + ) + val collection = mockk() + + controller.selectCollection(collection, tabs) + + verify { dismiss() } + verify { tabCollectionStorage.addTabsToCollection(collection, listOf(session)) } + verify { metrics.track(Event.CollectionTabsAdded(2, 1)) } + } + @Test fun `GIVEN previous step was SelectTabs or RenameCollection WHEN stepBack is called THEN null should be returned`() { assertNull(controller.stepBack(SaveCollectionStep.SelectTabs)) @@ -55,83 +174,79 @@ class DefaultCollectionCreationControllerTest { @Test fun `GIVEN previous step was SelectCollection AND more than one tab is open WHEN stepBack is called THEN SelectTabs should be returned`() { - every { state.tabs } returns listOf(mockk(), mockk()) + state = state.copy(tabs = listOf(mockk(), mockk())) assertEquals(SaveCollectionStep.SelectTabs, controller.stepBack(SaveCollectionStep.SelectCollection)) } @Test fun `GIVEN previous step was SelectCollection AND one or fewer tabs are open WHEN stepbback is called THEN null should be returned`() { - every { state.tabs } returns listOf(mockk()) + state = state.copy(tabs = listOf(mockk())) assertNull(controller.stepBack(SaveCollectionStep.SelectCollection)) - every { state.tabs } returns emptyList() + state = state.copy(tabs = emptyList()) assertNull(controller.stepBack(SaveCollectionStep.SelectCollection)) } @Test fun `GIVEN previous step was NameCollection AND tabCollections is empty AND more than one tab is open WHEN stepBack is called THEN SelectTabs should be returned`() { - every { state.tabCollections } returns emptyList() - every { state.tabs } returns listOf(mockk(), mockk()) + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk(), mockk())) assertEquals(SaveCollectionStep.SelectTabs, controller.stepBack(SaveCollectionStep.NameCollection)) } @Test fun `GIVEN previous step was NameCollection AND tabCollections is empty AND one or fewer tabs are open WHEN stepBack is called THEN null should be returned`() { - every { state.tabCollections } returns emptyList() - every { state.tabs } returns listOf(mockk()) + state = state.copy(tabCollections = emptyList(), tabs = listOf(mockk())) assertNull(controller.stepBack(SaveCollectionStep.NameCollection)) - every { state.tabCollections } returns emptyList() - every { state.tabs } returns emptyList() + state = state.copy(tabCollections = emptyList(), tabs = emptyList()) assertNull(controller.stepBack(SaveCollectionStep.NameCollection)) } @Test fun `GIVEN previous step was NameCollection AND tabCollections is not empty WHEN stepBack is called THEN SelectCollection should be returned`() { - every { state.tabCollections } returns listOf(mockk()) - + state = state.copy(tabCollections = listOf(mockk())) assertEquals(SaveCollectionStep.SelectCollection, controller.stepBack(SaveCollectionStep.NameCollection)) } @Test fun `GIVEN list of collections WHEN default collection number is required THEN return next default number`() { - val collections: MutableList = ArrayList() - collections.add(mockk { - every { title } returns "Collection 1" - }) - collections.add(mockk { - every { title } returns "Collection 2" - }) - collections.add(mockk { - every { title } returns "Collection 3" - }) - every { state.tabCollections } returns collections - + val collections = mutableListOf( + mockk { + every { title } returns "Collection 1" + }, + mockk { + every { title } returns "Collection 2" + }, + mockk { + every { title } returns "Collection 3" + } + ) + state = state.copy(tabCollections = collections) assertEquals(4, controller.getDefaultCollectionNumber()) collections.add(mockk { every { title } returns "Collection 5" }) + state = state.copy(tabCollections = collections) assertEquals(6, controller.getDefaultCollectionNumber()) collections.add(mockk { every { title } returns "Random name" }) + state = state.copy(tabCollections = collections) assertEquals(6, controller.getDefaultCollectionNumber()) collections.add(mockk { every { title } returns "Collection 10 10" }) + state = state.copy(tabCollections = collections) assertEquals(6, controller.getDefaultCollectionNumber()) } @Test fun `WHEN adding a new collection THEN dispatch NameCollection step changed`() { - val collections: List = ArrayList() - every { state.tabCollections } returns collections - controller.addNewCollection() verify { store.dispatch(CollectionCreationAction.StepChanged(SaveCollectionStep.NameCollection, 1)) } @@ -139,9 +254,6 @@ class DefaultCollectionCreationControllerTest { @Test fun `GIVEN empty list of collections WHEN saving tabs to collection THEN dispatch NameCollection step changed`() { - val collections: List = ArrayList() - every { state.tabCollections } returns collections - controller.saveTabsToCollection(ArrayList()) verify { store.dispatch(CollectionCreationAction.StepChanged(SaveCollectionStep.NameCollection, 1)) } @@ -149,14 +261,14 @@ class DefaultCollectionCreationControllerTest { @Test fun `GIVEN list of collections WHEN saving tabs to collection THEN dispatch NameCollection step changed`() { - val collections: MutableList = ArrayList() - collections.add(mockk { - every { title } returns "Collection 1" - }) - collections.add(mockk { - every { title } returns "Random Collection" - }) - every { state.tabCollections } returns collections + state = state.copy(tabCollections = listOf( + mockk { + every { title } returns "Collection 1" + }, + mockk { + every { title } returns "Random Collection" + } + )) controller.saveTabsToCollection(ArrayList()) @@ -165,27 +277,32 @@ class DefaultCollectionCreationControllerTest { @Test fun `normalSessionSize only counts non-private non-custom sessions`() { - fun session(isPrivate: Boolean, isCustom: Boolean) = mockk().apply { - every { private } returns isPrivate - every { isCustomTabSession() } returns isCustom - } + val normal1 = mockSession() + val normal2 = mockSession() + val normal3 = mockSession() - val normal1 = session(isPrivate = false, isCustom = false) - val normal2 = session(isPrivate = false, isCustom = false) - val normal3 = session(isPrivate = false, isCustom = false) + val private1 = mockSession(isPrivate = true) + val private2 = mockSession(isPrivate = true) - val private1 = session(isPrivate = true, isCustom = false) - val private2 = session(isPrivate = true, isCustom = false) + val custom1 = mockSession(isCustom = true) + val custom2 = mockSession(isCustom = true) + val custom3 = mockSession(isCustom = true) - val custom1 = session(isPrivate = false, isCustom = true) - val custom2 = session(isPrivate = false, isCustom = true) - val custom3 = session(isPrivate = false, isCustom = true) - - val privateCustom = session(isPrivate = true, isCustom = true) + val privateCustom = mockSession(isPrivate = true, isCustom = true) every { sessionManager.sessions } returns listOf(normal1, private1, private2, custom1, normal2, normal3, custom2, custom3, privateCustom) assertEquals(3, controller.normalSessionSize(sessionManager)) } + + private fun mockSession( + sessionId: String? = null, + isPrivate: Boolean = false, + isCustom: Boolean = false + ) = mockk { + sessionId?.let { every { id } returns it } + every { private } returns isPrivate + every { isCustomTabSession() } returns isCustom + } }