Merge pull request #48 from nextcloud/fix/noid/replacement-optional-for-hr-records

Make the replacement field optional and neutrally worded on HR-recorded absences

Author: karlitschek

SPECIFICATION.md

@@ -368,6 +368,18 @@ HR** on an employee's behalf (§5.6) — employees don't self-record it.
    covers for them. The UI is a **user picker over the whole organisation**
    (`NcSelect` + core autocomplete), excluding the employee themselves. Submit is blocked
    until one is chosen. Sick leave (HR-recorded) needs none.
+
+   **The requirement is on the employee, not on the record.** It exists because somebody
+   arranging their own leave knows who can cover and is asked to sort it out before
+   going. When HR records or corrects an absence *for somebody else* (§5.5, §5.6) they
+   are stating a fact, often after the event, and cannot nominate cover on that person's
+   behalf — so there the field is offered but never demanded, and reads "Who is the
+   replacement?" rather than "Who covers for you?". HR recording their *own* absence
+   gets the self-service wording and requirement, since it is their leave.
+
+   Who may be named is *not* conditional: a replacement must be an employee (not a
+   guest, §2.2) and not the person being covered for, whether or not the type demands
+   one.
 4. On submit, backend:
    - Validates dates (`start ≤ end`, not entirely in the past — in the *employee's*
      timezone, §3.9 — unless HR, not

js/absence-main.mjs

@@ -112,19 +112,33 @@ private function withEmployeeLock(string $employeeUid, callable $fn): mixed {
 	}
 
 	/**
-	 * Validate the nominated replacement for a leave type that requires one (§5.1).
-	 * Returns the (trimmed) replacement uid, or null when not required.
+	 * Validate the nominated replacement (§5.1). Returns the trimmed uid, or null
+	 * when none was given and none is demanded.
 	 *
+	 * @param bool $required whether an absent replacement is an error for a type that
+	 *                       demands one. True on the self-service paths; false when HR
+	 *                       records or corrects somebody else's absence.
 	 * @throws ValidationException
 	 */
-	private function resolveReplacement(string $employeeUid, LeaveType $type, ?string $replacementUid): ?string {
-		if (!$type->getRequiresReplacement()) {
-			return $replacementUid !== null && trim($replacementUid) !== '' ? trim($replacementUid) : null;
-		}
-		$replacementUid = trim((string)$replacementUid);
+	private function resolveReplacement(string $employeeUid, LeaveType $type, ?string $replacementUid, bool $required = true): ?string {
+		$replacementUid = $replacementUid !== null ? trim($replacementUid) : '';
+
 		if ($replacementUid === '') {
-			throw new ValidationException('Please choose a replacement for this leave.');
+			// The mandatory-replacement rule is about somebody applying for their own
+			// leave: they know who can cover and are asked to arrange it before going.
+			// HR recording or correcting an absence for somebody else is stating a fact,
+			// often after the event, and cannot be expected to nominate cover on their
+			// behalf — so there the field is offered but never demanded.
+			if ($required && $type->getRequiresReplacement()) {
+				throw new ValidationException('Please choose a replacement for this leave.');
+			}
+			return null;
 		}
+
+		// Validated whether or not the type demands a replacement. Only the *demand*
+		// is conditional; who may be named is not, and skipping these when the type
+		// happened not to require one let a guest — or the employee themselves — be
+		// recorded as covering.
 		if ($replacementUid === $employeeUid) {
 			throw new ValidationException('You cannot be your own replacement.');
 		}
@@ -289,7 +303,8 @@ public function create(string $actorUid, array $data): LeaveRequest {
 		$start = $this->normaliseDate((string)($data['startDate'] ?? ''));
 		$end = $this->normaliseDate((string)($data['endDate'] ?? ''));
 		$this->validateRange($actorUid, $start, $end, $type, (string)($data['reason'] ?? ''), (string)($data['attachmentNote'] ?? ''));
-		$replacementUid = $this->resolveReplacement($employeeUid, $type, $data['replacementUid'] ?? null);
+		// Not demanded when HR is recording for somebody else — see resolveReplacement().
+		$replacementUid = $this->resolveReplacement($employeeUid, $type, $data['replacementUid'] ?? null, required: !$onBehalf);
 
 		// The employee enters the number of working days; the manager verifies it (§7).
 		$workingDays = $this->normaliseWorkingDays($data['workingDays'] ?? null);
@@ -626,7 +641,7 @@ private function hrEdit(string $actorUid, LeaveRequest $request, array $data): L
 			} 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']));
+			$request->setReplacementUid($this->resolveReplacement($request->getEmployeeUid(), $type, $data['replacementUid'], required: false));
 		}
 		// HR may correct the working-day count (§5.5); otherwise it is kept as entered.
 		if (array_key_exists('workingDays', $data) && $data['workingDays'] !== null) {

js/absence-main.mjs.map

@@ -53,7 +53,8 @@
 
 			<div v-if="needsReplacement" class="dialog__field">
 				<label class="dialog__label">
-					{{ t('absence', 'Replacement') }}<span class="dialog__req">*</span>
+					{{ t('absence', 'Replacement') }}<span v-if="replacementRequired" class="dialog__req">*</span>
+					<span v-else class="dialog__optional">{{ t('absence', '(optional)') }}</span>
 				</label>
 				<!-- eslint-disable @nextcloud/no-deprecated-library-props -- NcSelectUsers migration deferred: needs live-instance testing -->
 				<NcSelect
@@ -63,12 +64,12 @@
 					:userSelect="true"
 					label="displayName"
 					:filterable="false"
-					:placeholder="t('absence', 'Who covers for you?')"
+					:placeholder="replacementPlaceholder"
 					:aria-label-combobox="t('absence', 'Replacement')"
 					@search="onReplacementSearch" />
 				<!-- eslint-enable @nextcloud/no-deprecated-library-props -->
 				<p class="dialog__hint">
-					{{ t('absence', 'A colleague who covers your duties while you are away. They are notified once your leave is approved.') }}
+					{{ replacementHint }}
 				</p>
 			</div>
 
@@ -270,6 +271,48 @@ export default {
 			return this.selectedType ? this.selectedType.requiresReplacement : false
 		},
 
+		/** Whether this dialog is about the signed-in user's own leave. */
+		isOwnLeave() {
+			return this.subjectUid === store.session.uid
+		},
+
+		/**
+		 * Whose absence this is, for wording aimed at somebody else's record.
+		 * Empty until HR has picked an employee, which the strings below allow for.
+		 */
+		subjectName() {
+			if (this.hrMode) {
+				return this.selectedEmployee ? this.selectedEmployee.displayName : ''
+			}
+			return this.request ? (this.request.employeeName || this.request.employeeUid) : ''
+		},
+
+		/**
+		 * Demanded only of somebody arranging their own leave: they know who can cover
+		 * and are asked to sort it out before going (§5.1). HR recording an absence for
+		 * somebody else is stating a fact, often after it happened, and cannot nominate
+		 * cover on their behalf — so the field is offered there but never required.
+		 * Mirrors the same rule in RequestService::resolveReplacement().
+		 */
+		replacementRequired() {
+			return this.needsReplacement && this.isOwnLeave
+		},
+
+		replacementPlaceholder() {
+			return this.isOwnLeave
+				? t('absence', 'Who covers for you?')
+				: t('absence', 'Who is the replacement?')
+		},
+
+		replacementHint() {
+			if (this.isOwnLeave) {
+				return t('absence', 'A colleague who covers your duties while you are away. They are notified once your leave is approved.')
+			}
+			return this.subjectName
+				? t('absence', 'Optional. A colleague who covers for {name} while they are away, and is notified once this is recorded.', { name: this.subjectName })
+				: t('absence', 'Optional. A colleague who covers these duties while this employee is away, and is notified once this is recorded.')
+		},
+
 		// Bridge the native date pickers (Date objects) to our ISO string state.
 		startDate: {
 			get() {
@@ -405,7 +448,7 @@ export default {
 			if (this.hrMode && !this.selectedEmployee) {
 				return false
 			}
-			if (this.needsReplacement && !this.selectedReplacement) {
+			if (this.replacementRequired && !this.selectedReplacement) {
 				return false
 			}
 			if (this.requiresNote && this.reason.trim() === '') {

lib/Service/RequestService.php

@@ -172,6 +172,73 @@ public function testAGuestCannotBeNominatedAsReplacement(): void {
 		]);
 	}
 
+	public function testApplyingForOwnLeaveStillDemandsAReplacement(): void {
+		// §5.1 unchanged for self-service: the employee knows who can cover and is
+		// asked to arrange it before going.
+		$type = $this->type(1, true);
+		$type->setRequiresReplacement(true);
+		$this->leaveTypeMapper->method('find')->with(1)->willReturn($type);
+		$this->permission->method('isHr')->with('emp')->willReturn(false);
+
+		$this->requestMapper->expects(self::never())->method('insert');
+
+		$start = date('Y-m-d', strtotime('+30 days'));
+		$this->expectException(ValidationException::class);
+		$this->service->create('emp', [
+			'typeId' => 1,
+			'startDate' => $start,
+			'endDate' => $start,
+			'workingDays' => 1.0,
+		]);
+	}
+
+	public function testHrRecordingForSomeoneElseNeedsNoReplacement(): void {
+		// HR is stating a fact about somebody else's absence, often after it happened,
+		// and cannot nominate cover on their behalf — so the type's requirement does
+		// not apply to them.
+		$type = $this->type(1, true);
+		$type->setRequiresReplacement(true);
+		$this->leaveTypeMapper->method('find')->with(1)->willReturn($type);
+		$this->permission->method('isHr')->with('hr')->willReturn(true);
+		$this->requestMapper->method('findOverlapping')->willReturn([]);
+		$this->requestMapper->method('insert')->willReturnArgument(0);
+
+		$start = date('Y-m-d', strtotime('+30 days'));
+		$created = $this->service->create('hr', [
+			'typeId' => 1,
+			'employeeUid' => 'emp',
+			'startDate' => $start,
+			'endDate' => $start,
+			'workingDays' => 1.0,
+		]);
+
+		self::assertNull($created->getReplacementUid());
+		self::assertSame('emp', $created->getEmployeeUid());
+	}
+
+	public function testAReplacementIsValidatedEvenWhenTheTypeDoesNotDemandOne(): void {
+		// Only the *demand* is conditional. Skipping the checks when the type happened
+		// not to require a replacement let a guest be recorded as covering.
+		$this->guestUids = ['ext'];
+		$this->userManager->method('userExists')->willReturn(true);
+		$type = $this->type(1, true);
+		$type->setRequiresReplacement(false);
+		$this->leaveTypeMapper->method('find')->with(1)->willReturn($type);
+		$this->permission->method('isHr')->with('emp')->willReturn(false);
+
+		$this->requestMapper->expects(self::never())->method('insert');
+
+		$start = date('Y-m-d', strtotime('+30 days'));
+		$this->expectException(ValidationException::class);
+		$this->service->create('emp', [
+			'typeId' => 1,
+			'startDate' => $start,
+			'endDate' => $start,
+			'workingDays' => 1.0,
+			'replacementUid' => 'ext',
+		]);
+	}
+
 	public function testEmployeeCannotReclassifyIntoHrOnlyType(): void {
 		$request = $this->pendingOwnRequest();
 		$this->requestMapper->method('find')->with(5)->willReturn($request);