From dc7d0b4a56d83a032306c9d6aada7e855641eed4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Feb 2016 06:03:41 -0800 Subject: [PATCH] Make repository callsigns optional Summary: Ref T4245. This could still use a little UI smoothing, but: - Don't require a callsign on the create flow (you can add one later in "Edit Basic Information" if you want). - Allow existing callsigns to be removed. Test Plan: - Created a new repository with no callsign. - Cloned it; pushed to it. - Browsed around Diffusion a bunch. - Visited a commit URI. - Added a callsign to it. - Removed the callsign again. - Referenced it with `R22` in remarkup. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4245 Differential Revision: https://secure.phabricator.com/D15305 --- .../sql/autopatches/20160218.callsigns.1.sql | 4 ++ .../DiffusionRepositoryCreateController.php | 61 +------------------ .../DiffusionRepositoryEditMainController.php | 7 ++- .../storage/PhabricatorRepository.php | 41 ++++++++++--- 4 files changed, 46 insertions(+), 67 deletions(-) create mode 100644 resources/sql/autopatches/20160218.callsigns.1.sql diff --git a/resources/sql/autopatches/20160218.callsigns.1.sql b/resources/sql/autopatches/20160218.callsigns.1.sql new file mode 100644 index 0000000000..09d1dd5a1b --- /dev/null +++ b/resources/sql/autopatches/20160218.callsigns.1.sql @@ -0,0 +1,4 @@ +/* Make callsigns nullable, and thus optional. */ + +ALTER TABLE {$NAMESPACE}_repository.repository + CHANGE callsign callsign VARCHAR(32) COLLATE {$COLLATE_SORT}; diff --git a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php index 4b876f3cf9..1333bf67f9 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php @@ -149,14 +149,6 @@ final class DiffusionRepositoryCreateController // If we're creating a new repository, set all this core stuff. if ($is_create) { - $callsign = $form->getPage('name') - ->getControl('callsign')->getValue(); - - // We must set this to a unique value to save the repository - // initially, and it's immutable, so we don't bother using - // transactions to apply this change. - $repository->setCallsign($callsign); - $xactions[] = id(clone $template) ->setTransactionType($type_name) ->setNewValue( @@ -343,7 +335,7 @@ final class DiffusionRepositoryCreateController } -/* -( Page: Name and Callsign )-------------------------------------------- */ +/* -( Page: Name )--------------------------------------------------------- */ private function buildNamePage() { @@ -359,23 +351,7 @@ final class DiffusionRepositoryCreateController ->addControl( id(new AphrontFormTextControl()) ->setName('name') - ->setLabel(pht('Name')) - ->setCaption(pht('Human-readable repository name.'))) - ->addRemarkupInstructions( - pht( - '**Choose a "Callsign" for the repository.** This is a short, '. - 'unique string which identifies commits elsewhere in Phabricator. '. - 'For example, you might use `M` for your mobile app repository '. - 'and `B` for your backend repository.'. - "\n\n". - '**Callsigns must be UPPERCASE**, and can not be edited after the '. - 'repository is created. Generally, you should choose short '. - 'callsigns.')) - ->addControl( - id(new AphrontFormTextControl()) - ->setName('callsign') - ->setLabel(pht('Callsign')) - ->setCaption(pht('Short UPPERCASE identifier.'))); + ->setLabel(pht('Name'))); } public function validateNamePage(PHUIFormPageView $page) { @@ -387,38 +363,7 @@ final class DiffusionRepositoryCreateController pht('You must choose a name for this repository.')); } - $c_call = $page->getControl('callsign'); - $v_call = $c_call->getValue(); - if (!strlen($v_call)) { - $c_call->setError(pht('Required')); - $page->addPageError( - pht('You must choose a callsign for this repository.')); - } else if (!preg_match('/^[A-Z]+\z/', $v_call)) { - $c_call->setError(pht('Invalid')); - $page->addPageError( - pht('The callsign must contain only UPPERCASE letters.')); - } else { - $exists = false; - try { - $repo = id(new PhabricatorRepositoryQuery()) - ->setViewer($this->getRequest()->getUser()) - ->withCallsigns(array($v_call)) - ->executeOne(); - $exists = (bool)$repo; - } catch (PhabricatorPolicyException $ex) { - $exists = true; - } - if ($exists) { - $c_call->setError(pht('Not Unique')); - $page->addPageError( - pht( - 'Another repository already uses that callsign. You must choose '. - 'a unique callsign.')); - } - } - - return $c_name->isValid() && - $c_call->isValid(); + return $c_name->isValid(); } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index 4efafc1a0e..0ec8cdd6b8 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -284,7 +284,12 @@ final class DiffusionRepositoryEditMainController $repository->getVersionControlSystem()); $view->addProperty(pht('Type'), $type); - $view->addProperty(pht('Callsign'), $repository->getCallsign()); + + $callsign = $repository->getCallsign(); + if (!strlen($callsign)) { + $callsign = phutil_tag('em', array(), pht('No Callsign')); + } + $view->addProperty(pht('Callsign'), $callsign); $short_name = $repository->getRepositorySlug(); if ($short_name === null) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index d4977d2b89..c2ed8023f3 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -93,7 +93,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ), self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort255', - 'callsign' => 'sort32', + 'callsign' => 'sort32?', 'repositorySlug' => 'sort64?', 'versionControlSystem' => 'text32', 'uuid' => 'text64?', @@ -149,13 +149,21 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } public function getMonogram() { - return 'r'.$this->getCallsign(); + $callsign = $this->getCallsign(); + if (strlen($callsign)) { + return "r{$callsign}"; + } + + $id = $this->getID(); + return "R{$id}"; } public function getDisplayName() { - // TODO: This is intended to produce a human-readable name that is not - // necessarily a global, unique identifier. Eventually, it may just return - // a string like "skynet" instead of "rSKYNET". + $slug = $this->getRepositorySlug(); + if (strlen($slug)) { + return $slug; + } + return $this->getMonogram(); } @@ -699,7 +707,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } public function getURI() { - return '/diffusion/'.$this->getCallsign().'/'; + $callsign = $this->getCallsign(); + if (strlen($callsign)) { + return "/diffusion/{$callsign}/"; + } + + $id = $this->getID(); + return "/diffusion/{$id}/"; } public function getPathURI($path) { @@ -708,7 +722,12 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO public function getCommitURI($identifier) { $callsign = $this->getCallsign(); - return "/r{$callsign}{$identifier}"; + if (strlen($callsign)) { + return "/r{$callsign}{$identifier}"; + } + + $id = $this->getID(); + return "/R{$id}:{$identifier}"; } public static function parseRepositoryServicePath($request_path) { @@ -1063,7 +1082,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } if ($need_scope) { - $scope = 'r'.$this->getCallsign(); + $callsign = $this->getCallsign(); + if ($callsign) { + $scope = "r{$callsign}"; + } else { + $id = $this->getID(); + $scope = "R{$id}:"; + } $name = $scope.$name; }