From 3c22100b8495434ea8c30f9cef2bb8ce554f180b Mon Sep 17 00:00:00 2001 From: Tiger Oakes Date: Wed, 19 Aug 2020 10:47:40 -0700 Subject: [PATCH] For #12565: Pass bookmark storage to controller --- .../library/bookmarks/BookmarkController.kt | 16 +++-- .../library/bookmarks/BookmarkFragment.kt | 2 + .../bookmarks/BookmarkControllerTest.kt | 72 +++++++++---------- .../fenix/share/ShareControllerTest.kt | 10 +-- 4 files changed, 52 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt index 842d1878c..cdf019aab 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt @@ -14,14 +14,14 @@ import kotlinx.coroutines.launch import mozilla.appservices.places.BookmarkRoot import mozilla.components.concept.engine.prompt.ShareData import mozilla.components.concept.storage.BookmarkNode +import mozilla.components.concept.storage.BookmarksStorage +import mozilla.components.service.fxa.manager.FxaAccountManager import mozilla.components.service.fxa.sync.SyncReason import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.ext.bookmarkStorage -import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.nav /** @@ -52,6 +52,8 @@ interface BookmarkController { @Suppress("TooManyFunctions") class DefaultBookmarkController( private val activity: HomeActivity, + private val bookmarkStorage: BookmarksStorage, + private val accountManager: FxaAccountManager, private val navController: NavController, private val clipboardManager: ClipboardManager?, private val scope: CoroutineScope, @@ -143,14 +145,14 @@ class DefaultBookmarkController( scope.launch { store.dispatch(BookmarkFragmentAction.StartSync) invokePendingDeletion() - activity.components.backgroundServices.accountManager.syncNow(SyncReason.User) + accountManager.syncNow(SyncReason.User) // The current bookmark node we are viewing may be made invalid after syncing so we // check if the current node is valid and if it isn't we find the nearest valid ancestor // and open it val validAncestorGuid = store.state.guidBackstack.findLast { guid -> - activity.bookmarkStorage.getBookmark(guid) != null + bookmarkStorage.getBookmark(guid) != null } ?: BookmarkRoot.Mobile.id - val node = activity.bookmarkStorage.getBookmark(validAncestorGuid)!! + val node = bookmarkStorage.getBookmark(validAncestorGuid)!! handleBookmarkExpand(node) store.dispatch(BookmarkFragmentAction.FinishSync) } @@ -160,12 +162,12 @@ class DefaultBookmarkController( invokePendingDeletion.invoke() scope.launch { val parentGuid = store.state.guidBackstack.findLast { guid -> - store.state.tree?.guid != guid && activity.bookmarkStorage.getBookmark(guid) != null + store.state.tree?.guid != guid && bookmarkStorage.getBookmark(guid) != null } if (parentGuid == null) { navController.popBackStack() } else { - val parent = activity.bookmarkStorage.getBookmark(parentGuid)!! + val parent = bookmarkStorage.getBookmark(parentGuid)!! handleBookmarkExpand(parent) } } diff --git a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt index 18c19943e..054635f70 100644 --- a/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt @@ -88,6 +88,8 @@ class BookmarkFragment : LibraryPageFragment(), UserInteractionHan _bookmarkInteractor = BookmarkFragmentInteractor( bookmarksController = DefaultBookmarkController( activity = requireActivity() as HomeActivity, + bookmarkStorage = requireComponents.core.bookmarksStorage, + accountManager = requireComponents.backgroundServices.accountManager, navController = findNavController(), clipboardManager = requireContext().getSystemService(), scope = viewLifecycleOwner.lifecycleScope, diff --git a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt index 2d0f2deb4..dcc41ec74 100644 --- a/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkControllerTest.kt @@ -6,20 +6,17 @@ package org.mozilla.fenix.library.bookmarks import android.content.ClipData import android.content.ClipboardManager -import android.content.Context import androidx.navigation.NavController -import androidx.navigation.NavDestination import androidx.navigation.NavDirections -import io.mockk.Runs +import io.mockk.MockKAnnotations import io.mockk.called import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every +import io.mockk.impl.annotations.MockK import io.mockk.just import io.mockk.mockk import io.mockk.runs -import io.mockk.slot -import io.mockk.spyk import io.mockk.verify import io.mockk.verifyOrder import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -27,6 +24,8 @@ import kotlinx.coroutines.test.TestCoroutineScope import mozilla.appservices.places.BookmarkRoot import mozilla.components.concept.storage.BookmarkNode import mozilla.components.concept.storage.BookmarkNodeType +import mozilla.components.concept.storage.BookmarksStorage +import mozilla.components.service.fxa.manager.FxaAccountManager import org.junit.After import org.junit.Assert.assertEquals import org.junit.Before @@ -35,31 +34,27 @@ import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode -import org.mozilla.fenix.components.Services import org.mozilla.fenix.components.metrics.Event -import org.mozilla.fenix.ext.bookmarkStorage -import org.mozilla.fenix.ext.components -@Suppress("TooManyFunctions", "LargeClass") @ExperimentalCoroutinesApi class BookmarkControllerTest { - private lateinit var controller: BookmarkController - - private val bookmarkStore = spyk(BookmarkFragmentStore(BookmarkFragmentState(null))) - private val context: Context = mockk(relaxed = true) private val scope = TestCoroutineScope() - private val clipboardManager: ClipboardManager = mockk(relaxUnitFun = true) - private val navController: NavController = mockk(relaxed = true) - private val sharedViewModel: BookmarksSharedViewModel = mockk() - private val loadBookmarkNode: suspend (String) -> BookmarkNode? = mockk(relaxed = true) - private val showSnackbar: (String) -> Unit = mockk(relaxed = true) - private val deleteBookmarkNodes: (Set, Event) -> Unit = mockk(relaxed = true) - private val deleteBookmarkFolder: (Set) -> Unit = mockk(relaxed = true) - private val invokePendingDeletion: () -> Unit = mockk(relaxed = true) - - private val homeActivity: HomeActivity = mockk(relaxed = true) - private val services: Services = mockk(relaxed = true) + + @MockK private lateinit var bookmarkStore: BookmarkFragmentStore + @MockK private lateinit var sharedViewModel: BookmarksSharedViewModel + @MockK(relaxUnitFun = true) private lateinit var clipboardManager: ClipboardManager + @MockK(relaxed = true) private lateinit var homeActivity: HomeActivity + @MockK(relaxed = true) private lateinit var bookmarkStorage: BookmarksStorage + @MockK(relaxed = true) private lateinit var accountManager: FxaAccountManager + @MockK(relaxed = true) private lateinit var navController: NavController + @MockK(relaxed = true) private lateinit var loadBookmarkNode: suspend (String) -> BookmarkNode? + @MockK(relaxed = true) private lateinit var showSnackbar: (String) -> Unit + @MockK(relaxed = true) private lateinit var deleteBookmarkNodes: (Set, Event) -> Unit + @MockK(relaxed = true) private lateinit var deleteBookmarkFolder: (Set) -> Unit + @MockK(relaxed = true) private lateinit var invokePendingDeletion: () -> Unit + + private lateinit var controller: BookmarkController private val item = BookmarkNode(BookmarkNodeType.ITEM, "456", "123", 0, "Mozilla", "http://mozilla.org", null) @@ -89,15 +84,20 @@ class BookmarkControllerTest { @Before fun setup() { - every { homeActivity.components.services } returns services - every { navController.currentDestination } returns NavDestination("").apply { - id = R.id.bookmarkFragment + MockKAnnotations.init(this) + loadBookmarkNode = mockk(relaxed = true) + + every { navController.currentDestination } returns mockk { + every { id } returns R.id.bookmarkFragment } + every { bookmarkStore.state } returns BookmarkFragmentState(null) every { bookmarkStore.dispatch(any()) } returns mockk() every { sharedViewModel.selectedFolder = any() } just runs controller = DefaultBookmarkController( activity = homeActivity, + bookmarkStorage = bookmarkStorage, + accountManager = accountManager, navController = navController, clipboardManager = clipboardManager, scope = scope, @@ -211,12 +211,12 @@ class BookmarkControllerTest { @Test fun `handleBookmarkSelected should show a toast when selecting a root folder`() { - val errorMessage = context.getString(R.string.bookmark_cannot_edit_root) + every { homeActivity.resources.getString(R.string.bookmark_cannot_edit_root) } returns "Can't edit default folders" controller.handleBookmarkSelected(root) verify { - showSnackbar(errorMessage) + showSnackbar("Can't edit default folders") } } @@ -240,25 +240,24 @@ class BookmarkControllerTest { @Test fun `handleCopyUrl should copy bookmark url to clipboard and show a toast`() { - val urlCopiedMessage = context.getString(R.string.url_copied) + every { homeActivity.resources.getString(R.string.url_copied) } returns "URL copied" controller.handleCopyUrl(item) verifyOrder { ClipData.newPlainText(item.url, item.url) - showSnackbar(urlCopiedMessage) + showSnackbar("URL copied") } } @Test fun `handleBookmarkSharing should navigate to the 'Share' fragment`() { - val navDirectionsSlot = slot() - every { navController.navigate(capture(navDirectionsSlot), null) } just Runs - controller.handleBookmarkSharing(item) verify { - navController.navigate(navDirectionsSlot.captured, null) + navController.navigate(withArg { + assertEquals(R.id.action_global_shareFragment, it.actionId) + }, null) } } @@ -313,8 +312,7 @@ class BookmarkControllerTest { @Test fun `handleRequestSync dispatches actions in the correct order`() { - every { homeActivity.components.backgroundServices.accountManager } returns mockk(relaxed = true) - coEvery { homeActivity.bookmarkStorage.getBookmark(any()) } returns tree + coEvery { bookmarkStorage.getBookmark(any()) } returns tree coEvery { loadBookmarkNode.invoke(any()) } returns tree controller.handleRequestSync() diff --git a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt index 9950dd5b6..a66336b71 100644 --- a/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/share/ShareControllerTest.kt @@ -83,11 +83,13 @@ class ShareControllerTest { @Test fun `handleShareToApp should start a new sharing activity and close this`() = runBlocking { + val appPackageName = "package" + val appClassName = "activity" val appShareOption = AppShareOption( name = "app", icon = mockk(), - packageName = "package", - activityName = "activity" + packageName = appPackageName, + activityName = appClassName ) val shareIntent = slot() // Our share Intent uses `FLAG_ACTIVITY_NEW_TASK` but when resolving the startActivity call @@ -108,8 +110,8 @@ class ShareControllerTest { assertEquals(textToShare, shareIntent.captured.extras!![Intent.EXTRA_TEXT]) assertEquals("text/plain", shareIntent.captured.type) assertEquals(Intent.FLAG_ACTIVITY_NEW_TASK, shareIntent.captured.flags) - assertEquals("package", shareIntent.captured.component!!.packageName) - assertEquals("activity", shareIntent.captured.component!!.className) + assertEquals(appPackageName, shareIntent.captured.component!!.packageName) + assertEquals(appClassName, shareIntent.captured.component!!.className) verify { recentAppStorage.updateRecentApp(appShareOption.activityName) } verifyOrder {