feat(spp_metric_service): breakdown expansion + SQL column support (re-land from #76)

[gonzalesedwin1123]

Re-lands the spp_metric_service portion of reverted PR #76 (revert: #271). Wave 2 — stacked on #275 (spp_cel_domain); merge that first. The base is set to reland/cel-domain so this diff shows only spp_metric_service; GitHub retargets to 19.0 when #275 merges — wait for CI to re-run green on the retargeted base before merging.

Summary

• Demographic breakdown expansion and SQL column support for metric disaggregation (uses the spp_cel_domain SQL CASE compiler).

Added on top of #76 (not in the original)

• gemini-code-assist review findings, all applied: recordset subtraction for individuals; batched memberships.individual.ids (avoids N+1); _fields membership check instead of hasattr on recordsets; early return for falsy raw_value before code search; default passed directly as a SQL parameter (two sites).
• Version bump + HISTORY entry.

Verification

• ./spp t spp_metric_service 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 merged: 161 passed, 0 failed
• All gemini-code-assist threads resolved with fixing-commit references

[gemini-code-assist]

Code Review

This pull request introduces demographic breakdown expansion and SQL column compilation support for metric disaggregation. Key changes include updating model names, refining the age group dimension to align with UNICEF/WHO buckets, automatically expanding groups to members for individual-level dimensions, and adding a to_sql_column method to compile dimensions into SQL expressions. The reviewer feedback focuses on performance and code quality optimizations, such as using recordset subtraction instead of lambda filtering, batch-fetching membership IDs to prevent N+1 queries, checking _fields on Odoo recordsets instead of using hasattr, guarding against falsy values early, and removing redundant SQL parameter wrapping.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gonzalesedwin1123]

gemini-code-assist disposition:

Applied (commit 778e037): recordset subtraction for individuals, batched memberships.individual.ids, _fields membership check instead of hasattr, early return for falsy raw_value. Suite: 161/0 on the integration state with #275.

Deferred: the two SQL("%s", default) wrapping comments — both forms parameterize identically with Odoo's SQL wrapper; cosmetic, and I'd rather keep the re-land diff byte-stable. Happy to fold in if reviewers prefer.

[gemini-code-assist]

Thank you for the detailed disposition. The changes you've implemented, particularly the shift to _fields checks and the batching of memberships.individual.ids, are solid improvements for performance and robustness in an Odoo environment. Deferring the cosmetic SQL wrapping is also a sensible decision to maintain diff stability. Everything looks well-aligned with the requirements.

[gonzalesedwin1123]

Update: the two deferred SQL("%s", default) simplifications are applied (direct parameter passing). Suite 161/0 on the integration state. All gemini findings on this PR are now addressed.

[kneckinator]

Code review — PR #279 feat(spp_metric_service): breakdown expansion + SQL column support (re-land from #76)

Reviewed the full gh pr diff against the code in the checkout (base reland/cel-domain, i.e. stacked on #275). Overall this is a clean, well-tested re-land. The headline security concern (SQL injection via dynamic column names) is properly mitigated. A few semantic/behavioral and minor test-gap notes below.

What the PR does

• Breakdown expansion (breakdown_service.py): compute_breakdown now auto-expands group IDs to their active individual members whenever any requested dimension has applies_to == "individuals", via new _expand_groups_to_members() (recordset subtraction for individuals, a single batched spp.group.membership search for groups, dedup via set).
• SQL column support (demographic_dimension.py): new to_sql_column() + helpers compile a dimension into an odoo.tools.sql.SQL expression (returned in a new SQLColumnResult dataclass with expression, joins, alias_counter). Field dimensions produce CAST/COALESCE column refs (with LEFT JOIN for Many2one, direct or dotted); expression dimensions delegate to spp.cel.translator.to_sql_case(...). Falls back to None (Python path) when it can't compile.
• get_label_for_value / _lookup_m2o_label: static value_labels_json still wins; added dynamic display_name lookup for single-segment Many2one field dimensions. _evaluate_field now uses _name/_fields checks and returns default for empty recordsets.
• Data: gender dimension → gender_id.code, applies_to=individuals; age-group CEL buckets refined to UNICEF/WHO bands.
• Docs/manifest: version 19.0.2.0.0→19.0.2.1.0, HISTORY/README changelog entries, and README/DESCRIPTION model-name typo fix spp.metrics.*→spp.metric.*.

Security — SQL injection (the big one): mitigated, no defect found

• All identifiers are validated against model metadata before use and then wrapped in SQL.identifier():
  • field_name checked against res.partner._fields (demographic_dimension.py:411, :429);
  • sub_field checked against comodel._fields (:475);
  • comodel_table comes from comodel._table (Odoo-controlled);
  • join_alias is f"_dim{alias_counter}" with an integer counter.
• All values (default, CEL literals) are passed as %s bind params, never interpolated (:461, :489, :507; CEL side emits SQL("%s", value)).
• field_path is admin-configurable but a non-existent field yields None (Python fallback), so a bad/hostile config cannot inject — it just disables the SQL path.
• Confirmed the callee to_sql_case(expression, model, alias, cfg=None) exists on the base branch and the call site matches its signature.

Correctness / behavioral notes

• compute_breakdown expansion is all-or-nothing across dimensions (verify). breakdown_service.py:59 triggers expansion if any dimension is individuals, then expands the entire registrant_ids set. If a caller mixes an individuals dimension with an all/groups dimension in the same group_by, those non-individual dimensions are then evaluated over member records rather than the original groups. Mixing is inherently ambiguous, but worth confirming this is the intended semantic.
• Breakdown totals can now exceed the scope count (verify downstream). The sole in-repo consumer, spp_analytics/models/service_aggregation.py:339, delegates straight to compute_breakdown. After expansion a group scope with an individual dimension returns member-level counts, so sum(breakdown counts) no longer equals the top-level registrant count. I checked service_aggregation.py — nothing asserts that invariant and privacy.enforce() only benefits from larger cells, so no crash; but any consumer comparing breakdown totals to the scope count will see a changed number. This is the intended fix, just observable.
• _expand_groups_to_members logic is correct: .exists() guards stale IDs, active-only via is_ended = False (matches the established pattern in spp_registry/models/individual.py:186-187), individuals pass through, dedup is correct. The sudo() is annotated with rationale and semgrep nosem markers consistent with the rest of the module.
• CEL SQL path drops joins — but that's fine. _to_sql_column_expression (:493) returns alias_counter unchanged and no joins; to_sql_case emits self-contained CASE/column refs against alias (no external joins), so nothing is lost.

Performance

• Expansion is a single batched membership search + set ops — no N+1. Good.
• Minor: _lookup_m2o_label issues one search per distinct value for field-based Many2one dimensions that lack a static value_labels_json entry (demographic_dimension.py:327). It's bounded by distinct breakdown cells (called once per new key in compute_breakdown, not per registrant), so low-cardinality dims (gender) are fine, but a high-cardinality dim like area_id with no static labels means one query per distinct area value. Consider a batched lookup if that becomes a hot path. Not a blocker.

Tests

• New test_sql_column.py (field/simple, M2O dotted, M2O direct, CEL CASE-WHEN, unsupported→None, invalid field→None, alias-counter increment, cross-module CEL) and test_breakdown_expansion.py (individual dim expands, all dim doesn't, mixed dedup, empty group, empty group_by, individual passthrough) are solid and match observed behavior. test_coverage.py adds empty-M2O-default, dynamic M2O label, and static-precedence cases. PR reports 161 passed 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.
• Test gap (low): the SQL applies_to CASE-wrap branch is not exercised. to_sql_column wraps the expression in CASE WHEN <alias>.is_group = FALSE/TRUE ... for individuals/groups dimensions (:373-397), but every dimension created in test_sql_column.py leaves applies_to at its default "all", so that wrapping (and its is_group reference) is never asserted. Worth one test each for an individuals and a groups dimension.

Conventions / consistency

• Version bump, readme/HISTORY.md, and README Changelog are all consistent at 19.0.2.1.0. Manifest already depends on spp_cel_domain, so the to_sql_case delegation is a declared dependency.
• The spp.metrics.*→spp.metric.* doc edits are a correctness fix (the actual _names were already singular, e.g. spp.metric.breakdown); grep found no plural spp.metrics. references in code, so no runtime impact.

Downstream impact (in-repo only)

• to_sql_column has no in-repo runtime consumer yet — it appears only in its definition and test_sql_column.py. The intended consumer is the stacked sibling PR feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76) #280 (spp_gis_report, base = reland/metric-service = this PR's head), which does not exist on 19.0. So this PR adds dormant public API that feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76) #280 relies on: to_sql_column() and the SQLColumnResult dataclass shape (expression/joins/alias_counter). If those signatures shift, feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76) #280 must move in lockstep. Merge order is feat(spp_cel_domain): SQL CASE compiler, read-only smart-op lookup, translator cache tests (re-land from #76) #275 → feat(spp_metric_service): breakdown expansion + SQL column support (re-land from #76) #279 → feat(spp_gis_report): metric disaggregation in GIS reports (re-land from #76) #280.
• Existing consumers of compute_breakdown: only spp_analytics/models/service_aggregation.py. It is source-compatible (same signature), but subject to the expansion behavior change noted above. Its existing tests use the registrant_type dimension (applies_to=all), so they won't trigger expansion and should stay green.
• No in-repo module imports the changed public symbols in a way that breaks. spp_api_v2_simulation and spp_simulation reference spp.demographic.dimension by name only; unaffected.

Verdict

No confirmed defects. SQL construction is safe and parameterized. The main things to consciously sign off on are (1) the group→member expansion is an intended, observable semantic change for group-scoped breakdowns, and (2) the small test gap on the SQL applies_to wrapper. The _lookup_m2o_label per-value query is a minor future perf watch-item. Given the stacking, confirm CI is green on the retargeted base before merge, and land #280 after this.

[gonzalesedwin1123]

Review response — completing the three items from your verdict (branch rebased onto current 19.0, which now contains merged #275/#276/#277, and pushed as a8b8f084; this also finally triggers CI on the real base, since the retarget after #275's merge didn't):

Sign-off 1 (all-or-nothing expansion): endorsed as intended. Mixing an individuals dimension with all/groups dimensions in one request is inherently ambiguous, and per-dimension evaluation sets would be a substantially different design; expanding the whole set is the coherent choice and matches the reviewed #76 behavior. Now stated explicitly in the compute_breakdown docstring and HISTORY.

Sign-off 2 (totals need not reconcile with scope count): endorsed as intended — member-counted totals are the point of the feature; the sole in-repo consumer (spp_analytics) doesn't compare the two, and the privacy layer only benefits from larger cells. Also documented.

Test gap: closed — test_applies_to_individuals_wraps_in_is_group_false_case and test_applies_to_groups_wraps_in_is_group_true_case assert the CASE-wrapping (and the default fall-through param) that #280's queries rely on. Suite: 163 passed, 0 failed on the rebased base.

The _lookup_m2o_label per-value query is noted as a future perf watch-item, agreed not a blocker. Once CI settles green here, this should be ready to merge ahead of #280 per the required order.

[codecov]

Codecov Report

❌ Patch coverage is 87.85714% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.85%. Comparing base (b3441ab) to head (a8b8f08).

Files with missing lines	Patch %	Lines
spp_metric_service/models/demographic_dimension.py	86.06%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #279      +/-   ##
==========================================
+ Coverage   69.15%   71.85%   +2.69%     
==========================================
  Files         159      245      +86     
  Lines       14117    19111    +4994     
==========================================
+ Hits         9763    13732    +3969     
- Misses       4354     5379    +1025     

Flag	Coverage Δ
spp_analytics	93.13% <ø> (?)
spp_api_v2_gis	71.52% <ø> (?)
spp_api_v2_simulation	71.12% <ø> (?)
spp_base_common	90.26% <ø> (ø)
spp_dci_demo	93.39% <ø> (ø)
spp_indicator	96.55% <ø> (?)
spp_indicator_studio	0.00% <ø> (?)
spp_metric_service	89.08% <87.85%> (?)
spp_mis_demo_v2	70.96% <ø> (ø)
spp_programs	65.27% <ø> (ø)
spp_registry	86.83% <ø> (+0.01%)	⬆️
spp_security	66.66% <ø> (ø)
spp_simulation	79.58% <ø> (?)

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

Files with missing lines	Coverage Δ
spp_metric_service/__manifest__.py	0.00% <ø> (ø)
spp_metric_service/models/breakdown_service.py	100.00% <100.00%> (ø)
spp_metric_service/models/demographic_dimension.py	90.62% <86.06%> (ø)

... and 84 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.