-
Notifications
You must be signed in to change notification settings - Fork 0
Add queries to test AOI for validate project #214
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
Conversation
b7ccfbf to
d1c9eae
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #214 +/- ##
===========================================
- Coverage 87.67% 87.27% -0.41%
===========================================
Files 205 205
Lines 12317 12471 +154
Branches 1042 1056 +14
===========================================
+ Hits 10799 10884 +85
- Misses 1137 1196 +59
- Partials 381 391 +10 ☔ View full report in Codecov by Sentry. |
d1c9eae to
63a6dc6
Compare
5f0bef6 to
ad48219
Compare
thenav56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For forwarding the error message from remote, we can add additional attributes to the Exception and use them in the Response Types
| "ohsome element count request failed: check for errors in filter or geometries", | ||
| extra=log_extra_response(response=response), | ||
| ) | ||
| raise ValidateApiCallError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to include the error message in the exception?
Will it be helpful for the users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does, currently we're replicating the behaviour of ohsome request call, we should refactor both of these functions. I'll add appropriate 'fixme' for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add this as a NOTE
project_types/validate/api_calls.py
Outdated
| if first_result is None: | ||
| return None | ||
|
|
||
| return first_result.get("value", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if the "value" is integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the int parsing just in case
thenav56
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
We might need to pass additional error details to the frontend to provide more context for users and debugging without the need to check server logs.
8361d4e to
6d7ec41
Compare
| project_id: strawberry.ID | None, | ||
| asset_id: strawberry.ID | None, | ||
| ohsome_filter: str | None, | ||
| ) -> TestValidateAoiObjectsResponse: | ||
| response = TestValidateAoiObjectsResponse( | ||
| project_id=project_id, | ||
| asset_id=asset_id, | ||
| ohsome_filter=ohsome_filter, | ||
| ) | ||
|
|
||
| if project_id is None: | ||
| return response.generate_error("project_id is required to test aoi elements") | ||
|
|
||
| if asset_id is None: | ||
| return response.generate_error("asset_id is required to test aoi elements") | ||
|
|
||
| if ohsome_filter is None: | ||
| return response.generate_error("ohsome_filter is required to test aoi elements") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be required on the API schema itself.
| @strawberry.field(extensions=[IsAuthenticated()]) | ||
| def test_tasking_manager_project( | ||
| self, | ||
| hot_tm_id: fields.PydanticId | None, | ||
| ohsome_filter: str | None, | ||
| ) -> TestValidateTaskingManagerProjectResponse: | ||
| response = TestValidateTaskingManagerProjectResponse( | ||
| hot_tm_id=hot_tm_id, | ||
| ohsome_filter=ohsome_filter, | ||
| ) | ||
|
|
||
| if hot_tm_id is None: | ||
| return response.generate_error("hot_tm_id is required to test HOT project aoi elements") | ||
|
|
||
| if ohsome_filter is None: | ||
| return response.generate_error("ohsome_filter is required to test HOT project aoi elements") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be required on the API schema itself.
| "AOI does not contain objects from selected filter.", | ||
| ) | ||
|
|
||
| allowed_count = 100000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100_000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not duplicate this logic.
| aoi_id = aoi_response["result"]["id"] | ||
|
|
||
| # Test AOI objects | ||
| ohsomeFilter = "building=* and geometry:polygon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohsome_filter
| area_km2 = get_area_of_geometry(geometry_collection) | ||
| allowed_area = 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not duplicate this logic.
No description provided.