diff --git a/SPECIFICATION.md b/SPECIFICATION.md index 102d167..234ca7b 100644 --- a/SPECIFICATION.md +++ b/SPECIFICATION.md @@ -378,6 +378,14 @@ HR can change it via the HR edit path. - **Approved request — withdraw:** employee requests withdrawal → status `WITHDRAWAL_PENDING`; manager/HR must approve the withdrawal. On approval → `CANCELLED` (balance restored); on rejection → back to `APPROVED`. + **Not while an edit is in flight** (409, same rule as above and for the same + reason): the edit excludes the original from its overlap check as part of the + supersedes chain, so an original sitting in `WITHDRAWAL_PENDING` when the edit is + approved would leave both in force — the same leave counted twice, and a declined + withdrawal would put two overlapping `APPROVED` requests on the same dates. Cancel + the edit first, then withdraw. Correspondingly, retiring a superseded request + treats `WITHDRAWAL_PENDING` as still in force, so rows written before this rule + still retire cleanly. - **Cancellation of any non-terminal request** restores pending/used balance. ### 5.4 Escalation (manager non-response) diff --git a/lib/Service/BalanceService.php b/lib/Service/BalanceService.php index 2502a3a..20c0e5c 100644 --- a/lib/Service/BalanceService.php +++ b/lib/Service/BalanceService.php @@ -14,6 +14,7 @@ use OCA\Absence\Db\LeaveRequestMapper; use OCA\Absence\Db\LeaveType; use OCA\Absence\Db\LeaveTypeMapper; +use OCA\Absence\Exception\ValidationException; use OCP\AppFramework\Db\DoesNotExistException; /** @@ -263,7 +264,15 @@ public function ensureEntitlement(string $employeeUid, int $year, int $typeId): // Mirror buildRow(): only the primary annual type inherits the configured // default; other counting types start at zero, so creating the row never // changes the computed balance (§6.1). - $type = $this->leaveTypeMapper->find($typeId); + // + // Its own try/catch: this runs *inside* the handler above, so a + // DoesNotExistException raised here is not caught by it and would leave + // the API answering a stale type id with a 500 instead of a 422. + try { + $type = $this->leaveTypeMapper->find($typeId); + } catch (DoesNotExistException) { + throw new ValidationException('Unknown leave type.'); + } $now = $this->clock->now(); $ent = new Entitlement(); $ent->setEmployeeUid($employeeUid); diff --git a/lib/Service/EntitlementService.php b/lib/Service/EntitlementService.php index e3f3b93..db9693c 100644 --- a/lib/Service/EntitlementService.php +++ b/lib/Service/EntitlementService.php @@ -96,10 +96,7 @@ public function setForEmployee(string $actorUid, string $employeeUid, int $year, if (!$this->employees->isEmployee($employeeUid)) { throw new ValidationException('Unknown employee.'); } - $type = $this->leaveTypeMapper->find($typeId); - if (!$type->getCountsAgainstBalance()) { - throw new ValidationException('Entitlements only apply to leave types that count against the balance.'); - } + $this->assertCountingType($typeId); $ent = $this->balanceService->ensureEntitlement($employeeUid, $year, $typeId); return $this->update($actorUid, $ent->getId(), $data); } @@ -111,10 +108,7 @@ public function setForEmployee(string $actorUid, string $employeeUid, int $year, * @return int number of employees affected */ public function bulkSet(int $year, int $typeId, float $baseDays, ?string $group): int { - $type = $this->leaveTypeMapper->find($typeId); - if (!$type->getCountsAgainstBalance()) { - throw new ValidationException('Entitlements only apply to leave types that count against the balance.'); - } + $this->assertCountingType($typeId); $count = 0; foreach ($this->targetUids($group) as $uid) { $ent = $this->balanceService->ensureEntitlement($uid, $year, $typeId); @@ -232,6 +226,28 @@ public function expireCarryOver(int $year): int { return $affected; } + /** + * An entitlement is only meaningful for a type that counts against the balance, + * and only for a type that exists. + * + * The mapper throws DoesNotExistException, which is not an AbsenceException, so + * letting it out turns a stale type id — an HR form left open while somebody else + * removed the type — into "An unexpected error occurred" and a 500 in the log, + * rather than a 422 saying what was wrong. + * + * @throws ValidationException + */ + private function assertCountingType(int $typeId): void { + try { + $type = $this->leaveTypeMapper->find($typeId); + } catch (DoesNotExistException) { + throw new ValidationException('Unknown leave type.'); + } + if (!$type->getCountsAgainstBalance()) { + throw new ValidationException('Entitlements only apply to leave types that count against the balance.'); + } + } + /** * @return string[] */ diff --git a/lib/Service/RequestService.php b/lib/Service/RequestService.php index a56a93a..083a578 100644 --- a/lib/Service/RequestService.php +++ b/lib/Service/RequestService.php @@ -515,7 +515,14 @@ private function hrEdit(string $actorUid, LeaveRequest $request, array $data): L $request->setReason($data['reason']); } if (array_key_exists('replacementUid', $data)) { - $type = $this->leaveTypeMapper->find($request->getTypeId()); + // Not resolveType(): that also rejects a disabled type, and HR must stay able + // to correct a historical request whose type has since been retired. Only the + // type's replacement rule is wanted here. + try { + $type = $this->leaveTypeMapper->find($request->getTypeId()); + } catch (DoesNotExistException) { + throw new ValidationException('This request refers to a leave type that no longer exists.'); + } $request->setReplacementUid($this->resolveReplacement($request->getEmployeeUid(), $type, $data['replacementUid'])); } // HR may correct the working-day count (§5.5); otherwise it is kept as entered. @@ -570,6 +577,14 @@ public function cancel(string $actorUid, int $id): LeaveRequest { if ($isHrOverride) { return $this->transitionToCancelled($actorUid, $request); } + // Not while an edit of this leave is awaiting a decision (§5.3). The edit + // excludes the original from its overlap check as part of the supersedes + // chain, and retireSuperseded() only retires an original that is still + // APPROVED — so moving this one to WITHDRAWAL_PENDING first and then + // approving the edit would leave both in force, the same leave counted + // twice, and a declined withdrawal would put two overlapping approved + // requests on the same dates. Cancel the edit first, then withdraw. + $this->assertNoPendingEdit($request); // Employee: approved leave requires a withdrawal approval step. $request = $this->atomic(function () use ($request): LeaveRequest { $request->setStatus(LeaveRequest::STATUS_WITHDRAWAL_PENDING); @@ -652,6 +667,11 @@ public function approve(string $actorUid, int $id, ?string $comment): LeaveReque // Calendar work follows the commit; see the class docblock. if ($retired !== null) { $this->calendar->onRemoved($retired); + // The retired request is closed now, so any decision it was still + // waiting on is moot — a withdrawal request against it above all, + // which would otherwise sit in the manager's notifications offering + // to withdraw leave that no longer exists. + $this->notifications->dismiss($retired); // retireSuperseded() cancels the original with a direct write, so it // never passes through transitionToCancelled() where the replacement // would normally be released. Whoever covered the *old* version and is @@ -856,7 +876,16 @@ private function retireSuperseded(LeaveRequest $request): ?LeaveRequest { } catch (DoesNotExistException) { return null; } - if ($original->getStatus() !== LeaveRequest::STATUS_APPROVED) { + // WITHDRAWAL_PENDING counts as still in force, not just APPROVED: it is approved + // leave awaiting a decision on withdrawing it, so it still occupies the dates and + // still counts against the balance. Leaving it standing here is what let an + // approved edit and its original both be counted. cancel() now refuses to start + // a withdrawal while an edit is pending, so this pair can no longer be created — + // but rows that predate that guard still have to retire cleanly. + if (!in_array($original->getStatus(), [ + LeaveRequest::STATUS_APPROVED, + LeaveRequest::STATUS_WITHDRAWAL_PENDING, + ], true)) { return null; } $now = $this->clock->now(); diff --git a/tests/Unit/Service/BalanceServiceTest.php b/tests/Unit/Service/BalanceServiceTest.php index c14310a..c7ac211 100644 --- a/tests/Unit/Service/BalanceServiceTest.php +++ b/tests/Unit/Service/BalanceServiceTest.php @@ -142,6 +142,19 @@ public function testEnsureEntitlementForAnnualUsesConfiguredDefault(): void { $this->assertSame(25.0, $entitlement->getBaseDays()); } + public function testEnsureEntitlementRejectsAnUnknownLeaveType(): void { + // The type lookup runs inside the handler for the missing-entitlement case, so a + // DoesNotExistException raised there is not caught by it. It used to escape as a + // 500 instead of the 422 a stale type id deserves. + $this->entitlementMapper->method('findFor')->willThrowException(new DoesNotExistException('')); + $this->leaveTypeMapper->method('find')->with(99)->willThrowException(new DoesNotExistException('')); + + $this->entitlementMapper->expects(self::never())->method('insert'); + + $this->expectException(\OCA\Absence\Exception\ValidationException::class); + $this->service->ensureEntitlement('alice', 2027, 99); + } + public function testEnsureEntitlementForOtherTypesStartsAtZero(): void { $type = new LeaveType(); $type->setId(2); diff --git a/tests/Unit/Service/EntitlementServiceTest.php b/tests/Unit/Service/EntitlementServiceTest.php index 22eddf6..255a320 100644 --- a/tests/Unit/Service/EntitlementServiceTest.php +++ b/tests/Unit/Service/EntitlementServiceTest.php @@ -11,6 +11,7 @@ use OCA\Absence\Db\Entitlement; use OCA\Absence\Db\EntitlementMapper; use OCA\Absence\Db\LeaveTypeMapper; +use OCA\Absence\Exception\ValidationException; use OCA\Absence\Service\ActivityPublisher; use OCA\Absence\Service\BalanceService; use OCA\Absence\Service\ConfigService; @@ -28,6 +29,7 @@ class EntitlementServiceTest extends TestCase { use ClockMockTrait; private EntitlementMapper&MockObject $entitlementMapper; + private LeaveTypeMapper&MockObject $leaveTypeMapper; private BalanceService&MockObject $balanceService; private ConfigService&MockObject $config; private EntitlementService $service; @@ -35,11 +37,12 @@ class EntitlementServiceTest extends TestCase { protected function setUp(): void { parent::setUp(); $this->entitlementMapper = $this->createMock(EntitlementMapper::class); + $this->leaveTypeMapper = $this->createMock(LeaveTypeMapper::class); $this->balanceService = $this->createMock(BalanceService::class); $this->config = $this->createMock(ConfigService::class); $this->service = new EntitlementService( $this->entitlementMapper, - $this->createMock(LeaveTypeMapper::class), + $this->leaveTypeMapper, $this->balanceService, $this->config, $this->clockAtRealTime(), @@ -130,4 +133,20 @@ public function testRolloverWithCappedPolicyCapsCarryOver(): void { $this->assertSame(30.0, $updated->getBaseDays()); $this->assertSame(10.0, $updated->getCarryOverDays()); } + + /** + * Covers assertCountingType(), which setForEmployee() shares — an HR form left open + * while somebody else removed the type used to answer with a 500, because + * DoesNotExistException is not an AbsenceException and never reached the handler + * that turns domain errors into a 422. + */ + public function testBulkSetRejectsAnUnknownLeaveType(): void { + $this->leaveTypeMapper->method('find')->with(99) + ->willThrowException(new DoesNotExistException('')); + + $this->entitlementMapper->expects(self::never())->method('update'); + + $this->expectException(ValidationException::class); + $this->service->bulkSet(2026, 99, 28.0, null); + } } diff --git a/tests/Unit/Service/RequestServiceTest.php b/tests/Unit/Service/RequestServiceTest.php index cab7c0c..5cbe9ee 100644 --- a/tests/Unit/Service/RequestServiceTest.php +++ b/tests/Unit/Service/RequestServiceTest.php @@ -246,6 +246,55 @@ public function testSecondSupersedingEditIsRejected(): void { $this->service->update('emp', 5, ['startDate' => '2026-03-02', 'endDate' => '2026-03-04']); } + public function testWithdrawalIsRefusedWhileAnEditIsPending(): void { + $original = $this->pendingOwnRequest(); + $original->setStatus(LeaveRequest::STATUS_APPROVED); + $this->requestMapper->method('find')->with(5)->willReturn($original); + $this->permission->method('canView')->willReturn(true); + $this->permission->method('canModify')->willReturn(true); + $this->permission->method('isHr')->willReturn(false); + + // An edit of this approved leave is already awaiting a decision. + $pendingEdit = new LeaveRequest(); + $pendingEdit->setId(6); + $pendingEdit->setSupersedesId(5); + $pendingEdit->setStatus(LeaveRequest::STATUS_PENDING); + $this->requestMapper->method('findBySupersedesId')->with(5)->willReturn([$pendingEdit]); + + // Moving the original to WITHDRAWAL_PENDING here used to be allowed. Approving + // the edit would then find the original no longer APPROVED, decline to retire + // it, and leave the same leave counted twice — as used by the edit and as + // pending by the original. + $this->requestMapper->expects(self::never())->method('update'); + $this->notifications->expects(self::never())->method('notifyWithdrawal'); + + $this->expectException(\OCA\Absence\Exception\ConflictException::class); + $this->service->cancel('emp', 5); + } + + public function testApprovingAnEditRetiresAnOriginalAwaitingWithdrawal(): void { + // The pair the guard above now prevents can still exist in rows written before + // it, so approving the edit has to close the original rather than walk past it. + $edit = $this->pendingOwnRequest(); + $edit->setId(6); + $edit->setSupersedesId(5); + $edit->setManagerUid('mgr'); + + $original = $this->pendingOwnRequest(); + $original->setStatus(LeaveRequest::STATUS_WITHDRAWAL_PENDING); + + $this->requestMapper->method('find')->willReturnMap([[6, $edit], [5, $original]]); + $this->requestMapper->method('update')->willReturnArgument(0); + $this->permission->method('canView')->willReturn(true); + $this->permission->method('canDecide')->willReturn(true); + + $result = $this->service->approve('mgr', 6, null); + + self::assertSame(LeaveRequest::STATUS_APPROVED, $result->getStatus()); + self::assertSame(LeaveRequest::STATUS_CANCELLED, $original->getStatus(), + 'the superseded original must not stay in force alongside the approved edit'); + } + // ------------------------------------------------ atomicity and locking ---- public function testApprovingAnEditRetiresTheOriginalInOneTransaction(): void {