From 2893c3871f8da88d29513990244f6ab713ac420f Mon Sep 17 00:00:00 2001 From: Dave Severns <149429124+dseverns-livefront@users.noreply.github.com> Date: Wed, 26 Feb 2025 13:48:10 -0500 Subject: [PATCH] PM-18636 Hide coach mark card if any login ciphers exist (#4787) Co-authored-by: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Co-authored-by: Philip Cappelli --- .../manager/FirstTimeActionManagerImpl.kt | 21 ++++++ .../manager/FirstTimeActionManagerTest.kt | 71 +++++++++++++++++-- 2 files changed, 87 insertions(+), 5 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 3c7fc0b1f0f..e3f919dfe07 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,7 @@ class FirstTimeActionManagerImpl @Inject constructor( get() = settingsDiskSource .getShouldShowAddLoginCoachMarkFlow() .map { it ?: true } + .mapFalseIfAnyLoginCiphersAvailable() .combine( featureFlagManager.getFeatureFlagFlow(FlagKey.OnboardingFlow), ) { shouldShow, featureIsEnabled -> @@ -172,6 +173,7 @@ class FirstTimeActionManagerImpl @Inject constructor( get() = settingsDiskSource .getShouldShowGeneratorCoachMarkFlow() .map { it ?: true } + .mapFalseIfAnyLoginCiphersAvailable() .combine( featureFlagManager.getFeatureFlagFlow(FlagKey.OnboardingFlow), ) { shouldShow, featureFlagEnabled -> @@ -294,4 +296,23 @@ class FirstTimeActionManagerImpl @Inject constructor( return settingsDiskSource.getShowAutoFillSettingBadge(userId) ?: false && !autofillEnabledManager.isAutofillEnabled } + + /** + * If there are any existing "Login" type ciphers then we'll map the current value + * of the receiver Flow to `false`. + */ + @OptIn(ExperimentalCoroutinesApi::class) + private fun Flow.mapFalseIfAnyLoginCiphersAvailable(): Flow = + authDiskSource + .activeUserIdChangesFlow + .filterNotNull() + .flatMapLatest { activeUserId -> + combine( + flow = this, + flow2 = vaultDiskSource.getCiphers(activeUserId), + ) { currentValue, ciphers -> + currentValue && ciphers.none { it.login != null } + } + } + .distinctUntilChanged() } 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 98a5fdb0cd0..866293e9458 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 @@ -303,10 +303,10 @@ class FirstTimeActionManagerTest { @Test fun `shouldShowAddLoginCoachMarkFlow updates when disk source updates`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE // Enable the feature for this test. mutableOnboardingFeatureFlow.update { true } firstTimeActionManager.shouldShowAddLoginCoachMarkFlow.test { - // Null will be mapped to false. assertTrue(awaitItem()) fakeSettingsDiskSource.storeShouldShowAddLoginCoachMark(shouldShow = false) assertFalse(awaitItem()) @@ -316,15 +316,45 @@ class FirstTimeActionManagerTest { @Test fun `shouldShowAddLoginCoachMarkFlow updates when feature flag for onboarding updates`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE firstTimeActionManager.shouldShowAddLoginCoachMarkFlow.test { - // Null will be mapped to false but feature being "off" will override to true. assertFalse(awaitItem()) mutableOnboardingFeatureFlow.update { true } - // Will use the value from disk source (null ?: false). assertTrue(awaitItem()) } } + @Suppress("MaxLineLength") + @Test + fun `if there are any login ciphers available for the active user should not show add login coach marks`() = + runTest { + val mockJsonWithNoLogin = mockk { + every { login } returns null + } + val mockJsonWithLogin = mockk { + every { login } returns mockk() + } + fakeAuthDiskSource.userState = MOCK_USER_STATE + // Enable feature flag so flow emits updates from disk. + mutableOnboardingFeatureFlow.update { true } + mutableCiphersListFlow.update { + listOf( + mockJsonWithNoLogin, + mockJsonWithNoLogin, + ) + } + firstTimeActionManager.shouldShowAddLoginCoachMarkFlow.test { + assertTrue(awaitItem()) + mutableCiphersListFlow.update { + listOf( + mockJsonWithLogin, + mockJsonWithNoLogin, + ) + } + assertFalse(awaitItem()) + } + } + @Suppress("MaxLineLength") @Test fun `markCoachMarkTourCompleted for the ADD_LOGIN type sets the value to true in the disk source for should show add logins coach mark`() { @@ -335,10 +365,10 @@ class FirstTimeActionManagerTest { @Test fun `shouldShowGeneratorCoachMarkFlow updates when disk source updates`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE // Enable feature flag so flow emits updates from disk. mutableOnboardingFeatureFlow.update { true } firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { - // Null will be mapped to false. assertTrue(awaitItem()) fakeSettingsDiskSource.storeShouldShowGeneratorCoachMark(shouldShow = false) assertFalse(awaitItem()) @@ -348,8 +378,8 @@ class FirstTimeActionManagerTest { @Test fun `shouldShowGeneratorCoachMarkFlow updates when onboarding feature value changes`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { - // Null will be mapped to false assertFalse(awaitItem()) mutableOnboardingFeatureFlow.update { true } // Take the value from disk. @@ -357,6 +387,37 @@ class FirstTimeActionManagerTest { } } + @Suppress("MaxLineLength") + @Test + fun `if there are any login ciphers available for the active user should not show generator coach marks`() = + runTest { + val mockJsonWithNoLogin = mockk { + every { login } returns null + } + val mockJsonWithLogin = mockk { + every { login } returns mockk() + } + fakeAuthDiskSource.userState = MOCK_USER_STATE + // Enable feature flag so flow emits updates from disk. + mutableOnboardingFeatureFlow.update { true } + mutableCiphersListFlow.update { + listOf( + mockJsonWithNoLogin, + mockJsonWithNoLogin, + ) + } + firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { + assertTrue(awaitItem()) + mutableCiphersListFlow.update { + listOf( + mockJsonWithLogin, + mockJsonWithNoLogin, + ) + } + assertFalse(awaitItem()) + } + } + @Suppress("MaxLineLength") @Test fun `markCoachMarkTourCompleted for the GENERATOR type sets the value to true in the disk source for should show generator coach mark`() {