Fix project hashtag bugs: allow simultaneously changing name and adding same name as a tag

Summary:
Fixes T8509. Changes these behaviors:

  - If you create a project named "QQQ" and add "qqq" as a hashtag at the same time, it fails in an unhelpful way. (Now: succeeds.)
  - If you add "qqq" as a hashtag to a project with primary hashtag "qqq", it fails in a correct but probably unnecessary way (Now: just works).

We could make one or both of these behaviors show the user an error instead, but I think it's likely that this behavior is just what they always want.

Test Plan:
  - Added failing tests and made them pass.
  - Executed both scenarios described above from the web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8509

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

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -249,6 +249,70 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
       $milestone->getMemberPHIDs());
   }
 
+  public function testSameSlugAsName() {
+    // It should be OK to type the primary hashtag into "additional hashtags",
+    // even if the primary hashtag doesn't exist yet because you're creating
+    // or renaming the project.
+
+    $user = $this->createUser();
+    $user->save();
+
+    $project = $this->createProject($user);
+
+    // In this first case, set the name and slugs at the same time.
+    $name = 'slugproject';
+
+    $xactions = array();
+    $xactions[] = id(new PhabricatorProjectTransaction())
+      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setNewValue($name);
+    $this->applyTransactions($project, $user, $xactions);
+
+    $xactions = array();
+    $xactions[] = id(new PhabricatorProjectTransaction())
+      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setNewValue(array($name));
+    $this->applyTransactions($project, $user, $xactions);
+
+    $project = id(new PhabricatorProjectQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($project->getPHID()))
+      ->needSlugs(true)
+      ->executeOne();
+
+    $slugs = $project->getSlugs();
+    $slugs = mpull($slugs, 'getSlug');
+
+    $this->assertTrue(in_array($name, $slugs));
+
+    // In this second case, set the name first and then the slugs separately.
+    $name2 = 'slugproject2';
+
+    $xactions = array();
+
+    $xactions[] = id(new PhabricatorProjectTransaction())
+      ->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME)
+      ->setNewValue($name2);
+
+    $xactions[] = id(new PhabricatorProjectTransaction())
+      ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS)
+      ->setNewValue(array($name2));
+
+    $this->applyTransactions($project, $user, $xactions);
+
+    $project = id(new PhabricatorProjectQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($project->getPHID()))
+      ->needSlugs(true)
+      ->executeOne();
+
+    $slugs = $project->getSlugs();
+    $slugs = mpull($slugs, 'getSlug');
+
+    $this->assertTrue(in_array($name2, $slugs));
+
+  }
+
   public function testDuplicateSlugs() {
     // Creating a project with multiple duplicate slugs should succeed.
 

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -146,9 +146,9 @@ final class PhabricatorProjectTransactionEditor
         // First, add the old name as a secondary slug; this is helpful
         // for renames and generally a good thing to do.
         if ($old !== null) {
-          $this->addSlug($object, $old);
+          $this->addSlug($object, $old, false);
         }
-        $this->addSlug($object, $new);
+        $this->addSlug($object, $new, false);
 
         return;
       case PhabricatorProjectTransaction::TYPE_SLUGS:
@@ -157,23 +157,11 @@ final class PhabricatorProjectTransactionEditor
         $add = array_diff($new, $old);
         $rem = array_diff($old, $new);
 
-        if ($add) {
+        foreach ($add as $slug) {
-          $add_slug_template = id(new PhabricatorProjectSlug())
+          $this->addSlug($object, $slug, true);
-            ->setProjectPHID($object->getPHID());
-          foreach ($add as $add_slug_str) {
-            $add_slug = id(clone $add_slug_template)
-              ->setSlug($add_slug_str)
-              ->save();
-          }
-        }
-        if ($rem) {
-          $rem_slugs = id(new PhabricatorProjectSlug())
-            ->loadAllWhere('slug IN (%Ls)', $rem);
-          foreach ($rem_slugs as $rem_slug) {
-            $rem_slug->delete();
-          }
         }
 
+        $this->removeSlugs($object, $rem);
         return;
       case PhabricatorProjectTransaction::TYPE_STATUS:
       case PhabricatorProjectTransaction::TYPE_IMAGE:
@@ -328,21 +316,12 @@ final class PhabricatorProjectTransactionEditor
 
         $slugs_used_already = mgroup($slugs_used_already, 'getProjectPHID');
         foreach ($slugs_used_already as $project_phid => $used_slugs) {
-          $used_slug_strs = mpull($used_slugs, 'getSlug');
           if ($project_phid == $object->getPHID()) {
-            if (in_array($object->getPrimarySlug(), $used_slug_strs)) {
-              $error = new PhabricatorApplicationTransactionValidationError(
-                $type,
-                pht('Invalid'),
-                pht(
-                  'Project hashtag "%s" is already the primary hashtag.',
-                  $object->getPrimarySlug()),
-                $slug_xaction);
-              $errors[] = $error;
-            }
             continue;
           }
 
+          $used_slug_strs = mpull($used_slugs, 'getSlug');
+
           $error = new PhabricatorApplicationTransactionValidationError(
             $type,
             pht('Invalid'),
@@ -594,27 +573,6 @@ final class PhabricatorProjectTransactionEditor
     return parent::extractFilePHIDsFromCustomTransaction($object, $xaction);
   }
 
-  private function addSlug(
-    PhabricatorLiskDAO $object,
-    $name) {
-
-    $slug = PhabricatorSlug::normalizeProjectSlug($name);
-
-    $slug_object = id(new PhabricatorProjectSlug())->loadOneWhere(
-      'slug = %s',
-      $slug);
-
-    if ($slug_object) {
-      return;
-    }
-
-    $new_slug = id(new PhabricatorProjectSlug())
-      ->setSlug($slug)
-      ->setProjectPHID($object->getPHID())
-      ->save();
-  }
-
-
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -643,6 +601,54 @@ final class PhabricatorProjectTransactionEditor
     return parent::applyFinalEffects($object, $xactions);
   }
 
+  private function addSlug(PhabricatorProject $project, $slug, $force) {
+    $slug = PhabricatorSlug::normalizeProjectSlug($slug);
+    $table = new PhabricatorProjectSlug();
+    $project_phid = $project->getPHID();
+
+    if ($force) {
+      // If we have the `$force` flag set, we only want to ignore an existing
+      // slug if it's for the same project. We'll error on collisions with
+      // other projects.
+      $current = $table->loadOneWhere(
+        'slug = %s AND projectPHID = %s',
+        $slug,
+        $project_phid);
+    } else {
+      // Without the `$force` flag, we'll just return without doing anything
+      // if any other project already has the slug.
+      $current = $table->loadOneWhere(
+        'slug = %s',
+        $slug);
+    }
+
+    if ($current) {
+      return;
+    }
+
+    return id(new PhabricatorProjectSlug())
+      ->setSlug($slug)
+      ->setProjectPHID($project_phid)
+      ->save();
+  }
+
+  private function removeSlugs(PhabricatorProject $project, array $slugs) {
+    $slugs = $this->normalizeSlugs($slugs);
+
+    if (!$slugs) {
+      return;
+    }
+
+    $objects = id(new PhabricatorProjectSlug())->loadAllWhere(
+      'projectPHID = %s AND slug IN (%Ls)',
+      $project->getPHID(),
+      $slugs);
+
+    foreach ($objects as $object) {
+      $object->delete();
+    }
+  }
+
   private function normalizeSlugs(array $slugs) {
     foreach ($slugs as $key => $slug) {
       $slugs[$key] = PhabricatorSlug::normalizeProjectSlug($slug);