diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -100,7 +100,7 @@ 'rsrc/css/application/policy/policy.css' => 'ceb56a08', 'rsrc/css/application/ponder/ponder-view.css' => '05a09d0a', 'rsrc/css/application/project/project-card-view.css' => '4e7371cd', - 'rsrc/css/application/project/project-triggers.css' => 'cb866c2d', + 'rsrc/css/application/project/project-triggers.css' => 'cd9c8bb9', 'rsrc/css/application/project/project-view.css' => '567858b3', 'rsrc/css/application/releeph/releeph-core.css' => 'f81ff2db', 'rsrc/css/application/releeph/releeph-preview-branch.css' => '22db5c07', @@ -882,7 +882,7 @@ 'policy-transaction-detail-css' => 'c02b8384', 'ponder-view-css' => '05a09d0a', 'project-card-view-css' => '4e7371cd', - 'project-triggers-css' => 'cb866c2d', + 'project-triggers-css' => 'cd9c8bb9', 'project-view-css' => '567858b3', 'releeph-core' => 'f81ff2db', 'releeph-preview-branch' => '22db5c07', diff --git a/src/applications/project/storage/PhabricatorProjectTrigger.php b/src/applications/project/storage/PhabricatorProjectTrigger.php --- a/src/applications/project/storage/PhabricatorProjectTrigger.php +++ b/src/applications/project/storage/PhabricatorProjectTrigger.php @@ -104,9 +104,9 @@ $allow_invalid, PhabricatorUser $viewer) { - // NOTE: With "$allow_invalid" set, we're trying to preserve the database + // NOTE: With "$allow_invalid" set, we're trying to preserve the database // state in the rule structure, even if it includes rule types we don't - // ha ve implementations for, or rules with invalid rule values. + // have implementations for, or rules with invalid rule values. // If an administrator adds or removes extensions which add rules, or // an upgrade affects rule validity, existing rules may become invalid. diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerAddProjectsRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerAddProjectsRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerAddProjectsRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerAddProjectsRule.php @@ -5,15 +5,15 @@ const TRIGGERTYPE = 'task.projects.add'; - public function getSelectControLname() { - return pht('Add projects'); + public function getSelectControlName() { + return pht('Add project tags'); } protected function getValueForEditorField() { return $this->getDatasource()->getWireTokens($this->getValue()); } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_array($value)) { throw new Exception( pht( @@ -23,6 +23,14 @@ } } + protected function assertValidRuleRecordValue($value) { + if (!$value) { + throw new Exception( + pht( + 'You must select at least one project tag to add.')); + } + } + protected function newDropTransactions($object, $value) { $project_edge_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; @@ -78,12 +86,12 @@ } public function getRuleViewLabel() { - return pht('Add Projects'); + return pht('Add Project Tags'); } public function getRuleViewDescription($value) { return pht( - 'Add projects: %s.', + 'Add project tags: %s.', phutil_tag( 'strong', array(), diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php @@ -24,7 +24,7 @@ return false; } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { return; } diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php @@ -21,21 +21,43 @@ return $value; } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_array($value)) { throw new Exception( pht( 'Owner rule value should be a list, but is not (value is "%s").', phutil_describe_type($value))); } + } + + protected function assertValidRuleRecordValue($value) { + if (!$value) { + throw new Exception( + pht( + 'Owner rule value is required. Specify a user to assign tasks '. + 'to, or the token "none()" to unassign tasks.')); + } - if (count($value) != 1) { + if (count($value) > 1) { throw new Exception( pht( - 'Owner rule value should be a list of exactly one user PHID, or the '. - 'token "none()" (value is "%s").', + 'Owner rule value must have only one elmement (value is "%s").', implode(', ', $value))); } + + $owner_phid = $this->convertTokenizerValueToOwner($value); + if ($owner_phid !== null) { + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs(array($owner_phid)) + ->executeOne(); + if (!$user) { + throw new Exception( + pht( + 'User PHID ("%s") is not a valid user.', + $owner_phid)); + } + } } protected function newDropTransactions($object, $value) { diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php @@ -9,20 +9,24 @@ return pht('Change priority to'); } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_string($value)) { throw new Exception( pht( 'Priority rule value should be a string, but is not (value is "%s").', phutil_describe_type($value))); } + } + protected function assertValidRuleRecordValue($value) { $map = ManiphestTaskPriority::getTaskPriorityMap(); if (!isset($map[$value])) { throw new Exception( pht( - 'Rule value ("%s") is not a valid task priority.', - $value)); + 'Task priority value ("%s") is not a valid task priority. '. + 'Valid priorities are: %s.', + $value, + implode(', ', array_keys($map)))); } } diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php @@ -9,20 +9,24 @@ return pht('Change status to'); } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_string($value)) { throw new Exception( pht( 'Status rule value should be a string, but is not (value is "%s").', phutil_describe_type($value))); } + } + protected function assertValidRuleRecordValue($value) { $map = ManiphestTaskStatus::getTaskStatusMap(); if (!isset($map[$value])) { throw new Exception( pht( - 'Rule value ("%s") is not a valid task status.', - $value)); + 'Task status value ("%s") is not a valid task status. '. + 'Valid statues are: %s.', + $value, + implode(', ', array_keys($map)))); } } diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerPlaySoundRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerPlaySoundRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerPlaySoundRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerPlaySoundRule.php @@ -9,20 +9,21 @@ return pht('Play sound'); } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_string($value)) { throw new Exception( pht( 'Status rule value should be a string, but is not (value is "%s").', phutil_describe_type($value))); } + } + protected function assertValidRuleRecordValue($value) { $map = self::getSoundMap(); - if (!isset($map[$value])) { throw new Exception( pht( - 'Rule value ("%s") is not a valid sound.', + 'Sound ("%s") is not a valid sound.', $value)); } } diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerRemoveProjectsRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerRemoveProjectsRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerRemoveProjectsRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerRemoveProjectsRule.php @@ -5,15 +5,15 @@ const TRIGGERTYPE = 'task.projects.remove'; - public function getSelectControLname() { - return pht('Remove projects'); + public function getSelectControlname() { + return pht('Remove project tags'); } protected function getValueForEditorField() { return $this->getDatasource()->getWireTokens($this->getValue()); } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { if (!is_array($value)) { throw new Exception( pht( @@ -23,6 +23,14 @@ } } + protected function assertValidRuleRecordValue($value) { + if (!$value) { + throw new Exception( + pht( + 'You must select at least one project tag to remove.')); + } + } + protected function newDropTransactions($object, $value) { $project_edge_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; @@ -78,12 +86,12 @@ } public function getRuleViewLabel() { - return pht('Remove Projects'); + return pht('Remove Project Tags'); } public function getRuleViewDescription($value) { return pht( - 'Remove projects: %s.', + 'Remove project tags: %s.', phutil_tag( 'strong', array(), diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php @@ -23,7 +23,7 @@ final public function setRecord(PhabricatorProjectTriggerRuleRecord $record) { $value = $record->getValue(); - $this->assertValidRuleValue($value); + $this->assertValidRuleRecordFormat($value); $this->record = $record; return $this; @@ -45,7 +45,22 @@ abstract public function getRuleViewLabel(); abstract public function getRuleViewDescription($value); abstract public function getRuleViewIcon($value); - abstract protected function assertValidRuleValue($value); + abstract protected function assertValidRuleRecordFormat($value); + + final public function getRuleRecordValueValidationException() { + try { + $this->assertValidRuleRecordValue($this->getRecord()->getValue()); + } catch (Exception $ex) { + return $ex; + } + + return null; + } + + protected function assertValidRuleRecordValue($value) { + return; + } + abstract protected function newDropTransactions($object, $value); abstract protected function newDropEffects($value); abstract protected function getDefaultValue(); diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php --- a/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php @@ -13,7 +13,7 @@ return false; } - protected function assertValidRuleValue($value) { + protected function assertValidRuleRecordFormat($value) { return; } diff --git a/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php --- a/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php +++ b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php @@ -20,16 +20,18 @@ } public function validateTransactions($object, array $xactions) { + $actor = $this->getActor(); $errors = array(); foreach ($xactions as $xaction) { $ruleset = $xaction->getNewValue(); try { - PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications( - $ruleset, - $allow_invalid = false, - $xaction->getViewer()); + $rules = + PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications( + $ruleset, + $allow_invalid = false, + $actor); } catch (PhabricatorProjectTriggerCorruptionException $ex) { $errors[] = $this->newInvalidError( pht( @@ -38,6 +40,19 @@ $xaction); continue; } + + foreach ($rules as $rule) { + $exception = $rule->getRuleRecordValueValidationException(); + if ($exception) { + $errors[] = $this->newInvalidError( + pht( + 'Value for "%s" rule is invalid: %s', + $rule->getSelectControlName(), + $exception->getMessage()), + $xaction); + continue; + } + } } return $errors; diff --git a/webroot/rsrc/css/application/project/project-triggers.css b/webroot/rsrc/css/application/project/project-triggers.css --- a/webroot/rsrc/css/application/project/project-triggers.css +++ b/webroot/rsrc/css/application/project/project-triggers.css @@ -27,6 +27,7 @@ .trigger-rules-table td.invalid-cell { padding-left: 12px; + width: 100%; } .trigger-rules-table td.invalid-cell .phui-icon-view {