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

Filter list_measurement by versions #703

Merged
merged 6 commits into from
Sep 7, 2023
Merged

Conversation

FedericoCeratto
Copy link
Contributor

@FedericoCeratto FedericoCeratto commented Aug 15, 2023

Adds filters for software_version, test_version, engine_version to list measurements
https://ams-pg-test.ooni.org/apidocs/#/default/get_api_v1_measurements
Related to #615

@FedericoCeratto FedericoCeratto marked this pull request as ready for review August 16, 2023 09:27
@FedericoCeratto
Copy link
Contributor Author

1.0.64~pr703-116 deployed on -test

@FedericoCeratto FedericoCeratto force-pushed the filter_by_versions branch 2 times, most recently from e296330 to cb2a372 Compare August 30, 2023 12:53
description: |
Will be true for confirmed network anomalies (we found a blockpage, a middlebox was found, the IM app is blocked, etc.).
Set "true" for confirmed network anomalies (we found a blockpage, a middlebox, etc.).
Default: both true and false
Copy link
Contributor

Choose a reason for hiding this comment

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

How can the default be true and false at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the value is not set it's handled as None and that generates an SQL query without filter.

Copy link
Contributor

@bassosimone bassosimone Aug 31, 2023

Choose a reason for hiding this comment

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

Thanks for explaining! I still think the docs are a bit confusing. Maybe there is a better way to express this concept? Such as "Default: we do not filter the results depending on their confirmed status".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -773,6 +773,34 @@ def test_list_measurements_ZZ(client):
assert resp.status_code == 403


def test_list_measurements_filter_software_version_nomatch(client):
# https://github.com/ooni/backend/issues/15
url = "measurements?since=2021-07-09&until=2021-07-10&software_version=9.9.9"
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the functionality is advertised as a comma separated list, can all the tests here be using the comma separated version of the parameter rather than using just a scalar? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new tests, thanks!

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd double check whether the issue URL you're using is correct, thanks!

🙏 🐳

api/tests/integ/test_integration.py Outdated Show resolved Hide resolved
@FedericoCeratto FedericoCeratto merged commit 462542a into master Sep 7, 2023
4 of 5 checks passed
@FedericoCeratto FedericoCeratto deleted the filter_by_versions branch September 7, 2023 14:25
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.

2 participants