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

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

wants to merge 5 commits into from

Conversation

aanorbel
Copy link
Member

Closes #341

Comment on lines 65 to 71
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?

@@ -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?

Copy link
Collaborator

@sdsantos sdsantos left a comment

Choose a reason for hiding this comment

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

I missed a couple of things on my first review. Left some comments. Also, the counts summary is not visible for Experimental, and for Performance it's a totally different thing (#343).

@@ -2,6 +2,10 @@ package org.ooni.probe.data.models

import kotlin.time.Duration.Companion.seconds

data class ResultCount(val total: Long, val succeeded: Long) {
val failed get() = total - succeeded
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realised that we're mixing failed and anomalies here.
Blocked = isAnomaly
Failed = isFailed

We shouldn't be counting failures inside the blocked count.

Comment on lines +63 to +65
<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>
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Result: Show success, anomaly and failed counts on the summary card
2 participants