Conversation
This commit introduces a new Odoo module `odf_data_quality_dashboard`. This module provides a non-blocking, post-import data validation system and a user interface for managing data quality issues. The module includes: - A new model `odf.data.quality.issue` to store data quality issues. - A daily scheduled action that scans for partners with invalid VAT numbers and creates issue records. - Kanban, form, and tree views for the `odf.data.quality.issue` model, grouped by status. - A "Data Quality Dashboard" menu item. - Security rules for the new model. This addresses the requirement of decoupling data validation from the import process, as outlined in section 4.2 of the Project Constitution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Odoo module for a Data Quality Dashboard. The implementation is a good starting point, but there are a few areas that need improvement, particularly regarding performance, security, and code cleanliness. I've identified a significant performance issue due to N+1 queries, a security risk with the cron job running as root, overly permissive access rights, and an unused import. Addressing these points will make the module more robust, secure, and maintainable.
| for partner in partners_to_check: | ||
| # This is a placeholder for a real VAT validation. | ||
| # For this example, we consider a VAT invalid if it has less than 3 chars. | ||
| if len(partner.vat) < 3: | ||
| # Avoid creating duplicate issues for the same partner. | ||
| existing_issue = self.search_count([ | ||
| ('related_record', '=', f'res.partner,{partner.id}'), | ||
| ('issue_type', '=', 'Invalid VAT'), | ||
| ('status', '!=', 'resolved'), | ||
| ]) | ||
| if not existing_issue: | ||
| self.create({ | ||
| 'name': f"Invalid VAT number for '{partner.display_name}'", | ||
| 'issue_type': 'Invalid VAT', | ||
| 'related_record': f'res.partner,{partner.id}', | ||
| }) |
There was a problem hiding this comment.
The current implementation iterates over partners and performs a database query (search_count and create) inside the loop. This causes an N+1 query problem and will perform poorly with a large number of records. It's much more efficient to process records in batches: fetch all necessary data with a minimal number of queries, process it in memory, and then write back to the database in a single batch operation.
# This is a placeholder for a real VAT validation.
# For this example, we consider a VAT invalid if it has less than 3 chars.
invalid_partners = partners_to_check.filtered(lambda p: p.vat and len(p.vat) < 3)
if not invalid_partners:
return
# Avoid creating duplicate issues for the same partner.
existing_issues = self.search_read(
[
('related_record', 'in', [f'res.partner,{p.id}' for p in invalid_partners]),
('issue_type', '=', 'Invalid VAT'),
('status', '!=', 'resolved'),
],
['related_record']
)
partners_with_issues = {issue['related_record'] for issue in existing_issues}
issues_to_create = []
for partner in invalid_partners:
partner_ref = f'res.partner,{partner.id}'
if partner_ref not in partners_with_issues:
issues_to_create.append({
'name': f"Invalid VAT number for '{partner.display_name}'",
'issue_type': 'Invalid VAT',
'related_record': partner_ref,
})
if issues_to_create:
self.create(issues_to_create)| <field name="model_id" ref="model_odf_data_quality_issue"/> | ||
| <field name="state">code</field> | ||
| <field name="code">model._run_data_quality_checks()</field> | ||
| <field name="user_id" ref="base.user_root"/> |
There was a problem hiding this comment.
The scheduled action is configured to run as the root user (base.user_root), which poses a security risk by granting it the highest level of permissions. It is a best practice to follow the principle of least privilege and run scheduled actions with a dedicated user that has only the necessary permissions for the task.
| @@ -0,0 +1,66 @@ | |||
| # -*- coding: utf-8 -*- | |||
| from odoo import api, fields, models | |||
| from odoo.addons.base.models.res_partner import Partner | |||
| @@ -0,0 +1,2 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
| access_odf_data_quality_issue_user,odf.data.quality.issue.user,model_odf_data_quality_issue,base.group_user,1,1,1,1 | |||
There was a problem hiding this comment.
The access control list grants full CRUD (Create, Read, Update, Delete) permissions to all internal users (base.group_user). This is overly permissive and could lead to unintended data modification or deletion. Consider defining more granular permissions using dedicated security groups (e.g., a 'Data Quality User' and 'Data Quality Manager' group) to restrict access based on user roles.
There was a problem hiding this comment.
Certainly! To address the overly permissive access control, I propose introducing more granular permissions by defining two new security groups: a 'Data Quality User' with read-only access and a 'Data Quality Manager' with full CRUD permissions.
Here's the suggested change for odf_data_quality_dashboard/security/ir.model.access.csv:
This change assumes that the new groups odf_data_quality_dashboard.group_data_quality_user and odf_data_quality_dashboard.group_data_quality_manager are defined. You would typically define these groups in a new XML file, for example, security/data_quality_groups.xml.
Example security/data_quality_groups.xml:
<?xml version="1.0" encoding="utf-8"?>
<odoo>
<data noupdate="0">
<record id="group_data_quality_user" model="res.groups">
<field name="name">Data Quality / User</field>
<field name="category_id" ref="base.module_category_extra_tools"/>
<field name="implied_ids" eval="[(4, ref('base.group_user'))]"/>
</record>
<record id="group_data_quality_manager" model="res.groups">
<field name="name">Data Quality / Manager</field>
<field name="category_id" ref="base.module_category_extra_tools"/>
<field name="implied_ids" eval="[(4, ref('group_data_quality_user'))]"/>
</record>
</data>
</odoo>Additionally, you would need to update odf_data_quality_dashboard/__manifest__.py to include this new XML file:
'data': [
'security/data_quality_groups.xml',
'security/ir.model.access.csv',
'views/data_quality_issue_views.xml',
'views/menus.xml',
'data/scheduled_actions.xml',
],This approach ensures that access to data quality issues is controlled based on user roles, improving the security and maintainability of the module.
| access_odf_data_quality_issue_user,odf.data.quality.issue.user,model_odf_data_quality_issue,base.group_user,1,1,1,1 | |
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |
| access_odf_data_quality_issue_user_read,odf.data.quality.issue.user.read,model_odf_data_quality_issue,odf_data_quality_dashboard.group_data_quality_user,1,0,0,0 | |
| access_odf_data_quality_issue_manager,odf.data.quality.issue.manager,model_odf_data_quality_issue,odf_data_quality_dashboard.group_data_quality_manager,1,1,1,1 |
Fixes #8