Migrate Project slugs to modular transactions

Test Plan: Unit tests all pass. Added/removed/altered some project hashtags and observed expected transactions in timeline. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D17952
2024-11-23 07:12:41 +01:00 · 2017-05-18 11:03:31 -07:00 · 2017-05-18 11:03:31 -07:00 · 91eb22cb3a
commit 91eb22cb3a
parent 1599c56217
7 changed files with 181 additions and 157 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -3666,6 +3666,7 @@ phutil_register_library_map(array(
    'PhabricatorProjectSilenceController' => 'applications/project/controller/PhabricatorProjectSilenceController.php',
    'PhabricatorProjectSilencedEdgeType' => 'applications/project/edge/PhabricatorProjectSilencedEdgeType.php',
    'PhabricatorProjectSlug' => 'applications/project/storage/PhabricatorProjectSlug.php',
+    'PhabricatorProjectSlugsTransaction' => 'applications/project/xaction/PhabricatorProjectSlugsTransaction.php',
    'PhabricatorProjectStandardCustomField' => 'applications/project/customfield/PhabricatorProjectStandardCustomField.php',
    'PhabricatorProjectStatus' => 'applications/project/constants/PhabricatorProjectStatus.php',
    'PhabricatorProjectSubprojectWarningController' => 'applications/project/controller/PhabricatorProjectSubprojectWarningController.php',
@ -9077,6 +9078,7 @@ phutil_register_library_map(array(
    'PhabricatorProjectSilenceController' => 'PhabricatorProjectController',
    'PhabricatorProjectSilencedEdgeType' => 'PhabricatorEdgeType',
    'PhabricatorProjectSlug' => 'PhabricatorProjectDAO',
+    'PhabricatorProjectSlugsTransaction' => 'PhabricatorProjectTransactionType',
    'PhabricatorProjectStandardCustomField' => array(
      'PhabricatorProjectCustomField',
      'PhabricatorStandardCustomFieldInterface',
--- a/src/applications/project/tests/PhabricatorProjectCoreTestCase.php
+++ b/src/applications/project/tests/PhabricatorProjectCoreTestCase.php
@ -362,7 +362,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {

    $xactions = array();
    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($name));
    $this->applyTransactions($project, $user, $xactions);

@ -386,7 +386,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
      ->setNewValue($name2);

    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($name2));

    $this->applyTransactions($project, $user, $xactions);
@ -416,7 +416,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
    $xactions = array();

    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($input, $input));

    $this->applyTransactions($project, $user, $xactions);
@ -448,7 +448,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
    $xactions = array();

    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($input));

    $this->applyTransactions($project, $user, $xactions);
@ -474,7 +474,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
    $xactions = array();

    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($input));

    $caught = null;
@ -605,7 +605,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
      ->setNewValue($name);

    $xactions[] = id(new PhabricatorProjectTransaction())
-      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setTransactionType(PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
      ->setNewValue(array($slug));

    $this->applyTransactions($project, $user, $xactions);
--- a/src/applications/project/conduit/ProjectCreateConduitAPIMethod.php
+++ b/src/applications/project/conduit/ProjectCreateConduitAPIMethod.php
@ -64,7 +64,8 @@ final class ProjectCreateConduitAPIMethod extends ProjectConduitAPIMethod {

    if ($request->getValue('tags')) {
      $xactions[] = id(new PhabricatorProjectTransaction())
-        ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+        ->setTransactionType(
+            PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
        ->setNewValue($request->getValue('tags'));
    }

--- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
+++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
@ -30,7 +30,6 @@ final class PhabricatorProjectTransactionEditor
    $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
    $types[] = PhabricatorTransactions::TYPE_JOIN_POLICY;

-    $types[] = PhabricatorProjectTransaction::TYPE_SLUGS;
    $types[] = PhabricatorProjectTransaction::TYPE_STATUS;
    $types[] = PhabricatorProjectTransaction::TYPE_IMAGE;
    $types[] = PhabricatorProjectTransaction::TYPE_ICON;
@ -51,11 +50,6 @@ final class PhabricatorProjectTransactionEditor
    PhabricatorApplicationTransaction $xaction) {

    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_SLUGS:
-        $slugs = $object->getSlugs();
-        $slugs = mpull($slugs, 'getSlug', 'getSlug');
-        unset($slugs[$object->getPrimarySlug()]);
-        return array_keys($slugs);
      case PhabricatorProjectTransaction::TYPE_STATUS:
        return $object->getStatus();
      case PhabricatorProjectTransaction::TYPE_IMAGE:
@ -105,8 +99,6 @@ final class PhabricatorProjectTransactionEditor
          return null;
        }
        return $value;
-      case PhabricatorProjectTransaction::TYPE_SLUGS:
-        return $this->normalizeSlugs($xaction->getNewValue());
    }

    return parent::getCustomTransactionNewValue($object, $xaction);
@ -117,8 +109,6 @@ final class PhabricatorProjectTransactionEditor
    PhabricatorApplicationTransaction $xaction) {

    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_SLUGS:
-        return;
      case PhabricatorProjectTransaction::TYPE_STATUS:
        $object->setStatus($xaction->getNewValue());
        return;
@ -167,18 +157,6 @@ final class PhabricatorProjectTransactionEditor
    $new = $xaction->getNewValue();

    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_SLUGS:
-        $old = $xaction->getOldValue();
-        $new = $xaction->getNewValue();
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-
-        foreach ($add as $slug) {
-          $this->addSlug($object, $slug, true);
-        }
-
-        $this->removeSlugs($object, $rem);
-        return;
      case PhabricatorProjectTransaction::TYPE_STATUS:
      case PhabricatorProjectTransaction::TYPE_IMAGE:
      case PhabricatorProjectTransaction::TYPE_ICON:
@ -278,65 +256,6 @@ final class PhabricatorProjectTransactionEditor
    $errors = parent::validateTransaction($object, $type, $xactions);

    switch ($type) {
-      case PhabricatorProjectTransaction::TYPE_SLUGS:
-        if (!$xactions) {
-          break;
-        }
-
-        $slug_xaction = last($xactions);
-
-        $new = $slug_xaction->getNewValue();
-
-        $invalid = array();
-        foreach ($new as $slug) {
-          if (!PhabricatorSlug::isValidProjectSlug($slug)) {
-            $invalid[] = $slug;
-          }
-        }
-
-        if ($invalid) {
-          $errors[] = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              'Hashtags must contain at least one letter or number. %s '.
-              'project hashtag(s) are invalid: %s.',
-              phutil_count($invalid),
-              implode(', ', $invalid)),
-            $slug_xaction);
-          break;
-        }
-
-        $new = $this->normalizeSlugs($new);
-
-        if ($new) {
-          $slugs_used_already = id(new PhabricatorProjectSlug())
-            ->loadAllWhere('slug IN (%Ls)', $new);
-        } else {
-          // The project doesn't have any extra slugs.
-          $slugs_used_already = array();
-        }
-
-        $slugs_used_already = mgroup($slugs_used_already, 'getProjectPHID');
-        foreach ($slugs_used_already as $project_phid => $used_slugs) {
-          if ($project_phid == $object->getPHID()) {
-            continue;
-          }
-
-          $used_slug_strs = mpull($used_slugs, 'getSlug');
-
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Invalid'),
-            pht(
-              '%s project hashtag(s) are already used by other projects: %s.',
-              phutil_count($used_slug_strs),
-              implode(', ', $used_slug_strs)),
-            $slug_xaction);
-          $errors[] = $error;
-        }
-
-        break;
      case PhabricatorProjectTransaction::TYPE_PARENT:
      case PhabricatorProjectTransaction::TYPE_MILESTONE:
        if (!$xactions) {
@ -674,7 +593,7 @@ final class PhabricatorProjectTransactionEditor
      ->save();
  }

-  private function removeSlugs(PhabricatorProject $project, array $slugs) {
+  public function removeSlugs(PhabricatorProject $project, array $slugs) {
    if (!$slugs) {
      return;
    }
@ -696,7 +615,7 @@ final class PhabricatorProjectTransactionEditor
    }
  }

-  private function normalizeSlugs(array $slugs) {
+  public function normalizeSlugs(array $slugs) {
    foreach ($slugs as $key => $slug) {
      $slugs[$key] = PhabricatorSlug::normalizeProjectSlug($slug);
    }
--- a/src/applications/project/engine/PhabricatorProjectEditEngine.php
+++ b/src/applications/project/engine/PhabricatorProjectEditEngine.php
@ -262,7 +262,8 @@ final class PhabricatorProjectEditEngine
      id(new PhabricatorStringListEditField())
        ->setKey('slugs')
        ->setLabel(pht('Additional Hashtags'))
-        ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+        ->setTransactionType(
+            PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE)
        ->setDescription(pht('Additional project slugs.'))
        ->setConduitDescription(pht('Change project slugs.'))
        ->setConduitTypeDescription(pht('New list of slugs.'))
--- a/src/applications/project/storage/PhabricatorProjectTransaction.php
+++ b/src/applications/project/storage/PhabricatorProjectTransaction.php
@ -3,7 +3,6 @@
 final class PhabricatorProjectTransaction
  extends PhabricatorModularTransaction {

-  const TYPE_SLUGS      = 'project:slugs';
  const TYPE_STATUS     = 'project:status';
  const TYPE_IMAGE      = 'project:image';
  const TYPE_ICON       = 'project:icon';
@ -134,8 +133,6 @@ final class PhabricatorProjectTransaction
        return 'fa-photo';
      case self::TYPE_MEMBERS:
        return 'fa-user';
-      case self::TYPE_SLUGS:
-        return 'fa-tag';
    }
    return parent::getIcon();
  }
@ -212,33 +209,6 @@ final class PhabricatorProjectTransaction
        }
        break;

-      case self::TYPE_SLUGS:
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-
-        if ($add && $rem) {
-          return pht(
-            '%s changed project hashtag(s), added %d: %s; removed %d: %s.',
-            $author_handle,
-            count($add),
-            $this->renderSlugList($add),
-            count($rem),
-            $this->renderSlugList($rem));
-        } else if ($add) {
-          return pht(
-            '%s added %d project hashtag(s): %s.',
-            $author_handle,
-            count($add),
-            $this->renderSlugList($add));
-        } else if ($rem) {
-            return pht(
-              '%s removed %d project hashtag(s): %s.',
-              $author_handle,
-              count($rem),
-              $this->renderSlugList($rem));
-        }
-        break;
-
      case self::TYPE_MEMBERS:
        $add = array_diff($new, $old);
        $rem = array_diff($old, $new);
@ -380,36 +350,6 @@ final class PhabricatorProjectTransaction
            $author_handle,
            $object_handle);
        }
-
-      case self::TYPE_SLUGS:
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-
-        if ($add && $rem) {
-          return pht(
-            '%s changed %s hashtag(s), added %d: %s; removed %d: %s.',
-            $author_handle,
-            $object_handle,
-            count($add),
-            $this->renderSlugList($add),
-            count($rem),
-            $this->renderSlugList($rem));
-        } else if ($add) {
-          return pht(
-            '%s added %d %s hashtag(s): %s.',
-            $author_handle,
-            count($add),
-            $object_handle,
-            $this->renderSlugList($add));
-        } else if ($rem) {
-          return pht(
-            '%s removed %d %s hashtag(s): %s.',
-            $author_handle,
-            count($rem),
-            $object_handle,
-            $this->renderSlugList($rem));
-        }
-
    }

    return parent::getTitleForFeed();
@ -418,8 +358,8 @@ final class PhabricatorProjectTransaction
  public function getMailTags() {
    $tags = array();
    switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-      case self::TYPE_SLUGS:
+      case PhabricatorProjectNameTransaction::TRANSACTIONTYPE:
+      case PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE:
      case self::TYPE_IMAGE:
      case self::TYPE_ICON:
      case self::TYPE_COLOR:
@ -447,8 +387,4 @@ final class PhabricatorProjectTransaction
    return $tags;
  }

-  private function renderSlugList($slugs) {
-    return implode(', ', $slugs);
-  }
-
 }
--- a/src/applications/project/xaction/PhabricatorProjectSlugsTransaction.php
+++ b/src/applications/project/xaction/PhabricatorProjectSlugsTransaction.php
@ -0,0 +1,165 @@
+<?php
+
+final class PhabricatorProjectSlugsTransaction
+  extends PhabricatorProjectTransactionType {
+
+  const TRANSACTIONTYPE = 'project:slugs';
+
+  public function generateOldValue($object) {
+    $slugs = $object->getSlugs();
+    $slugs = mpull($slugs, 'getSlug', 'getSlug');
+    unset($slugs[$object->getPrimarySlug()]);
+    return array_keys($slugs);
+  }
+
+  public function generateNewValue($object, $value) {
+    return $this->getEditor()->normalizeSlugs($value);
+  }
+
+  public function applyInternalEffects($object, $value) {
+    return;
+  }
+
+  public function applyExternalEffects($object, $value) {
+   $old = $this->getOldValue();
+   $new = $value;
+   $add = array_diff($new, $old);
+   $rem = array_diff($old, $new);
+
+   foreach ($add as $slug) {
+     $this->getEditor()->addSlug($object, $slug, true);
+   }
+
+   $this->getEditor()->removeSlugs($object, $rem);
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+
+    if ($add && $rem) {
+      return pht(
+        '%s changed project hashtag(s), added %d: %s; removed %d: %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderSlugList($add),
+        count($rem),
+        $this->renderSlugList($rem));
+    } else if ($add) {
+      return pht(
+        '%s added %d project hashtag(s): %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderSlugList($add));
+    } else if ($rem) {
+        return pht(
+          '%s removed %d project hashtag(s): %s.',
+          $this->renderAuthor(),
+          count($rem),
+          $this->renderSlugList($rem));
+    }
+    break;
+  }
+
+  public function getTitleForFeed() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+
+    if ($add && $rem) {
+      return pht(
+        '%s changed %s hashtag(s), added %d: %s; removed %d: %s.',
+        $this->renderAuthor(),
+        $this->renderObject(),
+        count($add),
+        $this->renderSlugList($add),
+        count($rem),
+        $this->renderSlugList($rem));
+    } else if ($add) {
+      return pht(
+        '%s added %d %s hashtag(s): %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderObject(),
+        $this->renderSlugList($add));
+    } else if ($rem) {
+      return pht(
+        '%s removed %d %s hashtag(s): %s.',
+        $this->renderAuthor(),
+        count($rem),
+        $this->renderObject(),
+        $this->renderSlugList($rem));
+    }
+  }
+
+  public function getIcon() {
+    return 'fa-tag';
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if (!$xactions) {
+      return $errors;
+    }
+
+    $slug_xaction = last($xactions);
+
+    $new = $slug_xaction->getNewValue();
+
+    $invalid = array();
+    foreach ($new as $slug) {
+      if (!PhabricatorSlug::isValidProjectSlug($slug)) {
+        $invalid[] = $slug;
+      }
+    }
+
+    if ($invalid) {
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Hashtags must contain at least one letter or number. %s '.
+          'project hashtag(s) are invalid: %s.',
+          phutil_count($invalid),
+          implode(', ', $invalid)));
+
+      return $errors;
+    }
+
+    $new = $this->getEditor()->normalizeSlugs($new);
+
+    if ($new) {
+      $slugs_used_already = id(new PhabricatorProjectSlug())
+        ->loadAllWhere('slug IN (%Ls)', $new);
+    } else {
+      // The project doesn't have any extra slugs.
+      $slugs_used_already = array();
+    }
+
+    $slugs_used_already = mgroup($slugs_used_already, 'getProjectPHID');
+    foreach ($slugs_used_already as $project_phid => $used_slugs) {
+      if ($project_phid == $object->getPHID()) {
+        continue;
+      }
+
+      $used_slug_strs = mpull($used_slugs, 'getSlug');
+
+      $errors[] = $this->newInvalidError(
+        pht(
+          '%s project hashtag(s) are already used by other projects: %s.',
+          phutil_count($used_slug_strs),
+          implode(', ', $used_slug_strs)));
+    }
+
+    return $errors;
+  }
+
+  private function renderSlugList($slugs) {
+    return implode(', ', $slugs);
+  }
+
+}