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.

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<SavedLogin>) : 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()
+            )
+        }
     }
 }
 

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)
         }
     }
 

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())
+            SavedLoginsController(
-        savedLoginsInteractor =
+                    store = savedLoginsStore,
-            SavedLoginsInteractor(savedLoginsController, ::itemClicked, ::openLearnMore)
+                    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) {
+    private fun openToBrowserAndLoad(searchTermOrURL: String, newTab: Boolean, from: BrowserDirection) {
-        context?.components?.analytics?.metrics?.track(Event.OpenOneLogin)
+        (activity as HomeActivity).openToBrowserAndLoad(searchTermOrURL, newTab, from)
-        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 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)
+                    )
                 }
             }
         }

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 savedLoginsController: SavedLoginsController
-    private val itemClicked: (SavedLogin) -> Unit,
-    private val learnMore: () -> Unit
 ) {
-    fun itemClicked(item: SavedLogin) {
+    fun onItemClicked(item: SavedLogin) {
-        itemClicked.invoke(item)
+        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
+        )
+    }
 }

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())
+    }
 }

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) }
     }
 }

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
+            )
+        }
+    }
 }

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 interactor = SavedLoginsInteractor(controller)
-    private val learnMore: () -> Unit = mockk(relaxed = true)
-    private val interactor = SavedLoginsInteractor(
-        controller,
-        savedLoginClicked,
-        learnMore
-    )
 
     @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 {
+        verifyAll {
-            savedLoginClicked.invoke(item)
+            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()
+        }
+    }
 }