[ADD] hr_timesheet_manager_approver: Allow approvers to manage subordinate timesheets#930
[ADD] hr_timesheet_manager_approver: Allow approvers to manage subordinate timesheets#930EmilioPascual wants to merge 1 commit into
Conversation
OCA-git-bot
added
series:18.0
mod:hr_timesheet_manager_approver
fcvalgar
left a comment
fcvalgar
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I have reviewed and tested the module, focusing on its intended scope: allowing timesheet approvers to manage timesheet lines of subordinate employees.
The following tests were performed:
Test 1: OK - A timesheet approver with Timesheets / User: all timesheets can see timesheet lines of a subordinate employee.
Test 2: OK - The approver can create timesheet lines for a subordinate employee.
Test 3: OK - The approver can edit timesheet lines of a subordinate employee.
Test 4: OK - The approver can delete timesheet lines of a subordinate employee.
Test 5: Not OK - The approver can delete timesheet lines of employees who are not subordinates. This seems to happen because the rule includes employee_id.parent_id.user_id in (False, user.id), so employees without a manager may also be accessible.
Test 6: OK - The access works as expected on projects with restricted visibility for subordinate employees, without requiring the approver to be a follower of the project.
Could you please review the non-subordinate access case? In my opinion, the rule should only grant the additional access when the employee’s manager is the current user, without extending it to employees without a manager.
Is that intentional @EmilioPascual ? |
No, it's a bug, I'm going to remove |
EmilioPascual
force-pushed
the
18-hr_timesheet_manager_approver
branch
from
9fa1764 to
76b84ab
Compare
|
Fixed. @fcvalgar, @chienandalu can you review again please? Thank you. |
fcvalgar
left a comment
fcvalgar
left a comment
There was a problem hiding this comment.
Thanks for the contribution @EmilioPascual and for the quick fix.
I have reviewed and tested the module again after the latest change to restrict the manager condition.
The behavior now works as expected: timesheet reviewers can manage the timesheet lines of their subordinate employees.
Functional review passed.
|
This PR has the |
There was a problem hiding this comment.
Nice job @EmilioPascual but requesting review before approval. Most tests passed successfully, but there is a discrepancy between the functionality described in the README and the actual behavior observed in the user interface.
The following tests were performed:
- Test 1: Access to subordinate timesheets in a restricted project: OK
- Test 2: Edit subordinate timesheets: OK
- Test 3: Create a new timesheet for a subordinate in a restricted project: NOK. According to the README, managers should be able to "see, edit, create and delete timesheets" for their subordinate employees, even on projects with "Invited employees" privacy. However, when accessing the Timesheets menu and attempting to create a new timesheet, the restricted project does not appear in the project selector because the manager is not a follower of the project. Additionally, filtering timesheets by that project returns no results, even though existing subordinate timesheets can be accessed from the generic timesheet list. As a result, the manager cannot create a new timesheet through the standard user interface workflow, which does not match the behavior described in the README.
- Test 4: Delete subordinate timesheets: OK
- Test 5: Restrict access to non-subordinate employees’ timesheets: OK
- Test 6: Access timesheets from multiple subordinate employees: OK
- Test 7: Access subordinate timesheets when the manager is not a project follower: OK
Please review Test 3. The current implementation allows managers to access and manage existing subordinate timesheets, but the standard UI workflow does not allow creating new timesheets on restricted projects because those projects are not available in the project selector. This behavior appears inconsistent with the functionality described in the README.
You're right, the readme is a bit confusing. I'll update it to make it clear that timesheets can only be created for projects to which the user has access. Thank you. |
…inate timesheets Extend the `timesheet_line_rule_approver` record rule so that users with group_hr_timesheet_approver can read, write, create and delete timesheets of employees whose parent_id.user_id matches them. Add unit tests to verify CRUD access on subordinate timesheets while restricting access to non-subordinates. @moduon MT-14395
EmilioPascual
force-pushed
the
18-hr_timesheet_manager_approver
branch
from
76b84ab to
a9f6ef6
Compare
|
I've update it @Gelojr. Could you review again, please? Thanks. |
Gelojr
left a comment
Gelojr
left a comment
There was a problem hiding this comment.
Congratulations on the contribution and thank you for addressing the issue identified during testing.
The following test was revalidated:
Test 3: Create a new timesheet for a subordinate in a restricted project: OK. The README has been updated to clarify that managers can create timesheets only in projects they have access to. This matches the behavior observed during testing, where restricted projects that the manager cannot access are not available in the project selector when creating a new timesheet. The documentation is now consistent with the actual functionality.
|
This PR has the |
7 participants
Extend the
timesheet_line_rule_approverrecord rule so that users with group_hr_timesheet_approver can read, write, create and delete timesheets of employees whose parent_id.user_id matches them. Add unit tests to verify CRUD access on subordinate timesheets while restricting access to non-subordinates.@rafaelbn @fcvalgar @chienandalu please review.
@moduon MT-14395