feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76)

[gonzalesedwin1123]

Re-lands the spp_gis_report portion of reverted PR #76 (revert: #271). Wave 3 — stacked on the spp_metric_service Wave-2 PR (and transitively #275); merge those first. Base is set to reland/metric-service so this diff shows only spp_gis_report.

Summary

• Metric disaggregation in GIS reports: breakdown dimensions, report helpers, wizard support, disaggregation tests.

Verification

• ./spp t spp_gis_report on an integration state with feat(spp_cel_domain): SQL CASE compiler, read-only smart-op lookup, translator cache tests (re-land from #76) #275 + Wave 2 merged: 188 passed, 0 failed

Scope addition (2026-07-14)

Also ports spp_mis_demo_v2/data/demo_gis_reports.xml to dimension_ids + member_expansion (taken verbatim from draft #295, credit @kneckinator, Co-authored-by preserved). Rationale: removing the disaggregate_by_* booleans while mainline demo data still sets them hard-breaks any install path pulling spp_mis_demo_v2 — CI caught it on this PR's merge ref (test (spp_dci_demo) ParseError). Folding the port makes this merge atomic with no broken-mainline window; #295 retains the independent STORY_AREA_MAP p-code remap. Verified: ./spp t spp_mis_demo_v2 272/0 with the ported data.

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[kneckinator]

Code Review — PR #280 feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76)

Reviewed the full gh pr diff (17 files, +1185/-87) against the 19.0 checkout, plus the metric-service API and in-repo consumers of spp_gis_report.

Overview

Replaces the three fixed boolean disaggregation flags (disaggregate_by_gender/_age/_disability) on spp.gis.report (and the wizard) with a flexible dimension_ids M2M to spp.demographic.dimension, and adds a member_expansion selection to drill group-filtered reports down to individual members. Disaggregation is computed in _compute_disaggregation, which compiles dimensions to a single parameterized SQL query (_disaggregation_sql → _build_direct_query / _build_expand_query) with a Python/ORM fallback (_disaggregation_python[_expanded]) for dimensions that can't compile to SQL. Results are stored on spp.gis.report.data.disaggregation and emitted in GeoJSON as flat disagg_<dim>_<value> properties plus a metadata.disaggregation block. A post_init_hook migrates legacy boolean data into dimension_ids. Threshold min_value/max_value are now surfaced in the legend and GeoJSON bucket info. Version bumped 19.0.2.0.1 → 19.0.2.1.0 with matching README/HISTORY/index.html changelog entries.

Overall this is a solid, well-tested change. The SQL is parameterized and injection-safe, record rules are respected (partner IDs come from search() before raw SQL), empty/None paths are handled throughout, and there is good new unit coverage. The main risks are cross-module/merge-order coupling rather than logic defects.

---

High severity

1. Merge-order coupling with #279 (confirmed).
_compute_disaggregation calls dimension.to_sql_column(partner_alias, alias_counter) and consumes a SQLColumnResult-shaped object (.expression, .joins, .alias_counter) — spp_gis_report/models/gis_report.py:410, :459-468. That API does not exist on spp.demographic.dimension in the current 19.0 base (grep -c to_sql_column spp_metric_service/models/demographic_dimension.py → 0); it is introduced by the sibling re-land #279. The PR base is correctly set to reland/metric-service, and the body says "merge those first", so this is expected — but it must be enforced: merging #280 ahead of #279 (and transitively #275) will break spp_gis_report at runtime the first time a report with dimension_ids is refreshed. Flagging so the merge order is respected.

2. Downstream demo data references removed fields (confirmed breakage).
spp_mis_demo_v2 depends on spp_gis_report (spp_mis_demo_v2/__manifest__.py:22) and its demo data still sets the now-deleted boolean fields:

• spp_mis_demo_v2/data/demo_gis_reports.xml:34 disaggregate_by_gender
• :63 disaggregate_by_gender
• :92 disaggregate_by_age
• :93 disaggregate_by_gender

These fields are removed by this PR (replaced by dimension_ids). Installing/updating spp_mis_demo_v2 will raise a ParseError (field does not exist). This PR does not touch that file, so the fix must be coordinated (either in this wave or a follow-up) — port those records to dimension_ids referencing the demographic dimensions. Confirmed defect for the spp_mis_demo_v2 install path.

---

Medium severity

3. post_init_hook migration does not run on module upgrade.
_migrate_boolean_disaggregation (spp_gis_report/__init__.py:42) is wired as post_init_hook (__manifest__.py:132). In Odoo, post_init_hook runs only on fresh install, not on -u/upgrade. Existing databases already on 19.0.2.0.1 that have boolean flags set will therefore not be migrated when they upgrade to 19.0.2.1.0; their disaggregation config is silently lost. If in-place upgrades of already-installed instances are a supported path, this logic belongs in a migrations/19.0.2.1.0/post-migrate.py script (or in addition to the hook). The SQL inside the hook itself is fine (column-existence guard, psycopg2.sql.Identifier quoting, ON CONFLICT DO NOTHING). Please verify whether an upgrade migration is required for this deployment.

4. GeoJSON output schema change (compatibility note).
Disaggregation output changed from a nested properties["disaggregation"] = {dim: {value: count}} to flat properties["disagg_<dim>_<value>"] = count (gis_report.py:1899-1906) with interpretation moved to metadata.disaggregation. In-repo this is safe — spp_api_v2_gis only forwards the include_disaggregation flag and never parses the structure (verified in layers_service.py/export_service.py), and the inheriting models (spp_gis_report_programs, spp_drims, spp_drims_sl_demo) don't touch any changed symbol. But this is a breaking change to the public /api/v2/GISReport/.../geojson payload shape for any external/QGIS consumer that read the old nested disaggregation key. It's a re-land of prior behavior, so likely intended — noting it for awareness and to confirm it's in the changelog appropriately.

---

Low / nits

• import json is done locally inside _to_geojson (gis_report.py:788) instead of at module top. Minor style inconsistency. The same value-labels JSON-string-vs-dict handling is duplicated between this block and demographic_dimension.get_label_for_value; not worth refactoring now but worth a comment.
• When a report mixes SQL-compilable and Python-fallback dimensions, the matching partner set is queried twice (Partner.search in _disaggregation_sql and Partner.search_read in _disaggregation_python). Acceptable; only relevant for mixed-dimension reports.
• With member_expansion="expand", disaggregation totals (individuals) intentionally won't reconcile with the base count aggregation (groups). This is by design and documented in the field help, but consumers comparing the two will see a mismatch.

---

Things verified clean

• SQL safety: all dynamic SQL uses odoo.tools.sql.SQL with %s params and SQL.identifier(...) for identifiers; dimension names / column aliases are quoted, never string-formatted. Registrant IDs come from search() (record-rule aware) before the raw query. No injection surface found.
• Schema references in expand query: spp.group.membership has real columns group, individual, and stored is_ended (spp_registry/models/group_membership.py:16-60); the reserved-word identifiers are correctly quoted via SQL.identifier. DISTINCT ON (ind.id) correctly dedups members shared across groups (covered by test_member_expansion_distinct_dedup).
• Edge/empty handling: _prepare_area_context returns None (no source model / no base areas); _compute_disaggregation early-returns {} for no dimensions, non-res.partner source, or None context; empty registrant/membership sets short-circuit. All covered by tests.
• View modifier: the new invisible="source_model != 'res.partner'" on member_expansion works because source_model is present (invisible) in the form view (views/gis_report_views.xml:234).
• Constraint: _check_member_expansion (@api.constrains) correctly rejects expand on non-partner sources; tested in both new test files.
• Consistency: manifest version, README.rst, readme/HISTORY.md, and static/description/index.html changelogs are all bumped consistently; manifest correctly adds the spp_metric_service dependency for the new breakdown API.
• Tests: two new files (test_disaggregation.py ~452 lines, test_gis_report_helpers.py), plus updates to test_area_ext, test_gis_report_api, test_gis_report_wizard, and tests/__init__.py. New behaviors (SQL + Python paths, member expansion, dedup, area inheritance, GeoJSON flat props + metadata, constraint) are covered. PR reports 188 passed / 0 failed on an integration state with feat(spp_cel_domain): SQL CASE compiler, read-only smart-op lookup, translator cache tests (re-land from #76) #275 + Wave 2.

---

Recommendation

Approve on logic, conditional on: (1) merging #279 (+#275) before this PR, and (2) resolving the spp_mis_demo_v2/data/demo_gis_reports.xml references to the removed boolean fields (High #2). Please also confirm whether an upgrade migration is needed for the post_init_hook gap (Medium #3).

[kneckinator]

The spp_mis_demo_v2 downstream breakage flagged in the review above is now fixed in #295 (draft).

This PR removes the boolean disaggregate_by_gender/disaggregate_by_age fields on spp.gis.report, but spp_mis_demo_v2/data/demo_gis_reports.xml still set them, causing a hard ParseError on install/update. #295 switches that demo data to dimension_ids + member_expansion.

Merge order: #295 depends on this PR (and #277), so it must merge after both and be rebased onto the result.

[kneckinator]

Two issues from review worth addressing before merge (plus minor notes left in the summary review). Both concern the boolean→dimension migration path and the member-expansion area attribution.

[kneckinator]

Pushed 3337da9 addressing the migration finding directly (converted the post_init_hook to a migrations/19.0.2.1.0/post-migrate.py script, and derived the m2m relation/column names from the field so the wrong-table-name bug is gone). Removed the now-dead hook code from __init__.py and __manifest__.py.

Ran the repo's pre-commit hooks on the changed files — ruff, Odoo-19 compat, bandit, whitespace/EOF all pass. (semgrep couldn't run locally due to a Python 3.14 toolchain crash, unrelated to this change; worth confirming in CI.)

Still open for you: the member-expansion area-attribution divergence (gis_report.py:767 comment) — that one needs a semantic decision (member's own area vs. group's area) so I left it for you rather than picking one unilaterally. Happy to implement whichever you choose.

[codecov]

Codecov Report

❌ Patch coverage is 80.27523% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.28%. Comparing base (ede1a1b) to head (8f9ded7).

Files with missing lines	Patch %	Lines
spp_gis_report/models/gis_report.py	79.71%	43 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #280      +/-   ##
==========================================
+ Coverage   71.85%   74.28%   +2.43%     
==========================================
  Files         245      372     +127     
  Lines       19111    25385    +6274     
==========================================
+ Hits        13732    18857    +5125     
- Misses       5379     6528    +1149     

Flag	Coverage Δ
spp_api_v2_cycles	70.65% <ø> (?)
spp_api_v2_entitlements	69.96% <ø> (?)
spp_api_v2_gis	71.52% <ø> (ø)
spp_api_v2_programs	92.03% <ø> (?)
spp_api_v2_simulation	71.12% <ø> (ø)
spp_audit_programs	0.00% <ø> (?)
spp_base_common	90.26% <ø> (ø)
spp_case_entitlements	97.61% <ø> (?)
spp_case_programs	97.14% <ø> (?)
spp_cr_type_assign_program	91.17% <ø> (?)
spp_dci_compliance	92.76% <ø> (?)
spp_dci_demo	93.39% <ø> (ø)
spp_dci_server_social	89.38% <ø> (?)
spp_drims	80.82% <ø> (?)
spp_drims_sl	0.00% <ø> (?)
spp_gis_report	82.78% <80.27%> (?)
spp_mis_demo_v2	70.96% <ø> (ø)
spp_programs	65.27% <ø> (ø)
spp_registry	86.83% <ø> (ø)
spp_security	66.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_gis_report/__manifest__.py	0.00% <ø> (ø)
spp_gis_report/controllers/geojson.py	74.22% <100.00%> (ø)
spp_gis_report/models/area_ext.py	75.29% <100.00%> (ø)
spp_gis_report/wizards/gis_report_wizard.py	97.84% <100.00%> (ø)
spp_mis_demo_v2/__manifest__.py	0.00% <ø> (ø)
spp_gis_report/models/gis_report.py	79.63% <79.71%> (ø)

... and 122 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.