From c428f60a97f2fb8558fc1fcdf1ffcc9f2262d33f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Apr 2018 10:05:39 -0700 Subject: [PATCH] Partially modularize AlmanacService transactions Summary: Ref T13120. Ref T12414. See PHI145. See PHI473. This partially modernizes AlmanacService transactions by moving them to ModularTransactions. This isn't complete because the "update property" and "remove property" transactions aren't modularized. They still //work//, since the parent Editor implements them, but they no longer render properly on the timeline since the `Transaction` object no longer has rendering logic for them. Tentatively, I'm going to try to convert the rest of the Almanac objects and then modularize those transactions. (Currently, all of Binding, Device, Namespace and Service support properties, although they can only actually be edited on Service, Device and Binding.) If that turns out to be really tricky for some reason I can just copy/paste the timeline rendering for now, but I think it won't be too hard. Test Plan: - Created and edited Services. - Tried to create a service with: a bad name, no name, a name which put it in a namespace I can't edit (got errors in all cases). - Edited and removed properties. The edits worked, the timeline just renders a generic story now ('X edited this object (transaction type "almanac:property:update").'). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13120, T12414 Differential Revision: https://secure.phabricator.com/D19317 --- src/__phutil_library_map__.php | 8 +- .../editor/AlmanacServiceEditEngine.php | 2 +- .../almanac/editor/AlmanacServiceEditor.php | 145 +----------------- .../storage/AlmanacServiceTransaction.php | 35 ++--- .../xaction/AlmanacServiceNameTransaction.php | 87 +++++++++++ .../xaction/AlmanacServiceTransactionType.php | 4 + .../xaction/AlmanacTransactionType.php | 4 + 7 files changed, 121 insertions(+), 164 deletions(-) create mode 100644 src/applications/almanac/xaction/AlmanacServiceNameTransaction.php create mode 100644 src/applications/almanac/xaction/AlmanacServiceTransactionType.php create mode 100644 src/applications/almanac/xaction/AlmanacTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e634bb9a88..fe8e000041 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -114,6 +114,7 @@ phutil_register_library_map(array( 'AlmanacServiceEditor' => 'applications/almanac/editor/AlmanacServiceEditor.php', 'AlmanacServiceListController' => 'applications/almanac/controller/AlmanacServiceListController.php', 'AlmanacServiceNameNgrams' => 'applications/almanac/storage/AlmanacServiceNameNgrams.php', + 'AlmanacServiceNameTransaction' => 'applications/almanac/xaction/AlmanacServiceNameTransaction.php', 'AlmanacServicePHIDType' => 'applications/almanac/phid/AlmanacServicePHIDType.php', 'AlmanacServicePropertyEditEngine' => 'applications/almanac/editor/AlmanacServicePropertyEditEngine.php', 'AlmanacServiceQuery' => 'applications/almanac/query/AlmanacServiceQuery.php', @@ -121,11 +122,13 @@ phutil_register_library_map(array( 'AlmanacServiceSearchEngine' => 'applications/almanac/query/AlmanacServiceSearchEngine.php', 'AlmanacServiceTransaction' => 'applications/almanac/storage/AlmanacServiceTransaction.php', 'AlmanacServiceTransactionQuery' => 'applications/almanac/query/AlmanacServiceTransactionQuery.php', + 'AlmanacServiceTransactionType' => 'applications/almanac/xaction/AlmanacServiceTransactionType.php', 'AlmanacServiceType' => 'applications/almanac/servicetype/AlmanacServiceType.php', 'AlmanacServiceTypeDatasource' => 'applications/almanac/typeahead/AlmanacServiceTypeDatasource.php', 'AlmanacServiceTypeTestCase' => 'applications/almanac/servicetype/__tests__/AlmanacServiceTypeTestCase.php', 'AlmanacServiceViewController' => 'applications/almanac/controller/AlmanacServiceViewController.php', 'AlmanacTransaction' => 'applications/almanac/storage/AlmanacTransaction.php', + 'AlmanacTransactionType' => 'applications/almanac/xaction/AlmanacTransactionType.php', 'AphlictDropdownDataQuery' => 'applications/aphlict/query/AphlictDropdownDataQuery.php', 'Aphront304Response' => 'aphront/response/Aphront304Response.php', 'Aphront400Response' => 'aphront/response/Aphront400Response.php', @@ -5316,18 +5319,21 @@ phutil_register_library_map(array( 'AlmanacServiceEditor' => 'AlmanacEditor', 'AlmanacServiceListController' => 'AlmanacServiceController', 'AlmanacServiceNameNgrams' => 'PhabricatorSearchNgrams', + 'AlmanacServiceNameTransaction' => 'AlmanacServiceTransactionType', 'AlmanacServicePHIDType' => 'PhabricatorPHIDType', 'AlmanacServicePropertyEditEngine' => 'AlmanacPropertyEditEngine', 'AlmanacServiceQuery' => 'AlmanacQuery', 'AlmanacServiceSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'AlmanacServiceSearchEngine' => 'PhabricatorApplicationSearchEngine', - 'AlmanacServiceTransaction' => 'AlmanacTransaction', + 'AlmanacServiceTransaction' => 'PhabricatorModularTransaction', 'AlmanacServiceTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'AlmanacServiceTransactionType' => 'AlmanacTransactionType', 'AlmanacServiceType' => 'Phobject', 'AlmanacServiceTypeDatasource' => 'PhabricatorTypeaheadDatasource', 'AlmanacServiceTypeTestCase' => 'PhabricatorTestCase', 'AlmanacServiceViewController' => 'AlmanacServiceController', 'AlmanacTransaction' => 'PhabricatorApplicationTransaction', + 'AlmanacTransactionType' => 'PhabricatorModularTransactionType', 'AphlictDropdownDataQuery' => 'Phobject', 'Aphront304Response' => 'AphrontResponse', 'Aphront400Response' => 'AphrontResponse', diff --git a/src/applications/almanac/editor/AlmanacServiceEditEngine.php b/src/applications/almanac/editor/AlmanacServiceEditEngine.php index 7f5ad698fd..8cf7b97991 100644 --- a/src/applications/almanac/editor/AlmanacServiceEditEngine.php +++ b/src/applications/almanac/editor/AlmanacServiceEditEngine.php @@ -98,7 +98,7 @@ final class AlmanacServiceEditEngine ->setKey('name') ->setLabel(pht('Name')) ->setDescription(pht('Name of the service.')) - ->setTransactionType(AlmanacServiceTransaction::TYPE_NAME) + ->setTransactionType(AlmanacServiceNameTransaction::TRANSACTIONTYPE) ->setIsRequired(true) ->setValue($object->getName()), ); diff --git a/src/applications/almanac/editor/AlmanacServiceEditor.php b/src/applications/almanac/editor/AlmanacServiceEditor.php index 8b0326f749..ba509dae34 100644 --- a/src/applications/almanac/editor/AlmanacServiceEditor.php +++ b/src/applications/almanac/editor/AlmanacServiceEditor.php @@ -7,152 +7,23 @@ final class AlmanacServiceEditor return pht('Almanac Service'); } + public function getCreateObjectTitle($author, $object) { + return pht('%s created this service.', $author); + } + + public function getCreateObjectTitleForFeed($author, $object) { + return pht('%s created %s.', $author, $object); + } + public function getTransactionTypes() { $types = parent::getTransactionTypes(); - $types[] = AlmanacServiceTransaction::TYPE_NAME; - $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; return $types; } - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case AlmanacServiceTransaction::TYPE_NAME: - return $object->getName(); - } - - return parent::getCustomTransactionOldValue($object, $xaction); - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacServiceTransaction::TYPE_NAME: - return $xaction->getNewValue(); - } - - return parent::getCustomTransactionNewValue($object, $xaction); - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacServiceTransaction::TYPE_NAME: - $object->setName($xaction->getNewValue()); - return; - } - - return parent::applyCustomInternalTransaction($object, $xaction); - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacServiceTransaction::TYPE_NAME: - return; - } - - return parent::applyCustomExternalTransaction($object, $xaction); - } - - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - switch ($type) { - case AlmanacServiceTransaction::TYPE_NAME: - $missing = $this->validateIsEmptyTextField( - $object->getName(), - $xactions); - - if ($missing) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('Service name is required.'), - nonempty(last($xactions), null)); - - $error->setIsMissingFieldError(true); - $errors[] = $error; - } else { - foreach ($xactions as $xaction) { - $message = null; - - $name = $xaction->getNewValue(); - - try { - AlmanacNames::validateName($name); - } catch (Exception $ex) { - $message = $ex->getMessage(); - } - - if ($message !== null) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - $message, - $xaction); - $errors[] = $error; - continue; - } - - $other = id(new AlmanacServiceQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withNames(array($name)) - ->executeOne(); - if ($other && ($other->getID() != $object->getID())) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Not Unique'), - pht('Almanac services must have unique names.'), - last($xactions)); - $errors[] = $error; - continue; - } - - if ($name === $object->getName()) { - continue; - } - - $namespace = AlmanacNamespace::loadRestrictedNamespace( - $this->getActor(), - $name); - if ($namespace) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Restricted'), - pht( - 'You do not have permission to create Almanac services '. - 'within the "%s" namespace.', - $namespace->getName()), - $xaction); - $errors[] = $error; - continue; - } - } - } - - break; - } - - return $errors; - } - - protected function validateAllTransactions( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/almanac/storage/AlmanacServiceTransaction.php b/src/applications/almanac/storage/AlmanacServiceTransaction.php index d357641cb6..692cc2997e 100644 --- a/src/applications/almanac/storage/AlmanacServiceTransaction.php +++ b/src/applications/almanac/storage/AlmanacServiceTransaction.php @@ -1,37 +1,22 @@ getAuthorPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - if ($old === null) { - return pht( - '%s created this service.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s renamed this service from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $old, - $new); - } - break; - } - - return parent::getTitle(); + public function getBaseTransactionClass() { + return 'AlmanacServiceTransactionType'; } } diff --git a/src/applications/almanac/xaction/AlmanacServiceNameTransaction.php b/src/applications/almanac/xaction/AlmanacServiceNameTransaction.php new file mode 100644 index 0000000000..24d9857bfa --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacServiceNameTransaction.php @@ -0,0 +1,87 @@ +getName(); + } + + public function applyInternalEffects($object, $value) { + $object->setName($value); + } + + public function getTitle() { + return pht( + '%s renamed this service from %s to %s.', + $this->renderAuthorLink(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function getTitleForFeed() { + return pht( + '%s renamed %s from %s to %s.', + $this->renderAuthorLink(), + $this->renderObject(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { + $errors[] = $this->newRequiredError( + pht('Almanac services must have a name.')); + } + + foreach ($xactions as $xaction) { + $name = $xaction->getNewValue(); + + $message = null; + try { + AlmanacNames::validateName($name); + } catch (Exception $ex) { + $message = $ex->getMessage(); + } + + if ($message !== null) { + $errors[] = $this->newInvalidError($message, $xaction); + continue; + } + + if ($name === $object->getName()) { + continue; + } + + $other = id(new AlmanacServiceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withNames(array($name)) + ->executeOne(); + if ($other && ($other->getID() != $object->getID())) { + $errors[] = $this->newInvalidError( + pht('Almanac services must have unique names.'), + $xaction); + continue; + } + + $namespace = AlmanacNamespace::loadRestrictedNamespace( + $this->getActor(), + $name); + if ($namespace) { + $errors[] = $this->newInvalidError( + pht( + 'You do not have permission to create Almanac services '. + 'within the "%s" namespace.', + $namespace->getName()), + $xaction); + continue; + } + } + + return $errors; + } +} diff --git a/src/applications/almanac/xaction/AlmanacServiceTransactionType.php b/src/applications/almanac/xaction/AlmanacServiceTransactionType.php new file mode 100644 index 0000000000..0e062806fd --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacServiceTransactionType.php @@ -0,0 +1,4 @@ +