From 71c77fcc3a153801fa6ec464e4038e2a54d6bfeb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 10 Apr 2018 08:39:10 -0700
Subject: [PATCH] Modularize transactions for Almanac Device
Summary:
Depends on D19328. Ref T13120. Ref T12414.
Prior work has left us with just a NAME transaction here, which is straightforward to modularize.
Test Plan:
- Created and renamed devices.
- Tried to set no name, a bad name, a duplicate name (got errors).
- Tried to create/rename into a namespace I could not edit (got an error).
- Grepped for `AlmanacDeviceTransaction::`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13120, T12414
Differential Revision: https://secure.phabricator.com/D19329
---
src/__phutil_library_map__.php | 6 +-
.../editor/AlmanacDeviceEditEngine.php | 2 +-
.../almanac/editor/AlmanacDeviceEditor.php | 135 ------------------
.../storage/AlmanacDeviceTransaction.php | 29 +---
.../xaction/AlmanacDeviceNameTransaction.php | 79 ++++++++++
.../xaction/AlmanacDeviceTransactionType.php | 4 +
6 files changed, 92 insertions(+), 163 deletions(-)
create mode 100644 src/applications/almanac/xaction/AlmanacDeviceNameTransaction.php
create mode 100644 src/applications/almanac/xaction/AlmanacDeviceTransactionType.php
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 955405f30c..6900238d6f 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -46,6 +46,7 @@ phutil_register_library_map(array(
'AlmanacDeviceEditor' => 'applications/almanac/editor/AlmanacDeviceEditor.php',
'AlmanacDeviceListController' => 'applications/almanac/controller/AlmanacDeviceListController.php',
'AlmanacDeviceNameNgrams' => 'applications/almanac/storage/AlmanacDeviceNameNgrams.php',
+ 'AlmanacDeviceNameTransaction' => 'applications/almanac/xaction/AlmanacDeviceNameTransaction.php',
'AlmanacDevicePHIDType' => 'applications/almanac/phid/AlmanacDevicePHIDType.php',
'AlmanacDevicePropertyEditEngine' => 'applications/almanac/editor/AlmanacDevicePropertyEditEngine.php',
'AlmanacDeviceQuery' => 'applications/almanac/query/AlmanacDeviceQuery.php',
@@ -53,6 +54,7 @@ phutil_register_library_map(array(
'AlmanacDeviceSearchEngine' => 'applications/almanac/query/AlmanacDeviceSearchEngine.php',
'AlmanacDeviceTransaction' => 'applications/almanac/storage/AlmanacDeviceTransaction.php',
'AlmanacDeviceTransactionQuery' => 'applications/almanac/query/AlmanacDeviceTransactionQuery.php',
+ 'AlmanacDeviceTransactionType' => 'applications/almanac/xaction/AlmanacDeviceTransactionType.php',
'AlmanacDeviceViewController' => 'applications/almanac/controller/AlmanacDeviceViewController.php',
'AlmanacDrydockPoolServiceType' => 'applications/almanac/servicetype/AlmanacDrydockPoolServiceType.php',
'AlmanacEditor' => 'applications/almanac/editor/AlmanacEditor.php',
@@ -5237,13 +5239,15 @@ phutil_register_library_map(array(
'AlmanacDeviceEditor' => 'AlmanacEditor',
'AlmanacDeviceListController' => 'AlmanacDeviceController',
'AlmanacDeviceNameNgrams' => 'PhabricatorSearchNgrams',
+ 'AlmanacDeviceNameTransaction' => 'AlmanacDeviceTransactionType',
'AlmanacDevicePHIDType' => 'PhabricatorPHIDType',
'AlmanacDevicePropertyEditEngine' => 'AlmanacPropertyEditEngine',
'AlmanacDeviceQuery' => 'AlmanacQuery',
'AlmanacDeviceSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
'AlmanacDeviceSearchEngine' => 'PhabricatorApplicationSearchEngine',
- 'AlmanacDeviceTransaction' => 'AlmanacTransaction',
+ 'AlmanacDeviceTransaction' => 'PhabricatorModularTransaction',
'AlmanacDeviceTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+ 'AlmanacDeviceTransactionType' => 'AlmanacTransactionType',
'AlmanacDeviceViewController' => 'AlmanacDeviceController',
'AlmanacDrydockPoolServiceType' => 'AlmanacServiceType',
'AlmanacEditor' => 'PhabricatorApplicationTransactionEditor',
diff --git a/src/applications/almanac/editor/AlmanacDeviceEditEngine.php b/src/applications/almanac/editor/AlmanacDeviceEditEngine.php
index 1d325e403b..84ca59d310 100644
--- a/src/applications/almanac/editor/AlmanacDeviceEditEngine.php
+++ b/src/applications/almanac/editor/AlmanacDeviceEditEngine.php
@@ -80,7 +80,7 @@ final class AlmanacDeviceEditEngine
->setKey('name')
->setLabel(pht('Name'))
->setDescription(pht('Name of the device.'))
- ->setTransactionType(AlmanacDeviceTransaction::TYPE_NAME)
+ ->setTransactionType(AlmanacDeviceNameTransaction::TRANSACTIONTYPE)
->setIsRequired(true)
->setValue($object->getName()),
);
diff --git a/src/applications/almanac/editor/AlmanacDeviceEditor.php b/src/applications/almanac/editor/AlmanacDeviceEditor.php
index 5a4246231e..ea04bffc07 100644
--- a/src/applications/almanac/editor/AlmanacDeviceEditor.php
+++ b/src/applications/almanac/editor/AlmanacDeviceEditor.php
@@ -10,145 +10,10 @@ final class AlmanacDeviceEditor
public function getTransactionTypes() {
$types = parent::getTransactionTypes();
- $types[] = AlmanacDeviceTransaction::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 AlmanacDeviceTransaction::TYPE_NAME:
- return $object->getName();
- }
-
- return parent::getCustomTransactionOldValue($object, $xaction);
- }
-
- protected function getCustomTransactionNewValue(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
-
- switch ($xaction->getTransactionType()) {
- case AlmanacDeviceTransaction::TYPE_NAME:
- return $xaction->getNewValue();
- }
-
- return parent::getCustomTransactionNewValue($object, $xaction);
- }
-
- protected function applyCustomInternalTransaction(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
-
- switch ($xaction->getTransactionType()) {
- case AlmanacDeviceTransaction::TYPE_NAME:
- $object->setName($xaction->getNewValue());
- return;
- }
-
- return parent::applyCustomInternalTransaction($object, $xaction);
- }
-
- protected function applyCustomExternalTransaction(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
-
- switch ($xaction->getTransactionType()) {
- case AlmanacDeviceTransaction::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 AlmanacDeviceTransaction::TYPE_NAME:
- $missing = $this->validateIsEmptyTextField(
- $object->getName(),
- $xactions);
-
- if ($missing) {
- $error = new PhabricatorApplicationTransactionValidationError(
- $type,
- pht('Required'),
- pht('Device 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 AlmanacDeviceQuery())
- ->setViewer(PhabricatorUser::getOmnipotentUser())
- ->withNames(array($name))
- ->executeOne();
- if ($other && ($other->getID() != $object->getID())) {
- $error = new PhabricatorApplicationTransactionValidationError(
- $type,
- pht('Not Unique'),
- pht('Almanac devices must have unique names.'),
- $xaction);
- $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 devices '.
- 'within the "%s" namespace.',
- $namespace->getName()),
- $xaction);
- $errors[] = $error;
- continue;
- }
- }
- }
-
- break;
- }
-
- return $errors;
- }
-
}
diff --git a/src/applications/almanac/storage/AlmanacDeviceTransaction.php b/src/applications/almanac/storage/AlmanacDeviceTransaction.php
index b70ffbe008..e74def1a9b 100644
--- a/src/applications/almanac/storage/AlmanacDeviceTransaction.php
+++ b/src/applications/almanac/storage/AlmanacDeviceTransaction.php
@@ -1,9 +1,7 @@
<?php
final class AlmanacDeviceTransaction
- extends AlmanacTransaction {
-
- const TYPE_NAME = 'almanac:device:name';
+ extends PhabricatorModularTransaction {
public function getApplicationName() {
return 'almanac';
@@ -17,29 +15,8 @@ final class AlmanacDeviceTransaction
return null;
}
- public function getTitle() {
- $author_phid = $this->getAuthorPHID();
-
- $old = $this->getOldValue();
- $new = $this->getNewValue();
-
- switch ($this->getTransactionType()) {
- case self::TYPE_NAME:
- if ($old === null) {
- return pht(
- '%s created this device.',
- $this->renderHandleLink($author_phid));
- } else {
- return pht(
- '%s renamed this device from "%s" to "%s".',
- $this->renderHandleLink($author_phid),
- $old,
- $new);
- }
- break;
- }
-
- return parent::getTitle();
+ public function getBaseTransactionClass() {
+ return 'AlmanacDeviceTransactionType';
}
}
diff --git a/src/applications/almanac/xaction/AlmanacDeviceNameTransaction.php b/src/applications/almanac/xaction/AlmanacDeviceNameTransaction.php
new file mode 100644
index 0000000000..d72b380472
--- /dev/null
+++ b/src/applications/almanac/xaction/AlmanacDeviceNameTransaction.php
@@ -0,0 +1,79 @@
+<?php
+
+final class AlmanacDeviceNameTransaction
+ extends AlmanacDeviceTransactionType {
+
+ const TRANSACTIONTYPE = 'almanac:device:name';
+
+ public function generateOldValue($object) {
+ return $object->getName();
+ }
+
+ public function applyInternalEffects($object, $value) {
+ $object->setName($value);
+ }
+
+ public function getTitle() {
+ return pht(
+ '%s renamed this device from %s to %s.',
+ $this->renderAuthor(),
+ $this->renderOldValue(),
+ $this->renderNewValue());
+ }
+
+ public function validateTransactions($object, array $xactions) {
+ $errors = array();
+
+ if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
+ $errors[] = $this->newRequiredError(
+ pht('Device name is required.'));
+ }
+
+ 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 AlmanacDeviceQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withNames(array($name))
+ ->executeOne();
+ if ($other && ($other->getID() != $object->getID())) {
+ $errors[] = $this->newInvalidError(
+ pht('Almanac devices 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 devices '.
+ 'within the "%s" namespace.',
+ $namespace->getName()),
+ $xaction);
+ continue;
+ }
+ }
+
+ return $errors;
+ }
+
+}
diff --git a/src/applications/almanac/xaction/AlmanacDeviceTransactionType.php b/src/applications/almanac/xaction/AlmanacDeviceTransactionType.php
new file mode 100644
index 0000000000..cf3b239ab2
--- /dev/null
+++ b/src/applications/almanac/xaction/AlmanacDeviceTransactionType.php
@@ -0,0 +1,4 @@
+<?php
+
+abstract class AlmanacDeviceTransactionType
+ extends AlmanacTransactionType {}