[FIX] hr_holidays_team_manager

[max3903]

No description provided.

[github-actions]

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[alexey-pelykh]

Thanks for looking at this. The domain change to [(1, '=', 1)] effectively removes the department-based filtering, making the rule a pass-all. Is that intentional? If so, might be worth a note in the PR description explaining why the original domain was too restrictive.

Also, tests are currently failing on both OCB and Odoo CI — worth checking before this can move forward.

[marcos-mendez]

Automated Review -- Tests Failed

1. Root Cause

The test failure is likely due to a security rule change in hr_holidays_team_manager/data/hr_holidays_security.xml, where the domain_force was changed from a conditional domain to [(1, '=', 1)], effectively making the rule always true. This could allow unintended access to time-off records, breaking expected permission logic and causing test failures related to access control.

---

2. Suggested Fix

The domain_force should be restored to its original logic, which was:

<field name="domain_force">['|',
    ('employee_id.department_id', 'in', [user.employee_ids.department_id.id]),
    ('employee_ids.department_id', 'in', [user.employee_ids.department_id.id])
]</field>

This ensures that users can only see time-off records for employees in their own department. The change to [(1, '=', 1)] is incorrect and must be reverted.

File: hr_holidays_team_manager/data/hr_holidays_security.xml
Line: 34
Action: Revert the domain_force value to the original domain logic.

---

3. Additional Code Issues

• None – The change is a security rule, not a functional logic issue. It's a breaking change in access control, not a bug in implementation.

---

4. Test Improvements

Add a TransactionCase test to verify that:

• Users in the same department can see time-off records of team members.
• Users in a different department cannot see time-off records.
• The rule is correctly enforced by testing access rights via self.env['hr.leave'].search([]) with different user contexts.

Suggested test class:

from odoo.tests import TransactionCase
from odoo.tests.common import Form

class TestHrHolidaysTeamManagerAccess(TransactionCase):
    def setUp(self):
        super().setUp()
        self.user_a = self.env['res.users'].create({
            'name': 'User A',
            'login': 'user_a',
            'groups_id': [(6, 0, [self.env.ref('hr_holidays.group_hr_holidays_user').id])]
        })
        self.user_b = self.env['res.users'].create({
            'name': 'User B',
            'login': 'user_b',
            'groups_id': [(6, 0, [self.env.ref('hr_holidays.group_hr_holidays_user').id])]
        })
        self.department_a = self.env['hr.department'].create({'name': 'Dept A'})
        self.department_b = self.env['hr.department'].create({'name': 'Dept B'})
        self.employee_a = self.env['hr.employee'].create({
            'name': 'Employee A',
            'user_id': self.user_a.id,
            'department_id': self.department_a.id,
        })
        self.employee_b = self.env['hr.employee'].create({
            'name': 'Employee B',
            'user_id': self.user_b.id,
            'department_id': self.department_b.id,
        })
        self.leave_a = self.env['hr.leave'].create({
            'name': 'Leave A',
            'employee_id': self.employee_a.id,
            'holiday_status_id': self.env.ref('hr_holidays.holiday_status_cl').id,
        })

    def test_user_can_see_same_department_leave(self):
        self.leave_a.with_user(self.user_a).read(['name'])  # Should succeed
        self.leave_a.with_user(self.user_b).read(['name'])  # Should fail

    def test_user_cannot_see_other_department_leave(self):
        self.leave_a.with_user(self.user_b).read(['name'])  # Should raise AccessError

This ensures access control is enforced as expected and prevents regressions in the security rule.

---

⏰ PR Aging Alert

This PR by @max3903 has been open for 151 days (5 months).

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