From 735809d733a68098dbc9ffe1cd0b2880c74afc6b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 23:57:11 +0800 Subject: [PATCH 1/6] feat(spp_metric_service): re-land breakdown expansion + SQL column support (from #76) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-lands the spp_metric_service portion of reverted PR #76. Depends on the spp_cel_domain SQL CASE compiler (#275) — do not merge before it. --- spp_metric_service/README.rst | 46 ++-- spp_metric_service/__manifest__.py | 2 +- .../data/demographic_dimensions.xml | 16 +- .../models/breakdown_service.py | 46 ++++ .../models/demographic_dimension.py | 260 ++++++++++++++++-- spp_metric_service/readme/DESCRIPTION.md | 14 +- spp_metric_service/readme/HISTORY.md | 4 + spp_metric_service/tests/__init__.py | 2 + .../tests/test_breakdown_expansion.py | 129 +++++++++ spp_metric_service/tests/test_coverage.py | 56 ++++ spp_metric_service/tests/test_sql_column.py | 155 +++++++++++ 11 files changed, 676 insertions(+), 54 deletions(-) create mode 100644 spp_metric_service/tests/test_breakdown_expansion.py create mode 100644 spp_metric_service/tests/test_sql_column.py diff --git a/spp_metric_service/README.rst b/spp_metric_service/README.rst index e306d7f6..9fa8846d 100644 --- a/spp_metric_service/README.rst +++ b/spp_metric_service/README.rst @@ -45,27 +45,27 @@ Key Capabilities Key Models ~~~~~~~~~~ -+---------------------------------+------------------------------------+ -| Model | Description | -+=================================+====================================+ -| ``spp.demographic.dimension`` | Configurable dimension for | -| | breakdowns (field or CEL) | -+---------------------------------+------------------------------------+ -| ``spp.metrics.fairness`` | Abstract service: equity/parity | -| | analysis | -+---------------------------------+------------------------------------+ -| ``spp.metrics.distribution`` | Abstract service: distribution | -| | statistics | -+---------------------------------+------------------------------------+ -| ``spp.metrics.breakdown`` | Abstract service: | -| | multi-dimensional grouping | -+---------------------------------+------------------------------------+ -| ``spp.metrics.privacy`` | Abstract service: k-anonymity | -| | enforcement | -+---------------------------------+------------------------------------+ -| ``spp.metrics.dimension.cache`` | Abstract service: dimension | -| | evaluation cache | -+---------------------------------+------------------------------------+ ++--------------------------------+-------------------------------------+ +| Model | Description | ++================================+=====================================+ +| ``spp.demographic.dimension`` | Configurable dimension for | +| | breakdowns (field or CEL) | ++--------------------------------+-------------------------------------+ +| ``spp.metric.fairness`` | Abstract service: equity/parity | +| | analysis | ++--------------------------------+-------------------------------------+ +| ``spp.metric.distribution`` | Abstract service: distribution | +| | statistics | ++--------------------------------+-------------------------------------+ +| ``spp.metric.breakdown`` | Abstract service: multi-dimensional | +| | grouping | ++--------------------------------+-------------------------------------+ +| ``spp.metric.privacy`` | Abstract service: k-anonymity | +| | enforcement | ++--------------------------------+-------------------------------------+ +| ``spp.metric.dimension.cache`` | Abstract service: dimension | +| | evaluation cache | ++--------------------------------+-------------------------------------+ Configuration ~~~~~~~~~~~~~ @@ -96,10 +96,10 @@ Group Access Extension Points ~~~~~~~~~~~~~~~~ -- Override ``_analyze_dimension()`` in ``spp.metrics.fairness`` for +- Override ``_analyze_dimension()`` in ``spp.metric.fairness`` for custom analysis logic - Add new dimension types by extending ``spp.demographic.dimension`` -- Override ``enforce()`` in ``spp.metrics.privacy`` for custom +- Override ``enforce()`` in ``spp.metric.privacy`` for custom suppression strategies Dependencies diff --git a/spp_metric_service/__manifest__.py b/spp_metric_service/__manifest__.py index e77c4ce9..dea316e7 100644 --- a/spp_metric_service/__manifest__.py +++ b/spp_metric_service/__manifest__.py @@ -3,7 +3,7 @@ "name": "OpenSPP Metric Service", "summary": "Computation services for fairness, distribution, breakdown, and privacy", "category": "OpenSPP", - "version": "19.0.2.0.0", + "version": "19.0.2.1.0", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_metric_service/data/demographic_dimensions.xml b/spp_metric_service/data/demographic_dimensions.xml index 862bb3f5..8a1e8578 100644 --- a/spp_metric_service/data/demographic_dimensions.xml +++ b/spp_metric_service/data/demographic_dimensions.xml @@ -7,8 +7,8 @@ Gender identity from ISO 5218 vocabulary 10 field - gender_id - all + gender_id.code + individuals {"0": "Not Known", "1": "Male", "2": "Female", "9": "Not Applicable"} @@ -56,23 +56,27 @@ >{"true": "Group/Household", "false": "Individual"} - + age_group Age Group - Age group based on birth date + Age group based on birth date (UNICEF/WHO-aligned) 50 expression individuals {"child": "Child (0-17)", "adult": "Adult (18-59)", "elderly": "Elderly (60+)", "unknown": "Unknown"} + >{"under_5": "Under 5", "child": "Child (5-14)", "adolescent": "Adolescent (15-17)", "adult": "Adult (18-59)", "elderly": "Elderly (60+)", "unknown": "Unknown"} diff --git a/spp_metric_service/models/breakdown_service.py b/spp_metric_service/models/breakdown_service.py index acb1524b..89e5d94b 100644 --- a/spp_metric_service/models/breakdown_service.py +++ b/spp_metric_service/models/breakdown_service.py @@ -56,6 +56,14 @@ def compute_breakdown(self, registrant_ids, group_by, statistics=None, context=N if not dimensions: return {} + # Auto-expand groups to members when any dimension applies to individuals only + needs_expansion = any(d.applies_to == "individuals" for d in dimensions) + if needs_expansion: + registrant_ids = self._expand_groups_to_members(registrant_ids) + + if not registrant_ids: + return {} + # Get cache service cache_service = self.env["spp.metric.dimension.cache"] @@ -95,3 +103,41 @@ def compute_breakdown(self, registrant_ids, group_by, statistics=None, context=N # TODO: Add per-cell statistics if needed return breakdown + + @api.model + def _expand_groups_to_members(self, registrant_ids): + """ + Expand group IDs to their individual member IDs. + + Groups are replaced by their active members. Individual IDs pass through. + The result is deduplicated. + + :param registrant_ids: List of partner IDs (groups and/or individuals) + :returns: Deduplicated list of individual partner IDs + :rtype: list + """ + # sudo: aggregate breakdown metrics must expand groups to their members + # across all registrants regardless of the caller's record rules. + # Read-only (no writes); callers are authorized at the service entry point. + Partner = self.env["res.partner"].sudo() # nosemgrep: odoo-sudo-without-context,odoo-sudo-on-sensitive-models + records = Partner.browse(registrant_ids).exists() + + group_ids = records.filtered("is_group").ids + individual_ids = set(records.filtered(lambda r: not r.is_group).ids) + + if not group_ids: + return list(individual_ids) + + # Expand groups via active memberships + Membership = self.env["spp.group.membership"].sudo() # nosemgrep: odoo-sudo-without-context + memberships = Membership.search( + [ + ("group", "in", group_ids), + ("is_ended", "=", False), + ] + ) + + for membership in memberships: + individual_ids.add(membership.individual.id) + + return list(individual_ids) diff --git a/spp_metric_service/models/demographic_dimension.py b/spp_metric_service/models/demographic_dimension.py index 03900eb7..86d5efa0 100644 --- a/spp_metric_service/models/demographic_dimension.py +++ b/spp_metric_service/models/demographic_dimension.py @@ -1,13 +1,25 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. import json import logging +from dataclasses import dataclass +from dataclasses import field as dataclass_field from odoo import _, api, fields, models from odoo.exceptions import ValidationError +from odoo.tools.sql import SQL _logger = logging.getLogger(__name__) +@dataclass +class SQLColumnResult: + """Result of compiling a demographic dimension to a SQL expression.""" + + expression: SQL + joins: list[SQL] = dataclass_field(default_factory=list) + alias_counter: int = 0 + + class DemographicDimension(models.Model): """ Configurable demographic dimensions for group_by breakdowns. @@ -173,8 +185,10 @@ def _evaluate_field(self, record): return self.default_value or "unknown" # Convert to string key - if hasattr(value, "id"): - # Many2one - use code or display_name for meaningful keys + if hasattr(value, "_name"): + # Odoo recordset (Many2one). Empty recordset means the field is unset. + if not value: + return self.default_value or "unknown" if hasattr(value, "code") and value.code: key = str(value.code) else: @@ -211,28 +225,70 @@ def get_label_for_value(self, value): """ Get the display label for a dimension value. + First checks value_labels_json for a static mapping. If not found and + the dimension is field-based pointing to a Many2one, dynamically looks + up the display_name from the related model. + :param value: The raw dimension value :returns: Display label :rtype: str """ self.ensure_one() - if not self.value_labels_json: - return value - - # Handle case where value_labels_json is a string (JSON not yet parsed) - labels = self.value_labels_json - if isinstance(labels, str): - try: - labels = json.loads(labels) - except (json.JSONDecodeError, TypeError): - return value - - # Convert value to string for lookup (keys are strings in JSON) - str_value = str(value) if value is not None else "null" - if str_value in labels: - return labels[str_value] + + # Check static labels first + if self.value_labels_json: + labels = self.value_labels_json + if isinstance(labels, str): + try: + labels = json.loads(labels) + except (json.JSONDecodeError, TypeError): + labels = {} + + str_value = str(value) if value is not None else "null" + if str_value in labels: + return labels[str_value] + + # For field-based dimensions pointing to a Many2one, try dynamic lookup + if self.dimension_type == "field" and self.field_path: + label = self._lookup_m2o_label(value) + if label: + return label + return value + def _lookup_m2o_label(self, raw_value): + """Look up display_name for a Many2one field-based dimension value. + + When field_path points to a Many2one (e.g. area_id, gender_id), the raw + value is typically the record's code. This method searches the related + model by code to return the display_name. + + :param raw_value: The raw dimension value (typically a code string) + :returns: display_name or None if not found + """ + # Only handle simple field paths (one segment, no dotted traversal + # like "gender_id.code" where the user explicitly chose a sub-field) + if "." in self.field_path: + return None + + field_name = self.field_path + partner_fields = self.env["res.partner"]._fields + if field_name not in partner_fields: + return None + + field = partner_fields[field_name] + if field.type != "many2one": + return None + + comodel = self.env[field.comodel_name] + # Search by code if the related model has a code field + if "code" in comodel._fields: + record = comodel.search([("code", "=", raw_value)], limit=1) + if record: + return record.display_name + + return None + @api.model def get_by_name(self, name): """ @@ -260,6 +316,176 @@ def get_active_dimensions(self, applies_to=None): domain.append(("applies_to", "=", applies_to)) return self.search(domain, order="sequence, name") + # ------------------------------------------------------------------------- + # SQL Column Generation + # ------------------------------------------------------------------------- + def to_sql_column(self, alias="ind", alias_counter=0): + """Generate a SQL expression for this dimension's value. + + Compiles this dimension to a SQL expression that can be used in a + SELECT clause. For field-based dimensions, generates column references + (with JOINs for Many2one). For CEL expression dimensions, delegates to + the CEL-to-SQL compiler. + + Args: + alias: SQL alias for the res_partner table (default "ind") + alias_counter: Counter for generating unique join aliases + + Returns: + SQLColumnResult | None: SQL expression + joins, or None if + SQL compilation is not possible (fall back to Python). + """ + self.ensure_one() + default = self.default_value or "unknown" + + if self.dimension_type == "field": + result = self._to_sql_column_field(alias, alias_counter, default) + elif self.dimension_type == "expression": + result = self._to_sql_column_expression(alias, alias_counter, default) + else: + return None + + if result is None: + return None + + # Wrap with applies_to filter: return default for non-matching registrants + if self.applies_to == "individuals": + result = SQLColumnResult( + expression=SQL( + "CASE WHEN %s.%s = FALSE THEN %s ELSE %s END", + SQL.identifier(alias), + SQL.identifier("is_group"), + result.expression, + SQL("%s", default), + ), + joins=result.joins, + alias_counter=result.alias_counter, + ) + elif self.applies_to == "groups": + result = SQLColumnResult( + expression=SQL( + "CASE WHEN %s.%s = TRUE THEN %s ELSE %s END", + SQL.identifier(alias), + SQL.identifier("is_group"), + result.expression, + SQL("%s", default), + ), + joins=result.joins, + alias_counter=result.alias_counter, + ) + + return result + + def _to_sql_column_field(self, alias, alias_counter, default): + """Generate SQL for a field-based dimension.""" + if not self.field_path: + return None + + parts = self.field_path.split(".") + partner_fields = self.env["res.partner"]._fields + + if len(parts) == 1: + # Simple field (e.g., is_group, area_id) + field_name = parts[0] + if field_name not in partner_fields: + return None + + field_def = partner_fields[field_name] + if field_def.type == "many2one": + # Many2one direct: use code from related model if available + return self._to_sql_column_m2o_direct(alias, alias_counter, field_name, field_def, default) + else: + # Scalar field: CAST to text + col = SQL("%s.%s", SQL.identifier(alias), SQL.identifier(field_name)) + expr = SQL("COALESCE(CAST(%s AS TEXT), %s)", col, default) + return SQLColumnResult(expression=expr, alias_counter=alias_counter) + + elif len(parts) == 2: + # Dotted path (e.g., gender_id.code) + field_name, sub_field = parts + if field_name not in partner_fields: + return None + + field_def = partner_fields[field_name] + if field_def.type != "many2one": + return None + + return self._to_sql_column_m2o_sub(alias, alias_counter, field_name, field_def, sub_field, default) + + # Deeper paths not supported in SQL + return None + + def _to_sql_column_m2o_direct(self, alias, alias_counter, field_name, field_def, default): + """Generate SQL for a direct Many2one field (e.g., gender_id, area_id). + + JOINs to the comodel and uses code if available, otherwise id as text. + """ + comodel_name = field_def.comodel_name + comodel = self.env[comodel_name] + join_alias = f"_dim{alias_counter}" + comodel_table = comodel._table + + fk_col = SQL("%s.%s", SQL.identifier(alias), SQL.identifier(field_name)) + join_id = SQL("%s.id", SQL.identifier(join_alias)) + join_sql = SQL( + "LEFT JOIN %s %s ON %s = %s", + SQL.identifier(comodel_table), + SQL.identifier(join_alias), + join_id, + fk_col, + ) + + if "code" in comodel._fields: + code_col = SQL("%s.%s", SQL.identifier(join_alias), SQL.identifier("code")) + expr = SQL("COALESCE(CAST(%s AS TEXT), %s)", code_col, default) + else: + id_col = SQL("%s.id", SQL.identifier(join_alias)) + expr = SQL("COALESCE(CAST(%s AS TEXT), %s)", id_col, default) + + return SQLColumnResult(expression=expr, joins=[join_sql], alias_counter=alias_counter + 1) + + def _to_sql_column_m2o_sub(self, alias, alias_counter, field_name, field_def, sub_field, default): + """Generate SQL for a Many2one dotted path (e.g., gender_id.code).""" + comodel_name = field_def.comodel_name + comodel = self.env[comodel_name] + join_alias = f"_dim{alias_counter}" + comodel_table = comodel._table + + if sub_field not in comodel._fields: + return None + + fk_col = SQL("%s.%s", SQL.identifier(alias), SQL.identifier(field_name)) + join_id = SQL("%s.id", SQL.identifier(join_alias)) + join_sql = SQL( + "LEFT JOIN %s %s ON %s = %s", + SQL.identifier(comodel_table), + SQL.identifier(join_alias), + join_id, + fk_col, + ) + + sub_col = SQL("%s.%s", SQL.identifier(join_alias), SQL.identifier(sub_field)) + expr = SQL("COALESCE(CAST(%s AS TEXT), %s)", sub_col, default) + + return SQLColumnResult(expression=expr, joins=[join_sql], alias_counter=alias_counter + 1) + + def _to_sql_column_expression(self, alias, alias_counter, default): + """Generate SQL for a CEL expression-based dimension.""" + if not self.cel_expression: + return None + + try: + translator = self.env["spp.cel.translator"] + except KeyError: + return None + + sql_expr = translator.to_sql_case(self.cel_expression, "res.partner", alias) + if sql_expr is None: + return None + + expr = SQL("COALESCE(CAST(%s AS TEXT), %s)", sql_expr, default) + return SQLColumnResult(expression=expr, alias_counter=alias_counter) + # ------------------------------------------------------------------------- # Cache Invalidation # ------------------------------------------------------------------------- diff --git a/spp_metric_service/readme/DESCRIPTION.md b/spp_metric_service/readme/DESCRIPTION.md index 13cad030..40f6fd6c 100644 --- a/spp_metric_service/readme/DESCRIPTION.md +++ b/spp_metric_service/readme/DESCRIPTION.md @@ -16,11 +16,11 @@ dashboards. No standalone UI; provides only programmatic service models. | Model | Description | | -------------------------------- | ---------------------------------------------------- | | `spp.demographic.dimension` | Configurable dimension for breakdowns (field or CEL) | -| `spp.metrics.fairness` | Abstract service: equity/parity analysis | -| `spp.metrics.distribution` | Abstract service: distribution statistics | -| `spp.metrics.breakdown` | Abstract service: multi-dimensional grouping | -| `spp.metrics.privacy` | Abstract service: k-anonymity enforcement | -| `spp.metrics.dimension.cache` | Abstract service: dimension evaluation cache | +| `spp.metric.fairness` | Abstract service: equity/parity analysis | +| `spp.metric.distribution` | Abstract service: distribution statistics | +| `spp.metric.breakdown` | Abstract service: multi-dimensional grouping | +| `spp.metric.privacy` | Abstract service: k-anonymity enforcement | +| `spp.metric.dimension.cache` | Abstract service: dimension evaluation cache | ### Configuration @@ -44,9 +44,9 @@ by `spp_aggregation`. ### Extension Points -- Override `_analyze_dimension()` in `spp.metrics.fairness` for custom analysis logic +- Override `_analyze_dimension()` in `spp.metric.fairness` for custom analysis logic - Add new dimension types by extending `spp.demographic.dimension` -- Override `enforce()` in `spp.metrics.privacy` for custom suppression strategies +- Override `enforce()` in `spp.metric.privacy` for custom suppression strategies ### Dependencies diff --git a/spp_metric_service/readme/HISTORY.md b/spp_metric_service/readme/HISTORY.md index 4aaf9afe..c3b826bb 100644 --- a/spp_metric_service/readme/HISTORY.md +++ b/spp_metric_service/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.1.0 + +- feat: demographic breakdown expansion and SQL column support for metric disaggregation (re-land from #76; uses the spp_cel_domain SQL CASE compiler). + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_metric_service/tests/__init__.py b/spp_metric_service/tests/__init__.py index 0c7a9d58..ba8bf44a 100644 --- a/spp_metric_service/tests/__init__.py +++ b/spp_metric_service/tests/__init__.py @@ -3,3 +3,5 @@ from . import test_services from . import test_dimension_cache from . import test_coverage +from . import test_sql_column +from . import test_breakdown_expansion diff --git a/spp_metric_service/tests/test_breakdown_expansion.py b/spp_metric_service/tests/test_breakdown_expansion.py new file mode 100644 index 00000000..94449289 --- /dev/null +++ b/spp_metric_service/tests/test_breakdown_expansion.py @@ -0,0 +1,129 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for BreakdownService group-to-member expansion.""" + +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestBreakdownExpansion(TransactionCase): + """Test that BreakdownService expands groups to members for individual-level dimensions.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + + cls.breakdown = cls.env["spp.metric.breakdown"] + cls.dim_model = cls.env["spp.demographic.dimension"] + + # Create a dimension that applies to individuals only + cls.gender_dim = cls.dim_model.search([("name", "=", "gender")], limit=1) + if not cls.gender_dim: + cls.gender_dim = cls.dim_model.create( + { + "name": "gender", + "label": "Gender", + "dimension_type": "field", + "field_path": "gender_id.code", + "applies_to": "individuals", + "default_value": "unknown", + } + ) + else: + # Ensure applies_to is set for this test + cls.gender_dim.applies_to = "individuals" + + # Create a dimension that applies to all + cls.type_dim = cls.dim_model.search([("name", "=", "registrant_type")], limit=1) + if not cls.type_dim: + cls.type_dim = cls.dim_model.create( + { + "name": "registrant_type", + "label": "Registrant Type", + "dimension_type": "field", + "field_path": "is_group", + "applies_to": "all", + "default_value": "unknown", + } + ) + + # Create a group with two individual members + cls.group = cls.env["res.partner"].create( + { + "name": "Expansion Test Group", + "is_registrant": True, + "is_group": True, + } + ) + + cls.member1 = cls.env["res.partner"].create( + { + "name": "Member 1", + "is_registrant": True, + "is_group": False, + } + ) + cls.member2 = cls.env["res.partner"].create( + { + "name": "Member 2", + "is_registrant": True, + "is_group": False, + } + ) + + # Create memberships + cls.env["spp.group.membership"].create({"group": cls.group.id, "individual": cls.member1.id}) + cls.env["spp.group.membership"].create({"group": cls.group.id, "individual": cls.member2.id}) + + def test_expansion_with_individual_dimension(self): + """Passing group IDs with individual-level dimensions expands to members.""" + result = self.breakdown.compute_breakdown([self.group.id], ["gender"]) + + # Should have breakdown entries (from the 2 members, not the 1 group) + total = sum(cell["count"] for cell in result.values()) + self.assertEqual(total, 2, "Should count 2 individual members, not 1 group") + + def test_no_expansion_with_all_dimension(self): + """Passing group IDs with applies_to='all' dimensions does NOT expand.""" + result = self.breakdown.compute_breakdown([self.group.id], ["registrant_type"]) + + total = sum(cell["count"] for cell in result.values()) + self.assertEqual(total, 1, "Should count 1 group record without expansion") + + def test_mixed_group_and_individual_ids(self): + """Mixed group + individual IDs: groups expand, individuals pass through.""" + result = self.breakdown.compute_breakdown( + [self.group.id, self.member1.id], + ["gender"], + ) + + # group expands to member1 + member2, plus member1 directly = 3 IDs + # but member1 appears twice, so after dedup = 2 unique individuals + total = sum(cell["count"] for cell in result.values()) + self.assertEqual(total, 2, "Duplicates from expansion should be deduplicated") + + def test_empty_group_no_phantom_entries(self): + """Group with no active members produces no entries.""" + empty_group = self.env["res.partner"].create( + { + "name": "Empty Group", + "is_registrant": True, + "is_group": True, + } + ) + result = self.breakdown.compute_breakdown([empty_group.id], ["gender"]) + total = sum(cell["count"] for cell in result.values()) + self.assertEqual(total, 0, "Empty group should produce no breakdown entries") + + def test_existing_tests_unaffected(self): + """Empty group_by still returns empty dict.""" + result = self.breakdown.compute_breakdown([self.group.id], []) + self.assertEqual(result, {}) + + def test_individual_ids_with_individual_dimension(self): + """Passing individual IDs with individual-level dimensions works without expansion.""" + result = self.breakdown.compute_breakdown( + [self.member1.id, self.member2.id], + ["gender"], + ) + total = sum(cell["count"] for cell in result.values()) + self.assertEqual(total, 2, "Individual IDs should pass through without expansion") diff --git a/spp_metric_service/tests/test_coverage.py b/spp_metric_service/tests/test_coverage.py index 16ed9861..c8afd478 100644 --- a/spp_metric_service/tests/test_coverage.py +++ b/spp_metric_service/tests/test_coverage.py @@ -299,6 +299,62 @@ def test_evaluate_for_record_error_no_default(self): # get_label_for_value # ------------------------------------------------------------------------- + def test_evaluate_field_empty_many2one_returns_default(self): + """Empty Many2one (unset field) returns default_value, not 'False'.""" + dim = self.dim_model.create( + { + "name": "test_empty_m2o", + "label": "Empty M2O", + "dimension_type": "field", + "field_path": "company_id", + "applies_to": "all", + "default_value": "unset", + } + ) + # Individual with company_id explicitly cleared + partner = self.partner_model.create( + { + "name": "No Company", + "is_registrant": True, + "is_group": False, + "company_id": False, + } + ) + result = dim.evaluate_for_record(partner) + self.assertEqual(result, "unset") + + def test_get_label_for_value_m2o_dynamic_lookup(self): + """get_label_for_value dynamically looks up Many2one display_name.""" + # Use company_id which doesn't have a 'code' field on res.company + dim = self.dim_model.create( + { + "name": "test_m2o_label_lookup", + "label": "Area Label", + "dimension_type": "field", + "field_path": "company_id", + "applies_to": "all", + } + ) + # For a field without code, dynamic lookup won't match + # (res.company has no code field), so it falls back to raw value + result = dim.get_label_for_value("some_code") + self.assertEqual(result, "some_code") + + def test_get_label_for_value_static_takes_precedence(self): + """Static value_labels_json takes precedence over dynamic lookup.""" + dim = self.dim_model.create( + { + "name": "test_static_precedence", + "label": "Static", + "dimension_type": "field", + "field_path": "company_id", + "applies_to": "all", + "value_labels_json": {"MY_CODE": "My Display Name"}, + } + ) + result = dim.get_label_for_value("MY_CODE") + self.assertEqual(result, "My Display Name") + def test_get_label_for_value_with_json_mapping(self): """Value label lookup from JSON mapping.""" dim = self.dim_model.create( diff --git a/spp_metric_service/tests/test_sql_column.py b/spp_metric_service/tests/test_sql_column.py new file mode 100644 index 00000000..d00a5bef --- /dev/null +++ b/spp_metric_service/tests/test_sql_column.py @@ -0,0 +1,155 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for DemographicDimension.to_sql_column() SQL compilation.""" + +from odoo.tests import TransactionCase, tagged +from odoo.tools.sql import SQL + +from ..models.demographic_dimension import SQLColumnResult + + +@tagged("post_install", "-at_install") +class TestDimensionSqlColumn(TransactionCase): + """Test to_sql_column() for field-based and expression-based dimensions.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + Dim = cls.env["spp.demographic.dimension"] + + # Simple boolean field + cls.dim_is_group = Dim.create( + { + "name": "test_is_group", + "label": "Is Group", + "dimension_type": "field", + "field_path": "is_group", + "default_value": "unknown", + } + ) + + # Many2one with .code path (gender_id.code) + cls.dim_gender_code = Dim.create( + { + "name": "test_gender_code", + "label": "Gender Code", + "dimension_type": "field", + "field_path": "gender_id.code", + "default_value": "unknown", + } + ) + + # Many2one direct (gender_id) + cls.dim_gender_direct = Dim.create( + { + "name": "test_gender_direct", + "label": "Gender Direct", + "dimension_type": "field", + "field_path": "gender_id", + "default_value": "unknown", + } + ) + + # CEL expression dimension + cls.dim_age_group = Dim.create( + { + "name": "test_age_group", + "label": "Age Group", + "dimension_type": "expression", + "cel_expression": ( + 'r.birthdate == null ? "unknown" : ' + 'age_years(r.birthdate) < 18 ? "child" : ' + 'age_years(r.birthdate) < 60 ? "adult" : "elderly"' + ), + "default_value": "unknown", + } + ) + + def test_field_simple_returns_sql(self): + """Test simple field produces CAST + COALESCE SQL.""" + result = self.dim_is_group.to_sql_column("p", 0) + self.assertIsNotNone(result) + self.assertIsInstance(result, SQLColumnResult) + self.assertIsInstance(result.expression, SQL) + self.assertEqual(result.joins, []) + sql_str = str(result.expression) + self.assertIn("COALESCE", sql_str) + self.assertIn("CAST", sql_str) + self.assertIn("is_group", sql_str) + + def test_field_m2o_code_path_produces_join(self): + """Test M2O dotted path (gender_id.code) produces LEFT JOIN.""" + result = self.dim_gender_code.to_sql_column("ind", 0) + self.assertIsNotNone(result) + self.assertEqual(len(result.joins), 1) + join_str = str(result.joins[0]) + self.assertIn("LEFT JOIN", join_str) + sql_str = str(result.expression) + self.assertIn("code", sql_str) + # alias_counter should be incremented + self.assertEqual(result.alias_counter, 1) + + def test_field_m2o_direct_produces_join(self): + """Test direct M2O field (gender_id) produces LEFT JOIN with code lookup.""" + result = self.dim_gender_direct.to_sql_column("ind", 0) + self.assertIsNotNone(result) + self.assertEqual(len(result.joins), 1) + join_str = str(result.joins[0]) + self.assertIn("LEFT JOIN", join_str) + + def test_expression_produces_case_when(self): + """Test CEL expression dimension compiles to CASE WHEN SQL.""" + result = self.dim_age_group.to_sql_column("ind", 0) + self.assertIsNotNone(result) + self.assertIsInstance(result, SQLColumnResult) + sql_str = str(result.expression) + self.assertIn("CASE WHEN", sql_str) + self.assertIn("COALESCE", sql_str) + + def test_unsupported_expression_returns_none(self): + """Test unsupported CEL expression returns None for fallback.""" + dim = self.env["spp.demographic.dimension"].create( + { + "name": "test_unsupported", + "label": "Unsupported", + "dimension_type": "expression", + "cel_expression": "r.income + r.bonus", + "default_value": "n/a", + } + ) + result = dim.to_sql_column("ind", 0) + self.assertIsNone(result) + + def test_invalid_field_path_returns_none(self): + """Test invalid field path returns None.""" + dim = self.env["spp.demographic.dimension"].create( + { + "name": "test_invalid_field", + "label": "Invalid Field", + "dimension_type": "field", + "field_path": "nonexistent_field", + "default_value": "n/a", + } + ) + result = dim.to_sql_column("ind", 0) + self.assertIsNone(result) + + def test_alias_counter_increments(self): + """Test multiple dimensions get unique join aliases.""" + r1 = self.dim_gender_code.to_sql_column("ind", 0) + r2 = self.dim_gender_direct.to_sql_column("ind", r1.alias_counter) + self.assertNotEqual(r1.alias_counter, r2.alias_counter) + # Join aliases should differ + join1_str = str(r1.joins[0]) + join2_str = str(r2.joins[0]) + self.assertIn("_dim0", join1_str) + self.assertIn("_dim1", join2_str) + + def test_cross_module_cel_call(self): + """Test to_sql_column works cross-module (spp_metric_service calling spp_cel_domain).""" + # This is the key cross-module integration test + result = self.dim_age_group.to_sql_column("t", 5) + self.assertIsNotNone(result) + sql_str = str(result.expression) + # Should reference the correct alias + self.assertIn("t", sql_str) + self.assertIn("birthdate", sql_str) From cecd70127e777c1825142be3d01bfd89aa28cf29 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 3 Jul 2026 02:32:16 +0800 Subject: [PATCH 2/6] perf(spp_metric_service): address gemini-code-assist review findings - recordset subtraction instead of lambda filter for individuals - batch individual ids from memberships (avoids N+1) - check _fields membership instead of hasattr on recordsets - early return for falsy raw_value before code search --- spp_metric_service/models/breakdown_service.py | 8 ++++---- spp_metric_service/models/demographic_dimension.py | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/spp_metric_service/models/breakdown_service.py b/spp_metric_service/models/breakdown_service.py index 89e5d94b..9144cef7 100644 --- a/spp_metric_service/models/breakdown_service.py +++ b/spp_metric_service/models/breakdown_service.py @@ -122,8 +122,9 @@ def _expand_groups_to_members(self, registrant_ids): Partner = self.env["res.partner"].sudo() # nosemgrep: odoo-sudo-without-context,odoo-sudo-on-sensitive-models records = Partner.browse(registrant_ids).exists() - group_ids = records.filtered("is_group").ids - individual_ids = set(records.filtered(lambda r: not r.is_group).ids) + groups = records.filtered("is_group") + group_ids = groups.ids + individual_ids = set((records - groups).ids) if not group_ids: return list(individual_ids) @@ -137,7 +138,6 @@ def _expand_groups_to_members(self, registrant_ids): ] ) - for membership in memberships: - individual_ids.add(membership.individual.id) + individual_ids.update(memberships.individual.ids) return list(individual_ids) diff --git a/spp_metric_service/models/demographic_dimension.py b/spp_metric_service/models/demographic_dimension.py index 86d5efa0..29bfd728 100644 --- a/spp_metric_service/models/demographic_dimension.py +++ b/spp_metric_service/models/demographic_dimension.py @@ -189,7 +189,7 @@ def _evaluate_field(self, record): # Odoo recordset (Many2one). Empty recordset means the field is unset. if not value: return self.default_value or "unknown" - if hasattr(value, "code") and value.code: + if "code" in value._fields and value.code: key = str(value.code) else: key = value.display_name or str(value.id) @@ -266,6 +266,9 @@ def _lookup_m2o_label(self, raw_value): :param raw_value: The raw dimension value (typically a code string) :returns: display_name or None if not found """ + if not raw_value: + return None + # Only handle simple field paths (one segment, no dotted traversal # like "gender_id.code" where the user explicitly chose a sub-field) if "." in self.field_path: From 9b484ee45e6038bd9e1d62b0361f5bd9310b743b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 3 Jul 2026 17:56:38 +0800 Subject: [PATCH 3/6] style(spp_metric_service): pass default directly as SQL parameter (gemini review) --- spp_metric_service/models/demographic_dimension.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spp_metric_service/models/demographic_dimension.py b/spp_metric_service/models/demographic_dimension.py index 29bfd728..e0c8ee81 100644 --- a/spp_metric_service/models/demographic_dimension.py +++ b/spp_metric_service/models/demographic_dimension.py @@ -359,7 +359,7 @@ def to_sql_column(self, alias="ind", alias_counter=0): SQL.identifier(alias), SQL.identifier("is_group"), result.expression, - SQL("%s", default), + default, ), joins=result.joins, alias_counter=result.alias_counter, @@ -371,7 +371,7 @@ def to_sql_column(self, alias="ind", alias_counter=0): SQL.identifier(alias), SQL.identifier("is_group"), result.expression, - SQL("%s", default), + default, ), joins=result.joins, alias_counter=result.alias_counter, From 34f0b9f9a7e65aa94a386264c227524556349613 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Mon, 6 Jul 2026 15:23:27 +0800 Subject: [PATCH 4/6] docs(spp_metric_service): regenerate README with the 19.0.2.1.0 HISTORY section --- spp_metric_service/README.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spp_metric_service/README.rst b/spp_metric_service/README.rst index 9fa8846d..87eb7d62 100644 --- a/spp_metric_service/README.rst +++ b/spp_metric_service/README.rst @@ -115,6 +115,13 @@ Dependencies Changelog ========= +19.0.2.1.0 +~~~~~~~~~~ + +- feat: demographic breakdown expansion and SQL column support for + metric disaggregation (re-land from #76; uses the spp_cel_domain SQL + CASE compiler). + 19.0.2.0.0 ~~~~~~~~~~ From c7ab7ad1e2889605d81703fcef7f927e462bf3f4 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 14 Jul 2026 08:57:27 +0800 Subject: [PATCH 5/6] test+docs(spp_metric_service): assert applies_to CASE wrapping; document expansion semantics Addresses the review's two verify-items and its test gap: - two tests assert the CASE WHEN is_group = FALSE/TRUE wrapping for individuals-/groups-scoped dimensions (previously unexercised); - compute_breakdown docstring and HISTORY state the endorsed semantics: all-or-nothing expansion when any individuals dimension is present, and member-counted totals that need not reconcile with group-level scope counts. --- .../models/breakdown_service.py | 9 +++++ spp_metric_service/readme/HISTORY.md | 2 +- spp_metric_service/tests/test_sql_column.py | 39 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/spp_metric_service/models/breakdown_service.py b/spp_metric_service/models/breakdown_service.py index 9144cef7..8512fe45 100644 --- a/spp_metric_service/models/breakdown_service.py +++ b/spp_metric_service/models/breakdown_service.py @@ -24,6 +24,15 @@ def compute_breakdown(self, registrant_ids, group_by, statistics=None, context=N Uses dimension cache for 5-10x performance improvement. + Expansion semantics (deliberate): if ANY requested dimension has + ``applies_to == "individuals"``, the ENTIRE registrant set is expanded + from groups to their active individual members before evaluation — + including for any non-individual dimensions mixed into the same + request (mixing scopes in one breakdown is inherently ambiguous; the + expansion is all-or-nothing by design). Consequently breakdown totals + count members and intentionally need not reconcile with a group-level + scope count. + :param registrant_ids: List of partner IDs :param group_by: List of dimension names :param statistics: List of statistic names (optional) diff --git a/spp_metric_service/readme/HISTORY.md b/spp_metric_service/readme/HISTORY.md index c3b826bb..af36ad6e 100644 --- a/spp_metric_service/readme/HISTORY.md +++ b/spp_metric_service/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.1.0 -- feat: demographic breakdown expansion and SQL column support for metric disaggregation (re-land from #76; uses the spp_cel_domain SQL CASE compiler). +- feat: demographic breakdown expansion and SQL column support for metric disaggregation (re-land from #76; uses the spp_cel_domain SQL CASE compiler). Expansion is all-or-nothing: any individuals-scoped dimension expands the whole registrant set to active members, so breakdown totals count members and need not reconcile with a group-level scope count. ### 19.0.2.0.0 diff --git a/spp_metric_service/tests/test_sql_column.py b/spp_metric_service/tests/test_sql_column.py index d00a5bef..f68978d7 100644 --- a/spp_metric_service/tests/test_sql_column.py +++ b/spp_metric_service/tests/test_sql_column.py @@ -153,3 +153,42 @@ def test_cross_module_cel_call(self): # Should reference the correct alias self.assertIn("t", sql_str) self.assertIn("birthdate", sql_str) + + def test_applies_to_individuals_wraps_in_is_group_false_case(self): + """An individuals-scoped dimension wraps its SQL in CASE WHEN is_group = FALSE.""" + dim = self.env["spp.demographic.dimension"].create( + { + "name": "test_individuals_scope", + "label": "Individuals Scope", + "dimension_type": "field", + "field_path": "is_group", + "default_value": "unknown", + "applies_to": "individuals", + } + ) + result = dim.to_sql_column("p", 0) + self.assertIsNotNone(result) + sql_str = str(result.expression) + self.assertIn("CASE WHEN", sql_str) + self.assertIn("is_group", sql_str) + self.assertIn("= FALSE", sql_str) + # Non-matching registrants fall through to the dimension default + self.assertIn("unknown", str(result.expression.params)) + + def test_applies_to_groups_wraps_in_is_group_true_case(self): + """A groups-scoped dimension wraps its SQL in CASE WHEN is_group = TRUE.""" + dim = self.env["spp.demographic.dimension"].create( + { + "name": "test_groups_scope", + "label": "Groups Scope", + "dimension_type": "field", + "field_path": "is_group", + "default_value": "unknown", + "applies_to": "groups", + } + ) + result = dim.to_sql_column("p", 0) + self.assertIsNotNone(result) + sql_str = str(result.expression) + self.assertIn("CASE WHEN", sql_str) + self.assertIn("= TRUE", sql_str) From a8b8f0844c8a7eafa1adc81c9730eeea6016b064 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Tue, 14 Jul 2026 09:01:35 +0800 Subject: [PATCH 6/6] docs(spp_metric_service): re-render README for the amended HISTORY entry --- spp_metric_service/README.rst | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spp_metric_service/README.rst b/spp_metric_service/README.rst index 87eb7d62..3d845db9 100644 --- a/spp_metric_service/README.rst +++ b/spp_metric_service/README.rst @@ -120,7 +120,10 @@ Changelog - feat: demographic breakdown expansion and SQL column support for metric disaggregation (re-land from #76; uses the spp_cel_domain SQL - CASE compiler). + CASE compiler). Expansion is all-or-nothing: any individuals-scoped + dimension expands the whole registrant set to active members, so + breakdown totals count members and need not reconcile with a + group-level scope count. 19.0.2.0.0 ~~~~~~~~~~