PCHR-4051: Allow Own Approver And Admin Cancel Own Leave Request Even When Absence Type Does Not Allow It #2805

tunbola · 2018-07-31T15:22:36Z

Overview

There are Absence type settings that determine whether a Leave request for that absence type can be cancelled or not. Currently the Admin and Leave manager can cancel leave requests irrespective of whether the Absence type allows it or not, however they cannot do so for their own requests. Other staff not Admin or Leave manager are bound by the Can staff cancel requests for this leave type after they have been made? setting of the Absence Type.
This PR makes the changes needed to allow Admins and Leave managers who are their own leave approvers cancel own requests irrespective of whether the absence type allows it or not.

Before

Leave Managers who are own leave approvers and Admins cannot cancel their own requests when absence type does not allow it

After

Leave Managers who are own leave approvers and Admins can now cancel their own requests irrespective of whether the absence type allows it or not.

Technical Details

The validateAbsenceTypeAllowRequestCancellationForLeaveRequestCancellation method of the LeaveRequest BAO is what determines who can cancel a leave request based on the Absence type allow_request_cancelation value. Since the checks will now involve roles and relationships, This method was removed and a new method that does similarly, canCancelForAbsenceType was added in the LeaveRequestRights service because this service allows checks for user roles and relationships.

 public function canCancelForAbsenceType($absenceTypeId, $contactId, DateTime $leaveFromDate) {
    if ($this->currentUserIsAdmin() || $this->currentUserIsLeaveManagerOf($contactId)) {
      return TRUE;
    }

    $absenceType = AbsenceType::findById($absenceTypeId);
    if ($absenceType->allow_request_cancelation == AbsenceType::REQUEST_CANCELATION_ALWAYS) {
      return TRUE;
    }

    $today = new DateTime('today');

    if ($absenceType->allow_request_cancelation != AbsenceType::REQUEST_CANCELATION_NO &&
      $leaveFromDate > $today
    ) {
      return TRUE;
    }

    return FALSE;
  }

…ts Service. Add tests.

…vice. Add tests. Remove validation for absence type cancellation in Leave Request BAO.

mickadoo

Mostly formatting stuff

mickadoo · 2018-07-31T15:26:50Z

uk.co.compucorp.civicrm.hrleaveandabsences/CRM/HRLeaveAndAbsences/Service/LeaveRequest.php

    $isCancelledStatus = $this->isCancelledStatus($params);
+
+    if ($isCancelledStatus) {
+      if(!$this->canCancelForAbsenceType($params)) {


Space after if

mickadoo · 2018-07-31T15:28:58Z

...o.compucorp.civicrm.hrleaveandabsences/CRM/HRLeaveAndAbsences/Service/LeaveRequestRights.php

+
+    $today = new DateTime('today');
+
+    if ($absenceType->allow_request_cancelation != AbsenceType::REQUEST_CANCELATION_NO &&


This is a bit long, and also a bit confusing to read, do you think you can make

$absenceType->allow_request_cancelation != AbsenceType::REQUEST_CANCELATION_NO into a descriptive variable?

mickadoo · 2018-07-31T15:30:12Z

...m.hrleaveandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestRightsTest.php

+  }
+
+  public function testCanCancelForAbsenceTypeReturnsTrueWhenAbsenceTypeAllowsCancellationForStaff() {
+    $absenceType = AbsenceTypeFabricator::fabricate(['allow_request_cancelation' => AbsenceType::REQUEST_CANCELATION_ALWAYS]);


Indentation is a bit strange, maybe you could put the array on the next line?

mickadoo · 2018-07-31T15:30:48Z

...civicrm.hrleaveandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestTest.php

@@ -499,6 +500,18 @@ private function getLeaveRequestService($isAdmin = false, $isManager = false, $a
    );
  }

+  private function getLeaveRequestServiceForWhenAbsenceTypeCannotBeCancelled($typeId, $contactID, $leaveFromDate) {


Maybe put the arguments on separate lines since this is very long?

mickadoo · 2018-07-31T15:31:01Z

...civicrm.hrleaveandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestTest.php

+  private function getLeaveRequestServiceForWhenAbsenceTypeCannotBeCancelled($typeId, $contactID, $leaveFromDate) {
+    $leaveRightsService = $this->prophesize(LeaveRequestRightsService::class);
+    $leaveRightsService->canCreateAndUpdateFor($contactID)->willReturn(TRUE);
+    $leaveRightsService->canCancelForAbsenceType($typeId, $contactID, new DateTime($leaveFromDate))->willReturn(FALSE);


Long line, maybe split?

mickadoo · 2018-07-31T15:31:59Z

...civicrm.hrleaveandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestTest.php

@@ -1116,7 +1129,8 @@ public function testGetBreakdownIncludeOnlyTheLeaveBalanceChangesOfTheLeaveReque
  public function testToilRequestWithPastDatesCanNotBeCancelledWhenUserIsLeaveContactAndAbsenceTypeDoesNotAllowPastAccrual() {
    $absenceType = AbsenceTypeFabricator::fabricate([
      'allow_accruals_request' => true,
-      'allow_accrue_in_the_past' => false
+      'allow_accrue_in_the_past' => false,


boolean case (and more in this file) - I think maybe we skipped this before but it should be fixed for all bools in this file at some point

mickadoo · 2018-07-31T15:32:20Z

...civicrm.hrleaveandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestTest.php

+    $leaveRequestService = $this->getLeaveRequestServiceForWhenAbsenceTypeCannotBeCancelled(
+      $params['type_id'], $params['contact_id'], $params['from_date']);
+
+    $this->setExpectedException(RuntimeException::class, 'You cannot cancel leave requests for this Absence type');


Long line, and above on 1264

mickadoo · 2018-07-31T15:57:21Z

...o.compucorp.civicrm.hrleaveandabsences/CRM/HRLeaveAndAbsences/Service/LeaveRequestRights.php

-    if ($absenceType->allow_request_cancelation != AbsenceType::REQUEST_CANCELATION_NO &&
-      $leaveFromDate > $today
-    ) {
+    $absenceTypeRequestCancellationNotNo =


Sorry to be picky about this but NotNo seems like it's possible to be improved, would it be possible to name this something like absenceTypeAllowsCancellation?

mickadoo

Looks good to me 👍

tunbola added 3 commits July 31, 2018 15:50

PCHR-4051: Add the canCancelForAbsenceType method in LeaveRequestRigh…

d56dfb0

…ts Service. Add tests.

PCHR-4051: Use the canCancelForAbsenceType method in LeaveRequest Ser…

3cf2fc4

…vice. Add tests. Remove validation for absence type cancellation in Leave Request BAO.

PCHR-4051: Remove obsolete tests. Fix broken tests.

c22b77e

mickadoo suggested changes Jul 31, 2018

View reviewed changes

tunbola added 2 commits July 31, 2018 16:43

PCHR-4051: Code formatting fixes

618dec1

PCHR-4051: Fix boolean case in tests.

bbd8856

mickadoo reviewed Jul 31, 2018

View reviewed changes

PCHR-4051: Refactor method for clarity.

38f1808

tunbola force-pushed the PCHR-4051-allow-own-approver-cancel branch from 85d88bf to 38f1808 Compare July 31, 2018 16:03

mickadoo approved these changes Jul 31, 2018

View reviewed changes

tunbola added the ready to test label Jul 31, 2018

tunbola merged commit da99782 into PCHR-3997-allow-own-leave-approver Jul 31, 2018

tunbola deleted the PCHR-4051-allow-own-approver-cancel branch July 31, 2018 16:34

igorpavlov-zz mentioned this pull request Aug 1, 2018

PCHR-3997: Allow own leave approver #2785

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PCHR-4051: Allow Own Approver And Admin Cancel Own Leave Request Even When Absence Type Does Not Allow It #2805

PCHR-4051: Allow Own Approver And Admin Cancel Own Leave Request Even When Absence Type Does Not Allow It #2805

tunbola commented Jul 31, 2018

mickadoo left a comment

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo Jul 31, 2018

mickadoo left a comment


		$today = new DateTime('today');

		if ($absenceType->allow_request_cancelation != AbsenceType::REQUEST_CANCELATION_NO &&

PCHR-4051: Allow Own Approver And Admin Cancel Own Leave Request Even When Absence Type Does Not Allow It #2805

PCHR-4051: Allow Own Approver And Admin Cancel Own Leave Request Even When Absence Type Does Not Allow It #2805

Conversation

tunbola commented Jul 31, 2018

Overview

Before

After

Technical Details

mickadoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickadoo left a comment

Choose a reason for hiding this comment