Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem reports not being kept correctly #5457

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +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.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
import androidx.compose.ui.graphics.Color
Expand All @@ -37,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
Expand Down Expand Up @@ -94,16 +91,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) }
)
}

Expand All @@ -124,7 +120,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
Expand All @@ -146,8 +145,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)) },
Expand All @@ -156,8 +155,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()
)
Expand All @@ -169,8 +168,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)
)
}
Expand All @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Expand Down Expand Up @@ -49,7 +49,7 @@ class MullvadProblemReport(context: Context, val dispatcher: CoroutineDispatcher
withContext(dispatcher) {
sendProblemReport(
userReport.email ?: "",
userReport.message,
userReport.description,
logsPath.absolutePath,
cacheDirectory.absolutePath
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -116,6 +117,8 @@ val uiModule = module {
}
}

single { ProblemReportRepository() }

// View models
viewModel { AccountViewModel(get(), get(), get(), get()) }
viewModel {
Expand All @@ -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()) }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<UserReport> = _problemReport.asStateFlow()

fun setEmail(email: String) {
_problemReport.value = _problemReport.value.copy(email = email)
}

fun setDescription(description: String) {
_problemReport.value = _problemReport.value.copy(description = description)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<SendingReportUiState?> = 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,
Expand All @@ -40,31 +61,42 @@ 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 {
mullvadProblemReporter.sendReport(UserReport(nullableEmail, description))
}
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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,14 +24,18 @@ 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

@Before
fun setUp() {
MockKAnnotations.init(this)
coEvery { mockMullvadProblemReport.collectLogs() } returns true
viewModel = ReportProblemViewModel(mockMullvadProblemReport)
coEvery { mockProblemReportRepository.problemReport } returns problemReportFlow
viewModel = ReportProblemViewModel(mockMullvadProblemReport, mockProblemReportRepository)
}

@After
Expand Down Expand Up @@ -114,4 +122,43 @@ class ReportProblemModelTest {
)
}
}

@Test
fun testUpdateEmail() = runTest {
// Arrange
val email = "[email protected]"

// 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("[email protected]", "My description")

// Act, Assert
viewModel.uiState.test {
awaitItem()
problemReportFlow.value = userReport
val result = awaitItem()
assertEquals(userReport.email, result.email)
assertEquals(userReport.description, result.description)
}
}
}
Loading