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

[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]

The migration never runs on the path that actually has legacy data — and its raw SQL targets a non-existent table.

post_init_hook fires only when the module is installed (odoo/modules/loading.py: the hook is gated on update_operation == 'install'), never on upgrade or reinit. On a fresh install the old boolean columns don't exist, so _migrate_boolean_disaggregation correctly no-ops. On an upgrade of an already-installed module — the only case where disaggregate_by_* data exists — the hook is never invoked, so those settings are silently dropped.

Separately, even if it did run, the INSERT INTO spp_gis_report_spp_demographic_dimension_rel target is wrong: Odoo names an implicit m2m relation table from the alphabetically sorted table names (fields_relational.py: tables = sorted([model._table, comodel._table])), so the real table is spp_demographic_dimension_spp_gis_report_rel. The hardcoded name would raise relation does not exist.

Recommend converting this to a post-migration script at spp_gis_report/migrations/19.0.2.1.0/post-migrate.py (def migrate(cr, version), return early when version is falsy), and deriving the relation/column names from the field metadata (env['spp.gis.report']._fields['dimension_ids'].relation / .column1 / .column2) instead of hardcoding. Then drop post_init_hook here and the migration code from __init__.py.

[kneckinator]

SQL and Python fallback disagree on member area attribution.

Here the SQL path resolves a member's area as COALESCE(ind.area, grp.area) — the individual's own area takes precedence, falling back to the group's. But _disaggregation_python_expanded only ever reads the group's area (grp_area_id = group_area.get(grp_id)) and never fetches the individual's own area_field.

So the two paths assign different base areas whenever a member's own area differs from their group's. Because _compute_disaggregation routes SQL-compilable dimensions through this path and fallback (e.g. CEL) dimensions through the Python path and then merges by area, the same person can land in area A for one dimension and area B for another — making an area's per-dimension totals mutually inconsistent for the same population.

The docstrings say "area inheritance from the group," and test_member_expansion_area_inheritance only covers the member-has-no-area case, so the divergence is untested. Please make both paths use the same rule and add a test where the member has a different area than its group.

[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.