From ab21c2416087d3768fa906eebc1eb1774f2a4c87 Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Wed, 12 Aug 2026 15:06:29 +0200 Subject: [PATCH 1/2] fix(requests): stop a withdrawal during a pending edit counting leave twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only one edit may be in flight per approved request, and createSuperseding() enforces that. Nothing enforced the other half: an employee could edit approved leave and then withdraw the original, and the two rules did not compose. The edit excludes the original from its overlap check as part of the supersedes chain, and retireSuperseded() only retired an original that was still APPROVED. So with the original moved to WITHDRAWAL_PENDING, approving the edit walked past it: the edit became APPROVED while the original stayed in force. The same leave was then counted twice — as used by the edit and as pending by the original — and BalanceService's netting could not help, because it only nets while the superseding request is itself pending. Declining the withdrawal afterwards left two overlapping APPROVED requests on the same dates, which is exactly the invariant this class documents itself as protecting. Fixed at both ends. cancel() now refuses to start a withdrawal while an edit is pending, with the message assertNoPendingEdit() already gives: cancel the edit first, then withdraw. And retireSuperseded() treats WITHDRAWAL_PENDING as still in force alongside APPROVED, so a pair written before this guard still retires cleanly rather than silently double-counting forever. Retiring an original also dismisses its notifications now: it is closed, so a withdrawal request against it would otherwise sit in the manager's list offering to withdraw leave that no longer exists. Both regression tests fail without the change and pass with it. Co-Authored-By: Claude Opus 5 (1M context) --- SPECIFICATION.md | 8 ++++ lib/Service/RequestService.php | 33 ++++++++++++++- tests/Unit/Service/RequestServiceTest.php | 49 +++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) 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/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/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 { From 453255329d569c4ae422442398aa7aa6c027e1fb Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Wed, 12 Aug 2026 15:06:29 +0200 Subject: [PATCH 2/2] fix(api): answer a stale leave-type id with 422 instead of 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LeaveTypeMapper::find() throws DoesNotExistException, which is not an AbsenceException, so ApiControllerTrait never recognised it: an HR form left open while somebody else removed the type answered "An unexpected error occurred" with a 500 and an error-level log line, instead of saying what was wrong. Three call sites were unguarded. The two in EntitlementService — reachable from the HR UI with a stale type id, via both the single and bulk endpoints — did the same find-then-check, so they are now one assertCountingType() helper. The third, in BalanceService::ensureEntitlement(), is the subtle one: it runs *inside* the handler for the missing-entitlement case, so the exception it raises was never caught by the try it sits in. resolveType() already did this correctly; these now match it. Both regression tests fail without the change and pass with it. Co-Authored-By: Claude Opus 5 (1M context) --- lib/Service/BalanceService.php | 11 ++++++- lib/Service/EntitlementService.php | 32 ++++++++++++++----- tests/Unit/Service/BalanceServiceTest.php | 13 ++++++++ tests/Unit/Service/EntitlementServiceTest.php | 21 +++++++++++- 4 files changed, 67 insertions(+), 10 deletions(-) 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/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); + } }