From 92912a60720c82db1e42edd3fe822604bf30cf91 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Dec 2015 03:51:07 -0800 Subject: [PATCH] Fix project hashtag bugs: duplicate tags, uppercase tags Summary: Ref T8509. This fixes three issues: - Adding a slug like `UPPERCASE` would not give you a normalized slug. (Now: normalizes as `uppercase`.) - Adding a slug like `UPPERCASE` would allow you to give two different projects the different tags `UPPERCASE` and `uppercase` (and `UpPeRcAsE`, etc). (Now: second tag is rejected as a duplicate.) - Adding multiple identical or similar slugs would produce a duplicate key exception. (Now: ignores the duplicates.) Test Plan: - Added test coverage. - Made tests pass. - Hit these cases in the UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8509 Differential Revision: https://secure.phabricator.com/D14870 --- .../PhabricatorProjectCoreTestCase.php | 84 +++++++++++++++++++ .../PhabricatorProjectTransactionEditor.php | 22 ++++- .../PhabricatorUSEnglishTranslation.php | 6 +- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index e239bd645f..4b0742e5d7 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -249,6 +249,90 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $milestone->getMemberPHIDs()); } + public function testDuplicateSlugs() { + // Creating a project with multiple duplicate slugs should succeed. + + $user = $this->createUser(); + $user->save(); + + $project = $this->createProject($user); + + $input = 'duplicate'; + + $xactions = array(); + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS) + ->setNewValue(array($input, $input)); + + $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($input, $slugs)); + } + + public function testNormalizeSlugs() { + // When a user creates a project with slug "XxX360n0sc0perXxX", normalize + // it before writing it. + + $user = $this->createUser(); + $user->save(); + + $project = $this->createProject($user); + + $input = 'NoRmAlIzE'; + $expect = 'normalize'; + + $xactions = array(); + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS) + ->setNewValue(array($input)); + + $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($expect, $slugs)); + + + // If another user tries to add the same slug in denormalized form, it + // should be caught and fail, even though the database version of the slug + // is normalized. + + $project2 = $this->createProject($user); + + $xactions = array(); + + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorProjectTransaction::TYPE_SLUGS) + ->setNewValue(array($input)); + + $caught = null; + try { + $this->applyTransactions($project2, $user, $xactions); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $caught = $ex; + } + + $this->assertTrue((bool)$caught); + } + public function testParentProject() { $user = $this->createUser(); $user->save(); diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index 401d5c6cde..ad87e77967 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -68,7 +68,6 @@ final class PhabricatorProjectTransactionEditor switch ($xaction->getTransactionType()) { case PhabricatorProjectTransaction::TYPE_NAME: - case PhabricatorProjectTransaction::TYPE_SLUGS: case PhabricatorProjectTransaction::TYPE_STATUS: case PhabricatorProjectTransaction::TYPE_IMAGE: case PhabricatorProjectTransaction::TYPE_ICON: @@ -76,6 +75,8 @@ final class PhabricatorProjectTransactionEditor case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_PARENT: return $xaction->getNewValue(); + case PhabricatorProjectTransaction::TYPE_SLUGS: + return $this->normalizeSlugs($xaction->getNewValue()); case PhabricatorProjectTransaction::TYPE_MILESTONE: $current = queryfx_one( $object->establishConnection('w'), @@ -313,7 +314,9 @@ final class PhabricatorProjectTransactionEditor } $slug_xaction = last($xactions); + $new = $slug_xaction->getNewValue(); + $new = $this->normalizeSlugs($new); if ($new) { $slugs_used_already = id(new PhabricatorProjectSlug()) @@ -332,7 +335,7 @@ final class PhabricatorProjectTransactionEditor $type, pht('Invalid'), pht( - 'Project hashtag %s is already the primary hashtag.', + 'Project hashtag "%s" is already the primary hashtag.', $object->getPrimarySlug()), $slug_xaction); $errors[] = $error; @@ -344,8 +347,8 @@ final class PhabricatorProjectTransactionEditor $type, pht('Invalid'), pht( - '%d project hashtag(s) are already used: %s.', - count($used_slug_strs), + '%s project hashtag(s) are already used by other projects: %s.', + phutil_count($used_slug_strs), implode(', ', $used_slug_strs)), $slug_xaction); $errors[] = $error; @@ -640,4 +643,15 @@ final class PhabricatorProjectTransactionEditor return parent::applyFinalEffects($object, $xactions); } + private function normalizeSlugs(array $slugs) { + foreach ($slugs as $key => $slug) { + $slugs[$key] = PhabricatorSlug::normalizeProjectSlug($slug); + } + + $slugs = array_unique($slugs); + $slugs = array_values($slugs); + + return $slugs; + } + } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 79c9ef567a..762910413c 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -766,9 +766,9 @@ final class PhabricatorUSEnglishTranslation ), ), - '%d project hashtag(s) are already used: %s.' => array( - 'Project hashtag %2$s is already used.', - '%d project hashtags are already used: %2$s.', + '%s project hashtag(s) are already used by other projects: %s.' => array( + 'Project hashtag "%2$s" is already used by another project.', + 'Some project hashtags are already used by other projects: %2$s.', ), '%s changed project hashtag(s), added %d: %s; removed %d: %s.' =>