Surface dead filters, and name board and stacks in the moved-card mail
Build package / package (push) Successful in 55s
Build package / php-lint (8.2) (push) Successful in 41s
Build package / php-lint (8.3) (push) Successful in 34s
Build package / php-lint (8.4) (push) Successful in 40s
Build package / xml-lint (push) Successful in 11s
Build package / unit-tests (push) Successful in 42s

A filter whose entries have all been deleted from the board was the one
failure mode of this app that was completely invisible: the workflow kept
running, kept updating last_run and matched nothing, forever. It happens
with deleted labels and just as easily when a filtered user loses board
access, because Deck's BoardService::deleteAcl() calls
assignedUsersMapper->deleteByParticipantOnBoard() and wipes that user's
card assignments.

Two low-stakes signals, no new column and no migration:

- WorkflowRunner::warnAboutDeadFilters() logs a warning on every run.
- workflowIssues in PersonalSettings.vue marks the row red.

Deliberately no mail and no `enabled = false`. One deleted label out of
three is harmless, and even a fully dead filter can be one board edit away
from being live again.

Both obey the rule the disable path already follows: only positive
knowledge. DeckIntegrationService::findFilterOptions() therefore throws
instead of degrading to [], unlike listLabels()/listParticipants(), where
an empty list only means an empty dropdown.

That also fixes an existing false positive of the same family:
loadStacksFor() stored [] when the request failed, so a single failed
fetch reported "Quell-Stapel und Ziel-Stapel nicht mehr vorhanden" for an
untouched board. Failed requests now store null and are read as "unknown".

The comparison itself lives in WorkflowRunner::findDeadFilters(), static
and side-effect free like cardMatchesFilters(), with unit tests -- notably
that an empty filter is never dead, which would otherwise flag every
unfiltered workflow.

Separately, the moved-card mail now names the board and both stacks.
DeckIntegrationService::describeTargets() resolves the titles at most once
per workflow run, and only when a card actually moved and the workflow
wants a mail; unreadable titles degrade to #<id> rather than costing the
user their notification.
This commit is contained in:
Patrick Niebeling
2026-08-13 23:02:13 +02:00
parent b4057809ed
commit 131ef2e938
7 changed files with 354 additions and 60 deletions
+16 -1
View File
@@ -27,7 +27,7 @@ In cron that is *never* the user being impersonated: `null` if any Deck-owned ba
Consequence for `getUserBoards()`: `listBoardsForCurrentUser()` stays **request-only** anyway — Deck may already have cached board lists behind that field, and the job has no use for a board *list*; it asks `findStackIds()` about one known board id. Before using any other `OCA\Deck\*` service from the runner, check whether it carries a frozen `$userId` — the failure mode is silent and user-crossing, not an error.
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`.
The filter logic (`WorkflowRunner::cardMatchesFilters()`, `::isOverdue()`, `::findDeadFilters()`) is deliberately `static` and side-effect-free so it's unit-testable without a real Deck installation — see `tests/Unit/Service/WorkflowRunnerFilterTest.php`. Keep new pure logic in that shape; it is the only part of this app that has tests at all.
## Workflows are auto-disabled when their Deck targets vanish
@@ -40,6 +40,21 @@ The filter-matching logic (`WorkflowRunner::cardMatchesFilters()`, `::isOverdue(
Archived boards deliberately do *not* trigger this (they stay readable, so `findStackIds()` succeeds), even though they're filtered out of the settings dropdowns.
## Dead filters are surfaced, never disabled
`findBrokenTarget()` covers the board and the two stacks — **not** `filter_user_ids` / `filter_label_ids`. Those are plain id lists compared with `array_intersect()` in `cardMatchesFilters()`, so an entry that no longer exists simply stops matching. With three labels of which one was deleted that is exactly right, and disabling would be wrong.
The pathological case is a filter whose entries are *all* gone: the workflow keeps running, updates `last_run`, and matches nothing — forever, with no mail, no log line and a healthy-looking row. It happens on deleted labels and just as easily on a user losing board access, because Deck's `BoardService::deleteAcl()` calls `assignedUsersMapper->deleteByParticipantOnBoard()` and wipes that user's card assignments.
Two low-stakes signals, no state and no migration:
- `WorkflowRunner::warnAboutDeadFilters()` logs a warning on every run.
- `workflowIssues` in `src/PersonalSettings.vue` marks the row red.
Both obey the same rule as the disable path: **only positive knowledge**. `DeckIntegrationService::findFilterOptions()` therefore *throws* instead of degrading to `[]` — unlike its siblings `listLabels()`/`listParticipants()`, which are wrapped in `call(…, [])` for the dropdowns, where an empty list is harmless. On the JS side the per-board caches store `null` for a failed request, and `isDeadFilter()` returns false for `null`; storing `[]` there would report every stack and filter of the board as deleted the moment one request fails. Don't collapse those two states back together.
Deliberately no mail and no `enabled = false`: one board edit can revive the filter, and there is no "already warned" column to keep a mail one-off with (unlike the disable case, which is one-off for free because a disabled workflow is never picked up again).
## The job has no interval — one run per cron pass
`RunWorkflowsJob` extends `OCP\BackgroundJob\Job`, **not** `TimedJob`, so it runs every time cron picks it up. There is no interval and no knob: the cadence *is* the instance's cron cadence. Don't reintroduce one — an earlier version read `workflow_deck_automation.interval` from `config.php` (default 300, 60s floor), and that config key is gone as of v0.2.0. `TimedJob` with `setInterval(0)` would behave nearly identically but keeps an interval concept nothing uses.