feat: GIS foundations and registrant GPS coordinates#42
Conversation
- 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
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.
| 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 |
| 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.
1 participant
Summary
Origin
Cherry-picked from
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan