Migrate Project names to modular transactions

Summary: Also changes access modifiers on `PhabricatorProjectTransactionEditor` and sets up `storage` for `applyExternalEffects`.

Test Plan: Created new projects, attempted to create without name, with too long of a name, and with a name that conflicts with other projects and observed expected errors.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T12673

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

resources/sql/patches/20131020.pxactionmig.php

@@ -32,7 +32,7 @@ foreach ($rows as $row) {
   $project_phid = $project_row['phid'];
 
   $type_map = array(
-    'name' => PhabricatorProjectTransaction::TYPE_NAME,
+    'name' => PhabricatorProjectNameTransaction::TRANSACTIONTYPE,
     'members' => PhabricatorProjectTransaction::TYPE_MEMBERS,
     'status' => PhabricatorProjectTransaction::TYPE_STATUS,
     'canview' => PhabricatorTransactions::TYPE_VIEW_POLICY,

src/__phutil_library_map__.php

@@ -3644,6 +3644,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectMenuItemController' => 'applications/project/controller/PhabricatorProjectMenuItemController.php',
     'PhabricatorProjectMoveController' => 'applications/project/controller/PhabricatorProjectMoveController.php',
     'PhabricatorProjectNameContextFreeGrammar' => 'applications/project/lipsum/PhabricatorProjectNameContextFreeGrammar.php',
+    'PhabricatorProjectNameTransaction' => 'applications/project/xaction/PhabricatorProjectNameTransaction.php',
     'PhabricatorProjectNoProjectsDatasource' => 'applications/project/typeahead/PhabricatorProjectNoProjectsDatasource.php',
     'PhabricatorProjectObjectHasProjectEdgeType' => 'applications/project/edge/PhabricatorProjectObjectHasProjectEdgeType.php',
     'PhabricatorProjectOrUserDatasource' => 'applications/project/typeahead/PhabricatorProjectOrUserDatasource.php',
@@ -3674,6 +3675,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectTransaction' => 'applications/project/storage/PhabricatorProjectTransaction.php',
     'PhabricatorProjectTransactionEditor' => 'applications/project/editor/PhabricatorProjectTransactionEditor.php',
     'PhabricatorProjectTransactionQuery' => 'applications/project/query/PhabricatorProjectTransactionQuery.php',
+    'PhabricatorProjectTransactionType' => 'applications/project/xaction/PhabricatorProjectTransactionType.php',
     'PhabricatorProjectUIEventListener' => 'applications/project/events/PhabricatorProjectUIEventListener.php',
     'PhabricatorProjectUpdateController' => 'applications/project/controller/PhabricatorProjectUpdateController.php',
     'PhabricatorProjectUserFunctionDatasource' => 'applications/project/typeahead/PhabricatorProjectUserFunctionDatasource.php',
@@ -9053,6 +9055,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectMenuItemController' => 'PhabricatorProjectController',
     'PhabricatorProjectMoveController' => 'PhabricatorProjectController',
     'PhabricatorProjectNameContextFreeGrammar' => 'PhutilContextFreeGrammar',
+    'PhabricatorProjectNameTransaction' => 'PhabricatorProjectTransactionType',
     'PhabricatorProjectNoProjectsDatasource' => 'PhabricatorTypeaheadDatasource',
     'PhabricatorProjectObjectHasProjectEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorProjectOrUserDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
@@ -9083,9 +9086,10 @@ phutil_register_library_map(array(
     'PhabricatorProjectSubprojectsController' => 'PhabricatorProjectController',
     'PhabricatorProjectSubprojectsProfileMenuItem' => 'PhabricatorProfileMenuItem',
     'PhabricatorProjectTestDataGenerator' => 'PhabricatorTestDataGenerator',
-    'PhabricatorProjectTransaction' => 'PhabricatorApplicationTransaction',
+    'PhabricatorProjectTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorProjectTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorProjectTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorProjectTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorProjectUIEventListener' => 'PhabricatorEventListener',
     'PhabricatorProjectUpdateController' => 'PhabricatorProjectController',
     'PhabricatorProjectUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -356,7 +356,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
 
     $xactions = array();
     $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
       ->setNewValue($name);
     $this->applyTransactions($project, $user, $xactions);
 
@@ -382,7 +382,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $xactions = array();
 
     $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
       ->setNewValue($name2);
 
     $xactions[] = id(new PhabricatorProjectTransaction())
@@ -503,7 +503,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $xactions = array();
 
     $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
       ->setNewValue($name);
 
     $xactions[] = id(new PhabricatorProjectTransaction())
@@ -601,7 +601,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $xactions = array();
 
     $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
       ->setNewValue($name);
 
     $xactions[] = id(new PhabricatorProjectTransaction())
@@ -1290,7 +1290,8 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $new_name = $proj->getName().' '.mt_rand();
 
     $xaction = new PhabricatorProjectTransaction();
-    $xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME);
+    $xaction->setTransactionType(
+      PhabricatorProjectNameTransaction::TRANSACTIONTYPE);
     $xaction->setNewValue($new_name);
 
     $this->applyTransactions($proj, $user, array($xaction));
@@ -1440,7 +1441,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $xactions = array();
 
     $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
       ->setNewValue($name);
 
     if ($parent) {

src/applications/project/conduit/ProjectCreateConduitAPIMethod.php

@@ -42,7 +42,7 @@ final class ProjectCreateConduitAPIMethod extends ProjectConduitAPIMethod {
       $user);
 
     $project = PhabricatorProject::initializeNewProject($user);
-    $type_name = PhabricatorProjectTransaction::TYPE_NAME;
+    $type_name = PhabricatorProjectNameTransaction::TRANSACTIONTYPE;
     $members = $request->getValue('members');
     $xactions = array();
 

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -10,7 +10,7 @@ final class PhabricatorProjectTransactionEditor
     return $this;
   }
 
-  private function getIsMilestone() {
+  public function getIsMilestone() {
     return $this->isMilestone;
   }
 
@@ -30,7 +30,6 @@ final class PhabricatorProjectTransactionEditor
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
     $types[] = PhabricatorTransactions::TYPE_JOIN_POLICY;
 
-    $types[] = PhabricatorProjectTransaction::TYPE_NAME;
     $types[] = PhabricatorProjectTransaction::TYPE_SLUGS;
     $types[] = PhabricatorProjectTransaction::TYPE_STATUS;
     $types[] = PhabricatorProjectTransaction::TYPE_IMAGE;
@@ -52,8 +51,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
-        return $object->getName();
       case PhabricatorProjectTransaction::TYPE_SLUGS:
         $slugs = $object->getSlugs();
         $slugs = mpull($slugs, 'getSlug', 'getSlug');
@@ -90,7 +87,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
       case PhabricatorProjectTransaction::TYPE_STATUS:
       case PhabricatorProjectTransaction::TYPE_IMAGE:
       case PhabricatorProjectTransaction::TYPE_ICON:
@@ -121,13 +117,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
-        $name = $xaction->getNewValue();
-        $object->setName($name);
-        if (!$this->getIsMilestone()) {
-          $object->setPrimarySlug(PhabricatorSlug::normalizeProjectSlug($name));
-        }
-        return;
       case PhabricatorProjectTransaction::TYPE_SLUGS:
         return;
       case PhabricatorProjectTransaction::TYPE_STATUS:
@@ -178,16 +167,6 @@ final class PhabricatorProjectTransactionEditor
     $new = $xaction->getNewValue();
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
-        // First, add the old name as a secondary slug; this is helpful
-        // for renames and generally a good thing to do.
-        if (!$this->getIsMilestone()) {
-          if ($old !== null) {
-            $this->addSlug($object, $old, false);
-          }
-          $this->addSlug($object, $new, false);
-        }
-        return;
       case PhabricatorProjectTransaction::TYPE_SLUGS:
         $old = $xaction->getOldValue();
         $new = $xaction->getNewValue();
@@ -299,59 +278,6 @@ final class PhabricatorProjectTransactionEditor
     $errors = parent::validateTransaction($object, $type, $xactions);
 
     switch ($type) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
-        $missing = $this->validateIsEmptyTextField(
-          $object->getName(),
-          $xactions);
-
-        if ($missing) {
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Required'),
-            pht('Project name is required.'),
-            nonempty(last($xactions), null));
-
-          $error->setIsMissingFieldError(true);
-          $errors[] = $error;
-        }
-
-        if (!$xactions) {
-          break;
-        }
-
-        if ($this->getIsMilestone()) {
-          break;
-        }
-
-        $name = last($xactions)->getNewValue();
-
-        if (!PhabricatorSlug::isValidProjectSlug($name)) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'Project names must contain at least one letter or number.'),
-            last($xactions));
-          break;
-        }
-
-        $slug = PhabricatorSlug::normalizeProjectSlug($name);
-
-        $slug_used_already = id(new PhabricatorProjectSlug())
-          ->loadOneWhere('slug = %s', $slug);
-        if ($slug_used_already &&
-            $slug_used_already->getProjectPHID() != $object->getPHID()) {
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Duplicate'),
-            pht(
-              'Project name generates the same hashtag ("%s") as another '.
-              'existing project. Choose a unique name.',
-              '#'.$slug),
-            nonempty(last($xactions), null));
-          $errors[] = $error;
-        }
-        break;
       case PhabricatorProjectTransaction::TYPE_SLUGS:
         if (!$xactions) {
           break;
@@ -497,7 +423,7 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_NAME:
+      case PhabricatorProjectNameTransaction::TRANSACTIONTYPE:
       case PhabricatorProjectTransaction::TYPE_STATUS:
       case PhabricatorProjectTransaction::TYPE_IMAGE:
       case PhabricatorProjectTransaction::TYPE_ICON:
@@ -717,7 +643,7 @@ final class PhabricatorProjectTransactionEditor
     return parent::applyFinalEffects($object, $xactions);
   }
 
-  private function addSlug(PhabricatorProject $project, $slug, $force) {
+  public function addSlug(PhabricatorProject $project, $slug, $force) {
     $slug = PhabricatorSlug::normalizeProjectSlug($slug);
     $table = new PhabricatorProjectSlug();
     $project_phid = $project->getPHID();

src/applications/project/engine/PhabricatorProjectEditEngine.php

@@ -235,7 +235,7 @@ final class PhabricatorProjectEditEngine
       id(new PhabricatorTextEditField())
         ->setKey('name')
         ->setLabel(pht('Name'))
-        ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+        ->setTransactionType(PhabricatorProjectNameTransaction::TRANSACTIONTYPE)
         ->setIsRequired(true)
         ->setDescription(pht('Project name.'))
         ->setConduitDescription(pht('Rename the project'))

src/applications/project/lipsum/PhabricatorProjectTestDataGenerator.php

@@ -16,7 +16,7 @@ final class PhabricatorProjectTestDataGenerator
     $xactions = array();
 
     $xactions[] = $this->newTransaction(
-      PhabricatorProjectTransaction::TYPE_NAME,
+      PhabricatorProjectNameTransaction::TRANSACTIONTYPE,
       $this->newProjectTitle());
 
     $xactions[] = $this->newTransaction(

src/applications/project/storage/PhabricatorProjectTransaction.php

@@ -1,9 +1,8 @@
 <?php
 
 final class PhabricatorProjectTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
 
-  const TYPE_NAME       = 'project:name';
   const TYPE_SLUGS      = 'project:slugs';
   const TYPE_STATUS     = 'project:status';
   const TYPE_IMAGE      = 'project:image';
@@ -33,6 +32,10 @@ final class PhabricatorProjectTransaction
     return PhabricatorProjectProjectPHIDType::TYPECONST;
   }
 
+  public function getBaseTransactionClass() {
+    return 'PhabricatorProjectTransactionType';
+  }
+
   public function getRequiredHandlePHIDs() {
     $old = $this->getOldValue();
     $new = $this->getNewValue();
@@ -149,20 +152,6 @@ final class PhabricatorProjectTransaction
           '%s created this project.',
           $this->renderHandleLink($author_phid));
 
-      case self::TYPE_NAME:
-        if ($old === null) {
-          return pht(
-            '%s created this project.',
-            $author_handle);
-        } else {
-          return pht(
-            '%s renamed this project from "%s" to "%s".',
-            $author_handle,
-            $old,
-            $new);
-        }
-        break;
-
       case self::TYPE_STATUS:
         if ($old == 0) {
           return pht(
@@ -329,20 +318,6 @@ final class PhabricatorProjectTransaction
     $new = $this->getNewValue();
 
     switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-        if ($old === null) {
-          return pht(
-            '%s created %s.',
-            $author_handle,
-            $object_handle);
-        } else {
-          return pht(
-            '%s renamed %s from "%s" to "%s".',
-            $author_handle,
-            $object_handle,
-            $old,
-            $new);
-        }
       case self::TYPE_STATUS:
         if ($old == 0) {
           return pht(

src/applications/project/xaction/PhabricatorProjectNameTransaction.php

@@ -0,0 +1,112 @@
+<?php
+
+final class PhabricatorProjectNameTransaction
+  extends PhabricatorProjectTransactionType {
+
+  const TRANSACTIONTYPE = 'project:name';
+
+  public function generateOldValue($object) {
+    return $object->getName();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setName($value);
+    if (!$this->getEditor()->getIsMilestone()) {
+      $object->setPrimarySlug(PhabricatorSlug::normalizeProjectSlug($value));
+    }
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $old = $this->getOldValue();
+
+    // First, add the old name as a secondary slug; this is helpful
+    // for renames and generally a good thing to do.
+    if (!$this->getEditor()->getIsMilestone()) {
+      if ($old !== null) {
+        $this->getEditor()->addSlug($object, $old, false);
+      }
+      $this->getEditor()->addSlug($object, $value, false);
+    }
+    return;
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    if ($old === null) {
+      return pht(
+        '%s created this project.',
+        $this->renderAuthor());
+    } else {
+      return pht(
+        '%s renamed this project from %s to %s.',
+        $this->renderAuthor(),
+        $this->renderOldValue(),
+        $this->renderNewValue());
+    }
+  }
+
+  public function getTitleForFeed() {
+    $old = $this->getOldValue();
+    if ($old === null) {
+      return pht(
+        '%s created %s.',
+        $this->renderAuthor(),
+        $this->renderObject());
+    } else {
+      return pht(
+        '%s renamed %s from %s to %s.',
+        $this->renderAuthor(),
+        $this->renderObject(),
+        $this->renderOldValue(),
+        $this->renderNewValue());
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
+      $errors[] = $this->newRequiredError(pht('Projects 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(
+            'Project names must not be longer than %s character(s).',
+            new PhutilNumber($max_length)));
+      }
+    }
+
+    if ($this->getEditor()->getIsMilestone() || !$xactions) {
+      return $errors;
+    }
+
+    $name = last($xactions)->getNewValue();
+
+    if (!PhabricatorSlug::isValidProjectSlug($name)) {
+      $errors[] = $this->newInvalidError(
+        pht('Project names must contain at least one letter or number.'));
+    }
+
+    $slug = PhabricatorSlug::normalizeProjectSlug($name);
+
+    $slug_used_already = id(new PhabricatorProjectSlug())
+      ->loadOneWhere('slug = %s', $slug);
+    if ($slug_used_already &&
+        $slug_used_already->getProjectPHID() != $object->getPHID()) {
+
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Project name generates the same hashtag ("%s") as another '.
+          'existing project. Choose a unique name.',
+          '#'.$slug));
+    }
+
+    return $errors;
+  }
+
+}

src/applications/project/xaction/PhabricatorProjectTransactionType.php

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

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -592,6 +592,8 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $xtype = $this->getModularTransactionType($type);
     if ($xtype) {
+      $xtype = clone $xtype;
+      $xtype->setStorage($xaction);
       return $xtype->applyExternalEffects($object, $xaction->getNewValue());
     }