diff --git a/CLAUDE.md b/CLAUDE.md index a1e3b09..eb6d054 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ Alle Lesezugriffe (Boards, Stacks, Karten) und das Verschieben laufen über Deck **Regel:** Bevor ein weiterer `OCA\Deck\*`-Dienst im Runner genutzt wird, prüfen, ob er eine eingefrorene `$userId` trägt – der Fehler ist still und nutzerübergreifend. Bevorzugt Mapper (QBMapper ohne Nutzerzustand) oder Methoden mit explizitem uid-Parameter verwenden. Deshalb wird `BoardService::getUserBoards()` nicht verwendet. -Der Datumsvergleich `WorkflowRunner::hasPassed()`, `TriggerDate` und die Empfängerregel `NotificationPolicy::shouldNotify()` sind bewusst `static` und seiteneffektfrei, also ohne Deck unit-testbar (`tests/Unit/Service/`). Das sind die einzigen getesteten Teile der App. Neue reine Logik in dieser Form halten. +Der Datumsvergleich `WorkflowRunner::hasPassed()`, `TriggerDate`, `LoopDetector` und die Empfängerregel `NotificationPolicy::shouldNotify()` sind bewusst `static` und seiteneffektfrei, also ohne Deck unit-testbar (`tests/Unit/Service/`). Das sind die einzigen getesteten Teile der App. Neue reine Logik in dieser Form halten. ## Workflows werden deaktiviert, wenn Board/Stack verschwinden @@ -79,6 +79,14 @@ Die App wird nur intern genutzt und spricht ausschließlich Deutsch. Oberfläche - **Texte:** `TriggerDate::describeReason()` liefert den Satzteil („ist überfällig“ / „hat ihr Startdatum erreicht“) für Mail und Notifier. `NotificationPusher` speichert `triggerDate` in den Subject-Parametern. Ältere Benachrichtigungen ohne den Schlüssel gelten als `due`. - **Empfänger:** Auch Startdatums-Workflows nutzen bei `notify_mode = board` Decks `notify-due`-Einstellung. Deck hat keine eigene für das Startdatum. +## Keine Schleifen, kein Quelle = Ziel + +Jeder aktive Workflow ist eine Kante „Quell-Stack → Ziel-Stack“. `LoopDetector` (rein, getestet) findet Kreise. „Quelle = Ziel“ ist ein Kreis der Länge 1. Das Auslöse-Datum wird bewusst ignoriert: Bei S1→S2 per Start und S2→S1 per Fälligkeit pendelt jede Karte, bei der beide Daten erreicht sind. Ohne Prüfung würde der Runner sie sogar im selben Lauf hin- und herschieben (jeder Workflow liest seinen Quell-Stack neu), mit einer Benachrichtigung pro Bewegung. Ketten sind erlaubt. + +- **Controller:** `validate()` → `assertNoLoop()` lehnt einen *aktiven* Workflow ab, dessen Ziel über die anderen aktiven Workflows des Boards zu seiner Quelle zurückführt, und nennt sie. Deaktivierte Workflows zählen nicht. Beim Aktivieren läuft dieselbe Prüfung, weil auch das über `update()` geht. +- **Runner und Liste:** Altbestand kann trotzdem Kreise bilden. `run()` überspringt alle Workflows auf einem Kreis (nur Log, keine Deaktivierung, keine Mail), und `index()` liefert `loop: true`, das die Oberfläche rot markiert. +- Stack-IDs sind in Deck boardübergreifend eindeutig, deshalb darf der Runner alle Workflows der Instanz in einen Graphen werfen. + ## Fälligkeit wird als UTC-Zeitpunkt verglichen Decks `duedate` und `startdate` sind `DATETIME`-Spalten ohne Zeitzone mit UTC-Uhrzeit (Frontend sendet `toISOString()`). `Entity` baut daraus `new \DateTime($string)` in PHPs Standardzone, die Nextcloud auf UTC setzt. `WorkflowRunner::asStoredUtc()` legt die Uhrzeit trotzdem ausdrücklich auf UTC fest, damit das Ergebnis nicht von dieser Standardzone abhängt. Decks eigene Pfade `Card::getDaysUntilDue()` (tagesgenau) und `CardMapper::findOverdue()` (vergleicht mit dem `NOW()` der Datenbank, also Ortszeit) sind dafür ungeeignet und werden nicht verwendet. diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index bd01ff2..b53a58c 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -57,6 +57,7 @@ composer run test:unit # PHPUnit Die Unit-Tests decken nur reine Logik ab, ohne Deck-Installation: - Datumsvergleich für Fälligkeits- und Startdatum (`WorkflowRunner::hasPassed()`) - Auslöser und Begründungstext (`TriggerDate`) +- Schleifenerkennung (`LoopDetector`) - Empfängerregel (`NotificationPolicy::shouldNotify()`) - Benachrichtigungsweg (`NotificationPolicy::sendsMail()`/`sendsNotification()`) diff --git a/README.md b/README.md index 6223285..86a2a70 100644 --- a/README.md +++ b/README.md @@ -36,6 +36,7 @@ Die Einstellungen liegen unter **Persönliche Einstellungen → Deck Workflow-Au - **Board:** Zur Auswahl stehen nur Boards, deren **Eigentümer** du bist. Geteilte, archivierte und gelöschte Boards erscheinen nicht. - **Quell- und Ziel-Stapel:** Das müssen zwei verschiedene Stapel desselben Boards sein. - **Karte verschieben, wenn:** das Fälligkeitsdatum überschritten oder das Startdatum erreicht ist. +- **Keine Schleifen:** Ein aktiver Workflow darf mit den anderen aktiven Workflows des Boards keinen Kreis bilden, etwa „A → B“ und „B → A“. Sonst würden Karten bei jedem Durchlauf hin- und hergeschoben. Beim Speichern wird das abgelehnt, mit den Namen der beteiligten Workflows. Das gilt unabhängig davon, ob die Workflows nach Fälligkeits- oder Startdatum verschieben. Ketten wie „A → B“ und „B → C“ sind erlaubt. - **Benachrichtigung bei verschobenen Karten** und **Benachrichtigen per:** siehe unten. - **Workflow aktiv:** Deaktivierte Workflows bleiben gespeichert, verschieben aber nichts. @@ -100,6 +101,7 @@ Deck schickt zusätzlich seine eigene Erinnerung, wenn eine Karte fällig wird. - **In der Übersicht** wird der Workflow rot markiert, zum Beispiel „Board nicht mehr vorhanden“ oder „Quell-Stapel nicht mehr vorhanden“. - **Beim nächsten Durchlauf** wird der Workflow automatisch deaktiviert. Der Eigentümer bekommt einmalig eine E-Mail und, wenn die App *Benachrichtigungen* aktiv ist, eine Nextcloud-Benachrichtigung. Das gilt unabhängig davon, was für verschobene Karten eingestellt ist. Danach kommt keine weitere Erinnerung. +- **Schleifen aus älteren Workflows** (gespeichert vor der Schleifenprüfung) werden in der Übersicht rot markiert und beim Durchlauf übersprungen, bis du einen der Workflows änderst oder deaktivierst. - **Archivierte Boards** deaktivieren den Workflow nicht. Sie lassen sich nur nicht mehr neu auswählen. - **Ist Deck vorübergehend nicht erreichbar**, passiert nichts: Der Workflow wird übersprungen und beim nächsten Durchlauf erneut versucht. diff --git a/lib/Controller/WorkflowController.php b/lib/Controller/WorkflowController.php index 4a9e148..58ad532 100644 --- a/lib/Controller/WorkflowController.php +++ b/lib/Controller/WorkflowController.php @@ -8,6 +8,7 @@ use OCA\WorkflowDeckAutomation\Db\Workflow; use OCA\WorkflowDeckAutomation\Db\WorkflowMapper; use OCA\WorkflowDeckAutomation\Service\DeckIntegrationService; use OCA\WorkflowDeckAutomation\Service\DeckUnavailableException; +use OCA\WorkflowDeckAutomation\Service\LoopDetector; use OCA\WorkflowDeckAutomation\Service\NotificationPolicy; use OCA\WorkflowDeckAutomation\Service\TriggerDate; use OCP\AppFramework\Db\DoesNotExistException; @@ -48,7 +49,17 @@ class WorkflowController extends OCSController { #[NoAdminRequired] public function index(): DataResponse { $workflows = $this->workflowMapper->findAllForBoards($this->ownedBoardIds()); - return new DataResponse(array_map(static fn (Workflow $w) => $w->jsonSerialize(), $workflows)); + + // validate() keeps new loops out, but workflows stored before that + // check can still form one; the runner skips them, the list says why. + $looping = LoopDetector::findLoopingWorkflows(LoopDetector::edgesOf( + array_filter($workflows, static fn (Workflow $w) => (bool)$w->getEnabled()), + )); + + return new DataResponse(array_map( + static fn (Workflow $w) => $w->jsonSerialize() + ['loop' => in_array((int)$w->getId(), $looping, true)], + $workflows, + )); } #[NoAdminRequired] @@ -62,7 +73,7 @@ class WorkflowController extends OCSController { string $notifyChannel = NotificationPolicy::CHANNEL_MAIL, bool $enabled = true, ): DataResponse { - $this->validate($title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel); + $this->validate(null, $title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel, $enabled); $workflow = new Workflow(); $workflow->setUserId($this->currentUserId()); @@ -85,7 +96,7 @@ class WorkflowController extends OCSController { bool $enabled = true, ): DataResponse { $workflow = $this->findOwnWorkflow($id); - $this->validate($title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel); + $this->validate($id, $title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel, $enabled); // Kept in step with the board owner so the job runs as, and mails, // the right user; validate() made sure the editor owns the board. @@ -161,7 +172,10 @@ class WorkflowController extends OCSController { } } - private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId, string $triggerDate, string $notifyMode, string $notifyChannel): void { + /** + * @param ?int $id the workflow being changed, `null` for a new one + */ + private function validate(?int $id, string $title, int $boardId, int $sourceStackId, int $targetStackId, string $triggerDate, string $notifyMode, string $notifyChannel, bool $enabled): void { if (trim($title) === '') { throw new OCSBadRequestException('Bitte einen Titel angeben.'); } @@ -208,6 +222,37 @@ class WorkflowController extends OCSController { if (!in_array($sourceStackId, $stackIds, true) || !in_array($targetStackId, $stackIds, true)) { throw new OCSBadRequestException('Der gewählte Stapel existiert auf diesem Board nicht.'); } + + // A disabled workflow moves nothing and can close no loop; it is + // checked again when it gets switched on, which also goes through here. + if ($enabled) { + $this->assertNoLoop($id, $boardId, $sourceStackId, $targetStackId); + } + } + + /** + * Rejects a workflow whose target stack leads back to its source through + * the board's other enabled workflows — cards would circle forever. See + * LoopDetector. + */ + private function assertNoLoop(?int $id, int $boardId, int $sourceStackId, int $targetStackId): void { + $others = []; + foreach ($this->workflowMapper->findAllForBoards([$boardId]) as $other) { + if ($other->getId() !== $id && $other->getEnabled()) { + $others[(int)$other->getId()] = $other; + } + } + + $path = LoopDetector::findPath(LoopDetector::edgesOf($others), $targetStackId, $sourceStackId); + if ($path === null) { + return; + } + + $titles = array_map(static fn (int $otherId) => '„' . $others[$otherId]->getTitle() . '“', $path); + throw new OCSBadRequestException(sprintf( + 'Zusammen mit %s würde dieser Workflow eine Schleife bilden, in der Karten immer wieder hin- und hergeschoben werden. Wähle einen anderen Ziel-Stapel oder deaktiviere einen der Workflows.', + implode(', ', $titles), + )); } private function applyFields( diff --git a/lib/Service/LoopDetector.php b/lib/Service/LoopDetector.php new file mode 100644 index 0000000..e6cbf51 --- /dev/null +++ b/lib/Service/LoopDetector.php @@ -0,0 +1,91 @@ + $workflows + * @return array + */ + public static function edgesOf(iterable $workflows): array { + $edges = []; + foreach ($workflows as $workflow) { + $edges[(int)$workflow->getId()] = [(int)$workflow->getSourceStackId(), (int)$workflow->getTargetStackId()]; + } + return $edges; + } + + /** + * The workflows leading from stack $from to stack $to, in order, or + * `null` if there is no way. `[]` if $from already is $to. + * + * @param array $edges workflow id => [source stack id, target stack id] + * @return list|null + */ + public static function findPath(array $edges, int $from, int $to): ?array { + if ($from === $to) { + return []; + } + + // Breadth-first, remembering how each stack was reached. + $reachedBy = [$from => null]; + $queue = [$from]; + while ($queue !== []) { + $stack = array_shift($queue); + foreach ($edges as $id => [$source, $target]) { + if ($source !== $stack || array_key_exists($target, $reachedBy)) { + continue; + } + $reachedBy[$target] = [$id, $stack]; + if ($target === $to) { + $path = []; + for ($at = $to; $reachedBy[$at] !== null; $at = $reachedBy[$at][1]) { + array_unshift($path, $reachedBy[$at][0]); + } + return $path; + } + $queue[] = $target; + } + } + + return null; + } + + /** + * Ids of the workflows that lie on a loop: those whose target leads back + * to their own source. + * + * @param array $edges workflow id => [source stack id, target stack id] + * @return list + */ + public static function findLoopingWorkflows(array $edges): array { + $looping = []; + foreach ($edges as $id => [$source, $target]) { + if (self::findPath($edges, $target, $source) !== null) { + $looping[] = $id; + } + } + return $looping; + } +} diff --git a/lib/Service/WorkflowRunner.php b/lib/Service/WorkflowRunner.php index 81d34a5..1c14696 100644 --- a/lib/Service/WorkflowRunner.php +++ b/lib/Service/WorkflowRunner.php @@ -67,11 +67,25 @@ class WorkflowRunner { return; } + // The controller refuses to store a loop, but workflows saved before + // that check — including ones whose source is their own target — can + // still form one. They would move the same cards on every run, with a + // notice each time, so they are left out until the user fixes them; + // the settings page marks them. + $looping = LoopDetector::findLoopingWorkflows(LoopDetector::edgesOf($workflows)); + $this->pendingNotices = []; $byUser = []; $owners = []; foreach ($workflows as $workflow) { $this->followBoardOwner($workflow, $owners); + if (in_array((int)$workflow->getId(), $looping, true)) { + $this->logger->warning('Skipping workflow {id}: it moves cards in a loop together with other workflows', [ + 'app' => 'workflow_deck_automation', + 'id' => $workflow->getId(), + ]); + continue; + } $byUser[$workflow->getUserId()][] = $workflow; } diff --git a/src/PersonalSettings.vue b/src/PersonalSettings.vue index f0135bc..7090f1e 100644 --- a/src/PersonalSettings.vue +++ b/src/PersonalSettings.vue @@ -278,8 +278,17 @@ const workflowIssues = computed(() => { } if (missing.length) { issues[workflow.id] = `${missing.join(' und ')} nicht mehr vorhanden` + continue } } + + // Set by the server for workflows stored before loops were refused; + // the background job skips them. + if (workflow.loop) { + issues[workflow.id] = workflow.sourceStackId === workflow.targetStackId + ? 'Quell- und Ziel-Stapel sind gleich, wird übersprungen' + : 'Bildet mit anderen Workflows eine Schleife, wird übersprungen' + } } return issues diff --git a/tests/Unit/Service/LoopDetectorTest.php b/tests/Unit/Service/LoopDetectorTest.php new file mode 100644 index 0000000..f09a837 --- /dev/null +++ b/tests/Unit/Service/LoopDetectorTest.php @@ -0,0 +1,76 @@ +assertSame([], LoopDetector::findLoopingWorkflows([])); + } + + public function testSameSourceAndTargetIsALoop(): void { + $this->assertSame([1], LoopDetector::findLoopingWorkflows([1 => [10, 10]])); + } + + public function testTwoWorkflowsBackAndForthAreALoop(): void { + $this->assertSame([1, 2], LoopDetector::findLoopingWorkflows([ + 1 => [10, 20], + 2 => [20, 10], + ])); + } + + public function testLongerCircleIsALoop(): void { + $this->assertSame([1, 2, 3], LoopDetector::findLoopingWorkflows([ + 1 => [10, 20], + 2 => [20, 30], + 3 => [30, 10], + ])); + } + + public function testChainIsNoLoop(): void { + $this->assertSame([], LoopDetector::findLoopingWorkflows([ + 1 => [10, 20], + 2 => [20, 30], + ])); + } + + public function testOnlyWorkflowsOnTheCircleAreReported(): void { + // 1 feeds into the circle 2 → 3 → 2 and 4 leads out of it; neither + // lies on it. + $this->assertSame([2, 3], LoopDetector::findLoopingWorkflows([ + 1 => [10, 20], + 2 => [20, 30], + 3 => [30, 20], + 4 => [30, 40], + ])); + } + + public function testTwoWorkflowsFromTheSameSourceAreNoLoop(): void { + $this->assertSame([], LoopDetector::findLoopingWorkflows([ + 1 => [10, 20], + 2 => [10, 30], + ])); + } + + public function testFindPathListsTheWorkflowsInOrder(): void { + $edges = [ + 7 => [30, 10], + 5 => [10, 20], + 6 => [20, 30], + ]; + $this->assertSame([5, 6], LoopDetector::findPath($edges, 10, 30)); + $this->assertSame([6, 7], LoopDetector::findPath($edges, 20, 10)); + } + + public function testFindPathWithoutConnectionIsNull(): void { + $this->assertNull(LoopDetector::findPath([1 => [10, 20]], 20, 10)); + } + + public function testFindPathToItselfIsEmpty(): void { + $this->assertSame([], LoopDetector::findPath([], 10, 10)); + } +}