Fix double-counted leave on withdrawal-during-edit, and 500s on a stale leave-type id

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)

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);

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[]
 	 */

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();

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);

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,18 +29,20 @@ 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;
 
 	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);
+	}
 }

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 {