From ac62355ade166d1339d920daea4f11861eb90a09 Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Mon, 10 Aug 2026 23:13:29 +0200 Subject: [PATCH 1/6] feat(notifications): decide a request straight from the notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A manager's common answer is "yes", and it cost a page load. The four notifications that ask for a decision — a new request, an escalation, the overdue reminder and a withdrawal — now carry Approve, Decline and Review. Approve POSTs to the same endpoint the app uses, so the notification dismisses itself and nothing opens. Declining is deliberately not a one-click verdict: §5.2 requires a reason, and a manager able to reject somebody's holiday from a toast without saying why would be a worse app, not a faster one. Its button is a deep link that opens the request with the reason box already unfolded, which is still a step better than "Review" for someone who has decided to say no. The reminder previously carried no buttons at all, which is exactly the notification where the decision is most overdue. A withdrawal asks the opposite question, so its buttons read "Approve withdrawal" and "Keep leave", matching the sidebar. Notifications that merely report an outcome stay button-free: nothing is owed on them. Co-Authored-By: Claude Opus 5 (1M context) --- lib/Notification/Notifier.php | 63 ++++++++++++-- src/components/RequestSidebar.vue | 18 ++++ tests/Unit/Notification/NotifierTest.php | 106 +++++++++++++++++++++-- 3 files changed, 172 insertions(+), 15 deletions(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index df359bf..93b4277 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -11,6 +11,7 @@ use OCA\Absence\Service\ConfigService; use OCA\Absence\Service\NoticeService; use OCA\Absence\Service\NotificationService; +use OCP\IL10N; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\L10N\IFactory; @@ -125,19 +126,63 @@ public function prepare(INotification $notification, string $languageCode): INot $link = $this->urlGenerator->linkToRouteAbsolute('absence.page.index') . '#/requests/' . $requestId; $notification->setLink($link); - // Actionable approve/reject buttons for decision-makers. - if (in_array($notification->getSubject(), [NotificationService::SUBJECT_NEW_REQUEST, NotificationService::SUBJECT_ESCALATION, NotificationService::SUBJECT_WITHDRAWAL], true)) { - $open = $notification->createAction(); - $open->setLabel('open') - ->setParsedLabel($l->t('Review')) - ->setLink($link, 'WEB') - ->setPrimary(true); - $notification->addParsedAction($open); - } + $this->addDecisionActions($notification, $l, $requestId, $link); return $notification; } + /** + * Buttons for the people who still owe this request a decision. + * + * Approving is the common answer and the one that needs nothing typed, so it + * happens in place: the button POSTs to the same endpoint the app uses, and the + * notification disappears. Declining is deliberately *not* a one-click verdict — + * §5.2 requires a reason, and a manager who could reject someone's holiday from + * a toast without saying why would be a worse app, not a faster one. Its button + * therefore opens the request with the reason box already unfolded, which is + * still a step better than "Review" for someone who has decided to say no. + */ + private function addDecisionActions(INotification $notification, IL10N $l, string $requestId, string $link): void { + $subject = $notification->getSubject(); + $deciding = [ + NotificationService::SUBJECT_NEW_REQUEST, + NotificationService::SUBJECT_ESCALATION, + // The reminder exists *because* the decision is overdue — it is the place + // a one-click answer pays off most. + NotificationService::SUBJECT_REMINDER, + NotificationService::SUBJECT_WITHDRAWAL, + ]; + if (!in_array($subject, $deciding, true)) { + return; + } + + // A withdrawal asks the opposite question, so the buttons have to read the + // opposite way — the same wording the sidebar uses (RequestSidebar.vue). + $isWithdrawal = $subject === NotificationService::SUBJECT_WITHDRAWAL; + + $approve = $notification->createAction(); + $approve->setLabel('approve') + ->setParsedLabel($isWithdrawal ? $l->t('Approve withdrawal') : $l->t('Approve')) + ->setLink($this->urlGenerator->linkToRouteAbsolute('absence.request.approve', ['id' => $requestId]), 'POST') + ->setPrimary(true); + $notification->addParsedAction($approve); + + $decline = $notification->createAction(); + $decline->setLabel('decline') + ->setParsedLabel($isWithdrawal ? $l->t('Keep leave') : $l->t('Decline')) + // The query rides inside the hash, where the SPA router reads it. + ->setLink($link . '?decide=decline', 'WEB') + ->setPrimary(false); + $notification->addParsedAction($decline); + + $open = $notification->createAction(); + $open->setLabel('open') + ->setParsedLabel($l->t('Review')) + ->setLink($link, 'WEB') + ->setPrimary(false); + $notification->addParsedAction($open); + } + private function displayName(string $uid): string { if ($uid === '') { return ''; diff --git a/src/components/RequestSidebar.vue b/src/components/RequestSidebar.vue index 4e7e0e6..286b800 100644 --- a/src/components/RequestSidebar.vue +++ b/src/components/RequestSidebar.vue @@ -413,6 +413,7 @@ export default { this.rejecting = false this.rejectComment = '' this.confirmingCancel = false + this.applyDecideIntent() } catch { showError(t('absence', 'Could not load the request')) this.$emit('close') @@ -431,6 +432,23 @@ export default { } }, + /** + * "Decline" in a notification links here with `?decide=decline`, so the + * reason box is already open when the sidebar arrives — the click that meant + * "no" should not land on a screen that looks like a read-only summary. The + * verdict itself is still the manager's to confirm, and the intent is + * consumed from the URL so re-selecting another request does not inherit it. + */ + applyDecideIntent() { + if (this.$route?.query?.decide !== 'decline') { + return + } + this.$router.replace({ path: this.$route.path }) + if (this.detail.canDecide && this.isDecidable) { + this.rejecting = true + } + }, + startReject() { this.rejecting = true }, diff --git a/tests/Unit/Notification/NotifierTest.php b/tests/Unit/Notification/NotifierTest.php index ccced4f..32941f4 100644 --- a/tests/Unit/Notification/NotifierTest.php +++ b/tests/Unit/Notification/NotifierTest.php @@ -28,6 +28,8 @@ class NotifierTest extends TestCase { private Notifier $notifier; private string $parsedSubject = ''; private string $parsedMessage = ''; + /** @var list */ + private array $actions = []; protected function setUp(): void { parent::setUp(); @@ -56,21 +58,60 @@ function (string $uid): ?IUser { }, ); - $this->notifier = new Notifier($l10nFactory, $this->createMock(IURLGenerator::class), $userManager); + // Routing is what the buttons are: a mislabelled one is cosmetic, a + // mis-routed one silently approves nothing (or the wrong thing). + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('linkToRouteAbsolute')->willReturnCallback( + static fn (string $route, array $args = []): string => match ($route) { + 'absence.page.index' => 'https://cloud.example/apps/absence/', + 'absence.request.approve' => 'https://cloud.example/apps/absence/api/requests/' . $args['id'] . '/approve', + default => 'https://cloud.example/' . $route, + }, + ); + + $this->notifier = new Notifier($l10nFactory, $urlGenerator, $userManager); + $this->actions = []; } /** @param array $parameters */ private function prepare(string $subject, array $parameters): void { - // The action is built with a fluent chain, so its setters have to return it. - $action = $this->createMock(IAction::class); - $action->method(self::anything())->willReturnSelf(); - + // Tests that prepare more than one notification assert on the last of them. + $this->actions = []; $notification = $this->createMock(INotification::class); $notification->method('getApp')->willReturn(ConfigService::APP_ID); $notification->method('getSubject')->willReturn($subject); $notification->method('getSubjectParameters')->willReturn($parameters); $notification->method('getObjectId')->willReturn('7'); - $notification->method('createAction')->willReturn($action); + // Each action is built with a fluent chain, so every setter returns it; the + // ones that carry meaning are recorded as they are called. + $notification->method('createAction')->willReturnCallback( + function (): IAction { + $slot = count($this->actions); + $this->actions[] = ['label' => '', 'link' => '', 'verb' => '', 'primary' => false]; + $action = $this->createMock(IAction::class); + $action->method(self::anything())->willReturnSelf(); + $action->method('setParsedLabel')->willReturnCallback( + function (string $label) use ($action, $slot): IAction { + $this->actions[$slot]['label'] = $label; + return $action; + }, + ); + $action->method('setLink')->willReturnCallback( + function (string $link, string $verb) use ($action, $slot): IAction { + $this->actions[$slot]['link'] = $link; + $this->actions[$slot]['verb'] = $verb; + return $action; + }, + ); + $action->method('setPrimary')->willReturnCallback( + function (bool $primary) use ($action, $slot): IAction { + $this->actions[$slot]['primary'] = $primary; + return $action; + }, + ); + return $action; + }, + ); $notification->method('setParsedSubject')->willReturnCallback( function (string $text) use ($notification): INotification { $this->parsedSubject = $text; @@ -154,4 +195,57 @@ public function testTheEscalationAndReminderCarryTheWarningToo(): void { self::assertSame('Short notice: Emp is still waiting for a decision', $this->parsedSubject); self::assertSame('The leave starts today, with none of the 14 days of notice expected.', $this->parsedMessage); } + + public function testApprovingIsOneClickAndDoesNotOpenTheApp(): void { + // The whole point: the common answer costs a click, not a page load. If this + // ever became a WEB link the feature would still look right and do nothing. + $this->prepare(NotificationService::SUBJECT_NEW_REQUEST, ['employee' => 'emp', 'requestId' => '7']); + + self::assertSame( + ['Approve', 'Decline', 'Review'], + array_column($this->actions, 'label'), + ); + self::assertSame([ + 'label' => 'Approve', + 'link' => 'https://cloud.example/apps/absence/api/requests/7/approve', + 'verb' => 'POST', + 'primary' => true, + ], $this->actions[0]); + } + + public function testDecliningOpensTheReasonFormRatherThanDecidingOutright(): void { + // A reason is mandatory (§5.2), so "Decline" may not be a verdict — it is a + // deep link that lands in the form with the box open. + $this->prepare(NotificationService::SUBJECT_NEW_REQUEST, ['employee' => 'emp', 'requestId' => '7']); + + self::assertSame('WEB', $this->actions[1]['verb']); + self::assertSame('https://cloud.example/apps/absence/#/requests/7?decide=decline', $this->actions[1]['link']); + self::assertFalse($this->actions[1]['primary']); + } + + public function testTheOverdueReminderCarriesTheSameButtons(): void { + // The reminder fires precisely because nobody has decided yet, so it is the + // notification where a one-click answer is worth the most. + $this->prepare(NotificationService::SUBJECT_REMINDER, ['employee' => 'emp', 'requestId' => '7']); + + self::assertSame(['Approve', 'Decline', 'Review'], array_column($this->actions, 'label')); + } + + public function testAWithdrawalAsksTheOppositeQuestion(): void { + // Here "approve" cancels the leave and "decline" keeps it — labelling both + // the usual way round would have managers clicking the opposite of what they mean. + $this->prepare(NotificationService::SUBJECT_WITHDRAWAL, ['employee' => 'emp', 'requestId' => '7']); + + self::assertSame(['Approve withdrawal', 'Keep leave', 'Review'], array_column($this->actions, 'label')); + } + + public function testAnEmployeeGetsNoDecisionButtonsOnTheirOwnOutcome(): void { + // Nothing is owed on these, and an Approve button on "your leave was + // approved" would point at an endpoint the recipient may not even call. + $this->prepare(NotificationService::SUBJECT_APPROVED, ['employee' => 'emp', 'requestId' => '7']); + self::assertSame([], $this->actions); + + $this->prepare(NotificationService::SUBJECT_COMMENT, ['employee' => 'emp', 'requestId' => '7']); + self::assertSame([], $this->actions); + } } From e156eadada3aeba76a19bf5722b1bdedd8e9f9f6 Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Mon, 10 Aug 2026 23:13:42 +0200 Subject: [PATCH 2/6] feat(requests): name the colleagues already off while the dates are picked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The coverage check has always existed, but only the manager saw it — at approval time, when changing the dates is expensive for everyone. The same team-scope query now runs as the dates move in the request dialog and names who is already off, warning when booking would take the team to the configured concurrency limit. It is advisory in the strongest sense: it never disables submit, a failed lookup simply omits the hint, and an employee stays free to book a clash they have already agreed with their team. The point is that the person choosing the dates learns what the manager will see while they can still cheaply choose differently. Shown only for one's own leave — the endpoint answers for the caller's team, so HR recording an absence for somebody else would otherwise be shown the wrong team's names, and no hint beats a misleading one. Leave types stay neutralised by the shared-calendar visibility policy, so a colleague's sick leave does not become visible because somebody opened the booking form. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/RequestDialog.vue | 100 ++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-) diff --git a/src/components/RequestDialog.vue b/src/components/RequestDialog.vue index 5b125a4..632b6a2 100644 --- a/src/components/RequestDialog.vue +++ b/src/components/RequestDialog.vue @@ -125,6 +125,15 @@ {{ t('absence', 'Heads up: this goes beyond your available balance. You can still submit — HR may approve it.') }} + + + + +