-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PCHR-4002: Allow User Who is Own Leave Approver Take Admin Actions on Own Requests #2792
PCHR-4002: Allow User Who is Own Leave Approver Take Admin Actions on Own Requests #2792
Conversation
…when he is own leave approver or an Admin. Add tests. Remove un-used test.
…ns set in earlier tests, Fixed by setting permission to empty.
da2c2cd
to
2cf519c
Compare
I think there might be a typo in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 small things
@@ -2962,6 +2962,7 @@ public function testLeaveRequestIsValidShouldReturnAnErrorWhenTheToilToAccrueDoe | |||
} | |||
|
|||
public function testToilCanBeAccruedWhenToilIsInHoursAndToilToAccrueValueIsNotAValidToilAmountOptionValue() { | |||
$this->setPermissions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added to tests in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some previous tests that ran set the permissions to that of an Admin. Also the same contact logged in is the leave contact for the test, so the test was assuming that the contact is an Admin that is trying to take actions on own request and an Admin cannot create a leave request with Awaiting approval status (status 3) based on the Manager status transition matrix. I simply set the permission to null which is what is needed when running the test for a staff for the purpose of this test.
|
||
$this->assertTrue($this->leaveRequestStatusMatrix->canTransitionTo($fromStatus, $toStatus, $adminID)); | ||
foreach($nonPossibleTransitions as $transition) { | ||
$this->assertFalse($this->leaveRequestStatusMatrix->canTransitionTo($transition[0], $transition[1], $adminID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few lines like this are pretty long, do you think you could split the arguments onto separate lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 🐧
Overview
Sequel to #2780, which allows a user to be assigned as own leave approver irrespective of the role of the user, This PR makes some changes in the way a user interacts with own requests especially when such a user is its own leave approver.
These changes allow a user irrespective of the role to assume the role of a leave manager on own request when the user is own leave approver. For example a Staff that is own leave approver can now cancel, approve and perform the same actions a leave manager can on a leave request for own request.
The new changes are summarised as follows
Before
After
An Admin can manage own requests irrespective of whether he is own leave approver or not.
Technical Details
getStatusMatrixForCurrentUser
of the leave request class would always return a Staff status transition matrix for the user even if the user is an Admin, this method is modified such that it returns the Manager status transition matrix for an Admin and for a user that is the leave contact and is own leave approver instead of returning the Staff status transition matrix.