From 21ba07d5bdc91bc2b4d8127ad8ed726fe7ebcb5a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 17 Dec 2011 11:58:55 -0800 Subject: [PATCH] Provide wiki pages for projects Summary: Provide tighter integration between Projects and Phriction. Partly, I have most of a rewrite for the Projects homepage ready but it's not currently possible to publish feed stories about a project so all the feeds are empty/boring. This partly makes them more useful and partly just provides a tool integration point. - When you create a project, all the wiki pages in projects//* are associated with it. - Publish updates to those pages as being related to the project so they'll show up in project feeds. - Show a project link on those pages. This is very "convention over configuration" but I think it's the right approach. We could provide some sort of, like, "@project=derp" tag to let you associated arbitrary pages to projects later, but just letting you move pages is probably far better. Test Plan: - Ran upgrade scripts against stupidly named projects ("der", " der", " der ", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects. - Ran unit tests. - Created /projects/ wiki documents and made sure they displayed correctly. - Verified feed stories publish as project-related. - Edited projects, including perfomring a name-colliding edit. - Created projects, including performing a name-colliding create. Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran, epriestley, btrahan Maniphest Tasks: T681 Differential Revision: 1231 --- resources/sql/patches/089.projectwiki.sql | 2 + .../patches/090.forceuniqueprojectnames.php | 117 ++++++++++++++++++ resources/sql/patches/091.uniqueslugkey.sql | 2 + src/__phutil_library_map__.php | 2 + .../document/PhrictionDocumentController.php | 25 +++- .../controller/document/__init__.php | 1 + .../document/PhrictionDocumentEditor.php | 27 +++- .../phriction/editor/document/__init__.php | 1 + .../storage/document/PhrictionDocument.php | 21 ++++ .../__tests__/PhrictionDocumentTestCase.php | 44 +++++++ .../PhabricatorProjectCreateController.php | 18 +-- .../project/controller/create/__init__.php | 1 + ...habricatorProjectProfileEditController.php | 12 +- .../controller/profileedit/__init__.php | 1 + .../project/PhabricatorProjectEditor.php | 103 +++++++++++++++ .../project/editor/project/__init__.php | 15 +++ ...abricatorProjectNameCollisionException.php | 25 ++++ .../exception/namecollison/__init__.php | 10 ++ .../storage/project/PhabricatorProject.php | 14 +++ .../project/storage/project/__init__.php | 1 + 20 files changed, 427 insertions(+), 15 deletions(-) create mode 100644 resources/sql/patches/089.projectwiki.sql create mode 100644 resources/sql/patches/090.forceuniqueprojectnames.php create mode 100644 resources/sql/patches/091.uniqueslugkey.sql create mode 100644 src/applications/project/editor/project/PhabricatorProjectEditor.php create mode 100644 src/applications/project/editor/project/__init__.php create mode 100644 src/applications/project/exception/namecollison/PhabricatorProjectNameCollisionException.php create mode 100644 src/applications/project/exception/namecollison/__init__.php diff --git a/resources/sql/patches/089.projectwiki.sql b/resources/sql/patches/089.projectwiki.sql new file mode 100644 index 0000000000..846947846a --- /dev/null +++ b/resources/sql/patches/089.projectwiki.sql @@ -0,0 +1,2 @@ +ALTER TABLE phabricator_project.project + ADD phrictionSlug varchar(512); diff --git a/resources/sql/patches/090.forceuniqueprojectnames.php b/resources/sql/patches/090.forceuniqueprojectnames.php new file mode 100644 index 0000000000..ba62c65d76 --- /dev/null +++ b/resources/sql/patches/090.forceuniqueprojectnames.php @@ -0,0 +1,117 @@ +loadAll(); + +$slug_map = array(); + +foreach ($projects as $project) { + $project->setPhrictionSlug($project->getName()); + $slug = $project->getPhrictionSlug(); + if ($slug == '/') { + $project_id = $project->getID(); + echo "Project #{$project_id} doesn't have a meaningful name...\n"; + $project->setName(trim('Unnamed Project '.$project->getName())); + } + $slug_map[$slug][] = $project->getID(); +} + + +foreach ($slug_map as $slug => $similar) { + if (count($similar) <= 1) { + continue; + } + echo "Too many projects are similar to '{$slug}'...\n"; + + foreach (array_slice($similar, 1, null, true) as $key => $project_id) { + $project = $projects[$project_id]; + $old_name = $project->getName(); + $new_name = rename_project($project, $projects); + + echo "Renaming project #{$project_id} ". + "from '{$old_name}' to '{$new_name}'.\n"; + $project->setName($new_name); + } +} + +$update = $projects; +while ($update) { + $size = count($update); + foreach ($update as $key => $project) { + $id = $project->getID(); + $name = $project->getName(); + $project->setPhrictionSlug($name); + $slug = $project->getPhrictionSlug(); + + echo "Updating project #{$id} '{$name}' ({$slug})..."; + try { + queryfx( + $project->establishConnection('w'), + 'UPDATE %T SET name = %s, phrictionSlug = %s WHERE id = %d', + $project->getTableName(), + $name, + $slug, + $project->getID()); + unset($update[$key]); + echo "okay.\n"; + } catch (AphrontQueryDuplicateKeyException $ex) { + echo "failed, will retry.\n"; + } + } + if (count($update) == $size) { + throw new Exception( + "Failed to make any progress while updating projects. Schema upgrade ". + "has failed. Go manually fix your project names to be unique (they are ". + "probably ridiculous?) and then try again."); + } +} + +echo "Done.\n"; + + +/** + * Rename the project so that it has a unique slug, by appending (2), (3), etc. + * to its name. + */ +function rename_project($project, $projects) { + $suffix = 2; + while (true) { + $new_name = $project->getName().' ('.$suffix.')'; + $project->setPhrictionSlug($new_name); + $new_slug = $project->getPhrictionSlug(); + + $okay = true; + foreach ($projects as $other) { + if ($other->getID() == $project->getID()) { + continue; + } + if ($other->getPhrictionSlug() == $new_slug) { + $okay = false; + break; + } + } + if ($okay) { + break; + } else { + $suffix++; + } + } + + return $new_name; +} diff --git a/resources/sql/patches/091.uniqueslugkey.sql b/resources/sql/patches/091.uniqueslugkey.sql new file mode 100644 index 0000000000..1743ed8c4b --- /dev/null +++ b/resources/sql/patches/091.uniqueslugkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE phabricator_project.project + ADD UNIQUE KEY (phrictionSlug); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 074f80cdae..6995c3f0bc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -575,7 +575,9 @@ phutil_register_library_map(array( 'PhabricatorProjectController' => 'applications/project/controller/base', 'PhabricatorProjectCreateController' => 'applications/project/controller/create', 'PhabricatorProjectDAO' => 'applications/project/storage/base', + 'PhabricatorProjectEditor' => 'applications/project/editor/project', 'PhabricatorProjectListController' => 'applications/project/controller/list', + 'PhabricatorProjectNameCollisionException' => 'applications/project/exception/namecollison', 'PhabricatorProjectProfile' => 'applications/project/storage/profile', 'PhabricatorProjectProfileController' => 'applications/project/controller/profile', 'PhabricatorProjectProfileEditController' => 'applications/project/controller/profileedit', diff --git a/src/applications/phriction/controller/document/PhrictionDocumentController.php b/src/applications/phriction/controller/document/PhrictionDocumentController.php index 91ee048f59..581a27ff33 100644 --- a/src/applications/phriction/controller/document/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/document/PhrictionDocumentController.php @@ -98,7 +98,19 @@ class PhrictionDocumentController } $page_title = $content->getTitle(); - $phids = array($content->getAuthorPHID()); + $project_phid = null; + if (PhrictionDocument::isProjectSlug($slug)) { + $project = id(new PhabricatorProject())->loadOneWhere( + 'phrictionSlug = %s', + PhrictionDocument::getProjectSlugIdentifier($slug)); + $project_phid = $project->getPHID(); + } + + $phids = array_filter( + array( + $content->getAuthorPHID(), + $project_phid, + )); $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); $age = time() - $content->getDateCreated(); @@ -112,10 +124,21 @@ class PhrictionDocumentController $when = "{$age} days ago"; } + + $project_info = null; + if ($project_phid) { + $project_info = + '
This document is about the project '. + $handles[$project_phid]->renderLink().'.'; + } + + + $byline = '
'. "Last updated {$when} by ". $handles[$content->getAuthorPHID()]->renderLink().'.'. + $project_info. '
'; $engine = PhabricatorMarkupEngine::newPhrictionMarkupEngine(); diff --git a/src/applications/phriction/controller/document/__init__.php b/src/applications/phriction/controller/document/__init__.php index 840de03791..e1951f08e5 100644 --- a/src/applications/phriction/controller/document/__init__.php +++ b/src/applications/phriction/controller/document/__init__.php @@ -15,6 +15,7 @@ phutil_require_module('phabricator', 'applications/phriction/constants/documents phutil_require_module('phabricator', 'applications/phriction/controller/base'); phutil_require_module('phabricator', 'applications/phriction/storage/content'); phutil_require_module('phabricator', 'applications/phriction/storage/document'); +phutil_require_module('phabricator', 'applications/project/storage/project'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phabricator', 'view/form/error'); diff --git a/src/applications/phriction/editor/document/PhrictionDocumentEditor.php b/src/applications/phriction/editor/document/PhrictionDocumentEditor.php index 9b7cd6de72..17b6d0c0e9 100644 --- a/src/applications/phriction/editor/document/PhrictionDocumentEditor.php +++ b/src/applications/phriction/editor/document/PhrictionDocumentEditor.php @@ -201,12 +201,28 @@ final class PhrictionDocumentEditor { $document->attachContent($new_content); PhabricatorSearchPhrictionIndexer::indexDocument($document); + $project_phid = null; + $slug = $document->getSlug(); + if (PhrictionDocument::isProjectSlug($slug)) { + $project = id(new PhabricatorProject())->loadOneWhere( + 'phrictionSlug = %s', + PhrictionDocument::getProjectSlugIdentifier($slug)); + if ($project) { + $project_phid = $project->getPHID(); + } + } + + $related_phids = array( + $document->getPHID(), + $this->user->getPHID(), + ); + + if ($project_phid) { + $related_phids[] = $project_phid; + } + id(new PhabricatorFeedStoryPublisher()) - ->setRelatedPHIDs( - array( - $document->getPHID(), - $this->user->getPHID(), - )) + ->setRelatedPHIDs($related_phids) ->setStoryAuthorPHID($this->user->getPHID()) ->setStoryTime(time()) ->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_PHRICTION) @@ -215,6 +231,7 @@ final class PhrictionDocumentEditor { 'phid' => $document->getPHID(), 'action' => $feed_action, 'content' => phutil_utf8_shorten($new_content->getContent(), 140), + 'project' => $project_phid, )) ->publish(); diff --git a/src/applications/phriction/editor/document/__init__.php b/src/applications/phriction/editor/document/__init__.php index fcd94526d1..037a05d1dd 100644 --- a/src/applications/phriction/editor/document/__init__.php +++ b/src/applications/phriction/editor/document/__init__.php @@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/phriction/constants/changetyp phutil_require_module('phabricator', 'applications/phriction/constants/documentstatus'); phutil_require_module('phabricator', 'applications/phriction/storage/content'); phutil_require_module('phabricator', 'applications/phriction/storage/document'); +phutil_require_module('phabricator', 'applications/project/storage/project'); phutil_require_module('phabricator', 'applications/search/index/indexer/phriction'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/phriction/storage/document/PhrictionDocument.php b/src/applications/phriction/storage/document/PhrictionDocument.php index 9b966ec1d6..1ec22e7511 100644 --- a/src/applications/phriction/storage/document/PhrictionDocument.php +++ b/src/applications/phriction/storage/document/PhrictionDocument.php @@ -135,4 +135,25 @@ class PhrictionDocument extends PhrictionDAO { return $this->contentObject; } + public static function isProjectSlug($slug) { + $slug = self::normalizeSlug($slug); + $prefix = 'projects/'; + if ($slug == $prefix) { + // The 'projects/' document is not itself a project slug. + return false; + } + return !strncmp($slug, $prefix, strlen($prefix)); + } + + public static function getProjectSlugIdentifier($slug) { + if (!self::isProjectSlug($slug)) { + throw new Exception("Slug '{$slug}' is not a project slug!"); + } + + $slug = self::normalizeSlug($slug); + $parts = explode('/', $slug); + return $parts[1].'/'; + } + + } diff --git a/src/applications/phriction/storage/document/__tests__/PhrictionDocumentTestCase.php b/src/applications/phriction/storage/document/__tests__/PhrictionDocumentTestCase.php index e21a0b99d4..5851755d17 100644 --- a/src/applications/phriction/storage/document/__tests__/PhrictionDocumentTestCase.php +++ b/src/applications/phriction/storage/document/__tests__/PhrictionDocumentTestCase.php @@ -26,6 +26,7 @@ class PhrictionDocumentTestCase extends PhabricatorTestCase { '' => '/', '/' => '/', '//' => '/', + '&&&' => '/', '/derp/' => 'derp/', 'derp' => 'derp/', 'derp//derp' => 'derp/derp/', @@ -72,4 +73,47 @@ class PhrictionDocumentTestCase extends PhabricatorTestCase { } } + public function testProjectSlugs() { + $slugs = array( + '/' => false, + 'zebra/' => false, + 'projects/' => false, + 'projects/a/' => true, + 'projects/a/b/' => true, + 'stuff/projects/a/' => false, + ); + + foreach ($slugs as $slug => $expect) { + $this->assertEqual( + $expect, + PhrictionDocument::isProjectSlug($slug), + "Is '{$slug}' a project slug?"); + } + } + + public function testProjectSlugIdentifiers() { + $slugs = array( + 'projects/' => null, + 'derp/' => null, + 'projects/a/' => 'a/', + 'projects/a/b/' => 'a/', + ); + + foreach ($slugs as $slug => $expect) { + $ex = null; + $result = null; + try { + $result = PhrictionDocument::getProjectSlugIdentifier($slug); + } catch (Exception $e) { + $ex = $e; + } + + if ($expect === null) { + $this->assertEqual(true, (bool)$ex, "Slug '{$slug}' is invalid."); + } else { + $this->assertEqual($expect, $result, "Slug '{$slug}' identifier."); + } + } + } + } diff --git a/src/applications/project/controller/create/PhabricatorProjectCreateController.php b/src/applications/project/controller/create/PhabricatorProjectCreateController.php index f4c8560f12..f1d486c774 100644 --- a/src/applications/project/controller/create/PhabricatorProjectCreateController.php +++ b/src/applications/project/controller/create/PhabricatorProjectCreateController.php @@ -33,18 +33,20 @@ class PhabricatorProjectCreateController $errors = array(); if ($request->isFormPost()) { - $project->setName($request->getStr('name')); + try { + $editor = new PhabricatorProjectEditor($project); + $editor->setUser($user); + $editor->setName($request->getStr('name')); + $editor->save(); + } catch (PhabricatorProjectNameCollisionException $ex) { + $e_name = 'Not Unique'; + $errors[] = $ex->getMessage(); + } + $project->setStatus(PhabricatorProjectStatus::ONGOING); $profile->setBlurb($request->getStr('blurb')); - if (!strlen($project->getName())) { - $e_name = 'Required'; - $errors[] = 'Project name is required.'; - } else { - $e_name = null; - } - if (!$errors) { $project->save(); $profile->setProjectPHID($project->getPHID()); diff --git a/src/applications/project/controller/create/__init__.php b/src/applications/project/controller/create/__init__.php index e9d5874a69..e8dccac003 100644 --- a/src/applications/project/controller/create/__init__.php +++ b/src/applications/project/controller/create/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'aphront/response/dialog'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/project/constants/status'); phutil_require_module('phabricator', 'applications/project/controller/base'); +phutil_require_module('phabricator', 'applications/project/editor/project'); phutil_require_module('phabricator', 'applications/project/storage/affiliation'); phutil_require_module('phabricator', 'applications/project/storage/profile'); phutil_require_module('phabricator', 'applications/project/storage/project'); diff --git a/src/applications/project/controller/profileedit/PhabricatorProjectProfileEditController.php b/src/applications/project/controller/profileedit/PhabricatorProjectProfileEditController.php index 07f5c058a5..64e2a79f7b 100644 --- a/src/applications/project/controller/profileedit/PhabricatorProjectProfileEditController.php +++ b/src/applications/project/controller/profileedit/PhabricatorProjectProfileEditController.php @@ -56,7 +56,17 @@ class PhabricatorProjectProfileEditController $errors = array(); $state = null; if ($request->isFormPost()) { - $project->setName($request->getStr('name')); + + try { + $editor = new PhabricatorProjectEditor($project); + $editor->setUser($user); + $editor->setName($request->getStr('name')); + $editor->save(); + } catch (PhabricatorProjectNameCollisionException $ex) { + $e_name = 'Not Unique'; + $errors[] = $ex->getMessage(); + } + $project->setStatus($request->getStr('status')); $project->setSubprojectPHIDs($request->getArr('set_subprojects')); $profile->setBlurb($request->getStr('blurb')); diff --git a/src/applications/project/controller/profileedit/__init__.php b/src/applications/project/controller/profileedit/__init__.php index 560d1d293f..df824b9bb9 100644 --- a/src/applications/project/controller/profileedit/__init__.php +++ b/src/applications/project/controller/profileedit/__init__.php @@ -13,6 +13,7 @@ phutil_require_module('phabricator', 'applications/files/transform'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'applications/project/constants/status'); phutil_require_module('phabricator', 'applications/project/controller/base'); +phutil_require_module('phabricator', 'applications/project/editor/project'); phutil_require_module('phabricator', 'applications/project/storage/affiliation'); phutil_require_module('phabricator', 'applications/project/storage/profile'); phutil_require_module('phabricator', 'applications/project/storage/project'); diff --git a/src/applications/project/editor/project/PhabricatorProjectEditor.php b/src/applications/project/editor/project/PhabricatorProjectEditor.php new file mode 100644 index 0000000000..a4338938dd --- /dev/null +++ b/src/applications/project/editor/project/PhabricatorProjectEditor.php @@ -0,0 +1,103 @@ +project = $project; + } + + public function setName($name) { + $this->projectName = $name; + return $this; + } + + public function setUser(PhabricatorUser $user) { + $this->user = $user; + return $this; + } + + public function save() { + if (!$this->user) { + throw new Exception('Call setUser() before save()!'); + } + + $project = $this->project; + + $is_new = !$project->getID(); + + if ($is_new) { + $project->setAuthorPHID($this->user->getPHID()); + } + + if (($this->projectName !== null) && + ($this->projectName !== $project->getName())) { + $project->setName($this->projectName); + $project->setPhrictionSlug($this->projectName); + $this->validateName($project); + } + + try { + $project->save(); + } catch (AphrontQueryDuplicateKeyException $ex) { + // We already validated the slug, but might race. Try again to see if + // that's the issue. If it is, we'll throw a more specific exception. If + // not, throw the original exception. + $this->validateName($project); + throw $ex; + } + + // TODO: If we rename a project, we should move its Phriction page. Do + // that once Phriction supports document moves. + + return $this; + } + + private function validateName(PhabricatorProject $project) { + $slug = $project->getPhrictionSlug(); + $name = $project->getName(); + + if ($slug == '/') { + throw new PhabricatorProjectNameCollisionException( + "Project names must be unique and contain some letters or numbers."); + } + + $id = $project->getID(); + $collision = id(new PhabricatorProject())->loadOneWhere( + '(name = %s OR phrictionSlug = %s) AND id %Q %nd', + $name, + $slug, + $id ? '!=' : 'IS NOT', + $id ? $id : null); + + if ($collision) { + $other_name = $collision->getName(); + $other_id = $collision->getID(); + throw new PhabricatorProjectNameCollisionException( + "Project names must be unique. The name '{$name}' is too similar to ". + "the name of another project, '{$other_name}' (Project ID: ". + "{$other_id}). Choose a unique name."); + } + } + +} diff --git a/src/applications/project/editor/project/__init__.php b/src/applications/project/editor/project/__init__.php new file mode 100644 index 0000000000..03b4a3c276 --- /dev/null +++ b/src/applications/project/editor/project/__init__.php @@ -0,0 +1,15 @@ +getPHID()]; } + public function setPhrictionSlug($slug) { + + // NOTE: We're doing a little magic here and stripping out '/' so that + // project pages always appear at top level under projects/ even if the + // display name is "Hack / Slash" or similar (it will become + // 'hack_slash' instead of 'hack/slash'). + + $slug = str_replace('/', ' ', $slug); + $slug = PhrictionDocument::normalizeSlug($slug); + $this->phrictionSlug = $slug; + return $this; + } + public function save() { $result = parent::save(); diff --git a/src/applications/project/storage/project/__init__.php b/src/applications/project/storage/project/__init__.php index 12477e8dc7..5f0638b4fb 100644 --- a/src/applications/project/storage/project/__init__.php +++ b/src/applications/project/storage/project/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); +phutil_require_module('phabricator', 'applications/phriction/storage/document'); phutil_require_module('phabricator', 'applications/project/constants/status'); phutil_require_module('phabricator', 'applications/project/storage/affiliation'); phutil_require_module('phabricator', 'applications/project/storage/base');