From 45062df6f248a1f4b64d0e90012bb9257b830e7c Mon Sep 17 00:00:00 2001
From: Dave Severns <dseverns@livefront.com>
Date: Wed, 22 Jan 2025 09:35:49 -0500
Subject: [PATCH] move feature check to manager

---
 .../manager/FirstTimeActionManagerImpl.kt     |  7 ++++
 .../addedit/VaultAddEditItemContent.kt        |  4 ++-
 .../feature/addedit/VaultAddEditViewModel.kt  | 16 +++------
 .../manager/FirstTimeActionManagerTest.kt     | 18 +++++++++-
 .../addedit/VaultAddEditViewModelTest.kt      | 33 +------------------
 5 files changed, 32 insertions(+), 46 deletions(-)

diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerImpl.kt
index afd729a79..7d5ebf8f8 100644
--- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerImpl.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerImpl.kt
@@ -159,6 +159,13 @@ class FirstTimeActionManagerImpl @Inject constructor(
             .getHasSeenAddLoginCoachMarkFlow()
             // default value of false.
             .map { it ?: false }
+            .combine(
+                featureFlagManager.getFeatureFlagFlow(FlagKey.OnboardingFlow),
+            ) { hasSeenTour, featureIsEnabled ->
+                // If the feature flag is off always return true so observers know
+                // the card has not been shown.
+                hasSeenTour || !featureIsEnabled
+            }
             .distinctUntilChanged()
 
     /**
diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditItemContent.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditItemContent.kt
index 3f378c436..e0cf75c42 100644
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditItemContent.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditItemContent.kt
@@ -92,7 +92,9 @@ fun CoachMarkScope<AddEditItemCoachMark>.VaultAddEditContent(
                     actionText = stringResource(R.string.get_started),
                     onActionClick = loginItemTypeHandlers.onStartLoginCoachMarkTour,
                     onDismissClick = loginItemTypeHandlers.onDismissLearnAboutLoginsCard,
-                    modifier = Modifier.standardHorizontalMargin(),
+                    modifier = Modifier
+                        .fillMaxWidth()
+                        .standardHorizontalMargin(),
                 )
             }
         }
diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
index 1ada08969..5b517262d 100644
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
@@ -15,13 +15,11 @@ import com.x8bit.bitwarden.data.autofill.fido2.model.Fido2CreateCredentialReques
 import com.x8bit.bitwarden.data.autofill.fido2.model.Fido2RegisterCredentialResult
 import com.x8bit.bitwarden.data.autofill.fido2.model.UserVerificationRequirement
 import com.x8bit.bitwarden.data.autofill.util.isActiveWithFido2Credentials
-import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
 import com.x8bit.bitwarden.data.platform.manager.FirstTimeActionManager
 import com.x8bit.bitwarden.data.platform.manager.PolicyManager
 import com.x8bit.bitwarden.data.platform.manager.SpecialCircumstanceManager
 import com.x8bit.bitwarden.data.platform.manager.clipboard.BitwardenClipboardManager
 import com.x8bit.bitwarden.data.platform.manager.event.OrganizationEventManager
-import com.x8bit.bitwarden.data.platform.manager.model.FlagKey
 import com.x8bit.bitwarden.data.platform.manager.model.OrganizationEvent
 import com.x8bit.bitwarden.data.platform.manager.network.NetworkConnectionManager
 import com.x8bit.bitwarden.data.platform.manager.util.toAutofillSaveItemOrNull
@@ -66,7 +64,6 @@ import com.x8bit.bitwarden.ui.vault.model.VaultCollection
 import com.x8bit.bitwarden.ui.vault.model.VaultIdentityTitle
 import com.x8bit.bitwarden.ui.vault.model.VaultLinkedFieldType
 import dagger.hilt.android.lifecycle.HiltViewModel
-import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.map
@@ -108,7 +105,6 @@ class VaultAddEditViewModel @Inject constructor(
     private val organizationEventManager: OrganizationEventManager,
     private val networkConnectionManager: NetworkConnectionManager,
     private val firstTimeActionManager: FirstTimeActionManager,
-    private val featureFlagManager: FeatureFlagManager,
 ) : BaseViewModel<VaultAddEditState, VaultAddEditEvent, VaultAddEditAction>(
     // We load the state from the savedStateHandle for testing purposes.
     initialState = savedStateHandle[KEY_STATE]
@@ -221,13 +217,9 @@ class VaultAddEditViewModel @Inject constructor(
 
         firstTimeActionManager
             .hasSeenAddLoginCoachMarkFlow
-            .combine(
-                featureFlagManager.getFeatureFlagFlow(key = FlagKey.OnboardingFlow),
-            ) { hasSeenTour, onboardFeatureFlag ->
-                // The result of this will hide the card if the tour has been shown or if the
-                // onboarding flow feature is off.
+            .map { hasSeenTour ->
                 VaultAddEditAction.Internal.HasSeenAddLoginCoachMarkValueChangeReceive(
-                    newValue = hasSeenTour || !onboardFeatureFlag,
+                    hasSeenCoachMark = hasSeenTour,
                 )
             }
             .onEach(::sendAction)
@@ -1495,7 +1487,7 @@ class VaultAddEditViewModel @Inject constructor(
     ) {
         mutableStateFlow.update {
             it.copy(
-                hasSeenLearnAboutLoginsCard = action.newValue,
+                hasSeenLearnAboutLoginsCard = action.hasSeenCoachMark,
             )
         }
     }
@@ -3211,7 +3203,7 @@ sealed class VaultAddEditAction {
          * The value for the hasSeenAddLoginCoachMark has changed.
          */
         data class HasSeenAddLoginCoachMarkValueChangeReceive(
-            val newValue: Boolean,
+            val hasSeenCoachMark: Boolean,
         ) : Internal()
     }
 }
diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerTest.kt
index ca30c70b8..44bf8b665 100644
--- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/FirstTimeActionManagerTest.kt
@@ -36,8 +36,10 @@ class FirstTimeActionManagerTest {
     }
 
     private val mutableImportLoginsFlow = MutableStateFlow(false)
+    private val mutableOnboardingFeatureFlow = MutableStateFlow(false)
     private val featureFlagManager = mockk<FeatureFlagManager> {
         every { getFeatureFlagFlow(FlagKey.ImportLoginsFlow) } returns mutableImportLoginsFlow
+        every { getFeatureFlagFlow(FlagKey.OnboardingFlow) } returns mutableOnboardingFeatureFlow
     }
 
     private val mutableAutofillEnabledFlow = MutableStateFlow(false)
@@ -300,14 +302,28 @@ class FirstTimeActionManagerTest {
 
     @Test
     fun `hasSeenAddLoginCoachMarkFlow updates when disk source updates`() = runTest {
+        // Enable the feature for this test.
+        mutableOnboardingFeatureFlow.update { true }
         firstTimeActionManager.hasSeenAddLoginCoachMarkFlow.test {
-            // null will be mapped to false
+            // Null will be mapped to false.
             assertFalse(awaitItem())
             fakeSettingsDiskSource.storeHasSeenAddLoginCoachMark(hasSeen = true)
             assertTrue(awaitItem())
         }
     }
 
+    @Test
+    fun `hasSeenAddLoginCoachMarkFlow updates when feature flag for onboarding updates`() =
+        runTest {
+            firstTimeActionManager.hasSeenAddLoginCoachMarkFlow.test {
+                // Null will be mapped to false but feature being "off" will override to true.
+                assertTrue(awaitItem())
+                mutableOnboardingFeatureFlow.update { true }
+                // Will use the value from disk source (null ?: false).
+                assertFalse(awaitItem())
+            }
+        }
+
     @Test
     fun `hasSeenAddLoginCoachMarkTour sets the value to true in the disk source`() {
         assertNull(fakeSettingsDiskSource.getHasSeenAddLoginCoachMark())
diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt
index efb441b6f..3cd043f05 100644
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt
@@ -25,7 +25,6 @@ import com.x8bit.bitwarden.data.autofill.fido2.model.createMockFido2CreateCreden
 import com.x8bit.bitwarden.data.autofill.model.AutofillSaveItem
 import com.x8bit.bitwarden.data.autofill.model.AutofillSelectionData
 import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager
-import com.x8bit.bitwarden.data.platform.manager.FeatureFlagManager
 import com.x8bit.bitwarden.data.platform.manager.FirstTimeActionManager
 import com.x8bit.bitwarden.data.platform.manager.PolicyManager
 import com.x8bit.bitwarden.data.platform.manager.SpecialCircumstanceManager
@@ -33,7 +32,6 @@ import com.x8bit.bitwarden.data.platform.manager.SpecialCircumstanceManagerImpl
 import com.x8bit.bitwarden.data.platform.manager.clipboard.BitwardenClipboardManager
 import com.x8bit.bitwarden.data.platform.manager.event.OrganizationEventManager
 import com.x8bit.bitwarden.data.platform.manager.model.FirstTimeState
-import com.x8bit.bitwarden.data.platform.manager.model.FlagKey
 import com.x8bit.bitwarden.data.platform.manager.model.OrganizationEvent
 import com.x8bit.bitwarden.data.platform.manager.model.SpecialCircumstance
 import com.x8bit.bitwarden.data.platform.manager.network.NetworkConnectionManager
@@ -175,14 +173,6 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
         every { hasSeenAddLoginCoachMarkFlow } returns mutableHasSeenAddLoginCoachMarkFlow
     }
 
-    // Feature flag default to be enabled.
-    private val mutableOnboardingFeatureFlagFlow = MutableStateFlow(true)
-    private val featureFlagManager = mockk<FeatureFlagManager> {
-        every {
-            getFeatureFlagFlow(FlagKey.OnboardingFlow)
-        } returns mutableOnboardingFeatureFlagFlow
-    }
-
     @BeforeEach
     fun setup() {
         mockkStatic(CipherView::toViewState)
@@ -2623,25 +2613,6 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
         }
     }
 
-    @Test
-    fun `when OnboardFlow feature flag is off, shouldShowLearnAboutNewLogins should be false`() {
-        mutableOnboardingFeatureFlagFlow.update { true }
-        val viewModel = createAddVaultItemViewModel(
-            savedStateHandle = createSavedStateHandleWithState(
-                state = createVaultAddItemState(
-                    typeContentViewState = createLoginTypeContentViewState(),
-                ),
-                vaultAddEditType = VaultAddEditType.AddItem(
-                    vaultItemCipherType = VaultItemCipherType.LOGIN,
-                ),
-            ),
-        )
-        assertTrue(viewModel.stateFlow.value.shouldShowLearnAboutNewLogins)
-
-        mutableOnboardingFeatureFlagFlow.update { false }
-        assertFalse(viewModel.stateFlow.value.shouldShowLearnAboutNewLogins)
-    }
-
     @Suppress("MaxLineLength")
     @Test
     fun `when first time action manager has seen logins tour value updates to true shouldShowLearnAboutNewLogins should update to false`() {
@@ -2710,7 +2681,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
 
     @Suppress("MaxLineLength")
     @Test
-    fun `LearnAboutLoginsDismissed action calls first time action manager hasSeenAddLoginCoachMarkTour called and show coach mark event sent`() =
+    fun `StartLearnAboutLogins action calls first time action manager hasSeenAddLoginCoachMarkTour called and show coach mark event sent`() =
         runTest {
             val viewModel = createAddVaultItemViewModel()
             viewModel.eventFlow.test {
@@ -3246,7 +3217,6 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
                 organizationEventManager = organizationEventManager,
                 networkConnectionManager = networkConnectionManager,
                 firstTimeActionManager = firstTimeActionManager,
-                featureFlagManager = featureFlagManager,
             )
         }
 
@@ -4508,7 +4478,6 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
             organizationEventManager = organizationEventManager,
             networkConnectionManager = networkConnectionManager,
             firstTimeActionManager = firstTimeActionManager,
-            featureFlagManager = featureFlagManager,
         )
 
     private fun createVaultData(