From e1fc0cc038a2155b7d9ba891345ed72f4e39286b Mon Sep 17 00:00:00 2001 From: Mugurell Date: Thu, 16 Jul 2020 22:40:08 +0300 Subject: [PATCH] Ensure logins deletion (#12507) * For #11227 - Cleanup saved logins list when one is selected Selecting a saved login will open a detail screen for it from where users can change details or even delete that particular login. After the change is made the user is brought back to the list of saved logins where for a brief moment (< 1s) until we get a new response from passwordsStorage.list() the user can see and even interact with the old list of items, which may still contain the just deleted one. To avoid users seeing obsolete logins or even interacting with them (selecting a previosuly deleted item will result in a crash) we will clean the list of logins just before the selected login is opened in the detailed view. When returning for a brief moment the users may see the "loading" UX until passwordsStorage.list() returns the up-to-date list of logins to display. * For #11227 - Refactor SavedLoginsView to be closer to MVI - Interactors should only get passed other Interactors or Controllers as dependencies to which they should delegate user actions. - Controllers should hold most of the business logic and get passed all final dependencies they need to do their job. --- .../settings/logins/LoginsFragmentStore.kt | 8 +++ .../settings/logins/LoginsListViewHolder.kt | 2 +- .../settings/logins/SavedLoginsFragment.kt | 37 +++++------- .../fenix/settings/logins/SavedLoginsView.kt | 60 +++++++++++++++---- .../logins/LoginsFragmentStoreTest.kt | 16 +++++ .../logins/LoginsListViewHolderTest.kt | 2 +- .../logins/SavedLoginsControllerTest.kt | 40 ++++++++++++- .../logins/SavedLoginsInteractorTest.kt | 33 +++++----- 8 files changed, 149 insertions(+), 49 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt index dcb399bfc..9e10508fd 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt @@ -54,6 +54,7 @@ sealed class LoginsAction : Action { data class UpdateLoginsList(val list: List) : LoginsAction() data class UpdateCurrentLogin(val item: SavedLogin) : LoginsAction() data class SortLogins(val sortingStrategy: SortingStrategy) : LoginsAction() + data class LoginSelected(val item: SavedLogin) : LoginsAction() } /** @@ -110,6 +111,13 @@ private fun savedLoginsStateReducer( state ) } + is LoginsAction.LoginSelected -> { + state.copy( + isLoading = true, + loginList = emptyList(), + filteredItems = emptyList() + ) + } } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsListViewHolder.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsListViewHolder.kt index 62183ae71..f7bd68faa 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsListViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsListViewHolder.kt @@ -34,7 +34,7 @@ class LoginsListViewHolder( updateFavIcon(item.origin) view.setOnClickListener { - interactor.itemClicked(item) + interactor.onItemClicked(item) } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsFragment.kt index e76800c6f..cec4c1c29 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsFragment.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsFragment.kt @@ -37,12 +37,10 @@ import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.R import org.mozilla.fenix.components.StoreProvider -import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.redirectToReAuth import org.mozilla.fenix.ext.settings import org.mozilla.fenix.ext.showToolbar -import org.mozilla.fenix.settings.SupportUtils @SuppressWarnings("TooManyFunctions") class SavedLoginsFragment : Fragment() { @@ -88,9 +86,14 @@ class SavedLoginsFragment : Fragment() { ) } val savedLoginsController: SavedLoginsController = - SavedLoginsController(savedLoginsStore, requireContext().settings()) - savedLoginsInteractor = - SavedLoginsInteractor(savedLoginsController, ::itemClicked, ::openLearnMore) + SavedLoginsController( + store = savedLoginsStore, + navController = findNavController(), + browserNavigator = ::openToBrowserAndLoad, + settings = requireContext().settings(), + metrics = requireContext().components.analytics.metrics + ) + savedLoginsInteractor = SavedLoginsInteractor(savedLoginsController) savedLoginsView = SavedLoginsView(view.savedLoginsLayout, savedLoginsInteractor) loadAndMapLogins() return view @@ -138,20 +141,8 @@ class SavedLoginsFragment : Fragment() { super.onPause() } - private fun itemClicked(item: SavedLogin) { - context?.components?.analytics?.metrics?.track(Event.OpenOneLogin) - val directions = - SavedLoginsFragmentDirections.actionSavedLoginsFragmentToLoginDetailFragment(item.guid) - findNavController().navigate(directions) - } - - private fun openLearnMore() { - (activity as HomeActivity).openToBrowserAndLoad( - searchTermOrURL = SupportUtils.getGenericSumoURLForTopic - (SupportUtils.SumoTopic.SYNC_SETUP), - newTab = true, - from = BrowserDirection.FromSavedLoginsFragment - ) + private fun openToBrowserAndLoad(searchTermOrURL: String, newTab: Boolean, from: BrowserDirection) { + (activity as HomeActivity).openToBrowserAndLoad(searchTermOrURL, newTab, from) } private fun loadAndMapLogins() { @@ -222,11 +213,15 @@ class SavedLoginsFragment : Fragment() { sortingStrategyMenu = SavedLoginsSortingStrategyMenu(requireContext(), itemToHighlight) { when (it) { SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort -> { - savedLoginsInteractor.sort(SortingStrategy.Alphabetically(requireContext().applicationContext)) + savedLoginsInteractor.onSortingStrategyChanged( + SortingStrategy.Alphabetically(requireContext().applicationContext) + ) } SavedLoginsSortingStrategyMenu.Item.LastUsedSort -> { - savedLoginsInteractor.sort(SortingStrategy.LastUsed(requireContext().applicationContext)) + savedLoginsInteractor.onSortingStrategyChanged( + SortingStrategy.LastUsed(requireContext().applicationContext) + ) } } } diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsView.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsView.kt index 3f83360f4..dd1564df3 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsView.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsView.kt @@ -9,11 +9,16 @@ import android.view.LayoutInflater import android.view.ViewGroup import android.widget.FrameLayout import androidx.core.view.isVisible +import androidx.navigation.NavController import androidx.recyclerview.widget.LinearLayoutManager import kotlinx.android.extensions.LayoutContainer import kotlinx.android.synthetic.main.component_saved_logins.view.* +import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.R +import org.mozilla.fenix.components.metrics.Event +import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.ext.addUnderline +import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Settings /** @@ -40,7 +45,7 @@ class SavedLoginsView( with(view.saved_passwords_empty_learn_more) { movementMethod = LinkMovementMethod.getInstance() addUnderline() - setOnClickListener { interactor.onLearnMore() } + setOnClickListener { interactor.onLearnMoreClicked() } } with(view.saved_passwords_empty_message) { @@ -54,6 +59,7 @@ class SavedLoginsView( } fun update(state: LoginsListState) { + // todo MVI views should not have logic. Needs refactoring. if (state.isLoading) { view.progress_bar.isVisible = true } else { @@ -67,29 +73,63 @@ class SavedLoginsView( /** * Interactor for the saved logins screen + * + * @param savedLoginsController [SavedLoginsController] which will be delegated for all users interactions. */ class SavedLoginsInteractor( - private val savedLoginsController: SavedLoginsController, - private val itemClicked: (SavedLogin) -> Unit, - private val learnMore: () -> Unit + private val savedLoginsController: SavedLoginsController ) { - fun itemClicked(item: SavedLogin) { - itemClicked.invoke(item) + fun onItemClicked(item: SavedLogin) { + savedLoginsController.handleItemClicked(item) } - fun onLearnMore() { - learnMore.invoke() + + fun onLearnMoreClicked() { + savedLoginsController.handleLearnMoreClicked() } - fun sort(sortingStrategy: SortingStrategy) { + + fun onSortingStrategyChanged(sortingStrategy: SortingStrategy) { savedLoginsController.handleSort(sortingStrategy) } } /** * Controller for the saved logins screen + * + * @param store Store used to hold in-memory collection state. + * @param navController NavController manages app navigation within a NavHost. + * @param browserNavigator Controller allowing browser navigation to any Uri. + * @param settings SharedPreferences wrapper for easier usage. + * @param metrics Controller that handles telemetry events. */ -class SavedLoginsController(val store: LoginsFragmentStore, val settings: Settings) { +class SavedLoginsController( + private val store: LoginsFragmentStore, + private val navController: NavController, + private val browserNavigator: ( + searchTermOrURL: String, + newTab: Boolean, + from: BrowserDirection + ) -> Unit, + private val settings: Settings, + private val metrics: MetricController +) { fun handleSort(sortingStrategy: SortingStrategy) { store.dispatch(LoginsAction.SortLogins(sortingStrategy)) settings.savedLoginsSortingStrategy = sortingStrategy } + + fun handleItemClicked(item: SavedLogin) { + store.dispatch(LoginsAction.LoginSelected(item)) + metrics.track(Event.OpenOneLogin) + navController.navigate( + SavedLoginsFragmentDirections.actionSavedLoginsFragmentToLoginDetailFragment(item.guid) + ) + } + + fun handleLearnMoreClicked() { + browserNavigator.invoke( + SupportUtils.getGenericSumoURLForTopic(SupportUtils.SumoTopic.SYNC_SETUP), + true, + BrowserDirection.FromSavedLoginsFragment + ) + } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt index 236023dfd..56fc68d6c 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt @@ -10,6 +10,7 @@ import mozilla.components.support.test.ext.joinBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test class LoginsFragmentStoreTest { @@ -123,4 +124,19 @@ class LoginsFragmentStoreTest { assertEquals("example", store.state.searchedForText) assertEquals(listOf(exampleLogin), store.state.filteredItems) } + + @Test + fun `LoginSelected action`() { + val store = LoginsFragmentStore(baseState.copy( + isLoading = false, + loginList = listOf(mockk()), + filteredItems = listOf(mockk()) + )) + + store.dispatch(LoginsAction.LoginSelected(mockk())).joinBlocking() + + assertTrue(store.state.isLoading) + assertTrue(store.state.loginList.isEmpty()) + assertTrue(store.state.filteredItems.isEmpty()) + } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListViewHolderTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListViewHolderTest.kt index 7509bb10a..3f15ad9fe 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListViewHolderTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListViewHolderTest.kt @@ -52,6 +52,6 @@ class LoginsListViewHolderTest { holder.bind(baseLogin) view.performClick() - verify { interactor.itemClicked(baseLogin) } + verify { interactor.onItemClicked(baseLogin) } } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsControllerTest.kt index 62f27e507..ea0e0f418 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsControllerTest.kt @@ -4,20 +4,30 @@ package org.mozilla.fenix.settings.logins +import androidx.navigation.NavController import io.mockk.mockk import io.mockk.verify +import io.mockk.verifyAll import mozilla.components.support.test.robolectric.testContext import org.junit.Test import org.junit.runner.RunWith +import org.mozilla.fenix.BrowserDirection +import org.mozilla.fenix.components.metrics.Event +import org.mozilla.fenix.components.metrics.MetricController import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Settings @RunWith(FenixRobolectricTestRunner::class) class SavedLoginsControllerTest { private val store: LoginsFragmentStore = mockk(relaxed = true) + private val navController: NavController = mockk(relaxed = true) + private val browserNavigator: (String, Boolean, BrowserDirection) -> Unit = mockk(relaxed = true) + private val settings: Settings = mockk(relaxed = true) + private val metrics: MetricController = mockk(relaxed = true) private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically(testContext) - private val controller = SavedLoginsController(store, settings) + private val controller = SavedLoginsController(store, navController, browserNavigator, settings, metrics) @Test fun `GIVEN a sorting strategy, WHEN handleSort is called on the controller, THEN the correct action should be dispatched and the strategy saved in sharedPref`() { @@ -34,4 +44,32 @@ class SavedLoginsControllerTest { settings.savedLoginsSortingStrategy = sortingStrategy } } + + @Test + fun `GIVEN a SavedLogin, WHEN handleItemClicked is called for it, THEN LoginsAction$LoginSelected should be emitted`() { + val login: SavedLogin = mockk(relaxed = true) + + controller.handleItemClicked(login) + + verifyAll { + store.dispatch(LoginsAction.LoginSelected(login)) + metrics.track(Event.OpenOneLogin) + navController.navigate( + SavedLoginsFragmentDirections.actionSavedLoginsFragmentToLoginDetailFragment(login.guid) + ) + } + } + + @Test + fun `GIVEN the learn more option, WHEN handleLearnMoreClicked is called for it, then we should open the right support webpage`() { + controller.handleLearnMoreClicked() + + verify { + browserNavigator.invoke( + SupportUtils.getGenericSumoURLForTopic(SupportUtils.SumoTopic.SYNC_SETUP), + true, + BrowserDirection.FromSavedLoginsFragment + ) + } + } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt index b9db6f70b..7e56dd379 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt @@ -5,7 +5,7 @@ package org.mozilla.fenix.settings.logins import io.mockk.mockk -import io.mockk.verify +import io.mockk.verifyAll import mozilla.components.support.test.robolectric.testContext import org.junit.Test import org.junit.runner.RunWith @@ -15,32 +15,35 @@ import kotlin.random.Random @RunWith(FenixRobolectricTestRunner::class) class SavedLoginsInteractorTest { private val controller: SavedLoginsController = mockk(relaxed = true) - private val savedLoginClicked: (SavedLogin) -> Unit = mockk(relaxed = true) - private val learnMore: () -> Unit = mockk(relaxed = true) - private val interactor = SavedLoginsInteractor( - controller, - savedLoginClicked, - learnMore - ) + private val interactor = SavedLoginsInteractor(controller) @Test - fun itemClicked() { + fun `GIVEN a SavedLogin being clicked, WHEN the interactor is called for it, THEN it should just delegate the controller`() { val item = SavedLogin("mozilla.org", "username", "password", "id", Random.nextLong()) - interactor.itemClicked(item) + interactor.onItemClicked(item) - verify { - savedLoginClicked.invoke(item) + verifyAll { + controller.handleItemClicked(item) } } @Test - fun `GIVEN a sorting strategy, WHEN sort method is called on the interactor, THEN controller should call handleSort with the same parameter`() { + fun `GIVEN a change in sorting strategy, WHEN the interactor is called for it, THEN it should just delegate the controller`() { val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically(testContext) - interactor.sort(sortingStrategy) + interactor.onSortingStrategyChanged(sortingStrategy) - verify { + verifyAll { controller.handleSort(sortingStrategy) } } + + @Test + fun `GIVEN the learn more option is clicked, WHEN the interactor is called for it, THEN it should just delegate the controller`() { + interactor.onLearnMoreClicked() + + verifyAll { + controller.handleLearnMoreClicked() + } + } }