Handle workflows whose board or stack was deleted
Build package / php-lint (8.2) (push) Successful in 49s
Build package / php-lint (8.3) (push) Successful in 45s
Build package / php-lint (8.4) (push) Successful in 44s
Build package / xml-lint (push) Successful in 13s
Build package / unit-tests (push) Successful in 45s
Build package / package (push) Successful in 1m3s
Build package / php-lint (8.2) (push) Successful in 49s
Build package / php-lint (8.3) (push) Successful in 45s
Build package / php-lint (8.4) (push) Successful in 44s
Build package / xml-lint (push) Successful in 13s
Build package / unit-tests (push) Successful in 45s
Build package / package (push) Successful in 1m3s
Deleting a board or stack left the workflow row pointing at nothing: Deck keeps the orphaned cards readable until its DeleteCron purges them, so the run kept failing on the move every few minutes, silently and forever. - The controller now rejects boards and stacks that do not exist (or are not the user's) on create and update, so no new broken row can be stored. - The settings list marks affected workflows in red instead of showing a row that looks healthy. - The background job disables such a workflow and mails its owner once. The mail ignores the notifyEmail flag: that one is about moved cards, this is a notice that the automation stopped. It stays a one-off because a disabled workflow is no longer picked up. The check deliberately goes through StackService::findAll() rather than BoardService::getUserBoards(): Deck injects the current user into BoardService as a string frozen at construction, so in the job -- one process, many users, a cached BoardService -- it would answer for the wrong user or for none, and every workflow on the instance would have been disabled. findStackIds() returns null only for a genuinely missing or forbidden board and throws for anything else, so a Deck outage skips the workflow instead of killing it.
This commit is contained in:
@@ -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');
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<int, array{id: int, title: string}>
|
||||
*
|
||||
* 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<int, array{id: int, title: string, color: string|null}>
|
||||
*/
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user