From 8500f78e451c61079ba9ca4e4f27a087e7ab1403 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 07:43:20 -0700 Subject: [PATCH] Move Files to ModularTransactions Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name. Test Plan: Created and edited files. {F4559287} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11357 Differential Revision: https://secure.phabricator.com/D17610 --- src/__phutil_library_map__.php | 6 +- .../PhabricatorBadgesBadgeNameTransaction.php | 2 +- .../PhabricatorFileEditController.php | 4 +- .../files/editor/PhabricatorFileEditor.php | 67 ------------------ .../storage/PhabricatorFileTransaction.php | 69 +------------------ .../__tests__/PhabricatorFileTestCase.php | 2 +- .../PhabricatorFileNameTransaction.php | 55 +++++++++++++++ .../PhabricatorFileTransactionType.php | 4 ++ 8 files changed, 71 insertions(+), 138 deletions(-) create mode 100644 src/applications/files/xaction/PhabricatorFileNameTransaction.php create mode 100644 src/applications/files/xaction/PhabricatorFileTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9ccc495e7a..14d84a9ae7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2765,6 +2765,7 @@ phutil_register_library_map(array( 'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', 'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php', + 'PhabricatorFileNameTransaction' => 'applications/files/xaction/PhabricatorFileNameTransaction.php', 'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php', 'PhabricatorFileROT13StorageFormat' => 'applications/files/format/PhabricatorFileROT13StorageFormat.php', 'PhabricatorFileRawStorageFormat' => 'applications/files/format/PhabricatorFileRawStorageFormat.php', @@ -2783,6 +2784,7 @@ phutil_register_library_map(array( 'PhabricatorFileTransaction' => 'applications/files/storage/PhabricatorFileTransaction.php', 'PhabricatorFileTransactionComment' => 'applications/files/storage/PhabricatorFileTransactionComment.php', 'PhabricatorFileTransactionQuery' => 'applications/files/query/PhabricatorFileTransactionQuery.php', + 'PhabricatorFileTransactionType' => 'applications/files/xaction/PhabricatorFileTransactionType.php', 'PhabricatorFileTransform' => 'applications/files/transform/PhabricatorFileTransform.php', 'PhabricatorFileTransformController' => 'applications/files/controller/PhabricatorFileTransformController.php', 'PhabricatorFileTransformListController' => 'applications/files/controller/PhabricatorFileTransformListController.php', @@ -7890,6 +7892,7 @@ phutil_register_library_map(array( 'PhabricatorFileLightboxController' => 'PhabricatorFileController', 'PhabricatorFileLinkView' => 'AphrontTagView', 'PhabricatorFileListController' => 'PhabricatorFileController', + 'PhabricatorFileNameTransaction' => 'PhabricatorFileTransactionType', 'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorFileROT13StorageFormat' => 'PhabricatorFileStorageFormat', 'PhabricatorFileRawStorageFormat' => 'PhabricatorFileStorageFormat', @@ -7905,9 +7908,10 @@ phutil_register_library_map(array( 'PhabricatorFileTestCase' => 'PhabricatorTestCase', 'PhabricatorFileTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorFileThumbnailTransform' => 'PhabricatorFileImageTransform', - 'PhabricatorFileTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorFileTransaction' => 'PhabricatorModularTransaction', 'PhabricatorFileTransactionComment' => 'PhabricatorApplicationTransactionComment', 'PhabricatorFileTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'PhabricatorFileTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorFileTransform' => 'Phobject', 'PhabricatorFileTransformController' => 'PhabricatorFileController', 'PhabricatorFileTransformListController' => 'PhabricatorFileController', diff --git a/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php b/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php index 1df7d89a70..3a609fedb2 100644 --- a/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php +++ b/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php @@ -43,7 +43,7 @@ final class PhabricatorBadgesBadgeNameTransaction $new_value = $xaction->getNewValue(); $new_length = strlen($new_value); if ($new_length > $max_length) { - $errors[] = $this->newRequiredError( + $errors[] = $this->newInvalidError( pht('The name can be no longer than %s characters.', new PhutilNumber($max_length))); } diff --git a/src/applications/files/controller/PhabricatorFileEditController.php b/src/applications/files/controller/PhabricatorFileEditController.php index e1b34afd73..a1e7a16d80 100644 --- a/src/applications/files/controller/PhabricatorFileEditController.php +++ b/src/applications/files/controller/PhabricatorFileEditController.php @@ -31,7 +31,7 @@ final class PhabricatorFileEditController extends PhabricatorFileController { $file_name = $request->getStr('name'); $errors = array(); - $type_name = PhabricatorFileTransaction::TYPE_NAME; + $type_name = PhabricatorFileNameTransaction::TRANSACTIONTYPE; $xactions = array(); @@ -40,7 +40,7 @@ final class PhabricatorFileEditController extends PhabricatorFileController { ->setNewValue($can_view); $xactions[] = id(new PhabricatorFileTransaction()) - ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME) + ->setTransactionType($type_name) ->setNewValue($file_name); $editor = id(new PhabricatorFileEditor()) diff --git a/src/applications/files/editor/PhabricatorFileEditor.php b/src/applications/files/editor/PhabricatorFileEditor.php index 9cd81a4c1e..28b781fb37 100644 --- a/src/applications/files/editor/PhabricatorFileEditor.php +++ b/src/applications/files/editor/PhabricatorFileEditor.php @@ -17,46 +17,9 @@ final class PhabricatorFileEditor $types[] = PhabricatorTransactions::TYPE_COMMENT; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; - $types[] = PhabricatorFileTransaction::TYPE_NAME; - return $types; } - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorFileTransaction::TYPE_NAME: - return $object->getName(); - } - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorFileTransaction::TYPE_NAME: - return $xaction->getNewValue(); - } - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorFileTransaction::TYPE_NAME: - $object->setName($xaction->getNewValue()); - break; - } - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) {} - protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { @@ -111,34 +74,4 @@ final class PhabricatorFileEditor return false; } - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - switch ($type) { - case PhabricatorFileTransaction::TYPE_NAME: - $missing = $this->validateIsEmptyTextField( - $object->getName(), - $xactions); - - if ($missing) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('File name is required.'), - nonempty(last($xactions), null)); - - $error->setIsMissingFieldError(true); - $errors[] = $error; - } - break; - } - - return $errors; - } - - } diff --git a/src/applications/files/storage/PhabricatorFileTransaction.php b/src/applications/files/storage/PhabricatorFileTransaction.php index 663dde28c7..3c72be6fd7 100644 --- a/src/applications/files/storage/PhabricatorFileTransaction.php +++ b/src/applications/files/storage/PhabricatorFileTransaction.php @@ -1,9 +1,7 @@ getAuthorPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - return pht( - '%s updated the name for this file from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $old, - $new); - break; - } - - return parent::getTitle(); + public function getBaseTransactionClass() { + return 'PhabricatorFileTransactionType'; } - public function getTitleForFeed() { - $author_phid = $this->getAuthorPHID(); - $object_phid = $this->getObjectPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - $type = $this->getTransactionType(); - switch ($type) { - case self::TYPE_NAME: - return pht( - '%s updated the name of %s from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid), - $old, - $new); - break; - } - - return parent::getTitleForFeed(); - } - - public function getIcon() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - return 'fa-pencil'; - } - - return parent::getIcon(); - } - - - public function getColor() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - return PhabricatorTransactions::COLOR_BLUE; - } - - return parent::getColor(); - } } diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php index 8aa22c6578..f9090c9c83 100644 --- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php +++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php @@ -32,7 +32,7 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase { // First, change the name: this should not scramble the secret. $xactions = array(); $xactions[] = id(new PhabricatorFileTransaction()) - ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME) + ->setTransactionType(PhabricatorFileNameTransaction::TRANSACTIONTYPE) ->setNewValue('test.dat2'); $engine = id(new PhabricatorFileEditor()) diff --git a/src/applications/files/xaction/PhabricatorFileNameTransaction.php b/src/applications/files/xaction/PhabricatorFileNameTransaction.php new file mode 100644 index 0000000000..7eb9396b66 --- /dev/null +++ b/src/applications/files/xaction/PhabricatorFileNameTransaction.php @@ -0,0 +1,55 @@ +getName(); + } + + public function applyInternalEffects($object, $value) { + $object->setName($value); + } + + public function getTitle() { + return pht( + '%s updated the name for this file from "%s" to "%s".', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function getTitleForFeed() { + return pht( + '%s updated the name of %s from "%s" to "%s".', + $this->renderAuthor(), + $this->renderObject(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { + $errors[] = $this->newRequiredError(pht('Files must have a name.')); + } + + $max_length = $object->getColumnMaximumByteLength('name'); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + $new_length = strlen($new_value); + if ($new_length > $max_length) { + $errors[] = $this->newInvalidError( + pht( + 'File names must not be longer than %s character(s).', + new PhutilNumber($max_length))); + } + } + + return $errors; + } + +} diff --git a/src/applications/files/xaction/PhabricatorFileTransactionType.php b/src/applications/files/xaction/PhabricatorFileTransactionType.php new file mode 100644 index 0000000000..cc708ac541 --- /dev/null +++ b/src/applications/files/xaction/PhabricatorFileTransactionType.php @@ -0,0 +1,4 @@ +