diff --git a/CLAUDE.md b/CLAUDE.md index 9c74b1f..35ac0dd 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. -Die Fälligkeitslogik `WorkflowRunner::isOverdue()` ist bewusst `static` und seiteneffektfrei → unit-testbar (`tests/Unit/Service/WorkflowRunnerTest.php`), der einzige getestete Teil der App. Neue reine Logik in dieser Form halten. +Die Fälligkeitslogik `WorkflowRunner::isOverdue()` 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 @@ -34,7 +34,7 @@ Die Fälligkeitslogik `WorkflowRunner::isOverdue()` ist bewusst `static` und sei - **Nur positives Wissen deaktiviert.** `findStackIds()` liefert `null` nur bei „Board-Zeile weg“ (`DoesNotExistException`, Decks `NotFoundException`), „Board im Papierkorb“ (`deleted_at > 0`) oder „uid hat keine Leserechte“ – alles andere *wirft*, und der Runner überspringt den Workflow. Degradiert die Methode auf `[]`/`null`, deaktiviert ein Deck-Ausfall alle Workflows der Instanz und mailt alle Nutzer an. - **`findStackIds()` fragt nie einen Deck-Dienst, *wer* fragt.** Sie bekommt die uid als Argument und nutzt nur `BoardMapper`/`StackMapper` plus `PermissionService::getPermissions($boardId, $userId)` mit expliziter uid. Nicht zu `StackService::findAll($boardId)` „vereinfachen“ – genau das prüfte gegen die eingefrorene `null`-uid, warf `NoPermissionException`, wurde als „Board weg“ gelesen und verschickte Deaktivierungsmails für intakte Boards. - **`NoPermissionException` beweist nichts** und steht deshalb bewusst nicht in `isMissing()`. -- **Die Deaktivierungsmail ignoriert `notifyEmail`** (das betrifft verschobene Karten). Sie bleibt einmalig, weil `findAllEnabled()` deaktivierte Workflows nicht mehr lädt. +- **Die Deaktivierungsmail ignoriert `notifyMode`** (das betrifft verschobene Karten) und geht immer nur an den Owner. Sie bleibt einmalig, weil `findAllEnabled()` deaktivierte Workflows nicht mehr lädt. Archivierte Boards lösen das nicht aus (sie bleiben lesbar), werden aber in den Dropdowns ausgeblendet. @@ -49,6 +49,19 @@ Wird ein Board in Deck übertragen, wandern seine Workflows mit. `user_id` heißt also nur „als wer läuft der Job / wer bekommt Mail“. +## Mails über verschobene Karten folgen Decks Benachrichtigungseinstellung + +`notify_mode` (`NotificationPolicy::MODES`) ersetzt seit `Version1002…`/`Version1003…` das frühere `notify_email` (`true` → `owner`): + +- `off`: keine Mail. `owner`: nur der Board-Owner, ohne Rücksicht auf seine Deck-Einstellung (altes Verhalten). +- `board`: alle Board-Berechtigten, gefiltert nach deren Deck-Nutzereinstellung `deck` / `board::notify-due` (`off`/`assigned`/`all`, Default `assigned`). `NotificationPolicy::shouldNotify()` bildet Decks Regel aus `NotificationHelper::sendCardDuedate()` nach und ist reine, getestete Logik. Dazu gehört: Der Owner eines ungeteilten Boards wird außer bei `off` immer benachrichtigt. +- `DeckIntegrationService::findNotificationRecipients()` nutzt nur uid-explizite Bausteine: `PermissionService::findUsers($boardId, true)` (Owner + ACL, löst Gruppen/Circles auf, `true` umgeht den Prozess-Cache), `AssignmentMapper::isUserAssigned()` (auch Gruppen-/Circle-Zuweisungen) und `IConfig`. `FederatedUser` wird übersprungen. Bei jedem Fehler wirft die Methode, und der Runner verschickt dann **keine** Mail, statt an eine falsch aufgelöste Empfängerliste zu schicken. +- Der Runner sammelt verschobene Karten in `$pendingMails` und verschickt **nach** der Nutzer-Schleife (also ohne Impersonation) eine Sammelmail pro Empfänger und Lauf. Die Sprache kommt pro Empfänger aus `IFactory::getUserLanguage()`, weil ein injiziertes `IL10N` im Cron nur die Instanz-Standardsprache kennt. + +## Fälligkeit wird als UTC-Zeitpunkt verglichen + +Decks `duedate` ist eine `DATETIME`-Spalte 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. + ## Kartenfilter wurden entfernt Die früheren Filter (`filter_user_ids`/`filter_label_ids`) wurden auf Wunsch entfernt. `Version1001Date20260930120000` löscht die Spalten, und gefilterte Workflows laufen seitdem *ungefiltert* weiter (bewusste Entscheidung). Die Spalten müssen gelöscht bleiben: `Entity::fromRow()` ruft pro Spalte einen Setter auf, den `Workflow` nicht mehr hat. Mit entfernt wurden: Labels-/Participants-Endpunkte, `listLabels()`/`listParticipants()`/`findFilterOptions()` und `CardService::enrichCards()` in `getActiveCardsInStack()` (lief über `BoardService::find()` mit eingefrorener uid). Die Karten dort sind also **nicht angereichert** (keine `assignedUsers`/Labels). @@ -78,7 +91,7 @@ Hintergrundjob und persönliche Einstellungen stehen in `appinfo/info.xml` (`:notify-due`. ### Wichtiger Hinweis zu Decks internen Klassen @@ -42,7 +47,7 @@ Ein Workflow verweist per ID auf ein Board und zwei Stapel. Wird eines davon in - **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. +- **Im Hintergrundjob** wird der Workflow automatisch deaktiviert und der Besitzer einmalig per E-Mail informiert — unabhängig von der E-Mail-Einstellung des Workflows, 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. @@ -129,7 +134,7 @@ In [`.gitea/workflows/`](.gitea/workflows/): ## Datenmodell -Workflows werden in der Tabelle `wfda_workflows` gespeichert (siehe [`lib/Migration/Version1000Date20260813120000.php`](lib/Migration/Version1000Date20260813120000.php)): eine Zeile pro Workflow, mit Board-/Stapel-IDs, `user_id` (der Board-Eigentümer, in dessen Namen der Job läuft — wird bei jedem Lauf aus Deck aktualisiert) sowie `enabled`/`notify_email`/`last_run`. Die früheren Filter-Spalten `filter_user_ids`/`filter_label_ids` entfernt [`Version1001Date20260930120000`](lib/Migration/Version1001Date20260930120000.php). +Workflows werden in der Tabelle `wfda_workflows` gespeichert (siehe [`lib/Migration/Version1000Date20260813120000.php`](lib/Migration/Version1000Date20260813120000.php)): eine Zeile pro Workflow, mit Board-/Stapel-IDs, `user_id` (der Board-Eigentümer, in dessen Namen der Job läuft — wird bei jedem Lauf aus Deck aktualisiert) sowie `enabled`/`notify_mode`/`last_run`. Die früheren Filter-Spalten `filter_user_ids`/`filter_label_ids` entfernt [`Version1001Date20260930120000`](lib/Migration/Version1001Date20260930120000.php). [`Version1002Date20261001120000`](lib/Migration/Version1002Date20261001120000.php) ersetzt das frühere Ja/Nein-Feld `notify_email` durch `notify_mode` (`off`/`owner`/`board`; gesetztes Häkchen → `owner`), [`Version1003Date20261001120100`](lib/Migration/Version1003Date20261001120100.php) entfernt die alte Spalte. ## Lizenz diff --git a/appinfo/info.xml b/appinfo/info.xml index c9910c5..de091c8 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -24,7 +24,7 @@ This app lets every user configure Deck automation workflows for the boards they A background job periodically evaluates all enabled workflows and moves cards whose due date has passed, using Deck's own internal PHP classes only (no HTTP/OCS calls). ]]> - 35.1.0 + 35.2.0 agpl Patrick Niebeling WorkflowDeckAutomation diff --git a/lib/Controller/WorkflowController.php b/lib/Controller/WorkflowController.php index 88febcd..e67a3d3 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\NotificationPolicy; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; @@ -55,14 +56,14 @@ class WorkflowController extends OCSController { int $boardId, int $sourceStackId, int $targetStackId, - bool $notifyEmail = false, + string $notifyMode = NotificationPolicy::MODE_OFF, bool $enabled = true, ): DataResponse { - $this->validate($title, $boardId, $sourceStackId, $targetStackId); + $this->validate($title, $boardId, $sourceStackId, $targetStackId, $notifyMode); $workflow = new Workflow(); $workflow->setUserId($this->currentUserId()); - $this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $notifyEmail, $enabled); + $this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $notifyMode, $enabled); $workflow = $this->workflowMapper->insert($workflow); return new DataResponse($workflow->jsonSerialize(), Http::STATUS_CREATED); @@ -75,16 +76,16 @@ class WorkflowController extends OCSController { int $boardId, int $sourceStackId, int $targetStackId, - bool $notifyEmail = false, + string $notifyMode = NotificationPolicy::MODE_OFF, bool $enabled = true, ): DataResponse { $workflow = $this->findOwnWorkflow($id); - $this->validate($title, $boardId, $sourceStackId, $targetStackId); + $this->validate($title, $boardId, $sourceStackId, $targetStackId, $notifyMode); // 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. $workflow->setUserId($this->currentUserId()); - $this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $notifyEmail, $enabled); + $this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $notifyMode, $enabled); $workflow = $this->workflowMapper->update($workflow); return new DataResponse($workflow->jsonSerialize()); @@ -155,13 +156,16 @@ class WorkflowController extends OCSController { } } - private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId): void { + private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId, string $notifyMode): void { if (trim($title) === '') { throw new OCSBadRequestException('Title must not be empty'); } if ($sourceStackId === $targetStackId) { throw new OCSBadRequestException('Source and target stack must differ'); } + if (!NotificationPolicy::isValidMode($notifyMode)) { + throw new OCSBadRequestException('Notification mode must be one of: ' . implode(', ', NotificationPolicy::MODES)); + } // 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. @@ -198,14 +202,14 @@ class WorkflowController extends OCSController { int $boardId, int $sourceStackId, int $targetStackId, - bool $notifyEmail, + string $notifyMode, bool $enabled, ): void { $workflow->setTitle($title); $workflow->setBoardId($boardId); $workflow->setSourceStackId($sourceStackId); $workflow->setTargetStackId($targetStackId); - $workflow->setNotifyEmail($notifyEmail); + $workflow->setNotifyMode($notifyMode); $workflow->setEnabled($enabled); } } diff --git a/lib/Db/Workflow.php b/lib/Db/Workflow.php index d98af13..9aa71db 100644 --- a/lib/Db/Workflow.php +++ b/lib/Db/Workflow.php @@ -18,8 +18,8 @@ use OCP\DB\Types; * @method void setSourceStackId(int $sourceStackId) * @method int getTargetStackId() * @method void setTargetStackId(int $targetStackId) - * @method bool getNotifyEmail() - * @method void setNotifyEmail(bool $notifyEmail) + * @method string getNotifyMode() + * @method void setNotifyMode(string $notifyMode) * @method bool getEnabled() * @method void setEnabled(bool $enabled) * @method \DateTime|null getLastRun() @@ -31,7 +31,7 @@ class Workflow extends Entity implements \JsonSerializable { protected $boardId; protected $sourceStackId; protected $targetStackId; - protected $notifyEmail; + protected $notifyMode; protected $enabled; protected $lastRun; @@ -42,7 +42,7 @@ class Workflow extends Entity implements \JsonSerializable { $this->addType('boardId', Types::INTEGER); $this->addType('sourceStackId', Types::INTEGER); $this->addType('targetStackId', Types::INTEGER); - $this->addType('notifyEmail', Types::BOOLEAN); + $this->addType('notifyMode', Types::STRING); $this->addType('enabled', Types::BOOLEAN); $this->addType('lastRun', Types::DATETIME); } @@ -55,7 +55,7 @@ class Workflow extends Entity implements \JsonSerializable { 'boardId' => $this->getBoardId(), 'sourceStackId' => $this->getSourceStackId(), 'targetStackId' => $this->getTargetStackId(), - 'notifyEmail' => (bool)$this->getNotifyEmail(), + 'notifyMode' => (string)$this->getNotifyMode(), 'enabled' => (bool)$this->getEnabled(), 'lastRun' => $this->getLastRun()?->format(\DateTimeInterface::ATOM), ]; diff --git a/lib/Migration/Version1002Date20261001120000.php b/lib/Migration/Version1002Date20261001120000.php new file mode 100644 index 0000000..34aae88 --- /dev/null +++ b/lib/Migration/Version1002Date20261001120000.php @@ -0,0 +1,64 @@ +hasTable('wfda_workflows')) { + return null; + } + + $table = $schema->getTable('wfda_workflows'); + if ($table->hasColumn('notify_mode')) { + return null; + } + + $table->addColumn('notify_mode', Types::STRING, [ + 'notnull' => true, + 'length' => 16, + 'default' => 'off', + ]); + + return $schema; + } + + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + if (!$schema->hasTable('wfda_workflows') || !$schema->getTable('wfda_workflows')->hasColumn('notify_email')) { + return; + } + + $qb = $this->db->getQueryBuilder(); + $qb->update('wfda_workflows') + ->set('notify_mode', $qb->createNamedParameter('owner')) + ->where($qb->expr()->eq('notify_email', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))); + $qb->executeStatement(); + } +} diff --git a/lib/Migration/Version1003Date20261001120100.php b/lib/Migration/Version1003Date20261001120100.php new file mode 100644 index 0000000..11a6ac2 --- /dev/null +++ b/lib/Migration/Version1003Date20261001120100.php @@ -0,0 +1,34 @@ +hasTable('wfda_workflows')) { + return null; + } + + $table = $schema->getTable('wfda_workflows'); + if (!$table->hasColumn('notify_email')) { + return null; + } + + $table->dropColumn('notify_email'); + return $schema; + } +} diff --git a/lib/Service/DeckIntegrationService.php b/lib/Service/DeckIntegrationService.php index 6a0ac94..7b5e860 100644 --- a/lib/Service/DeckIntegrationService.php +++ b/lib/Service/DeckIntegrationService.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace OCA\WorkflowDeckAutomation\Service; +use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\Card; use OCA\Deck\Db\CardMapper; @@ -14,6 +15,7 @@ use OCA\Deck\Service\PermissionService; use OCA\Deck\Service\StackService; use OCP\App\IAppManager; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IConfig; use OCP\IUser; use OCP\Server; use Psr\Log\LoggerInterface; @@ -66,6 +68,7 @@ class DeckIntegrationService { public function __construct( private IAppManager $appManager, + private IConfig $config, private LoggerInterface $logger, ) { } @@ -302,6 +305,81 @@ class DeckIntegrationService { return $owner; } + /** + * Who should hear about each of the given (moved) cards, according to + * every board member's own Deck notification setting for this board — + * the "Notifications: none / assigned cards / all cards" menu, stored as + * the user preference `deck` / `board::notify-due`. The decision + * itself is NotificationPolicy::shouldNotify(), a copy of Deck's rules. + * + * Like findStackIds(), assembled only from pieces that take the users + * explicitly and never consult Deck's frozen `$userId`: + * PermissionService::findUsers() reads the owner and ACL through mappers + * and expands groups and circles itself; AssignmentMapper is a plain + * mapper that also resolves group/circle assignments; the preference is + * read straight from IConfig. + * + * Throws on any failure, so the caller can log and skip the mail instead + * of mailing a half-resolved or wrong set of people. + * + * @param int[] $cardIds + * @return array> card id => uids to notify + * @throws DeckUnavailableException + */ + public function findNotificationRecipients(int $boardId, array $cardIds): array { + try { + $board = $this->resolve(BoardMapper::class)->find($boardId, false, true); + $owner = $this->unwrapUid($board->getOwner()); + // Deck notifies the owner of a board nobody else can see about + // every card: "assigned" means nothing there. + $unshared = count($board->getAcl() ?? []) === 0; + + // `true` bypasses PermissionService's per-process cache, which + // would otherwise outlive ACL changes for the whole cron pass. + $participants = $this->resolve(PermissionService::class)->findUsers($boardId, true); + $assignments = $this->resolve(AssignmentMapper::class); + + $settings = []; + foreach ($participants as $uid => $participant) { + // Users of other instances (FederatedUser) have no local + // account and no preference to read. + if (!is_a($participant, 'OCA\Deck\Db\User')) { + continue; + } + // Numeric uids come back as integer array keys. + $uid = (string)$uid; + $settings[$uid] = (string)$this->config->getUserValue( + $uid, + self::DECK_APP_ID, + 'board:' . $boardId . ':notify-due', + NotificationPolicy::DECK_DEFAULT, + ); + } + + $recipients = []; + foreach ($cardIds as $cardId) { + $recipients[$cardId] = []; + foreach ($settings as $uid => $setting) { + $uid = (string)$uid; + $notify = NotificationPolicy::shouldNotify( + $setting, + $unshared && $uid === $owner, + static fn (): bool => (bool)$assignments->isUserAssigned($cardId, $uid), + ); + if ($notify) { + $recipients[$cardId][] = $uid; + } + } + } + } catch (DeckUnavailableException $e) { + throw $e; + } catch (Throwable $e) { + throw new DeckUnavailableException('Could not resolve notification recipients for board ' . $boardId . ': ' . $e->getMessage(), 0, $e); + } + + return $recipients; + } + /** * 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 @@ -368,9 +446,10 @@ class DeckIntegrationService { public function getActiveCardsInStack(int $stackId): array { return $this->call(function () use ($stackId) { // Deliberately not enriched: CardService::enrichCards() only adds - // labels, assignees and comment counts — none of which is used — - // and runs BoardService::find(), a permission check against - // Deck's frozen uid (see beginUserContext()). + // labels, assignees and comment counts, and runs + // BoardService::find(), a permission check against Deck's frozen + // uid (see beginUserContext()). Assignments, where needed for + // mails, come from findNotificationRecipients() instead. $cards = $this->resolve(CardMapper::class)->findAllByStack($stackId); return array_values(array_filter($cards, static function (Card $card) { diff --git a/lib/Service/NotificationMailer.php b/lib/Service/NotificationMailer.php index 9144c2f..050728a 100644 --- a/lib/Service/NotificationMailer.php +++ b/lib/Service/NotificationMailer.php @@ -9,6 +9,7 @@ use OCA\WorkflowDeckAutomation\Db\Workflow; use OCP\IL10N; use OCP\IURLGenerator; use OCP\IUser; +use OCP\L10N\IFactory; use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; use OCP\Util; @@ -16,6 +17,8 @@ use Psr\Log\LoggerInterface; use Throwable; class NotificationMailer { + private const APP_ID = 'workflow_deck_automation'; + /** * Descriptions are unbounded in Deck; keep the mail readable and well * clear of any MTA size limits. @@ -25,70 +28,88 @@ class NotificationMailer { public function __construct( private IMailer $mailer, private IURLGenerator $urlGenerator, - private IL10N $l10n, + private IFactory $l10nFactory, private LoggerInterface $logger, ) { } /** - * @param array{board: string, sourceStack: string, targetStack: string} $targets - * titles resolved by DeckIntegrationService::describeTargets() + * One mail listing every card moved for $recipient in this run — they + * may come from several workflows and boards. + * + * @param non-empty-list $entries + * targets are the titles resolved by DeckIntegrationService::describeTargets() */ - public function sendCardMovedNotification(IUser $user, Workflow $workflow, Card $card, array $targets): void { - $email = $user->getEMailAddress(); + public function sendCardsMovedNotification(IUser $recipient, array $entries): void { + $email = $recipient->getEMailAddress(); if ($email === null || $email === '') { - $this->logger->info('Skipping notification for workflow ' . $workflow->getId() . ': user ' . $user->getUID() . ' has no email address', [ - 'app' => 'workflow_deck_automation', + $this->logger->info('Skipping moved-cards mail: user ' . $recipient->getUID() . ' has no email address', [ + 'app' => self::APP_ID, ]); return; } - try { - $cardLink = $this->urlGenerator->getAbsoluteURL( - '/apps/deck/board/' . $workflow->getBoardId() . '/card/' . $card->getId(), - ); + $count = count($entries); + $l10n = $this->l10nFor($recipient); - $template = $this->mailer->createEMailTemplate('workflow_deck_automation.CardMoved', [ - 'cardTitle' => $card->getTitle(), - 'workflowTitle' => $workflow->getTitle(), + try { + $template = $this->mailer->createEMailTemplate(self::APP_ID . '.CardsMoved', [ + 'count' => $count, + 'cardTitles' => array_map(static fn (array $entry) => $entry['card']->getTitle(), $entries), ]); - $template->setSubject($this->l10n->t('Deck card moved: %s', [$card->getTitle()])); + $template->setSubject($count === 1 + ? $l10n->t('Deck card moved: %s', [$entries[0]['card']->getTitle()]) + : $l10n->n('%n Deck card moved', '%n Deck cards moved', $count)); $template->addHeader(); - $template->addHeading($this->l10n->t('A card was moved automatically'), false); - $template->addBodyText($this->l10n->t( - 'The card "%1$s" was moved because it is overdue (workflow "%2$s").', - [$card->getTitle(), $workflow->getTitle()], - )); - // Single-argument addBodyText() runs htmlspecialchars() itself, so - // these board- and stack-titles (user input) are safe as-is — - // unlike the description below, which passes both parts. - $template->addBodyText($this->l10n->t( - 'Board "%1$s": moved from "%2$s" to "%3$s".', - [$targets['board'], $targets['sourceStack'], $targets['targetStack']], - )); - $this->addCardDescription($template, $card); - $template->addBodyButton($this->l10n->t('Open card'), $cardLink); + $template->addHeading($l10n->n('A card was moved automatically', '%n cards were moved automatically', $count), false); + + foreach ($entries as $entry) { + $this->addCard($template, $l10n, $entry['workflow'], $entry['card'], $entry['targets']); + } + $template->addFooter(); $message = $this->mailer->createMessage(); - $message->setTo([$email => $user->getDisplayName()]); - $message->setFrom([Util::getDefaultEmailAddress('noreply') => $this->l10n->t('Deck Workflow Automation')]); + $message->setTo([$email => $recipient->getDisplayName()]); + $message->setFrom([Util::getDefaultEmailAddress('noreply') => $l10n->t('Deck Workflow Automation')]); $message->useTemplate($template); $failedRecipients = $this->mailer->send($message); if (!empty($failedRecipients)) { - $this->logger->error('Notification mail for card ' . $card->getId() . ' failed for: ' . implode(', ', $failedRecipients), [ - 'app' => 'workflow_deck_automation', + $this->logger->error('Moved-cards mail failed for: ' . implode(', ', $failedRecipients), [ + 'app' => self::APP_ID, ]); } } catch (Throwable $e) { - $this->logger->error('Could not send notification mail for card ' . $card->getId() . ': ' . $e->getMessage(), [ - 'app' => 'workflow_deck_automation', + $this->logger->error('Could not send moved-cards mail to ' . $recipient->getUID() . ': ' . $e->getMessage(), [ + 'app' => self::APP_ID, 'exception' => $e, ]); } } + /** + * @param array{board: string, sourceStack: string, targetStack: string} $targets + */ + private function addCard(IEMailTemplate $template, IL10N $l10n, Workflow $workflow, Card $card, array $targets): void { + $template->addBodyText($l10n->t( + 'The card "%1$s" was moved because it is overdue (workflow "%2$s").', + [$card->getTitle(), $workflow->getTitle()], + )); + // Single-argument addBodyText() runs htmlspecialchars() itself, so + // these board- and stack-titles (user input) are safe as-is — + // unlike the description below, which passes both parts. + $template->addBodyText($l10n->t( + 'Board "%1$s": moved from "%2$s" to "%3$s".', + [$targets['board'], $targets['sourceStack'], $targets['targetStack']], + )); + $this->addCardDescription($template, $l10n, $card); + $template->addBodyButton( + $l10n->t('Open card'), + $this->urlGenerator->getAbsoluteURL('/apps/deck/board/' . $workflow->getBoardId() . '/card/' . $card->getId()), + ); + } + /** * One-off notice that a workflow was switched off because the board or * stack it points at is gone. @@ -99,55 +120,65 @@ class NotificationMailer { $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', + 'app' => self::APP_ID, ]); return; } + $l10n = $this->l10nFor($user); $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'), + WorkflowRunner::TARGET_SOURCE_STACK => $l10n->t('its source stack no longer exists'), + WorkflowRunner::TARGET_TARGET_STACK => $l10n->t('its target stack no longer exists'), + default => $l10n->t('its board no longer exists or is no longer available to you'), }; try { - $settingsLink = $this->urlGenerator->getAbsoluteURL('/settings/user/workflow_deck_automation'); + $settingsLink = $this->urlGenerator->getAbsoluteURL('/settings/user/' . self::APP_ID); - $template = $this->mailer->createEMailTemplate('workflow_deck_automation.WorkflowDisabled', [ + $template = $this->mailer->createEMailTemplate(self::APP_ID . '.WorkflowDisabled', [ 'workflowTitle' => $workflow->getTitle(), ]); - $template->setSubject($this->l10n->t('Deck workflow deactivated: %s', [$workflow->getTitle()])); + $template->setSubject($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( + $template->addHeading($l10n->t('A workflow was deactivated'), false); + $template->addBodyText($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( + $template->addBodyText($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->addBodyButton($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->setFrom([Util::getDefaultEmailAddress('noreply') => $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', + 'app' => self::APP_ID, ]); } } catch (Throwable $e) { $this->logger->error('Could not send deactivation mail for workflow ' . $workflow->getId() . ': ' . $e->getMessage(), [ - 'app' => 'workflow_deck_automation', + 'app' => self::APP_ID, 'exception' => $e, ]); } } + /** + * The recipient's own language. Cron has no request to take one from, so + * an injected IL10N would always speak the instance default — wrong as + * soon as a mail goes to someone other than whoever set that default. + */ + private function l10nFor(IUser $user): IL10N { + return $this->l10nFactory->get(self::APP_ID, $this->l10nFactory->getUserLanguage($user)); + } + /** * Appends the card's description, if it has one. * @@ -155,7 +186,7 @@ class NotificationMailer { * rendered: pulling a Markdown parser in just for the mail would be a * lot of machinery, and unrendered Markdown still reads fine. */ - private function addCardDescription(IEMailTemplate $template, Card $card): void { + private function addCardDescription(IEMailTemplate $template, IL10N $l10n, Card $card): void { $description = trim((string)$card->getDescription()); if ($description === '') { return; @@ -165,7 +196,7 @@ class NotificationMailer { $description = mb_substr($description, 0, self::MAX_DESCRIPTION_LENGTH) . ' […]'; } - $template->addBodyText($this->l10n->t('Card content:')); + $template->addBodyText($l10n->t('Card content:')); // addBodyText() only runs htmlspecialchars() when it has to build the // plain-text part itself. Passing both parts means escaping is on us diff --git a/lib/Service/NotificationPolicy.php b/lib/Service/NotificationPolicy.php new file mode 100644 index 0000000..b6a3cc0 --- /dev/null +++ b/lib/Service/NotificationPolicy.php @@ -0,0 +1,70 @@ +:notify-due` (Deck's + * ConfigService::SETTING_BOARD_NOTIFICATION_DUE_*), inlined so a renamed + * constant cannot fatal this app. It is the board's "Notifications" menu + * in Deck — the due-date reminder setting, which is exactly what fits a + * card being moved because it fell due. + */ + public const DECK_OFF = 'off'; + public const DECK_ASSIGNED = 'assigned'; + public const DECK_ALL = 'all'; + /** What Deck assumes for a user who never touched the setting. */ + public const DECK_DEFAULT = self::DECK_ASSIGNED; + + public static function isValidMode(string $mode): bool { + return in_array($mode, self::MODES, true); + } + + /** + * Mirrors Deck's own decision in NotificationHelper::sendCardDuedate(): + * + * - `off` never notifies — not even the owner of an unshared board; + * - `all` always notifies; + * - the owner of an unshared board is notified for every card, since on a + * board nobody else can see "assigned" would otherwise mean nothing; + * - `assigned` notifies users assigned to the card, directly or through a + * group or circle. + * + * An unknown value is treated as Deck's default. Deck rejects anything + * else when storing the setting, so that only happens if the preference + * was edited by hand. + * + * @param bool|callable(): bool $isAssigned a callable is only invoked when + * the answer actually depends on it — it costs a query per user + */ + public static function shouldNotify(string $deckSetting, bool $isOwnerOfUnsharedBoard, bool|callable $isAssigned): bool { + if (!in_array($deckSetting, [self::DECK_OFF, self::DECK_ASSIGNED, self::DECK_ALL], true)) { + $deckSetting = self::DECK_DEFAULT; + } + + if ($deckSetting === self::DECK_OFF) { + return false; + } + if ($deckSetting === self::DECK_ALL || $isOwnerOfUnsharedBoard) { + return true; + } + + return is_callable($isAssigned) ? (bool)$isAssigned() : $isAssigned; + } +} diff --git a/lib/Service/WorkflowRunner.php b/lib/Service/WorkflowRunner.php index 284f638..f437057 100644 --- a/lib/Service/WorkflowRunner.php +++ b/lib/Service/WorkflowRunner.php @@ -38,6 +38,13 @@ class WorkflowRunner { private ?IUser $impersonationRestore = null; + /** + * Moved cards per recipient uid, collected over the whole run. + * + * @var array> + */ + private array $pendingMails = []; + public function __construct( private WorkflowMapper $workflowMapper, private DeckIntegrationService $deckService, @@ -56,6 +63,7 @@ class WorkflowRunner { return; } + $this->pendingMails = []; $byUser = []; $owners = []; foreach ($workflows as $workflow) { @@ -80,6 +88,8 @@ class WorkflowRunner { ]); } } + + $this->sendPendingMails(); } /** @@ -211,10 +221,7 @@ class WorkflowRunner { return; } - // Resolved at most once per run, and only if a card actually moves - // and the workflow wants a mail — this is decoration, not a check. - $targets = null; - + $moved = []; $now = $this->timeFactory->getDateTime(); foreach ($cards as $card) { @@ -230,22 +237,96 @@ class WorkflowRunner { 'target' => $workflow->getTargetStackId(), ]); - if (!$this->moveCard($workflow, $card)) { - continue; - } - - if ($workflow->getNotifyEmail()) { - $targets ??= $this->deckService->describeTargets( - $workflow->getBoardId(), - $workflow->getSourceStackId(), - $workflow->getTargetStackId(), - ); - $this->mailer->sendCardMovedNotification($user, $workflow, $card, $targets); + if ($this->moveCard($workflow, $card)) { + $moved[] = $card; } } $workflow->setLastRun($this->timeFactory->getDateTime()); $this->workflowMapper->update($workflow); + + if ($moved !== []) { + $this->queueMails($user, $workflow, $moved); + } + } + + /** + * Works out who hears about the moved cards and parks them in + * $pendingMails; sendPendingMails() delivers them once per recipient at + * the end of the run. + * + * Mails are decoration, not part of the move: nothing in here may fail + * the workflow, and if Deck cannot say who wants a mail, nobody gets one + * rather than people who asked Deck for silence. + * + * @param Card[] $cards + */ + private function queueMails(IUser $owner, Workflow $workflow, array $cards): void { + $cardIds = array_map(static fn (Card $card): int => (int)$card->getId(), $cards); + + switch ($workflow->getNotifyMode()) { + case NotificationPolicy::MODE_OWNER: + $recipients = array_fill_keys($cardIds, [$owner->getUID()]); + break; + case NotificationPolicy::MODE_BOARD: + try { + $recipients = $this->deckService->findNotificationRecipients($workflow->getBoardId(), $cardIds); + } catch (DeckUnavailableException $e) { + $this->logger->warning('No mails for workflow {id}: ' . $e->getMessage(), [ + 'app' => 'workflow_deck_automation', + 'id' => $workflow->getId(), + ]); + return; + } + break; + default: + return; + } + + $targets = $this->deckService->describeTargets( + $workflow->getBoardId(), + $workflow->getSourceStackId(), + $workflow->getTargetStackId(), + ); + + foreach ($cards as $card) { + foreach ($recipients[(int)$card->getId()] ?? [] as $uid) { + $this->pendingMails[$uid][] = [ + 'workflow' => $workflow, + 'card' => $card, + 'targets' => $targets, + ]; + } + } + } + + /** + * One mail per recipient and run, however many cards and workflows it + * covers — a board with twenty overdue cards and fifteen members would + * otherwise send three hundred mails at once. + * + * Runs after the per-user loop, so no impersonation is active any more. + */ + private function sendPendingMails(): void { + $pending = $this->pendingMails; + $this->pendingMails = []; + + foreach ($pending as $uid => $entries) { + // Same reasoning as in run(): nothing may escape. + try { + $recipient = $this->userManager->get((string)$uid); + if ($recipient === null || !$recipient->isEnabled()) { + continue; + } + $this->mailer->sendCardsMovedNotification($recipient, $entries); + } catch (Throwable $e) { + $this->logger->error('Could not send the moved-cards mail to {user}: ' . $e->getMessage(), [ + 'app' => 'workflow_deck_automation', + 'user' => $uid, + 'exception' => $e, + ]); + } + } } /** @@ -289,7 +370,7 @@ class WorkflowRunner { 'target' => $brokenTarget, ]); - // Sent regardless of the workflow's notifyEmail setting: that flag is + // Sent to the owner regardless of the workflow's notifyMode: that 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. diff --git a/package.json b/package.json index 291c706..d617ced 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "workflow_deck_automation", - "version": "35.1.0", + "version": "35.2.0", "private": true, "type": "module", "description": "Automates moving overdue Deck cards between stacks, configurable per user.", diff --git a/src/PersonalSettings.vue b/src/PersonalSettings.vue index f19154f..b20d00c 100644 --- a/src/PersonalSettings.vue +++ b/src/PersonalSettings.vue @@ -34,7 +34,7 @@ {{ stackLabel(workflow.boardId, workflow.sourceStackId) }} {{ stackLabel(workflow.boardId, workflow.targetStackId) }} - {{ yesNo(workflow.notifyEmail) }} + {{ notifyLabel(workflow.notifyMode) }} {{ yesNo(workflow.enabled) }} @@ -88,9 +88,24 @@ :disabled="!form.boardId" placeholder="Ziel-Stapel wählen" /> - - Benachrichtigungs-E-Mail an mich senden - +
+ E-Mail bei verschobenen Karten + + {{ option.label }} + +

+ Jedes Board-Mitglied bekommt die E-Mail entsprechend seiner eigenen + Deck-Einstellung „Benachrichtigungen“ für dieses Board: bei „Alle Karten“ immer, + bei „Zugewiesene Karten“ nur für Karten, denen es zugewiesen ist, bei „Keine“ nie. + Mehrere Karten eines Laufs werden zu einer E-Mail zusammengefasst. +

+
Workflow aktiv @@ -166,7 +181,7 @@ const sortedWorkflows = computed(() => { workflow.title, stackLabel(workflow.boardId, workflow.sourceStackId), stackLabel(workflow.boardId, workflow.targetStackId), - yesNo(workflow.notifyEmail), + notifyLabel(workflow.notifyMode), yesNo(workflow.enabled), ] return workflows.value @@ -229,11 +244,22 @@ function emptyForm() { boardId: null, sourceStackId: null, targetStackId: null, - notifyEmail: false, + notifyMode: 'off', enabled: true, } } +// Values match NotificationPolicy::MODES on the server. +const notifyOptions = [ + { value: 'off', label: 'Keine E-Mail', short: 'Keine' }, + { value: 'owner', label: 'Nur an mich', short: 'Nur ich' }, + { value: 'board', label: 'An Board-Mitglieder laut ihrer Deck-Einstellung', short: 'Board-Mitglieder' }, +] + +function notifyLabel(mode) { + return notifyOptions.find((option) => option.value === mode)?.short ?? mode +} + function boardLabel(boardId) { const board = boards.value.find((entry) => entry.id === boardId) return board ? board.title : `#${boardId}` @@ -329,7 +355,7 @@ async function editWorkflow(workflow) { boardId: workflow.boardId, sourceStackId: workflow.sourceStackId, targetStackId: workflow.targetStackId, - notifyEmail: workflow.notifyEmail, + notifyMode: workflow.notifyMode, enabled: workflow.enabled, }) editingId.value = workflow.id @@ -437,6 +463,22 @@ onMounted(loadAll) border-radius: var(--border-radius-large); } +.wfda-notify { + display: flex; + flex-direction: column; + gap: 4px; +} + +.wfda-notify legend { + font-weight: bold; + margin-bottom: 4px; +} + +.wfda-hint { + color: var(--color-text-maxcontrast); + font-size: 0.9em; +} + .wfda-form-actions { display: flex; gap: 8px; diff --git a/tests/Unit/Service/NotificationPolicyTest.php b/tests/Unit/Service/NotificationPolicyTest.php new file mode 100644 index 0000000..bed40fb --- /dev/null +++ b/tests/Unit/Service/NotificationPolicyTest.php @@ -0,0 +1,56 @@ +assertFalse(NotificationPolicy::shouldNotify('off', false, true)); + // Not even the owner of an unshared board — "off" is "off" in Deck too. + $this->assertFalse(NotificationPolicy::shouldNotify('off', true, true)); + } + + public function testAllAlwaysNotifies(): void { + $this->assertTrue(NotificationPolicy::shouldNotify('all', false, false)); + $this->assertTrue(NotificationPolicy::shouldNotify('all', true, false)); + } + + public function testAssignedNotifiesOnlyAssignedUsers(): void { + $this->assertTrue(NotificationPolicy::shouldNotify('assigned', false, true)); + $this->assertFalse(NotificationPolicy::shouldNotify('assigned', false, false)); + } + + public function testOwnerOfUnsharedBoardIsNotifiedWithAssigned(): void { + $this->assertTrue(NotificationPolicy::shouldNotify('assigned', true, false)); + } + + public function testUnknownSettingFallsBackToDeckDefault(): void { + $this->assertTrue(NotificationPolicy::shouldNotify('', false, true)); + $this->assertFalse(NotificationPolicy::shouldNotify('bogus', false, false)); + $this->assertTrue(NotificationPolicy::shouldNotify('bogus', true, false)); + } + + public function testAssignmentIsOnlyLookedUpWhenItMatters(): void { + $lookup = function (): bool { + $this->fail('assignment lookup must not run'); + }; + $this->assertFalse(NotificationPolicy::shouldNotify('off', false, $lookup)); + $this->assertTrue(NotificationPolicy::shouldNotify('all', false, $lookup)); + $this->assertTrue(NotificationPolicy::shouldNotify('assigned', true, $lookup)); + + $this->assertTrue(NotificationPolicy::shouldNotify('assigned', false, static fn (): bool => true)); + $this->assertFalse(NotificationPolicy::shouldNotify('assigned', false, static fn (): bool => false)); + } + + public function testModes(): void { + foreach (['off', 'owner', 'board'] as $mode) { + $this->assertTrue(NotificationPolicy::isValidMode($mode)); + } + $this->assertFalse(NotificationPolicy::isValidMode('')); + $this->assertFalse(NotificationPolicy::isValidMode('all')); + } +}