Distinguish between "bad record format" and "bad record value" when validating Trigger rules

Summary:
Depends on D20416. Ref T13269. See D20329.

If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.

Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.

Test Plan:
First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):

{F6374205}

Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:

{F6374211}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13269

Differential Revision: https://secure.phabricator.com/D20417

resources/celerity/map.php

@@ -100,7 +100,7 @@ return array(
     '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 @@ return array(
     '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',

src/applications/project/storage/PhabricatorProjectTrigger.php

@@ -104,9 +104,9 @@ final class PhabricatorProjectTrigger
     $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.

src/applications/project/trigger/PhabricatorProjectTriggerAddProjectsRule.php

@@ -5,15 +5,15 @@ final class PhabricatorProjectTriggerAddProjectsRule
 
   const TRIGGERTYPE = 'task.projects.add';
 
-  public function getSelectControLname() {
+  public function getSelectControlName() {
-    return pht('Add projects');
+    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 @@ final class PhabricatorProjectTriggerAddProjectsRule
     }
   }
 
+  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 @@ final class PhabricatorProjectTriggerAddProjectsRule
   }
 
   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(),

src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php

@@ -24,7 +24,7 @@ final class PhabricatorProjectTriggerInvalidRule
     return false;
   }
 
-  protected function assertValidRuleValue($value) {
+  protected function assertValidRuleRecordFormat($value) {
     return;
   }
 

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php

@@ -21,21 +21,43 @@ final class PhabricatorProjectTriggerManiphestOwnerRule
     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)));
     }
+  }
 
-    if (count($value) != 1) {
+  protected function assertValidRuleRecordValue($value) {
+    if (!$value) {
       throw new Exception(
         pht(
-          'Owner rule value should be a list of exactly one user PHID, or the '.
+          'Owner rule value is required. Specify a user to assign tasks '.
-          'token "none()" (value is "%s").',
+          'to, or the token "none()" to unassign tasks.'));
+    }
+
+    if (count($value) > 1) {
+      throw new Exception(
+        pht(
+          '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) {

src/applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php

@@ -9,20 +9,24 @@ final class PhabricatorProjectTriggerManiphestPriorityRule
     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.',
+          'Task priority value ("%s") is not a valid task priority. '.
-          $value));
+          'Valid priorities are: %s.',
+          $value,
+          implode(', ', array_keys($map))));
     }
   }
 

src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php

@@ -9,20 +9,24 @@ final class PhabricatorProjectTriggerManiphestStatusRule
     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.',
+          'Task status value ("%s") is not a valid task status. '.
-          $value));
+          'Valid statues are: %s.',
+          $value,
+          implode(', ', array_keys($map))));
     }
   }
 

src/applications/project/trigger/PhabricatorProjectTriggerPlaySoundRule.php

@@ -9,20 +9,21 @@ final class PhabricatorProjectTriggerPlaySoundRule
     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));
     }
   }

src/applications/project/trigger/PhabricatorProjectTriggerRemoveProjectsRule.php

@@ -5,15 +5,15 @@ final class PhabricatorProjectTriggerRemoveProjectsRule
 
   const TRIGGERTYPE = 'task.projects.remove';
 
-  public function getSelectControLname() {
+  public function getSelectControlname() {
-    return pht('Remove projects');
+    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 @@ final class PhabricatorProjectTriggerRemoveProjectsRule
     }
   }
 
+  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 @@ final class PhabricatorProjectTriggerRemoveProjectsRule
   }
 
   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(),

src/applications/project/trigger/PhabricatorProjectTriggerRule.php

@@ -23,7 +23,7 @@ abstract class PhabricatorProjectTriggerRule
   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 class PhabricatorProjectTriggerRule
   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();

src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php

@@ -13,7 +13,7 @@ final class PhabricatorProjectTriggerUnknownRule
     return false;
   }
 
-  protected function assertValidRuleValue($value) {
+  protected function assertValidRuleRecordFormat($value) {
     return;
   }
 

src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php

@@ -20,16 +20,18 @@ final class PhabricatorProjectTriggerRulesetTransaction
   }
 
   public function validateTransactions($object, array $xactions) {
+    $actor = $this->getActor();
     $errors = array();
 
     foreach ($xactions as $xaction) {
       $ruleset = $xaction->getNewValue();
 
       try {
-        PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications(
+        $rules =
-          $ruleset,
+          PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications(
-          $allow_invalid = false,
+            $ruleset,
-          $xaction->getViewer());
+            $allow_invalid = false,
+            $actor);
       } catch (PhabricatorProjectTriggerCorruptionException $ex) {
         $errors[] = $this->newInvalidError(
           pht(
@@ -38,6 +40,19 @@ final class PhabricatorProjectTriggerRulesetTransaction
           $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;

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 {