From f5716edc8eb60adb42a9635f4205511a146f8eb3 Mon Sep 17 00:00:00 2001 From: Jonatan Rhodin Date: Fri, 17 Nov 2023 13:13:03 +0100 Subject: [PATCH 1/3] Persist problem reports in memory until successfuly submitted --- .../compose/screen/ReportProblemScreen.kt | 26 ++++---- .../dataproxy/MullvadProblemReport.kt | 4 +- .../net/mullvad/mullvadvpn/di/UiModule.kt | 5 +- .../repository/ProblemReportRepository.kt | 19 ++++++ .../ui/fragment/ProblemReportFragment.kt | 4 +- .../viewmodel/ReportProblemViewModel.kt | 64 ++++++++++++++----- 6 files changed, 89 insertions(+), 33 deletions(-) create mode 100644 android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ProblemReportRepository.kt diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt index fbfb632e2333..219bc6b8d3e1 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt @@ -15,9 +15,7 @@ import androidx.compose.material3.Text import androidx.compose.material3.TextField import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember -import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -94,16 +92,15 @@ fun ReportProblemScreen( onDismissNoEmailDialog: () -> Unit = {}, onClearSendResult: () -> Unit = {}, onNavigateToViewLogs: () -> Unit = {}, + updateEmail: (String) -> Unit = {}, + updateDescription: (String) -> Unit = {}, onBackClick: () -> Unit = {} ) { - var email by rememberSaveable { mutableStateOf("") } - var description by rememberSaveable { mutableStateOf("") } - // Dialog to show confirm if no email was added if (uiState.showConfirmNoEmail) { ReportProblemNoEmailDialog( onDismiss = onDismissNoEmailDialog, - onConfirm = { onSendReport(email, description) } + onConfirm = { onSendReport(uiState.email, uiState.description) } ) } @@ -124,7 +121,10 @@ fun ReportProblemScreen( when (uiState.sendingState) { SendingReportUiState.Sending -> SendingContent() is SendingReportUiState.Error -> - ErrorContent({ onSendReport(email, description) }, onClearSendResult) + ErrorContent( + { onSendReport(uiState.email, uiState.description) }, + onClearSendResult + ) is SendingReportUiState.Success -> SentContent(uiState.sendingState) } return@ScaffoldWithMediumTopBar @@ -146,8 +146,8 @@ fun ReportProblemScreen( TextField( modifier = Modifier.fillMaxWidth(), - value = email, - onValueChange = { email = it }, + value = uiState.email, + onValueChange = updateEmail, maxLines = 1, singleLine = true, placeholder = { Text(text = stringResource(id = R.string.user_email_hint)) }, @@ -156,8 +156,8 @@ fun ReportProblemScreen( TextField( modifier = Modifier.fillMaxWidth().weight(1f), - value = description, - onValueChange = { description = it }, + value = uiState.description, + onValueChange = updateDescription, placeholder = { Text(stringResource(R.string.user_message_hint)) }, colors = mullvadWhiteTextFieldColors() ) @@ -169,8 +169,8 @@ fun ReportProblemScreen( ) Spacer(modifier = Modifier.height(Dimens.buttonSpacing)) VariantButton( - onClick = { onSendReport(email, description) }, - isEnabled = description.isNotEmpty(), + onClick = { onSendReport(uiState.email, uiState.description) }, + isEnabled = uiState.description.isNotEmpty(), text = stringResource(id = R.string.send) ) } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt index 67eaeca48d1a..02c2d81a3dbc 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt @@ -19,7 +19,7 @@ sealed interface SendProblemReportResult { } } -data class UserReport(val email: String?, val message: String) +data class UserReport(val email: String?, val description: String) class MullvadProblemReport(context: Context, val dispatcher: CoroutineDispatcher = Dispatchers.IO) { @@ -49,7 +49,7 @@ class MullvadProblemReport(context: Context, val dispatcher: CoroutineDispatcher withContext(dispatcher) { sendProblemReport( userReport.email ?: "", - userReport.message, + userReport.description, logsPath.absolutePath, cacheDirectory.absolutePath ) diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt index 5be527ac0cc9..56df6699de97 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt @@ -18,6 +18,7 @@ import net.mullvad.mullvadvpn.repository.ChangelogRepository import net.mullvad.mullvadvpn.repository.DeviceRepository import net.mullvad.mullvadvpn.repository.InAppNotificationController import net.mullvad.mullvadvpn.repository.PrivacyDisclaimerRepository +import net.mullvad.mullvadvpn.repository.ProblemReportRepository import net.mullvad.mullvadvpn.repository.SettingsRepository import net.mullvad.mullvadvpn.ui.serviceconnection.RelayListListener import net.mullvad.mullvadvpn.ui.serviceconnection.ServiceConnectionManager @@ -116,6 +117,8 @@ val uiModule = module { } } + single { ProblemReportRepository() } + // View models viewModel { AccountViewModel(get(), get(), get(), get()) } viewModel { @@ -131,7 +134,7 @@ val uiModule = module { viewModel { VoucherDialogViewModel(get(), get()) } viewModel { VpnSettingsViewModel(get(), get(), get(), get(), get()) } viewModel { WelcomeViewModel(get(), get(), get(), get()) } - viewModel { ReportProblemViewModel(get()) } + viewModel { ReportProblemViewModel(get(), get()) } viewModel { ViewLogsViewModel(get()) } viewModel { OutOfTimeViewModel(get(), get(), get(), get()) } } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ProblemReportRepository.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ProblemReportRepository.kt new file mode 100644 index 000000000000..3086ee9b802c --- /dev/null +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/ProblemReportRepository.kt @@ -0,0 +1,19 @@ +package net.mullvad.mullvadvpn.repository + +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import net.mullvad.mullvadvpn.dataproxy.UserReport + +class ProblemReportRepository { + private val _problemReport = MutableStateFlow(UserReport("", "")) + val problemReport: StateFlow = _problemReport.asStateFlow() + + fun setEmail(email: String) { + _problemReport.value = _problemReport.value.copy(email = email) + } + + fun setDescription(description: String) { + _problemReport.value = _problemReport.value.copy(description = description) + } +} diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/ProblemReportFragment.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/ProblemReportFragment.kt index 397039719c0e..fd489e563f40 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/ProblemReportFragment.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/ProblemReportFragment.kt @@ -30,7 +30,9 @@ class ProblemReportFragment : BaseFragment() { onSendReport = { email, description -> vm.sendReport(email, description) }, onDismissNoEmailDialog = vm::dismissConfirmNoEmail, onClearSendResult = vm::clearSendResult, - onNavigateToViewLogs = { showLogs() } + onNavigateToViewLogs = { showLogs() }, + updateEmail = vm::updateEmail, + updateDescription = vm::updateDescription ) { activity?.onBackPressed() } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModel.kt index a7daf9e8d9d2..82e66b0c4b98 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModel.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemViewModel.kt @@ -5,17 +5,21 @@ import androidx.lifecycle.viewModelScope import kotlinx.coroutines.async import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.update +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import net.mullvad.mullvadvpn.constant.MINIMUM_LOADING_TIME_MILLIS import net.mullvad.mullvadvpn.dataproxy.MullvadProblemReport import net.mullvad.mullvadvpn.dataproxy.SendProblemReportResult import net.mullvad.mullvadvpn.dataproxy.UserReport +import net.mullvad.mullvadvpn.repository.ProblemReportRepository data class ReportProblemUiState( val showConfirmNoEmail: Boolean = false, - val sendingState: SendingReportUiState? = null + val sendingState: SendingReportUiState? = null, + val email: String = "", + val description: String = "", ) sealed interface SendingReportUiState { @@ -26,11 +30,28 @@ sealed interface SendingReportUiState { data class Error(val error: SendProblemReportResult.Error) : SendingReportUiState } -class ReportProblemViewModel(private val mullvadProblemReporter: MullvadProblemReport) : - ViewModel() { - - private val _uiState = MutableStateFlow(ReportProblemUiState()) - val uiState = _uiState.asStateFlow() +class ReportProblemViewModel( + private val mullvadProblemReporter: MullvadProblemReport, + private val problemReportRepository: ProblemReportRepository +) : ViewModel() { + + private val showConfirmNoEmail = MutableStateFlow(false) + private val sendingState: MutableStateFlow = MutableStateFlow(null) + + val uiState = + combine( + showConfirmNoEmail, + sendingState, + problemReportRepository.problemReport, + ) { showConfirmNoEmail, pendingState, userReport -> + ReportProblemUiState( + showConfirmNoEmail = showConfirmNoEmail, + sendingState = pendingState, + email = userReport.email ?: "", + description = userReport.description, + ) + } + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(), ReportProblemUiState()) fun sendReport( email: String, @@ -40,11 +61,10 @@ class ReportProblemViewModel(private val mullvadProblemReporter: MullvadProblemR val userEmail = email.trim() val nullableEmail = if (email.isEmpty()) null else userEmail if (shouldShowConfirmNoEmail(nullableEmail)) { - _uiState.update { it.copy(showConfirmNoEmail = true) } + showConfirmNoEmail.tryEmit(true) } else { - _uiState.update { - it.copy(sendingState = SendingReportUiState.Sending, showConfirmNoEmail = false) - } + sendingState.tryEmit(SendingReportUiState.Sending) + showConfirmNoEmail.tryEmit(false) // Ensure we show loading for at least MINIMUM_LOADING_TIME_MILLIS val deferredResult = async { @@ -52,19 +72,31 @@ class ReportProblemViewModel(private val mullvadProblemReporter: MullvadProblemR } delay(MINIMUM_LOADING_TIME_MILLIS) - _uiState.update { - it.copy(sendingState = deferredResult.await().toUiResult(nullableEmail)) + val result = deferredResult.await() + // Clear saved problem report if report was sent successfully + if (result is SendProblemReportResult.Success) { + problemReportRepository.setEmail("") + problemReportRepository.setDescription("") } + sendingState.tryEmit(deferredResult.await().toUiResult(nullableEmail)) } } } fun clearSendResult() { - _uiState.update { it.copy(sendingState = null) } + sendingState.tryEmit(null) } fun dismissConfirmNoEmail() { - _uiState.update { it.copy(showConfirmNoEmail = false) } + showConfirmNoEmail.tryEmit(false) + } + + fun updateEmail(email: String) { + problemReportRepository.setEmail(email) + } + + fun updateDescription(description: String) { + problemReportRepository.setDescription(description) } private fun shouldShowConfirmNoEmail(userEmail: String?): Boolean = From 72afbbfb6f8e8fca71b9a5190c1686861ff8fe8b Mon Sep 17 00:00:00 2001 From: Jonatan Rhodin Date: Fri, 17 Nov 2023 13:14:58 +0100 Subject: [PATCH 2/3] Add secure view to problem report success --- .../mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt index 219bc6b8d3e1..0621c7ebcd37 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt @@ -14,9 +14,7 @@ import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.material3.TextField import androidx.compose.runtime.Composable -import androidx.compose.runtime.getValue import androidx.compose.runtime.remember -import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color @@ -35,6 +33,7 @@ import net.mullvad.mullvadvpn.compose.component.NavigateBackIconButton import net.mullvad.mullvadvpn.compose.component.ScaffoldWithMediumTopBar import net.mullvad.mullvadvpn.compose.dialog.ReportProblemNoEmailDialog import net.mullvad.mullvadvpn.compose.textfield.mullvadWhiteTextFieldColors +import net.mullvad.mullvadvpn.compose.util.SecureScreenWhileInView import net.mullvad.mullvadvpn.dataproxy.SendProblemReportResult import net.mullvad.mullvadvpn.lib.theme.AppTheme import net.mullvad.mullvadvpn.lib.theme.Dimens @@ -193,6 +192,7 @@ private fun ColumnScope.SendingContent() { @Composable private fun ColumnScope.SentContent(sendingState: SendingReportUiState.Success) { + SecureScreenWhileInView() Icon( painter = painterResource(id = R.drawable.icon_success), contentDescription = stringResource(id = R.string.sent), From a3502f553d2e82e72a9dec93c3dcc0275253bc04 Mon Sep 17 00:00:00 2001 From: Jonatan Rhodin Date: Fri, 17 Nov 2023 13:30:06 +0100 Subject: [PATCH 3/3] Fix and add tests in ReportProblemModelTest --- .../viewmodel/ReportProblemModelTest.kt | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt b/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt index 4f91189d0a42..e0b1c5274c5e 100644 --- a/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt +++ b/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt @@ -5,12 +5,16 @@ import app.cash.turbine.test import io.mockk.MockKAnnotations import io.mockk.coEvery import io.mockk.impl.annotations.MockK +import io.mockk.verify import kotlin.test.assertEquals import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest import net.mullvad.mullvadvpn.dataproxy.MullvadProblemReport import net.mullvad.mullvadvpn.dataproxy.SendProblemReportResult +import net.mullvad.mullvadvpn.dataproxy.UserReport import net.mullvad.mullvadvpn.lib.common.test.TestCoroutineRule +import net.mullvad.mullvadvpn.repository.ProblemReportRepository import org.junit.After import org.junit.Before import org.junit.Rule @@ -20,6 +24,9 @@ class ReportProblemModelTest { @get:Rule val testCoroutineRule = TestCoroutineRule() @MockK private lateinit var mockMullvadProblemReport: MullvadProblemReport + @MockK(relaxed = true) private lateinit var mockProblemReportRepository: ProblemReportRepository + + private val problemReportFlow = MutableStateFlow(UserReport("", "")) private lateinit var viewModel: ReportProblemViewModel @@ -27,7 +34,8 @@ class ReportProblemModelTest { fun setUp() { MockKAnnotations.init(this) coEvery { mockMullvadProblemReport.collectLogs() } returns true - viewModel = ReportProblemViewModel(mockMullvadProblemReport) + coEvery { mockProblemReportRepository.problemReport } returns problemReportFlow + viewModel = ReportProblemViewModel(mockMullvadProblemReport, mockProblemReportRepository) } @After @@ -114,4 +122,43 @@ class ReportProblemModelTest { ) } } + + @Test + fun testUpdateEmail() = runTest { + // Arrange + val email = "my@email.com" + + // Act + viewModel.updateEmail(email) + + // Assert + verify { mockProblemReportRepository.setEmail(email) } + } + + @Test + fun testUpdateDescription() = runTest { + // Arrange + val description = "My description" + + // Act + viewModel.updateDescription(description) + + // Assert + verify { mockProblemReportRepository.setDescription(description) } + } + + @Test + fun testUpdateProblemReport() = runTest { + // Arrange + val userReport = UserReport("my@email.com", "My description") + + // Act, Assert + viewModel.uiState.test { + awaitItem() + problemReportFlow.value = userReport + val result = awaitItem() + assertEquals(userReport.email, result.email) + assertEquals(userReport.description, result.description) + } + } }