Conversation
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)
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.
| - **Computation**: metric_type, cel_expression, aggregation | ||
| - **Presentation**: format, unit, decimal_places |
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.
| Models | ||
| ------ | ||
| - ``spp.statistic``: Publishable statistic referencing a CEL variable | ||
| - ``spp.statistic.category``: Organization categories for statistics |
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 |
| string="CEL Variable", | ||
| required=True, | ||
| ondelete="restrict", | ||
| domain="[('source_type', 'in', ['aggregate', 'computed', 'field']), " "('state', '=', 'active')]", |
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')]"| ], | ||
| "installable": True, | ||
| # Bridge module: auto-install when both spp_statistic and spp_studio are present | ||
| "auto_install": ["spp_statistic", "spp_studio"], |
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, |
1 participant
Summary
Dependencies
spp_statisticusesspp_cel_domainOrigin
From
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan