Migrate Project parent and milestone to modular transactions

Test Plan: Unit tests pass. Went through the UI for creating new subprojects and milestones, but didn't setup some API calls to check that all the validation errors were still caught.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

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

src/__phutil_library_map__.php

@@ -3660,6 +3660,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectMembersRemoveController' => 'applications/project/controller/PhabricatorProjectMembersRemoveController.php',
     'PhabricatorProjectMembersViewController' => 'applications/project/controller/PhabricatorProjectMembersViewController.php',
     'PhabricatorProjectMenuItemController' => 'applications/project/controller/PhabricatorProjectMenuItemController.php',
+    'PhabricatorProjectMilestoneTransaction' => 'applications/project/xaction/PhabricatorProjectMilestoneTransaction.php',
     'PhabricatorProjectMoveController' => 'applications/project/controller/PhabricatorProjectMoveController.php',
     'PhabricatorProjectNameContextFreeGrammar' => 'applications/project/lipsum/PhabricatorProjectNameContextFreeGrammar.php',
     'PhabricatorProjectNameTransaction' => 'applications/project/xaction/PhabricatorProjectNameTransaction.php',
@@ -3668,6 +3669,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectOrUserDatasource' => 'applications/project/typeahead/PhabricatorProjectOrUserDatasource.php',
     'PhabricatorProjectOrUserFunctionDatasource' => 'applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php',
     'PhabricatorProjectPHIDResolver' => 'applications/phid/resolver/PhabricatorProjectPHIDResolver.php',
+    'PhabricatorProjectParentTransaction' => 'applications/project/xaction/PhabricatorProjectParentTransaction.php',
     'PhabricatorProjectPictureProfileMenuItem' => 'applications/project/menuitem/PhabricatorProjectPictureProfileMenuItem.php',
     'PhabricatorProjectPointsProfileMenuItem' => 'applications/project/menuitem/PhabricatorProjectPointsProfileMenuItem.php',
     'PhabricatorProjectProfileController' => 'applications/project/controller/PhabricatorProjectProfileController.php',
@@ -3696,6 +3698,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectTransactionEditor' => 'applications/project/editor/PhabricatorProjectTransactionEditor.php',
     'PhabricatorProjectTransactionQuery' => 'applications/project/query/PhabricatorProjectTransactionQuery.php',
     'PhabricatorProjectTransactionType' => 'applications/project/xaction/PhabricatorProjectTransactionType.php',
+    'PhabricatorProjectTypeTransaction' => 'applications/project/xaction/PhabricatorProjectTypeTransaction.php',
     'PhabricatorProjectUIEventListener' => 'applications/project/events/PhabricatorProjectUIEventListener.php',
     'PhabricatorProjectUpdateController' => 'applications/project/controller/PhabricatorProjectUpdateController.php',
     'PhabricatorProjectUserFunctionDatasource' => 'applications/project/typeahead/PhabricatorProjectUserFunctionDatasource.php',
@@ -9093,6 +9096,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectMembersRemoveController' => 'PhabricatorProjectController',
     'PhabricatorProjectMembersViewController' => 'PhabricatorProjectController',
     'PhabricatorProjectMenuItemController' => 'PhabricatorProjectController',
+    'PhabricatorProjectMilestoneTransaction' => 'PhabricatorProjectTypeTransaction',
     'PhabricatorProjectMoveController' => 'PhabricatorProjectController',
     'PhabricatorProjectNameContextFreeGrammar' => 'PhutilContextFreeGrammar',
     'PhabricatorProjectNameTransaction' => 'PhabricatorProjectTransactionType',
@@ -9101,6 +9105,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectOrUserDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorProjectOrUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorProjectPHIDResolver' => 'PhabricatorPHIDResolver',
+    'PhabricatorProjectParentTransaction' => 'PhabricatorProjectTypeTransaction',
     'PhabricatorProjectPictureProfileMenuItem' => 'PhabricatorProfileMenuItem',
     'PhabricatorProjectPointsProfileMenuItem' => 'PhabricatorProfileMenuItem',
     'PhabricatorProjectProfileController' => 'PhabricatorProjectController',
@@ -9132,6 +9137,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorProjectTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorProjectTransactionType' => 'PhabricatorModularTransactionType',
+    'PhabricatorProjectTypeTransaction' => 'PhabricatorProjectTransactionType',
     'PhabricatorProjectUIEventListener' => 'PhabricatorEventListener',
     'PhabricatorProjectUpdateController' => 'PhabricatorProjectController',
     'PhabricatorProjectUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -1447,11 +1447,13 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     if ($parent) {
       if ($is_milestone) {
         $xactions[] = id(new PhabricatorProjectTransaction())
-          ->setTransactionType(PhabricatorProjectTransaction::TYPE_MILESTONE)
+          ->setTransactionType(
+              PhabricatorProjectMilestoneTransaction::TRANSACTIONTYPE)
           ->setNewValue($parent->getPHID());
       } else {
         $xactions[] = id(new PhabricatorProjectTransaction())
-          ->setTransactionType(PhabricatorProjectTransaction::TYPE_PARENT)
+          ->setTransactionType(
+              PhabricatorProjectParentTransaction::TRANSACTIONTYPE)
           ->setNewValue($parent->getPHID());
       }
     }

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -30,8 +30,6 @@ final class PhabricatorProjectTransactionEditor
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
     $types[] = PhabricatorTransactions::TYPE_JOIN_POLICY;
 
-    $types[] = PhabricatorProjectTransaction::TYPE_PARENT;
-    $types[] = PhabricatorProjectTransaction::TYPE_MILESTONE;
     $types[] = PhabricatorProjectTransaction::TYPE_HASWORKBOARD;
     $types[] = PhabricatorProjectTransaction::TYPE_DEFAULT_SORT;
     $types[] = PhabricatorProjectTransaction::TYPE_DEFAULT_FILTER;
@@ -47,9 +45,6 @@ final class PhabricatorProjectTransactionEditor
     switch ($xaction->getTransactionType()) {
       case PhabricatorProjectTransaction::TYPE_HASWORKBOARD:
         return (int)$object->getHasWorkboard();
-      case PhabricatorProjectTransaction::TYPE_PARENT:
-      case PhabricatorProjectTransaction::TYPE_MILESTONE:
-        return null;
       case PhabricatorProjectTransaction::TYPE_DEFAULT_SORT:
         return $object->getDefaultWorkboardSort();
       case PhabricatorProjectTransaction::TYPE_DEFAULT_FILTER:
@@ -66,8 +61,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_PARENT:
-      case PhabricatorProjectTransaction::TYPE_MILESTONE:
       case PhabricatorProjectTransaction::TYPE_DEFAULT_SORT:
       case PhabricatorProjectTransaction::TYPE_DEFAULT_FILTER:
         return $xaction->getNewValue();
@@ -89,14 +82,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_PARENT:
-        $object->setParentProjectPHID($xaction->getNewValue());
-        return;
-      case PhabricatorProjectTransaction::TYPE_MILESTONE:
-        $number = $object->getParentProject()->loadNextMilestoneNumber();
-        $object->setMilestoneNumber($number);
-        $object->setParentProjectPHID($xaction->getNewValue());
-        return;
       case PhabricatorProjectTransaction::TYPE_HASWORKBOARD:
         $object->setHasWorkboard($xaction->getNewValue());
         return;
@@ -122,8 +107,6 @@ final class PhabricatorProjectTransactionEditor
     $new = $xaction->getNewValue();
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_PARENT:
-      case PhabricatorProjectTransaction::TYPE_MILESTONE:
       case PhabricatorProjectTransaction::TYPE_HASWORKBOARD:
       case PhabricatorProjectTransaction::TYPE_DEFAULT_SORT:
       case PhabricatorProjectTransaction::TYPE_DEFAULT_FILTER:
@@ -145,8 +128,8 @@ final class PhabricatorProjectTransactionEditor
     $parent_xaction = null;
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
-        case PhabricatorProjectTransaction::TYPE_PARENT:
+        case PhabricatorProjectParentTransaction::TRANSACTIONTYPE:
-        case PhabricatorProjectTransaction::TYPE_MILESTONE:
+        case PhabricatorProjectMilestoneTransaction::TRANSACTIONTYPE:
           if ($xaction->getNewValue() === null) {
             continue;
           }
@@ -208,95 +191,6 @@ final class PhabricatorProjectTransactionEditor
     return $errors;
   }
 
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    switch ($type) {
-      case PhabricatorProjectTransaction::TYPE_PARENT:
-      case PhabricatorProjectTransaction::TYPE_MILESTONE:
-        if (!$xactions) {
-          break;
-        }
-
-        $xaction = last($xactions);
-
-        $parent_phid = $xaction->getNewValue();
-        if (!$parent_phid) {
-          continue;
-        }
-
-        if (!$this->getIsNewObject()) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'You can only set a parent or milestone project when creating a '.
-              'project for the first time.'),
-            $xaction);
-          break;
-        }
-
-        $projects = id(new PhabricatorProjectQuery())
-          ->setViewer($this->requireActor())
-          ->withPHIDs(array($parent_phid))
-          ->requireCapabilities(
-            array(
-              PhabricatorPolicyCapability::CAN_VIEW,
-              PhabricatorPolicyCapability::CAN_EDIT,
-            ))
-          ->execute();
-        if (!$projects) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'Parent or milestone project PHID ("%s") must be the PHID of a '.
-              'valid, visible project which you have permission to edit.',
-              $parent_phid),
-            $xaction);
-          break;
-        }
-
-        $project = head($projects);
-
-        if ($project->isMilestone()) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'Parent or milestone project PHID ("%s") must not be a '.
-              'milestone. Milestones may not have subprojects or milestones.',
-              $parent_phid),
-            $xaction);
-          break;
-        }
-
-        $limit = PhabricatorProject::getProjectDepthLimit();
-        if ($project->getProjectDepth() >= ($limit - 1)) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'You can not create a subproject or mielstone under this parent '.
-              'because it would nest projects too deeply. The maximum '.
-              'nesting depth of projects is %s.',
-              new PhutilNumber($limit)),
-            $xaction);
-          break;
-        }
-
-        $object->attachParentProject($project);
-        break;
-    }
-
-    return $errors;
-  }
-
-
   protected function requireCapabilities(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
@@ -458,8 +352,8 @@ final class PhabricatorProjectTransactionEditor
               break;
           }
           break;
-        case PhabricatorProjectTransaction::TYPE_PARENT:
+        case PhabricatorProjectParentTransaction::TRANSACTIONTYPE:
-        case PhabricatorProjectTransaction::TYPE_MILESTONE:
+        case PhabricatorProjectMilestoneTransaction::TRANSACTIONTYPE:
           $materialize = true;
           $new_parent = $object->getParentProject();
           break;
@@ -634,7 +528,7 @@ final class PhabricatorProjectTransactionEditor
     $is_milestone = $object->isMilestone();
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
-        case PhabricatorProjectTransaction::TYPE_MILESTONE:
+        case PhabricatorProjectMilestoneTransaction::TRANSACTIONTYPE:
           if ($xaction->getNewValue() !== null) {
             $is_milestone = true;
           }

src/applications/project/engine/PhabricatorProjectEditEngine.php

@@ -202,7 +202,8 @@ final class PhabricatorProjectEditEngine
           pht('Choose a parent project to create a subproject beneath.'))
         ->setConduitTypeDescription(pht('PHID of the parent project.'))
         ->setAliases(array('parentPHID'))
-        ->setTransactionType(PhabricatorProjectTransaction::TYPE_PARENT)
+        ->setTransactionType(
+            PhabricatorProjectParentTransaction::TRANSACTIONTYPE)
         ->setHandleParameterType(new AphrontPHIDHTTPParameterType())
         ->setSingleValue($parent_phid)
         ->setIsReorderable(false)
@@ -217,7 +218,8 @@ final class PhabricatorProjectEditEngine
           pht('Choose a parent project to create a new milestone for.'))
         ->setConduitTypeDescription(pht('PHID of the parent project.'))
         ->setAliases(array('milestonePHID'))
-        ->setTransactionType(PhabricatorProjectTransaction::TYPE_MILESTONE)
+        ->setTransactionType(
+            PhabricatorProjectMilestoneTransaction::TRANSACTIONTYPE)
         ->setHandleParameterType(new AphrontPHIDHTTPParameterType())
         ->setSingleValue($milestone_phid)
         ->setIsReorderable(false)
@@ -244,7 +246,8 @@ final class PhabricatorProjectEditEngine
       id(new PhabricatorIconSetEditField())
         ->setKey('icon')
         ->setLabel(pht('Icon'))
-        ->setTransactionType(PhabricatorProjectIconTransaction::TRANSACTIONTYPE)
+        ->setTransactionType(
+            PhabricatorProjectIconTransaction::TRANSACTIONTYPE)
         ->setIconSet(new PhabricatorProjectIconSet())
         ->setDescription(pht('Project icon.'))
         ->setConduitDescription(pht('Change the project icon.'))

src/applications/project/storage/PhabricatorProjectTransaction.php

@@ -3,8 +3,6 @@
 final class PhabricatorProjectTransaction
   extends PhabricatorModularTransaction {
 
-  const TYPE_PARENT = 'project:parent';
-  const TYPE_MILESTONE = 'project:milestone';
   const TYPE_HASWORKBOARD = 'project:hasworkboard';
   const TYPE_DEFAULT_SORT = 'project:sort';
   const TYPE_DEFAULT_FILTER = 'project:filter';

src/applications/project/xaction/PhabricatorProjectMilestoneTransaction.php

@@ -0,0 +1,31 @@
+<?php
+
+final class PhabricatorProjectMilestoneTransaction
+  extends PhabricatorProjectTypeTransaction {
+
+  const TRANSACTIONTYPE = 'project:milestone';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $parent_phid = $value;
+    $project = id(new PhabricatorProjectQuery())
+      ->setViewer($this->getActor())
+      ->withPHIDs(array($parent_phid))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+
+    $object->attachParentProject($project);
+
+    $number = $object->getParentProject()->loadNextMilestoneNumber();
+    $object->setMilestoneNumber($number);
+    $object->setParentProjectPHID($value);
+  }
+
+}

src/applications/project/xaction/PhabricatorProjectParentTransaction.php

@@ -0,0 +1,29 @@
+<?php
+
+final class PhabricatorProjectParentTransaction
+  extends PhabricatorProjectTypeTransaction {
+
+  const TRANSACTIONTYPE = 'project:parent';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $parent_phid = $value;
+    $project = id(new PhabricatorProjectQuery())
+      ->setViewer($this->getActor())
+      ->withPHIDs(array($parent_phid))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+
+    $object->attachParentProject($project);
+
+    $object->setParentProjectPHID($value);
+  }
+
+}

src/applications/project/xaction/PhabricatorProjectTypeTransaction.php

@@ -0,0 +1,71 @@
+<?php
+
+abstract class PhabricatorProjectTypeTransaction
+  extends PhabricatorProjectTransactionType {
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if (!$xactions) {
+      return $errors;
+    }
+
+    $xaction = last($xactions);
+
+    $parent_phid = $xaction->getNewValue();
+    if (!$parent_phid) {
+      return $errors;
+    }
+
+    if (!$this->getEditor()->getIsNewObject()) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'You can only set a parent or milestone project when creating a '.
+          'project for the first time.'));
+      return $errors;
+    }
+
+    $projects = id(new PhabricatorProjectQuery())
+      ->setViewer($this->getActor())
+      ->withPHIDs(array($parent_phid))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->execute();
+    if (!$projects) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Parent or milestone project PHID ("%s") must be the PHID of a '.
+          'valid, visible project which you have permission to edit.',
+          $parent_phid));
+      return $errors;
+    }
+
+    $project = head($projects);
+
+    if ($project->isMilestone()) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Parent or milestone project PHID ("%s") must not be a '.
+          'milestone. Milestones may not have subprojects or milestones.',
+          $parent_phid));
+      return $errors;
+    }
+
+    $limit = PhabricatorProject::getProjectDepthLimit();
+    if ($project->getProjectDepth() >= ($limit - 1)) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'You can not create a subproject or milestone under this parent '.
+          'because it would nest projects too deeply. The maximum '.
+          'nesting depth of projects is %s.',
+          new PhutilNumber($limit)));
+      return $errors;
+    }
+
+    return $errors;
+  }
+
+}