feat: upgrade spp_cel_event from Beta to Production/Stable #103
Open
gonzalesedwin1123 wants to merge 12 commits into19.0from
Open
feat: upgrade spp_cel_event from Beta to Production/Stable #103gonzalesedwin1123 wants to merge 12 commits into19.0from
gonzalesedwin1123 wants to merge 12 commits into19.0from
Conversation
… 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
- 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
- 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
- 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
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.
- 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
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
Critical: - Map "exists" aggregate type to has_event() instead of events_exists() which the translator does not handle. Previously generated unparseable CEL expressions for exists-type event variables. Important: - Add source_type/aggregate_target guard to _check_temporal_value constraint to prevent false positives on non-event variables - Update SPEC_COMPLIANCE.md: mark YYYY-HN and YYYY-WNN period formats as implemented (they were already working in cel_event_functions), remove stale "Not Implemented" section, update compliance to 97% - Remove where= generation from _build_event_aggregate_cel since where_predicate is not implemented in either execution path
The test_event_with_custom_filter test expected where= in the generated expression, but we intentionally removed that since where_predicate is not implemented. Updated test to verify where= is NOT generated.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request marks a significant milestone for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #103 +/- ##
==========================================
+ Coverage 71.17% 71.45% +0.28%
==========================================
Files 704 713 +9
Lines 38554 39367 +813
==========================================
+ Hits 27439 28131 +692
- Misses 11115 11236 +121
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and high-quality pull request that successfully upgrades the spp_cel_event module to a production-ready state. The changes are comprehensive, addressing critical bugs, removing dead code, adding extensive tests, and improving security, maintainability, and user experience. The refactoring throughout the codebase, such as centralizing period parsing and using dictionary dispatch, greatly improves code quality. The addition of numerous unit and integration tests is particularly valuable. I have one suggestion for a minor refactoring to reduce code duplication.
Comment on lines
149
to
162
| # 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": | ||
| parts.append("states=['active', 'superseded', 'expired']") |
There was a problem hiding this comment.
This block of code for building temporal and state filter arguments is duplicated in the new _build_has_event_cel method (lines 177-190). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this common logic should be extracted into a helper method.
You can create a helper method like this and place it before _build_event_aggregate_cel:
def _get_event_filter_parts(self):
"""Builds a list of CEL argument parts for temporal and state filters."""
parts = []
# Temporal filter
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":
parts.append("states=['active', 'superseded', 'expired']")
return partsThen, you can replace this block and the duplicated one in _build_has_event_cel with a single call to this new helper.
Suggested change
| # 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": | |
| parts.append("states=['active', 'superseded', 'expired']") | |
| parts.extend(self._get_event_filter_parts()) |
Add USAGE.md with 11 test scenarios covering all event aggregation workflows, validation errors, onchange behavior, and expression preview. Regenerate README.rst via oca-gen-addon-readme.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spp_cel_eventmodule from Beta to Production/Stable for public releaseevents_existsgap where generated expressions were unparseable by the translatorEventsCollectiondataclass), consolidate duplicate period parsingwhereparameter from profiles and expression generationauto_installfromTrueto explicit dependency list per module-visibility principlePlan
Execution followed the 13-item plan at
docs/plans/SPP_CEL_EVENT_STABLE_RELEASE_PLAN.md, with two rounds of staff engineer + UX expert review.Test plan
./scripts/test_single_module.sh spp_cel_event)auto_installtriggers only when all three dependencies are present