feat: GIS foundations and registrant GPS coordinates#42
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the Geographic Information System (GIS) capabilities within the OpenSPP platform. It provides foundational improvements to the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant GIS functionality. It adds support for table aliases in GIS operators to prevent ambiguous column names in complex queries, which is a solid improvement. A new module spp_registrant_gis is introduced to store GPS coordinates for registrants, complete with UI additions and tests. The GIS reporting has been refactored to handle hierarchical data by aggregating from descendant administrative areas up to a base level.
While the changes are largely positive, I've identified a critical bug in the report aggregation logic that incorrectly calculates sum, min, and max aggregations. I've also found a significant performance issue (N+1 query) in the same reporting feature. Addressing these issues is crucial for the correctness and scalability of the GIS reports. I've also left a minor comment on a newly added empty security file.
spp_gis_report/models/gis_report.py
Outdated
| results[base_id]["raw"] += value * count | ||
| results[base_id]["count"] += count | ||
| results[base_id]["weight"] += count |
There was a problem hiding this comment.
There is a critical bug in the aggregation logic for sum, min, and max methods. The current implementation results[base_id]['raw'] += value * count is only correct when recalculating a weighted average (for the avg method).
- For
sum:valueis already the sum for the subgroup. Multiplying it bycountwill inflate the total sum incorrectly. It should just be added to the total. - For
minandmax: Multiplying bycountis meaningless. You should be taking theminof the minimums ormaxof the maximums across the subgroups.
This logic needs to be branched based on the agg_func.
| results[base_id]["raw"] += value * count | |
| results[base_id]["count"] += count | |
| results[base_id]["weight"] += count | |
| if agg_func == "avg": | |
| # For avg, accumulate weighted sum to recalculate later | |
| results[base_id]["raw"] += value * count | |
| elif agg_func == "sum": | |
| results[base_id]["raw"] += value | |
| elif agg_func == "min": | |
| # Initialize if it's the first value for this base_id | |
| if results[base_id]["count"] == 0: | |
| results[base_id]["raw"] = value | |
| else: | |
| results[base_id]["raw"] = min(results[base_id]["raw"], value) | |
| elif agg_func == "max": | |
| # Initialize if it's the first value for this base_id | |
| if results[base_id]["count"] == 0: | |
| results[base_id]["raw"] = value | |
| else: | |
| results[base_id]["raw"] = max(results[base_id]["raw"], value) | |
| results[base_id]["count"] += count | |
| results[base_id]["weight"] += count |
spp_gis_report/models/gis_report.py
Outdated
| for base_area in base_areas: | ||
| child_to_base[base_area.id] = base_area.id | ||
| descendants = self.env["spp.area"].search( | ||
| [ | ||
| ("id", "child_of", base_area.id), | ||
| ("id", "!=", base_area.id), | ||
| ] | ||
| ) | ||
| for desc in descendants: | ||
| child_to_base[desc.id] = base_area.id |
There was a problem hiding this comment.
There is a performance issue here due to an N+1 query problem. The self.env['spp.area'].search(...) call is inside a loop that iterates over base_areas. If there are many base areas, this will result in a large number of database queries, significantly slowing down the report generation.
To improve performance, you should refactor this to avoid making database queries inside a loop. One approach is to fetch all descendant areas at once and then process them in memory to build the child_to_base mapping. This will reduce the number of queries from N (where N is the number of base areas) to a small constant number.
| @@ -0,0 +1 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
There was a problem hiding this comment.
This ir.model.access.csv file contains only a header row. If no specific access rights are needed for this module, this file should be removed to avoid confusion. If access rights are intended, they should be added below the header. An empty or header-only access file can be misleading.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #42 +/- ##
===========================================
- Coverage 71.31% 58.74% -12.57%
===========================================
Files 299 249 -50
Lines 23618 21282 -2336
===========================================
- Hits 16844 12503 -4341
- Misses 6774 8779 +2005
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add OGC API support and area boundary handling to spp_gis - Refactor GIS report model for improved data layer management - Add new spp_registrant_gis module with GPS coordinates for registrants
- For sum: accumulate the subgroup sum directly instead of multiplying by count (which inflated results) - For min/max: track the true minimum/maximum across subgroups rather than multiplying by count (which was meaningless) - For avg: behaviour unchanged (weighted accumulation then divide) - Replace per-base-area descendant search loop with a single batched query followed by in-memory parent-chain traversal, eliminating the N+1 query pattern
jeremi
force-pushed
the
feat/gis-foundations
branch
from
b6d283b to
18a18bb
Compare
1 participant
Summary
Origin
Cherry-picked from
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan
Note
Medium Risk
Touches spatial SQL generation and report aggregation logic, so regressions could impact query correctness and reported metrics across inherited models/area hierarchies; the changes are scoped and backed by new tests.
Overview
Fixes GIS domain SQL generation in joined queries by passing Odoo’s
aliasintoOperatorand emitting a table-qualified geometry column ("alias"."field") in PostGIS expressions; adds regression tests to ensure backward-compatible behavior when no alias is provided.Improves GIS report aggregation correctness across area hierarchies by expanding the base-level area filter to include all descendant areas and rolling their
read_groupresults up to the configured base ancestor (with special handling foravg/sum/min/maxrollups), and makes GeoJSON export more robust by accepting either Shapely geometries or WKT/WKB strings for area polygons.Adds a new addon,
spp_registrant_gis, that extendsres.partnerwith acoordinatesGeoPointField, surfaces it on individual/group profile forms via thegeo_pointwidget, and includes basic installation/docs and transaction tests.Written by Cursor Bugbot for commit b6d283b. This will update automatically on new commits. Configure here.