Conversation
luisDIXMIT
left a comment
There was a problem hiding this comment.
Maybe this can see as an improvement of project_task_material module. Anyway I tested it on runboat and LGTM!
alexey-pelykh
left a comment
There was a problem hiding this comment.
Nice, clean addition. The stored editable computed pattern for the cost field is spot on. Code review LGTM.
|
This PR has the |
1 similar comment
|
This PR has the |
alexey-pelykh
left a comment
There was a problem hiding this comment.
Review: project_task_material_cost
Overall this is a clean, focused module that adds cost tracking to task materials. The structure follows OCA conventions well. A few observations below.
Correctness
1. Field naming: cost vs purchase_price -- The field is named cost but the compute method is _compute_purchase_price. This inconsistency is minor but worth noting. Consider aligning them (e.g., field name purchase_price to match the method, or rename the method to _compute_cost).
2. copy=False on cost -- Since cost is a computed stored field that depends on product_id, when a material line is duplicated the product_id is copied and the compute will fire, so copy=False is technically fine but also unnecessary since the compute will recompute it. Not blocking, just a note.
3. min_display_digits="Product Price" -- This references the Product Price decimal precision. Ensure this precision record exists in the base Odoo product module (it does in standard Odoo, so this should be fine).
Odoo Best Practices
4. Model file naming -- The model file is models/project.py which inherits project.task.material. OCA convention typically names model files after the model they define/extend. Since this extends project.task.material, a name like project_task_material.py would be more discoverable. The parent module uses project.py for both project.task and project.task.material together, so following the same convention is acceptable here, though it is worth considering for clarity.
Security
5. groups="base.group_user" on cost field -- This restricts the cost field to internal users. This is reasonable since cost data is sensitive. However, note that the base project.task.material model grants read access to base.group_user (employees) but full CRUD to project.group_project_user. If a project user can see the material line but not the cost field, this could cause UI confusion. Verify the UX is acceptable.
Test Coverage
6. Test coverage is minimal but present. The single test test_01_add_task_material_cost verifies the basic happy path (add material, check cost equals standard_price). Consider adding:
- A test for when
product_idis changed (does cost recompute?). - A test for a product with
standard_price = 0(edge case). - A test verifying the
readonly=Falsebehavior (can user override the computed cost?).
These are suggestions for robustness, not blockers.
Manifest
7. Manifest looks correct. Version 18.0.1.0.0, proper dependency on project_task_material, AGPL-3 license, data files listed. No issues found.
View
8. View inheritance is clean. The xpath targets //field[@name='material_ids']/list with position="inside" to append the cost column with a sum aggregate. This is correct and follows the parent view structure.
Documentation
9. The description is very brief -- "This module add cost in material." (also minor grammar: "add" should be "adds", and "in material" could be more descriptive, e.g., "Adds cost tracking to task material lines based on the product's standard price").
i18n
10. No i18n directory -- This is fine for an initial PR. OCA bot will generate .pot files after merge.
Summary
This is a well-structured, focused module. The issues noted above are minor observations and suggestions rather than blockers. The PR has already been approved and labeled "ready to merge", so these comments are offered as constructive feedback.
Review posted via CorporateHub OCA review campaign
| ) | ||
|
|
||
| @api.depends("product_id") | ||
| def _compute_purchase_price(self): |
There was a problem hiding this comment.
Suggestion: The field is named cost but the compute method is _compute_purchase_price. Consider aligning naming -- either rename the field to purchase_price or rename the method to _compute_cost for consistency.
Review posted via CorporateHub OCA review campaign
| @@ -0,0 +1 @@ | |||
| This module add cost in material. | |||
There was a problem hiding this comment.
Minor: Grammar suggestion -- "This module add cost in material." could be improved to something like "This module adds cost tracking to task material lines based on the product's standard price."
Review posted via CorporateHub OCA review campaign
This module add cost in task material
