refactor: rename analytics module ecosystem for clarity#78
refactor: rename analytics module ecosystem for clarity#78
Conversation
The foundation module gets the plain name following Odoo convention. Model names (spp.metric.base, spp.metric.category) are unchanged.
- Rename models: spp.metrics.* -> spp.metric.* (breakdown, fairness, distribution, privacy, dimension.cache) - Remove validate_access_level() and get_k_threshold() from privacy service (Fix 1 part 1: removes upward dependency into aggregation) - Update all internal env[] references and tests
- Rename models: spp.aggregation.* -> spp.analytics.* - Rename spp.aggregation.statistic.registry -> spp.analytics.indicator.registry - Fix 1 part 2: absorb access rule lookups into analytics service (_determine_access_level, _get_k_threshold now use spp.analytics.access.rule directly) - Update all Python import paths (spp_aggregation -> spp_analytics) - Update consumer manifests: spp_simulation, spp_api_v2_gis, spp_api_v2_simulation, spp_mis_demo_v2 - Update cross-module menu XML ID reference - Keep security group XML IDs (group_aggregation_*) to avoid migration
- Rename models: spp.statistic -> spp.indicator, spp.statistic.context -> spp.indicator.context - Add spp_metric_service as hard dependency (Fix 3) - Remove inline suppression fallback, use direct env["spp.metric.privacy"] - Update security CSV model references - Update spp_metric tests to reference spp.indicator - Update spp_mis_demo_v2 demo XML references
- Update manifest depends to spp_indicator - Update cross-module security CSV XML IDs - Update view model references to spp.indicator - Update menu display names to "Indicators"
- Simulation distribution and fairness now go through spp.analytics.service instead of calling metric services directly - This ensures the analytics service is the single entry point while not degrading simulation utility - Update temporal comments to be evergreen
Update leftover spp.statistic, spp_statistic, and spp.aggregation references in test skip messages, docstrings, README files, and HTML descriptions to use the new indicator/analytics names.
- Consolidate 4 repeated get_effective_rule_for_user() calls into single _get_effective_rule() lookup per compute_aggregation() call - Revert simulation to call metric services directly: distribution has no access control, fairness needs target_type-aware base_domain - Fix stale README documenting removed privacy service methods - Rename Python classes Statistic->Indicator, StatisticContext-> IndicatorContext to match model names - Update _description fields to match new naming
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 undertakes a significant refactoring of the analytics module ecosystem to enhance clarity, improve architectural layering, and resolve naming inconsistencies. The changes involve renaming several core modules and their associated models, consolidating access control logic within the analytics service, and updating all dependent modules to align with the new structure. This refactoring aims to create a more intuitive and maintainable codebase for future development. 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 primarily renames core modules and models within the OpenSPP system for consistency. Specifically, spp_aggregation is renamed to spp_analytics, spp_statistic to spp_indicator, spp_metrics_core to spp_metric, and spp_metrics_services to spp_metric_service. This renaming impacts module manifests, model definitions, service calls, cron jobs, security rules, menu items, and documentation across numerous files. Additionally, the spp_analytics service is refactored to optimize access rule lookups by resolving the effective rule once and passing it to subsequent validation methods, and the spp_indicator module's apply_suppression method now directly delegates to the spp.metric.privacy service. Review comments suggest further consistency improvements, such as updating 'source' labels in statistic registries, renaming Python files and classes (e.g., statistic.py to indicator.py, AggregationApiService to AnalyticsApiService), adjusting field labels and view strings from 'Statistic(s)' to 'Indicator(s)', and correcting a README statement about the necessity of code changes during the migration.
Note: Security Review did not run due to the size of the PR.
| # From spp.statistic | ||
| stat_model = self.env.get("spp.statistic") | ||
| # From spp.indicator | ||
| stat_model = self.env.get("spp.indicator") | ||
| if stat_model: | ||
| for stat in stat_model.sudo().search([("active", "=", True)]): # nosemgrep: odoo-sudo-without-context | ||
| available.append({"name": stat.name, "label": stat.label, "source": "statistic"}) |
There was a problem hiding this comment.
The source is hardcoded as 'statistic'. To align with the refactoring of spp.statistic to spp.indicator, this should be updated to 'indicator' for consistency.
| available.append({"name": stat.name, "label": stat.label, "source": "statistic"}) | |
| available.append({"name": stat.name, "label": stat.label, "source": "indicator"}) |
| @@ -145,13 +145,13 @@ def _compute_gini(self, registrant_ids): | |||
|
|
|||
| @api.model | |||
| def _try_statistic_model(self, stat_name, registrant_ids): | |||
There was a problem hiding this comment.
The method name _try_statistic_model is misleading after the refactoring, as it now works with the spp.indicator model. Renaming it to _try_indicator_model would improve clarity. You will also need to update the call to this method inside the compute method.
| def _try_statistic_model(self, stat_name, registrant_ids): | |
| def _try_indicator_model(self, stat_name, registrant_ids): |
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class AggregationApiService: |
There was a problem hiding this comment.
The class name AggregationApiService is inconsistent with the refactoring to spp.analytics.service. Renaming it to AnalyticsApiService would improve consistency.
| class AggregationApiService: | |
| class AnalyticsApiService: |
| @@ -10,22 +10,22 @@ | |||
| _logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| class Statistic(models.Model): | |||
| """A publishable statistic based on a CEL variable. | |||
| class Indicator(models.Model): | |||
There was a problem hiding this comment.
This file defines the Indicator model but is named statistic.py. For consistency with the refactoring, consider renaming the file to indicator.py.
| @@ -4,20 +4,20 @@ | |||
| from odoo import fields, models | |||
|
|
|||
|
|
|||
| class StatisticContext(models.Model): | |||
| """Context-specific presentation configuration for a statistic. | |||
| class IndicatorContext(models.Model): | |||
There was a problem hiding this comment.
This file defines the IndicatorContext model but is named statistic_context.py. For consistency with the refactoring, consider renaming the file to indicator_context.py.
| statistic_id = fields.Many2one( | ||
| comodel_name="spp.statistic", | ||
| comodel_name="spp.indicator", | ||
| string="Statistic", |
There was a problem hiding this comment.
The field label is 'Statistic', but it refers to the spp.indicator model. For consistency, it should be 'Indicator'.
| string="Statistic", | |
| string="Indicator", |
| @@ -8,7 +8,7 @@ | |||
| class TestStatisticCategory(TransactionCase): | |||
There was a problem hiding this comment.
This test file is named test_statistic.py but it tests the spp.indicator model. For consistency, consider renaming it to test_indicator.py.
| <field name="name">spp.indicator.view.list</field> | ||
| <field name="model">spp.indicator</field> | ||
| <field name="arch" type="xml"> | ||
| <list string="Statistics" default_order="category_id, sequence"> |
There was a problem hiding this comment.
The string 'Statistics' should be updated to 'Indicators' for consistency with the refactoring. Other occurrences of 'Statistic(s)' in this file should also be updated.
| <list string="Statistics" default_order="category_id, sequence"> | |
| <list string="Indicators" default_order="category_id, sequence"> |
| Model names remain unchanged for backward compatibility. | ||
| These services were extracted from `spp_analytics` to enable reuse across modules. | ||
|
|
||
| **No code changes required** - Existing code continues to work: |
There was a problem hiding this comment.
The statement "No code changes required" is not entirely accurate, as model names have changed (e.g., from spp.metrics.* to spp.metric.*), which does require code changes in consuming modules, as shown in the example below.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #78 +/- ##
==========================================
+ Coverage 66.31% 67.11% +0.80%
==========================================
Files 369 399 +30
Lines 22311 23874 +1563
==========================================
+ Hits 14795 16023 +1228
- Misses 7516 7851 +335
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Rename files, classes, and view strings to eliminate stale "aggregation" and "statistic" naming from the Python layer. spp_analytics: - aggregation_access.py -> analytics_access.py (AggregationAccessRule -> AnalyticsAccessRule) - aggregation_scope.py -> analytics_scope.py (AggregationScope -> AnalyticsScope) - statistic_registry.py -> indicator_registry.py (StatisticRegistry -> IndicatorRegistry) - AggregationService -> AnalyticsService, AggregationCacheService -> AnalyticsCacheService - AggregationTestCase -> AnalyticsTestCase - View XML files renamed accordingly spp_indicator: - statistic.py -> indicator.py, statistic_context.py -> indicator_context.py - statistic_categories.xml -> indicator_categories.xml - View strings: "Statistic(s)" -> "Indicator(s)" - Field label: string="Statistic" -> string="Indicator" spp_indicator_studio: - statistic_views.xml -> indicator_views.xml - statistic_category_views.xml -> indicator_category_views.xml spp_api_v2_simulation: - aggregation_api_service.py -> analytics_api_service.py - schemas/aggregation.py -> schemas/analytics.py - routers/aggregation.py -> routers/analytics.py - AggregationApiService -> AnalyticsApiService - AggregationScopeRequest -> AnalyticsScopeRequest - AggregationResponse -> AnalyticsResponse spp_api_v2_gis: - StatisticInfo -> IndicatorInfo - StatisticCategoryInfo -> IndicatorCategoryInfo - StatisticsListResponse -> IndicatorsListResponse
1 participant
Summary
Renames 5 analytics modules and fixes 3 architectural issues to resolve naming confusion and improve layering. No database migration needed (modules never installed anywhere).
Module renames:
spp_metrics_core->spp_metric(foundation gets the plain name)spp_metrics_services->spp_metric_service(computation services)spp_aggregation->spp_analytics(query engine, avoids "aggregation" collision)spp_statistic->spp_indicator(standard social protection term)spp_statistic_studio->spp_indicator_studio(follows parent)Model renames:
spp.metrics.*->spp.metric.*(breakdown, fairness, distribution, privacy, dimension.cache)spp.aggregation.*->spp.analytics.*(service, scope, resolver, access.rule, cache, cache.entry)spp.aggregation.statistic.registry->spp.analytics.indicator.registryspp.statistic->spp.indicator,spp.statistic.context->spp.indicator.contextArchitectural fixes:
spp.analytics.servicewith a single consolidated DB lookup per request.spp_indicatorhard-depends onspp_metric_service, removing inline fallback suppression logic.All consuming modules updated:
spp_simulation,spp_api_v2_gis,spp_api_v2_simulation,spp_mis_demo_v2manifests, imports, and env[] references.Test plan
spp_metric: 11 tests passingspp_metric_service: 28 tests passingspp_analytics: 131 tests passingspp_indicator: 9 tests passingspp_indicator_studio: installs cleanly (no tests)spp_simulation: 114 tests passingspp_api_v2_gis: 191 tests passingspp_api_v2_simulation: 111 tests passing