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.

Sign up for GitHub

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

feat: add counts to summary cards #352

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: add counts to summary cards #352

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd8e33a
feat: add counts to summary cards
aanorbel Dec 12, 2024
0cb840b
chore: correct tests
aanorbel Dec 13, 2024
ed18a39
fix based on review
aanorbel Dec 18, 2024
ac61165
fix based on review
aanorbel Dec 18, 2024
52f319e
fix: tests
aanorbel Dec 18, 2024
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
6 changes: 6 additions & 0 deletions composeApp/src/commonMain/composeResources/values/strings-common.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@
<string name="TestResults_NotAvailable">N/A</string>
<string name="TestResults_Summary_Performance_Hero_Upload">Upload</string>
<string name="TestResults_Summary_Performance_Hero_Download">Download</string>


<string name="TestResults_Summary_Websites_Hero_Tested_Singular">Tested</string>
<string name="TestResults_Summary_Websites_Hero_Blocked_Singular">Blocked</string>
<string name="TestResults_Summary_Websites_Hero_Reachable_Singular">Accessible</string>
Comment on lines +63 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are different strings for circunvention, websites and instant messaging

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its thesame string. I doubt we want to keep 12+ strings saying similar things.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about other languages, but at least in English, Instant Messaging uses accessible and Circumvention uses available.


<string name="Modal_ReRun_Title">Re-run test</string>
<string name="Modal_ReRun_Websites_Title">You are about to re-test %1$s websites.</string>
<string name="Modal_ReRun_Websites_Run">Run</string>
Expand Down
3 changes: 3 additions & 0 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/data/models/ResultItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ package org.ooni.probe.data.models

import kotlin.time.Duration.Companion.seconds

data class ResultCount(val total: Long, val failed: Long?, val succeeded: Long?)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why could failed and succeeded be null?


data class ResultItem(
val result: ResultModel,
val descriptor: Descriptor,
val network: NetworkModel?,
val measurements: List<MeasurementWithUrl>,
val measurementCounts: ResultCount,
) {
val anyMeasurementMissingUpload =
result.isDone && measurements.any { it.measurement.isDoneAndMissingUpload }
Expand Down
8 changes: 8 additions & 0 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/data/repositories/MeasurementRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.ooni.probe.data.repositories

import app.cash.sqldelight.coroutines.asFlow
import app.cash.sqldelight.coroutines.mapToList
import app.cash.sqldelight.coroutines.mapToOneNotNull
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.withContext
Expand All @@ -13,6 +14,7 @@ import org.ooni.probe.data.Url
import org.ooni.probe.data.models.InstalledTestDescriptorModel
import org.ooni.probe.data.models.MeasurementModel
import org.ooni.probe.data.models.MeasurementWithUrl
import org.ooni.probe.data.models.ResultCount
import org.ooni.probe.data.models.ResultModel
import org.ooni.probe.data.models.UrlModel
import org.ooni.probe.shared.toEpoch
Expand Down Expand Up @@ -54,6 +56,12 @@ class MeasurementRepository(
.mapToList(backgroundContext)
.map { list -> list.mapNotNull { it.toModel() } }

fun countByResultId(resultId: ResultModel.Id): Flow<ResultCount> {
return database.measurementQueries.countByResultId(resultId.value).asFlow()
.mapToOneNotNull(backgroundContext)
.map { ResultCount(it.total_measurements, it.failed_measurements, it.ok_measurements) }
}

suspend fun createOrUpdate(model: MeasurementModel): MeasurementModel.Id =
withContext(backgroundContext) {
database.transactionWithResult {
Expand Down
1 change: 1 addition & 0 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ class Dependencies(
getResultById = resultRepository::getById,
getTestDescriptors = getTestDescriptors.invoke(),
getMeasurementsByResultId = measurementRepository::listByResultId,
countByResultId = measurementRepository::countByResultId,
)
}
private val clearStorage by lazy {
Expand Down
6 changes: 5 additions & 1 deletion composeApp/src/commonMain/kotlin/org/ooni/probe/domain/GetResult.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,30 @@ import kotlinx.coroutines.flow.combine
import org.ooni.probe.data.models.Descriptor
import org.ooni.probe.data.models.MeasurementWithUrl
import org.ooni.probe.data.models.NetworkModel
import org.ooni.probe.data.models.ResultCount
import org.ooni.probe.data.models.ResultItem
import org.ooni.probe.data.models.ResultModel

class GetResult(
private val getResultById: (ResultModel.Id) -> Flow<Pair<ResultModel, NetworkModel?>?>,
private val getTestDescriptors: Flow<List<Descriptor>>,
private val getMeasurementsByResultId: (ResultModel.Id) -> Flow<List<MeasurementWithUrl>>,
private val countByResultId: (ResultModel.Id) -> Flow<ResultCount>,
) {
operator fun invoke(resultId: ResultModel.Id): Flow<ResultItem?> =
combine(
getResultById(resultId),
getTestDescriptors,
getMeasurementsByResultId(resultId),
) { resultWithNetwork, descriptors, measurements ->
countByResultId(resultId),
) { resultWithNetwork, descriptors, measurements, measurementCounts ->
val result = resultWithNetwork?.first ?: return@combine null
ResultItem(
result = result,
descriptor = descriptors.forResult(result) ?: return@combine null,
network = resultWithNetwork.second,
measurements = measurements,
measurementCounts = measurementCounts,
)
}
}
85 changes: 71 additions & 14 deletions composeApp/src/commonMain/kotlin/org/ooni/probe/ui/result/ResultScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.IntrinsicSize
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.WindowInsets
import androidx.compose.foundation.layout.asPaddingValues
import androidx.compose.foundation.layout.defaultMinSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.navigationBars
import androidx.compose.foundation.layout.offset
import androidx.compose.foundation.layout.padding
Expand All @@ -30,6 +32,7 @@ import androidx.compose.material3.Surface
import androidx.compose.material3.Text
import androidx.compose.material3.TextButton
import androidx.compose.material3.TopAppBarDefaults
import androidx.compose.material3.VerticalDivider
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
Expand All @@ -50,7 +53,6 @@ import ooniprobe.composeapp.generated.resources.NetworkType_Vpn
import ooniprobe.composeapp.generated.resources.Res
import ooniprobe.composeapp.generated.resources.TestResults_NotAvailable
import ooniprobe.composeapp.generated.resources.TestResults_Overview_Hero_DataUsage
import ooniprobe.composeapp.generated.resources.TestResults_Overview_Hero_Tests
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Hero_Country
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Hero_DateAndTime
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Hero_Mobile
Expand All @@ -60,6 +62,9 @@ import ooniprobe.composeapp.generated.resources.TestResults_Summary_Hero_Runtime
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Hero_WiFi
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Performance_Hero_Download
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Performance_Hero_Upload
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Websites_Hero_Blocked_Singular
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Websites_Hero_Reachable_Singular
import ooniprobe.composeapp.generated.resources.TestResults_Summary_Websites_Hero_Tested_Singular
import ooniprobe.composeapp.generated.resources.ic_download
import ooniprobe.composeapp.generated.resources.ic_replay
import ooniprobe.composeapp.generated.resources.ic_upload
Expand Down Expand Up @@ -166,7 +171,7 @@ fun ResultScreen(

@Composable
private fun Summary(item: ResultItem) {
val pagerState = rememberPagerState(pageCount = { 2 })
val pagerState = rememberPagerState(pageCount = { 3 })
Box {
HorizontalPager(
state = pagerState,
Expand All @@ -176,8 +181,9 @@ private fun Summary(item: ResultItem) {
.defaultMinSize(minHeight = 128.dp),
) { page ->
when (page) {
0 -> SummaryDetails(item)
1 -> SummaryNetwork(item)
0 -> SummaryStats(item)
1 -> SummaryDetails(item)
2 -> SummaryNetwork(item)
}
}
Row(
Expand Down Expand Up @@ -210,21 +216,72 @@ private fun Summary(item: ResultItem) {
}

@Composable
private fun SummaryDetails(item: ResultItem) {
Column(Modifier.padding(horizontal = 16.dp)) {
val labelModifier = Modifier.weight(4f)
val valueModifier = Modifier.weight(6f)
Row {
private fun SummaryStats(item: ResultItem) {
Row(
modifier = Modifier
.fillMaxWidth()
.height(IntrinsicSize.Min) // So VerticalDividers don't expand to the whole screen
.padding(16.dp),
) {
Column(
modifier = Modifier.weight(1f),
horizontalAlignment = Alignment.CenterHorizontally,
) {
Text(
stringResource(Res.string.TestResults_Overview_Hero_Tests),
fontWeight = FontWeight.Bold,
modifier = labelModifier,
item.measurementCounts.total.toString(),
style = MaterialTheme.typography.headlineMedium,
)

Text(
stringResource(Res.string.TestResults_Summary_Websites_Hero_Tested_Singular),
style = MaterialTheme.typography.labelLarge,
modifier = Modifier.padding(bottom = 8.dp),
)
}

VerticalDivider(Modifier.padding(4.dp), color = LocalContentColor.current)

Column(
modifier = Modifier.weight(1f),
horizontalAlignment = Alignment.CenterHorizontally,
) {
Text(
item.measurements.size.toString(),
modifier = valueModifier,
item.measurementCounts.failed?.toString() ?: "0",
style = MaterialTheme.typography.headlineMedium,
)

Text(
stringResource(Res.string.TestResults_Summary_Websites_Hero_Blocked_Singular),
style = MaterialTheme.typography.labelLarge,
modifier = Modifier.padding(bottom = 8.dp),
)
}

VerticalDivider(Modifier.padding(4.dp), color = LocalContentColor.current)

Column(
modifier = Modifier.weight(1f),
horizontalAlignment = Alignment.CenterHorizontally,
) {
Text(
item.measurementCounts.succeeded?.toString() ?: "0",
style = MaterialTheme.typography.headlineMedium,
)

Text(
stringResource(Res.string.TestResults_Summary_Websites_Hero_Reachable_Singular),
style = MaterialTheme.typography.labelLarge,
modifier = Modifier.padding(bottom = 8.dp),
)
}
}
}

@Composable
private fun SummaryDetails(item: ResultItem) {
Column(Modifier.padding(horizontal = 16.dp)) {
val labelModifier = Modifier.weight(4f)
val valueModifier = Modifier.weight(6f)
Row {
Text(
stringResource(Res.string.TestResults_Summary_Hero_DateAndTime),
Expand Down
8 changes: 8 additions & 0 deletions composeApp/src/commonMain/sqldelight/org/ooni/probe/data/Measurement.sq
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ SELECT * FROM Measurement
LEFT JOIN Url ON Measurement.url_id = Url.id
WHERE Measurement.result_id = ?;

countByResultId:
SELECT
COUNT(*) AS total_measurements,
SUM(CASE WHEN is_rerun = 0 AND is_done = 1 AND is_failed = 0 AND is_anomaly = 0 THEN 1 ELSE 0 END) AS ok_measurements,
SUM(CASE WHEN is_rerun = 0 AND is_done = 1 AND is_failed = 1 THEN 1 ELSE 0 END) AS failed_measurements
FROM Measurement
WHERE result_id = ?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the extra DB query? Can't we just get this values from the whole list?


selectAllNotUploaded:
SELECT * FROM Measurement
WHERE Measurement.is_done = 1
Expand Down
4 changes: 4 additions & 0 deletions composeApp/src/commonTest/kotlin/org/ooni/probe/ui/result/ResultScreenTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import ooniprobe.composeapp.generated.resources.Modal_ReRun_Websites_Run
import ooniprobe.composeapp.generated.resources.Res
import org.jetbrains.compose.resources.getString
import org.ooni.probe.data.models.MeasurementModel
import org.ooni.probe.data.models.ResultCount
import org.ooni.probe.data.models.ResultItem
import org.ooni.testing.factories.DescriptorFactory
import org.ooni.testing.factories.MeasurementModelFactory
Expand All @@ -26,6 +27,7 @@ class ResultScreenTest {
network = null,
descriptor = DescriptorFactory.buildDescriptorWithInstalled(),
measurements = emptyList(),
measurementCounts = ResultCount(0, 0, 0),
)
var title: String? = null
setContent {
Expand Down Expand Up @@ -55,6 +57,7 @@ class ResultScreenTest {
),
),
),
measurementCounts = ResultCount(0, 0, 0),
)
setContent {
ResultScreen(
Expand All @@ -80,6 +83,7 @@ class ResultScreenTest {
network = null,
descriptor = DescriptorFactory.buildDescriptorWithInstalled(),
measurements = emptyList(),
measurementCounts = ResultCount(0, 0, 0),
)
setContent {
ResultScreen(
Expand Down
2 changes: 2 additions & 0 deletions composeApp/src/commonTest/kotlin/org/ooni/probe/ui/result/ResultViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.runTest
import org.ooni.probe.data.models.MeasurementModel
import org.ooni.probe.data.models.ResultCount
import org.ooni.probe.data.models.ResultItem
import org.ooni.probe.data.models.ResultModel
import org.ooni.testing.factories.DescriptorFactory
Expand All @@ -32,6 +33,7 @@ class ResultViewModelTest {
network = null,
descriptor = DescriptorFactory.buildDescriptorWithInstalled(),
measurements = emptyList(),
measurementCounts = ResultCount(0, 0, 0),
)
val viewModel = buildViewModel(getResult = { flowOf(item) })

Expand Down