[16.0][ADD] hr_holidays_accrual_by_tag: new module#201
[16.0][ADD] hr_holidays_accrual_by_tag: new module#201
Conversation
|
Please avoid plurals in module names: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#11modules |
ba26957 to
f26133b
Compare
HekkiMelody
left a comment
There was a problem hiding this comment.
A couple of small notes, looks pretty good overall
| @@ -0,0 +1 @@ | |||
| from . import hr_holidays_accrual_by_tag | |||
There was a problem hiding this comment.
chore: If you extend the model hr.employee, the file should be called hr_employee.py
|
|
||
| @api.model | ||
| def create(self, vals): | ||
| employee = super(HrEmployee, self).create(vals) |
There was a problem hiding this comment.
chore(non-blocking):
| employee = super(HrEmployee, self).create(vals) | |
| employee = super().create(vals) |
| return employee | ||
|
|
||
| def write(self, vals): | ||
| res = super(HrEmployee, self).write(vals) |
There was a problem hiding this comment.
chore(non-blocking):
| res = super(HrEmployee, self).write(vals) | |
| res = super().write(vals) |
| class HrEmployee(models.Model): | ||
| _inherit = "hr.employee" | ||
|
|
||
| @api.model |
There was a problem hiding this comment.
chore: This causes the following warning
2025-07-15 14:42:21,727 283 WARNING odoo odoo.api.create: The model odoo.addons.hr_holidays_accrual_by_tag.models.hr_holidays_accrual_by_tag is not overriding the create method in batch
let's use @api.model_create_multi and adapt the method to be compatible with a recordset containing multiple records
f26133b to
34225c1
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
|
@OCA/human-resources-maintainers Hello, can you take a look at this? We have been using it successfully in production for a couple of months now. Thanks |
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause
The test failure occurs because the database connection fails during the Odoo startup process (Connection to the database failed). This is not a code logic error in the module but rather an environment or setup issue unrelated to the actual module logic.
2. Suggested Fix
No code fix is needed for the failure itself. However, to avoid such failures in CI/CD pipelines, ensure that:
- The database service (
postgres.runboat-infra.svc.cluster.local) is reachable. - The database user and credentials are correctly configured.
- The environment has sufficient resources and proper network access.
This is likely a transient infrastructure problem and not related to this PR's code.
3. Additional Code Issues
a. Potential Performance Bottleneck in _assign_accrual_plans_by_tags
In hr_holidays_accrual_by_tag/models/hr_employee.py, lines 35–44:
for allocation in accrual_allocations:
existing_assignment = self.env["hr.leave.allocation"].search(
[
("employee_id", "=", self.id),
("accrual_plan_id", "=", allocation.accrual_plan_id.id),
("date_to", "=", False),
],
limit=1,
)- Issue: For each allocation, a new search is performed. If there are many active accrual plans, this can cause performance degradation.
- Fix: Precompute or batch the
existing_assignmentlookup using a single query with grouping orread_group.
b. Inefficient Use of action_validate()
Lines 50–52:
childs = allocation.with_context(
mail_notify_force_send=False, mail_activity_automation_skip=True
).create(new_allocation)
childs.action_validate()- Issue: Calling
action_validate()on a newly created record inside a loop may not be optimal; it could be replaced by setting thestatedirectly during creation if possible. - Fix: Consider using
state='validate'innew_allocationinstead of callingaction_validate()separately.
c. Missing Context Handling in create() Call
Lines 48–52:
childs = allocation.with_context(
mail_notify_force_send=False, mail_activity_automation_skip=True
).create(new_allocation)- Issue: The context is applied only to the
.create()call, but it might be better to useself.with_context(...)or pass it viacreate()parameters if supported. - Fix: Clarify whether this is necessary and whether the context is correctly scoped.
4. Test Improvements
a. Add Edge Case Tests
- Test with zero tags on employee creation.
- Test with duplicate tag names.
- Test with non-existent categories (should not crash).
- Test with many tags (performance test).
b. Use SavepointCase for Isolation
In test_hr_holidays_accrual_by_tag.py, consider using SavepointCase for better isolation:
from odoo.tests.common import SavepointCaseThis ensures that database changes in one test do not affect others.
c. Test write() Logic More Thoroughly
The current test only covers:
- Adding a tag.
- Removing a tag.
It should also cover:
- Replacing a tag with another.
- Setting
category_idsto an empty list. - Updating multiple fields in
write()at once (e.g.,name+category_ids).
d. Verify State Transition Logic
Ensure tests cover:
- That
date_tois correctly set when a tag is removed. - That
date_tois not set when a tag is added and an existing allocation already exists.
e. Test Concurrent Access
If possible, simulate concurrent access to ensure no race conditions occur during tag assignment.
Summary of Recommendations:
| Area | Recommendation |
|---|---|
| Root Cause | Not a code bug; likely infrastructure/network issue |
| Code Fixes | Optimize search in _assign_accrual_plans_by_tags, improve action_validate() usage |
| Test Coverage | Add edge cases, use SavepointCase, test more write() scenarios |
| Patterns | Follow OCA testing patterns: TransactionCase, SavepointCase, Command usage |
⚠️ PR Aging Alert: CRITICAL
This PR by @quirino95 has been waiting for 243 days — that is over 8 months without being merged or closed.
💤 No activity for 112 days. Has this PR been forgotten?
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
- hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
- stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
- stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
- stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
- purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix
Reviewing each other's work helps the whole community move forward. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
This module automates creation and management of accrual allocations for employees, based on assigned tags (which have to be related to an accrual plan).
When a new employee is created, and some tags are assigned to, related accrual allocations are created automatically when saving.
Also adding a tag to an existing employee will create an accrual allocation.
If a tag is removed from an employee,
date_toon related accrual allocation is written in order to stop accruing hours.If the same tag is added again, a new independent accrual allocation is created.
Of course, is already possible to do all these steps manually, but it' very hard. Using this module makes everything easier.
From a technical point of view, in order to achieve this, I've extended behavior of
createandwritemethods ofhr.employeemodule.