diff --git a/CLAUDE.md b/CLAUDE.md index 484d3db..d2967dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,6 +18,17 @@ Deck's ACL/permission checks read the *live* `IUserSession`, not a value frozen The filter-matching logic (`WorkflowRunner::cardMatchesFilters()`, `::isOverdue()`) is deliberately `static` and side-effect-free so it's unit-testable without a real Deck installation — see `tests/Unit/Service/WorkflowRunnerFilterTest.php`. +**Not everything in Deck reads the live session, though.** `BoardService` takes the current user as `private ?string $userId` — a plain string frozen when the container builds it. `getUserBoards()` uses exactly that, so in the background job (one process, many users, a container-cached `BoardService`) it answers for whoever happened to be impersonated at first resolution, or for `null`. `listBoardsForCurrentUser()` is therefore **request-only**; the job asks `findStackIds()` instead, which goes through `StackService::findAll()` → `PermissionService` → live session. Before using any other `OCA\Deck\*` service from the runner, check which of the two kinds it is — the failure mode is silent and user-crossing, not an error. + +## Workflows are auto-disabled when their Deck targets vanish + +`WorkflowRunner::findBrokenTarget()` checks board and stacks before every run and, if something is gone, switches the workflow off (`enabled = false`) and sends a one-off mail. Two invariants hold this together and must survive refactors: + +- **Only positive knowledge disables.** `DeckIntegrationService::findStackIds()` returns `null` only for "board missing/not readable" (`DoesNotExistException`, Deck's `NoPermissionException`/`NotFoundException`) and *throws* for everything else. The runner skips the workflow on a throw. If you make that method degrade to `[]`/`null` on generic errors, a Deck outage will disable every workflow on the instance and mail every user about it. +- **The mail ignores the `notifyEmail` flag** — that flag is about moved cards; this is a notice that the user's configuration stopped working. It stays a one-off simply because a disabled workflow is not picked up by `findAllEnabled()` again. + +Archived boards deliberately do *not* trigger this (they stay readable, so `findStackIds()` succeeds), even though they're filtered out of the settings dropdowns. + ## Job interval comes from config.php `RunWorkflowsJob` reads `workflow_deck_automation.interval` (seconds, default 300, clamped to a 60s minimum) via `IConfig::getSystemValueInt()` in its constructor. That works *because* `TimedJob` re-reads `$this->interval` on every cron pass from the freshly constructed job — so editing `config.php` takes effect immediately, with no `occ` command and no re-registration. Don't move this into `appinfo/info.xml` or a stored app config; the constants (`INTERVAL_CONFIG_KEY`, `DEFAULT_INTERVAL`, `MINIMUM_INTERVAL`) on the job are the single source of truth and are referenced from the README. diff --git a/README.md b/README.md index 915c274..49e2bd4 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,16 @@ Ein Nutzer kann beliebig viele Workflows anlegen, bearbeiten, deaktivieren oder Jeder Nutzer findet die Einstellungen unter **Persönliche Einstellungen → Deck Workflow-Automatisierung**. Dort können neue Workflows angelegt, bestehende bearbeitet oder gelöscht werden. Die Dropdowns für Board/Stapel/Label/Benutzer werden live aus Deck geladen. Zur Auswahl stehen nur aktive Boards — archivierte und im Papierkorb liegende Boards sowie gelöschte Stapel blendet Deck bzw. die App aus. +### Gelöschte Boards und Stapel + +Ein Workflow verweist per ID auf ein Board und zwei Stapel. Wird eines davon in Deck gelöscht, passiert Folgendes: + +- **Beim Speichern** werden Board und Stapel geprüft; ein Workflow auf nicht mehr existierende Ziele lässt sich gar nicht erst anlegen. +- **In der Übersicht** wird ein betroffener Workflow rot markiert („Board nicht mehr vorhanden", „Quell-Stapel nicht mehr vorhanden" …). +- **Im Hintergrundjob** wird der Workflow automatisch deaktiviert und der Besitzer einmalig per E-Mail informiert — unabhängig davon, ob für den Workflow „E-Mail senden" aktiviert ist, denn hier geht es nicht um verschobene Karten, sondern darum, dass die Automatisierung nicht mehr läuft. Danach bleibt es still: Ein deaktivierter Workflow wird nicht erneut ausgewertet. + +Ist Deck vorübergehend nicht erreichbar, wird **nicht** deaktiviert — der Job überspringt den Workflow und versucht es beim nächsten Lauf erneut. Archivierte Boards lösen die Deaktivierung ebenfalls nicht aus, sie sind nur in den Auswahlfeldern ausgeblendet. + ## Konfiguration Das Prüfintervall des Hintergrundjobs lässt sich in der `config.php` setzen (Wert in **Sekunden**): diff --git a/lib/Controller/WorkflowController.php b/lib/Controller/WorkflowController.php index 6c8f364..87349fa 100644 --- a/lib/Controller/WorkflowController.php +++ b/lib/Controller/WorkflowController.php @@ -59,7 +59,7 @@ class WorkflowController extends OCSController { bool $notifyEmail = false, bool $enabled = true, ): DataResponse { - $this->validate($title, $sourceStackId, $targetStackId); + $this->validate($title, $boardId, $sourceStackId, $targetStackId); $workflow = new Workflow(); $workflow->setUserId($this->currentUserId()); @@ -85,7 +85,7 @@ class WorkflowController extends OCSController { bool $notifyEmail = false, bool $enabled = true, ): DataResponse { - $this->validate($title, $sourceStackId, $targetStackId); + $this->validate($title, $boardId, $sourceStackId, $targetStackId); try { $workflow = $this->workflowMapper->findForUser($id, $this->currentUserId()); @@ -147,13 +147,35 @@ class WorkflowController extends OCSController { } } - private function validate(string $title, int $sourceStackId, int $targetStackId): void { + private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId): void { if (trim($title) === '') { throw new OCSBadRequestException('Title must not be empty'); } if ($sourceStackId === $targetStackId) { throw new OCSBadRequestException('Source and target stack must differ'); } + + // Reject references to boards or stacks that are gone (or were never + // the user's) instead of storing a workflow that can only fail later. + // assertDeck() ran first, so an empty list here means "not there", + // not "Deck is unreachable". + $this->assertDeck(); + + $boardIds = array_map( + static fn (array $board) => (int)$board['id'], + $this->deckService->listBoardsForCurrentUser(), + ); + if (!in_array($boardId, $boardIds, true)) { + throw new OCSBadRequestException('The selected board does not exist or is not accessible'); + } + + $stackIds = array_map( + static fn (array $stack) => (int)$stack['id'], + $this->deckService->listStacks($boardId), + ); + if (!in_array($sourceStackId, $stackIds, true) || !in_array($targetStackId, $stackIds, true)) { + throw new OCSBadRequestException('The selected stack does not exist on this board'); + } } /** diff --git a/lib/Service/DeckIntegrationService.php b/lib/Service/DeckIntegrationService.php index 78a07d0..e8be7ce 100644 --- a/lib/Service/DeckIntegrationService.php +++ b/lib/Service/DeckIntegrationService.php @@ -10,6 +10,7 @@ use OCA\Deck\Service\BoardService; use OCA\Deck\Service\CardService; use OCA\Deck\Service\StackService; use OCP\App\IAppManager; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -57,6 +58,13 @@ class DeckIntegrationService { /** * @return array + * + * Only ever call this from a *request*: Deck injects the current user id + * into BoardService as a plain string frozen at construction time, so in + * the background job (one process, many users, a container-cached + * BoardService) this would answer for the wrong user — or for none at + * all. The job uses findStackIds() instead, which goes through Deck's + * PermissionService and therefore reads the live session. */ public function listBoardsForCurrentUser(): array { $boards = $this->call(function () { @@ -110,11 +118,61 @@ class DeckIntegrationService { }, []); return array_map( - static fn ($stack) => ['id' => $stack->getId(), 'title' => $stack->getTitle()], + static fn ($stack) => ['id' => (int)$stack->getId(), 'title' => $stack->getTitle()], $stacks, ); } + /** + * Ids of the board's stacks, for deciding whether a stored workflow still + * points at anything real. + * + * Returns `null` when the board itself is gone or not readable for the + * current user — `StackService::findAll()` runs a Deck permission check + * first, and that one reads the *live* session, which is what makes this + * usable from the impersonating background job. Any other failure throws, + * because "Deck is broken right now" must never be mistaken for "the user + * deleted this board". + * + * @return int[]|null + * @throws DeckUnavailableException + */ + public function findStackIds(int $boardId): ?array { + try { + $stacks = $this->resolve(StackService::class)->findAll($boardId); + } catch (DeckUnavailableException $e) { + throw $e; + } catch (Throwable $e) { + if ($this->isMissingOrForbidden($e)) { + return null; + } + $this->logger->error('Could not list stacks of board ' . $boardId . ': ' . $e->getMessage(), [ + 'app' => 'workflow_deck_automation', + 'exception' => $e, + ]); + throw new DeckUnavailableException('Deck call failed: ' . $e->getMessage(), 0, $e); + } + + return array_map(static fn ($stack) => (int)$stack->getId(), $stacks); + } + + /** + * Deck's own exception classes are referenced by name: `is_a()` with a + * string simply returns false when the class does not exist, so a missing + * or renamed Deck degrades to "unknown error" instead of fataling. + */ + private function isMissingOrForbidden(Throwable $e): bool { + if ($e instanceof DoesNotExistException) { + return true; + } + foreach (['OCA\Deck\NoPermissionException', 'OCA\Deck\NotFoundException'] as $class) { + if (is_a($e, $class)) { + return true; + } + } + return false; + } + /** * @return array */ diff --git a/lib/Service/NotificationMailer.php b/lib/Service/NotificationMailer.php index 8c35f38..c803f06 100644 --- a/lib/Service/NotificationMailer.php +++ b/lib/Service/NotificationMailer.php @@ -78,6 +78,65 @@ class NotificationMailer { } } + /** + * One-off notice that a workflow was switched off because the board or + * stack it points at is gone. + * + * @param WorkflowRunner::TARGET_* $brokenTarget + */ + public function sendWorkflowDisabledNotification(IUser $user, Workflow $workflow, string $brokenTarget): void { + $email = $user->getEMailAddress(); + if ($email === null || $email === '') { + $this->logger->info('Cannot notify about disabled workflow ' . $workflow->getId() . ': user ' . $user->getUID() . ' has no email address', [ + 'app' => 'workflow_deck_automation', + ]); + return; + } + + $reason = match ($brokenTarget) { + WorkflowRunner::TARGET_SOURCE_STACK => $this->l10n->t('its source stack no longer exists'), + WorkflowRunner::TARGET_TARGET_STACK => $this->l10n->t('its target stack no longer exists'), + default => $this->l10n->t('its board no longer exists or is no longer available to you'), + }; + + try { + $settingsLink = $this->urlGenerator->getAbsoluteURL('/settings/user/workflow_deck_automation'); + + $template = $this->mailer->createEMailTemplate('workflow_deck_automation.WorkflowDisabled', [ + 'workflowTitle' => $workflow->getTitle(), + ]); + $template->setSubject($this->l10n->t('Deck workflow deactivated: %s', [$workflow->getTitle()])); + $template->addHeader(); + $template->addHeading($this->l10n->t('A workflow was deactivated'), false); + $template->addBodyText($this->l10n->t( + 'The workflow "%1$s" was switched off automatically because %2$s. No cards are being moved by it any more.', + [$workflow->getTitle(), $reason], + )); + $template->addBodyText($this->l10n->t( + 'Delete the workflow or point it at an existing board and stack to reactivate it. You will not be reminded about this workflow again.', + )); + $template->addBodyButton($this->l10n->t('Open settings'), $settingsLink); + $template->addFooter(); + + $message = $this->mailer->createMessage(); + $message->setTo([$email => $user->getDisplayName()]); + $message->setFrom([Util::getDefaultEmailAddress('noreply') => $this->l10n->t('Deck Workflow Automation')]); + $message->useTemplate($template); + + $failedRecipients = $this->mailer->send($message); + if (!empty($failedRecipients)) { + $this->logger->error('Deactivation mail for workflow ' . $workflow->getId() . ' failed for: ' . implode(', ', $failedRecipients), [ + 'app' => 'workflow_deck_automation', + ]); + } + } catch (Throwable $e) { + $this->logger->error('Could not send deactivation mail for workflow ' . $workflow->getId() . ': ' . $e->getMessage(), [ + 'app' => 'workflow_deck_automation', + 'exception' => $e, + ]); + } + } + /** * Appends the card's description, if it has one. * diff --git a/lib/Service/WorkflowRunner.php b/lib/Service/WorkflowRunner.php index 9f58922..65b0617 100644 --- a/lib/Service/WorkflowRunner.php +++ b/lib/Service/WorkflowRunner.php @@ -26,6 +26,10 @@ use Throwable; * the standard Nextcloud pattern for multi-user cron jobs. */ class WorkflowRunner { + public const TARGET_BOARD = 'board'; + public const TARGET_SOURCE_STACK = 'sourceStack'; + public const TARGET_TARGET_STACK = 'targetStack'; + private ?IUser $impersonationRestore = null; public function __construct( @@ -110,6 +114,23 @@ class WorkflowRunner { } private function runWorkflow(IUser $user, Workflow $workflow): void { + try { + $brokenTarget = $this->findBrokenTarget($workflow); + } catch (DeckUnavailableException $e) { + // Could not determine it either way — leave the workflow alone + // rather than disabling it over a temporary Deck problem. + $this->logger->warning('Could not verify targets of workflow {id}: ' . $e->getMessage(), [ + 'app' => 'workflow_deck_automation', + 'id' => $workflow->getId(), + ]); + return; + } + + if ($brokenTarget !== null) { + $this->disableBrokenWorkflow($user, $workflow, $brokenTarget); + return; + } + try { $cards = $this->deckService->getActiveCardsInStack($workflow->getSourceStackId()); } catch (DeckUnavailableException $e) { @@ -141,6 +162,54 @@ class WorkflowRunner { $this->workflowMapper->update($workflow); } + /** + * Which of the workflow's Deck references no longer exists, if any. + * + * Deleting a board or stack leaves the workflow row untouched — there is + * no foreign key — and Deck keeps the orphaned cards readable until its + * own DeleteCron purges them, so the workflow would otherwise keep + * failing on every single run. + * + * @return self::TARGET_*|null + * @throws DeckUnavailableException if Deck could not be asked + */ + private function findBrokenTarget(Workflow $workflow): ?string { + $stackIds = $this->deckService->findStackIds($workflow->getBoardId()); + if ($stackIds === null) { + return self::TARGET_BOARD; + } + + if (!in_array($workflow->getSourceStackId(), $stackIds, true)) { + return self::TARGET_SOURCE_STACK; + } + if (!in_array($workflow->getTargetStackId(), $stackIds, true)) { + return self::TARGET_TARGET_STACK; + } + + return null; + } + + /** + * @param self::TARGET_* $brokenTarget + */ + private function disableBrokenWorkflow(IUser $user, Workflow $workflow, string $brokenTarget): void { + $workflow->setEnabled(false); + $workflow->setLastRun($this->timeFactory->getDateTime()); + $this->workflowMapper->update($workflow); + + $this->logger->warning('Disabled workflow {id}: its {target} no longer exists', [ + 'app' => 'workflow_deck_automation', + 'id' => $workflow->getId(), + 'target' => $brokenTarget, + ]); + + // Sent regardless of the workflow's notifyEmail setting: that flag is + // about moved cards, this is a one-off notice that the automation the + // user configured has been switched off. It stays one-off because a + // disabled workflow is not picked up again. + $this->mailer->sendWorkflowDisabledNotification($user, $workflow, $brokenTarget); + } + private function moveAndNotify(IUser $user, Workflow $workflow, Card $card): void { try { $this->deckService->moveCard($card->getId(), $workflow->getTargetStackId()); diff --git a/src/PersonalSettings.vue b/src/PersonalSettings.vue index 721117a..3c2e28e 100644 --- a/src/PersonalSettings.vue +++ b/src/PersonalSettings.vue @@ -25,7 +25,12 @@ - {{ workflow.title }} + + {{ workflow.title }} + + {{ workflowIssues[workflow.id] }} + + {{ stackLabel(workflow.boardId, workflow.sourceStackId) }} {{ stackLabel(workflow.boardId, workflow.targetStackId) }} @@ -175,6 +180,40 @@ const boardOptions = computed(() => boards.value.map((board) => ({ value: board. const sourceStackOptions = computed(() => stackOptions.value.filter((option) => option.value !== form.targetStackId)) const targetStackOptions = computed(() => stackOptions.value.filter((option) => option.value !== form.sourceStackId)) +/** + * Workflows whose board or stacks were deleted in Deck, keyed by id. The + * background job switches these off by itself, but a row that just stopped + * working needs to say so instead of looking healthy. + */ +const workflowIssues = computed(() => { + const issues = {} + + for (const workflow of workflows.value) { + if (!boards.value.some((board) => board.id === workflow.boardId)) { + issues[workflow.id] = 'Board nicht mehr vorhanden' + continue + } + + const stacks = stacksByBoard[workflow.boardId] + if (!stacks) { + continue + } + + const missing = [] + if (!stacks.some((stack) => stack.id === workflow.sourceStackId)) { + missing.push('Quell-Stapel') + } + if (!stacks.some((stack) => stack.id === workflow.targetStackId)) { + missing.push('Ziel-Stapel') + } + if (missing.length) { + issues[workflow.id] = `${missing.join(' und ')} nicht mehr vorhanden` + } + } + + return issues +}) + const form = reactive(emptyForm()) function emptyForm() { @@ -370,6 +409,12 @@ onMounted(loadAll) justify-content: flex-end; } +.wfda-issue { + display: block; + color: var(--color-error); + font-size: 0.9em; +} + .wfda-empty { color: var(--color-text-maxcontrast); margin-bottom: 16px;