From 3dae9701298fe4a85d5e9b039bc86e807555777c Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 8 May 2017 16:29:27 -0700 Subject: [PATCH] Clean up some rough Macro transaction edges Summary: Ref T12685, cleans up various macro issues, remove subscribers, fix feed stories, etc. Test Plan: Create a new macro, see no subscribers, edit various macros. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T12685 Differential Revision: https://secure.phabricator.com/D17848 --- .../editor/PhabricatorMacroEditEngine.php | 23 +++++++++- .../PhabricatorMacroFileTransaction.php | 42 +++++++++++-------- .../PhabricatorMacroNameTransaction.php | 3 +- ...icatorSubscriptionsEditEngineExtension.php | 3 +- 4 files changed, 50 insertions(+), 21 deletions(-) diff --git a/src/applications/macro/editor/PhabricatorMacroEditEngine.php b/src/applications/macro/editor/PhabricatorMacroEditEngine.php index 2cafea937f..95627d2e11 100644 --- a/src/applications/macro/editor/PhabricatorMacroEditEngine.php +++ b/src/applications/macro/editor/PhabricatorMacroEditEngine.php @@ -21,6 +21,10 @@ final class PhabricatorMacroEditEngine return 'PhabricatorMacroApplication'; } + public function isEngineConfigurable() { + return false; + } + protected function newEditableObject() { $viewer = $this->getViewer(); return PhabricatorFileImageMacro::initializeNewFileImageMacro($viewer); @@ -35,7 +39,7 @@ final class PhabricatorMacroEditEngine } protected function getObjectEditTitleText($object) { - return pht('Edit %s', $object->getName()); + return pht('Edit Macro %s', $object->getName()); } protected function getObjectEditShortText($object) { @@ -63,6 +67,19 @@ final class PhabricatorMacroEditEngine PhabricatorMacroManageCapability::CAPABILITY); } + protected function willConfigureFields($object, array $fields) { + if ($this->getIsCreate()) { + $subscribers_field = idx($fields, + PhabricatorSubscriptionsEditEngineExtension::FIELDKEY); + if ($subscribers_field) { + // By default, hide the subscribers field when creating a macro + // because it makes the workflow SO HARD and wastes SO MUCH TIME. + $subscribers_field->setIsHidden(true); + } + } + return $fields; + } + protected function buildCustomEditFields($object) { return array( @@ -73,6 +90,7 @@ final class PhabricatorMacroEditEngine ->setConduitDescription(pht('Rename the macro.')) ->setConduitTypeDescription(pht('New macro name.')) ->setTransactionType(PhabricatorMacroNameTransaction::TRANSACTIONTYPE) + ->setIsRequired(true) ->setValue($object->getName()), id(new PhabricatorFileEditField()) ->setKey('filePHID') @@ -80,7 +98,8 @@ final class PhabricatorMacroEditEngine ->setDescription(pht('Image file to import.')) ->setTransactionType(PhabricatorMacroFileTransaction::TRANSACTIONTYPE) ->setConduitDescription(pht('File PHID to import.')) - ->setConduitTypeDescription(pht('File PHID.')), + ->setConduitTypeDescription(pht('File PHID.')) + ->setValue($object->getFilePHID()), ); } diff --git a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php index 35091cd585..fb0c56f1c1 100644 --- a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php @@ -49,7 +49,7 @@ final class PhabricatorMacroFileTransaction public function getTitleForFeed() { return pht( - '%s changed the image for macro %s.', + '%s changed the image for %s.', $this->renderAuthor(), $this->renderObject()); } @@ -58,29 +58,37 @@ final class PhabricatorMacroFileTransaction $errors = array(); $viewer = $this->getActor(); + $old_phid = $object->getFilePHID(); + foreach ($xactions as $xaction) { $file_phid = $xaction->getNewValue(); - if ($this->isEmptyTextTransaction($file_phid, $xactions)) { - $errors[] = $this->newRequiredError( - pht('Image macros must have a file.')); + if (!$old_phid) { + if ($this->isEmptyTextTransaction($file_phid, $xactions)) { + $errors[] = $this->newRequiredError( + pht('Image macros must have a file.')); + return $errors; + } } - $file = id(new PhabricatorFileQuery()) - ->setViewer($viewer) - ->withPHIDs(array($file_phid)) - ->executeOne(); + // Only validate if file was uploaded + if ($file_phid) { + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); - if (!$file) { - $errors[] = $this->newInvalidError( - pht('"%s" is not a valid file PHID.', - $file_phid)); - } else { - if (!$file->isViewableInBrowser()) { - $mime_type = $file->getMimeType(); + if (!$file) { $errors[] = $this->newInvalidError( - pht('File mime type of "%s" is not a valid viewable image.', - $mime_type)); + pht('"%s" is not a valid file PHID.', + $file_phid)); + } else { + if (!$file->isViewableImage()) { + $mime_type = $file->getMimeType(); + $errors[] = $this->newInvalidError( + pht('File mime type of "%s" is not a valid viewable image.', + $mime_type)); + } } } diff --git a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php index 5b7b6f4417..68710605aa 100644 --- a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php @@ -23,7 +23,7 @@ final class PhabricatorMacroNameTransaction public function getTitleForFeed() { return pht( - '%s renamed %s macro %s to %s.', + '%s renamed %s from %s to %s.', $this->renderAuthor(), $this->renderObject(), $this->renderOldValue(), @@ -37,6 +37,7 @@ final class PhabricatorMacroNameTransaction if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { $errors[] = $this->newRequiredError( pht('Macros must have a name.')); + return $errors; } $max_length = $object->getColumnMaximumByteLength('name'); diff --git a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php index ac0d3896cc..c37933a938 100644 --- a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php +++ b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php @@ -4,6 +4,7 @@ final class PhabricatorSubscriptionsEditEngineExtension extends PhabricatorEditEngineExtension { const EXTENSIONKEY = 'subscriptions.subscribers'; + const FIELDKEY = 'subscriberPHIDs'; const EDITKEY_ADD = 'subscribers.add'; const EDITKEY_SET = 'subscribers.set'; @@ -42,7 +43,7 @@ final class PhabricatorSubscriptionsEditEngineExtension } $subscribers_field = id(new PhabricatorSubscribersEditField()) - ->setKey('subscriberPHIDs') + ->setKey(self::FIELDKEY) ->setLabel(pht('Subscribers')) ->setEditTypeKey('subscribers') ->setAliases(array('subscriber', 'subscribers'))