Write workboard trigger rules to the database

Summary: Ref T5474. Read and write trigger rules so users can actually edit them.

Test Plan: Added, modified, and removed trigger rules. Saved changes, used "Show Details" to review edits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

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

src/__phutil_library_map__.php

@@ -4186,6 +4186,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectTriggerQuery' => 'applications/project/query/PhabricatorProjectTriggerQuery.php',
     'PhabricatorProjectTriggerRule' => 'applications/project/trigger/PhabricatorProjectTriggerRule.php',
     'PhabricatorProjectTriggerRuleRecord' => 'applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php',
+    'PhabricatorProjectTriggerRulesetTransaction' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php',
     'PhabricatorProjectTriggerSearchEngine' => 'applications/project/query/PhabricatorProjectTriggerSearchEngine.php',
     'PhabricatorProjectTriggerTransaction' => 'applications/project/storage/PhabricatorProjectTriggerTransaction.php',
     'PhabricatorProjectTriggerTransactionQuery' => 'applications/project/query/PhabricatorProjectTriggerTransactionQuery.php',
@@ -10319,6 +10320,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectTriggerQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorProjectTriggerRule' => 'Phobject',
     'PhabricatorProjectTriggerRuleRecord' => 'Phobject',
+    'PhabricatorProjectTriggerRulesetTransaction' => 'PhabricatorProjectTriggerTransactionType',
     'PhabricatorProjectTriggerSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorProjectTriggerTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorProjectTriggerTransactionQuery' => 'PhabricatorApplicationTransactionQuery',

src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php

@@ -55,6 +55,7 @@ final class PhabricatorProjectTriggerEditController
 
     $v_name = $trigger->getName();
     $v_edit = $trigger->getEditPolicy();
+    $v_rules = $trigger->getTriggerRules();
 
     $e_name = null;
     $e_edit = null;
@@ -65,8 +66,15 @@ final class PhabricatorProjectTriggerEditController
         $v_name = $request->getStr('name');
         $v_edit = $request->getStr('editPolicy');
 
-        $v_rules = $request->getStr('rules');
-        $v_rules = phutil_json_decode($v_rules);
+        // Read the JSON rules from the request and convert them back into
+        // "TriggerRule" objects so we can render the correct form state
+        // if the user is modifying the rules
+        $raw_rules = $request->getStr('rules');
+        $raw_rules = phutil_json_decode($raw_rules);
+
+        $copy = clone $trigger;
+        $copy->setRuleset($raw_rules);
+        $v_rules = $copy->getTriggerRules();
 
         $xactions = array();
         if (!$trigger->getID()) {
@@ -84,7 +92,10 @@ final class PhabricatorProjectTriggerEditController
           ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY)
           ->setNewValue($v_edit);
 
-        // TODO: Actually write the new rules to the database.
+        $xactions[] = $trigger->getApplicationTransactionTemplate()
+          ->setTransactionType(
+            PhabricatorProjectTriggerRulesetTransaction::TRANSACTIONTYPE)
+          ->setNewValue($raw_rules);
 
         $editor = $trigger->getApplicationTransactionEditor()
           ->setActor($viewer)
@@ -207,6 +218,7 @@ final class PhabricatorProjectTriggerEditController
 
     $this->setupEditorBehavior(
       $trigger,
+      $v_rules,
       $form_id,
       $table_id,
       $create_id,
@@ -250,12 +262,12 @@ final class PhabricatorProjectTriggerEditController
 
   private function setupEditorBehavior(
     PhabricatorProjectTrigger $trigger,
+    array $rule_list,
     $form_id,
     $table_id,
     $create_id,
     $input_id) {
 
-    $rule_list = $trigger->getTriggerRules();
     $rule_list = mpull($rule_list, 'toDictionary');
     $rule_list = array_values($rule_list);
 

src/applications/project/storage/PhabricatorProjectTrigger.php

@@ -62,107 +62,19 @@ final class PhabricatorProjectTrigger
     return pht('Trigger %d', $this->getID());
   }
 
+  public function setRuleset(array $ruleset) {
+    // Clear any cached trigger rules, since we're changing the ruleset
+    // for the trigger.
+    $this->triggerRules = null;
+
+    parent::setRuleset($ruleset);
+  }
+
   public function getTriggerRules() {
     if ($this->triggerRules === null) {
-
-      // TODO: Temporary hard-coded rule specification.
-      $rule_specifications = array(
-        array(
-          'type' => 'status',
-          'value' => ManiphestTaskStatus::getDefaultClosedStatus(),
-        ),
-        // This is an intentionally unknown rule.
-        array(
-          'type' => 'quack',
-          'value' => 'aaa',
-        ),
-        // This is an intentionally invalid rule.
-        array(
-          'type' => 'status',
-          'value' => 'quack',
-        ),
-      );
-
-      // NOTE: We're trying to preserve the database state in the rule
-      // structure, even if it includes rule types we don't 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.
-      // When they do, we still want the UI to reflect the ruleset state
-      // accurately and "Edit" + "Save" shouldn't destroy data unless the
-      // user explicitly modifies the ruleset.
-
-      // When we run into rules which are structured correctly but which have
-      // types we don't know about, we replace them with "Unknown Rules". If
-      // we know about the type of a rule but the value doesn't validate, we
-      // replace it with "Invalid Rules". These two rule types don't take any
-      // actions when a card is dropped into the column, but they show the user
-      // what's wrong with the ruleset and can be saved without causing any
-      // collateral damage.
-
-      $rule_map = PhabricatorProjectTriggerRule::getAllTriggerRules();
-
-      // If the stored rule data isn't a list of rules (or we encounter other
-      // fundamental structural problems, below), there isn't much we can do
-      // to try to represent the state.
-      if (!is_array($rule_specifications)) {
-        throw new PhabricatorProjectTriggerCorruptionException(
-          pht(
-            'Trigger ("%s") has a corrupt ruleset: expected a list of '.
-            'rule specifications, found "%s".',
-            $this->getPHID(),
-            phutil_describe_type($rule_specifications)));
-      }
-
-      $trigger_rules = array();
-      foreach ($rule_specifications as $key => $rule) {
-        if (!is_array($rule)) {
-          throw new PhabricatorProjectTriggerCorruptionException(
-            pht(
-              'Trigger ("%s") has a corrupt ruleset: rule (with key "%s") '.
-              'should be a rule specification, but is actually "%s".',
-              $this->getPHID(),
-              $key,
-              phutil_describe_type($rule)));
-        }
-
-        try {
-          PhutilTypeSpec::checkMap(
-            $rule,
-            array(
-              'type' => 'string',
-              'value' => 'wild',
-            ));
-        } catch (PhutilTypeCheckException $ex) {
-          throw new PhabricatorProjectTriggerCorruptionException(
-            pht(
-              'Trigger ("%s") has a corrupt ruleset: rule (with key "%s") '.
-              'is not a valid rule specification: %s',
-              $this->getPHID(),
-              $key,
-              $ex->getMessage()));
-        }
-
-        $record = id(new PhabricatorProjectTriggerRuleRecord())
-          ->setType(idx($rule, 'type'))
-          ->setValue(idx($rule, 'value'));
-
-        if (!isset($rule_map[$record->getType()])) {
-          $rule = new PhabricatorProjectTriggerUnknownRule();
-        } else {
-          $rule = clone $rule_map[$record->getType()];
-        }
-
-        try {
-          $rule->setRecord($record);
-        } catch (Exception $ex) {
-          $rule = id(new PhabricatorProjectTriggerInvalidRule())
-            ->setRecord($record);
-        }
-
-        $trigger_rules[] = $rule;
-      }
+      $trigger_rules = self::newTriggerRulesFromRuleSpecifications(
+        $this->getRuleset(),
+        $allow_invalid = true);
 
       $this->triggerRules = $trigger_rules;
     }
@@ -170,6 +82,108 @@ final class PhabricatorProjectTrigger
     return $this->triggerRules;
   }
 
+  public static function newTriggerRulesFromRuleSpecifications(
+    array $list,
+    $allow_invalid) {
+
+    // 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.
+
+    // If an administrator adds or removes extensions which add rules, or
+    // an upgrade affects rule validity, existing rules may become invalid.
+    // When they do, we still want the UI to reflect the ruleset state
+    // accurately and "Edit" + "Save" shouldn't destroy data unless the
+    // user explicitly modifies the ruleset.
+
+    // In this mode, when we run into rules which are structured correctly but
+    // which have types we don't know about, we replace them with "Unknown
+    // Rules". If we know about the type of a rule but the value doesn't
+    // validate, we replace it with "Invalid Rules". These two rule types don't
+    // take any actions when a card is dropped into the column, but they show
+    // the user what's wrong with the ruleset and can be saved without causing
+    // any collateral damage.
+
+    $rule_map = PhabricatorProjectTriggerRule::getAllTriggerRules();
+
+    // If the stored rule data isn't a list of rules (or we encounter other
+    // fundamental structural problems, below), there isn't much we can do
+    // to try to represent the state.
+    if (!is_array($list)) {
+      throw new PhabricatorProjectTriggerCorruptionException(
+        pht(
+          'Trigger ruleset is corrupt: expected a list of rule '.
+          'specifications, found "%s".',
+          phutil_describe_type($list)));
+    }
+
+    $trigger_rules = array();
+    foreach ($list as $key => $rule) {
+      if (!is_array($rule)) {
+        throw new PhabricatorProjectTriggerCorruptionException(
+          pht(
+            'Trigger ruleset is corrupt: rule (with key "%s") should be a '.
+            'rule specification, but is actually "%s".',
+            $key,
+            phutil_describe_type($rule)));
+      }
+
+      try {
+        PhutilTypeSpec::checkMap(
+          $rule,
+          array(
+            'type' => 'string',
+            'value' => 'wild',
+          ));
+      } catch (PhutilTypeCheckException $ex) {
+        throw new PhabricatorProjectTriggerCorruptionException(
+          pht(
+            'Trigger ruleset is corrupt: rule (with key "%s") is not a '.
+            'valid rule specification: %s',
+            $key,
+            $ex->getMessage()));
+      }
+
+      $record = id(new PhabricatorProjectTriggerRuleRecord())
+        ->setType(idx($rule, 'type'))
+        ->setValue(idx($rule, 'value'));
+
+      if (!isset($rule_map[$record->getType()])) {
+        if (!$allow_invalid) {
+          throw new PhabricatorProjectTriggerCorruptionException(
+            pht(
+              'Trigger ruleset is corrupt: rule type "%s" is unknown.',
+              $record->getType()));
+        }
+
+        $rule = new PhabricatorProjectTriggerUnknownRule();
+      } else {
+        $rule = clone $rule_map[$record->getType()];
+      }
+
+      try {
+        $rule->setRecord($record);
+      } catch (Exception $ex) {
+        if (!$allow_invalid) {
+          throw new PhabricatorProjectTriggerCorruptionException(
+            pht(
+              'Trigger ruleset is corrupt, rule (of type "%s") does not '.
+              'validate: %s',
+              $record->getType(),
+              $ex->getMessage()));
+        }
+
+        $rule = id(new PhabricatorProjectTriggerInvalidRule())
+          ->setRecord($record);
+      }
+
+      $trigger_rules[] = $rule;
+    }
+
+    return $trigger_rules;
+  }
+
+
   public function getDropEffects() {
     $effects = array();
 

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

@@ -0,0 +1,65 @@
+<?php
+
+final class PhabricatorProjectTriggerRulesetTransaction
+  extends PhabricatorProjectTriggerTransactionType {
+
+  const TRANSACTIONTYPE = 'ruleset';
+
+  public function generateOldValue($object) {
+    return $object->getRuleset();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setRuleset($value);
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s updated the ruleset for this trigger.',
+      $this->renderAuthor());
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $ruleset = $xaction->getNewValue();
+
+      try {
+        PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications(
+          $ruleset,
+          $allow_invalid = false);
+      } catch (PhabricatorProjectTriggerCorruptionException $ex) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Ruleset specification is not valid. %s',
+            $ex->getMessage()),
+          $xaction);
+        continue;
+      }
+    }
+
+    return $errors;
+  }
+
+  public function hasChangeDetailView() {
+    return true;
+  }
+
+  public function newChangeDetailView() {
+    $viewer = $this->getViewer();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $json = new PhutilJSON();
+    $old_json = $json->encodeAsList($old);
+    $new_json = $json->encodeAsList($new);
+
+    return id(new PhabricatorApplicationTransactionTextDiffDetailView())
+      ->setViewer($viewer)
+      ->setOldText($old_json)
+      ->setNewText($new_json);
+  }
+
+}