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
2024-11-26 08:42:41 +01:00 · 2018-04-10 05:08:02 -07:00 · 2018-04-10 05:08:02 -07:00 · f62494355d
commit f62494355d
parent 5ada1211cd
8 changed files with 159 additions and 222 deletions
--- 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',
--- 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();

--- 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();

--- 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();
+  public function getCreateObjectTitleForFeed($author, $object) {
+    return pht('%s created %s.', $author, $object);
  }

-    return parent::getCustomTransactionOldValue($object, $xaction);
-  }
-
-  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;
-  }
-
-
-
 }
--- a/src/applications/almanac/storage/AlmanacBindingTransaction.php
+++ b/src/applications/almanac/storage/AlmanacBindingTransaction.php
@ -1,10 +1,7 @@
 <?php

 final class AlmanacBindingTransaction
-  extends AlmanacTransaction {
-
-  const TYPE_INTERFACE = 'almanac:binding:interface';
-  const TYPE_DISABLE = 'almanac:binding:disable';
+  extends PhabricatorModularTransaction {

  public function getApplicationName() {
    return 'almanac';
@ -18,60 +15,8 @@ final class AlmanacBindingTransaction
    return null;
  }

-  public function getRequiredHandlePHIDs() {
-    $phids = parent::getRequiredHandlePHIDs();
-
-    $old = $this->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';
  }

 }
--- a/src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php
+++ b/src/applications/almanac/xaction/AlmanacBindingDisableTransaction.php
@ -0,0 +1,28 @@
+<?php
+
+final class AlmanacBindingDisableTransaction
+  extends AlmanacBindingTransactionType {
+
+  const TRANSACTIONTYPE = 'almanac:binding:disable';
+
+  public function generateOldValue($object) {
+    return (bool)$object->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());
+    }
+  }
+
+}
--- a/src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php
+++ b/src/applications/almanac/xaction/AlmanacBindingInterfaceTransaction.php
@ -0,0 +1,111 @@
+<?php
+
+final class AlmanacBindingInterfaceTransaction
+  extends AlmanacBindingTransactionType {
+
+  const TRANSACTIONTYPE = 'almanac:binding:interface';
+
+  public function generateOldValue($object) {
+    return $object->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();
+  }
+}
--- a/src/applications/almanac/xaction/AlmanacBindingTransactionType.php
+++ b/src/applications/almanac/xaction/AlmanacBindingTransactionType.php
@ -0,0 +1,4 @@
+<?php
+
+abstract class AlmanacBindingTransactionType
+  extends AlmanacTransactionType {}