Modularize workboard column transactions

Summary: Depends on D20279. Ref T5474. Modernize these transactions before I add a new "TriggerTransaction" for setting triggers.

Test Plan: Created a column. Edited a column name and point limit. Hid and un-hid a column. Grepped for removed symbols.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5474

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

src/__phutil_library_map__.php

@@ -4059,6 +4059,8 @@ phutil_register_library_map(array(
     'PhabricatorProjectColumnEditController' => 'applications/project/controller/PhabricatorProjectColumnEditController.php',
     'PhabricatorProjectColumnHeader' => 'applications/project/order/PhabricatorProjectColumnHeader.php',
     'PhabricatorProjectColumnHideController' => 'applications/project/controller/PhabricatorProjectColumnHideController.php',
+    'PhabricatorProjectColumnLimitTransaction' => 'applications/project/xaction/column/PhabricatorProjectColumnLimitTransaction.php',
+    'PhabricatorProjectColumnNameTransaction' => 'applications/project/xaction/column/PhabricatorProjectColumnNameTransaction.php',
     'PhabricatorProjectColumnNaturalOrder' => 'applications/project/order/PhabricatorProjectColumnNaturalOrder.php',
     'PhabricatorProjectColumnOrder' => 'applications/project/order/PhabricatorProjectColumnOrder.php',
     'PhabricatorProjectColumnOwnerOrder' => 'applications/project/order/PhabricatorProjectColumnOwnerOrder.php',
@@ -4070,10 +4072,12 @@ phutil_register_library_map(array(
     'PhabricatorProjectColumnQuery' => 'applications/project/query/PhabricatorProjectColumnQuery.php',
     'PhabricatorProjectColumnSearchEngine' => 'applications/project/query/PhabricatorProjectColumnSearchEngine.php',
     'PhabricatorProjectColumnStatusOrder' => 'applications/project/order/PhabricatorProjectColumnStatusOrder.php',
+    'PhabricatorProjectColumnStatusTransaction' => 'applications/project/xaction/column/PhabricatorProjectColumnStatusTransaction.php',
     'PhabricatorProjectColumnTitleOrder' => 'applications/project/order/PhabricatorProjectColumnTitleOrder.php',
     'PhabricatorProjectColumnTransaction' => 'applications/project/storage/PhabricatorProjectColumnTransaction.php',
     'PhabricatorProjectColumnTransactionEditor' => 'applications/project/editor/PhabricatorProjectColumnTransactionEditor.php',
     'PhabricatorProjectColumnTransactionQuery' => 'applications/project/query/PhabricatorProjectColumnTransactionQuery.php',
+    'PhabricatorProjectColumnTransactionType' => 'applications/project/xaction/column/PhabricatorProjectColumnTransactionType.php',
     'PhabricatorProjectConfigOptions' => 'applications/project/config/PhabricatorProjectConfigOptions.php',
     'PhabricatorProjectConfiguredCustomField' => 'applications/project/customfield/PhabricatorProjectConfiguredCustomField.php',
     'PhabricatorProjectController' => 'applications/project/controller/PhabricatorProjectController.php',
@@ -10166,6 +10170,8 @@ phutil_register_library_map(array(
     'PhabricatorProjectColumnEditController' => 'PhabricatorProjectBoardController',
     'PhabricatorProjectColumnHeader' => 'Phobject',
     'PhabricatorProjectColumnHideController' => 'PhabricatorProjectBoardController',
+    'PhabricatorProjectColumnLimitTransaction' => 'PhabricatorProjectColumnTransactionType',
+    'PhabricatorProjectColumnNameTransaction' => 'PhabricatorProjectColumnTransactionType',
     'PhabricatorProjectColumnNaturalOrder' => 'PhabricatorProjectColumnOrder',
     'PhabricatorProjectColumnOrder' => 'Phobject',
     'PhabricatorProjectColumnOwnerOrder' => 'PhabricatorProjectColumnOrder',
@@ -10180,10 +10186,12 @@ phutil_register_library_map(array(
     'PhabricatorProjectColumnQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorProjectColumnSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorProjectColumnStatusOrder' => 'PhabricatorProjectColumnOrder',
+    'PhabricatorProjectColumnStatusTransaction' => 'PhabricatorProjectColumnTransactionType',
     'PhabricatorProjectColumnTitleOrder' => 'PhabricatorProjectColumnOrder',
-    'PhabricatorProjectColumnTransaction' => 'PhabricatorApplicationTransaction',
+    'PhabricatorProjectColumnTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorProjectColumnTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorProjectColumnTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorProjectColumnTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorProjectConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorProjectConfiguredCustomField' => array(
       'PhabricatorProjectStandardCustomField',

src/applications/project/controller/PhabricatorProjectColumnEditController.php

@@ -76,8 +76,8 @@ final class PhabricatorProjectColumnEditController
 
       $xactions = array();
 
-      $type_name = PhabricatorProjectColumnTransaction::TYPE_NAME;
+      $type_name = PhabricatorProjectColumnNameTransaction::TRANSACTIONTYPE;
-      $type_limit = PhabricatorProjectColumnTransaction::TYPE_LIMIT;
+      $type_limit = PhabricatorProjectColumnLimitTransaction::TRANSACTIONTYPE;
 
       if (!$column->getProxy()) {
         $xactions[] = id(new PhabricatorProjectColumnTransaction())
@@ -93,7 +93,6 @@ final class PhabricatorProjectColumnEditController
         $editor = id(new PhabricatorProjectColumnTransactionEditor())
           ->setActor($viewer)
           ->setContinueOnNoEffect(true)
-          ->setContinueOnMissingFields(true)
           ->setContentSourceFromRequest($request)
           ->applyTransactions($column, $xactions);
         return id(new AphrontRedirectResponse())->setURI($view_uri);

src/applications/project/controller/PhabricatorProjectColumnHideController.php

@@ -82,7 +82,9 @@ final class PhabricatorProjectColumnHideController
           $new_status = PhabricatorProjectColumn::STATUS_HIDDEN;
         }
 
-        $type_status = PhabricatorProjectColumnTransaction::TYPE_STATUS;
+        $type_status =
+          PhabricatorProjectColumnStatusTransaction::TRANSACTIONTYPE;
+
         $xactions = array(
           id(new PhabricatorProjectColumnTransaction())
             ->setTransactionType($type_status)

src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php

@@ -11,130 +11,12 @@ final class PhabricatorProjectColumnTransactionEditor
     return pht('Workboard Columns');
   }
 
-  public function getTransactionTypes() {
+  public function getCreateObjectTitle($author, $object) {
-    $types = parent::getTransactionTypes();
+    return pht('%s created this column.', $author);
-
-    $types[] = PhabricatorProjectColumnTransaction::TYPE_NAME;
-    $types[] = PhabricatorProjectColumnTransaction::TYPE_STATUS;
-    $types[] = PhabricatorProjectColumnTransaction::TYPE_LIMIT;
-
-    return $types;
   }
 
-  protected function getCustomTransactionOldValue(
+  public function getCreateObjectTitleForFeed($author, $object) {
-    PhabricatorLiskDAO $object,
+    return pht('%s created %s.', $author, $object);
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectColumnTransaction::TYPE_NAME:
-        return $object->getName();
-      case PhabricatorProjectColumnTransaction::TYPE_STATUS:
-        return $object->getStatus();
-      case PhabricatorProjectColumnTransaction::TYPE_LIMIT:
-        return $object->getPointLimit();
-
-    }
-
-    return parent::getCustomTransactionOldValue($object, $xaction);
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectColumnTransaction::TYPE_NAME:
-      case PhabricatorProjectColumnTransaction::TYPE_STATUS:
-        return $xaction->getNewValue();
-      case PhabricatorProjectColumnTransaction::TYPE_LIMIT:
-        $value = $xaction->getNewValue();
-        if (strlen($value)) {
-          return (int)$xaction->getNewValue();
-        } else {
-          return null;
-        }
-    }
-
-    return parent::getCustomTransactionNewValue($object, $xaction);
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectColumnTransaction::TYPE_NAME:
-        $object->setName($xaction->getNewValue());
-        return;
-      case PhabricatorProjectColumnTransaction::TYPE_STATUS:
-        $object->setStatus($xaction->getNewValue());
-        return;
-      case PhabricatorProjectColumnTransaction::TYPE_LIMIT:
-        $object->setPointLimit($xaction->getNewValue());
-        return;
-    }
-
-    return parent::applyCustomInternalTransaction($object, $xaction);
-  }
-
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectColumnTransaction::TYPE_NAME:
-      case PhabricatorProjectColumnTransaction::TYPE_STATUS:
-      case PhabricatorProjectColumnTransaction::TYPE_LIMIT:
-        return;
-    }
-
-    return parent::applyCustomExternalTransaction($object, $xaction);
-  }
-
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    switch ($type) {
-      case PhabricatorProjectColumnTransaction::TYPE_LIMIT:
-        foreach ($xactions as $xaction) {
-          $value = $xaction->getNewValue();
-          if (strlen($value) && !preg_match('/^\d+\z/', $value)) {
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht(
-                'Column point limit must either be empty or a nonnegative '.
-                'integer.'),
-              $xaction);
-          }
-        }
-        break;
-      case PhabricatorProjectColumnTransaction::TYPE_NAME:
-        $missing = $this->validateIsEmptyTextField(
-          $object->getName(),
-          $xactions);
-
-        // The default "Backlog" column is allowed to be unnamed, which
-        // means we use the default name.
-
-        if ($missing && !$object->isDefaultColumn()) {
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Required'),
-            pht('Column name is required.'),
-            nonempty(last($xactions), null));
-
-          $error->setIsMissingFieldError(true);
-          $errors[] = $error;
-        }
-        break;
-    }
-
-    return $errors;
   }
 
 }

src/applications/project/storage/PhabricatorProjectColumnTransaction.php

@@ -1,11 +1,7 @@
 <?php
 
 final class PhabricatorProjectColumnTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
-
-  const TYPE_NAME       = 'project:col:name';
-  const TYPE_STATUS     = 'project:col:status';
-  const TYPE_LIMIT      = 'project:col:limit';
 
   public function getApplicationName() {
     return 'project';
@@ -15,68 +11,8 @@ final class PhabricatorProjectColumnTransaction
     return PhabricatorProjectColumnPHIDType::TYPECONST;
   }
 
-  public function getTitle() {
+  public function getBaseTransactionClass() {
-    $old = $this->getOldValue();
+    return 'PhabricatorProjectColumnTransactionType';
-    $new = $this->getNewValue();
-    $author_handle = $this->renderHandleLink($this->getAuthorPHID());
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-        if ($old === null) {
-          return pht(
-            '%s created this column.',
-            $author_handle);
-        } else {
-          if (!strlen($old)) {
-            return pht(
-              '%s named this column "%s".',
-              $author_handle,
-              $new);
-          } else if (strlen($new)) {
-            return pht(
-              '%s renamed this column from "%s" to "%s".',
-              $author_handle,
-              $old,
-              $new);
-          } else {
-            return pht(
-              '%s removed the custom name of this column.',
-              $author_handle);
-          }
-        }
-      case self::TYPE_LIMIT:
-        if (!$old) {
-          return pht(
-            '%s set the point limit for this column to %s.',
-            $author_handle,
-            $new);
-        } else if (!$new) {
-          return pht(
-            '%s removed the point limit for this column.',
-            $author_handle);
-        } else {
-          return pht(
-            '%s changed point limit for this column from %s to %s.',
-            $author_handle,
-            $old,
-            $new);
-        }
-
-      case self::TYPE_STATUS:
-        switch ($new) {
-          case PhabricatorProjectColumn::STATUS_ACTIVE:
-            return pht(
-              '%s marked this column visible.',
-              $author_handle);
-          case PhabricatorProjectColumn::STATUS_HIDDEN:
-            return pht(
-              '%s marked this column hidden.',
-              $author_handle);
-        }
-        break;
-    }
-
-    return parent::getTitle();
   }
 
 }

src/applications/project/xaction/column/PhabricatorProjectColumnLimitTransaction.php

@@ -0,0 +1,63 @@
+<?php
+
+final class PhabricatorProjectColumnLimitTransaction
+  extends PhabricatorProjectColumnTransactionType {
+
+  const TRANSACTIONTYPE = 'project:col:limit';
+
+  public function generateOldValue($object) {
+    return $object->getPointLimit();
+  }
+
+  public function generateNewValue($object, $value) {
+    if (strlen($value)) {
+      return (int)$value;
+    } else {
+      return null;
+    }
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setPointLimit($value);
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    if (!$old) {
+      return pht(
+        '%s set the point limit for this column to %s.',
+        $this->renderAuthor(),
+        $this->renderNewValue());
+    } else if (!$new) {
+      return pht(
+        '%s removed the point limit for this column.',
+        $this->renderAuthor());
+    } else {
+      return pht(
+        '%s changed the point limit for this column from %s to %s.',
+        $this->renderAuthor(),
+        $this->renderOldValue(),
+        $this->renderNewValue());
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $value = $xaction->getNewValue();
+      if (strlen($value) && !preg_match('/^\d+\z/', $value)) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Column point limit must either be empty or a nonnegative '.
+            'integer.'),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/project/xaction/column/PhabricatorProjectColumnNameTransaction.php

@@ -0,0 +1,66 @@
+<?php
+
+final class PhabricatorProjectColumnNameTransaction
+  extends PhabricatorProjectColumnTransactionType {
+
+  const TRANSACTIONTYPE = 'project:col:name';
+
+  public function generateOldValue($object) {
+    return $object->getName();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setName($value);
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    if (!strlen($old)) {
+      return pht(
+        '%s named this column %s.',
+        $this->renderAuthor(),
+        $this->renderNewValue());
+    } else if (strlen($new)) {
+      return pht(
+        '%s renamed this column from %s to %s.',
+        $this->renderAuthor(),
+        $this->renderOldValue(),
+        $this->renderNewValue());
+    } else {
+      return pht(
+        '%s removed the custom name of this column.',
+        $this->renderAuthor());
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
+      // The default "Backlog" column is allowed to be unnamed, which
+      // means we use the default name.
+      if (!$object->isDefaultColumn()) {
+        $errors[] = $this->newRequiredError(
+          pht('Columns 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(
+            'Column names must not be longer than %s characters.',
+            new PhutilNumber($max_length)),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/project/xaction/column/PhabricatorProjectColumnStatusTransaction.php

@@ -0,0 +1,55 @@
+<?php
+
+final class PhabricatorProjectColumnStatusTransaction
+  extends PhabricatorProjectColumnTransactionType {
+
+  const TRANSACTIONTYPE = 'project:col:status';
+
+  public function generateOldValue($object) {
+    return $object->getStatus();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setStatus($value);
+  }
+
+  public function getTitle() {
+    $new = $this->getNewValue();
+
+    switch ($new) {
+      case PhabricatorProjectColumn::STATUS_ACTIVE:
+        return pht(
+          '%s unhid this column.',
+          $this->renderAuthor());
+      case PhabricatorProjectColumn::STATUS_HIDDEN:
+        return pht(
+          '%s hid this column.',
+          $this->renderAuthor());
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $map = array(
+      PhabricatorProjectColumn::STATUS_ACTIVE,
+      PhabricatorProjectColumn::STATUS_HIDDEN,
+    );
+    $map = array_fuse($map);
+
+    foreach ($xactions as $xaction) {
+      $value = $xaction->getNewValue();
+      if (!isset($map[$value])) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Column status "%s" is unrecognized, valid statuses are: %s.',
+            $value,
+            implode(', ', array_keys($map))),
+          $xaction);
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/project/xaction/column/PhabricatorProjectColumnTransactionType.php

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