PM-14009 complete fix importlogins card show logic (#4175)

This commit is contained in:
Dave Severns 2024-10-28 14:22:30 -04:00 committed by GitHub
parent deb9eb8d9b
commit 21a5242abe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 205 additions and 36 deletions

View file

@ -298,4 +298,20 @@ interface SettingsDiskSource {
* Emits updates that track [getShowUnlockSettingBadge] for the given [userId].
*/
fun getShowUnlockSettingBadgeFlow(userId: String): Flow<Boolean?>
/**
* Gets whether or not the given [userId] has signalled they want to import logins later.
*/
fun getShowImportLoginsSettingBadge(userId: String): Boolean?
/**
* Stores the given value for whether or not the given [userId] has signalled they want to
* set import logins later, during first time usage.
*/
fun storeShowImportLoginsSettingBadge(userId: String, showBadge: Boolean?)
/**
* Emits updates that track [getShowImportLoginsSettingBadge] for the given [userId].
*/
fun getShowImportLoginsSettingBadgeFlow(userId: String): Flow<Boolean?>
}

View file

@ -35,6 +35,7 @@ private const val INITIAL_AUTOFILL_DIALOG_SHOWN = "addSitePromptShown"
private const val HAS_USER_LOGGED_IN_OR_CREATED_AN_ACCOUNT_KEY = "hasUserLoggedInOrCreatedAccount"
private const val SHOW_AUTOFILL_SETTING_BADGE = "showAutofillSettingBadge"
private const val SHOW_UNLOCK_SETTING_BADGE = "showUnlockSettingBadge"
private const val SHOW_IMPORT_LOGINS_SETTING_BADGE = "showImportLoginsSettingBadge"
private const val LAST_SCHEME_CHANGE_INSTANT = "lastDatabaseSchemeChangeInstant"
/**
@ -65,6 +66,9 @@ class SettingsDiskSourceImpl(
private val mutableShowUnlockSettingBadgeFlowMap =
mutableMapOf<String, MutableSharedFlow<Boolean?>>()
private val mutableShowImportLoginsSettingBadgeFlowMap =
mutableMapOf<String, MutableSharedFlow<Boolean?>>()
private val mutableIsIconLoadingDisabledFlow = bufferedMutableSharedFlow<Boolean?>()
private val mutableIsCrashLoggingEnabledFlow = bufferedMutableSharedFlow<Boolean?>()
@ -412,6 +416,24 @@ class SettingsDiskSourceImpl(
getMutableShowUnlockSettingBadgeFlow(userId = userId)
.onSubscription { emit(getShowUnlockSettingBadge(userId)) }
override fun getShowImportLoginsSettingBadge(userId: String): Boolean? {
return getBoolean(
key = SHOW_IMPORT_LOGINS_SETTING_BADGE.appendIdentifier(userId),
)
}
override fun storeShowImportLoginsSettingBadge(userId: String, showBadge: Boolean?) {
putBoolean(
key = SHOW_IMPORT_LOGINS_SETTING_BADGE.appendIdentifier(userId),
showBadge,
)
getMutableShowImportLoginsSettingBadgeFlow(userId).tryEmit(showBadge)
}
override fun getShowImportLoginsSettingBadgeFlow(userId: String): Flow<Boolean?> =
getMutableShowImportLoginsSettingBadgeFlow(userId)
.onSubscription { emit(getShowImportLoginsSettingBadge(userId)) }
private fun getMutableLastSyncFlow(
userId: String,
): MutableSharedFlow<Instant?> =
@ -455,4 +477,11 @@ class SettingsDiskSourceImpl(
mutableShowUnlockSettingBadgeFlowMap.getOrPut(userId) {
bufferedMutableSharedFlow(replay = 1)
}
private fun getMutableShowImportLoginsSettingBadgeFlow(
userId: String,
): MutableSharedFlow<Boolean?> =
mutableShowImportLoginsSettingBadgeFlowMap.getOrPut(userId) {
bufferedMutableSharedFlow(replay = 1)
}
}

View file

@ -61,4 +61,9 @@ interface FirstTimeActionManager {
* Update the value of the showImportLogins status for the active user.
*/
fun storeShowImportLogins(showImportLogins: Boolean)
/**
* Update the value of the showImportLoginsSettingsBadge status for the active user.
*/
fun storeShowImportLoginsSettingsBadge(showBadge: Boolean)
}

View file

@ -99,11 +99,11 @@ class FirstTimeActionManagerImpl @Inject constructor(
.filterNotNull()
.flatMapLatest {
combine(
getShowImportLoginsFlowInternal(userId = it),
getShowImportLoginsSettingBadgeFlowInternal(userId = it),
featureFlagManager.getFeatureFlagFlow(FlagKey.ImportLoginsFlow),
) { showImportLogins, importLoginsEnabled ->
val shouldShowImportLogins = showImportLogins && importLoginsEnabled
listOf(shouldShowImportLogins)
val shouldShowImportLoginsSettings = showImportLogins && importLoginsEnabled
listOf(shouldShowImportLoginsSettings)
}
.map { list ->
list.count { showImportLogins -> showImportLogins }
@ -129,12 +129,14 @@ class FirstTimeActionManagerImpl @Inject constructor(
getShowImportLoginsFlowInternal(userId = activeUserId),
settingsDiskSource.getShowUnlockSettingBadgeFlow(userId = activeUserId),
settingsDiskSource.getShowAutoFillSettingBadgeFlow(userId = activeUserId),
getShowImportLoginsSettingBadgeFlowInternal(userId = activeUserId),
),
) {
FirstTimeState(
showImportLoginsCard = it[0],
showSetupUnlockCard = it[1],
showSetupAutofillCard = it[2],
showImportLoginsCardInSettings = it[3],
)
}
}
@ -144,24 +146,12 @@ class FirstTimeActionManagerImpl @Inject constructor(
showImportLoginsCard = null,
showSetupUnlockCard = null,
showSetupAutofillCard = null,
showImportLoginsCardInSettings = null,
),
)
}
.distinctUntilChanged()
/**
* Internal implementation to get a flow of the showImportLogins value which takes
* into account if the vault is empty.
*/
private fun getShowImportLoginsFlowInternal(userId: String): Flow<Boolean> {
return authDiskSource.getShowImportLoginsFlow(userId)
.combine(
vaultDiskSource.getCiphers(userId),
) { showImportLogins, ciphers ->
showImportLogins ?: true && ciphers.isEmpty()
}
}
/**
* Get the current [FirstTimeState] of the active user if available, otherwise return
* a default configuration.
@ -176,12 +166,15 @@ class FirstTimeActionManagerImpl @Inject constructor(
showImportLoginsCard = authDiskSource.getShowImportLogins(it),
showSetupUnlockCard = settingsDiskSource.getShowUnlockSettingBadge(it),
showSetupAutofillCard = settingsDiskSource.getShowAutoFillSettingBadge(it),
showImportLoginsCardInSettings = settingsDiskSource
.getShowImportLoginsSettingBadge(it),
)
}
?: FirstTimeState(
showImportLoginsCard = null,
showSetupUnlockCard = null,
showSetupAutofillCard = null,
showImportLoginsCardInSettings = null,
)
override fun storeShowUnlockSettingBadge(showBadge: Boolean) {
@ -207,4 +200,40 @@ class FirstTimeActionManagerImpl @Inject constructor(
showImportLogins = showImportLogins,
)
}
override fun storeShowImportLoginsSettingsBadge(showBadge: Boolean) {
val activeUserId = authDiskSource.userState?.activeUserId ?: return
settingsDiskSource.storeShowImportLoginsSettingBadge(
userId = activeUserId,
showBadge = showBadge,
)
}
/**
* Internal implementation to get a flow of the showImportLogins value which takes
* into account if the vault is empty.
*/
private fun getShowImportLoginsFlowInternal(userId: String): Flow<Boolean> {
return authDiskSource
.getShowImportLoginsFlow(userId)
.combine(
vaultDiskSource.getCiphers(userId),
) { showImportLogins, ciphers ->
showImportLogins ?: true && ciphers.isEmpty()
}
}
/**
* Internal implementation to get a flow of the showImportLogins value which takes
* into account if the vault is empty.
*/
private fun getShowImportLoginsSettingBadgeFlowInternal(userId: String): Flow<Boolean> {
return settingsDiskSource
.getShowImportLoginsSettingBadgeFlow(userId)
.combine(
vaultDiskSource.getCiphers(userId),
) { showImportLogins, ciphers ->
showImportLogins ?: false && ciphers.isEmpty()
}
}
}

View file

@ -5,6 +5,7 @@ package com.x8bit.bitwarden.data.platform.manager.model
*/
data class FirstTimeState(
val showImportLoginsCard: Boolean,
val showImportLoginsCardInSettings: Boolean,
val showSetupUnlockCard: Boolean,
val showSetupAutofillCard: Boolean,
) {
@ -16,9 +17,11 @@ data class FirstTimeState(
showImportLoginsCard: Boolean? = null,
showSetupUnlockCard: Boolean? = null,
showSetupAutofillCard: Boolean? = null,
showImportLoginsCardInSettings: Boolean? = null,
) : this(
showImportLoginsCard = showImportLoginsCard ?: true,
showSetupUnlockCard = showSetupUnlockCard ?: false,
showSetupAutofillCard = showSetupAutofillCard ?: false,
showImportLoginsCardInSettings = showImportLoginsCardInSettings ?: false,
)
}

View file

@ -33,7 +33,7 @@ class VaultSettingsViewModel @Inject constructor(
.toBaseWebVaultImportUrl,
isNewImportLoginsFlowEnabled = featureFlagManager
.getFeatureFlag(FlagKey.ImportLoginsFlow),
showImportActionCard = firstTimeState.showImportLoginsCard,
showImportActionCard = firstTimeState.showImportLoginsCardInSettings,
)
},
) {
@ -48,7 +48,7 @@ class VaultSettingsViewModel @Inject constructor(
.firstTimeStateFlow
.map {
VaultSettingsAction.Internal.UserFirstTimeStateChanged(
showImportLoginsCard = it.showImportLoginsCard,
showImportLoginsCard = it.showImportLoginsCardInSettings,
)
}
.onEach(::sendAction)
@ -79,7 +79,7 @@ class VaultSettingsViewModel @Inject constructor(
private fun handleImportLoginsCardDismissClicked() {
if (!state.shouldShowImportCard) return
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
}
private fun handleImportLoginsCardClicked() {

View file

@ -97,6 +97,7 @@ class ImportLoginsViewModel @Inject constructor(
is SyncVaultDataResult.Success -> {
if (result.itemsAvailable) {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
mutableStateFlow.update {
it.copy(
showBottomSheet = true,
@ -160,6 +161,8 @@ class ImportLoginsViewModel @Inject constructor(
private fun handleConfirmImportLater() {
dismissDialog()
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = true)
sendEvent(ImportLoginsEvent.NavigateBack)
}

View file

@ -186,6 +186,7 @@ class VaultViewModel @Inject constructor(
}
private fun handleDismissImportActionCard() {
firstTimeActionManager.storeShowImportLoginsSettingsBadge(true)
if (!state.showImportActionCard) return
firstTimeActionManager.storeShowImportLogins(false)
}

View file

@ -1062,10 +1062,10 @@ class SettingsDiskSourceTest {
@Test
fun `storeShowAutoFillSettingBadge should update the flow value`() = runTest {
val mockUserId = "mockUserId"
settingsDiskSource.storeShowAutoFillSettingBadge(mockUserId, true)
settingsDiskSource.getShowAutoFillSettingBadgeFlow(userId = mockUserId).test {
// The initial values of the Flow are in sync
assertTrue(awaitItem() ?: false)
assertFalse(awaitItem() ?: false)
settingsDiskSource.storeShowAutoFillSettingBadge(mockUserId, true)
assertTrue(awaitItem() ?: false)
// update the value to false
@ -1103,10 +1103,10 @@ class SettingsDiskSourceTest {
@Test
fun `storeShowUnlockSettingsBadge should update the flow value`() = runTest {
val mockUserId = "mockUserId"
settingsDiskSource.storeShowUnlockSettingBadge(mockUserId, true)
settingsDiskSource.getShowUnlockSettingBadgeFlow(userId = mockUserId).test {
// The initial values of the Flow are in sync
assertTrue(awaitItem() ?: false)
assertFalse(awaitItem() ?: false)
settingsDiskSource.storeShowUnlockSettingBadge(mockUserId, true)
assertTrue(awaitItem() ?: false)
// update the value to false
@ -1165,4 +1165,47 @@ class SettingsDiskSourceTest {
actual,
)
}
@Test
fun `getShowImportLoginsSettingBadge should pull from shared preferences`() {
val mockUserId = "mockUserId"
val showImportLoginsSettingBadgeKey =
"bwPreferencesStorage:showImportLoginsSettingBadge_$mockUserId"
fakeSharedPreferences.edit {
putBoolean(showImportLoginsSettingBadgeKey, true)
}
assertTrue(
settingsDiskSource.getShowImportLoginsSettingBadge(userId = mockUserId)!!,
)
}
@Test
fun `storeShowImportLoginsSettingBadge should update SharedPreferences`() {
val mockUserId = "mockUserId"
val showImportLoginsSettingBadgeKey =
"bwPreferencesStorage:showImportLoginsSettingBadge_$mockUserId"
settingsDiskSource.storeShowImportLoginsSettingBadge(mockUserId, true)
assertTrue(
fakeSharedPreferences.getBoolean(showImportLoginsSettingBadgeKey, false),
)
}
@Test
fun `storeShowImportLoginsSettingBadge should update the flow value`() = runTest {
val mockUserId = "mockUserId"
settingsDiskSource.getShowImportLoginsSettingBadgeFlow(mockUserId).test {
// The initial values of the Flow are in sync
assertFalse(awaitItem() ?: false)
settingsDiskSource.storeShowImportLoginsSettingBadge(
userId = mockUserId,
showBadge = true,
)
assertTrue(awaitItem() ?: false)
// update the value to false
settingsDiskSource.storeShowImportLoginsSettingBadge(
userId = mockUserId, false,
)
assertFalse(awaitItem() ?: true)
}
}
}

View file

@ -63,6 +63,7 @@ class FakeSettingsDiskSource : SettingsDiskSource {
private val userSignIns = mutableMapOf<String, Boolean>()
private val userShowAutoFillBadge = mutableMapOf<String, Boolean?>()
private val userShowUnlockBadge = mutableMapOf<String, Boolean?>()
private val userShowImportLoginsBadge = mutableMapOf<String, Boolean?>()
private var storedLastDatabaseSchemeChangeInstant: Instant? = null
private val mutableShowAutoFillSettingBadgeFlowMap =
@ -71,6 +72,9 @@ class FakeSettingsDiskSource : SettingsDiskSource {
private val mutableShowUnlockSettingBadgeFlowMap =
mutableMapOf<String, MutableSharedFlow<Boolean?>>()
private val mutableShowImportLoginsSettingBadgeFlowMap =
mutableMapOf<String, MutableSharedFlow<Boolean?>>()
override var appLanguage: AppLanguage? = null
override var appTheme: AppTheme
@ -323,6 +327,19 @@ class FakeSettingsDiskSource : SettingsDiskSource {
emit(getShowUnlockSettingBadge(userId = userId))
}
override fun getShowImportLoginsSettingBadge(userId: String): Boolean? =
userShowImportLoginsBadge[userId]
override fun storeShowImportLoginsSettingBadge(userId: String, showBadge: Boolean?) {
userShowImportLoginsBadge[userId] = showBadge
getMutableShowImportLoginsSettingBadgeFlow(userId).tryEmit(showBadge)
}
override fun getShowImportLoginsSettingBadgeFlow(userId: String): Flow<Boolean?> =
getMutableShowImportLoginsSettingBadgeFlow(userId = userId).onSubscription {
emit(getShowImportLoginsSettingBadge(userId = userId))
}
//region Private helper functions
private fun getMutableScreenCaptureAllowedFlow(userId: String): MutableSharedFlow<Boolean?> {
return mutableScreenCaptureAllowedFlowMap.getOrPut(userId) {
@ -369,5 +386,11 @@ class FakeSettingsDiskSource : SettingsDiskSource {
bufferedMutableSharedFlow(replay = 1)
}
private fun getMutableShowImportLoginsSettingBadgeFlow(
userId: String,
): MutableSharedFlow<Boolean?> = mutableShowImportLoginsSettingBadgeFlowMap.getOrPut(userId) {
bufferedMutableSharedFlow(replay = 1)
}
//endregion Private helper functions
}

View file

@ -110,7 +110,7 @@ class FirstTimeActionManagerTest {
// for the import logins count it is dependent on the feature flag state and
// cipher list being empty
mutableImportLoginsFlow.update { true }
fakeAuthDiskSource.storeShowImportLogins(USER_ID, true)
fakeSettingsDiskSource.storeShowImportLoginsSettingBadge(USER_ID, true)
assertEquals(2, awaitItem())
}
}
@ -127,15 +127,15 @@ class FirstTimeActionManagerTest {
firstTimeActionManager.allVaultSettingsBadgeCountFlow.test {
assertEquals(0, awaitItem())
mutableImportLoginsFlow.update { true }
fakeAuthDiskSource.storeShowImportLogins(USER_ID, true)
fakeSettingsDiskSource.storeShowImportLoginsSettingBadge(USER_ID, true)
assertEquals(1, awaitItem())
mutableImportLoginsFlow.update { false }
assertEquals(0, awaitItem())
mutableImportLoginsFlow.update { true }
assertEquals(1, awaitItem())
fakeAuthDiskSource.storeShowImportLogins(USER_ID, false)
fakeSettingsDiskSource.storeShowImportLoginsSettingBadge(USER_ID, false)
assertEquals(0, awaitItem())
fakeAuthDiskSource.storeShowImportLogins(USER_ID, true)
fakeSettingsDiskSource.storeShowImportLoginsSettingBadge(USER_ID, true)
assertEquals(1, awaitItem())
mutableCiphersListFlow.update {
listOf(mockCipher)
@ -156,7 +156,7 @@ class FirstTimeActionManagerTest {
),
awaitItem(),
)
fakeAuthDiskSource.storeShowImportLogins(USER_ID, false)
firstTimeActionManager.storeShowImportLogins(false)
assertEquals(
FirstTimeState(
showImportLoginsCard = false,
@ -233,6 +233,13 @@ class FirstTimeActionManagerTest {
firstTimeActionManager.storeShowImportLogins(showImportLogins = true)
assertTrue(fakeAuthDiskSource.getShowImportLogins(userId = USER_ID)!!)
}
@Test
fun `storeShowImportLoginsSettingsBadge should store value of true to disk`() {
fakeAuthDiskSource.userState = MOCK_USER_STATE
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
assertFalse(fakeSettingsDiskSource.getShowImportLoginsSettingBadge(userId = USER_ID)!!)
}
}
private const val USER_ID: String = "userId"

View file

@ -31,7 +31,7 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
private val firstTimeActionManager = mockk<FirstTimeActionManager> {
every { currentOrDefaultUserFirstTimeState } returns DEFAULT_FIRST_TIME_STATE
every { firstTimeStateFlow } returns mutableFirstTimeStateFlow
every { storeShowImportLogins(any()) } just runs
every { storeShowImportLoginsSettingsBadge(any()) } just runs
}
@Test
@ -84,7 +84,7 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
val viewModel = createViewModel()
assertTrue(viewModel.stateFlow.value.shouldShowImportCard)
mutableFirstTimeStateFlow.update {
it.copy(showImportLoginsCard = false)
it.copy(showImportLoginsCardInSettings = false)
}
assertFalse(viewModel.stateFlow.value.shouldShowImportCard)
}
@ -111,7 +111,7 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
)
}
verify(exactly = 0) {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
}
}
@ -121,7 +121,7 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
val viewModel = createViewModel()
viewModel.trySendAction(VaultSettingsAction.ImportLoginsCardDismissClick)
verify(exactly = 1) {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
}
}
@ -132,7 +132,7 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
val viewModel = createViewModel()
viewModel.trySendAction(VaultSettingsAction.ImportLoginsCardDismissClick)
verify(exactly = 0) {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
}
}
@ -143,4 +143,4 @@ class VaultSettingsViewModelTest : BaseViewModelTest() {
)
}
private val DEFAULT_FIRST_TIME_STATE = FirstTimeState(showImportLoginsCard = true)
private val DEFAULT_FIRST_TIME_STATE = FirstTimeState(showImportLoginsCardInSettings = true)

View file

@ -28,6 +28,7 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
private val firstTimeActionManager: FirstTimeActionManager = mockk {
every { storeShowImportLogins(any()) } just runs
every { storeShowImportLoginsSettingsBadge(any()) } just runs
}
@Test
@ -100,8 +101,9 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
}
}
@Suppress("MaxLineLength")
@Test
fun `ConfirmImportLater sets dialog state to null and sends NavigateBack event`() = runTest {
fun `ConfirmImportLater sets dialog state to null, sends NavigateBack event, and stores first time values`() = runTest {
val viewModel = createViewModel()
viewModel.stateEventFlow(backgroundScope) { stateFlow, eventFlow ->
// Initial state
@ -133,6 +135,10 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
eventFlow.awaitItem(),
)
}
verify(exactly = 1) {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = true)
}
}
@Test
@ -320,6 +326,7 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
)
verify {
firstTimeActionManager.storeShowImportLogins(showImportLogins = false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(showBadge = false)
}
}

View file

@ -86,6 +86,7 @@ class VaultViewModelTest : BaseViewModelTest() {
private val firstTimeActionManager: FirstTimeActionManager = mockk {
every { firstTimeStateFlow } returns mutableFirstTimeStateFlow
every { storeShowImportLogins(any()) } just runs
every { storeShowImportLoginsSettingsBadge(any()) } just runs
}
private val authRepository: AuthRepository =
@ -1555,12 +1556,14 @@ class VaultViewModelTest : BaseViewModelTest() {
}
}
@Suppress("MaxLineLength")
@Test
fun `when DismissImportActionCard is sent, repository called to set value to false`() {
fun `when DismissImportActionCard is sent, repository called to showImportLogins to false and storeShowImportLoginsBadge to true`() {
val viewModel = createViewModel()
viewModel.trySendAction(VaultAction.DismissImportActionCard)
verify(exactly = 1) {
firstTimeActionManager.storeShowImportLogins(false)
firstTimeActionManager.storeShowImportLoginsSettingsBadge(true)
}
}