Skip to content

[18.0][ADD] project_task_material_cost#1666

Open
Saran440 wants to merge 1 commit intoOCA:18.0from
ecosoft-odoo:18.0-add-project_task_material_cost
Open

[18.0][ADD] project_task_material_cost#1666
Saran440 wants to merge 1 commit intoOCA:18.0from
ecosoft-odoo:18.0-add-project_task_material_cost

Conversation

@Saran440
Copy link
Member

@Saran440 Saran440 commented Feb 6, 2026

This module add cost in task material
selection_043

Copy link

@luisDIXMIT luisDIXMIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this can see as an improvement of project_task_material module. Anyway I tested it on runboat and LGTM!

Copy link
Contributor

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, clean addition. The stored editable computed pattern for the cost field is spot on. Code review LGTM.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_id is changed (does cost recompute?).
  • A test for a product with standard_price = 0 (edge case).
  • A test verifying the readonly=False behavior (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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants