fix(spp_gis_report): fix aggregation logic for sum/min/max and N+1 query

jeremi · jeremi · commit 18a18bb2782b · 2026-02-18T21:20:21.000+07:00
- 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
diff --git a/spp_gis_report/models/gis_report.py b/spp_gis_report/models/gis_report.py
@@ -472,17 +472,23 @@ def _compute_base_aggregation(self):
         # (e.g., barangays when base level is municipality). We include all
         # descendant areas in the query and aggregate results back to the
         # base-level parent.
-        child_to_base = {}
-        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
+        #
+        # Fetch all descendants of all base areas in a single query, then build
+        # the child_to_base mapping in memory to avoid an N+1 query pattern.
+        child_to_base = {base_area.id: base_area.id for base_area in base_areas}
+        all_descendants = self.env["spp.area"].search(
+            [
+                ("id", "child_of", base_areas.ids),
+                ("id", "not in", base_areas.ids),
+            ]
+        )
+        # For each descendant, walk up its parent chain to find the base-level ancestor
+        for desc in all_descendants:
+            ancestor = desc.parent_id
+            while ancestor and ancestor.id not in child_to_base:
+                ancestor = ancestor.parent_id
+            if ancestor:
+                child_to_base[desc.id] = child_to_base[ancestor.id]
 
         # Add area filter to domain (base areas + all descendants)
         domain.append((area_field, "in", list(child_to_base.keys())))
@@ -525,12 +531,30 @@ def _compute_base_aggregation(self):
                     base_id = child_to_base.get(area_id, area_id)
                     value = group.get(self.aggregation_field) or 0
                     count = group[f"{area_field}_count"]
-                    # Accumulate weighted values for proper re-aggregation
-                    results[base_id]["raw"] += value * count
+                    if agg_func == "avg":
+                        # Accumulate weighted sum so we can compute a proper
+                        # weighted average across subgroups when rolling up.
+                        results[base_id]["raw"] += value * count
+                    elif agg_func == "sum":
+                        # read_group already returns the sum for the subgroup;
+                        # multiplying by count would inflate the total.
+                        results[base_id]["raw"] += value
+                    elif agg_func == "min":
+                        # Keep the lowest value seen across subgroups.
+                        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":
+                        # Keep the highest value seen across subgroups.
+                        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 avg: convert accumulated (value*count) back to weighted average
+            # For avg: convert accumulated weighted sum back to weighted average
             if agg_func == "avg":
                 for data in results.values():
                     if data["count"] > 0:
diff --git a/spp_registrant_gis/README.rst b/spp_registrant_gis/README.rst
@@ -20,9 +20,9 @@ OpenSPP Registrant GIS
 .. |badge2| image:: https://img.shields.io/badge/license-LGPL--3-blue.png
     :target: http://www.gnu.org/licenses/lgpl-3.0-standalone.html
     :alt: License: LGPL-3
-.. |badge3| image:: https://img.shields.io/badge/github-OpenSPP%2Fopenspp--modules-lightgray.png?logo=github
-    :target: https://github.com/OpenSPP/openspp-modules/tree/19.0/spp_registrant_gis
-    :alt: OpenSPP/openspp-modules
+.. |badge3| image:: https://img.shields.io/badge/github-OpenSPP%2FOpenSPP2-lightgray.png?logo=github
+    :target: https://github.com/OpenSPP/OpenSPP2/tree/19.0/spp_registrant_gis
+    :alt: OpenSPP/OpenSPP2
 
 |badge1| |badge2| |badge3|
 
@@ -33,11 +33,11 @@ groups, enabling proximity-based targeting and mapping.
 Key Capabilities
 ~~~~~~~~~~~~~~~~
 
--  Store latitude/longitude coordinates on any registrant (individual or
-   group)
--  Query registrants by geographic location using PostGIS spatial
-   operators
--  Visualize registrant locations on maps via GIS widgets
+- Store latitude/longitude coordinates on any registrant (individual or
+  group)
+- Query registrants by geographic location using PostGIS spatial
+  operators
+- Visualize registrant locations on maps via GIS widgets
 
 Key Models
 ~~~~~~~~~~
@@ -53,11 +53,11 @@ Model           Extension
 UI Location
 ~~~~~~~~~~~
 
--  **Individual Form**: Located in Profile tab under "Location" section
-   (after phone numbers)
--  **Group Form**: Located in Profile tab under "Location" section
-   (after phone numbers)
--  Field is read-only when registrant is disabled
+- **Individual Form**: Located in Profile tab under "Location" section
+  (after phone numbers)
+- **Group Form**: Located in Profile tab under "Location" section (after
+  phone numbers)
+- Field is read-only when registrant is disabled
 
 Security
 ~~~~~~~~
@@ -68,11 +68,11 @@ permissions from ``spp_registry``.
 Technical Details
 ~~~~~~~~~~~~~~~~~
 
--  Field type: ``fields.GeoPointField`` (from ``spp_gis``)
--  Storage: PostGIS POINT geometry with SRID 4326 (WGS84)
--  Supports spatial operators: ``gis_intersects``, ``gis_within``,
-   ``gis_contains``, ``gis_distance``, etc.
--  Widget: ``geo_point`` for coordinate input/display
+- Field type: ``fields.GeoPointField`` (from ``spp_gis``)
+- Storage: PostGIS POINT geometry with SRID 4326 (WGS84)
+- Supports spatial operators: ``gis_intersects``, ``gis_within``,
+  ``gis_contains``, ``gis_distance``, etc.
+- Widget: ``geo_point`` for coordinate input/display
 
 Dependencies
 ~~~~~~~~~~~~
@@ -92,10 +92,10 @@ Dependencies
 Bug Tracker
 ===========
 
-Bugs are tracked on `GitHub Issues <https://github.com/OpenSPP/openspp-modules/issues>`_.
+Bugs are tracked on `GitHub Issues <https://github.com/OpenSPP/OpenSPP2/issues>`_.
 In case of trouble, please check there if your issue has already been reported.
 If you spotted it first, help us to smash it by providing a detailed and welcomed
-`feedback <https://github.com/OpenSPP/openspp-modules/issues/new?body=module:%20spp_registrant_gis%0Aversion:%2019.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.
+`feedback <https://github.com/OpenSPP/OpenSPP2/issues/new?body=module:%20spp_registrant_gis%0Aversion:%2019.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**>`_.
 
 Do not contact contributors directly about support or help with technical issues.
 
@@ -124,6 +124,6 @@ Current maintainers:
 
 |maintainer-jeremi| |maintainer-gonzalesedwin1123| |maintainer-reichie020212| 
 
-This module is part of the `OpenSPP/openspp-modules <https://github.com/OpenSPP/openspp-modules/tree/19.0/spp_registrant_gis>`_ project on GitHub.
+This module is part of the `OpenSPP/OpenSPP2 <https://github.com/OpenSPP/OpenSPP2/tree/19.0/spp_registrant_gis>`_ project on GitHub.
 
 You are welcome to contribute.
diff --git a/spp_registrant_gis/static/description/index.html b/spp_registrant_gis/static/description/index.html
@@ -374,7 +374,7 @@ <h1>OpenSPP Registrant GIS</h1>
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 !! source digest: sha256:3f603a69c4731312b90dc10708243432cd74193750065d7fe86c2364732c1e9a
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! -->
-<p><a class="reference external image-reference" href="https://odoo-community.org/page/development-status"><img alt="Alpha" src="https://img.shields.io/badge/maturity-Alpha-red.png" /></a> <a class="reference external image-reference" href="http://www.gnu.org/licenses/lgpl-3.0-standalone.html"><img alt="License: LGPL-3" src="https://img.shields.io/badge/license-LGPL--3-blue.png" /></a> <a class="reference external image-reference" href="https://github.com/OpenSPP/openspp-modules/tree/19.0/spp_registrant_gis"><img alt="OpenSPP/openspp-modules" src="https://img.shields.io/badge/github-OpenSPP%2Fopenspp--modules-lightgray.png?logo=github" /></a></p>
+<p><a class="reference external image-reference" href="https://odoo-community.org/page/development-status"><img alt="Alpha" src="https://img.shields.io/badge/maturity-Alpha-red.png" /></a> <a class="reference external image-reference" href="http://www.gnu.org/licenses/lgpl-3.0-standalone.html"><img alt="License: LGPL-3" src="https://img.shields.io/badge/license-LGPL--3-blue.png" /></a> <a class="reference external image-reference" href="https://github.com/OpenSPP/OpenSPP2/tree/19.0/spp_registrant_gis"><img alt="OpenSPP/OpenSPP2" src="https://img.shields.io/badge/github-OpenSPP%2FOpenSPP2-lightgray.png?logo=github" /></a></p>
 <p>Extends registrants with GPS coordinates for spatial queries and
 geographic analysis. Adds a PostGIS point field to both individuals and
 groups, enabling proximity-based targeting and mapping.</p>
@@ -413,8 +413,8 @@ <h2>UI Location</h2>
 <ul class="simple">
 <li><strong>Individual Form</strong>: Located in Profile tab under “Location” section
 (after phone numbers)</li>
-<li><strong>Group Form</strong>: Located in Profile tab under “Location” section
-(after phone numbers)</li>
+<li><strong>Group Form</strong>: Located in Profile tab under “Location” section (after
+phone numbers)</li>
 <li>Field is read-only when registrant is disabled</li>
 </ul>
 </div>
@@ -455,10 +455,10 @@ <h2>Dependencies</h2>
 </div>
 <div class="section" id="bug-tracker">
 <h3><a class="toc-backref" href="#toc-entry-1">Bug Tracker</a></h3>
-<p>Bugs are tracked on <a class="reference external" href="https://github.com/OpenSPP/openspp-modules/issues">GitHub Issues</a>.
+<p>Bugs are tracked on <a class="reference external" href="https://github.com/OpenSPP/OpenSPP2/issues">GitHub Issues</a>.
 In case of trouble, please check there if your issue has already been reported.
 If you spotted it first, help us to smash it by providing a detailed and welcomed
-<a class="reference external" href="https://github.com/OpenSPP/openspp-modules/issues/new?body=module:%20spp_registrant_gis%0Aversion:%2019.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**">feedback</a>.</p>
+<a class="reference external" href="https://github.com/OpenSPP/OpenSPP2/issues/new?body=module:%20spp_registrant_gis%0Aversion:%2019.0%0A%0A**Steps%20to%20reproduce**%0A-%20...%0A%0A**Current%20behavior**%0A%0A**Expected%20behavior**">feedback</a>.</p>
 <p>Do not contact contributors directly about support or help with technical issues.</p>
 </div>
 <div class="section" id="credits">
@@ -473,7 +473,7 @@ <h4><a class="toc-backref" href="#toc-entry-3">Authors</a></h4>
 <h4><a class="toc-backref" href="#toc-entry-4">Maintainers</a></h4>
 <p>Current maintainers:</p>
 <p><a class="reference external image-reference" href="https://github.com/jeremi"><img alt="jeremi" src="https://github.com/jeremi.png?size=40px" /></a> <a class="reference external image-reference" href="https://github.com/gonzalesedwin1123"><img alt="gonzalesedwin1123" src="https://github.com/gonzalesedwin1123.png?size=40px" /></a> <a class="reference external image-reference" href="https://github.com/reichie020212"><img alt="reichie020212" src="https://github.com/reichie020212.png?size=40px" /></a></p>
-<p>This module is part of the <a class="reference external" href="https://github.com/OpenSPP/openspp-modules/tree/19.0/spp_registrant_gis">OpenSPP/openspp-modules</a> project on GitHub.</p>
+<p>This module is part of the <a class="reference external" href="https://github.com/OpenSPP/OpenSPP2/tree/19.0/spp_registrant_gis">OpenSPP/OpenSPP2</a> project on GitHub.</p>
 <p>You are welcome to contribute.</p>
 </div>
 </div>
diff --git a/spp_registrant_gis/views/res_partner_views.xml b/spp_registrant_gis/views/res_partner_views.xml
@@ -7,7 +7,10 @@
         <field name="inherit_id" ref="spp_registry.view_individuals_form" />
         <field name="arch" type="xml">
             <!-- Add GPS coordinates after phone numbers section -->
-            <xpath expr="//page[@name='profile']//group[@name='phone_section']" position="after">
+            <xpath
+                expr="//page[@name='profile']//group[@name='phone_section']"
+                position="after"
+            >
                 <separator string="Location" />
                 <group name="gis_section">
                     <field name="coordinates" readonly="disabled" widget="geo_point" />
@@ -23,7 +26,10 @@
         <field name="inherit_id" ref="spp_registry.view_individuals_form" />
         <field name="arch" type="xml">
             <!-- Add GPS coordinates after phone numbers section -->
-            <xpath expr="//page[@name='group_profile']//group[@name='group_phone_section']" position="after">
+            <xpath
+                expr="//page[@name='group_profile']//group[@name='group_phone_section']"
+                position="after"
+            >
                 <separator string="Location" />
                 <group name="group_gis_section">
                     <field name="coordinates" readonly="disabled" widget="geo_point" />
