From 33f55a85b09f5b9021552314ab7ec7a80ccdd1cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Dec 2015 04:12:09 -0800 Subject: [PATCH] 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 --- .../PhabricatorProjectCoreTestCase.php | 64 +++++++++++ .../PhabricatorProjectTransactionEditor.php | 104 +++++++++--------- 2 files changed, 119 insertions(+), 49 deletions(-) diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index 4b0742e5d7..f6b74801e6 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/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. diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index ad87e77967..ff9b23e477 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/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) { - $add_slug_template = id(new PhabricatorProjectSlug()) - ->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(); - } + foreach ($add as $slug) { + $this->addSlug($object, $slug, true); } + $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);