PCHR-3820: Add Leave Request ACL Rules to LeaveRequest.getAttachments API #2682
Conversation
tunbola
changed the title
tunbola
changed the title
| @@ -4370,24 +4370,40 @@ public function testGetAttachmentsShouldThrowAnExceptionIfLeaveRequestIDIsMissin | |||
| } | |||
|
|
|||
| public function testGetAttachments() { | |||
There was a problem hiding this comment.
I wonder if we still need this test if with all new ones you've added. What does it have that the others don't?
There was a problem hiding this comment.
tbh, I wanted to remove it but the reason I left i was because the test checks for all the expected fields that the API is expected to return which the others don't. I could remove it though. What do you think?
There was a problem hiding this comment.
Maybe then it would make more sense to rename this method to better clarify that it is more about the fields? Or create some custom assert method that checks all the fields, reuse it in the other tests and delete this one.
| } | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
Please also write tests for this method
There was a problem hiding this comment.
I was thinking it will be a duplication of the tests for the LeaveRequest.getAttachments API, since this API uses this method directly.
I could modify this method to accept params and also the LeaveRequestRights service so that I can mock that and pass it in the function but I would still have to create leave requests and all. What do you suggest?
There was a problem hiding this comment.
Unfortunately, because of how the API works, there's no way to avoid this duplication. The tests wouldn't be completely duplicated though, since, as you said, you can mock a few things. In this case it's better to have some duplication than not having tests for this method
| $this->assertEquals($staff1Attachment['values'][0]['id'], $attachment1['id']); | ||
| $this->assertEquals($staff1Attachment['values'][0]['name'], $attachment1['name']); | ||
|
|
||
| $this->assertCount(1, $staff1Attachment['values']); |
There was a problem hiding this comment.
I believe this should actually be $staff2Attachment, right?
| CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer leave and absences']; | ||
| $leaveManagerService = $this->getLeaveManagerServiceWhenUserIsAdmin(); | ||
| $leaveRequestRights = new LeaveRightsService($leaveManagerService); | ||
| $leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRequestRights, $leaveManagerService); |
There was a problem hiding this comment.
This instantiation code is duplicated in a few places. What do you think of moving it to a createLeaveRequestAttachmentsServiceWhenUserIsAdmin() (I know, a bit too long, but quite descriptive and still shorter than 3 lines of code)
Overview
Currently the LeaveRequest.getAttachments API allows a staff to view leave attachments for another staff. This PR incorporates the Leave ACL rules into this API, basically, A staff should only be able to view own attachments, A manager should be able to view attachments for managees only while Admin can view leave attachments for all contacts leave requests.
Before
After