From f62494355daf5e825670a11939fa9fdf0a13f0e4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Apr 2018 05:08:02 -0700 Subject: [PATCH] Modularize Almanac Binding transactions Summary: Depends on D19320. Ref T13120. Ref T12414. Move transactions for Almanac Bindings to ModularTransactions. Test Plan: - Created a new binding. - Tried to create a duplicate binding, got an error. - Edited a binding to rebind it to a different device. - Disabled and enabled bindings. - Grepped for `AlmanacBindingTransaction::` constants. When a binding is created, it currently renders a bad "changed the interface from ??? to X" transaction. This is because creation isn't currently using EditEngine. I plan to swap it shortly, which will turn this into a real "Create" transaction and fix the issue. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13120, T12414 Differential Revision: https://secure.phabricator.com/D19321 --- src/__phutil_library_map__.php | 8 +- .../AlmanacBindingDisableController.php | 2 +- .../AlmanacBindingEditController.php | 2 +- .../almanac/editor/AlmanacBindingEditor.php | 165 +----------------- .../storage/AlmanacBindingTransaction.php | 61 +------ .../AlmanacBindingDisableTransaction.php | 28 +++ .../AlmanacBindingInterfaceTransaction.php | 111 ++++++++++++ .../xaction/AlmanacBindingTransactionType.php | 4 + 8 files changed, 159 insertions(+), 222 deletions(-) create mode 100644 src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php create mode 100644 src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php create mode 100644 src/applications/almanac/xaction/AlmanacBindingTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 57e58ac74e..cfa091f048 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -13,14 +13,17 @@ phutil_register_library_map(array( 'AlmanacAddress' => 'applications/almanac/util/AlmanacAddress.php', 'AlmanacBinding' => 'applications/almanac/storage/AlmanacBinding.php', 'AlmanacBindingDisableController' => 'applications/almanac/controller/AlmanacBindingDisableController.php', + 'AlmanacBindingDisableTransaction' => 'applications/almanac/xaction/AlmanacBindingDisableTransaction.php', 'AlmanacBindingEditController' => 'applications/almanac/controller/AlmanacBindingEditController.php', 'AlmanacBindingEditor' => 'applications/almanac/editor/AlmanacBindingEditor.php', + 'AlmanacBindingInterfaceTransaction' => 'applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php', 'AlmanacBindingPHIDType' => 'applications/almanac/phid/AlmanacBindingPHIDType.php', 'AlmanacBindingPropertyEditEngine' => 'applications/almanac/editor/AlmanacBindingPropertyEditEngine.php', 'AlmanacBindingQuery' => 'applications/almanac/query/AlmanacBindingQuery.php', 'AlmanacBindingTableView' => 'applications/almanac/view/AlmanacBindingTableView.php', 'AlmanacBindingTransaction' => 'applications/almanac/storage/AlmanacBindingTransaction.php', 'AlmanacBindingTransactionQuery' => 'applications/almanac/query/AlmanacBindingTransactionQuery.php', + 'AlmanacBindingTransactionType' => 'applications/almanac/xaction/AlmanacBindingTransactionType.php', 'AlmanacBindingViewController' => 'applications/almanac/controller/AlmanacBindingViewController.php', 'AlmanacBindingsSearchEngineAttachment' => 'applications/almanac/engineextension/AlmanacBindingsSearchEngineAttachment.php', 'AlmanacCacheEngineExtension' => 'applications/almanac/engineextension/AlmanacCacheEngineExtension.php', @@ -5179,14 +5182,17 @@ phutil_register_library_map(array( 'PhabricatorExtendedPolicyInterface', ), 'AlmanacBindingDisableController' => 'AlmanacServiceController', + 'AlmanacBindingDisableTransaction' => 'AlmanacBindingTransactionType', 'AlmanacBindingEditController' => 'AlmanacServiceController', 'AlmanacBindingEditor' => 'AlmanacEditor', + 'AlmanacBindingInterfaceTransaction' => 'AlmanacBindingTransactionType', 'AlmanacBindingPHIDType' => 'PhabricatorPHIDType', 'AlmanacBindingPropertyEditEngine' => 'AlmanacPropertyEditEngine', 'AlmanacBindingQuery' => 'AlmanacQuery', 'AlmanacBindingTableView' => 'AphrontView', - 'AlmanacBindingTransaction' => 'AlmanacTransaction', + 'AlmanacBindingTransaction' => 'PhabricatorModularTransaction', 'AlmanacBindingTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'AlmanacBindingTransactionType' => 'AlmanacTransactionType', 'AlmanacBindingViewController' => 'AlmanacServiceController', 'AlmanacBindingsSearchEngineAttachment' => 'AlmanacSearchEngineAttachment', 'AlmanacCacheEngineExtension' => 'PhabricatorCacheEngineExtension', diff --git a/src/applications/almanac/controller/AlmanacBindingDisableController.php b/src/applications/almanac/controller/AlmanacBindingDisableController.php index eb1f093125..d709db3e82 100644 --- a/src/applications/almanac/controller/AlmanacBindingDisableController.php +++ b/src/applications/almanac/controller/AlmanacBindingDisableController.php @@ -40,7 +40,7 @@ final class AlmanacBindingDisableController if ($request->isFormPost()) { - $type_disable = AlmanacBindingTransaction::TYPE_DISABLE; + $type_disable = AlmanacBindingDisableTransaction::TRANSACTIONTYPE; $xactions = array(); diff --git a/src/applications/almanac/controller/AlmanacBindingEditController.php b/src/applications/almanac/controller/AlmanacBindingEditController.php index 5f1fe30043..f33bf73ccb 100644 --- a/src/applications/almanac/controller/AlmanacBindingEditController.php +++ b/src/applications/almanac/controller/AlmanacBindingEditController.php @@ -58,7 +58,7 @@ final class AlmanacBindingEditController if ($request->isFormPost()) { $v_interface = $request->getArr('interfacePHIDs'); - $type_interface = AlmanacBindingTransaction::TYPE_INTERFACE; + $type_interface = AlmanacBindingInterfaceTransaction::TRANSACTIONTYPE; $xactions = array(); diff --git a/src/applications/almanac/editor/AlmanacBindingEditor.php b/src/applications/almanac/editor/AlmanacBindingEditor.php index 6532bf671f..d84845722c 100644 --- a/src/applications/almanac/editor/AlmanacBindingEditor.php +++ b/src/applications/almanac/editor/AlmanacBindingEditor.php @@ -3,173 +3,16 @@ final class AlmanacBindingEditor extends AlmanacEditor { - private $devicePHID; - public function getEditorObjectsDescription() { return pht('Almanac Binding'); } - public function getTransactionTypes() { - $types = parent::getTransactionTypes(); - - $types[] = AlmanacBindingTransaction::TYPE_INTERFACE; - $types[] = AlmanacBindingTransaction::TYPE_DISABLE; - - return $types; + public function getCreateObjectTitle($author, $object) { + return pht('%s created this binding.', $author); } - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case AlmanacBindingTransaction::TYPE_INTERFACE: - return $object->getInterfacePHID(); - case AlmanacBindingTransaction::TYPE_DISABLE: - return $object->getIsDisabled(); - } - - return parent::getCustomTransactionOldValue($object, $xaction); + public function getCreateObjectTitleForFeed($author, $object) { + return pht('%s created %s.', $author, $object); } - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacBindingTransaction::TYPE_INTERFACE: - return $xaction->getNewValue(); - case AlmanacBindingTransaction::TYPE_DISABLE: - return (int)$xaction->getNewValue(); - } - - return parent::getCustomTransactionNewValue($object, $xaction); - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacBindingTransaction::TYPE_INTERFACE: - $interface = id(new AlmanacInterfaceQuery()) - ->setViewer($this->requireActor()) - ->withPHIDs(array($xaction->getNewValue())) - ->executeOne(); - $object->setDevicePHID($interface->getDevicePHID()); - $object->setInterfacePHID($interface->getPHID()); - return; - case AlmanacBindingTransaction::TYPE_DISABLE: - $object->setIsDisabled($xaction->getNewValue()); - return; - } - - return parent::applyCustomInternalTransaction($object, $xaction); - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case AlmanacBindingTransaction::TYPE_DISABLE: - return; - case AlmanacBindingTransaction::TYPE_INTERFACE: - $interface_phids = array(); - - $interface_phids[] = $xaction->getOldValue(); - $interface_phids[] = $xaction->getNewValue(); - - $interface_phids = array_filter($interface_phids); - $interface_phids = array_unique($interface_phids); - - $interfaces = id(new AlmanacInterfaceQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($interface_phids) - ->execute(); - - $device_phids = array(); - foreach ($interfaces as $interface) { - $device_phids[] = $interface->getDevicePHID(); - } - - $device_phids = array_unique($device_phids); - - $devices = id(new AlmanacDeviceQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($device_phids) - ->execute(); - - foreach ($devices as $device) { - $device->rebuildClusterBindingStatus(); - } - return; - } - - return parent::applyCustomExternalTransaction($object, $xaction); - } - - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - switch ($type) { - case AlmanacBindingTransaction::TYPE_INTERFACE: - $missing = $this->validateIsEmptyTextField( - $object->getInterfacePHID(), - $xactions); - if ($missing) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('Bindings must specify an interface.'), - nonempty(last($xactions), null)); - $error->setIsMissingFieldError(true); - $errors[] = $error; - } else if ($xactions) { - foreach ($xactions as $xaction) { - $interfaces = id(new AlmanacInterfaceQuery()) - ->setViewer($this->requireActor()) - ->withPHIDs(array($xaction->getNewValue())) - ->execute(); - if (!$interfaces) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht( - 'You can not bind a service to an invalid or restricted '. - 'interface.'), - $xaction); - $errors[] = $error; - } - } - - $final_value = last($xactions)->getNewValue(); - - $binding = id(new AlmanacBindingQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withServicePHIDs(array($object->getServicePHID())) - ->withInterfacePHIDs(array($final_value)) - ->executeOne(); - if ($binding && ($binding->getID() != $object->getID())) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Already Bound'), - pht( - 'You can not bind a service to the same interface multiple '. - 'times.'), - last($xactions)); - $errors[] = $error; - } - } - break; - } - - return $errors; - } - - - } diff --git a/src/applications/almanac/storage/AlmanacBindingTransaction.php b/src/applications/almanac/storage/AlmanacBindingTransaction.php index 11120f9827..24b606639f 100644 --- a/src/applications/almanac/storage/AlmanacBindingTransaction.php +++ b/src/applications/almanac/storage/AlmanacBindingTransaction.php @@ -1,10 +1,7 @@ getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_INTERFACE: - if ($old) { - $phids[] = $old; - } - if ($new) { - $phids[] = $new; - } - break; - } - - return $phids; - } - - public function getTitle() { - $author_phid = $this->getAuthorPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_INTERFACE: - if ($old === null) { - return pht( - '%s created this binding.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s changed this binding from %s to %s.', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($old), - $this->renderHandleLink($new)); - } - break; - case self::TYPE_DISABLE: - if ($new) { - return pht( - '%s disabled this binding.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s enabled this binding.', - $this->renderHandleLink($author_phid)); - } - break; - } - - return parent::getTitle(); + public function getBaseTransactionClass() { + return 'AlmanacBindingTransactionType'; } } diff --git a/src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php b/src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php new file mode 100644 index 0000000000..1592686835 --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php @@ -0,0 +1,28 @@ +getIsDisabled(); + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + } + + public function getTitle() { + if ($this->getNewValue()) { + return pht( + '%s disabled this binding.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this binding.', + $this->renderAuthor()); + } + } + +} diff --git a/src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php b/src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php new file mode 100644 index 0000000000..f43fcdfa86 --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php @@ -0,0 +1,111 @@ +getInterfacePHID(); + } + + public function applyInternalEffects($object, $value) { + $interface = $this->loadInterface($value); + + $object + ->setDevicePHID($interface->getDevicePHID()) + ->setInterfacePHID($interface->getPHID()); + } + + public function applyExternalEffects($object, $value) { + + // When we change which services a device is bound to, we need to + // recalculate whether it is a cluster device or not so we can tell if + // the "Can Manage Cluster Services" permission applies to it. + + $viewer = PhabricatorUser::getOmnipotentUser(); + $interface_phids = array(); + + $interface_phids[] = $this->getOldValue(); + $interface_phids[] = $this->getNewValue(); + + $interface_phids = array_filter($interface_phids); + $interface_phids = array_unique($interface_phids); + + $interfaces = id(new AlmanacInterfaceQuery()) + ->setViewer($viewer) + ->withPHIDs($interface_phids) + ->execute(); + + $device_phids = array(); + foreach ($interfaces as $interface) { + $device_phids[] = $interface->getDevicePHID(); + } + + $device_phids = array_unique($device_phids); + + $devices = id(new AlmanacDeviceQuery()) + ->setViewer($viewer) + ->withPHIDs($device_phids) + ->execute(); + + foreach ($devices as $device) { + $device->rebuildClusterBindingStatus(); + } + } + + public function getTitle() { + return pht( + '%s changed the interface for this binding from %s to %s.', + $this->renderAuthor(), + $this->renderOldHandle(), + $this->renderNewHandle()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $interface_phid = $object->getInterfacePHID(); + if ($this->isEmptyTextTransaction($interface_phid, $xactions)) { + $errors[] = $this->newRequiredError( + pht('Bindings must specify an interface.')); + } + + foreach ($xactions as $xaction) { + $interface_phid = $xaction->getNewValue(); + + $interface = $this->loadInterface($interface_phid); + if (!$interface) { + $errors[] = $this->newInvalidError( + pht( + 'You can not bind a service to an invalid or restricted '. + 'interface.'), + $xaction); + continue; + } + + $binding = id(new AlmanacBindingQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withServicePHIDs(array($object->getServicePHID())) + ->withInterfacePHIDs(array($interface_phid)) + ->executeOne(); + if ($binding && ($binding->getID() != $object->getID())) { + $errors[] = $this->newInvalidError( + pht( + 'You can not bind a service to the same interface multiple '. + 'times.'), + $xaction); + continue; + } + } + + return $errors; + } + + private function loadInterface($phid) { + return id(new AlmanacInterfaceQuery()) + ->setViewer($this->getActor()) + ->withPHIDs(array($phid)) + ->executeOne(); + } +} diff --git a/src/applications/almanac/xaction/AlmanacBindingTransactionType.php b/src/applications/almanac/xaction/AlmanacBindingTransactionType.php new file mode 100644 index 0000000000..3a3d9408b5 --- /dev/null +++ b/src/applications/almanac/xaction/AlmanacBindingTransactionType.php @@ -0,0 +1,4 @@ +