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 43b195e8f9..4df8d6b705 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 @@ -9,6 +9,7 @@ import com.x8bit.bitwarden.data.platform.manager.model.CoachMarkTourType import com.x8bit.bitwarden.data.platform.manager.model.FirstTimeState import com.x8bit.bitwarden.data.platform.manager.model.FlagKey import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource +import com.x8bit.bitwarden.data.vault.datasource.network.model.PolicyTypeJson import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow @@ -158,7 +159,7 @@ class FirstTimeActionManagerImpl @Inject constructor( override val shouldShowAddLoginCoachMarkFlow: Flow get() = settingsDiskSource .getShouldShowAddLoginCoachMarkFlow() - .map { it ?: true } + .map { it != false } .mapFalseIfAnyLoginCiphersAvailable() .combine( featureFlagManager.getFeatureFlagFlow(FlagKey.OnboardingFlow), @@ -172,11 +173,13 @@ class FirstTimeActionManagerImpl @Inject constructor( override val shouldShowGeneratorCoachMarkFlow: Flow get() = settingsDiskSource .getShouldShowGeneratorCoachMarkFlow() - .map { it ?: true } + .map { it != false } .mapFalseIfAnyLoginCiphersAvailable() .combine( featureFlagManager.getFeatureFlagFlow(FlagKey.OnboardingFlow), ) { shouldShow, featureFlagEnabled -> + // If the feature flag is off always return true so observers know + // the card has not been shown. shouldShow && featureFlagEnabled } .distinctUntilChanged() @@ -298,8 +301,8 @@ class FirstTimeActionManagerImpl @Inject constructor( } /** - * If there are any existing "Login" type ciphers then we'll map the current value - * of the receiver Flow to `false`. + * Maps the receiver Flow to `false` if a personal (non-org) Login cipher exists, + * unless an `ONLY_ORG` policy is active, in which case it remains `true`. */ @OptIn(ExperimentalCoroutinesApi::class) private fun Flow.mapFalseIfAnyLoginCiphersAvailable(): Flow = @@ -310,10 +313,14 @@ class FirstTimeActionManagerImpl @Inject constructor( combine( flow = this, flow2 = vaultDiskSource.getCiphers(activeUserId), - ) { currentValue, ciphers -> - currentValue && ciphers.none { + flow3 = authDiskSource.getPoliciesFlow(activeUserId), + ) { receiverCurrentValue, ciphers, policies -> + val hasLoginsWithNoOrganizations = ciphers.any { it.login != null && it.organizationId == null } + val onlyOrgPolicy = policies?.any { it.type == PolicyTypeJson.ONLY_ORG } == true + + receiverCurrentValue && (!hasLoginsWithNoOrganizations || onlyOrgPolicy) } } .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 b51da39078..370c1aabfb 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 @@ -15,7 +15,9 @@ import com.x8bit.bitwarden.data.platform.manager.model.CoachMarkTourType import com.x8bit.bitwarden.data.platform.manager.model.FirstTimeState import com.x8bit.bitwarden.data.platform.manager.model.FlagKey import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource +import com.x8bit.bitwarden.data.vault.datasource.network.model.PolicyTypeJson import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson +import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockPolicy import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.flow.MutableStateFlow @@ -425,7 +427,7 @@ class FirstTimeActionManagerTest { @Test fun `if there are login ciphers attached to an organization we should show coach marks`() = runTest { - val mockJsonWithNoLoginAndWithOrganizationId = mockk { + val mockJsonWithLoginAndWithOrganizationId = mockk { every { login } returns mockk() every { organizationId } returns "1234" } @@ -433,13 +435,74 @@ class FirstTimeActionManagerTest { // Enable feature flag so flow emits updates from disk. mutableOnboardingFeatureFlow.update { true } mutableCiphersListFlow.update { - listOf(mockJsonWithNoLoginAndWithOrganizationId) + listOf(mockJsonWithLoginAndWithOrganizationId) } firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { assertTrue(awaitItem()) } } + @Test + fun `if a user has a org only policy with no login items we show coach marks`() = + runTest { + val userState = MOCK_USER_STATE + fakeAuthDiskSource.userState = userState + fakeAuthDiskSource.storePolicies( + userState.activeUserId, + listOf( + createMockPolicy( + isEnabled = true, + number = 2, + organizationId = "1234", + type = PolicyTypeJson.ONLY_ORG, + ), + ), + ) + // Enable feature flag so flow emits updates from disk. + mutableOnboardingFeatureFlow.update { true } + + firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { + assertTrue(awaitItem()) + // Make sure when we change the disk source that we honor that value. + fakeSettingsDiskSource.storeShouldShowGeneratorCoachMark(shouldShow = false) + assertFalse(awaitItem()) + } + } + + @Test + fun `if a user has a org only policy with login items we show coach marks`() = + runTest { + val userState = MOCK_USER_STATE + fakeAuthDiskSource.userState = userState + fakeAuthDiskSource.storePolicies( + userState.activeUserId, + listOf( + createMockPolicy( + isEnabled = true, + number = 2, + organizationId = "1234", + type = PolicyTypeJson.ONLY_ORG, + ), + ), + ) + val mockJsonWithLoginAndWithOrganizationId = mockk { + every { login } returns mockk() + every { organizationId } returns "1234" + } + mutableCiphersListFlow.update { + listOf(mockJsonWithLoginAndWithOrganizationId) + } + // Enable feature flag so flow emits updates from disk. + mutableOnboardingFeatureFlow.update { true } + + firstTimeActionManager.shouldShowGeneratorCoachMarkFlow.test { + assertTrue(awaitItem()) + // Make sure when we change the disk source that we honor that value. + fakeSettingsDiskSource.storeShouldShowGeneratorCoachMark(shouldShow = false) + 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`() {