revert 0e66319a06
Build package / php-lint (8.4) (push) Successful in 43s
Build package / php-lint (8.5) (push) Successful in 40s
Build package / xml-lint (push) Successful in 12s
Build package / unit-tests (push) Successful in 38s
Build package / package (push) Successful in 52s
Build package / php-lint (8.4) (push) Successful in 43s
Build package / php-lint (8.5) (push) Successful in 40s
Build package / xml-lint (push) Successful in 12s
Build package / unit-tests (push) Successful in 38s
Build package / package (push) Successful in 52s
revert Let each workflow watch the due date or the start date
Deck 1.18 added a start date to cards (Card::$startdate), so a workflow
no longer has to mean "overdue". Each rule now carries a date_field
('due' | 'start'), chosen in the settings form next to board and stacks.
Existing rules keep the due date: the new column's default supplies it
for every stored row, so there is no backfill step, and
Workflow::getDateFieldOrDue() covers entities that were never near the
database as well as hand-edited values. The controller is the one place
that rejects an unknown value instead of normalising it - a client
asking for a rule it would not get should hear about it.
The probe for the field is property_exists(), not method_exists():
Deck's entities declare no getter as real code - getStartdate(),
getDuedate(), even getId() all go through Entity::__call(), which
method_exists() ignores by definition. A method_exists() guard would
have been false on every Deck version and would have turned every
start-date rule into a silent no-op.
assertDeckAvailable() now also refuses Deck older than 1.18.0. That
floor cannot live in appinfo/info.xml - neither the server's nor the app
store's schema has an app-to-app dependency element - but
<nextcloud min-version="34"> already implies it, since 1.18.x is the
only Deck release line published for Nextcloud 34.
isOverdue() becomes hasPassed(): one function for both dates, since the
comparison and the argument against Deck's day-truncating
getDaysUntilDue() are identical for either.
Also carries a pending NcSelect icon-alignment fix that was already in
the working tree.
Bump to 34.1.0.
This commit is contained in:
@@ -56,15 +56,14 @@ class WorkflowController extends OCSController {
|
||||
int $targetStackId,
|
||||
array $filterUserIds = [],
|
||||
array $filterLabelIds = [],
|
||||
string $dateField = Workflow::DATE_FIELD_DUE,
|
||||
bool $notifyEmail = false,
|
||||
bool $enabled = true,
|
||||
): DataResponse {
|
||||
$this->validate($title, $boardId, $sourceStackId, $targetStackId, $dateField);
|
||||
$this->validate($title, $boardId, $sourceStackId, $targetStackId);
|
||||
|
||||
$workflow = new Workflow();
|
||||
$workflow->setUserId($this->currentUserId());
|
||||
$this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $filterUserIds, $filterLabelIds, $dateField, $notifyEmail, $enabled);
|
||||
$this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $filterUserIds, $filterLabelIds, $notifyEmail, $enabled);
|
||||
|
||||
$workflow = $this->workflowMapper->insert($workflow);
|
||||
return new DataResponse($workflow->jsonSerialize(), Http::STATUS_CREATED);
|
||||
@@ -83,11 +82,10 @@ class WorkflowController extends OCSController {
|
||||
int $targetStackId,
|
||||
array $filterUserIds = [],
|
||||
array $filterLabelIds = [],
|
||||
string $dateField = Workflow::DATE_FIELD_DUE,
|
||||
bool $notifyEmail = false,
|
||||
bool $enabled = true,
|
||||
): DataResponse {
|
||||
$this->validate($title, $boardId, $sourceStackId, $targetStackId, $dateField);
|
||||
$this->validate($title, $boardId, $sourceStackId, $targetStackId);
|
||||
|
||||
try {
|
||||
$workflow = $this->workflowMapper->findForUser($id, $this->currentUserId());
|
||||
@@ -95,7 +93,7 @@ class WorkflowController extends OCSController {
|
||||
throw new OCSNotFoundException('Workflow not found');
|
||||
}
|
||||
|
||||
$this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $filterUserIds, $filterLabelIds, $dateField, $notifyEmail, $enabled);
|
||||
$this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $filterUserIds, $filterLabelIds, $notifyEmail, $enabled);
|
||||
$workflow = $this->workflowMapper->update($workflow);
|
||||
|
||||
return new DataResponse($workflow->jsonSerialize());
|
||||
@@ -149,19 +147,13 @@ class WorkflowController extends OCSController {
|
||||
}
|
||||
}
|
||||
|
||||
private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId, string $dateField): 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');
|
||||
}
|
||||
// Rejected rather than silently normalised to 'due': a client sending
|
||||
// something else is asking for a rule it would not get, and the runner
|
||||
// falls back to the due date without a word.
|
||||
if (!in_array($dateField, Workflow::DATE_FIELDS, true)) {
|
||||
throw new OCSBadRequestException('Unknown date field: ' . $dateField);
|
||||
}
|
||||
|
||||
// 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,7 +190,6 @@ class WorkflowController extends OCSController {
|
||||
int $targetStackId,
|
||||
array $filterUserIds,
|
||||
array $filterLabelIds,
|
||||
string $dateField,
|
||||
bool $notifyEmail,
|
||||
bool $enabled,
|
||||
): void {
|
||||
@@ -208,7 +199,6 @@ class WorkflowController extends OCSController {
|
||||
$workflow->setTargetStackId($targetStackId);
|
||||
$workflow->setFilterUserIds($filterUserIds === [] ? null : json_encode(array_values($filterUserIds)));
|
||||
$workflow->setFilterLabelIds($filterLabelIds === [] ? null : json_encode(array_values($filterLabelIds)));
|
||||
$workflow->setDateField($dateField);
|
||||
$workflow->setNotifyEmail($notifyEmail);
|
||||
$workflow->setEnabled($enabled);
|
||||
}
|
||||
|
||||
@@ -22,8 +22,6 @@ use OCP\DB\Types;
|
||||
* @method void setFilterUserIds(?string $filterUserIds)
|
||||
* @method string|null getFilterLabelIds()
|
||||
* @method void setFilterLabelIds(?string $filterLabelIds)
|
||||
* @method string|null getDateField()
|
||||
* @method void setDateField(string $dateField)
|
||||
* @method bool getNotifyEmail()
|
||||
* @method void setNotifyEmail(bool $notifyEmail)
|
||||
* @method bool getEnabled()
|
||||
@@ -32,14 +30,6 @@ use OCP\DB\Types;
|
||||
* @method void setLastRun(?\DateTime $lastRun)
|
||||
*/
|
||||
class Workflow extends Entity implements \JsonSerializable {
|
||||
/** Deck's `Card::$duedate` — the card is picked up once it is overdue. */
|
||||
public const DATE_FIELD_DUE = 'due';
|
||||
|
||||
/** Deck's `Card::$startdate` — the card is picked up once its start date has been reached. */
|
||||
public const DATE_FIELD_START = 'start';
|
||||
|
||||
public const DATE_FIELDS = [self::DATE_FIELD_DUE, self::DATE_FIELD_START];
|
||||
|
||||
protected $userId;
|
||||
protected $title;
|
||||
protected $boardId;
|
||||
@@ -47,7 +37,6 @@ class Workflow extends Entity implements \JsonSerializable {
|
||||
protected $targetStackId;
|
||||
protected $filterUserIds;
|
||||
protected $filterLabelIds;
|
||||
protected $dateField;
|
||||
protected $notifyEmail;
|
||||
protected $enabled;
|
||||
protected $lastRun;
|
||||
@@ -61,29 +50,11 @@ class Workflow extends Entity implements \JsonSerializable {
|
||||
$this->addType('targetStackId', Types::INTEGER);
|
||||
$this->addType('filterUserIds', Types::STRING);
|
||||
$this->addType('filterLabelIds', Types::STRING);
|
||||
$this->addType('dateField', Types::STRING);
|
||||
$this->addType('notifyEmail', Types::BOOLEAN);
|
||||
$this->addType('enabled', Types::BOOLEAN);
|
||||
$this->addType('lastRun', Types::DATETIME);
|
||||
}
|
||||
|
||||
/**
|
||||
* The date field to evaluate, never anything else.
|
||||
*
|
||||
* Rows written before the column existed carry 'due' from the schema
|
||||
* default, but a freshly constructed entity holds `null` and a hand-edited
|
||||
* row could hold anything. Falling back to the due date keeps the old
|
||||
* behaviour for every value that is not explicitly the start date — the
|
||||
* safe direction, since 'due' is what every workflow meant before this
|
||||
* choice existed.
|
||||
*
|
||||
* @return self::DATE_FIELD_*
|
||||
*/
|
||||
public function getDateFieldOrDue(): string {
|
||||
$field = $this->getDateField();
|
||||
return in_array($field, self::DATE_FIELDS, true) ? $field : self::DATE_FIELD_DUE;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return string[]
|
||||
*/
|
||||
@@ -119,7 +90,6 @@ class Workflow extends Entity implements \JsonSerializable {
|
||||
'targetStackId' => $this->getTargetStackId(),
|
||||
'filterUserIds' => $this->getFilterUserIdsArray(),
|
||||
'filterLabelIds' => $this->getFilterLabelIdsArray(),
|
||||
'dateField' => $this->getDateFieldOrDue(),
|
||||
'notifyEmail' => (bool)$this->getNotifyEmail(),
|
||||
'enabled' => (bool)$this->getEnabled(),
|
||||
'lastRun' => $this->getLastRun()?->format(\DateTimeInterface::ATOM),
|
||||
|
||||
@@ -1,47 +0,0 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace OCA\WorkflowDeckAutomation\Migration;
|
||||
|
||||
use Closure;
|
||||
use OCA\WorkflowDeckAutomation\Db\Workflow;
|
||||
use OCP\DB\ISchemaWrapper;
|
||||
use OCP\DB\Types;
|
||||
use OCP\Migration\IOutput;
|
||||
use OCP\Migration\SimpleMigrationStep;
|
||||
|
||||
/**
|
||||
* Adds the per-workflow choice between Deck's due date and its start date.
|
||||
*
|
||||
* A separate migration rather than an edit to Version1000...: that one is
|
||||
* wrapped in `if (!$schema->hasTable(...))`, so changing it would only ever
|
||||
* reach fresh installs.
|
||||
*
|
||||
* The `default` is what migrates the existing rows — every workflow written
|
||||
* before this release was evaluated against the due date, and the column
|
||||
* default gives exactly that without a backfill step.
|
||||
*/
|
||||
class Version1001Date20260825120000 extends SimpleMigrationStep {
|
||||
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
|
||||
/** @var ISchemaWrapper $schema */
|
||||
$schema = $schemaClosure();
|
||||
|
||||
if (!$schema->hasTable('wfda_workflows')) {
|
||||
return null;
|
||||
}
|
||||
|
||||
$table = $schema->getTable('wfda_workflows');
|
||||
if ($table->hasColumn('date_field')) {
|
||||
return null;
|
||||
}
|
||||
|
||||
$table->addColumn('date_field', Types::STRING, [
|
||||
'notnull' => true,
|
||||
'length' => 16,
|
||||
'default' => Workflow::DATE_FIELD_DUE,
|
||||
]);
|
||||
|
||||
return $schema;
|
||||
}
|
||||
}
|
||||
@@ -4,7 +4,6 @@ declare(strict_types=1);
|
||||
|
||||
namespace OCA\WorkflowDeckAutomation\Service;
|
||||
|
||||
use DateTimeInterface;
|
||||
use OCA\Deck\Db\BoardMapper;
|
||||
use OCA\Deck\Db\Card;
|
||||
use OCA\Deck\Db\CardMapper;
|
||||
@@ -36,25 +35,6 @@ use Throwable;
|
||||
class DeckIntegrationService {
|
||||
private const DECK_APP_ID = 'deck';
|
||||
|
||||
/**
|
||||
* Oldest Deck this app will talk to.
|
||||
*
|
||||
* Deck 1.18.0 is the release that added the card start date
|
||||
* (`Card::$startdate`, PR #7749), which the start-date workflows read.
|
||||
* The floor cannot live in appinfo/info.xml: neither the server's
|
||||
* app-info.xsd nor the app store's info.xsd has an app-to-app dependency
|
||||
* element, only <nextcloud>.
|
||||
*
|
||||
* That pin does most of the work anyway — Deck 1.18.x is the only release
|
||||
* line declaring `>=34.0.0,<35.0.0`, so on the Nextcloud 34 this app
|
||||
* requires, every installable Deck already carries the field. What is left
|
||||
* for this check is a Deck put in place by hand or from git.
|
||||
*
|
||||
* version_compare() sorts `1.18.0-beta.x` *below* `1.18.0`, and that is
|
||||
* intended: the betas of that line may predate the field.
|
||||
*/
|
||||
private const MIN_DECK_VERSION = '1.18.0';
|
||||
|
||||
/**
|
||||
* Deck's Acl::PERMISSION_TYPE_* values, inlined so this class keeps
|
||||
* working (degrading to "treat it as a user") if Deck ever moves or
|
||||
@@ -88,31 +68,11 @@ class DeckIntegrationService {
|
||||
'OCA\Deck\Activity\ActivityManager',
|
||||
];
|
||||
|
||||
/**
|
||||
* `Workflow::DATE_FIELD_*` mapped to the property on Deck's Card entity.
|
||||
* Kept here rather than on the Workflow so this class stays the only one
|
||||
* that has to know what Deck calls its columns.
|
||||
*/
|
||||
private const CARD_DATE_PROPERTIES = [
|
||||
'due' => 'duedate',
|
||||
'start' => 'startdate',
|
||||
];
|
||||
|
||||
private const DEFAULT_CARD_DATE = 'due';
|
||||
|
||||
/**
|
||||
* @var list<array{ReflectionProperty, object, ?string}>
|
||||
*/
|
||||
private array $identityRestore = [];
|
||||
|
||||
/**
|
||||
* Card properties already reported as missing, so the warning in
|
||||
* getCardDate() is one line per process instead of one per card.
|
||||
*
|
||||
* @var array<string, true>
|
||||
*/
|
||||
private array $reportedMissingCardProperties = [];
|
||||
|
||||
public function __construct(
|
||||
private IAppManager $appManager,
|
||||
private IUserManager $userManager,
|
||||
@@ -206,16 +166,6 @@ class DeckIntegrationService {
|
||||
if (!$this->appManager->isEnabledForUser(self::DECK_APP_ID, $user)) {
|
||||
throw new DeckUnavailableException('Deck is not enabled for user ' . $user->getUID());
|
||||
}
|
||||
|
||||
$version = $this->appManager->getAppVersion(self::DECK_APP_ID);
|
||||
if ($version === '' || version_compare($version, self::MIN_DECK_VERSION, '<')) {
|
||||
throw new DeckUnavailableException(sprintf(
|
||||
'Deck %s is installed, but %s or newer is required',
|
||||
$version !== '' ? $version : '(unknown version)',
|
||||
self::MIN_DECK_VERSION,
|
||||
));
|
||||
}
|
||||
|
||||
if (!class_exists(CardService::class) || !class_exists(BoardService::class) || !class_exists(StackService::class)) {
|
||||
throw new DeckUnavailableException('Deck internal classes are not available');
|
||||
}
|
||||
@@ -537,7 +487,7 @@ class DeckIntegrationService {
|
||||
// *returns* CardDetails wrappers whose own fields (duedate, stackId, ...) were never
|
||||
// copied from the wrapped card - only its overridden jsonSerialize() reads through to
|
||||
// them. Capturing that return value here made every card's getDuedate()/getDaysUntilDue()
|
||||
// resolve to null, so hasPassed() was always false and no workflow ever matched a card.
|
||||
// resolve to null, so isOverdue() was always false and no workflow ever matched a card.
|
||||
// Keep using the original, now-enriched $cards instead.
|
||||
$cardService->enrichCards($cards);
|
||||
|
||||
@@ -550,45 +500,6 @@ class DeckIntegrationService {
|
||||
}, []);
|
||||
}
|
||||
|
||||
/**
|
||||
* The card date a workflow is evaluated against: Deck's due date or its
|
||||
* start date, per `Workflow::DATE_FIELD_*`. Anything unrecognised reads the
|
||||
* due date, which is what every workflow meant before the choice existed.
|
||||
*
|
||||
* The guard is `property_exists()`, deliberately, and this is the one thing
|
||||
* to get right here: **`method_exists()` cannot see Deck's getters.**
|
||||
* `Card`, `Board`, `Label`, `Acl` and `Assignment` declare none of them as
|
||||
* real code — `getStartdate()`, `getDuedate()`, even `getId()` all go
|
||||
* through `OCP\AppFramework\Db\Entity::__call()`, and `method_exists()`
|
||||
* ignores `__call()` by definition. A `method_exists($card, 'getStartdate')`
|
||||
* probe would therefore be false on *every* Deck version and silently turn
|
||||
* every start-date workflow into a no-op. `property_exists()` sees the
|
||||
* protected `$startdate` declaration and is exactly what `Entity::getter()`
|
||||
* checks internally before throwing.
|
||||
*
|
||||
* In practice the property is always there — `assertDeckAvailable()`
|
||||
* enforces Deck >= MIN_DECK_VERSION, which is the release that added it —
|
||||
* so this branch is a backstop for a hand-installed Deck, loud enough to be
|
||||
* found in the log rather than silently matching nothing.
|
||||
*/
|
||||
public function getCardDate(Card $card, string $dateField): ?DateTimeInterface {
|
||||
$property = self::CARD_DATE_PROPERTIES[$dateField] ?? self::CARD_DATE_PROPERTIES[self::DEFAULT_CARD_DATE];
|
||||
|
||||
if (!property_exists($card, $property)) {
|
||||
if (!isset($this->reportedMissingCardProperties[$property])) {
|
||||
$this->reportedMissingCardProperties[$property] = true;
|
||||
$this->logger->warning(
|
||||
'This Deck version has no Card::$' . $property . '; workflows using it cannot match any card',
|
||||
['app' => 'workflow_deck_automation'],
|
||||
);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
$value = $property === 'startdate' ? $card->getStartdate() : $card->getDuedate();
|
||||
return $value instanceof DateTimeInterface ? $value : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return string[]
|
||||
*/
|
||||
|
||||
@@ -55,19 +55,10 @@ class NotificationMailer {
|
||||
$template->setSubject($this->l10n->t('Deck card moved: %s', [$card->getTitle()]));
|
||||
$template->addHeader();
|
||||
$template->addHeading($this->l10n->t('A card was moved automatically'), false);
|
||||
// Two whole strings rather than one sentence with an interpolated
|
||||
// reason: the trigger is the point of the mail, and a translator
|
||||
// needs to see it in the sentence to get the grammar right.
|
||||
$reason = $workflow->getDateFieldOrDue() === Workflow::DATE_FIELD_START
|
||||
? $this->l10n->t(
|
||||
'The card "%1$s" was moved because its start date has been reached (workflow "%2$s").',
|
||||
[$card->getTitle(), $workflow->getTitle()],
|
||||
)
|
||||
: $this->l10n->t(
|
||||
'The card "%1$s" was moved because it is overdue (workflow "%2$s").',
|
||||
[$card->getTitle(), $workflow->getTitle()],
|
||||
);
|
||||
$template->addBodyText($reason);
|
||||
$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.
|
||||
|
||||
@@ -17,8 +17,7 @@ use Psr\Log\LoggerInterface;
|
||||
use Throwable;
|
||||
|
||||
/**
|
||||
* Evaluates all enabled workflows and moves the cards whose watched date -
|
||||
* Deck's due date or its start date, per workflow - has passed.
|
||||
* Evaluates all enabled workflows and moves overdue cards.
|
||||
*
|
||||
* This runner has to evaluate rules for many different users within a
|
||||
* single background-job process, so workflows are grouped by owner and
|
||||
@@ -168,10 +167,9 @@ class WorkflowRunner {
|
||||
$targets = null;
|
||||
|
||||
$now = $this->timeFactory->getDateTime();
|
||||
$dateField = $workflow->getDateFieldOrDue();
|
||||
|
||||
foreach ($cards as $card) {
|
||||
if (!self::hasPassed($this->deckService->getCardDate($card, $dateField), $now)) {
|
||||
if (!self::isOverdue($card->getDuedate(), $now)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -181,12 +179,11 @@ class WorkflowRunner {
|
||||
continue;
|
||||
}
|
||||
|
||||
$this->logger->info('Card {card} ("{title}") matches workflow {id} on its {dateField} date; moving to stack {target}.', [
|
||||
$this->logger->info('Card {card} ("{title}") matches workflow {id}; moving to stack {target}.', [
|
||||
'app' => 'workflow_deck_automation',
|
||||
'card' => $card->getId(),
|
||||
'title' => $card->getTitle(),
|
||||
'id' => $workflow->getId(),
|
||||
'dateField' => $dateField,
|
||||
'target' => $workflow->getTargetStackId(),
|
||||
]);
|
||||
|
||||
@@ -326,21 +323,16 @@ class WorkflowRunner {
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the card date a workflow watches - its due date or its start date,
|
||||
* see Workflow::DATE_FIELD_* - lies in the past. A card without that date set
|
||||
* is never picked up.
|
||||
*
|
||||
* Compares the card's actual timestamp against $now instead of Deck's own
|
||||
* Compares the card's actual due timestamp against $now instead of Deck's own
|
||||
* Card::getDaysUntilDue(), which truncates both sides to midnight before diffing
|
||||
* (DateInterval's %a is a whole-day count) and therefore reports 0 - not overdue -
|
||||
* for any card whose due time has already passed today. That silently held back
|
||||
* every card for up to 24h after it actually fell due; comparing the raw datetime
|
||||
* instead matches what Deck's own UI shows (e.g. "vor 2 Stunden") and has no
|
||||
* day-rounding step to get wrong. The same reasoning applies unchanged to the
|
||||
* start date, which is why this is one function and not two.
|
||||
* day-rounding step to get wrong.
|
||||
*/
|
||||
public static function hasPassed(?DateTimeInterface $cardDate, DateTimeInterface $now): bool {
|
||||
return $cardDate !== null && $cardDate < $now;
|
||||
public static function isOverdue(?DateTimeInterface $duedate, DateTimeInterface $now): bool {
|
||||
return $duedate !== null && $duedate < $now;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user