From cccf3a29045cf9698a824966fbe40011addaa6e6 Mon Sep 17 00:00:00 2001 From: Phil Cappelli <150719757+phil-livefront@users.noreply.github.com> Date: Wed, 26 Feb 2025 15:37:48 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=8D=92=20PM-18636=20Hide=20coach=20mark?= =?UTF-8?q?=20card=20if=20any=20login=20ciphers=20exist=20(#4797)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../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 3c7fc0b1f0..e3f919dfe0 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 98a5fdb0cd..866293e945 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`() {