From fbfe7304521464151986aee99d71fa07b20e0768 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 16 Apr 2016 18:17:13 -0700 Subject: [PATCH] Support more transactions types in RepositoryEditEngine Summary: Ref T10748. This supports more transaction types in the modern editor and improves validation so Conduit benefits. You can technically create repositories via `diffusion.repository.edit` now, although they aren't very useful. Test Plan: - Used `diffusion.repository.edit` to create and edit repositories. - Used `/editpro/` to edit repositories. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15740 --- .../DiffusionRepositoryCreateController.php | 8 +- ...fusionRepositoryEditActivateController.php | 9 +- .../editor/DiffusionRepositoryEditEngine.php | 88 ++++++++++++ .../constants/PhabricatorRepositoryType.php | 8 +- .../editor/PhabricatorRepositoryEditor.php | 131 +++++++++++++++--- .../storage/PhabricatorRepository.php | 49 ++++++- .../PhabricatorRepositoryTransaction.php | 30 ++-- 7 files changed, 284 insertions(+), 39 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php index ae28bf3993..09fdd23565 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php @@ -162,9 +162,15 @@ final class DiffusionRepositoryCreateController $activate = $form->getPage('done') ->getControl('activate')->getValue(); + if ($activate == 'start') { + $initial_status = PhabricatorRepository::STATUS_ACTIVE; + } else { + $initial_status = PhabricatorRepository::STATUS_INACTIVE; + } + $xactions[] = id(clone $template) ->setTransactionType($type_activate) - ->setNewValue(($activate == 'start')); + ->setNewValue($initial_status); if ($service) { $xactions[] = id(clone $template) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditActivateController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditActivateController.php index c8428833ee..55caaa59b9 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditActivateController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditActivateController.php @@ -16,12 +16,19 @@ final class DiffusionRepositoryEditActivateController $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); if ($request->isFormPost()) { + if (!$repository->isTracked()) { + $new_status = PhabricatorRepository::STATUS_ACTIVE; + } else { + $new_status = PhabricatorRepository::STATUS_INACTIVE; + } + $xaction = id(new PhabricatorRepositoryTransaction()) ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ACTIVATE) - ->setNewValue(!$repository->isTracked()); + ->setNewValue($new_status); $editor = id(new PhabricatorRepositoryEditor()) ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) ->setContentSourceFromRequest($request) ->setActor($viewer) ->applyTransactions($repository, array($xaction)); diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 2bf99c1bc5..602a3059d6 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -68,7 +68,28 @@ final class DiffusionRepositoryEditEngine } protected function buildCustomEditFields($object) { + $viewer = $this->getViewer(); + + $policies = id(new PhabricatorPolicyQuery()) + ->setViewer($viewer) + ->setObject($object) + ->execute(); + return array( + id(new PhabricatorSelectEditField()) + ->setKey('vcs') + ->setLabel(pht('Version Control System')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_VCS) + ->setIsConduitOnly(true) + ->setIsCopyable(true) + ->setOptions(PhabricatorRepositoryType::getAllRepositoryTypes()) + ->setDescription(pht('Underlying repository version control system.')) + ->setConduitDescription( + pht( + 'Choose which version control system to use when creating a '. + 'repository.')) + ->setConduitTypeDescription(pht('Version control system selection.')) + ->setValue($object->getVersionControlSystem()), id(new PhabricatorTextEditField()) ->setKey('name') ->setLabel(pht('Name')) @@ -78,6 +99,73 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Rename the repository.')) ->setConduitTypeDescription(pht('New repository name.')) ->setValue($object->getName()), + id(new PhabricatorTextEditField()) + ->setKey('callsign') + ->setLabel(pht('Callsign')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_CALLSIGN) + ->setDescription(pht('The repository callsign.')) + ->setConduitDescription(pht('Change the repository callsign.')) + ->setConduitTypeDescription(pht('New repository callsign.')) + ->setValue($object->getCallsign()), + id(new PhabricatorTextEditField()) + ->setKey('shortName') + ->setLabel(pht('Short Name')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_SLUG) + ->setDescription(pht('Short, unique repository name.')) + ->setConduitDescription(pht('Change the repository short name.')) + ->setConduitTypeDescription(pht('New short name for the repository.')) + ->setValue($object->getRepositorySlug()), + id(new PhabricatorRemarkupEditField()) + ->setKey('description') + ->setLabel(pht('Description')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_DESCRIPTION) + ->setDescription(pht('Repository description.')) + ->setConduitDescription(pht('Change the repository description.')) + ->setConduitTypeDescription(pht('New repository description.')) + ->setValue($object->getDetail('description')), + id(new PhabricatorTextEditField()) + ->setKey('encoding') + ->setLabel(pht('Text Encoding')) + ->setIsCopyable(true) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ENCODING) + ->setDescription(pht('Default text encoding.')) + ->setConduitDescription(pht('Change the default text encoding.')) + ->setConduitTypeDescription(pht('New text encoding.')) + ->setValue($object->getDetail('encoding')), + id(new PhabricatorSelectEditField()) + ->setKey('status') + ->setLabel(pht('Status')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ACTIVATE) + ->setIsConduitOnly(true) + ->setOptions(PhabricatorRepository::getStatusNameMap()) + ->setDescription(pht('Active or inactive status.')) + ->setConduitDescription(pht('Active or deactivate the repository.')) + ->setConduitTypeDescription(pht('New repository status.')) + ->setValue($object->getStatus()), + id(new PhabricatorTextEditField()) + ->setKey('defaultBranch') + ->setLabel(pht('Default Branch')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_DEFAULT_BRANCH) + ->setIsCopyable(true) + ->setDescription(pht('Default branch name.')) + ->setConduitDescription(pht('Set the default branch name.')) + ->setConduitTypeDescription(pht('New default branch name.')) + ->setValue($object->getDetail('default-branch')), + id(new PhabricatorPolicyEditField()) + ->setKey('policy.push') + ->setLabel(pht('Push Policy')) + ->setAliases(array('push')) + ->setIsCopyable(true) + ->setCapability(DiffusionPushCapability::CAPABILITY) + ->setPolicies($policies) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY) + ->setDescription( + pht('Controls who can push changes to the repository.')) + ->setConduitDescription( + pht('Change the push policy of the repository.')) + ->setConduitTypeDescription(pht('New policy PHID or constant.')) + ->setValue($object->getPolicy(DiffusionPushCapability::CAPABILITY)), ); } diff --git a/src/applications/repository/constants/PhabricatorRepositoryType.php b/src/applications/repository/constants/PhabricatorRepositoryType.php index 557d77ac07..4861641411 100644 --- a/src/applications/repository/constants/PhabricatorRepositoryType.php +++ b/src/applications/repository/constants/PhabricatorRepositoryType.php @@ -7,10 +7,10 @@ final class PhabricatorRepositoryType extends Phobject { const REPOSITORY_TYPE_MERCURIAL = 'hg'; public static function getAllRepositoryTypes() { - static $map = array( - self::REPOSITORY_TYPE_GIT => 'Git', - self::REPOSITORY_TYPE_SVN => 'Subversion', - self::REPOSITORY_TYPE_MERCURIAL => 'Mercurial', + $map = array( + self::REPOSITORY_TYPE_GIT => pht('Git'), + self::REPOSITORY_TYPE_MERCURIAL => pht('Mercurial'), + self::REPOSITORY_TYPE_SVN => pht('Subversion'), ); return $map; } diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index fa585a826a..5de7f068e7 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -248,25 +248,7 @@ final class PhabricatorRepositoryEditor $object->setCallsign($xaction->getNewValue()); return; case PhabricatorRepositoryTransaction::TYPE_ENCODING: - // Make sure the encoding is valid by converting to UTF-8. This tests - // that the user has mbstring installed, and also that they didn't type - // a garbage encoding name. Note that we're converting from UTF-8 to - // the target encoding, because mbstring is fine with converting from - // a nonsense encoding. - $encoding = $xaction->getNewValue(); - if (strlen($encoding)) { - try { - phutil_utf8_convert('.', $encoding, 'UTF-8'); - } catch (Exception $ex) { - throw new PhutilProxyException( - pht( - "Error setting repository encoding '%s': %s'", - $encoding, - $ex->getMessage()), - $ex); - } - } - $object->setDetail('encoding', $encoding); + $object->setDetail('encoding', $xaction->getNewValue()); break; } } @@ -461,6 +443,117 @@ final class PhabricatorRepositoryEditor } break; + case PhabricatorRepositoryTransaction::TYPE_VCS: + $vcs_map = PhabricatorRepositoryType::getAllRepositoryTypes(); + $current_vcs = $object->getVersionControlSystem(); + + if (!$this->getIsNewObject()) { + foreach ($xactions as $xaction) { + if ($xaction->getNewValue() == $current_vcs) { + continue; + } + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Immutable'), + pht( + 'You can not change the version control system an existing '. + 'repository uses. It can only be set when a repository is '. + 'first created.'), + $xaction); + } + } else { + $value = $object->getVersionControlSystem(); + foreach ($xactions as $xaction) { + $value = $xaction->getNewValue(); + + if (empty($vcs_map[$value])) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Specified version control system must be a VCS '. + 'recognized by Phabricator: %s.', + implode(', ', array_keys($vcs_map))), + $xaction); + } + } + + if (!strlen($value)) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht( + 'When creating a repository, you must specify a valid '. + 'underlying version control system: %s.', + implode(', ', array_keys($vcs_map))), + nonempty(last($xactions), null)); + $error->setIsMissingFieldError(true); + $errors[] = $error; + } + } + break; + + case PhabricatorRepositoryTransaction::TYPE_NAME: + $missing = $this->validateIsEmptyTextField( + $object->getName(), + $xactions); + + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('Repository name is required.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + } + break; + + case PhabricatorRepositoryTransaction::TYPE_ACTIVATE: + $status_map = PhabricatorRepository::getStatusMap(); + foreach ($xactions as $xaction) { + $status = $xaction->getNewValue(); + if (empty($status_map[$status])) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Repository status "%s" is not valid.', + $status), + $xaction); + } + } + break; + + case PhabricatorRepositoryTransaction::TYPE_ENCODING: + foreach ($xactions as $xaction) { + // Make sure the encoding is valid by converting to UTF-8. This tests + // that the user has mbstring installed, and also that they didn't + // type a garbage encoding name. Note that we're converting from + // UTF-8 to the target encoding, because mbstring is fine with + // converting from a nonsense encoding. + $encoding = $xaction->getNewValue(); + if (!strlen($encoding)) { + continue; + } + + try { + phutil_utf8_convert('.', $encoding, 'UTF-8'); + } catch (Exception $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Repository encoding "%s" is not valid: %s', + $encoding, + $ex->getMessage()), + $xaction); + } + } + break; + case PhabricatorRepositoryTransaction::TYPE_SLUG: foreach ($xactions as $xaction) { $old = $xaction->getOldValue(); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 3c298fb286..18e45359ce 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -46,6 +46,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO const BECAUSE_BRANCH_NOT_AUTOCLOSE = 'auto/noclose'; const BECAUSE_AUTOCLOSE_FORCED = 'auto/forced'; + const STATUS_ACTIVE = 'active'; + const STATUS_INACTIVE = 'inactive'; + protected $name; protected $callsign; protected $repositorySlug; @@ -132,6 +135,31 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO PhabricatorRepositoryRepositoryPHIDType::TYPECONST); } + public static function getStatusMap() { + return array( + self::STATUS_ACTIVE => array( + 'name' => pht('Active'), + 'isTracked' => 1, + ), + self::STATUS_INACTIVE => array( + 'name' => pht('Inactive'), + 'isTracked' => 0, + ), + ); + } + + public static function getStatusNameMap() { + return ipull(self::getStatusMap(), 'name'); + } + + public function getStatus() { + if ($this->isTracked()) { + return self::STATUS_ACTIVE; + } else { + return self::STATUS_INACTIVE; + } + } + public function toDictionary() { return array( 'id' => $this->getID(), @@ -146,7 +174,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'isActive' => $this->isTracked(), 'isHosted' => $this->isHosted(), 'isImporting' => $this->isImporting(), - 'encoding' => $this->getDetail('encoding'), + 'encoding' => $this->getDefaultTextEncoding(), 'staging' => array( 'supported' => $this->supportsStaging(), 'prefix' => 'phabricator', @@ -155,6 +183,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ); } + public function getDefaultTextEncoding() { + return $this->getDetail('encoding', 'UTF-8'); + } + public function getMonogram() { $callsign = $this->getCallsign(); if (strlen($callsign)) { @@ -994,7 +1026,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } public function isTracked() { - return $this->getDetail('tracking-enabled', false); + $status = $this->getDetail('tracking-enabled'); + $map = self::getStatusMap(); + $spec = idx($map, $status); + + if (!$spec) { + if ($status) { + $status = self::STATUS_ACTIVE; + } else { + $status = self::STATUS_INACTIVE; + } + $spec = idx($map, $status); + } + + return (bool)idx($spec, 'isTracked', false); } public function getDefaultBranch() { diff --git a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php index 20c975cb33..76e75a70b0 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php @@ -4,24 +4,17 @@ final class PhabricatorRepositoryTransaction extends PhabricatorApplicationTransaction { const TYPE_VCS = 'repo:vcs'; - const TYPE_ACTIVATE = 'repo:activate'; - const TYPE_NAME = 'repo:name'; - const TYPE_DESCRIPTION = 'repo:description'; - const TYPE_ENCODING = 'repo:encoding'; + const TYPE_ACTIVATE = 'repo:activate'; + const TYPE_NAME = 'repo:name'; + const TYPE_DESCRIPTION = 'repo:description'; + const TYPE_ENCODING = 'repo:encoding'; const TYPE_DEFAULT_BRANCH = 'repo:default-branch'; const TYPE_TRACK_ONLY = 'repo:track-only'; const TYPE_AUTOCLOSE_ONLY = 'repo:autoclose-only'; const TYPE_SVN_SUBPATH = 'repo:svn-subpath'; - const TYPE_UUID = 'repo:uuid'; const TYPE_NOTIFY = 'repo:notify'; const TYPE_AUTOCLOSE = 'repo:autoclose'; - const TYPE_REMOTE_URI = 'repo:remote-uri'; - const TYPE_LOCAL_PATH = 'repo:local-path'; - const TYPE_HOSTING = 'repo:hosting'; - const TYPE_PROTOCOL_HTTP = 'repo:serve-http'; - const TYPE_PROTOCOL_SSH = 'repo:serve-ssh'; const TYPE_PUSH_POLICY = 'repo:push-policy'; - const TYPE_CREDENTIAL = 'repo:credential'; const TYPE_DANGEROUS = 'repo:dangerous'; const TYPE_SLUG = 'repo:slug'; const TYPE_SERVICE = 'repo:service'; @@ -37,6 +30,13 @@ final class PhabricatorRepositoryTransaction const TYPE_SSH_KEYFILE = 'repo:ssh-keyfile'; const TYPE_HTTP_LOGIN = 'repo:http-login'; const TYPE_HTTP_PASS = 'repo:http-pass'; + const TYPE_CREDENTIAL = 'repo:credential'; + const TYPE_PROTOCOL_HTTP = 'repo:serve-http'; + const TYPE_PROTOCOL_SSH = 'repo:serve-ssh'; + const TYPE_HOSTING = 'repo:hosting'; + const TYPE_LOCAL_PATH = 'repo:local-path'; + const TYPE_REMOTE_URI = 'repo:remote-uri'; + const TYPE_UUID = 'repo:uuid'; public function getApplicationName() { return 'repository'; @@ -134,7 +134,13 @@ final class PhabricatorRepositoryTransaction '%s created this repository.', $this->renderHandleLink($author_phid)); case self::TYPE_ACTIVATE: - if ($new) { + // TODO: Old versions of this transaction use a boolean value, but + // should be migrated. + $is_deactivate = + (!$new) || + ($new == PhabricatorRepository::STATUS_INACTIVE); + + if (!$is_deactivate) { return pht( '%s activated this repository.', $this->renderHandleLink($author_phid));