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 review descriptor bugs #388

Open
wants to merge 1 commit into
base: keep-old-descriptors
Choose a base branch
from
Open
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 @@ -34,12 +34,19 @@ data class Descriptor(

val isExpired get() = expirationDate != null && expirationDate < LocalDateTime.now()

val updatable get() = updateStatus is UpdateStatus.UpdateRejected
val updatable get() = updatedDescriptor != null

val updatedDescriptor
get() = when (updateStatus) {
is UpdateStatus.Updatable -> updateStatus.updatedDescriptor
is UpdateStatus.UpdateRejected -> updateStatus.updatedDescriptor
else -> null
}

val key: String
get() = when (source) {
is Source.Default -> name
is Source.Installed -> source.value.id.value.toString()
is Source.Installed -> source.value.id.value
}

val allTests get() = netTests + longRunningTests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ class Dependencies(
getDefaultTestDescriptors = getDefaultTestDescriptors::invoke,
listAllInstalledTestDescriptors = testDescriptorRepository::listAll,
listLatestInstalledTestDescriptors = testDescriptorRepository::listLatest,
descriptorUpdates = getDescriptorUpdate::observeAvailableUpdatesState,
descriptorUpdates = getDescriptorUpdate::observeStatus,
getPreferenceValues = preferenceRepository::allSettings,
)
}
Expand Down Expand Up @@ -426,7 +426,7 @@ class Dependencies(
observeTestRunErrors = runBackgroundStateManager.observeErrors(),
shouldShowVpnWarning = shouldShowVpnWarning::invoke,
fetchDescriptorUpdate = fetchDescriptorUpdate,
observeAvailableUpdatesState = getDescriptorUpdate::observeAvailableUpdatesState,
observeAvailableUpdatesState = getDescriptorUpdate::observeStatus,
reviewUpdates = getDescriptorUpdate::reviewUpdates,
cancelUpdates = getDescriptorUpdate::cancelUpdates,
)
Expand All @@ -449,7 +449,7 @@ class Dependencies(
fetchDescriptorUpdate = fetchDescriptorUpdate,
setAutoUpdate = testDescriptorRepository::setAutoUpdate,
reviewUpdates = getDescriptorUpdate::reviewUpdates,
descriptorUpdates = getDescriptorUpdate::observeAvailableUpdatesState,
descriptorUpdates = getDescriptorUpdate::observeStatus,
)

fun logViewModel(onBack: () -> Unit) =
Expand Down Expand Up @@ -537,8 +537,9 @@ class Dependencies(
return ReviewUpdatesViewModel(
onBack = onBack,
saveTestDescriptors = saveTestDescriptors::invoke,
observeAvailableUpdatesState = getDescriptorUpdate::observeStatus,
cancelUpdates = getDescriptorUpdate::cancelUpdates,
observeAvailableUpdatesState = getDescriptorUpdate::observeAvailableUpdatesState,
markAsUpdated = getDescriptorUpdate::markAsUpdated,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,22 @@ class FetchDescriptorUpdate(
private val saveTestDescriptors: suspend (List<InstalledTestDescriptorModel>, SaveTestDescriptors.Mode) -> Unit,
private val listInstalledTestDescriptors: () -> Flow<List<InstalledTestDescriptorModel>>,
) {
private val availableUpdates = MutableStateFlow(DescriptorUpdatesStatus())
private val status = MutableStateFlow(DescriptorUpdatesStatus())

fun observeStatus() = status.asStateFlow()

suspend fun invoke(
descriptors: List<InstalledTestDescriptorModel>,
): Map<ResultStatus, MutableList<Result<InstalledTestDescriptorModel?, MkException>>> {
availableUpdates.update { _ ->
status.update { _ ->
DescriptorUpdatesStatus(
refreshType = UpdateStatusType.FetchingUpdates,
)
}
val response = coroutineScope {
descriptors.map { descriptor ->
async {
Pair(descriptor, fetchDescriptor(descriptor.id.value.toString()))
Pair(descriptor, fetchDescriptor(descriptor.id.value))
}
}.awaitAll()
}
Expand Down Expand Up @@ -73,7 +75,7 @@ class FetchDescriptorUpdate(
val autoUpdated: List<InstalledTestDescriptorModel> = resultsMap[ResultStatus.AutoUpdated]?.mapNotNull { result ->
result.get()
}.orEmpty()
availableUpdates.update { _ ->
status.update { _ ->
DescriptorUpdatesStatus(
availableUpdates = updatesAvailable,
autoUpdated = autoUpdated,
Expand All @@ -93,7 +95,7 @@ class FetchDescriptorUpdate(
}

fun cancelUpdates(descriptors: List<InstalledTestDescriptorModel>) {
availableUpdates.update { currentItems ->
status.update { currentItems ->
currentItems.copy(
availableUpdates = currentItems.availableUpdates - descriptors,
rejectedUpdates = currentItems.availableUpdates + descriptors,
Expand All @@ -103,15 +105,23 @@ class FetchDescriptorUpdate(
}

fun reviewUpdates(itemsForReview: List<InstalledTestDescriptorModel>) {
availableUpdates.update { currentItems ->
status.update { currentItems ->
currentItems.copy(
reviewUpdates = itemsForReview,
refreshType = UpdateStatusType.None,
)
}
}

fun observeAvailableUpdatesState() = availableUpdates.asStateFlow()
fun markAsUpdated(items: List<InstalledTestDescriptorModel>) {
status.update { status ->
status.copy(
availableUpdates = status.availableUpdates - items,
rejectedUpdates = status.rejectedUpdates - items,
reviewUpdates = status.reviewUpdates - items,
)
}
}
}

enum class ResultStatus {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ fun DashboardScreen(
onClick = {
onEvent(DashboardViewModel.Event.DescriptorClicked(descriptor))
},
onUpdateClick = {
onEvent(DashboardViewModel.Event.UpdateDescriptorClicked(descriptor))
},
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,19 @@ class DashboardViewModel(
}
.launchIn(viewModelScope)

events
.filterIsInstance<Event.UpdateDescriptorClicked>()
.onEach {
reviewUpdates(
listOf(
(it.descriptor.source as? Descriptor.Source.Installed)?.value
?: return@onEach,
),
)
goToReviewDescriptorUpdates()
}
.launchIn(viewModelScope)

events
.filterIsInstance<Event.CancelUpdatesClicked>()
.onEach {
Expand Down Expand Up @@ -181,6 +194,8 @@ class DashboardViewModel(

data class DescriptorClicked(val descriptor: Descriptor) : Event

data class UpdateDescriptorClicked(val descriptor: Descriptor) : Event

data object FetchUpdatedDescriptors : Event

data object ReviewUpdatesClicked : Event
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import org.ooni.probe.ui.shared.UpdatesChip
fun TestDescriptorItem(
descriptor: Descriptor,
onClick: () -> Unit,
updateDescriptor: () -> Unit = {},
onUpdateClick: () -> Unit,
) {
Row(
verticalAlignment = Alignment.CenterVertically,
Expand Down Expand Up @@ -55,7 +55,7 @@ fun TestDescriptorItem(
}
}
if (descriptor.updatable) {
UpdatesChip(onClick = updateDescriptor)
UpdatesChip(onClick = onUpdateClick)
}
if (descriptor.isExpired) {
ExpiredChip()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import org.ooni.probe.config.OrganizationConfig
import org.ooni.probe.config.TestDisplayMode
import org.ooni.probe.data.models.Descriptor
import org.ooni.probe.data.models.NetTest
import org.ooni.probe.data.models.UpdateStatus
import org.ooni.probe.ui.shared.ExpiredChip
import org.ooni.probe.ui.shared.MarkdownViewer
import org.ooni.probe.ui.shared.SelectableItem
Expand Down Expand Up @@ -261,15 +262,16 @@ private fun DescriptorDetails(
}
}

if (descriptor.updatable) {
UpdatesChip(onClick = { }, modifier = Modifier.padding(top = 8.dp))
}

if (descriptor.isExpired) {
ExpiredChip()
}

state.updatedDescriptor?.let {
if (descriptor.updateStatus is UpdateStatus.UpdateRejected) {
UpdatesChip(
onClick = { onEvent(DescriptorViewModel.Event.UpdateDescriptor) },
modifier = Modifier.padding(top = 8.dp),
)
} else if (descriptor.updateStatus is UpdateStatus.Updatable) {
OutlinedButton(
onClick = { onEvent(DescriptorViewModel.Event.UpdateDescriptor) },
border = ButtonDefaults.outlinedButtonBorder(enabled = true).copy(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import org.ooni.probe.data.models.NetTest
import org.ooni.probe.data.models.ResultModel
import org.ooni.probe.data.models.SettingsKey
import org.ooni.probe.data.models.UpdateStatusType
import org.ooni.probe.data.models.toDescriptor
import org.ooni.probe.data.repositories.PreferenceRepository
import org.ooni.probe.ui.shared.SelectableItem
import kotlin.time.Duration
Expand All @@ -39,8 +38,7 @@ class DescriptorViewModel(
private val preferenceRepository: PreferenceRepository,
private val launchUrl: (String) -> Unit,
deleteTestDescriptor: suspend (InstalledTestDescriptorModel) -> Unit,
fetchDescriptorUpdate:
suspend (List<InstalledTestDescriptorModel>) -> Unit,
fetchDescriptorUpdate: suspend (List<InstalledTestDescriptorModel>) -> Unit,
setAutoUpdate: suspend (InstalledTestDescriptorModel.Id, Boolean) -> Unit,
reviewUpdates: (List<InstalledTestDescriptorModel>) -> Unit,
descriptorUpdates: () -> Flow<DescriptorUpdatesStatus>,
Expand All @@ -62,18 +60,6 @@ class DescriptorViewModel(
},
)
}
if (results.availableUpdates.size == 1) {
results.availableUpdates.first().let { updatedDescriptor ->
if (updatedDescriptor.id.value == descriptorKey) {
_state.update {
it.copy(
updatedDescriptor = updatedDescriptor.toDescriptor(),
refreshType = UpdateStatusType.None,
)
}
}
}
}
}
.launchIn(viewModelScope)

Expand Down Expand Up @@ -178,7 +164,7 @@ class DescriptorViewModel(

if (descriptor.source !is Descriptor.Source.Installed) return@onEach
_state.update {
it.copy(refreshType = UpdateStatusType.UpdateLink, updatedDescriptor = null)
it.copy(refreshType = UpdateStatusType.UpdateLink)
}

fetchDescriptorUpdate(listOf(descriptor.source.value))
Expand All @@ -187,12 +173,11 @@ class DescriptorViewModel(

events.filterIsInstance<Event.UpdateDescriptor>()
.onEach {
val descriptor = state.value.updatedDescriptor ?: return@onEach
if (descriptor.source !is Descriptor.Source.Installed) return@onEach
val newDescriptor = state.value.descriptor?.updatedDescriptor ?: return@onEach
_state.update {
it.copy(refreshType = UpdateStatusType.None, updatedDescriptor = null)
it.copy(refreshType = UpdateStatusType.None)
}
reviewUpdates(listOf(descriptor.source.value))
reviewUpdates(listOf(newDescriptor))
goToReviewDescriptorUpdates()
}
.launchIn(viewModelScope)
Expand Down Expand Up @@ -229,7 +214,6 @@ class DescriptorViewModel(

data class State(
val descriptor: Descriptor? = null,
val updatedDescriptor: Descriptor? = null,
val estimatedTime: Duration? = null,
val tests: List<SelectableItem<NetTest>> = emptyList(),
val lastResult: ResultModel? = null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ import org.ooni.probe.domain.SaveTestDescriptors
class ReviewUpdatesViewModel(
private val onBack: () -> Unit,
saveTestDescriptors: suspend (List<InstalledTestDescriptorModel>, SaveTestDescriptors.Mode) -> Unit,
cancelUpdates: (List<InstalledTestDescriptorModel>) -> Unit,
observeAvailableUpdatesState: () -> Flow<DescriptorUpdatesStatus>,
cancelUpdates: (List<InstalledTestDescriptorModel>) -> Unit,
markAsUpdated: (List<InstalledTestDescriptorModel>) -> Unit,
) : ViewModel() {
private val events = MutableSharedFlow<Event>(extraBufferCapacity = 1)

Expand Down Expand Up @@ -52,6 +53,7 @@ class ReviewUpdatesViewModel(
listOf(descriptor),
SaveTestDescriptors.Mode.CreateOrUpdate,
)
markAsUpdated(listOf(descriptor))
navigateToNextItemOrClose(it.index)
} else {
onBack()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.ooni.probe.ui.shared

import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.LocalContentColor
import androidx.compose.material3.SuggestionChip
import androidx.compose.material3.SuggestionChipDefaults
import androidx.compose.material3.Text
Expand All @@ -17,9 +17,12 @@ fun UpdatesChip(
) {
SuggestionChip(
onClick = onClick,
enabled = false,
colors = SuggestionChipDefaults.suggestionChipColors(
labelColor = MaterialTheme.colorScheme.error,
labelColor = LocalContentColor.current,
),
border = SuggestionChipDefaults.suggestionChipBorder(
enabled = true,
borderColor = LocalContentColor.current,
),
label = { Text(stringResource(Res.string.Dashboard_RunV2_UpdateTag)) },
modifier = modifier,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class FetchDescriptorUpdateTest {

subject()

val state = subject.observeAvailableUpdatesState().value
val state = subject.observeStatus().value

assertEquals(0, state.autoUpdated.size)
assertEquals(UpdateStatusType.None, state.refreshType)
Expand All @@ -56,7 +56,7 @@ class FetchDescriptorUpdateTest {

subject()

val state = subject.observeAvailableUpdatesState().value
val state = subject.observeStatus().value

assertEquals(1, state.autoUpdated.size)
assertEquals(UpdateStatusType.None, state.refreshType)
Expand All @@ -83,7 +83,7 @@ class FetchDescriptorUpdateTest {

subject()

val state = subject.observeAvailableUpdatesState().value
val state = subject.observeStatus().value

assertEquals(1, state.availableUpdates.size)
assertEquals(0, state.reviewUpdates.size)
Expand All @@ -92,7 +92,7 @@ class FetchDescriptorUpdateTest {

subject.reviewUpdates(listOf(newDescriptor))

val reviewState = subject.observeAvailableUpdatesState().value
val reviewState = subject.observeStatus().value

assertEquals(1, reviewState.reviewUpdates.size)
assertEquals(UpdateStatusType.None, reviewState.refreshType)
Expand Down
Loading