Conversation
Summary of ChangesHello @jeremi, 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 introduces a comprehensive system for managing metrics and statistics within OpenSPP. It establishes a robust core module that standardizes metric definitions and categorization, enabling consistent data representation and reducing redundancy. Building on this foundation, a new statistics module allows for the creation of publishable statistics, incorporating essential features like k-anonymity for privacy and flexible context-specific configurations. A dedicated studio module provides an intuitive user interface for configuring and overseeing these statistics, streamlining the process of making data available across various platforms and reports. Highlights
Changelog
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
|
![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 pull request introduces a new spp_metrics_core module to establish a unified foundation for all metric types, including statistics and simulations. The core module defines an abstract base model (spp.metric.base) for genuinely shared fields like identity, presentation, and categorization, and a shared category model (spp.metric.category). A pre-migration script is included to rename the old spp.statistic.category table and sequence to spp.metric.category, ensuring data preservation. The spp_statistic module is updated to depend on spp_metrics_core, with its spp.statistic model now inheriting from spp.metric.base and using spp.metric.category for categorization. The spp_statistic_studio module is also updated to reflect these changes in its views and menus. Review comments highlight several areas for improvement: ensuring correct SQL constraint definitions using _sql_constraints instead of models.Constraint in spp_statistic/models/statistic.py and spp_statistic/models/statistic_context.py, verifying the validity of relative documentation links in spp_metrics_core/README.md, correcting inconsistencies in the spp_metrics_core/__manifest__.py description regarding fields provided by the base model and the category model name, improving the domain string readability in spp_statistic/models/statistic.py, and refactoring spp_metrics_core tests to be self-contained by not depending on spp_statistic.
| # active inherited from spp.metric.base | ||
|
|
||
| # ─── Constraints ──────────────────────────────────────────────────── | ||
| _name_unique = models.Constraint("UNIQUE(name)", "Statistic name must be unique.") |
There was a problem hiding this comment.
Using models.Constraint is not the standard Odoo way to define a SQL constraint and will not create a database-level unique constraint. To ensure data integrity at the database level, you should use the _sql_constraints attribute.
_sql_constraints = [("name_unique", "UNIQUE(name)", "Statistic name must be unique!")]| _statistic_context_unique = models.Constraint( | ||
| "UNIQUE(statistic_id, context)", | ||
| "Each statistic can only have one configuration per context.", | ||
| ) |
There was a problem hiding this comment.
Using models.Constraint is not the standard Odoo way to define a SQL constraint and will not create a database-level unique constraint. To ensure a unique configuration per statistic and context, you should use the _sql_constraints attribute.
_sql_constraints = [
("statistic_context_unique", "UNIQUE(statistic_id, context)", "Each statistic can only have one configuration per context.")
]| category = env['spp.metric.category'].search([...]) | ||
| ``` | ||
|
|
||
| See [Migration Guide](../../docs/migration/statistics-refactoring.md) for details. |
There was a problem hiding this comment.
The relative links to documentation in this file (here and on line 177) might be broken depending on the repository structure or how the documentation is viewed. It would be beneficial to verify that these links resolve correctly to avoid confusion for developers.
spp_metrics_core/__manifest__.py
Outdated
There was a problem hiding this comment.
The Fields Provided list in the manifest's description is inconsistent with the implementation in metric_base.py and the README.md. The base model intentionally does not include metric_type, cel_expression, aggregation, or format fields; these are meant for concrete models to define. This section should be updated to accurately reflect the fields provided by spp.metric.base.
| - **Computation**: metric_type, cel_expression, aggregation | |
| - **Presentation**: format, unit, decimal_places | |
| - **Presentation**: unit, decimal_places |
|
|
||
|
|
||
| @tagged("post_install", "-at_install") | ||
| class TestMetricBase(TransactionCase): |
There was a problem hiding this comment.
The tests in spp_metrics_core depend on the spp_statistic module to test the functionality of the spp.metric.base abstract model. Core module tests should be self-contained to avoid circular dependencies and simplify testing.
Consider refactoring these tests to create a temporary, dummy model within the test case that inherits from spp.metric.base. This would allow you to test the abstract model's functionality in isolation, making the tests for this core module more robust and independent.
spp_statistic/__manifest__.py
Outdated
There was a problem hiding this comment.
The manifest description incorrectly refers to spp.statistic.category. This model has been migrated to spp.metric.category and now resides in the spp_metrics_core module. The documentation should be updated to reflect this change.
| - ``spp.statistic.category``: Organization categories for statistics | |
| - ``spp.metric.category``: Organization categories for statistics |
spp_statistic/models/statistic.py
Outdated
There was a problem hiding this comment.
The domain string is constructed by concatenating two separate string literals. While this is valid Python, it's unconventional and slightly harder to read. It's better to define the domain as a single string for clarity and maintainability.
domain="[('source_type', 'in', ['aggregate', 'computed', 'field']), ('state', '=', 'active')]"
spp_statistic_studio/__manifest__.py
Outdated
There was a problem hiding this comment.
The auto_install key in an Odoo manifest expects a boolean value. Setting it to True will ensure the module is installed automatically when all its dependencies are present. The current value is a list of strings, which is incorrect.
| "auto_install": ["spp_statistic", "spp_studio"], | |
| "auto_install": True, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #43 +/- ##
===========================================
- Coverage 71.31% 51.54% -19.78%
===========================================
Files 299 127 -172
Lines 23618 9463 -14155
===========================================
- Hits 16844 4878 -11966
+ Misses 6774 4585 -2189
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "unit": self.unit, | ||
| "decimal_places": self.decimal_places, | ||
| "category": self.category_id.code if self.category_id else None, | ||
| "group": config.get("group"), |
There was a problem hiding this comment.
to_dict group field missing fallback default value
Medium Severity
config.get("group") on line 308 lacks a fallback default, unlike label, format, and minimum_count which all provide one. When context is None, config is {} and group is always None, even when the statistic has a category_id. Calling to_dict("gis") returns the category code as group, but to_dict() returns None — an inconsistent result for API consumers.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| # Get context-specific config | ||
| config = self.get_context_config(context) if context else {} | ||
| min_count = config.get("minimum_count", self.minimum_count) or 5 |
There was a problem hiding this comment.
Falsy or check silently ignores zero minimum_count
High Severity
The or operator treats 0 as falsy, so minimum_count set to 0 is silently overridden. In apply_suppression, config.get("minimum_count", self.minimum_count) or 5 replaces a zero value with 5. In get_context_config, ctx_config.minimum_count or self.minimum_count ignores a context override of 0. This makes it impossible to configure a lower-than-default or zero suppression threshold — a privacy-critical setting that users may intentionally adjust.
Additional Locations (1)
New modules: - spp_metrics_core: unified foundation for all metrics (categories, base model) - spp_statistic: publishable statistics with k-anonymity privacy protection - spp_statistic_studio: Studio UI for statistics configuration (auto-installs when both spp_statistic and spp_studio are present)
…to_install value - Set auto_install to boolean True in spp_statistic_studio (was a list) - Remove non-existent fields from spp_metrics_core Fields Provided section - Fix model name reference from spp.statistic.category to spp.metric.category - Combine split domain string literals in statistic.py into one
Empty recordsets in Odoo are falsy, so checking `if privacy_service:` would always evaluate to False even when the model is installed. Changed to `if privacy_service is not None:` to correctly detect when the spp.metrics.privacy model is available.
jeremi
force-pushed
the
feat/statistics-system
branch
from
7121999 to
f1740c6
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
Test Failures: 2 errors of 20 tests
spp_metrics_core/tests/test_metric_base.py passes label when creating spp.cel.variable records, but that field doesn't exist on the model:
ValueError: Invalid field 'label' in 'spp.cel.variable'
Failing tests:
test_metric_base_default_values(line 78)test_metric_base_inherited_by_statistic(line 107)
Fix: remove "label": ... from the spp.cel.variable create dicts in those tests.
Rest of code review — no other issues
- K-anonymity, ACLs, migration script, abstract model inheritance, views all look correct.
2 participants
Summary
Dependencies
spp_statisticusesspp_cel_domainOrigin
From
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan
Note
Medium Risk
Introduces new Odoo models, ACLs, and UI plus a database table/sequence rename migration, which could impact upgrades and category data if assumptions differ across existing deployments.
Overview
Introduces a new
spp_metrics_coremodule that standardizes metric metadata via an abstractspp.metric.basemodel and a sharedspp.metric.categorytaxonomy (with default categories and access rules), plus a pre-migration that renames the legacyspp_statistic_categorytable/sequence tospp_metric_category.Adds a new
spp_statisticmodule built on that core, providing publishable statistics backed by CEL variables with per-context publication flags, context override records (spp.statistic.context), and k-anonymity-style small-cell suppression utilities (including query helpers likeget_published_for_contextandget_published_by_category), along with default statistic categories and tests.Adds
spp_statistic_studio, an auto-install bridge that exposes Studio menus/actions and views to manage statistics and metric categories, with studio-manager access controls.Written by Cursor Bugbot for commit f1740c6. This will update automatically on new commits. Configure here.