From 853f25bf573dcf440854843ff10da664648d5887 Mon Sep 17 00:00:00 2001 From: Dave Severns <149429124+dseverns-livefront@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:37:30 -0400 Subject: [PATCH] [PM-12595] add notification badge to settings item in settings tab screen (#3976) --- .../feature/settings/SettingsScreen.kt | 46 ++++++- .../feature/settings/SettingsViewModel.kt | 70 ++++++++++- .../feature/settings/SettingsScreenTest.kt | 119 +++++++++++------- .../feature/settings/SettingsViewModelTest.kt | 70 ++++++++++- 4 files changed, 246 insertions(+), 59 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreen.kt index 3a128db88..fbbfde466 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreen.kt @@ -1,15 +1,18 @@ package com.x8bit.bitwarden.ui.platform.feature.settings +import androidx.compose.foundation.background import androidx.compose.foundation.clickable import androidx.compose.foundation.interaction.MutableInteractionSource import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.defaultMinSize import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.verticalScroll import androidx.compose.material3.ExperimentalMaterial3Api @@ -20,6 +23,7 @@ import androidx.compose.material3.TopAppBarDefaults import androidx.compose.material3.rememberTopAppBarState import androidx.compose.material3.ripple import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -29,12 +33,14 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.hilt.navigation.compose.hiltViewModel +import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.x8bit.bitwarden.R import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.bottomDivider import com.x8bit.bitwarden.ui.platform.base.util.mirrorIfRtl import com.x8bit.bitwarden.ui.platform.components.appbar.BitwardenMediumTopAppBar +import com.x8bit.bitwarden.ui.platform.components.badge.NotificationBadge import com.x8bit.bitwarden.ui.platform.components.scaffold.BitwardenScaffold import com.x8bit.bitwarden.ui.platform.components.util.rememberVectorPainter import com.x8bit.bitwarden.ui.platform.theme.BitwardenTheme @@ -53,6 +59,7 @@ fun SettingsScreen( onNavigateToVault: () -> Unit, viewModel: SettingsViewModel = hiltViewModel(), ) { + val state by viewModel.stateFlow.collectAsStateWithLifecycle() EventsEffect(viewModel = viewModel) { event -> when (event) { SettingsEvent.NavigateAbout -> onNavigateToAbout() @@ -82,16 +89,20 @@ fun SettingsScreen( .fillMaxSize() .verticalScroll(state = rememberScrollState()), ) { - Settings.entries.forEach { + Settings.entries.forEach { settingEntry -> SettingsRow( - text = it.text, + text = settingEntry.text, onClick = remember(viewModel) { - { viewModel.trySendAction(SettingsAction.SettingsClick(it)) } + { viewModel.trySendAction(SettingsAction.SettingsClick(settingEntry)) } }, modifier = Modifier - .testTag(it.testTag) + .testTag(settingEntry.testTag) .padding(horizontal = 16.dp) .fillMaxWidth(), + notificationCount = state.notificationBadgeCountMap.getOrDefault( + key = settingEntry, + defaultValue = 0, + ), ) } } @@ -103,6 +114,7 @@ private fun SettingsRow( text: Text, onClick: () -> Unit, modifier: Modifier = Modifier, + notificationCount: Int, ) { Row( modifier = Modifier @@ -126,6 +138,27 @@ private fun SettingsRow( style = MaterialTheme.typography.bodyLarge, color = MaterialTheme.colorScheme.onSurface, ) + TrailingContent(notificationCount = notificationCount) + } +} + +@Composable +private fun TrailingContent( + notificationCount: Int, + modifier: Modifier = Modifier, +) { + Row( + modifier = modifier, + verticalAlignment = Alignment.CenterVertically, + ) { + val notificationBadgeVisible = notificationCount > 0 + NotificationBadge( + notificationCount = notificationCount, + isVisible = notificationBadgeVisible, + ) + if (notificationBadgeVisible) { + Spacer(modifier = Modifier.width(12.dp)) + } Icon( painter = rememberVectorPainter(id = R.drawable.ic_navigate_next), contentDescription = null, @@ -138,18 +171,21 @@ private fun SettingsRow( } @Preview +@Preview(name = "Right-To-Left", locale = "ar") @Composable private fun SettingsRows_preview() { BitwardenTheme { Column( modifier = Modifier + .background(MaterialTheme.colorScheme.background) .padding(16.dp) .fillMaxSize(), ) { - Settings.entries.forEach { + Settings.entries.forEachIndexed { index, it -> SettingsRow( text = it.text, onClick = { }, + notificationCount = index % 3, ) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModel.kt index 087a6e2d8..90765b7be 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModel.kt @@ -1,22 +1,62 @@ package com.x8bit.bitwarden.ui.platform.feature.settings import androidx.compose.material3.Text +import androidx.lifecycle.viewModelScope import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.asText import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.update import javax.inject.Inject /** * View model for the settings screen. */ @HiltViewModel -class SettingsViewModel @Inject constructor() : BaseViewModel( - initialState = Unit, +class SettingsViewModel @Inject constructor( + settingsRepository: SettingsRepository, +) : BaseViewModel( + initialState = SettingsState( + securityCount = settingsRepository.allSecuritySettingsBadgeCountFlow.value, + autoFillCount = settingsRepository.allAutofillSettingsBadgeCountFlow.value, + ), ) { + + init { + combine( + settingsRepository.allSecuritySettingsBadgeCountFlow, + settingsRepository.allAutofillSettingsBadgeCountFlow, + ) { securityCount, autofillCount -> + SettingsAction.Internal.SettingsNotificationCountUpdate( + securityCount = securityCount, + autoFillCount = autofillCount, + ) + } + .onEach(::sendAction) + .launchIn(viewModelScope) + } + override fun handleAction(action: SettingsAction): Unit = when (action) { is SettingsAction.SettingsClick -> handleAccountSecurityClick(action) + is SettingsAction.Internal.SettingsNotificationCountUpdate -> { + handleSettingsNotificationCountUpdate(action) + } + } + + private fun handleSettingsNotificationCountUpdate( + action: SettingsAction.Internal.SettingsNotificationCountUpdate, + ) { + mutableStateFlow.update { + it.copy( + autoFillCount = action.autoFillCount, + securityCount = action.securityCount, + ) + } } private fun handleAccountSecurityClick(action: SettingsAction.SettingsClick) { @@ -48,6 +88,19 @@ class SettingsViewModel @Inject constructor() : BaseViewModel = mapOf( + Settings.ACCOUNT_SECURITY to autoFillCount, + Settings.AUTO_FILL to securityCount, + ) +} + /** * Models events for the settings screen. */ @@ -93,6 +146,19 @@ sealed class SettingsAction { data class SettingsClick( val settings: Settings, ) : SettingsAction() + + /** + * Models internal actions for the settings screen. + */ + sealed class Internal : SettingsAction() { + /** + * Update the notification count for the settings rows. + */ + data class SettingsNotificationCountUpdate( + val autoFillCount: Int, + val securityCount: Int, + ) : Internal() + } } /** diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreenTest.kt index 084c89f7c..0b008c26a 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsScreenTest.kt @@ -1,26 +1,33 @@ package com.x8bit.bitwarden.ui.platform.feature.settings +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick +import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.ui.platform.base.BaseComposeTest import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.runs import io.mockk.verify -import kotlinx.coroutines.flow.emptyFlow -import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.update import org.junit.Assert.assertTrue import org.junit.Test class SettingsScreenTest : BaseComposeTest() { + private val mutableStateFlow = MutableStateFlow(DEFAULT_STATE) + private val mutableEventFlow = bufferedMutableSharedFlow() + private val viewModel = mockk { + every { stateFlow } returns mutableStateFlow + every { eventFlow } returns mutableEventFlow + } + @Test fun `on about row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { trySendAction(SettingsAction.SettingsClick(Settings.ABOUT)) } just runs - } + + every { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.ABOUT)) } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -38,12 +45,9 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on account security row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { - trySendAction(SettingsAction.SettingsClick(Settings.ACCOUNT_SECURITY)) - } just runs - } + every { + viewModel.trySendAction(SettingsAction.SettingsClick(Settings.ACCOUNT_SECURITY)) + } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -61,10 +65,9 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on appearance row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { trySendAction(SettingsAction.SettingsClick(Settings.APPEARANCE)) } just runs - } + every { + viewModel.trySendAction(SettingsAction.SettingsClick(Settings.APPEARANCE)) + } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -82,10 +85,10 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on auto-fill row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { trySendAction(SettingsAction.SettingsClick(Settings.AUTO_FILL)) } just runs - } + + every { + viewModel.trySendAction(SettingsAction.SettingsClick(Settings.AUTO_FILL)) + } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -103,10 +106,8 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on other row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { trySendAction(SettingsAction.SettingsClick(Settings.OTHER)) } just runs - } + + every { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.OTHER)) } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -124,10 +125,8 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on vault row click should emit SettingsClick`() { - val viewModel = mockk { - every { eventFlow } returns emptyFlow() - every { trySendAction(SettingsAction.SettingsClick(Settings.VAULT)) } just runs - } + + every { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.VAULT)) } just runs composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -146,9 +145,6 @@ class SettingsScreenTest : BaseComposeTest() { @Test fun `on NavigateAbout should call onNavigateToAbout`() { var haveCalledNavigateToAbout = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateAbout) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -162,15 +158,13 @@ class SettingsScreenTest : BaseComposeTest() { onNavigateToVault = { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateAbout) assertTrue(haveCalledNavigateToAbout) } @Test fun `on NavigateAccountSecurity should call onNavigateToAccountSecurity`() { var haveCalledNavigateToAccountSecurity = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateAccountSecurity) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -184,15 +178,13 @@ class SettingsScreenTest : BaseComposeTest() { onNavigateToVault = { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateAccountSecurity) assertTrue(haveCalledNavigateToAccountSecurity) } @Test fun `on NavigateAccountSecurity should call NavigateAppearance`() { var haveCalledNavigateToAppearance = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateAppearance) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -204,15 +196,13 @@ class SettingsScreenTest : BaseComposeTest() { onNavigateToVault = { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateAppearance) assertTrue(haveCalledNavigateToAppearance) } @Test fun `on NavigateAccountSecurity should call onNavigateToAutoFill`() { var haveCalledNavigateToAutoFill = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateAutoFill) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -226,15 +216,13 @@ class SettingsScreenTest : BaseComposeTest() { onNavigateToVault = { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateAutoFill) assertTrue(haveCalledNavigateToAutoFill) } @Test fun `on NavigateAccountSecurity should call onNavigateToOther`() { var haveCalledNavigateToOther = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateOther) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -248,15 +236,13 @@ class SettingsScreenTest : BaseComposeTest() { onNavigateToVault = { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateOther) assertTrue(haveCalledNavigateToOther) } @Test fun `on NavigateAccountSecurity should call NavigateVault`() { var haveCalledNavigateToVault = false - val viewModel = mockk { - every { eventFlow } returns flowOf(SettingsEvent.NavigateVault) - } composeTestRule.setContent { SettingsScreen( viewModel = viewModel, @@ -270,6 +256,47 @@ class SettingsScreenTest : BaseComposeTest() { }, ) } + mutableEventFlow.tryEmit(SettingsEvent.NavigateVault) assertTrue(haveCalledNavigateToVault) } + + @Test + fun `Settings screen should show correct number of notification badges based on state`() { + composeTestRule.setContent { + SettingsScreen( + viewModel = viewModel, + onNavigateToAbout = {}, + onNavigateToAppearance = {}, + onNavigateToAutoFill = {}, + onNavigateToOther = {}, + onNavigateToVault = {}, + onNavigateToAccountSecurity = {}, + ) + } + + composeTestRule + .onNodeWithText(text = "1", useUnmergedTree = true) + .assertDoesNotExist() + + mutableStateFlow.update { it.copy(autoFillCount = 1) } + + composeTestRule + .onNodeWithText(text = "1", useUnmergedTree = true) + .assertExists() + + mutableStateFlow.update { it.copy(securityCount = 1) } + + composeTestRule + .onAllNodesWithText(text = "1", useUnmergedTree = true)[0] + .assertExists() + + composeTestRule + .onAllNodesWithText(text = "1", useUnmergedTree = true)[1] + .assertExists() + } } + +private val DEFAULT_STATE = SettingsState( + securityCount = 0, + autoFillCount = 0, +) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModelTest.kt index 6f20a1901..cff3ad698 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/SettingsViewModelTest.kt @@ -1,16 +1,28 @@ package com.x8bit.bitwarden.ui.platform.feature.settings import app.cash.turbine.test +import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.base.BaseViewModelTest +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.update import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test class SettingsViewModelTest : BaseViewModelTest() { + private val mutableAutofillBadgeCountFlow = MutableStateFlow(0) + private val mutableSecurityBadgeCountFlow = MutableStateFlow(0) + private val settingsRepository = mockk { + every { allSecuritySettingsBadgeCountFlow } returns mutableSecurityBadgeCountFlow + every { allAutofillSettingsBadgeCountFlow } returns mutableAutofillBadgeCountFlow + } + @Test fun `on SettingsClick with ABOUT should emit NavigateAbout`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.ABOUT)) assertEquals(SettingsEvent.NavigateAbout, awaitItem()) @@ -19,7 +31,7 @@ class SettingsViewModelTest : BaseViewModelTest() { @Test fun `on SettingsClick with ACCOUNT_SECURITY should emit NavigateAccountSecurity`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.ACCOUNT_SECURITY)) assertEquals(SettingsEvent.NavigateAccountSecurity, awaitItem()) @@ -28,7 +40,7 @@ class SettingsViewModelTest : BaseViewModelTest() { @Test fun `on SettingsClick with APPEARANCE should emit NavigateAppearance`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.APPEARANCE)) assertEquals(SettingsEvent.NavigateAppearance, awaitItem()) @@ -37,7 +49,7 @@ class SettingsViewModelTest : BaseViewModelTest() { @Test fun `on SettingsClick with AUTO_FILL should emit NavigateAutoFill`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.AUTO_FILL)) assertEquals(SettingsEvent.NavigateAutoFill, awaitItem()) @@ -46,7 +58,7 @@ class SettingsViewModelTest : BaseViewModelTest() { @Test fun `on SettingsClick with OTHER should emit NavigateOther`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.OTHER)) assertEquals(SettingsEvent.NavigateOther, awaitItem()) @@ -55,10 +67,56 @@ class SettingsViewModelTest : BaseViewModelTest() { @Test fun `on SettingsClick with VAULT should emit NavigateVault`() = runTest { - val viewModel = SettingsViewModel() + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) viewModel.eventFlow.test { viewModel.trySendAction(SettingsAction.SettingsClick(Settings.VAULT)) assertEquals(SettingsEvent.NavigateVault, awaitItem()) } } + + @Test + fun `initial state reflects the current state of the repository`() { + mutableAutofillBadgeCountFlow.update { 1 } + mutableSecurityBadgeCountFlow.update { 2 } + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) + assertEquals( + SettingsState( + autoFillCount = 1, + securityCount = 2, + ), + viewModel.stateFlow.value, + ) + } + + @Test + fun `State updates when repository emits new values for badge counts`() = runTest { + val viewModel = SettingsViewModel(settingsRepository = settingsRepository) + viewModel.stateFlow.test { + assertEquals( + SettingsState( + autoFillCount = 0, + securityCount = 0, + ), + awaitItem(), + ) + + mutableSecurityBadgeCountFlow.update { 2 } + assertEquals( + SettingsState( + autoFillCount = 0, + securityCount = 2, + ), + awaitItem(), + ) + + mutableAutofillBadgeCountFlow.update { 1 } + assertEquals( + SettingsState( + autoFillCount = 1, + securityCount = 2, + ), + awaitItem(), + ) + } + } }