feat: add ooni test descriptors. #275

aanorbel · 2024-11-18T12:43:51Z

Known issues

Experimental description

sdsantos

I feel we should remove Defaults descriptors all together. If we don't have them anymore, we don't need the extra complexity. That means:

No need to keep both Descriptor and InstalledTestDescriptorModel classes
Descriptor.Source is not needed
DefaultTestDescriptor is not needed
GetDefaultTestDescriptors is not needed
Only isDefault() method is needed, the isInstalledNonDefaultDescriptor() should just be !isDefault().

That should simplify a lot of the existing code.

sdsantos · 2024-12-03T15:05:06Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt


+    val isDefaultTestDescriptor get() = id.value in 10470..10474 // TODO(aanorbel): switch to OONI reserved namespace
+
+    val key get() = if (isDefaultTestDescriptor) {


This is more like a settings key now no? Just to make it clear why we still need this. Because for all other purposes now we can use the ID.

sdsantos · 2024-12-03T15:27:27Z

I feel we should remove Defaults descriptors all together. If we don't have them anymore, we don't need the extra complexity. That means:

No need to keep both Descriptor and InstalledTestDescriptorModel classes

Descriptor.Source is not needed

DefaultTestDescriptor is not needed

GetDefaultTestDescriptors is not needed

Only isDefault() method is needed, the isInstalledNonDefaultDescriptor() should just be !isDefault().

That should simplify a lot of the existing code.

Maybe we can keep both Descriptor and InstalledTestDescriptorModel. It may be useful for the updateStatus for example. But there's no need to duplicate fields, we can just have the installed model as a field of the descriptor.

sdsantos

I found a bug. When you just have the OONI descriptors, and no custom descriptor installed, the pull-to-refresh on the Dashboard screen doesn't seem to work. The refresh icon just says at the top without animation.

sdsantos · 2024-12-04T10:25:53Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/RunSpecification.kt

-                    }
-            }
+        companion object {
+            fun fromDescriptor(descriptor: Descriptor) = descriptor.source.id


This companion object isn't needed, it should return an object of the type Test, or it should be called something like sourceFromDescriptor. It's weird that a static factory method only returns a specific field inside its class.

sdsantos · 2024-12-04T10:27:38Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/RunSpecification.kt

    @Serializable
    data class Test(
-        val source: Source,
+        val source: InstalledTestDescriptorModel.Id? = null,


Maybe call this sourceId or descriptorId to avoid confusion?

And this should never be null right?

sdsantos · 2024-12-04T10:29:00Z

composeApp/src/commonMain/kotlin/org/ooni/probe/ui/choosewebsites/ChooseWebsitesViewModel.kt

                    RunSpecification.Full(
                        tests = listOf(
                            RunSpecification.Test(
-                                source = RunSpecification.Test.Source.Default("websites"),


We should fetch it from the DB, or hardcode the websites descriptor ID somewhere.

Now we have OoniTests.Websites.

sdsantos · 2024-12-04T10:30:53Z

composeApp/src/dwMain/kotlin/org/ooni/probe/config/OrganizationSettings.kt

Is this is empty, it can be removed no?

sdsantos · 2024-12-04T14:45:44Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt


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

+    val isDefaultTestDescriptor get() = id.value in 10470..10474 // TODO(aanorbel): switch to OONI reserved namespace


We could also check if they are in OoniTest now.

aanorbel · 2024-12-09T22:24:11Z

This will be done eventually but converting to draft for now. https://openobservatory.slack.com/archives/GKGRFHXT7/p1733782481912289?thread_ts=1733329764.355889&cid=GKGRFHXT7

sdsantos

We should also migrate the ResultModel to stop having descriptorName and descriptorKey, and just have descriptorKey. That will simplify the SQL queries we have that filter by descriptor.

sdsantos · 2025-09-30T11:10:02Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/Descriptor.kt

    val netTests: List<NetTest>,
    val longRunningTests: List<NetTest> = emptyList(),
-    val source: Source,
+    val source: InstalledTestDescriptorModel,


I had mentioned this before, but keeping both the Descriptor and the InstalledTestDescriptorModel is just a big duplication of fields now. If we are to keep both because it helps with UpdateStatus, then we should remove the duplication:

The Descriptor class should just have the source and UpdateStatus

Optionally, InstalledTestDescriptorModel could be renamed to Descriptor and Descriptor could become a DescriptorItem or DescriptorWithUpdateStatus

sdsantos · 2025-09-30T11:11:33Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt

+    WEBSITES(10470L, "websites"),
+    INSTANT_MESSAGING(10471L, "instant_messaging"),
+    CIRCUMVENTION(10472L, "circumvention"),
+    PERFORMANCE(10473L, "performance"),
+    EXPERIMENTAL(10474L, "experimental"),


We're using regular capitalization for enum and sealed classes in this project: WEBSITES -> Websites.

sdsantos · 2025-09-30T11:13:26Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt

+
+        fun fromId(id: Long) = map[id]
+
+        fun isValidId(id: Long): Boolean = entries.any { it.id == id }


Since we're already caching the entries as a map, this could be fromId(id) != null.

sdsantos · 2025-09-30T11:13:40Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt

 import org.ooni.probe.shared.stringMonthArrayResource
 import org.ooni.probe.shared.toEpoch

+enum class OoniTest(


This should be in a separate file, there's no need to be here.

sdsantos · 2025-09-30T11:15:57Z

composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/InstalledTestDescriptorModel.kt

        val revision: Long,
    )

+    val isDefaultTestDescriptor get() = OoniTest.isValidId(id.value.toLongOrNull() ?: -1L)


Not sure if we should keep with the default concept or not. It's confusing with the NewsMediaScan descriptors, which are default, but not OONI tests. Maybe split this between isOoniDescriptor and isUninstallable.

sdsantos · 2025-09-30T11:29:00Z

composeApp/src/commonMain/kotlin/org/ooni/probe/ui/choosewebsites/ChooseWebsitesViewModel.kt

                    RunSpecification.Full(
                        tests = listOf(
                            RunSpecification.Test(
-                                source = RunSpecification.Test.Source.Default("websites"),


Now we have OoniTests.Websites.

sdsantos · 2025-09-30T11:30:35Z

composeApp/src/commonMain/kotlin/org/ooni/probe/domain/GetBootstrapTestDescriptors.kt

 import kotlin.coroutines.CoroutineContext

 class GetBootstrapTestDescriptors(
    private val readAssetFile: (String) -> String,


Not from the PR, but this is not needed.

sdsantos · 2025-09-30T11:32:37Z

composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorViewModel.kt

                val descriptor = state.value.descriptor ?: return@onEach
-                if (descriptor.source !is Descriptor.Source.Installed) return@onEach
-                setAutoUpdate(descriptor.source.value.id, it.value)
+                if (descriptor.source == null) return@onEach


Source can never be null.

sdsantos · 2025-09-30T11:35:05Z

composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/DescriptorViewModel.kt

                else -> ToggleableState.Indeterminate
            }
-        val isRefreshEnabled get() = descriptor?.source is Descriptor.Source.Installed
+        val isRefreshEnabled get() = descriptor?.source != null


Now it's always enabled as soon as descriptor != null no?

sdsantos · 2025-09-30T11:35:20Z

composeApp/src/commonTest/kotlin/org/ooni/probe/data/repositories/MeasurementRepositoryTest.kt

        runTest {
            val descriptor = DescriptorFactory.buildDescriptorWithInstalled()
-            val installedDescriptor = (descriptor.source as Descriptor.Source.Installed).value
+            val installedDescriptor = descriptor.source!!


!! not needed.

aanorbel force-pushed the issues/174 branch from 6687847 to caca071 Compare November 18, 2024 12:44

aanorbel marked this pull request as draft November 18, 2024 12:45

aanorbel force-pushed the issues/174 branch from 24ef5ca to 70aa880 Compare November 25, 2024 11:38

aanorbel requested a review from sdsantos December 3, 2024 10:51

aanorbel marked this pull request as ready for review December 3, 2024 10:51

sdsantos requested changes Dec 3, 2024

View reviewed changes

aanorbel requested a review from sdsantos December 4, 2024 07:22

sdsantos requested changes Dec 4, 2024

View reviewed changes

aanorbel requested a review from sdsantos December 4, 2024 12:14

sdsantos approved these changes Dec 4, 2024

View reviewed changes

aanorbel marked this pull request as draft December 9, 2024 22:24

hellais added funder/otffoss2025 funder/otffoss2023-2024 and removed funder/otffoss2023-2024 labels Jan 13, 2025

sdsantos closed this Mar 5, 2025

sdsantos deleted the issues/174 branch March 5, 2025 12:57

sdsantos restored the issues/174 branch March 5, 2025 12:57

sdsantos reopened this Mar 5, 2025

aanorbel force-pushed the issues/174 branch from 1323b3b to 700a097 Compare September 23, 2025 18:42

feat: add ooni test descriptors

196832a

aanorbel force-pushed the issues/174 branch from 700a097 to 196832a Compare September 23, 2025 18:44

chore: cleanup unrequired classes

66d0d6a

aanorbel requested a review from sdsantos September 29, 2025 08:58

aanorbel marked this pull request as ready for review September 29, 2025 08:58

sdsantos requested changes Sep 30, 2025

View reviewed changes

cleanup for default descriptors

e4ecd82


		val isDefaultTestDescriptor get() = id.value in 10470..10474 // TODO(aanorbel): switch to OONI reserved namespace

		val key get() = if (isDefaultTestDescriptor) {


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

		val isDefaultTestDescriptor get() = id.value in 10470..10474 // TODO(aanorbel): switch to OONI reserved namespace


		fun fromId(id: Long) = map[id]

		fun isValidId(id: Long): Boolean = entries.any { it.id == id }

feat: add ooni test descriptors. #275

Are you sure you want to change the base?

feat: add ooni test descriptors. #275

Uh oh!

Conversation

aanorbel commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Known issues

Uh oh!

sdsantos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdsantos commented Dec 3, 2024

Uh oh!

sdsantos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanorbel commented Dec 9, 2024

Uh oh!

sdsantos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aanorbel commented Nov 18, 2024 •

edited

Loading