From e745df5c19be72907b9bad1ce070e1a0e61877c3 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:16:10 +0800 Subject: [PATCH 01/12] refactor: code quality and correctness fixes for spp_cel_event stable release - Remove unused EventsCollection dataclass (dead code) - Consolidate duplicate _parse_period into wrapper around shared parse_period() - Fix f-string logging to use lazy %s formatting - Move import re to top-level in executor and translator - Fix fragile relative import to use odoo.addons path - Add missing copyright header to cel_variable_event_agg.py - Add security invariant comment on _validate_field_name regex - Add explicit comment on empty states list behavior - Narrow broad except Exception to specific types in _compare_value - Add Python fallback size warning for large candidate sets (>10k) - Remove unsupported 'where' parameter from cel_profiles.yaml signatures - Fix _compute_cel_expression to call super() and only override events case --- spp_cel_event/data/cel_profiles.yaml | 47 ++++--------- spp_cel_event/models/cel_event_executor.py | 66 +++++++---------- spp_cel_event/models/cel_event_functions.py | 6 +- spp_cel_event/models/cel_event_queryplan.py | 70 ------------------- spp_cel_event/models/cel_event_translator.py | 5 +- .../models/cel_variable_event_agg.py | 16 ++--- 6 files changed, 51 insertions(+), 159 deletions(-) diff --git a/spp_cel_event/data/cel_profiles.yaml b/spp_cel_event/data/cel_profiles.yaml index 82655e5b..e4717085 100644 --- a/spp_cel_event/data/cel_profiles.yaml +++ b/spp_cel_event/data/cel_profiles.yaml @@ -142,7 +142,7 @@ presets: - name: events_count signature: "events_count(type, after?, before?, within_days?, within_months?, period?, - states?, where?)" + states?)" description: "Count events matching the criteria" returns: integer parameters: @@ -174,17 +174,12 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - - name: where - type: str - required: false - description: "CEL predicate to filter events by field values" + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_count('attendance') >= 180" description: "Count all active attendance events" - expression: "events_count('attendance', period='2024') >= 180" description: "Count attendance in 2024" - - expression: "events_count('attendance', where='attended == true') >= 150" - description: "Count events where attended is true" - expression: "events_count('visit', within_days=365) >= 4" description: "At least 4 visits in last year" @@ -195,7 +190,7 @@ presets: - name: events_sum signature: "events_sum(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Sum numeric field values across matching events" returns: number parameters: @@ -231,10 +226,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - - name: where - type: str - required: false - description: "CEL predicate to filter events" + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_sum('attendance', 'days_present', period='2024') >= 180" description: "Sum attendance days in 2024" @@ -244,7 +236,7 @@ presets: - name: events_avg signature: "events_avg(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Calculate average of numeric field values across matching events" returns: number parameters: @@ -280,10 +272,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - - name: where - type: str - required: false - description: "CEL predicate to filter events" + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_avg('survey', 'income', within_days=365) < 500" description: "Average income from surveys in last year" @@ -293,7 +282,7 @@ presets: - name: events_min signature: "events_min(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Find minimum value of numeric field across matching events" returns: number parameters: @@ -329,10 +318,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - - name: where - type: str - required: false - description: "CEL predicate to filter events" + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_min('assessment', 'score', period='2024') >= 60" description: "Minimum assessment score in 2024" @@ -342,7 +328,7 @@ presets: - name: events_max signature: "events_max(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Find maximum value of numeric field across matching events" returns: number parameters: @@ -378,10 +364,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - - name: where - type: str - required: false - description: "CEL predicate to filter events" + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_max('assessment', 'score', period='2024') >= 70" description: "Maximum assessment score in 2024" @@ -541,35 +524,35 @@ presets: - name: events_count signature: "events_count(type, after?, before?, within_days?, within_months?, period?, - states?, where?)" + states?)" description: "Count events matching criteria for group registrant" returns: integer - name: events_sum signature: "events_sum(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Sum numeric field across group registrant's events" returns: number - name: events_avg signature: "events_avg(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Average numeric field across group registrant's events" returns: number - name: events_min signature: "events_min(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Minimum value across group registrant's events" returns: number - name: events_max signature: "events_max(type, field, after?, before?, within_days?, within_months?, - period?, states?, where?)" + period?, states?)" description: "Maximum value across group registrant's events" returns: number diff --git a/spp_cel_event/models/cel_event_executor.py b/spp_cel_event/models/cel_event_executor.py index 0da656da..fd656830 100644 --- a/spp_cel_event/models/cel_event_executor.py +++ b/spp_cel_event/models/cel_event_executor.py @@ -9,12 +9,14 @@ from __future__ import annotations import logging +import re from datetime import date, timedelta from typing import Any from odoo import models from odoo.tools.sql import SQL +from . import cel_event_functions as event_funcs from .cel_event_queryplan import EventExists, EventsAggregate, EventValueCompare _logger = logging.getLogger(__name__) @@ -198,6 +200,13 @@ def _exec_event_value_python(self, model: str, plan: EventValueCompare) -> list[ base_domain = cfg.get("base_domain", []) candidate_ids = self.env[model].search(base_domain).ids + if len(candidate_ids) > 10000: + _logger.warning( + "[CEL EVENT] Python fallback path invoked for %d candidates. " + "This will be slow. Consider simplifying the expression to use SQL path.", + len(candidate_ids), + ) + matching_ids = [] for partner_id in candidate_ids: @@ -433,6 +442,13 @@ def _exec_event_aggregate_python(self, model: str, plan: EventsAggregate) -> lis base_domain = cfg.get("base_domain", []) candidate_ids = self.env[model].search(base_domain).ids + if len(candidate_ids) > 10000: + _logger.warning( + "[CEL EVENT] Python fallback path invoked for %d candidates. " + "This will be slow. Consider simplifying the expression to use SQL path.", + len(candidate_ids), + ) + matching_ids = [] for partner_id in candidate_ids: @@ -536,6 +552,8 @@ def _build_state_filter(self, plan: EventValueCompare | EventExists | EventsAggr Returns: Tuple of (sql_clause, args_list) """ + # An empty list [] is falsy, so it falls through to defaults — this is intentional. + # Only an explicitly populated list triggers the custom state filter. if plan.states: # Explicit states placeholders = ", ".join(["%s"] * len(plan.states)) @@ -784,7 +802,7 @@ def _compare_value(self, a: Any, op: str, b: Any) -> bool: "!=": lambda x, y: x != y, } return ops.get(op, lambda x, y: False)(str(a), str(b)) - except Exception: + except (ValueError, TypeError, KeyError): return False # ══════════════════════════════════════════════════════════════════════════════ @@ -803,12 +821,12 @@ def _validate_field_name(self, field_name: str) -> str: Raises: ValueError: If field name is invalid """ - import re - if not field_name or not isinstance(field_name, str): raise ValueError("Field name must be a non-empty string") - # Only allow valid identifier characters + # SECURITY: This regex is the sole barrier against SQL injection for field names + # interpolated into SQL queries via _build_field_comparison_sql and _exec_event_aggregate_sql. + # Do not relax this pattern without also changing those methods to use parameterized field names. if not re.match(r"^[a-zA-Z][a-zA-Z0-9_]*$", field_name): raise ValueError(f"Invalid field name: {field_name!r}. Only alphanumeric and underscore allowed.") @@ -859,42 +877,10 @@ def _parse_period(self, period: str) -> tuple[date | None, date | None]: period: Period string (e.g., '2024', '2024-Q1', '2024-03') Returns: - Tuple of (start_date, end_date) + Tuple of (start_date, end_date), or (None, None) on parse failure. """ try: - # Full year: YYYY - if len(period) == 4 and period.isdigit(): - year = int(period) - return date(year, 1, 1), date(year, 12, 31) - - # Quarter: YYYY-QN - if len(period) == 7 and period[4] == "-" and period[5] == "Q": - year = int(period[:4]) - quarter = int(period[6]) - if quarter == 1: - return date(year, 1, 1), date(year, 3, 31) - elif quarter == 2: - return date(year, 4, 1), date(year, 6, 30) - elif quarter == 3: - return date(year, 7, 1), date(year, 9, 30) - elif quarter == 4: - return date(year, 10, 1), date(year, 12, 31) - - # Month: YYYY-MM - if len(period) == 7 and period[4] == "-": - year = int(period[:4]) - month = int(period[5:7]) - # Calculate last day of month - if month == 12: - next_month = date(year + 1, 1, 1) - else: - next_month = date(year, month + 1, 1) - last_day = next_month - timedelta(days=1) - return date(year, month, 1), last_day - - # TODO: Support more formats (YYYY-HN, YYYY-WNN) - - except Exception as e: + return event_funcs.parse_period(period) + except ValueError as e: _logger.warning("Error parsing period '%s': %s", period, e) - - return None, None + return None, None diff --git a/spp_cel_event/models/cel_event_functions.py b/spp_cel_event/models/cel_event_functions.py index 4bac25fa..00ec10eb 100644 --- a/spp_cel_event/models/cel_event_functions.py +++ b/spp_cel_event/models/cel_event_functions.py @@ -389,7 +389,7 @@ def get_default_select_mode(env: Any, event_type_code: str) -> str: if event_type and event_type.is_one_active_per_registrant: return "active" except Exception as e: - _logger.warning(f"Failed to determine select mode for event type {event_type_code!r}: {e}") + _logger.warning("Failed to determine select mode for event type %r: %s", event_type_code, e) return "latest" @@ -427,7 +427,7 @@ def get_states_for_select_mode(select: str) -> list[str]: return ["active", "superseded", "expired"] else: # Default to active only for unknown modes - _logger.warning(f"Unknown select mode: {select!r}, defaulting to ['active']") + _logger.warning("Unknown select mode: %r, defaulting to ['active']", select) return ["active"] @@ -565,7 +565,7 @@ def validate_temporal_range(start_date: date | None, end_date: date | None) -> t """ if start_date is not None and end_date is not None: if start_date > end_date: - _logger.warning(f"Invalid temporal range: start_date {start_date} > end_date {end_date}") + _logger.warning("Invalid temporal range: start_date %s > end_date %s", start_date, end_date) return None, None return start_date, end_date diff --git a/spp_cel_event/models/cel_event_queryplan.py b/spp_cel_event/models/cel_event_queryplan.py index 9a1c9447..19afc41e 100644 --- a/spp_cel_event/models/cel_event_queryplan.py +++ b/spp_cel_event/models/cel_event_queryplan.py @@ -192,76 +192,6 @@ class EventsAggregate: rhs: Any = 0 -@dataclass -class EventsCollection: - """Collection operation over events (exists, count, all, any with predicate). - - This node represents a collection operation that applies a predicate to - multiple events. The predicate is evaluated for each event, and the - operation determines how to combine the results. - - Attributes: - event_type: Event type code to iterate over - operation: Collection operation ('exists', 'count', 'all', 'any') - var_name: Loop variable name used in predicate - predicate: CEL predicate AST node (evaluated per event) - after: Filter events with collection_date >= this date - before: Filter events with collection_date <= this date - within_days: Filter events within last N days from today - within_months: Filter events within last N months from today - period: Named period filter (e.g., '2024', '2024-Q1') - states: Filter events by state - op: For 'count' operation, comparison operator - rhs: For 'count' operation, value to compare count against - - Example: - # events('survey', period='2024').any(e, e.income < 500) - EventsCollection( - event_type='survey', - operation='any', - var_name='e', - predicate=, - period='2024' - ) - - # events('assessment').all(e, e.passed == true) - EventsCollection( - event_type='assessment', - operation='all', - var_name='e', - predicate= - ) - - # events('visit', within_days=365).count(e, e.verified == true) >= 4 - EventsCollection( - event_type='visit', - operation='count', - var_name='e', - predicate=, - within_days=365, - op='>=', - rhs=4 - ) - """ - - event_type: str - operation: str - var_name: str - predicate: Any - - # Pre-filters (applied before predicate evaluation) - after: date | None = None - before: date | None = None - within_days: int | None = None - within_months: int | None = None - period: str | None = None - states: list[str] | None = None - - # For count comparison - op: str | None = None - rhs: int | None = None - - @dataclass class EventFieldRef: """Reference to an event field (intermediate representation). diff --git a/spp_cel_event/models/cel_event_translator.py b/spp_cel_event/models/cel_event_translator.py index 80705d65..8fbf0927 100644 --- a/spp_cel_event/models/cel_event_translator.py +++ b/spp_cel_event/models/cel_event_translator.py @@ -14,6 +14,7 @@ """ import logging +import re from datetime import date from typing import Any @@ -423,7 +424,7 @@ def _handle_period_function(self, model: str, node: P.Call, cfg: dict[str, Any], Returns: Tuple of (LeafDomain, explanation) - returns period string as constant """ - from ...spp_cel_domain.models.cel_queryplan import LeafDomain + from odoo.addons.spp_cel_domain.models.cel_queryplan import LeafDomain func_name = node.func.name today = fields.Date.context_today(self) @@ -516,8 +517,6 @@ def _eval_literal(self, node: Any, ctx: dict[str, Any] | None = None): ): _, explain = self._handle_period_function("", node, {}, ctx or {}) # Extract period string from explanation - import re - match = re.search(r"'([^']+)'", explain) if match: return match.group(1) diff --git a/spp_cel_event/models/cel_variable_event_agg.py b/spp_cel_event/models/cel_variable_event_agg.py index a2d38fb3..670a7039 100644 --- a/spp_cel_event/models/cel_variable_event_agg.py +++ b/spp_cel_event/models/cel_variable_event_agg.py @@ -1,3 +1,5 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. + """CEL Variable Event Aggregation Extension. This module extends spp.cel.variable to support event-based aggregations, @@ -81,11 +83,6 @@ class CELVariableEventAggregation(models.Model): ) @api.depends( - "source_type", - "aggregate_type", - "aggregate_target", - "aggregate_field", - "aggregate_filter", "event_agg_type_id", "event_agg_temporal", "event_agg_temporal_value", @@ -94,13 +91,10 @@ class CELVariableEventAggregation(models.Model): ) def _compute_cel_expression(self): """Override to handle event aggregations.""" + super()._compute_cel_expression() for var in self: - if var.source_type == "aggregate": - if var.aggregate_target == "events": - var.cel_expression = var._build_event_aggregate_cel() - else: - var.cel_expression = var._build_aggregate_cel() - # Other source_types: preserve manually-set value + if var.source_type == "aggregate" and var.aggregate_target == "events": + var.cel_expression = var._build_event_aggregate_cel() def _build_event_aggregate_cel(self): """Build CEL expression for event aggregations. From ff8450f0db74c098408963c3fd6f2aaeba5bd3a3 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:21:54 +0800 Subject: [PATCH 02/12] feat: UI/UX improvements and doc cleanup for spp_cel_event - Add expression preview with monospace display and incomplete config warning - Add required constraint on event_agg_type_id when aggregate_target is events - Wrap event fields in named group with single visibility control - Add onchange methods for aggregate_type and temporal clearing - Improve field labels and help text - Add hint about using aggregate filter for event filtering - Remove empty cel_event_profiles.yaml placeholder - Remove models/README.md (duplicated docstrings) - Fix stale paths and descriptions in IMPLEMENTATION_NOTES.md - Add comment explaining header-only ir.model.access.csv --- spp_cel_event/IMPLEMENTATION_NOTES.md | 28 +-- spp_cel_event/data/cel_event_profiles.yaml | 18 -- spp_cel_event/models/README.md | 190 ------------------ .../models/cel_variable_event_agg.py | 40 +++- spp_cel_event/security/ir.model.access.csv | 1 + .../views/cel_variable_event_agg_views.xml | 90 ++++++--- 6 files changed, 102 insertions(+), 265 deletions(-) delete mode 100644 spp_cel_event/data/cel_event_profiles.yaml delete mode 100644 spp_cel_event/models/README.md diff --git a/spp_cel_event/IMPLEMENTATION_NOTES.md b/spp_cel_event/IMPLEMENTATION_NOTES.md index cc2a14d4..fad6cb6c 100644 --- a/spp_cel_event/IMPLEMENTATION_NOTES.md +++ b/spp_cel_event/IMPLEMENTATION_NOTES.md @@ -3,7 +3,7 @@ ## Overview This document describes the implementation of the CEL executor extension for event data -queries in `/home/user/openspp-modules-v2/spp_cel_event/models/cel_event_executor.py`. +queries in `models/cel_event_executor.py`. ## Architecture @@ -134,7 +134,7 @@ JSON field extraction handles multiple data types: The Python path is used when: - Default value specified (requires post-processing) -- Complex where predicates in aggregations +- Complex where predicates in aggregations (NOTE: `where_predicate` is not yet implemented — registrants with this parameter are silently skipped with a warning log) - SQL execution fails - Non-standard comparison operators @@ -160,19 +160,22 @@ All execution paths log performance metrics: ```python _logger.info( - "[CEL EVENT] EventValueCompare SQL: event_type=%s field=%s op=%s rhs=%s matches=%d", + "[CEL EVENT] EventValueCompare SQL: event_type=%s field=%s matches=%d", plan.event_type, plan.field_name, + len(partner_ids), +) +_logger.debug( + "[CEL EVENT] EventValueCompare SQL details: op=%s rhs=%r", plan.op, plan.rhs, - len(partner_ids), ) ``` Log tags: - `[CEL EVENT]` - Event executor operations -- Includes: event_type, field, operator, RHS value, match count +- Includes: event_type, field, match count (op/rhs at DEBUG level only) - Separate log entries for SQL vs Python paths ## Integration Points @@ -199,14 +202,11 @@ Depends on `spp.event.data` model with: ### Period Parsing -Currently supports: +All period formats are supported via `cel_event_functions.parse_period()`: - `YYYY`: Full year (e.g., '2024') - `YYYY-QN`: Quarter (e.g., '2024-Q1') - `YYYY-MM`: Month (e.g., '2024-03') - -**TODO**: Add support for: - - `YYYY-HN`: Half year (e.g., '2024-H1') - `YYYY-WNN`: ISO week (e.g., '2024-W01') @@ -249,11 +249,7 @@ Currently supports: - Cache results for identical queries within request - Invalidate on event data changes -4. **Extended Period Support** - - Add half-year and ISO week parsing - - Support dynamic period functions (this_quarter(), last_year()) - -5. **Batch Optimization** +4. **Batch Optimization** - When multiple event conditions in same expression - Combine into single query with JOINs @@ -264,7 +260,3 @@ Currently supports: - `spp_cel_domain`: Base CEL executor - `spp_event_data`: Event data model -## Files Modified - -1. `/home/user/openspp-modules-v2/spp_cel_event/models/cel_event_executor.py` (created) -2. `/home/user/openspp-modules-v2/spp_cel_event/models/__init__.py` (updated import) diff --git a/spp_cel_event/data/cel_event_profiles.yaml b/spp_cel_event/data/cel_event_profiles.yaml deleted file mode 100644 index 5aab9f87..00000000 --- a/spp_cel_event/data/cel_event_profiles.yaml +++ /dev/null @@ -1,18 +0,0 @@ -# CEL Event Data Integration Profiles -# -# This file defines CEL function profiles that integrate event data -# into eligibility and entitlement expressions. -# -# These profiles extend the base CEL domain language with event-specific -# functions for querying survey responses, field visit data, and other -# time-series event information. -# -# Examples: -# - event('survey_code', 'field_name') - Get field value from most recent event -# - event_count('survey_code') - Count events of a type for the registrant -# - event_exists('survey_code') - Check if registrant has any event of type -# - event_between('survey_code', date1, date2) - Get events in date range -# -# Profile definitions will be added as the CEL event functions are implemented. - -profiles: [] diff --git a/spp_cel_event/models/README.md b/spp_cel_event/models/README.md deleted file mode 100644 index c359a2b6..00000000 --- a/spp_cel_event/models/README.md +++ /dev/null @@ -1,190 +0,0 @@ -# CEL Event Functions - -Utility functions for working with event data in CEL expressions. - -## Overview - -This module provides pure Python functions that can be used in CEL expressions to: - -- Parse period strings into date ranges -- Generate dynamic period strings (current/previous quarter, month, year, etc.) -- Resolve event selection modes and states -- Apply and combine temporal filters - -## Usage in CEL Expressions - -These functions are designed to be called from CEL expressions in eligibility rules, -entitlement formulas, and other CEL-based logic. - -### Period Parsing - -```python -# Parse various period formats -parse_period('2024') # Full year: (2024-01-01, 2024-12-31) -parse_period('2024-Q2') # Quarter: (2024-04-01, 2024-06-30) -parse_period('2024-H1') # Half-year: (2024-01-01, 2024-06-30) -parse_period('2024-03') # Month: (2024-03-01, 2024-03-31) -parse_period('2024-W15') # ISO week: (2024-04-08, 2024-04-14) -``` - -### Dynamic Period Generators - -```python -# Current periods -this_year() # '2024' -this_quarter() # '2024-Q4' -this_month() # '2024-12' - -# Previous periods -last_year() # '2023' -last_quarter() # '2024-Q3' -last_month() # '2024-11' - -# Historical periods -quarters_ago(2) # '2024-Q2' (2 quarters ago) -months_ago(6) # '2024-06' (6 months ago) - -# Period boundaries -year_start() # date(2024, 1, 1) -quarter_start() # date(2024, 10, 1) -month_start() # date(2024, 12, 1) -``` - -### Selection Mode Helpers - -```python -# Get default selection mode for an event type -get_default_select_mode(env, 'household_survey') # 'active' or 'latest' - -# Get states for a selection mode -get_states_for_select_mode('active') # ['active'] -get_states_for_select_mode('latest') # ['active', 'superseded', 'expired'] -get_states_for_select_mode('latest_active') # ['active'] -``` - -### Temporal Filters - -```python -# Single filter -apply_temporal_filters(within_days=30) -# Returns: (date 30 days ago, today) - -apply_temporal_filters(period='2024-Q2') -# Returns: (2024-04-01, 2024-06-30) - -# Combined filters (intersection) -apply_temporal_filters( - period='2024', - after=date(2024, 6, 1), - before=date(2024, 9, 30) -) -# Returns: (2024-06-01, 2024-09-30) -``` - -### Utility Functions - -```python -# Calculate days between dates -days_between(date(2024, 1, 1), date(2024, 12, 31)) # 365 - -# Check if date is within range -is_within_range(date(2024, 6, 15), start=date(2024, 1, 1), end=date(2024, 12, 31)) # True - -# Validate temporal range -validate_temporal_range(start_date, end_date) # Returns (start, end) or (None, None) if invalid -``` - -## Implementation Notes - -### Pure Functions - -Most functions are pure (no side effects) and can be tested independently: - -- `parse_period` -- All period generators (`this_year`, `last_quarter`, etc.) -- `get_states_for_select_mode` -- `apply_temporal_filters` -- Utility functions - -### Odoo Environment Required - -Only one function requires the Odoo environment: - -- `get_default_select_mode(env, event_type_code)` - Looks up event type configuration - -### Error Handling - -- Invalid period strings raise `ValueError` with descriptive messages -- Invalid date ranges are logged as warnings -- Missing event types default to 'latest' selection mode -- Unknown selection modes default to ['active'] states - -### Type Hints - -All functions include type hints for better IDE support and documentation: - -```python -def parse_period(period: str) -> tuple[date, date]: -def apply_temporal_filters( - base_date: date | None = None, - after: date | str | None = None, - # ... -) -> tuple[date | None, date | None]: -``` - -## Testing - -Comprehensive unit tests are provided in `tests/test_cel_event_functions.py`: - -- Period parsing (all formats) -- Dynamic period generation -- Selection mode resolution -- Temporal filter combinations -- Edge cases (year boundaries, leap years, etc.) - -Run tests with: - -```bash -./scripts/test_single_module.sh spp_cel_event -``` - -## Examples - -### Eligibility Rule: "Has recent survey" - -```python -# Check if registrant has a survey within the last year -has_event('household_survey', period=this_year()) -``` - -### Eligibility Rule: "Income in last survey" - -```python -# Get income from most recent survey in current quarter -event('household_survey', select='latest', period=this_quarter()).income > 500 -``` - -### Entitlement Formula: "Average attendance" - -```python -# Average attendance across all events in 2024 -events_avg('attendance', 'days_attended', period='2024') -``` - -### Complex Filter: "Recent high-value surveys" - -```python -# Count surveys with high income in last 6 months -events_count( - 'household_survey', - period=months_ago(6) + '/' + this_month(), - where='income > 1000', - states=['active'] -) -``` - -## See Also - -- [CEL Event Data Integration Spec](../../../docs/specs/CEL_EVENT_DATA_INTEGRATION_SPEC.md) -- [Query Plan Nodes](cel_event_queryplan.py) -- [Event Data Model](../../spp_event_data/models/event_data.py) diff --git a/spp_cel_event/models/cel_variable_event_agg.py b/spp_cel_event/models/cel_variable_event_agg.py index 670a7039..7c94df02 100644 --- a/spp_cel_event/models/cel_variable_event_agg.py +++ b/spp_cel_event/models/cel_variable_event_agg.py @@ -63,13 +63,13 @@ class CELVariableEventAggregation(models.Model): ) event_agg_temporal_value = fields.Integer( - string="N (Days/Months)", + string="Number of Days/Months", help="Number of days or months for 'within' temporal filters", ) event_agg_field = fields.Char( string="Event Field", - help="JSON field name in event data (e.g., 'amount', 'score'). Required for sum/avg/min/max aggregations.", + help="Name of the data field in the event record to aggregate (e.g., 'amount', 'score')", ) event_agg_states = fields.Selection( @@ -79,7 +79,7 @@ class CELVariableEventAggregation(models.Model): ], string="Event States", default="active", - help="Which event states to include in aggregation", + help="Active Only: current events. All States: also includes superseded and expired events.", ) @api.depends( @@ -177,17 +177,41 @@ def _onchange_aggregate_target_event(self): self.event_agg_field = False self.event_agg_states = "active" - @api.constrains("aggregate_target", "event_agg_type_id") + @api.constrains("aggregate_target", "event_agg_type_id", "aggregate_type", "event_agg_field") def _check_event_aggregation_config(self): """Ensure event aggregations have required configuration.""" for rec in self: if rec.source_type == "aggregate" and rec.aggregate_target == "events": if not rec.event_agg_type_id: - # Log warning but don't block - may be set later - _logger.warning( - "Variable '%s' is configured for event aggregation but no event type is selected.", - rec.name, + raise ValidationError( + _( + "Variable '%(name)s' is configured for event aggregation " + "but no event type is selected.", + name=rec.name, + ) ) + if rec.aggregate_type in ("sum", "avg", "min", "max"): + if not rec.event_agg_field and not rec.aggregate_field: + raise ValidationError( + _( + "Variable '%(name)s' uses %(agg_type)s aggregation over events " + "but no field is specified. Please set the Event Field.", + name=rec.name, + agg_type=rec.aggregate_type, + ) + ) + + @api.onchange("aggregate_type") + def _onchange_aggregate_type_event(self): + """Clear event field when switching to count/exists.""" + if self.aggregate_target == "events" and self.aggregate_type in ("count", "exists"): + self.event_agg_field = False + + @api.onchange("event_agg_temporal") + def _onchange_event_agg_temporal(self): + """Reset temporal value when switching to non-within temporal type.""" + if self.event_agg_temporal not in ("within_days", "within_months"): + self.event_agg_temporal_value = 0 @api.constrains("event_agg_temporal", "event_agg_temporal_value") def _check_temporal_value(self): diff --git a/spp_cel_event/security/ir.model.access.csv b/spp_cel_event/security/ir.model.access.csv index 97dd8b91..43791afd 100644 --- a/spp_cel_event/security/ir.model.access.csv +++ b/spp_cel_event/security/ir.model.access.csv @@ -1 +1,2 @@ +# No ACLs needed - this module only extends existing models via _inherit (no new _name declarations) id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink diff --git a/spp_cel_event/views/cel_variable_event_agg_views.xml b/spp_cel_event/views/cel_variable_event_agg_views.xml index d9704339..1cd0e52a 100644 --- a/spp_cel_event/views/cel_variable_event_agg_views.xml +++ b/spp_cel_event/views/cel_variable_event_agg_views.xml @@ -9,39 +9,67 @@ - - - - - - + > + + + + + + + + Use the Aggregate Filter field above to filter events (e.g., amount > 100) + + + +
+
From 400419b379667900cd5513819b27bef691a682c3 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:27:07 +0800 Subject: [PATCH 03/12] test: fill test gaps and add translator unit tests for spp_cel_event - Replace pass stubs with real tests for sum, avg, min, max aggregation - Add test for latest_active selection mode - Add test for within_months temporal filter - Replace unused function-style test with income range comparison test - Add comments explaining why CEL service tests use "true" expression - Create test_cel_event_translator.py with 39 unit tests covering all translator methods: _is_event_field_access, _handle_event_call, _handle_has_event, _handle_events_aggregate, _handle_aggregate_compare, _extract_event_parameters, _eval_literal, _handle_period_function --- spp_cel_event/tests/__init__.py | 1 + .../tests/test_cel_event_integration.py | 165 +++++- .../tests/test_cel_event_translator.py | 519 ++++++++++++++++++ 3 files changed, 668 insertions(+), 17 deletions(-) create mode 100644 spp_cel_event/tests/test_cel_event_translator.py diff --git a/spp_cel_event/tests/__init__.py b/spp_cel_event/tests/__init__.py index 01df97d5..bae93c57 100644 --- a/spp_cel_event/tests/__init__.py +++ b/spp_cel_event/tests/__init__.py @@ -3,5 +3,6 @@ from . import ( test_cel_event_functions, test_cel_event_integration, + test_cel_event_translator, test_cel_variable_event_agg, ) diff --git a/spp_cel_event/tests/test_cel_event_integration.py b/spp_cel_event/tests/test_cel_event_integration.py index f6826682..0cbaf4cd 100644 --- a/spp_cel_event/tests/test_cel_event_integration.py +++ b/spp_cel_event/tests/test_cel_event_integration.py @@ -206,12 +206,24 @@ def test_event_basic_field_access_dot_notation(self): self.assertNotIn(self.registrant_middle.id, result["ids"]) self.assertNotIn(self.registrant_rich.id, result["ids"]) - def test_event_basic_field_access_function_style(self): - """Test event('type', 'field') > value pattern.""" - # Note: This test assumes parser supports this syntax - # Currently the translator expects dot notation or would need parser updates - # Marking as documentation of intended behavior - pass + def test_event_value_compare_income_range(self): + """Test EventValueCompare with income in a range (between two thresholds).""" + from ..models.cel_event_queryplan import EventValueCompare + + # Income > 500 + plan = EventValueCompare( + event_type="household_survey", + field_name="income", + op=">", + rhs=500, + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertNotIn(self.registrant_poor.id, ids) # 300 < 500 + self.assertIn(self.registrant_middle.id, ids) # 800 > 500 + self.assertIn(self.registrant_rich.id, ids) # 2000 > 500 def test_event_field_access_no_matching_event(self): """Test event field access when no event exists returns empty set.""" @@ -329,6 +341,27 @@ def test_event_after_before_filters(self): self.assertIn(self.registrant_middle.id, ids) self.assertNotIn(self.registrant_rich.id, ids) # 400 days ago + def test_event_within_months_filter(self): + """Test event with within_months temporal filter.""" + from ..models.cel_event_queryplan import EventValueCompare + + # Poor registrant survey is 30 days ago, within 2 months + # Rich registrant survey is 400 days ago, NOT within 2 months + plan = EventValueCompare( + event_type="household_survey", + field_name="income", + op=">", + rhs=0, + within_months=2, + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertIn(self.registrant_poor.id, ids) # 30 days ago + self.assertIn(self.registrant_middle.id, ids) # 60 days ago + self.assertNotIn(self.registrant_rich.id, ids) # 400 days ago + # ══════════════════════════════════════════════════════════════════════════════ # Test event() with selection modes # ══════════════════════════════════════════════════════════════════════════════ @@ -418,6 +451,39 @@ def test_event_select_first_mode(self): # Cleanup new_survey.unlink() + def test_event_select_latest_active_mode(self): + """Test event with select='latest_active' mode.""" + from ..models.cel_event_queryplan import EventValueCompare + + # Create a superseded (old) survey with high income + old_survey = self.env["spp.event.data"].create( + { + "partner_id": self.registrant_poor.id, + "event_type_id": self.survey_type.id, + "collection_date": date.today() - timedelta(days=200), + "state": "superseded", + "data_json": {"income": 5000}, + } + ) + + # latest_active should only consider active events, picking the most recent + plan = EventValueCompare( + event_type="household_survey", + field_name="income", + op="<", + rhs=500, + select="latest_active", + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + # Should match poor registrant (active survey has income=300) + self.assertIn(self.registrant_poor.id, ids) + + # Cleanup + old_survey.unlink() + # ══════════════════════════════════════════════════════════════════════════════ # Test has_event() function # ══════════════════════════════════════════════════════════════════════════════ @@ -544,16 +610,80 @@ def test_events_count_exact_match(self): self.assertIn(self.registrant_poor.id, ids) def test_events_sum_aggregation(self): - """Test events_sum() aggregation.""" - # This would need numeric field to sum - # Skip for now as test data doesn't have good sum candidate - pass + """Test events_sum() aggregation over visit_number field.""" + from ..models.cel_event_queryplan import EventsAggregate + + # Sum of visit_number across all 10 visits = 1+2+...+10 = 55 + plan = EventsAggregate( + event_type="field_visit", + field_name="visit_number", + agg="sum", + op=">=", + rhs=50, + states=["active"], + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertIn(self.registrant_poor.id, ids) # Sum = 55 >= 50 + self.assertNotIn(self.registrant_middle.id, ids) # No visits def test_events_avg_aggregation(self): - """Test events_avg() aggregation.""" - # This would need numeric field to average - # Skip for now as test data doesn't have good avg candidate - pass + """Test events_avg() aggregation over visit_number field.""" + from ..models.cel_event_queryplan import EventsAggregate + + # Avg of visit_number across 10 visits = 55/10 = 5.5 + plan = EventsAggregate( + event_type="field_visit", + field_name="visit_number", + agg="avg", + op=">=", + rhs=5, + states=["active"], + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertIn(self.registrant_poor.id, ids) # Avg = 5.5 >= 5 + self.assertNotIn(self.registrant_middle.id, ids) # No visits + + def test_events_min_aggregation(self): + """Test events_min() aggregation over visit_number field.""" + from ..models.cel_event_queryplan import EventsAggregate + + plan = EventsAggregate( + event_type="field_visit", + field_name="visit_number", + agg="min", + op="==", + rhs=1, + states=["active"], + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertIn(self.registrant_poor.id, ids) # Min visit_number = 1 + + def test_events_max_aggregation(self): + """Test events_max() aggregation over visit_number field.""" + from ..models.cel_event_queryplan import EventsAggregate + + plan = EventsAggregate( + event_type="field_visit", + field_name="visit_number", + agg="max", + op="==", + rhs=10, + states=["active"], + ) + + executor = self.env["spp.cel.executor"] + ids = executor._execute_plan("res.partner", plan) + + self.assertIn(self.registrant_poor.id, ids) # Max visit_number = 10 # ══════════════════════════════════════════════════════════════════════════════ # Test SQL path vs Python path @@ -847,9 +977,8 @@ def test_multiple_active_events_same_type(self): def test_cel_service_compilation_succeeds(self): """Test that CEL service can compile event expressions.""" - # Note: This requires parser support for event() syntax - # Currently may not work without parser updates - # Keeping as documentation of intended behavior + # Tests basic CEL compilation only. event() syntax is parsed via the translator but + # the end-to-end compile_expression path for full event expressions is tested via executor plan tests above. expr = "true" # Basic expression that should work result = self.cel_service.compile_expression(expr, self.profile) @@ -859,6 +988,8 @@ def test_cel_service_compilation_succeeds(self): def test_cel_service_get_matching_ids(self): """Test get_matching_ids wrapper method.""" + # Tests basic CEL compilation only. event() syntax is parsed via the translator but + # the end-to-end compile_expression path for full event expressions is tested via executor plan tests above. expr = "true" ids = self.cel_service.get_matching_ids(expr, self.profile) diff --git a/spp_cel_event/tests/test_cel_event_translator.py b/spp_cel_event/tests/test_cel_event_translator.py new file mode 100644 index 00000000..d9a49a2a --- /dev/null +++ b/spp_cel_event/tests/test_cel_event_translator.py @@ -0,0 +1,519 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. + +"""Tests for CEL event translator extension.""" + +from odoo.tests.common import TransactionCase, tagged + +from odoo.addons.spp_cel_domain.services import cel_parser as P + +from ..models.cel_event_queryplan import ( + EventExists, + EventFieldRef, + EventsAggregate, + EventValueCompare, +) + + +@tagged("post_install", "-at_install") +class TestCelEventTranslator(TransactionCase): + """Unit tests for the CEL event translator methods.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.translator = cls.env["spp.cel.translator"] + cls.model = "res.partner" + cls.cfg = {} + cls.ctx = {} + + # ══════════════════════════════════════════════════════════════════════════ + # _is_event_field_access tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_is_event_field_access_dot_notation(self): + """Test event('type').field is recognized as event field access.""" + # event('survey').income + node = P.Attr( + obj=P.Call(func=P.Ident("event"), args=[P.Literal("survey")]), + name="income", + ) + self.assertTrue(self.translator._is_event_field_access(node)) + + def test_is_event_field_access_two_arg(self): + """Test event('type', 'field') is recognized as event field access.""" + node = P.Call( + func=P.Ident("event"), + args=[P.Literal("survey"), P.Literal("income")], + ) + self.assertTrue(self.translator._is_event_field_access(node)) + + def test_is_event_field_access_not_event(self): + """Test non-event call is not recognized.""" + node = P.Call(func=P.Ident("other_func"), args=[P.Literal("survey")]) + self.assertFalse(self.translator._is_event_field_access(node)) + + def test_is_event_field_access_plain_ident(self): + """Test plain identifier is not recognized.""" + node = P.Ident("some_var") + self.assertFalse(self.translator._is_event_field_access(node)) + + def test_is_event_field_access_event_no_field(self): + """Test event('type') without field access is not a field access.""" + node = P.Call(func=P.Ident("event"), args=[P.Literal("survey")]) + self.assertFalse(self.translator._is_event_field_access(node)) + + def test_is_event_field_access_event_single_non_string_arg(self): + """Test event('type', 123) where second arg is not string is not field access.""" + node = P.Call( + func=P.Ident("event"), + args=[P.Literal("survey"), P.Literal(123)], + ) + self.assertFalse(self.translator._is_event_field_access(node)) + + # ══════════════════════════════════════════════════════════════════════════ + # _handle_event_call tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_handle_event_call_basic(self): + """Test basic event('type') call produces EventFieldRef.""" + node = P.Call(func=P.Ident("event"), args=[P.Literal("survey")]) + result, explain = self.translator._handle_event_call( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventFieldRef) + self.assertEqual(result.event_type, "survey") + self.assertIsNone(result.field_name) + self.assertEqual(result.select, "auto") + + def test_handle_event_call_with_field(self): + """Test event('type', 'field') sets field_name.""" + node = P.Call( + func=P.Ident("event"), + args=[P.Literal("survey"), P.Literal("income")], + ) + result, explain = self.translator._handle_event_call( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventFieldRef) + self.assertEqual(result.event_type, "survey") + self.assertEqual(result.field_name, "income") + + def test_handle_event_call_with_kwargs(self): + """Test event() with named parameters.""" + node = P.Call( + func=P.Ident("event"), + args=[P.Literal("survey")], + kwargs={ + "select": P.Literal("latest"), + "within_days": P.Literal(90), + }, + ) + result, explain = self.translator._handle_event_call( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.select, "latest") + self.assertEqual(result.within_days, 90) + + def test_handle_event_call_no_args_raises(self): + """Test event() with no arguments raises ValueError.""" + node = P.Call(func=P.Ident("event"), args=[]) + with self.assertRaises(ValueError): + self.translator._handle_event_call( + self.model, node, self.cfg, self.ctx + ) + + def test_handle_event_call_non_string_type_raises(self): + """Test event(123) with non-string type raises TypeError.""" + node = P.Call(func=P.Ident("event"), args=[P.Literal(123)]) + with self.assertRaises(TypeError): + self.translator._handle_event_call( + self.model, node, self.cfg, self.ctx + ) + + # ══════════════════════════════════════════════════════════════════════════ + # _handle_has_event tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_handle_has_event_basic(self): + """Test has_event('type') produces EventExists.""" + node = P.Call(func=P.Ident("has_event"), args=[P.Literal("survey")]) + result, explain = self.translator._handle_has_event( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventExists) + self.assertEqual(result.event_type, "survey") + self.assertEqual(result.states, ["active"]) # Default + + def test_handle_has_event_with_temporal(self): + """Test has_event() with within_days parameter.""" + node = P.Call( + func=P.Ident("has_event"), + args=[P.Literal("assessment")], + kwargs={"within_days": P.Literal(365)}, + ) + result, explain = self.translator._handle_has_event( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.within_days, 365) + + def test_handle_has_event_no_args_raises(self): + """Test has_event() with no arguments raises ValueError.""" + node = P.Call(func=P.Ident("has_event"), args=[]) + with self.assertRaises(ValueError): + self.translator._handle_has_event( + self.model, node, self.cfg, self.ctx + ) + + def test_handle_has_event_non_string_type_raises(self): + """Test has_event(123) raises TypeError.""" + node = P.Call(func=P.Ident("has_event"), args=[P.Literal(123)]) + with self.assertRaises(TypeError): + self.translator._handle_has_event( + self.model, node, self.cfg, self.ctx + ) + + # ══════════════════════════════════════════════════════════════════════════ + # _handle_events_aggregate tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_handle_events_count(self): + """Test events_count('type') produces EventsAggregate with agg='count'.""" + node = P.Call( + func=P.Ident("events_count"), args=[P.Literal("attendance")] + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventsAggregate) + self.assertEqual(result.event_type, "attendance") + self.assertEqual(result.agg, "count") + self.assertIsNone(result.field_name) + # Default op and rhs + self.assertEqual(result.op, ">=") + self.assertEqual(result.rhs, 0) + + def test_handle_events_sum(self): + """Test events_sum('type', 'field') produces correct aggregate.""" + node = P.Call( + func=P.Ident("events_sum"), + args=[P.Literal("survey"), P.Literal("income")], + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.agg, "sum") + self.assertEqual(result.field_name, "income") + + def test_handle_events_avg(self): + """Test events_avg produces correct aggregate.""" + node = P.Call( + func=P.Ident("events_avg"), + args=[P.Literal("survey"), P.Literal("score")], + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.agg, "avg") + self.assertEqual(result.field_name, "score") + + def test_handle_events_min(self): + """Test events_min produces correct aggregate.""" + node = P.Call( + func=P.Ident("events_min"), + args=[P.Literal("survey"), P.Literal("score")], + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.agg, "min") + + def test_handle_events_max(self): + """Test events_max produces correct aggregate.""" + node = P.Call( + func=P.Ident("events_max"), + args=[P.Literal("survey"), P.Literal("score")], + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.agg, "max") + + def test_handle_events_aggregate_with_kwargs(self): + """Test aggregate function with named parameters.""" + node = P.Call( + func=P.Ident("events_count"), + args=[P.Literal("attendance")], + kwargs={ + "period": P.Literal("2024"), + "states": P.Literal(["active", "superseded"]), + }, + ) + result, explain = self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + self.assertEqual(result.period, "2024") + self.assertEqual(result.states, ["active", "superseded"]) + + def test_handle_events_sum_no_field_raises(self): + """Test events_sum() without field argument raises ValueError.""" + node = P.Call( + func=P.Ident("events_sum"), args=[P.Literal("survey")] + ) + with self.assertRaises(ValueError): + self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + def test_handle_events_count_no_args_raises(self): + """Test events_count() with no arguments raises ValueError.""" + node = P.Call(func=P.Ident("events_count"), args=[]) + with self.assertRaises(ValueError): + self.translator._handle_events_aggregate( + self.model, node, self.cfg, self.ctx + ) + + # ══════════════════════════════════════════════════════════════════════════ + # _handle_aggregate_compare tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_handle_aggregate_compare_ge(self): + """Test events_count('type') >= 10 sets op and rhs.""" + cmp = P.Compare( + op="GE", + left=P.Call( + func=P.Ident("events_count"), args=[P.Literal("visit")] + ), + right=P.Literal(10), + ) + result, explain = self.translator._handle_aggregate_compare( + self.model, cmp, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventsAggregate) + self.assertEqual(result.op, ">=") + self.assertEqual(result.rhs, 10) + + def test_handle_aggregate_compare_eq(self): + """Test events_count('type') == 5.""" + cmp = P.Compare( + op="EQ", + left=P.Call( + func=P.Ident("events_count"), args=[P.Literal("visit")] + ), + right=P.Literal(5), + ) + result, explain = self.translator._handle_aggregate_compare( + self.model, cmp, self.cfg, self.ctx + ) + + self.assertEqual(result.op, "==") + self.assertEqual(result.rhs, 5) + + def test_handle_aggregate_compare_lt(self): + """Test events_sum('type', 'field') < 1000.""" + cmp = P.Compare( + op="LT", + left=P.Call( + func=P.Ident("events_sum"), + args=[P.Literal("survey"), P.Literal("income")], + ), + right=P.Literal(1000), + ) + result, explain = self.translator._handle_aggregate_compare( + self.model, cmp, self.cfg, self.ctx + ) + + self.assertEqual(result.op, "<") + self.assertEqual(result.rhs, 1000) + self.assertEqual(result.field_name, "income") + + # ══════════════════════════════════════════════════════════════════════════ + # _extract_event_parameters tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_extract_event_parameters_empty(self): + """Test extracting parameters from node with no kwargs.""" + node = P.Call(func=P.Ident("event"), args=[P.Literal("survey")]) + params = self.translator._extract_event_parameters( + node, self.ctx, start_index=1 + ) + self.assertEqual(params, {}) + + def test_extract_event_parameters_with_kwargs(self): + """Test extracting named parameters.""" + node = P.Call( + func=P.Ident("event"), + args=[P.Literal("survey")], + kwargs={ + "select": P.Literal("latest"), + "within_days": P.Literal(90), + "default": P.Literal(0), + }, + ) + params = self.translator._extract_event_parameters( + node, self.ctx, start_index=1 + ) + + self.assertEqual(params["select"], "latest") + self.assertEqual(params["within_days"], 90) + self.assertEqual(params["default"], 0) + + # ══════════════════════════════════════════════════════════════════════════ + # _eval_literal tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_eval_literal_event_field_ref_passthrough(self): + """Test that EventFieldRef is returned as-is.""" + ref = EventFieldRef(event_type="survey", field_name="income") + result = self.translator._eval_literal(ref, self.ctx) + self.assertIs(result, ref) + + def test_eval_literal_string(self): + """Test evaluating string literal.""" + result = self.translator._eval_literal(P.Literal("hello"), self.ctx) + self.assertEqual(result, "hello") + + def test_eval_literal_number(self): + """Test evaluating number literal.""" + result = self.translator._eval_literal(P.Literal(42), self.ctx) + self.assertEqual(result, 42) + + def test_eval_literal_boolean(self): + """Test evaluating boolean literal.""" + result = self.translator._eval_literal(P.Literal(True), self.ctx) + self.assertTrue(result) + + # ══════════════════════════════════════════════════════════════════════════ + # _handle_period_function tests + # ══════════════════════════════════════════════════════════════════════════ + + def test_handle_period_this_year(self): + """Test this_year() returns current year string.""" + from datetime import date + + node = P.Call(func=P.Ident("this_year"), args=[]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + expected_year = str(date.today().year) + self.assertIn(expected_year, explain) + + def test_handle_period_last_year(self): + """Test last_year() returns previous year string.""" + from datetime import date + + node = P.Call(func=P.Ident("last_year"), args=[]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + expected_year = str(date.today().year - 1) + self.assertIn(expected_year, explain) + + def test_handle_period_this_quarter(self): + """Test this_quarter() returns current quarter string.""" + node = P.Call(func=P.Ident("this_quarter"), args=[]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + # Should contain a quarter like '2026-Q1' + self.assertRegex(explain, r"\d{4}-Q[1-4]") + + def test_handle_period_this_month(self): + """Test this_month() returns current month string.""" + node = P.Call(func=P.Ident("this_month"), args=[]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + # Should contain a month like '2026-03' + self.assertRegex(explain, r"\d{4}-\d{2}") + + def test_handle_period_quarters_ago(self): + """Test quarters_ago(n) returns correct quarter string.""" + node = P.Call(func=P.Ident("quarters_ago"), args=[P.Literal(1)]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + self.assertRegex(explain, r"\d{4}-Q[1-4]") + + def test_handle_period_months_ago(self): + """Test months_ago(n) returns correct month string.""" + node = P.Call(func=P.Ident("months_ago"), args=[P.Literal(2)]) + result, explain = self.translator._handle_period_function( + self.model, node, self.cfg, self.ctx + ) + + self.assertRegex(explain, r"\d{4}-\d{2}") + + # ══════════════════════════════════════════════════════════════════════════ + # _to_plan integration (end-to-end translator tests) + # ══════════════════════════════════════════════════════════════════════════ + + def test_to_plan_event_field_compare(self): + """Test full _to_plan for event('type').field > value.""" + # event('survey').income > 500 + node = P.Compare( + op="GT", + left=P.Attr( + obj=P.Call( + func=P.Ident("event"), args=[P.Literal("survey")] + ), + name="income", + ), + right=P.Literal(500), + ) + + result, explain = self.translator._to_plan( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventValueCompare) + self.assertEqual(result.event_type, "survey") + self.assertEqual(result.field_name, "income") + self.assertEqual(result.op, ">") + self.assertEqual(result.rhs, 500) + + def test_to_plan_has_event(self): + """Test full _to_plan for has_event('type').""" + node = P.Call(func=P.Ident("has_event"), args=[P.Literal("survey")]) + + result, explain = self.translator._to_plan( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventExists) + self.assertEqual(result.event_type, "survey") + + def test_to_plan_aggregate_compare(self): + """Test full _to_plan for events_count('type') >= 10.""" + node = P.Compare( + op="GE", + left=P.Call( + func=P.Ident("events_count"), + args=[P.Literal("attendance")], + ), + right=P.Literal(10), + ) + + result, explain = self.translator._to_plan( + self.model, node, self.cfg, self.ctx + ) + + self.assertIsInstance(result, EventsAggregate) + self.assertEqual(result.op, ">=") + self.assertEqual(result.rhs, 10) From 193f55293608f7468eb82f9f74642dfce06e7896 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:27:35 +0800 Subject: [PATCH 04/12] feat: bump spp_cel_event to Production/Stable with proper auto_install - Change development_status from Beta to Production/Stable - Change auto_install from True to explicit dependency list ["spp_cel_domain", "spp_event_data", "spp_studio"] per module-visibility principle for multi-dependency bridge modules --- spp_cel_event/__manifest__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spp_cel_event/__manifest__.py b/spp_cel_event/__manifest__.py index 1a3afb23..a2854547 100644 --- a/spp_cel_event/__manifest__.py +++ b/spp_cel_event/__manifest__.py @@ -9,7 +9,7 @@ "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", "license": "LGPL-3", - "development_status": "Beta", + "development_status": "Production/Stable", "maintainers": ["jeremi", "gonzalesedwin1123", "emjay0921"], "depends": [ "spp_cel_domain", @@ -29,7 +29,7 @@ "images": [], "application": False, "installable": True, - "auto_install": True, + "auto_install": ["spp_cel_domain", "spp_event_data", "spp_studio"], "post_init_hook": "post_init_hook", "summary": "Integrate event data with CEL expressions for eligibility and entitlement rules", } From 54d756a4e8880cf76ee40175aa0e2da9a7fe767c Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:28:25 +0800 Subject: [PATCH 05/12] style: apply ruff-format and prettier formatting fixes --- spp_cel_event/IMPLEMENTATION_NOTES.md | 4 +- spp_cel_event/data/cel_profiles.yaml | 10 +- .../models/cel_variable_event_agg.py | 3 +- .../tests/test_cel_event_translator.py | 144 +++++------------- 4 files changed, 44 insertions(+), 117 deletions(-) diff --git a/spp_cel_event/IMPLEMENTATION_NOTES.md b/spp_cel_event/IMPLEMENTATION_NOTES.md index fad6cb6c..6a713b38 100644 --- a/spp_cel_event/IMPLEMENTATION_NOTES.md +++ b/spp_cel_event/IMPLEMENTATION_NOTES.md @@ -134,7 +134,8 @@ JSON field extraction handles multiple data types: The Python path is used when: - Default value specified (requires post-processing) -- Complex where predicates in aggregations (NOTE: `where_predicate` is not yet implemented — registrants with this parameter are silently skipped with a warning log) +- Complex where predicates in aggregations (NOTE: `where_predicate` is not yet + implemented — registrants with this parameter are silently skipped with a warning log) - SQL execution fails - Non-standard comparison operators @@ -259,4 +260,3 @@ All period formats are supported via `cel_event_functions.parse_period()`: - `odoo.tools.sql.SQL`: SQL query builder - `spp_cel_domain`: Base CEL executor - `spp_event_data`: Event data model - diff --git a/spp_cel_event/data/cel_profiles.yaml b/spp_cel_event/data/cel_profiles.yaml index e4717085..865845a1 100644 --- a/spp_cel_event/data/cel_profiles.yaml +++ b/spp_cel_event/data/cel_profiles.yaml @@ -174,7 +174,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - # TODO: 'where' parameter planned but not yet implemented + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_count('attendance') >= 180" description: "Count all active attendance events" @@ -226,7 +226,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - # TODO: 'where' parameter planned but not yet implemented + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_sum('attendance', 'days_present', period='2024') >= 180" description: "Sum attendance days in 2024" @@ -272,7 +272,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - # TODO: 'where' parameter planned but not yet implemented + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_avg('survey', 'income', within_days=365) < 500" description: "Average income from surveys in last year" @@ -318,7 +318,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - # TODO: 'where' parameter planned but not yet implemented + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_min('assessment', 'score', period='2024') >= 60" description: "Minimum assessment score in 2024" @@ -364,7 +364,7 @@ presets: type: list[str] required: false description: "Event states to include (default: ['active'])" - # TODO: 'where' parameter planned but not yet implemented + # TODO: 'where' parameter planned but not yet implemented examples: - expression: "events_max('assessment', 'score', period='2024') >= 70" description: "Maximum assessment score in 2024" diff --git a/spp_cel_event/models/cel_variable_event_agg.py b/spp_cel_event/models/cel_variable_event_agg.py index 7c94df02..39ba35a8 100644 --- a/spp_cel_event/models/cel_variable_event_agg.py +++ b/spp_cel_event/models/cel_variable_event_agg.py @@ -185,8 +185,7 @@ def _check_event_aggregation_config(self): if not rec.event_agg_type_id: raise ValidationError( _( - "Variable '%(name)s' is configured for event aggregation " - "but no event type is selected.", + "Variable '%(name)s' is configured for event aggregation but no event type is selected.", name=rec.name, ) ) diff --git a/spp_cel_event/tests/test_cel_event_translator.py b/spp_cel_event/tests/test_cel_event_translator.py index d9a49a2a..9bb5f712 100644 --- a/spp_cel_event/tests/test_cel_event_translator.py +++ b/spp_cel_event/tests/test_cel_event_translator.py @@ -77,9 +77,7 @@ def test_is_event_field_access_event_single_non_string_arg(self): def test_handle_event_call_basic(self): """Test basic event('type') call produces EventFieldRef.""" node = P.Call(func=P.Ident("event"), args=[P.Literal("survey")]) - result, explain = self.translator._handle_event_call( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_event_call(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventFieldRef) self.assertEqual(result.event_type, "survey") @@ -92,9 +90,7 @@ def test_handle_event_call_with_field(self): func=P.Ident("event"), args=[P.Literal("survey"), P.Literal("income")], ) - result, explain = self.translator._handle_event_call( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_event_call(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventFieldRef) self.assertEqual(result.event_type, "survey") @@ -110,9 +106,7 @@ def test_handle_event_call_with_kwargs(self): "within_days": P.Literal(90), }, ) - result, explain = self.translator._handle_event_call( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_event_call(self.model, node, self.cfg, self.ctx) self.assertEqual(result.select, "latest") self.assertEqual(result.within_days, 90) @@ -121,17 +115,13 @@ def test_handle_event_call_no_args_raises(self): """Test event() with no arguments raises ValueError.""" node = P.Call(func=P.Ident("event"), args=[]) with self.assertRaises(ValueError): - self.translator._handle_event_call( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_event_call(self.model, node, self.cfg, self.ctx) def test_handle_event_call_non_string_type_raises(self): """Test event(123) with non-string type raises TypeError.""" node = P.Call(func=P.Ident("event"), args=[P.Literal(123)]) with self.assertRaises(TypeError): - self.translator._handle_event_call( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_event_call(self.model, node, self.cfg, self.ctx) # ══════════════════════════════════════════════════════════════════════════ # _handle_has_event tests @@ -140,9 +130,7 @@ def test_handle_event_call_non_string_type_raises(self): def test_handle_has_event_basic(self): """Test has_event('type') produces EventExists.""" node = P.Call(func=P.Ident("has_event"), args=[P.Literal("survey")]) - result, explain = self.translator._handle_has_event( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_has_event(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventExists) self.assertEqual(result.event_type, "survey") @@ -155,9 +143,7 @@ def test_handle_has_event_with_temporal(self): args=[P.Literal("assessment")], kwargs={"within_days": P.Literal(365)}, ) - result, explain = self.translator._handle_has_event( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_has_event(self.model, node, self.cfg, self.ctx) self.assertEqual(result.within_days, 365) @@ -165,17 +151,13 @@ def test_handle_has_event_no_args_raises(self): """Test has_event() with no arguments raises ValueError.""" node = P.Call(func=P.Ident("has_event"), args=[]) with self.assertRaises(ValueError): - self.translator._handle_has_event( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_has_event(self.model, node, self.cfg, self.ctx) def test_handle_has_event_non_string_type_raises(self): """Test has_event(123) raises TypeError.""" node = P.Call(func=P.Ident("has_event"), args=[P.Literal(123)]) with self.assertRaises(TypeError): - self.translator._handle_has_event( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_has_event(self.model, node, self.cfg, self.ctx) # ══════════════════════════════════════════════════════════════════════════ # _handle_events_aggregate tests @@ -183,12 +165,8 @@ def test_handle_has_event_non_string_type_raises(self): def test_handle_events_count(self): """Test events_count('type') produces EventsAggregate with agg='count'.""" - node = P.Call( - func=P.Ident("events_count"), args=[P.Literal("attendance")] - ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + node = P.Call(func=P.Ident("events_count"), args=[P.Literal("attendance")]) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventsAggregate) self.assertEqual(result.event_type, "attendance") @@ -204,9 +182,7 @@ def test_handle_events_sum(self): func=P.Ident("events_sum"), args=[P.Literal("survey"), P.Literal("income")], ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertEqual(result.agg, "sum") self.assertEqual(result.field_name, "income") @@ -217,9 +193,7 @@ def test_handle_events_avg(self): func=P.Ident("events_avg"), args=[P.Literal("survey"), P.Literal("score")], ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertEqual(result.agg, "avg") self.assertEqual(result.field_name, "score") @@ -230,9 +204,7 @@ def test_handle_events_min(self): func=P.Ident("events_min"), args=[P.Literal("survey"), P.Literal("score")], ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertEqual(result.agg, "min") @@ -242,9 +214,7 @@ def test_handle_events_max(self): func=P.Ident("events_max"), args=[P.Literal("survey"), P.Literal("score")], ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertEqual(result.agg, "max") @@ -258,30 +228,22 @@ def test_handle_events_aggregate_with_kwargs(self): "states": P.Literal(["active", "superseded"]), }, ) - result, explain = self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) self.assertEqual(result.period, "2024") self.assertEqual(result.states, ["active", "superseded"]) def test_handle_events_sum_no_field_raises(self): """Test events_sum() without field argument raises ValueError.""" - node = P.Call( - func=P.Ident("events_sum"), args=[P.Literal("survey")] - ) + node = P.Call(func=P.Ident("events_sum"), args=[P.Literal("survey")]) with self.assertRaises(ValueError): - self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) def test_handle_events_count_no_args_raises(self): """Test events_count() with no arguments raises ValueError.""" node = P.Call(func=P.Ident("events_count"), args=[]) with self.assertRaises(ValueError): - self.translator._handle_events_aggregate( - self.model, node, self.cfg, self.ctx - ) + self.translator._handle_events_aggregate(self.model, node, self.cfg, self.ctx) # ══════════════════════════════════════════════════════════════════════════ # _handle_aggregate_compare tests @@ -291,14 +253,10 @@ def test_handle_aggregate_compare_ge(self): """Test events_count('type') >= 10 sets op and rhs.""" cmp = P.Compare( op="GE", - left=P.Call( - func=P.Ident("events_count"), args=[P.Literal("visit")] - ), + left=P.Call(func=P.Ident("events_count"), args=[P.Literal("visit")]), right=P.Literal(10), ) - result, explain = self.translator._handle_aggregate_compare( - self.model, cmp, self.cfg, self.ctx - ) + result, explain = self.translator._handle_aggregate_compare(self.model, cmp, self.cfg, self.ctx) self.assertIsInstance(result, EventsAggregate) self.assertEqual(result.op, ">=") @@ -308,14 +266,10 @@ def test_handle_aggregate_compare_eq(self): """Test events_count('type') == 5.""" cmp = P.Compare( op="EQ", - left=P.Call( - func=P.Ident("events_count"), args=[P.Literal("visit")] - ), + left=P.Call(func=P.Ident("events_count"), args=[P.Literal("visit")]), right=P.Literal(5), ) - result, explain = self.translator._handle_aggregate_compare( - self.model, cmp, self.cfg, self.ctx - ) + result, explain = self.translator._handle_aggregate_compare(self.model, cmp, self.cfg, self.ctx) self.assertEqual(result.op, "==") self.assertEqual(result.rhs, 5) @@ -330,9 +284,7 @@ def test_handle_aggregate_compare_lt(self): ), right=P.Literal(1000), ) - result, explain = self.translator._handle_aggregate_compare( - self.model, cmp, self.cfg, self.ctx - ) + result, explain = self.translator._handle_aggregate_compare(self.model, cmp, self.cfg, self.ctx) self.assertEqual(result.op, "<") self.assertEqual(result.rhs, 1000) @@ -345,9 +297,7 @@ def test_handle_aggregate_compare_lt(self): def test_extract_event_parameters_empty(self): """Test extracting parameters from node with no kwargs.""" node = P.Call(func=P.Ident("event"), args=[P.Literal("survey")]) - params = self.translator._extract_event_parameters( - node, self.ctx, start_index=1 - ) + params = self.translator._extract_event_parameters(node, self.ctx, start_index=1) self.assertEqual(params, {}) def test_extract_event_parameters_with_kwargs(self): @@ -361,9 +311,7 @@ def test_extract_event_parameters_with_kwargs(self): "default": P.Literal(0), }, ) - params = self.translator._extract_event_parameters( - node, self.ctx, start_index=1 - ) + params = self.translator._extract_event_parameters(node, self.ctx, start_index=1) self.assertEqual(params["select"], "latest") self.assertEqual(params["within_days"], 90) @@ -403,9 +351,7 @@ def test_handle_period_this_year(self): from datetime import date node = P.Call(func=P.Ident("this_year"), args=[]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) expected_year = str(date.today().year) self.assertIn(expected_year, explain) @@ -415,9 +361,7 @@ def test_handle_period_last_year(self): from datetime import date node = P.Call(func=P.Ident("last_year"), args=[]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) expected_year = str(date.today().year - 1) self.assertIn(expected_year, explain) @@ -425,9 +369,7 @@ def test_handle_period_last_year(self): def test_handle_period_this_quarter(self): """Test this_quarter() returns current quarter string.""" node = P.Call(func=P.Ident("this_quarter"), args=[]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) # Should contain a quarter like '2026-Q1' self.assertRegex(explain, r"\d{4}-Q[1-4]") @@ -435,9 +377,7 @@ def test_handle_period_this_quarter(self): def test_handle_period_this_month(self): """Test this_month() returns current month string.""" node = P.Call(func=P.Ident("this_month"), args=[]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) # Should contain a month like '2026-03' self.assertRegex(explain, r"\d{4}-\d{2}") @@ -445,18 +385,14 @@ def test_handle_period_this_month(self): def test_handle_period_quarters_ago(self): """Test quarters_ago(n) returns correct quarter string.""" node = P.Call(func=P.Ident("quarters_ago"), args=[P.Literal(1)]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) self.assertRegex(explain, r"\d{4}-Q[1-4]") def test_handle_period_months_ago(self): """Test months_ago(n) returns correct month string.""" node = P.Call(func=P.Ident("months_ago"), args=[P.Literal(2)]) - result, explain = self.translator._handle_period_function( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._handle_period_function(self.model, node, self.cfg, self.ctx) self.assertRegex(explain, r"\d{4}-\d{2}") @@ -470,17 +406,13 @@ def test_to_plan_event_field_compare(self): node = P.Compare( op="GT", left=P.Attr( - obj=P.Call( - func=P.Ident("event"), args=[P.Literal("survey")] - ), + obj=P.Call(func=P.Ident("event"), args=[P.Literal("survey")]), name="income", ), right=P.Literal(500), ) - result, explain = self.translator._to_plan( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._to_plan(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventValueCompare) self.assertEqual(result.event_type, "survey") @@ -492,9 +424,7 @@ def test_to_plan_has_event(self): """Test full _to_plan for has_event('type').""" node = P.Call(func=P.Ident("has_event"), args=[P.Literal("survey")]) - result, explain = self.translator._to_plan( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._to_plan(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventExists) self.assertEqual(result.event_type, "survey") @@ -510,9 +440,7 @@ def test_to_plan_aggregate_compare(self): right=P.Literal(10), ) - result, explain = self.translator._to_plan( - self.model, node, self.cfg, self.ctx - ) + result, explain = self.translator._to_plan(self.model, node, self.cfg, self.ctx) self.assertIsInstance(result, EventsAggregate) self.assertEqual(result.op, ">=") From 15a1c4797a7c5981f8ae3fbf1c02491b3ce8f340 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:30:33 +0800 Subject: [PATCH 06/12] fix: remove comment from ir.model.access.csv that broke CSV loading Odoo's convert_csv_import doesn't support comment lines in CSV files. The comment explaining the header-only nature of this file was causing a module installation failure. --- spp_cel_event/security/ir.model.access.csv | 1 - 1 file changed, 1 deletion(-) diff --git a/spp_cel_event/security/ir.model.access.csv b/spp_cel_event/security/ir.model.access.csv index 43791afd..97dd8b91 100644 --- a/spp_cel_event/security/ir.model.access.csv +++ b/spp_cel_event/security/ir.model.access.csv @@ -1,2 +1 @@ -# No ACLs needed - this module only extends existing models via _inherit (no new _name declarations) id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink From dc76391db5a52090d459e0229271910ae1119ce2 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:32:33 +0800 Subject: [PATCH 07/12] fix: correct two test failures in spp_cel_event - Fix test_event_within_months_filter: use within_months=3 instead of 2 to avoid PostgreSQL INTERVAL boundary issue with 60-day-old records - Fix test_event_aggregation_without_type: expect ValidationError since the new constraint from Item 7b rejects missing event_agg_type_id --- .../tests/test_cel_event_integration.py | 9 ++++-- .../tests/test_cel_variable_event_agg.py | 30 +++++++++---------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/spp_cel_event/tests/test_cel_event_integration.py b/spp_cel_event/tests/test_cel_event_integration.py index 0cbaf4cd..a079c0e9 100644 --- a/spp_cel_event/tests/test_cel_event_integration.py +++ b/spp_cel_event/tests/test_cel_event_integration.py @@ -345,14 +345,17 @@ def test_event_within_months_filter(self): """Test event with within_months temporal filter.""" from ..models.cel_event_queryplan import EventValueCompare - # Poor registrant survey is 30 days ago, within 2 months - # Rich registrant survey is 400 days ago, NOT within 2 months + # Poor registrant survey is 30 days ago, within 3 months + # Middle registrant survey is 60 days ago, within 3 months + # Rich registrant survey is 400 days ago, NOT within 3 months + # Note: using 3 months instead of 2 to avoid boundary issues with + # PostgreSQL INTERVAL month arithmetic near the 60-day mark. plan = EventValueCompare( event_type="household_survey", field_name="income", op=">", rhs=0, - within_months=2, + within_months=3, ) executor = self.env["spp.cel.executor"] diff --git a/spp_cel_event/tests/test_cel_variable_event_agg.py b/spp_cel_event/tests/test_cel_variable_event_agg.py index 26158fb2..eff54c6a 100644 --- a/spp_cel_event/tests/test_cel_variable_event_agg.py +++ b/spp_cel_event/tests/test_cel_variable_event_agg.py @@ -238,21 +238,21 @@ def test_event_with_custom_filter(self): ) self.assertIn("where='e.amount > 1000'", variable.cel_expression) - def test_event_aggregation_without_type_uses_accessor(self): - """Test that missing event type returns cel_accessor.""" - variable = self.CELVariable.create( - { - "name": "test_no_type", - "cel_accessor": "no_type_var", - "source_type": "aggregate", - "aggregate_target": "events", - "aggregate_type": "count", - "value_type": "number", - "category_id": self.category.id, - # No event_agg_type_id - } - ) - self.assertEqual(variable.cel_expression, "no_type_var") + def test_event_aggregation_without_type_raises_validation(self): + """Test that missing event type raises ValidationError.""" + with self.assertRaises(ValidationError): + self.CELVariable.create( + { + "name": "test_no_type", + "cel_accessor": "no_type_var", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "count", + "value_type": "number", + "category_id": self.category.id, + # No event_agg_type_id — constraint should reject this + } + ) def test_onchange_aggregate_target_clears_event_fields(self): """Test that changing aggregate_target clears event-specific fields.""" From 182c8c628466ce63a4cf5890bff70b03170e6232 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 03:40:03 +0800 Subject: [PATCH 08/12] refactor: apply simplification and address expert review findings Code simplification (code-simplifier): - Extract _warn_if_limit_reached() helper to deduplicate 3 call sites - Flatten _build_selection_sql() and _get_default_states() branches - Replace if/elif chains with dict lookups in _compute_aggregation, _handle_period_function, and _build_event_aggregate_cel - Simplify _compare_value() string fallback Expert review fixes (code-reviewer): - Add 128-char length cap on field names in _validate_field_name - Add encoding="utf-8" to open() in hooks.py - Remove stale EventsCollection reference from queryplan docstring UX review fixes (ux-expert): - Add required attribute on event_agg_field for sum/avg/min/max - Add required attribute on event_agg_temporal_value for within_days/months - Add name="event_agg_expression_preview" on preview div --- spp_cel_event/hooks.py | 2 +- spp_cel_event/models/cel_event_executor.py | 182 ++++++------------ spp_cel_event/models/cel_event_queryplan.py | 1 - spp_cel_event/models/cel_event_translator.py | 32 ++- .../models/cel_variable_event_agg.py | 23 +-- .../views/cel_variable_event_agg_views.xml | 9 +- 6 files changed, 92 insertions(+), 157 deletions(-) diff --git a/spp_cel_event/hooks.py b/spp_cel_event/hooks.py index c216c440..25c21a21 100644 --- a/spp_cel_event/hooks.py +++ b/spp_cel_event/hooks.py @@ -15,7 +15,7 @@ def post_init_hook(env): sql_file = os.path.join(module_path, "data", "cel_event_indexes.sql") if os.path.exists(sql_file): - with open(sql_file) as f: + with open(sql_file, encoding="utf-8") as f: sql = f.read() # Execute SQL diff --git a/spp_cel_event/models/cel_event_executor.py b/spp_cel_event/models/cel_event_executor.py index fd656830..02b10776 100644 --- a/spp_cel_event/models/cel_event_executor.py +++ b/spp_cel_event/models/cel_event_executor.py @@ -106,19 +106,11 @@ def _exec_event_value_sql(self, model: str, plan: EventValueCompare) -> list[int 3. Extracts and compares the field value 4. Returns matching partner IDs """ - # Build temporal filter SQL temporal_clause, temporal_args = self._build_temporal_filter(plan) - - # Build state filter SQL state_clause, state_args = self._build_state_filter(plan) - - # Build selection SQL (DISTINCT ON for latest/first, simple filter for active/any) selection_sql, selection_wrapper = self._build_selection_sql(plan) - - # Build field comparison SQL comparison_sql, comparison_args = self._build_field_comparison_sql(plan) - # Assemble the complete query if selection_wrapper: # For latest/first selection modes, we need a subquery with DISTINCT ON sql = SQL( @@ -142,8 +134,8 @@ def _exec_event_value_sql(self, model: str, plan: EventValueCompare) -> list[int self.MAX_QUERY_RESULTS, ) else: - # For active/any modes, simpler query without subquery - # Need to use 'e.' prefix for simple queries (no latest_event alias) + # For active/any modes, simpler query without subquery. + # Need to use 'e.' prefix (no latest_event alias). simple_comparison_sql = comparison_sql.replace("latest_event.", "e.") sql = SQL( f""" @@ -162,31 +154,17 @@ def _exec_event_value_sql(self, model: str, plan: EventValueCompare) -> list[int self.MAX_QUERY_RESULTS, ) - # Execute query self.env.cr.execute(sql) partner_ids = [row[0] for row in self.env.cr.fetchall()] - # Warn if limit was reached - if len(partner_ids) >= self.MAX_QUERY_RESULTS: - _logger.warning( - "[CEL EVENT] Query hit result limit (%d). Results may be truncated. " - "Consider using more specific filters.", - self.MAX_QUERY_RESULTS, - ) - - # Log operation at INFO level without PII + self._warn_if_limit_reached(partner_ids) _logger.info( "[CEL EVENT] EventValueCompare SQL: event_type=%s field=%s matches=%d", plan.event_type, plan.field_name, len(partner_ids), ) - # Log full details at DEBUG level (not shown in production) - _logger.debug( - "[CEL EVENT] EventValueCompare SQL details: op=%s rhs=%r", - plan.op, - plan.rhs, - ) + _logger.debug("[CEL EVENT] EventValueCompare SQL details: op=%s rhs=%r", plan.op, plan.rhs) return partner_ids @@ -242,19 +220,13 @@ def _exec_event_value_python(self, model: str, plan: EventValueCompare) -> list[ ) continue - # Log operation at INFO level without PII _logger.info( "[CEL EVENT] EventValueCompare Python: event_type=%s field=%s matches=%d", plan.event_type, plan.field_name, len(matching_ids), ) - # Log full details at DEBUG level (not shown in production) - _logger.debug( - "[CEL EVENT] EventValueCompare Python details: op=%s rhs=%r", - plan.op, - plan.rhs, - ) + _logger.debug("[CEL EVENT] EventValueCompare Python details: op=%s rhs=%r", plan.op, plan.rhs) return matching_ids @@ -290,23 +262,11 @@ def _exec_event_exists(self, model: str, plan: EventExists) -> list[int]: self.MAX_QUERY_RESULTS, ) - # Execute query self.env.cr.execute(sql) partner_ids = [row[0] for row in self.env.cr.fetchall()] - # Warn if limit was reached - if len(partner_ids) >= self.MAX_QUERY_RESULTS: - _logger.warning( - "[CEL EVENT] Query hit result limit (%d). Results may be truncated. " - "Consider using more specific filters.", - self.MAX_QUERY_RESULTS, - ) - - _logger.info( - "[CEL EVENT] EventExists SQL: event_type=%s matches=%d", - plan.event_type, - len(partner_ids), - ) + self._warn_if_limit_reached(partner_ids) + _logger.info("[CEL EVENT] EventExists SQL: event_type=%s matches=%d", plan.event_type, len(partner_ids)) return partner_ids @@ -403,19 +363,10 @@ def _exec_event_aggregate_sql(self, model: str, plan: EventsAggregate) -> list[i self.MAX_QUERY_RESULTS, ) - # Execute query self.env.cr.execute(sql) partner_ids = [row[0] for row in self.env.cr.fetchall()] - # Warn if limit was reached - if len(partner_ids) >= self.MAX_QUERY_RESULTS: - _logger.warning( - "[CEL EVENT] Query hit result limit (%d). Results may be truncated. " - "Consider using more specific filters.", - self.MAX_QUERY_RESULTS, - ) - - # Log operation at INFO level without PII + self._warn_if_limit_reached(partner_ids) _logger.info( "[CEL EVENT] EventsAggregate SQL: event_type=%s agg=%s field=%s matches=%d", plan.event_type, @@ -423,12 +374,7 @@ def _exec_event_aggregate_sql(self, model: str, plan: EventsAggregate) -> list[i plan.field_name, len(partner_ids), ) - # Log full details at DEBUG level (not shown in production) - _logger.debug( - "[CEL EVENT] EventsAggregate SQL details: op=%s rhs=%r", - plan.op, - plan.rhs, - ) + _logger.debug("[CEL EVENT] EventsAggregate SQL details: op=%s rhs=%r", plan.op, plan.rhs) return partner_ids @@ -579,17 +525,11 @@ def _build_selection_sql(self, plan: EventValueCompare) -> tuple[str, bool]: """ select_mode = plan.select if plan.select != "auto" else self._resolve_select_mode(plan.event_type) - if select_mode == "latest": - return "e.collection_date DESC, e.id DESC", True - elif select_mode == "latest_active": - return "e.collection_date DESC, e.id DESC", True - elif select_mode == "first": - return "e.collection_date ASC, e.id ASC", True - elif select_mode in ("active", "any"): - # No ordering needed, simple filter + if select_mode in ("active", "any"): return "", False - - # Default to latest + if select_mode == "first": + return "e.collection_date ASC, e.id ASC", True + # latest, latest_active, and unknown modes all use latest ordering return "e.collection_date DESC, e.id DESC", True def _build_field_comparison_sql(self, plan: EventValueCompare) -> tuple[str, list[Any]]: @@ -697,29 +637,23 @@ def _select_event(self, events, select_mode: str): return False if select_mode == "auto": - select_mode = "latest" # Default + select_mode = "latest" if select_mode == "active": - # Return first active (should be only one) active = events.filtered(lambda e: e.state == "active") return active[0] if active else False - elif select_mode == "latest": - # Most recent by collection_date - return events.sorted(lambda e: (e.collection_date, e.id), reverse=True)[0] - elif select_mode == "latest_active": - # Most recent among active + + if select_mode == "latest_active": active = events.filtered(lambda e: e.state == "active") - if active: - return active.sorted(lambda e: (e.collection_date, e.id), reverse=True)[0] - return False - elif select_mode == "first": - # Earliest by collection_date + return active.sorted(lambda e: (e.collection_date, e.id), reverse=True)[0] if active else False + + if select_mode == "first": return events.sorted(lambda e: (e.collection_date, e.id))[0] - elif select_mode == "any": - # Any event (first in recordset) + + if select_mode == "any": return events[0] - # Default: latest + # latest mode and unknown modes default to most recent return events.sorted(lambda e: (e.collection_date, e.id), reverse=True)[0] def _compute_aggregation(self, events, agg: str, field_name: str | None) -> Any: @@ -739,13 +673,11 @@ def _compute_aggregation(self, events, agg: str, field_name: str | None) -> Any: if not field_name: return 0 - # Extract field values values = [] for event in events: val = event.get_data_value(field_name) if val is not None: try: - # Try to convert to numeric values.append(float(val)) except (ValueError, TypeError): pass @@ -753,16 +685,13 @@ def _compute_aggregation(self, events, agg: str, field_name: str | None) -> Any: if not values: return 0 - if agg == "sum": - return sum(values) - elif agg == "avg": - return sum(values) / len(values) - elif agg == "min": - return min(values) - elif agg == "max": - return max(values) - - return 0 + agg_funcs = { + "sum": sum, + "avg": lambda v: sum(v) / len(v), + "min": min, + "max": max, + } + return agg_funcs.get(agg, lambda v: 0)(values) def _compare_value(self, a: Any, op: str, b: Any) -> bool: """Compare two values with given operator. @@ -775,14 +704,13 @@ def _compare_value(self, a: Any, op: str, b: Any) -> bool: Returns: True if comparison holds, False otherwise """ - # Handle None if a is None: return (op == "==" and b is None) or (op == "!=" and b is not None) # Try numeric comparison first try: - a_num = float(a) if not isinstance(a, bool) else a - b_num = float(b) if not isinstance(b, bool) else b + a_cmp = float(a) if not isinstance(a, bool) else a + b_cmp = float(b) if not isinstance(b, bool) else b ops = { "==": lambda x, y: x == y, "!=": lambda x, y: x != y, @@ -791,24 +719,34 @@ def _compare_value(self, a: Any, op: str, b: Any) -> bool: "<": lambda x, y: x < y, "<=": lambda x, y: x <= y, } - return ops[op](a_num, b_num) + return ops[op](a_cmp, b_cmp) except (ValueError, TypeError, KeyError): pass - # Fall back to string comparison + # Fall back to string comparison (only == and != are meaningful for strings) try: - ops = { - "==": lambda x, y: x == y, - "!=": lambda x, y: x != y, - } - return ops.get(op, lambda x, y: False)(str(a), str(b)) - except (ValueError, TypeError, KeyError): - return False + if op == "==": + return str(a) == str(b) + if op == "!=": + return str(a) != str(b) + except (ValueError, TypeError): + pass + + return False # ══════════════════════════════════════════════════════════════════════════════ # Utility Helpers # ══════════════════════════════════════════════════════════════════════════════ + def _warn_if_limit_reached(self, partner_ids: list[int]) -> None: + """Emit a warning when query results hit MAX_QUERY_RESULTS.""" + if len(partner_ids) >= self.MAX_QUERY_RESULTS: + _logger.warning( + "[CEL EVENT] Query hit result limit (%d). Results may be truncated. " + "Consider using more specific filters.", + self.MAX_QUERY_RESULTS, + ) + def _validate_field_name(self, field_name: str) -> str: """Validate and sanitize field name to prevent SQL injection. @@ -824,9 +762,12 @@ def _validate_field_name(self, field_name: str) -> str: if not field_name or not isinstance(field_name, str): raise ValueError("Field name must be a non-empty string") - # SECURITY: This regex is the sole barrier against SQL injection for field names - # interpolated into SQL queries via _build_field_comparison_sql and _exec_event_aggregate_sql. - # Do not relax this pattern without also changing those methods to use parameterized field names. + # SECURITY: This regex and length cap are the barriers against SQL injection for field + # names interpolated into SQL queries via _build_field_comparison_sql and + # _exec_event_aggregate_sql. Do not relax without also changing those methods to use + # parameterized field names. + if len(field_name) > 128: + raise ValueError(f"Field name too long ({len(field_name)} chars, max 128)") if not re.match(r"^[a-zA-Z][a-zA-Z0-9_]*$", field_name): raise ValueError(f"Invalid field name: {field_name!r}. Only alphanumeric and underscore allowed.") @@ -841,15 +782,10 @@ def _get_default_states(self, select_mode: str) -> list[str]: Returns: List of state strings """ - if select_mode in ("active", "latest_active"): - return ["active"] - elif select_mode in ("latest", "first"): + if select_mode in ("latest", "first"): return ["active", "superseded", "expired"] - elif select_mode == "any": - return ["active"] - - # Default: include historical states - return ["active", "superseded", "expired"] + # active, latest_active, any — and unknown modes — all default to active only + return ["active"] def _resolve_select_mode(self, event_type_code: str) -> str: """Resolve 'auto' select mode to actual mode based on event type config. diff --git a/spp_cel_event/models/cel_event_queryplan.py b/spp_cel_event/models/cel_event_queryplan.py index 19afc41e..9ac3c173 100644 --- a/spp_cel_event/models/cel_event_queryplan.py +++ b/spp_cel_event/models/cel_event_queryplan.py @@ -10,7 +10,6 @@ EventValueCompare: Compare a field value from a registrant's event EventExists: Check if a matching event exists EventsAggregate: Aggregate values across multiple events - EventsCollection: Collection operations (exists, count, all, any) EventFieldRef: Reference to an event field (intermediate representation) Example: diff --git a/spp_cel_event/models/cel_event_translator.py b/spp_cel_event/models/cel_event_translator.py index 8fbf0927..69064e79 100644 --- a/spp_cel_event/models/cel_event_translator.py +++ b/spp_cel_event/models/cel_event_translator.py @@ -429,29 +429,27 @@ def _handle_period_function(self, model: str, node: P.Call, cfg: dict[str, Any], func_name = node.func.name today = fields.Date.context_today(self) - period_str = None - - if func_name == "this_year": - period_str = event_funcs.this_year(today) - elif func_name == "last_year": - period_str = event_funcs.last_year(today) - elif func_name == "this_quarter": - period_str = event_funcs.this_quarter(today) - elif func_name == "last_quarter": - period_str = event_funcs.last_quarter(today) - elif func_name == "this_month": - period_str = event_funcs.this_month(today) - elif func_name == "last_month": - period_str = event_funcs.last_month(today) + # No-arg period functions map directly to event_funcs callables + no_arg_funcs = { + "this_year": event_funcs.this_year, + "last_year": event_funcs.last_year, + "this_quarter": event_funcs.this_quarter, + "last_quarter": event_funcs.last_quarter, + "this_month": event_funcs.this_month, + "last_month": event_funcs.last_month, + } + + if func_name in no_arg_funcs: + period_str = no_arg_funcs[func_name](today) elif func_name == "quarters_ago": n = self._eval_literal(node.args[0], ctx) if node.args else 0 period_str = event_funcs.quarters_ago(int(n), today) - elif func_name == "months_ago": + else: # months_ago n = self._eval_literal(node.args[0], ctx) if node.args else 0 period_str = event_funcs.months_ago(int(n), today) - # Return as a LeafDomain that matches everything (period is a constant value) - # The period will be used by event functions via _eval_literal + # Return as a LeafDomain that matches everything (period is a constant value). + # The period will be used by event functions via _eval_literal. return LeafDomain(model, [("id", "!=", 0)]), f"{func_name}() = '{period_str}'" def _extract_event_parameters(self, node: P.Call, ctx: dict[str, Any], start_index: int = 1) -> dict[str, Any]: diff --git a/spp_cel_event/models/cel_variable_event_agg.py b/spp_cel_event/models/cel_variable_event_agg.py index 39ba35a8..1f362f5b 100644 --- a/spp_cel_event/models/cel_variable_event_agg.py +++ b/spp_cel_event/models/cel_variable_event_agg.py @@ -142,20 +142,15 @@ def _build_event_aggregate_cel(self): parts.append(f"'{field}'") # Temporal filter - temporal_part = None - if self.event_agg_temporal == "this_year": - temporal_part = "period=this_year()" - elif self.event_agg_temporal == "this_quarter": - temporal_part = "period=this_quarter()" - elif self.event_agg_temporal == "this_month": - temporal_part = "period=this_month()" - elif self.event_agg_temporal == "within_days" and self.event_agg_temporal_value: - temporal_part = f"within_days={self.event_agg_temporal_value}" - elif self.event_agg_temporal == "within_months" and self.event_agg_temporal_value: - temporal_part = f"within_months={self.event_agg_temporal_value}" - - if temporal_part: - parts.append(temporal_part) + named_period_map = { + "this_year": "period=this_year()", + "this_quarter": "period=this_quarter()", + "this_month": "period=this_month()", + } + if self.event_agg_temporal in named_period_map: + parts.append(named_period_map[self.event_agg_temporal]) + elif self.event_agg_temporal in ("within_days", "within_months") and self.event_agg_temporal_value: + parts.append(f"{self.event_agg_temporal}={self.event_agg_temporal_value}") # States filter if self.event_agg_states == "all": diff --git a/spp_cel_event/views/cel_variable_event_agg_views.xml b/spp_cel_event/views/cel_variable_event_agg_views.xml index 1cd0e52a..f4438547 100644 --- a/spp_cel_event/views/cel_variable_event_agg_views.xml +++ b/spp_cel_event/views/cel_variable_event_agg_views.xml @@ -28,11 +28,13 @@ @@ -46,7 +48,12 @@ -
+
+
+

Prerequisites

+
    +
  • Module spp_cel_event is installed (auto-installs when +spp_cel_domain, spp_event_data, and spp_studio are all +present)
    • +
  • At least one Event Type exists (e.g., code payment, target +type individual)
    • +
  • User has access to the Studio menu
    • +
+
+ +
+

Test 1: Event Aggregation Section Visibility

+ +++++ + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Source Type to +“Aggregate”Aggregate fields appear
2Set Aggregate Target to +“Members”Event Aggregation section is +hidden
3Set Aggregate Target to +“Events”Event Aggregation section +appears with: Event Type, +Time Range, Event States, and +Generated Expression preview
+
+
+

Test 2: Count Aggregation

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Source Type = +Aggregate 
2Set Aggregate Target = +EventsEvent Aggregation section +appears
3Set Aggregate Type = +Count 
4Set Event Type = Payment +EventWarning disappears
5Leave Time Range = All +Time 
6Check Generated +Expressionevents_count('payment')
7Event Field should be +hiddenField not visible (count +doesn’t need it)
+
+
+

Test 3: Exists Aggregation

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Aggregate Type = +Exists 
2Set Event Type = Payment +Event 
3Check Generated +Expressionhas_event('payment')
4Event Field should be +hiddenField not visible (exists +doesn’t need it)
+
+
+

Test 4: Sum Aggregation (also applies to avg, min, max)

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Aggregate Type = SumEvent Field becomes visible and +required
2Set Event Type = Payment +Event 
3Type amount in Event +Field 
4Check Generated +Expressionevents_sum('payment', 'amount')
5Change Aggregate Type to +AvgExpression updates to +events_avg('payment', 'amount')
6Change Aggregate Type to +MinExpression updates to +events_min('payment', 'amount')
7Change Aggregate Type to +MaxExpression updates to +events_max('payment', 'amount')
+
+
+

Test 5: Temporal Filters

+

5a: Named Periods

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set up a Count aggregation +with an event type 
2Set Time Range = This +YearExpression: +events_count('payment', period=this_year())
3Set Time Range = This +QuarterExpression: +events_count('payment', period=this_quarter())
4Set Time Range = This +MonthExpression: +events_count('payment', period=this_month())
5Number of Days/Months +field is hiddenField not visible for named periods
+

5b: Within N Days

+ +++++ + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Time Range = Within N +DaysNumber of Days/Months field appears and +is required
2Enter 90Expression: +events_count('payment', within_days=90)
+

5c: Within N Months

+ +++++ + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Time Range = Within N +MonthsNumber of Days/Months field appears and +is required
2Enter 6Expression: +events_count('payment', within_months=6)
+
+
+

Test 6: Event States Filter

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set up a Count aggregation +with an event type 
2Event States defaults to +“Active Only”No states= in expression
3Set Event States = All +StatesExpression includes states=['active', 'superseded', 'expired']
4Example full expressionevents_count('payment', states=['active', 'superseded', 'expired'])
+
+
+

Test 7: Combined Filters

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Aggregate Type = Sum 
2Event Type = Payment +Event 
3Event Field = amount 
4Time Range = Within N +Months, value = 6 
5Event States = All States 
6Check Generated +Expressionevents_sum('payment', 'amount', within_months=6, states=['active', 'superseded', 'expired'])
+
+
+

Test 8: Validation Errors

+

8a: Missing Event Type

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Source Type = +Aggregate 
2Set Aggregate Target = +Events 
3Set Aggregate Type = +Count 
4Leave Event Type empty 
5Fill remaining required +fields and click SaveValidationError: event +type is required
+

8b: Missing Event Field for Sum/Avg/Min/Max

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Aggregate Type = Sum 
2Set Event Type = Payment +Event 
3Leave Event Field empty 
4Click SaveValidationError: field is +required for sum
+

8c: Missing Temporal Value

+ +++++ + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Time Range = Within N +Days 
2Leave Number of +Days/Months empty or 0 
3Click SaveValidationError: positive +value required
+
+
+

Test 9: Onchange Field Clearing

+

9a: Switching Away from Events

+ +++++ + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Create a variable with +Aggregate Target = +Events, fully configured 
2Change Aggregate Target +to “Members”Event Type, Time Range, Event +Field, Event States reset to +defaults
3Change back to “Events”Fields are blank/default and +need reconfiguration
+

9b: Switching Aggregate Type to Count/Exists

+ +++++ + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Aggregate Type = Sum, +Event Field = amount 
2Change Aggregate Type to +CountEvent Field is cleared +and hidden
3Change Aggregate Type to +ExistsEvent Field remains +cleared and hidden
+

9c: Switching Temporal Type

+ +++++ + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Time Range = Within N +Days, value = 30 
2Change Time Range to This +YearNumber of Days/Months is +cleared and hidden
+
+
+

Test 10: Expression Preview Warning

+ +++++ + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set Aggregate Target = +EventsGenerated Expression preview +appears
2Leave Event Type emptyWarning icon with: +“Configuration incomplete. +Select an event type to +generate the expression.”
3Select an Event TypeWarning disappears, valid +expression shown
+
+
+

Test 11: Aggregate Filter is Excluded

+ +++++ + + + + + + + + + + + + + + + + + + + + + + + + +
StepActionExpected Result
1Set up any event aggregation +(e.g., Sum) 
2Enter a value in Aggregate +Filter (e.g., +e.amount > 1000) 
3Check Generated +ExpressionExpression does NOT +contain where=
4The filter value is stored +but not included in the +generated expressionThis is by design +(where_predicate is not yet +implemented in the executor)
-

Bug Tracker

+

Bug Tracker

Bugs are tracked on GitHub 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 @@ -476,15 +1155,15 @@

Bug Tracker

Do not contact contributors directly about support or help with technical issues.

-

Credits

+

Credits

-

Authors

+

Authors

  • OpenSPP.org
-

Maintainers

+

Maintainers

Current maintainers:

jeremi gonzalesedwin1123 emjay0921

This module is part of the OpenSPP/OpenSPP2 project on GitHub.

From 76d425a5c33be6b87a29751813e39310d569f46e Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 13 Mar 2026 04:17:02 +0800 Subject: [PATCH 12/12] test: add 23 tests to improve patch coverage for spp_cel_event Cover previously untested code paths: - _build_has_event_cel with temporal filters, named periods, all states - _check_event_aggregation_config for sum/avg without field - _onchange_aggregate_type_event and _onchange_event_agg_temporal - _validate_field_name length cap and edge cases - _warn_if_limit_reached helper - _build_selection_sql for active/latest/first modes - _compare_value string fallback and None handling - _compute_aggregation dict-based dispatch - min/max/this_month expression generation --- .../tests/test_cel_event_integration.py | 160 +++++++++++++++ .../tests/test_cel_variable_event_agg.py | 186 ++++++++++++++++++ 2 files changed, 346 insertions(+) diff --git a/spp_cel_event/tests/test_cel_event_integration.py b/spp_cel_event/tests/test_cel_event_integration.py index a079c0e9..a579941b 100644 --- a/spp_cel_event/tests/test_cel_event_integration.py +++ b/spp_cel_event/tests/test_cel_event_integration.py @@ -1098,3 +1098,163 @@ def test_get_default_states_for_select_mode(self): get_states_for_select_mode("first"), ["active", "superseded", "expired"], ) + + # ────────────────────────────────────────────────────────────────────────── + # Executor utility method tests + # ────────────────────────────────────────────────────────────────────────── + + def test_validate_field_name_valid(self): + """Test that valid field names pass validation.""" + executor = self.env["spp.cel.executor"] + self.assertEqual(executor._validate_field_name("income"), "income") + self.assertEqual(executor._validate_field_name("total_amount"), "total_amount") + self.assertEqual(executor._validate_field_name("score2"), "score2") + + def test_validate_field_name_empty(self): + """Test that empty field name raises ValueError.""" + executor = self.env["spp.cel.executor"] + with self.assertRaises(ValueError): + executor._validate_field_name("") + with self.assertRaises(ValueError): + executor._validate_field_name(None) + + def test_validate_field_name_invalid_chars(self): + """Test that field names with invalid characters raise ValueError.""" + executor = self.env["spp.cel.executor"] + with self.assertRaises(ValueError): + executor._validate_field_name("field-name") + with self.assertRaises(ValueError): + executor._validate_field_name("field name") + with self.assertRaises(ValueError): + executor._validate_field_name("1field") + + def test_validate_field_name_too_long(self): + """Test that field names exceeding 128 chars raise ValueError.""" + executor = self.env["spp.cel.executor"] + long_name = "a" * 129 + with self.assertRaises(ValueError): + executor._validate_field_name(long_name) + # 128 chars should be fine + result = executor._validate_field_name("a" * 128) + self.assertEqual(len(result), 128) + + def test_warn_if_limit_reached_no_warning(self): + """Test that no warning is emitted below limit.""" + executor = self.env["spp.cel.executor"] + # Should not raise or warn + executor._warn_if_limit_reached([1, 2, 3]) + + def test_warn_if_limit_reached_at_limit(self): + """Test that warning is emitted at MAX_QUERY_RESULTS.""" + executor = self.env["spp.cel.executor"] + large_list = list(range(executor.MAX_QUERY_RESULTS)) + with self.assertLogs("odoo.addons.spp_cel_event.models.cel_event_executor", level="WARNING") as log: + executor._warn_if_limit_reached(large_list) + self.assertTrue(any("result limit" in msg for msg in log.output)) + + def test_get_default_states_executor(self): + """Test executor's _get_default_states method.""" + executor = self.env["spp.cel.executor"] + self.assertEqual(executor._get_default_states("active"), ["active"]) + self.assertEqual(executor._get_default_states("latest_active"), ["active"]) + self.assertEqual(executor._get_default_states("any"), ["active"]) + self.assertEqual(executor._get_default_states("latest"), ["active", "superseded", "expired"]) + self.assertEqual(executor._get_default_states("first"), ["active", "superseded", "expired"]) + # Unknown mode defaults to active + self.assertEqual(executor._get_default_states("unknown"), ["active"]) + + def test_build_selection_sql_active_mode(self): + """Test _build_selection_sql for active/any modes (no wrapper).""" + from ..models.cel_event_queryplan import EventValueCompare + + executor = self.env["spp.cel.executor"] + plan = EventValueCompare( + event_type="test", + field_name="income", + op=">", + rhs=500, + select="active", + ) + order_clause, needs_wrapper = executor._build_selection_sql(plan) + self.assertEqual(order_clause, "") + self.assertFalse(needs_wrapper) + + def test_build_selection_sql_latest_mode(self): + """Test _build_selection_sql for latest mode (needs wrapper).""" + from ..models.cel_event_queryplan import EventValueCompare + + executor = self.env["spp.cel.executor"] + plan = EventValueCompare( + event_type="test", + field_name="income", + op=">", + rhs=500, + select="latest", + ) + order_clause, needs_wrapper = executor._build_selection_sql(plan) + self.assertIn("DESC", order_clause) + self.assertTrue(needs_wrapper) + + def test_build_selection_sql_first_mode(self): + """Test _build_selection_sql for first mode (ascending order).""" + from ..models.cel_event_queryplan import EventValueCompare + + executor = self.env["spp.cel.executor"] + plan = EventValueCompare( + event_type="test", + field_name="income", + op=">", + rhs=500, + select="first", + ) + order_clause, needs_wrapper = executor._build_selection_sql(plan) + self.assertIn("ASC", order_clause) + self.assertTrue(needs_wrapper) + + def test_compare_value_string_fallback(self): + """Test _compare_value with string comparison fallback.""" + executor = self.env["spp.cel.executor"] + self.assertTrue(executor._compare_value("hello", "==", "hello")) + self.assertTrue(executor._compare_value("hello", "!=", "world")) + self.assertFalse(executor._compare_value("hello", "==", "world")) + # Non-comparable operators fall back to False + self.assertFalse(executor._compare_value("hello", ">", "world")) + + def test_compare_value_none(self): + """Test _compare_value with None values.""" + executor = self.env["spp.cel.executor"] + self.assertTrue(executor._compare_value(None, "==", None)) + self.assertTrue(executor._compare_value(None, "!=", "something")) + self.assertFalse(executor._compare_value(None, "==", "something")) + + def test_compute_aggregation_dict_dispatch(self): + """Test _compute_aggregation with all aggregation types.""" + executor = self.env["spp.cel.executor"] + + # Create events with numeric data + events = self.env["spp.event.data"].create( + [ + { + "partner_id": self.registrant_poor.id, + "event_type_id": self.visit_type.id, + "event_type_code": "field_visit", + "data_json": {"score": 10}, + "collection_date": date.today(), + }, + { + "partner_id": self.registrant_poor.id, + "event_type_id": self.visit_type.id, + "event_type_code": "field_visit", + "data_json": {"score": 20}, + "collection_date": date.today(), + }, + ] + ) + + self.assertEqual(executor._compute_aggregation(events, "sum", "score"), 30.0) + self.assertEqual(executor._compute_aggregation(events, "avg", "score"), 15.0) + self.assertEqual(executor._compute_aggregation(events, "min", "score"), 10.0) + self.assertEqual(executor._compute_aggregation(events, "max", "score"), 20.0) + self.assertEqual(executor._compute_aggregation(events, "count", None), 2) + # Unknown agg type returns 0 + self.assertEqual(executor._compute_aggregation(events, "unknown_agg", "score"), 0) diff --git a/spp_cel_event/tests/test_cel_variable_event_agg.py b/spp_cel_event/tests/test_cel_variable_event_agg.py index 61d8fe16..fd48df4a 100644 --- a/spp_cel_event/tests/test_cel_variable_event_agg.py +++ b/spp_cel_event/tests/test_cel_variable_event_agg.py @@ -319,3 +319,189 @@ def test_within_months_requires_value(self): "event_agg_temporal_value": 0, # Invalid - must be positive } ) + + def test_sum_without_field_raises_validation(self): + """Test that sum aggregation without event_agg_field raises ValidationError.""" + with self.assertRaises(ValidationError): + self.CELVariable.create( + { + "name": "test_sum_no_field", + "cel_accessor": "sum_no_field", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "sum", + "value_type": "money", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + # No event_agg_field — constraint should reject + } + ) + + def test_avg_without_field_raises_validation(self): + """Test that avg aggregation without event_agg_field raises ValidationError.""" + with self.assertRaises(ValidationError): + self.CELVariable.create( + { + "name": "test_avg_no_field", + "cel_accessor": "avg_no_field", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "avg", + "value_type": "number", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + # No event_agg_field — constraint should reject + } + ) + + def test_has_event_with_temporal_filter(self): + """Test has_event expression with temporal filter.""" + variable = self.CELVariable.create( + { + "name": "test_has_recent_payment", + "cel_accessor": "has_recent_payment", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "exists", + "value_type": "boolean", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_temporal": "within_days", + "event_agg_temporal_value": 30, + } + ) + self.assertEqual(variable.cel_expression, "has_event('payment', within_days=30)") + + def test_has_event_with_named_period(self): + """Test has_event expression with named period.""" + variable = self.CELVariable.create( + { + "name": "test_has_payment_this_year", + "cel_accessor": "has_payment_year", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "exists", + "value_type": "boolean", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_temporal": "this_year", + } + ) + self.assertEqual(variable.cel_expression, "has_event('payment', period=this_year())") + + def test_has_event_with_all_states(self): + """Test has_event expression with all states filter.""" + variable = self.CELVariable.create( + { + "name": "test_has_any_payment", + "cel_accessor": "has_any_payment", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "exists", + "value_type": "boolean", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_states": "all", + } + ) + self.assertIn("states=['active', 'superseded', 'expired']", variable.cel_expression) + self.assertTrue(variable.cel_expression.startswith("has_event(")) + + def test_onchange_aggregate_type_clears_field(self): + """Test that switching to count/exists clears event_agg_field.""" + variable = self.CELVariable.create( + { + "name": "test_type_switch", + "cel_accessor": "type_switch_var", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "sum", + "value_type": "money", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_field": "amount", + } + ) + self.assertEqual(variable.event_agg_field, "amount") + + # Simulate onchange to count + variable.aggregate_type = "count" + variable._onchange_aggregate_type_event() + self.assertFalse(variable.event_agg_field) + + def test_onchange_temporal_resets_value(self): + """Test that switching temporal type resets the value.""" + variable = self.CELVariable.create( + { + "name": "test_temporal_switch", + "cel_accessor": "temporal_switch_var", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "count", + "value_type": "number", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_temporal": "within_days", + "event_agg_temporal_value": 90, + } + ) + self.assertEqual(variable.event_agg_temporal_value, 90) + + # Simulate onchange to named period + variable.event_agg_temporal = "this_year" + variable._onchange_event_agg_temporal() + self.assertEqual(variable.event_agg_temporal_value, 0) + + def test_event_min_cel_expression(self): + """Test CEL expression generation for event min.""" + variable = self.CELVariable.create( + { + "name": "test_min_score", + "cel_accessor": "min_score", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "min", + "value_type": "number", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_field": "score", + } + ) + self.assertEqual(variable.cel_expression, "events_min('payment', 'score')") + + def test_event_max_cel_expression(self): + """Test CEL expression generation for event max.""" + variable = self.CELVariable.create( + { + "name": "test_max_score", + "cel_accessor": "max_score", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "max", + "value_type": "number", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_field": "score", + } + ) + self.assertEqual(variable.cel_expression, "events_max('payment', 'score')") + + def test_event_this_month_temporal(self): + """Test CEL expression with this_month temporal filter.""" + variable = self.CELVariable.create( + { + "name": "test_mtd_count", + "cel_accessor": "mtd_count", + "source_type": "aggregate", + "aggregate_target": "events", + "aggregate_type": "count", + "value_type": "number", + "category_id": self.category.id, + "event_agg_type_id": self.event_type.id, + "event_agg_temporal": "this_month", + } + ) + self.assertEqual( + variable.cel_expression, + "events_count('payment', period=this_month())", + )