Modularize HeraldRule transactions

Summary:
Ref T13249. See PHI1115. I initially wanted to make `bin/policy unlock --owner <user> H123` work to transfer ownership of a Herald rule, although I'm no longer really sure this makes much sense.

In any case, this makes things a little better and more modern.

I removed the storage table for rule comments. Adding comments to Herald rules doesn't work and probably doesn't make much sense.

Test Plan: Created and edited Herald rules, grepped for all the transaction type constants.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13249

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

resources/sql/autopatches/20190307.herald.01.comments.sql

@@ -0,0 +1 @@
+DROP TABLE {$NAMESPACE}_herald.herald_ruletransaction_comment;

src/__phutil_library_map__.php

@@ -1535,10 +1535,13 @@ phutil_register_library_map(array(
     'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php',
     'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php',
     'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php',
+    'HeraldRuleDisableTransaction' => 'applications/herald/xaction/HeraldRuleDisableTransaction.php',
+    'HeraldRuleEditTransaction' => 'applications/herald/xaction/HeraldRuleEditTransaction.php',
     'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php',
     'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php',
     'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php',
     'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php',
+    'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php',
     'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php',
     'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php',
     'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php',
@@ -1546,7 +1549,7 @@ phutil_register_library_map(array(
     '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',
+    'HeraldRuleTransactionType' => 'applications/herald/xaction/HeraldRuleTransactionType.php',
     'HeraldRuleTranscript' => 'applications/herald/storage/transcript/HeraldRuleTranscript.php',
     'HeraldRuleTypeConfig' => 'applications/herald/config/HeraldRuleTypeConfig.php',
     'HeraldRuleTypeDatasource' => 'applications/herald/typeahead/HeraldRuleTypeDatasource.php',
@@ -7188,18 +7191,21 @@ phutil_register_library_map(array(
     'HeraldRuleAdapterField' => 'HeraldRuleField',
     'HeraldRuleController' => 'HeraldController',
     'HeraldRuleDatasource' => 'PhabricatorTypeaheadDatasource',
+    'HeraldRuleDisableTransaction' => 'HeraldRuleTransactionType',
+    'HeraldRuleEditTransaction' => 'HeraldRuleTransactionType',
     'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor',
     'HeraldRuleField' => 'HeraldField',
     'HeraldRuleFieldGroup' => 'HeraldFieldGroup',
     'HeraldRuleListController' => 'HeraldController',
+    'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType',
     'HeraldRulePHIDType' => 'PhabricatorPHIDType',
     'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler',
     'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'HeraldRuleSerializer' => 'Phobject',
     'HeraldRuleTestCase' => 'PhabricatorTestCase',
-    'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction',
-    'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment',
+    'HeraldRuleTransaction' => 'PhabricatorModularTransaction',
+    'HeraldRuleTransactionType' => 'PhabricatorModularTransactionType',
     'HeraldRuleTranscript' => 'Phobject',
     'HeraldRuleTypeConfig' => 'Phobject',
     'HeraldRuleTypeDatasource' => 'PhabricatorTypeaheadDatasource',

src/applications/herald/controller/HeraldDisableController.php

@@ -31,7 +31,7 @@ final class HeraldDisableController extends HeraldController {
 
     if ($request->isFormPost()) {
       $xaction = id(new HeraldRuleTransaction())
-        ->setTransactionType(HeraldRuleTransaction::TYPE_DISABLE)
+        ->setTransactionType(HeraldRuleDisableTransaction::TRANSACTIONTYPE)
         ->setNewValue($is_disable);
 
       id(new HeraldRuleEditor())

src/applications/herald/controller/HeraldRuleController.php

@@ -359,11 +359,21 @@ final class HeraldRuleController extends HeraldController {
         $repetition_policy);
 
       $xactions = array();
+
+      // Until this moves to EditEngine, manually add a "CREATE" transaction
+      // if we're creating a new rule. This improves rendering of the initial
+      // group of transactions.
+      $is_new = (bool)(!$rule->getID());
+      if ($is_new) {
+        $xactions[] = id(new HeraldRuleTransaction())
+          ->setTransactionType(PhabricatorTransactions::TYPE_CREATE);
+      }
+
       $xactions[] = id(new HeraldRuleTransaction())
-        ->setTransactionType(HeraldRuleTransaction::TYPE_EDIT)
+        ->setTransactionType(HeraldRuleEditTransaction::TRANSACTIONTYPE)
         ->setNewValue($new_state);
       $xactions[] = id(new HeraldRuleTransaction())
-        ->setTransactionType(HeraldRuleTransaction::TYPE_NAME)
+        ->setTransactionType(HeraldRuleNameTransaction::TRANSACTIONTYPE)
         ->setNewValue($new_name);
 
       try {

src/applications/herald/editor/HeraldRuleEditor.php

@@ -11,82 +11,6 @@ final class HeraldRuleEditor
     return pht('Herald Rules');
   }
 
-  public function getTransactionTypes() {
-    $types = parent::getTransactionTypes();
-
-    $types[] = PhabricatorTransactions::TYPE_COMMENT;
-    $types[] = HeraldRuleTransaction::TYPE_EDIT;
-    $types[] = HeraldRuleTransaction::TYPE_NAME;
-    $types[] = HeraldRuleTransaction::TYPE_DISABLE;
-
-    return $types;
-  }
-
-  protected function getCustomTransactionOldValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    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();
-    }
-
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    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(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    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']);
-
-        $new_repetition = $new_state['repetition_policy'];
-        $object->setRepetitionPolicyStringConstant($new_repetition);
-
-        return $object;
-    }
-
-  }
-
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-    switch ($xaction->getTransactionType()) {
-      case HeraldRuleTransaction::TYPE_EDIT:
-        $object->saveConditions($object->getConditions());
-        $object->saveActions($object->getActions());
-        break;
-    }
-    return;
-  }
-
   protected function shouldApplyHeraldRules(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -137,7 +61,6 @@ final class HeraldRuleEditor
     return pht('[Herald]');
   }
 
-
   protected function buildMailBody(
     PhabricatorLiskDAO $object,
     array $xactions) {

src/applications/herald/storage/HeraldRuleTransaction.php

@@ -1,11 +1,9 @@
 <?php
 
 final class HeraldRuleTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
 
   const TYPE_EDIT = 'herald:edit';
-  const TYPE_NAME = 'herald:name';
-  const TYPE_DISABLE = 'herald:disable';
 
   public function getApplicationName() {
     return 'herald';
@@ -15,121 +13,8 @@ final class HeraldRuleTransaction
     return HeraldRulePHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return new HeraldRuleTransactionComment();
-  }
-
-  public function getColor() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DISABLE:
-        if ($new) {
-          return 'red';
-        } else {
-          return 'green';
-        }
-    }
-
-    return parent::getColor();
-  }
-
-  public function getActionName() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DISABLE:
-        if ($new) {
-          return pht('Disabled');
-        } else {
-          return pht('Enabled');
-        }
-      case self::TYPE_NAME:
-        return pht('Renamed');
-    }
-
-    return parent::getActionName();
-  }
-
-  public function getIcon() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DISABLE:
-        if ($new) {
-          return 'fa-ban';
-        } else {
-          return 'fa-check';
-        }
-    }
-
-    return parent::getIcon();
-  }
-
-
-  public function getTitle() {
-    $author_phid = $this->getAuthorPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DISABLE:
-        if ($new) {
-          return pht(
-            '%s disabled this rule.',
-            $this->renderHandleLink($author_phid));
-        } else {
-          return pht(
-            '%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());
+  public function getBaseTransactionClass() {
+    return 'HeraldRuleTransactionType';
   }
 
 }

src/applications/herald/storage/HeraldRuleTransactionComment.php

@@ -1,10 +0,0 @@
-<?php
-
-final class HeraldRuleTransactionComment
-  extends PhabricatorApplicationTransactionComment {
-
-  public function getApplicationTransactionObject() {
-    return new HeraldRuleTransaction();
-  }
-
-}

src/applications/herald/xaction/HeraldRuleDisableTransaction.php

@@ -0,0 +1,32 @@
+<?php
+
+final class HeraldRuleDisableTransaction
+  extends HeraldRuleTransactionType {
+
+  const TRANSACTIONTYPE = 'herald:disable';
+
+  public function generateOldValue($object) {
+    return (bool)$object->getIsDisabled();
+  }
+
+  public function generateNewValue($object, $value) {
+    return (bool)$value;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setIsDisabled((int)$value);
+  }
+
+  public function getTitle() {
+    if ($this->getNewValue()) {
+      return pht(
+        '%s disabled this rule.',
+        $this->renderAuthor());
+    } else {
+      return pht(
+        '%s enabled this rule.',
+        $this->renderAuthor());
+    }
+  }
+
+}

src/applications/herald/xaction/HeraldRuleEditTransaction.php

@@ -0,0 +1,56 @@
+<?php
+
+final class HeraldRuleEditTransaction
+  extends HeraldRuleTransactionType {
+
+  const TRANSACTIONTYPE = 'herald:edit';
+
+  public function generateOldValue($object) {
+    return id(new HeraldRuleSerializer())
+      ->serializeRule($object);
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $new_state = id(new HeraldRuleSerializer())
+      ->deserializeRuleComponents($value);
+
+    $object->setMustMatchAll((int)$new_state['match_all']);
+    $object->attachConditions($new_state['conditions']);
+    $object->attachActions($new_state['actions']);
+
+    $new_repetition = $new_state['repetition_policy'];
+    $object->setRepetitionPolicyStringConstant($new_repetition);
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $object->saveConditions($object->getConditions());
+    $object->saveActions($object->getActions());
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s edited this rule.',
+      $this->renderAuthor());
+  }
+
+  public function hasChangeDetailView() {
+    return true;
+  }
+
+  public function newChangeDetailView() {
+    $viewer = $this->getViewer();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $json = new PhutilJSON();
+    $old_json = $json->encodeFormatted($old);
+    $new_json = $json->encodeFormatted($new);
+
+    return id(new PhabricatorApplicationTransactionTextDiffDetailView())
+      ->setViewer($viewer)
+      ->setOldText($old_json)
+      ->setNewText($new_json);
+  }
+
+}

src/applications/herald/xaction/HeraldRuleNameTransaction.php

@@ -0,0 +1,48 @@
+<?php
+
+final class HeraldRuleNameTransaction
+  extends HeraldRuleTransactionType {
+
+  const TRANSACTIONTYPE = 'herald:name';
+
+  public function generateOldValue($object) {
+    return $object->getName();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setName($value);
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s renamed this rule from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
+      $errors[] = $this->newRequiredError(
+        pht('Rules must have a name.'));
+    }
+
+    $max_length = $object->getColumnMaximumByteLength('name');
+    foreach ($xactions as $xaction) {
+      $new_value = $xaction->getNewValue();
+
+      $new_length = strlen($new_value);
+      if ($new_length > $max_length) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Rule names can be no longer than %s characters.',
+            new PhutilNumber($max_length)));
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/herald/xaction/HeraldRuleTransactionType.php

@@ -0,0 +1,4 @@
+<?php
+
+abstract class HeraldRuleTransactionType
+  extends PhabricatorModularTransactionType {}