Merge pull request #388 from ooni/fix-review-bugs

Fix review descriptor bugs
ooni · Jan 20, 2025 · 336fe3b · 336fe3b
2 parents fb11601 + e7e8d14
commit 336fe3b
Show file tree

Hide file tree

Showing 11 changed files with 76 additions and 49 deletions.
diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/Descriptor.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/Descriptor.kt
@@ -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

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt
@@ -291,7 +291,7 @@ class Dependencies(
             getDefaultTestDescriptors = getDefaultTestDescriptors::invoke,
             listAllInstalledTestDescriptors = testDescriptorRepository::listAll,
             listLatestInstalledTestDescriptors = testDescriptorRepository::listLatest,
-            descriptorUpdates = getDescriptorUpdate::observeAvailableUpdatesState,
+            descriptorUpdates = getDescriptorUpdate::observeStatus,
             getPreferenceValues = preferenceRepository::allSettings,
         )
     }
@@ -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,
     )
@@ -449,7 +449,7 @@ class Dependencies(
         fetchDescriptorUpdate = fetchDescriptorUpdate,
         setAutoUpdate = testDescriptorRepository::setAutoUpdate,
         reviewUpdates = getDescriptorUpdate::reviewUpdates,
-        descriptorUpdates = getDescriptorUpdate::observeAvailableUpdatesState,
+        descriptorUpdates = getDescriptorUpdate::observeStatus,
     )
 
     fun logViewModel(onBack: () -> Unit) =
@@ -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,
         )
     }
 

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchDescriptorUpdate.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/domain/FetchDescriptorUpdate.kt
@@ -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()
         }
@@ -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,
@@ -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,
@@ -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 {

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/DashboardScreen.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/DashboardScreen.kt
@@ -112,6 +112,9 @@ fun DashboardScreen(
                             onClick = {
                                 onEvent(DashboardViewModel.Event.DescriptorClicked(descriptor))
                             },
+                            onUpdateClick = {
+                                onEvent(DashboardViewModel.Event.UpdateDescriptorClicked(descriptor))
+                            },
                         )
                     }
                 }

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/DashboardViewModel.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/DashboardViewModel.kt
@@ -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 {
@@ -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

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/TestDescriptorItem.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/dashboard/TestDescriptorItem.kt
@@ -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,
@@ -55,7 +55,7 @@ fun TestDescriptorItem(
             }
         }
         if (descriptor.updatable) {
-            UpdatesChip(onClick = updateDescriptor)
+            UpdatesChip(onClick = onUpdateClick)
         }
         if (descriptor.isExpired) {
             ExpiredChip()

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorScreen.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorScreen.kt
@@ -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
@@ -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(

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorViewModel.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorViewModel.kt
@@ -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
@@ -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>,
@@ -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)
 
@@ -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))
@@ -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)
@@ -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,

diff --git a/...seApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/review/ReviewUpdatesViewModel.kt b/...seApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/review/ReviewUpdatesViewModel.kt
@@ -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)
 
@@ -52,6 +53,7 @@ class ReviewUpdatesViewModel(
                             listOf(descriptor),
                             SaveTestDescriptors.Mode.CreateOrUpdate,
                         )
+                        markAsUpdated(listOf(descriptor))
                         navigateToNextItemOrClose(it.index)
                     } else {
                         onBack()

diff --git a/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/shared/UpdatesChip.kt b/composeApp/src/commonMain/kotlin/org/ooni/probe/ui/shared/UpdatesChip.kt
@@ -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
@@ -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,

diff --git a/composeApp/src/commonTest/kotlin/org/ooni/probe/domain/FetchDescriptorUpdateTest.kt b/composeApp/src/commonTest/kotlin/org/ooni/probe/domain/FetchDescriptorUpdateTest.kt
@@ -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)
@@ -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)
@@ -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)
@@ -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)