Fix/noid/statistics range and clock

lib/Controller/ExportController.php

@@ -14,8 +14,8 @@
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
-use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
+use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\DataDownloadResponse;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\IRequest;

lib/Service/BalanceService.php

@@ -278,7 +278,7 @@ public function ensureEntitlement(string $employeeUid, int $year, int $typeId):
 			// default; other counting types start at zero, so creating the row never
 			// changes the computed balance (§6.1).
 			$type = $this->leaveTypeMapper->find($typeId);
-			$now = new \DateTime();
+			$now = $this->clock->now();
 			$ent = new Entitlement();
 			$ent->setEmployeeUid($employeeUid);
 			$ent->setYear($year);

lib/Service/EntitlementService.php

@@ -64,7 +64,7 @@ public function update(string $actorUid, int $id, array $data): Entitlement {
 			$ent->setManualAdjustment($adjustment);
 			$ent->setAdjustmentNote($data['adjustmentNote'] ?? $ent->getAdjustmentNote());
 		}
-		$ent->setUpdatedAt(new \DateTime());
+		$ent->setUpdatedAt($this->clock->now());
 		$ent = $this->entitlementMapper->update($ent);
 
 		$this->activity->publish(ActivityPublisher::SUBJECT_BALANCE_ADJUSTED, [
@@ -119,7 +119,7 @@ public function bulkSet(int $year, int $typeId, float $baseDays, ?string $group)
 		foreach ($this->targetUids($group) as $uid) {
 			$ent = $this->balanceService->ensureEntitlement($uid, $year, $typeId);
 			$ent->setBaseDays($baseDays);
-			$ent->setUpdatedAt(new \DateTime());
+			$ent->setUpdatedAt($this->clock->now());
 			$this->entitlementMapper->update($ent);
 			$count++;
 		}
@@ -154,7 +154,7 @@ public function rollover(int $fromYear): int {
 			} catch (DoesNotExistException) {
 				// The new year continues the prior year's base — never the global
 				// default, which would silently override HR-set custom entitlements.
-				$now = new \DateTime();
+				$now = $this->clock->now();
 				$next = new Entitlement();
 				$next->setEmployeeUid($prior->getEmployeeUid());
 				$next->setYear($toYear);
@@ -166,7 +166,7 @@ public function rollover(int $fromYear): int {
 				$next = $this->entitlementMapper->insert($next);
 			}
 			$next->setCarryOverDays($carry);
-			$next->setUpdatedAt(new \DateTime());
+			$next->setUpdatedAt($this->clock->now());
 			$this->entitlementMapper->update($next);
 			$affected++;
 		}
@@ -216,7 +216,7 @@ public function expireCarryOver(int $year): int {
 		foreach ($this->entitlementMapper->findForYear($year) as $ent) {
 			if ($ent->getCarryOverDays() > 0.0) {
 				$ent->setCarryOverDays(0.0);
-				$ent->setUpdatedAt(new \DateTime());
+				$ent->setUpdatedAt($this->clock->now());
 				$this->entitlementMapper->update($ent);
 				$affected++;
 			}

lib/Service/RequestService.php

@@ -652,6 +652,17 @@ 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);
+				// 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
+				// not covering the new one has to be told, or they go on believing they
+				// are on the hook. Staying silent when the person is unchanged is
+				// deliberate: they still cover, and "no longer covering" immediately
+				// followed by "you are covering" is noise, not information.
+				$previous = $retired->getReplacementUid();
+				if ($previous !== null && $previous !== '' && $previous !== $request->getReplacementUid()) {
+					$this->notifications->notifyReplacementCancelled($retired);
+				}
 			}
 			$this->applyCalendar($request);
 			// Clear the now-stale "needs a decision" notifications other deciders

src/store.js

@@ -52,7 +52,7 @@
 	/**
 	 * True when the type is recorded by HR (e.g. sick leave), not self-requested.
 	 *
 	 * @param request
 	 */
 	isHrRecorded(request) {
 		const type = this.leaveType(request.typeId)
@@ -62,7 +62,7 @@
 	 * Whether to show a status chip. HR-recorded leave (sick) that is approved has no
 	 * approval concept, so the "Approved" label is hidden as noise.
 	 *
 	 * @param request
 	 */
 	statusVisible(request) {
 		return !(this.isHrRecorded(request) && request.status === 'APPROVED')
@@ -100,7 +100,13 @@
 	},
 
 	async loadMyBalance(year) {
-		this.balance = await api.getMyBalance(year)
+		// Mirrors loadRequests: callers run the two in a Promise.all, so an
+		// unhandled rejection here took the request list down with it.
+		try {
+			this.balance = await api.getMyBalance(year)
+		} catch {
+			showError(t('absence', 'Could not load your balance'))
+		}
 	},
 
 	async createRequest(data) {
@@ -146,7 +152,7 @@
  * `text` uses Nextcloud's contrast-optimised *-text variables so labels stay
  * readable; `tint` is the base semantic colour used for the chip background.
  *
  * @param status
  */
 export function statusMeta(status) {
 	switch (status) {

src/views/hr/HrStatistics.vue

@@ -66,6 +66,7 @@
 </template>
 
 <script>
+import { showError } from '@nextcloud/dialogs'
 import { t } from '@nextcloud/l10n'
 import NcDateTimePickerNative from '@nextcloud/vue/components/NcDateTimePickerNative'
 import NcEmptyContent from '@nextcloud/vue/components/NcEmptyContent'
@@ -77,6 +78,10 @@ import StatTile from '../../components/StatTile.vue'
 import api from '../../api.js'
 import { formatDate, toIso } from '../../utils/dates.js'
 
+// Enough for a decade of monthly points; past this a line chart is unreadable
+// anyway and the range is almost certainly a typo.
+const MAX_MONTHS = 120
+
 export default {
 	name: 'HrStatistics',
 	components: { NcDateTimePickerNative, NcEmptyContent, ChartLine, LineChart, DonutChart, SkeletonList, StatTile },
@@ -91,10 +96,41 @@ export default {
 	},
 
 	computed: {
+		/**
+		 * Every month the selected range covers, in order — including the ones with
+		 * no leave at all.
+		 *
+		 * The report only returns months that *have* approved leave, which is the
+		 * right shape for a sum and the wrong one for everything else here: an
+		 * average over it divides by the months that happened to be busy, and a line
+		 * chart drawn from it joins January straight to April as though they were
+		 * adjacent, hiding the quiet quarter between them.
+		 */
+		monthsInRange() {
+			if (!this.from || !this.to || this.from > this.to) {
+				return []
+			}
+			const months = []
+			const cursor = new Date(this.from.getFullYear(), this.from.getMonth(), 1)
+			const last = new Date(this.to.getFullYear(), this.to.getMonth(), 1)
+			// A hand-typed year like 0202 would otherwise ask for tens of thousands of
+			// points; the cap keeps a fat-fingered date from freezing the page.
+			while (cursor <= last && months.length < MAX_MONTHS) {
+				months.push(`${cursor.getFullYear()}-${String(cursor.getMonth() + 1).padStart(2, '0')}`)
+				cursor.setMonth(cursor.getMonth() + 1)
+			}
+			return months
+		},
+
+		/** True once the range covers more than one year, when "Jan" stops being unique. */
+		spansYears() {
+			return new Set(this.monthsInRange.map((month) => month.slice(0, 4))).size > 1
+		},
+
 		monthData() {
-			return Object.entries(this.trends.byMonth).map(([month, value]) => ({
-				label: month.slice(5),
-				value,
+			return this.monthsInRange.map((month) => ({
+				label: this.monthLabel(month, this.spansYears ? { month: 'short', year: '2-digit' } : { month: 'short' }),
+				value: this.trends.byMonth[month] ?? 0,
 			}))
 		},
 
@@ -106,8 +142,9 @@ export default {
 			}))
 		},
 
+		/** Averaged over the months asked about, not the months that happened to be busy. */
 		perMonthAvg() {
-			const months = Object.keys(this.trends.byMonth).length
+			const months = this.monthsInRange.length
 			return months ? this.trends.total / months : 0
 		},
 
@@ -116,18 +153,22 @@ export default {
 		 * hides the August everybody disappears in.
 		 */
 		busiestMonth() {
-			const entries = Object.entries(this.trends.byMonth)
-			if (!entries.length) {
+			const peak = this.monthsInRange.reduce(
+				(best, month) => ((this.trends.byMonth[month] ?? 0) > best.value
+					? { month, value: this.trends.byMonth[month] }
+					: best),
+				{ month: null, value: 0 },
+			)
+			if (peak.month === null) {
 				return { value: 0, label: '' }
 			}
-			const [month, value] = entries.reduce((best, e) => (e[1] > best[1] ? e : best))
-			return {
-				value,
-				label: new Date(month + '-01T00:00:00').toLocaleDateString(undefined, { month: 'long', year: 'numeric' }),
-			}
+			return { value: peak.value, label: this.monthLabel(peak.month, { month: 'long', year: 'numeric' }) }
 		},
 
 		rangeCaption() {
+			if (!this.from || !this.to) {
+				return ''
+			}
 			return `${formatDate(toIso(this.from))} – ${formatDate(toIso(this.to))}`
 		},
 	},
@@ -144,10 +185,32 @@ export default {
 	methods: {
 		t,
 		fmt(v) { return Number(v).toLocaleString(undefined, { maximumFractionDigits: 1 }) },
+
+		/**
+		 * Localised name for a 'YYYY-MM' key.
+		 *
+		 * @param {string} month 'YYYY-MM'
+		 * @param {object} options Intl.DateTimeFormat options
+		 * @return {string}
+		 */
+		monthLabel(month, options) {
+			return new Date(month + '-01T00:00:00').toLocaleDateString(undefined, options)
+		},
+
 		async reload() {
+			// The native date inputs report null when cleared, and every date helper
+			// here would throw on it. Nothing to ask the server for either.
+			if (!this.from || !this.to) {
+				return
+			}
 			this.loading = true
 			try {
 				this.trends = await api.reportTrends(toIso(this.from), toIso(this.to))
+			} catch (e) {
+				// Without this the view kept the previous range's figures on screen
+				// with no hint that the new ones never arrived.
+				this.trends = { byMonth: {}, byType: [], total: 0 }
+				showError(e.response?.data?.message || t('absence', 'Could not load statistics'))
 			} finally {
 				this.loading = false
 			}

tests/Unit/Service/RequestServiceTest.php

@@ -276,6 +276,62 @@ public function testApprovingAnEditRetiresTheOriginalInOneTransaction(): void {
 		$this->assertSame(LeaveRequest::STATUS_CANCELLED, $original->getStatus());
 	}
 
+	public function testReplacingTheReplacementReleasesThePreviousOne(): void {
+		// retireSuperseded() cancels the original with a direct write, so it never
+		// passes through transitionToCancelled() where the replacement is normally
+		// released. Without this the colleague who agreed to cover the old dates is
+		// left believing they still do, while somebody else is told they cover.
+		$original = $this->pendingOwnRequest();
+		$original->setId(5);
+		$original->setStatus(LeaveRequest::STATUS_APPROVED);
+		$original->setReplacementUid('ada');
+
+		$edit = $this->pendingOwnRequest();
+		$edit->setId(6);
+		$edit->setSupersedesId(5);
+		$edit->setStatus(LeaveRequest::STATUS_PENDING);
+		$edit->setReplacementUid('grace');
+
+		$this->requestMapper->method('find')->willReturnMap([[6, $edit], [5, $original]]);
+		$this->permission->method('canView')->willReturn(true);
+		$this->permission->method('canDecide')->willReturn(true);
+		$this->requestMapper->method('update')->willReturnArgument(0);
+
+		$released = null;
+		$this->notifications->method('notifyReplacementCancelled')
+			->willReturnCallback(function (LeaveRequest $r) use (&$released): void {
+				$released = $r->getReplacementUid();
+			});
+
+		$this->service->approve('boss', 6, null);
+
+		$this->assertSame('ada', $released);
+	}
+
+	public function testKeepingTheSameReplacementDoesNotNotifyThem(): void {
+		// They still cover. "No longer covering" immediately followed by "you are
+		// covering" is noise, not information.
+		$original = $this->pendingOwnRequest();
+		$original->setId(5);
+		$original->setStatus(LeaveRequest::STATUS_APPROVED);
+		$original->setReplacementUid('ada');
+
+		$edit = $this->pendingOwnRequest();
+		$edit->setId(6);
+		$edit->setSupersedesId(5);
+		$edit->setStatus(LeaveRequest::STATUS_PENDING);
+		$edit->setReplacementUid('ada');
+
+		$this->requestMapper->method('find')->willReturnMap([[6, $edit], [5, $original]]);
+		$this->permission->method('canView')->willReturn(true);
+		$this->permission->method('canDecide')->willReturn(true);
+		$this->requestMapper->method('update')->willReturnArgument(0);
+
+		$this->notifications->expects(self::never())->method('notifyReplacementCancelled');
+
+		$this->service->approve('boss', 6, null);
+	}
+
 	public function testAFailedWriteRollsTheTransactionBack(): void {
 		$edit = $this->pendingOwnRequest();
 		$edit->setId(6);