Drive Herald edits via transactions

Summary: This is kinda bad in terms of UI (It just makes a json of the thing and diffs that), but it's a start.

Test Plan: edit rule, create rule, add/remove/edit conditions, actions

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

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

src/__phutil_library_map__.php

@@ -1230,6 +1230,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php',
     'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php',
     'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php',
+    'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php',
     'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php',
     'HeraldRuleTransaction' => 'applications/herald/storage/HeraldRuleTransaction.php',
     'HeraldRuleTransactionComment' => 'applications/herald/storage/HeraldRuleTransactionComment.php',
@@ -5497,6 +5498,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'PhabricatorPHIDType',
     'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine',
+    'HeraldRuleSerializer' => 'Phobject',
     'HeraldRuleTestCase' => 'PhabricatorTestCase',
     'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction',
     'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment',

src/applications/herald/controller/HeraldRuleController.php

@@ -259,18 +259,15 @@ final class HeraldRuleController extends HeraldController {
   }
 
   private function saveRule(HeraldAdapter $adapter, $rule, $request) {
-    $rule->setName($request->getStr('name'));
+    $new_name = $request->getStr('name');
     $match_all = ($request->getStr('must_match') == 'all');
-    $rule->setMustMatchAll((int)$match_all);
 
     $repetition_policy_param = $request->getStr('repetition_policy');
-    $rule->setRepetitionPolicy(
-      HeraldRepetitionPolicyConfig::toInt($repetition_policy_param));
 
     $e_name = true;
     $errors = array();
 
-    if (!strlen($rule->getName())) {
+    if (!strlen($new_name)) {
       $e_name = pht('Required');
       $errors[] = pht('Rule must have a name.');
     }
@@ -343,19 +340,41 @@ final class HeraldRuleController extends HeraldController {
       $actions[] = $obj;
     }
 
+    if (!$errors) {
+      $new_state = id(new HeraldRuleSerializer())->serializeRuleComponents(
+        $match_all,
+        $conditions,
+        $actions,
+        $repetition_policy_param);
+
+      $xactions = array();
+      $xactions[] = id(new HeraldRuleTransaction())
+        ->setTransactionType(HeraldRuleTransaction::TYPE_EDIT)
+        ->setNewValue($new_state);
+      $xactions[] = id(new HeraldRuleTransaction())
+        ->setTransactionType(HeraldRuleTransaction::TYPE_NAME)
+        ->setNewValue($new_name);
+
+      try {
+        id(new HeraldRuleEditor())
+          ->setActor($this->getViewer())
+          ->setContinueOnNoEffect(true)
+          ->setContentSourceFromRequest($request)
+          ->applyTransactions($rule, $xactions);
+        return array(null, null);
+      } catch (Exception $ex) {
+        $errors[] = $ex->getMessage();
+      }
+    }
+
+    // mutate current rule, so it would be sent to the client in the right state
+    $rule->setMustMatchAll((int)$match_all);
+    $rule->setName($new_name);
+    $rule->setRepetitionPolicy(
+      HeraldRepetitionPolicyConfig::toInt($repetition_policy_param));
     $rule->attachConditions($conditions);
     $rule->attachActions($actions);
 
-    if (!$errors) {
-      $edit_action = $rule->getID() ? 'edit' : 'create';
-
-      $rule->openTransaction();
-        $rule->save();
-        $rule->saveConditions($conditions);
-        $rule->saveActions($actions);
-      $rule->saveTransaction();
-    }
-
     return array($e_name, $errors);
   }
 

src/applications/herald/editor/HeraldRuleEditor.php

@@ -15,6 +15,8 @@ final class HeraldRuleEditor
     $types = parent::getTransactionTypes();
 
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
+    $types[] = HeraldRuleTransaction::TYPE_EDIT;
+    $types[] = HeraldRuleTransaction::TYPE_NAME;
     $types[] = HeraldRuleTransaction::TYPE_DISABLE;
 
     return $types;
@@ -27,6 +29,11 @@ final class HeraldRuleEditor
     switch ($xaction->getTransactionType()) {
       case HeraldRuleTransaction::TYPE_DISABLE:
         return (int)$object->getIsDisabled();
+      case HeraldRuleTransaction::TYPE_EDIT:
+        return id(new HeraldRuleSerializer())
+          ->serializeRule($object);
+      case HeraldRuleTransaction::TYPE_NAME:
+        return $object->getName();
     }
 
   }
@@ -38,8 +45,10 @@ final class HeraldRuleEditor
     switch ($xaction->getTransactionType()) {
       case HeraldRuleTransaction::TYPE_DISABLE:
         return (int)$xaction->getNewValue();
+      case HeraldRuleTransaction::TYPE_EDIT:
+      case HeraldRuleTransaction::TYPE_NAME:
+        return $xaction->getNewValue();
     }
-
   }
 
   protected function applyCustomInternalTransaction(
@@ -49,6 +58,17 @@ final class HeraldRuleEditor
     switch ($xaction->getTransactionType()) {
       case HeraldRuleTransaction::TYPE_DISABLE:
         return $object->setIsDisabled($xaction->getNewValue());
+      case HeraldRuleTransaction::TYPE_NAME:
+        return $object->setName($xaction->getNewValue());
+      case HeraldRuleTransaction::TYPE_EDIT:
+        $new_state = id(new HeraldRuleSerializer())
+          ->deserializeRuleComponents($xaction->getNewValue());
+        $object->setMustMatchAll((int)$new_state['match_all']);
+        $object->attachConditions($new_state['conditions']);
+        $object->attachActions($new_state['actions']);
+        $object->setRepetitionPolicy(
+          HeraldRepetitionPolicyConfig::toInt($new_state['repetition_policy']));
+        return $object;
     }
 
   }
@@ -56,6 +76,12 @@ final class HeraldRuleEditor
   protected function applyCustomExternalTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
+    switch ($xaction->getTransactionType()) {
+      case HeraldRuleTransaction::TYPE_EDIT:
+        $object->saveConditions($object->getConditions());
+        $object->saveActions($object->getActions());
+        break;
+    }
     return;
   }
 

src/applications/herald/editor/HeraldRuleSerializer.php

@@ -0,0 +1,73 @@
+<?php
+
+/**
+ * Serialize for RuleTransactions / Editor.
+ */
+final class HeraldRuleSerializer extends Phobject {
+  public function serializeRule(HeraldRule $rule) {
+    return $this->serializeRuleComponents(
+      (bool)$rule->getMustMatchAll(),
+      $rule->getConditions(),
+      $rule->getActions(),
+      HeraldRepetitionPolicyConfig::toString($rule->getRepetitionPolicy()));
+  }
+
+  public function serializeRuleComponents(
+    $match_all,
+    array $conditions,
+    array $actions,
+    $repetition_policy) {
+
+    assert_instances_of($conditions, 'HeraldCondition');
+    assert_instances_of($actions, 'HeraldActionRecord');
+
+    $conditions_array = array();
+    foreach ($conditions as $condition) {
+      $conditions_array[] = array(
+        'field' => $condition->getFieldName(),
+        'condition' => $condition->getFieldCondition(),
+        'value' => $condition->getValue(),
+      );
+    }
+
+    $actions_array = array();
+    foreach ($actions as $action) {
+      $actions_array[] = array(
+        'action' => $action->getAction(),
+        'target' => $action->getTarget(),
+      );
+    }
+
+    return array(
+      'match_all' => $match_all,
+      'conditions' => $conditions_array,
+      'actions' => $actions_array,
+      'repetition_policy' => $repetition_policy,
+    );
+  }
+
+  public function deserializeRuleComponents(array $serialized) {
+    $deser_conditions = array();
+    foreach ($serialized['conditions'] as $condition) {
+      $deser_conditions[] = id(new HeraldCondition())
+        ->setFieldName($condition['field'])
+        ->setFieldCondition($condition['condition'])
+        ->setValue($condition['value']);
+    }
+
+    $deser_actions = array();
+    foreach ($serialized['actions'] as $action) {
+      $deser_actions[] = id(new HeraldActionRecord())
+        ->setAction($action['action'])
+        ->setTarget($action['target']);
+    }
+
+    return array(
+      'match_all' => $serialized['match_all'],
+      'conditions' => $deser_conditions,
+      'actions' => $deser_actions,
+      'repetition_policy' => $serialized['repetition_policy'],
+    );
+  }
+
+}

src/applications/herald/storage/HeraldRuleTransaction.php

@@ -4,6 +4,7 @@ final class HeraldRuleTransaction
   extends PhabricatorApplicationTransaction {
 
   const TYPE_EDIT = 'herald:edit';
+  const TYPE_NAME = 'herald:name';
   const TYPE_DISABLE = 'herald:disable';
 
   public function getApplicationName() {
@@ -45,6 +46,8 @@ final class HeraldRuleTransaction
         } else {
           return pht('Enabled');
         }
+      case self::TYPE_NAME:
+        return pht('Renamed');
     }
 
     return parent::getActionName();
@@ -84,9 +87,49 @@ final class HeraldRuleTransaction
             '%s enabled this rule.',
             $this->renderHandleLink($author_phid));
         }
+      case self::TYPE_NAME:
+        if ($old == null) {
+          return pht(
+            '%s created this rule.',
+            $this->renderHandleLink($author_phid));
+        } else {
+          return pht(
+            '%s renamed this rule from "%s" to "%s".',
+            $this->renderHandleLink($author_phid),
+            $old,
+            $new);
+        }
+      case self::TYPE_EDIT:
+        return pht(
+          '%s edited this rule.',
+          $this->renderHandleLink($author_phid));
     }
 
     return parent::getTitle();
   }
 
+  public function hasChangeDetails() {
+    switch ($this->getTransactionType()) {
+      case self::TYPE_EDIT:
+        return true;
+    }
+    return parent::hasChangeDetails();
+  }
+
+  public function renderChangeDetails(PhabricatorUser $viewer) {
+    $json = new PhutilJSON();
+    switch ($this->getTransactionType()) {
+      case self::TYPE_EDIT:
+        return $this->renderTextCorpusChangeDetails(
+          $viewer,
+          $json->encodeFormatted($this->getOldValue()),
+          $json->encodeFormatted($this->getNewValue()));
+    }
+
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
+  }
+
 }