From c878bf50339c6296cd3d01ac570b05bae705c89f Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 3 May 2017 11:11:14 -0700 Subject: [PATCH] Update Macro for EditEngine Summary: Updates Macro to use EditEngine. Also removes "URL" field for adding a Macro, which I think it's worth pursuing. Test Plan: - Create a Macro - Forget to name it - Try a PDF - Use a Macro - Edit a macro (not working) Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17821 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorMacroApplication.php | 3 +- .../PhabricatorMacroEditController.php | 302 +----------------- .../editor/PhabricatorMacroEditEngine.php | 88 +++++ .../macro/editor/PhabricatorMacroEditor.php | 9 +- .../storage/PhabricatorFileImageMacro.php | 20 +- .../PhabricatorMacroFileTransaction.php | 35 ++ .../PhabricatorMacroNameTransaction.php | 25 ++ 8 files changed, 181 insertions(+), 305 deletions(-) create mode 100644 src/applications/macro/editor/PhabricatorMacroEditEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0c71c8eab1..7716a29ce7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2982,6 +2982,7 @@ phutil_register_library_map(array( 'PhabricatorMacroDisableController' => 'applications/macro/controller/PhabricatorMacroDisableController.php', 'PhabricatorMacroDisabledTransaction' => 'applications/macro/xaction/PhabricatorMacroDisabledTransaction.php', 'PhabricatorMacroEditController' => 'applications/macro/controller/PhabricatorMacroEditController.php', + 'PhabricatorMacroEditEngine' => 'applications/macro/editor/PhabricatorMacroEditEngine.php', 'PhabricatorMacroEditor' => 'applications/macro/editor/PhabricatorMacroEditor.php', 'PhabricatorMacroFileTransaction' => 'applications/macro/xaction/PhabricatorMacroFileTransaction.php', 'PhabricatorMacroListController' => 'applications/macro/controller/PhabricatorMacroListController.php', @@ -8187,7 +8188,8 @@ phutil_register_library_map(array( 'PhabricatorMacroDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorMacroDisableController' => 'PhabricatorMacroController', 'PhabricatorMacroDisabledTransaction' => 'PhabricatorMacroTransactionType', - 'PhabricatorMacroEditController' => 'PhabricatorMacroController', + 'PhabricatorMacroEditController' => 'PhameBlogController', + 'PhabricatorMacroEditEngine' => 'PhabricatorEditEngine', 'PhabricatorMacroEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorMacroFileTransaction' => 'PhabricatorMacroTransactionType', 'PhabricatorMacroListController' => 'PhabricatorMacroController', diff --git a/src/applications/macro/application/PhabricatorMacroApplication.php b/src/applications/macro/application/PhabricatorMacroApplication.php index 3519a3f520..d0e6bbab04 100644 --- a/src/applications/macro/application/PhabricatorMacroApplication.php +++ b/src/applications/macro/application/PhabricatorMacroApplication.php @@ -33,7 +33,8 @@ final class PhabricatorMacroApplication extends PhabricatorApplication { 'create/' => 'PhabricatorMacroEditController', 'view/(?P[1-9]\d*)/' => 'PhabricatorMacroViewController', 'comment/(?P[1-9]\d*)/' => 'PhabricatorMacroCommentController', - 'edit/(?P[1-9]\d*)/' => 'PhabricatorMacroEditController', + $this->getEditRoutePattern('edit/') + => 'PhabricatorMacroEditController', 'audio/(?P[1-9]\d*)/' => 'PhabricatorMacroAudioController', 'disable/(?P[1-9]\d*)/' => 'PhabricatorMacroDisableController', 'meme/' => 'PhabricatorMacroMemeController', diff --git a/src/applications/macro/controller/PhabricatorMacroEditController.php b/src/applications/macro/controller/PhabricatorMacroEditController.php index fc84e057ae..d4b7d5aeec 100644 --- a/src/applications/macro/controller/PhabricatorMacroEditController.php +++ b/src/applications/macro/controller/PhabricatorMacroEditController.php @@ -1,304 +1,10 @@ getViewer(); - $id = $request->getURIData('id'); - - $this->requireApplicationCapability( - PhabricatorMacroManageCapability::CAPABILITY); - - if ($id) { - $macro = id(new PhabricatorMacroQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->needFiles(true) - ->executeOne(); - if (!$macro) { - return new Aphront404Response(); - } - } else { - $macro = new PhabricatorFileImageMacro(); - $macro->setAuthorPHID($viewer->getPHID()); - } - - $errors = array(); - $e_name = true; - $e_file = null; - $file = null; - - if ($request->isFormPost()) { - $original = clone $macro; - - $new_name = null; - if ($request->getBool('name_form') || !$macro->getID()) { - $new_name = $request->getStr('name'); - - $macro->setName($new_name); - - if (!strlen($macro->getName())) { - $errors[] = pht('Macro name is required.'); - $e_name = pht('Required'); - } else if (!preg_match('/^[a-z0-9:_-]{3,}\z/', $macro->getName())) { - $errors[] = pht( - 'Macro must be at least three characters long and contain only '. - 'lowercase letters, digits, hyphens, colons and underscores.'); - $e_name = pht('Invalid'); - } else { - $e_name = null; - } - } - - $uri = $request->getStr('url'); - - $engine = new PhabricatorDestructionEngine(); - - $file = null; - if ($request->getFileExists('file')) { - $file = PhabricatorFile::newFromPHPUpload( - $_FILES['file'], - array( - 'name' => $request->getStr('name'), - 'authorPHID' => $viewer->getPHID(), - 'isExplicitUpload' => true, - 'canCDN' => true, - )); - } else if ($uri) { - try { - // Rate limit outbound fetches to make this mechanism less useful for - // scanning networks and ports. - PhabricatorSystemActionEngine::willTakeAction( - array($viewer->getPHID()), - new PhabricatorFilesOutboundRequestAction(), - 1); - - $file = PhabricatorFile::newFromFileDownload( - $uri, - array( - 'name' => $request->getStr('name'), - 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, - 'isExplicitUpload' => true, - 'canCDN' => true, - )); - - if (!$file->isViewableInBrowser()) { - $mime_type = $file->getMimeType(); - $engine->destroyObject($file); - $file = null; - throw new Exception( - pht( - 'The URI "%s" does not correspond to a valid image file, got '. - 'a file with MIME type "%s". You must specify the URI of a '. - 'valid image file.', - $uri, - $mime_type)); - } else { - $file - ->setAuthorPHID($viewer->getPHID()) - ->save(); - } - } catch (HTTPFutureHTTPResponseStatus $status) { - $errors[] = pht( - 'The URI "%s" could not be loaded, got %s error.', - $uri, - $status->getStatusCode()); - } catch (Exception $ex) { - $errors[] = $ex->getMessage(); - } - } else if ($request->getStr('phid')) { - $file = id(new PhabricatorFileQuery()) - ->setViewer($viewer) - ->withPHIDs(array($request->getStr('phid'))) - ->executeOne(); - } - - if ($file) { - if (!$file->isViewableInBrowser()) { - $errors[] = pht('You must upload an image.'); - $e_file = pht('Invalid'); - } else { - $macro->setFilePHID($file->getPHID()); - $macro->attachFile($file); - $e_file = null; - } - } - - if (!$macro->getID() && !$file) { - $errors[] = pht('You must upload an image to create a macro.'); - $e_file = pht('Required'); - } - - if (!$errors) { - try { - $xactions = array(); - - if ($new_name !== null) { - $xactions[] = id(new PhabricatorMacroTransaction()) - ->setTransactionType( - PhabricatorMacroNameTransaction::TRANSACTIONTYPE) - ->setNewValue($new_name); - } - - if ($file) { - $xactions[] = id(new PhabricatorMacroTransaction()) - ->setTransactionType( - PhabricatorMacroFileTransaction::TRANSACTIONTYPE) - ->setNewValue($file->getPHID()); - } - - $editor = id(new PhabricatorMacroEditor()) - ->setActor($viewer) - ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); - - $xactions = $editor->applyTransactions($original, $xactions); - - $view_uri = $this->getApplicationURI('/view/'.$original->getID().'/'); - return id(new AphrontRedirectResponse())->setURI($view_uri); - } catch (AphrontDuplicateKeyQueryException $ex) { - throw $ex; - $errors[] = pht('Macro name is not unique!'); - $e_name = pht('Duplicate'); - } - } - } - - $current_file = null; - if ($macro->getFilePHID()) { - $current_file = $macro->getFile(); - } - - $form = new AphrontFormView(); - $form->addHiddenInput('name_form', 1); - $form->setUser($request->getUser()); - - $form - ->setEncType('multipart/form-data') - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Name')) - ->setName('name') - ->setValue($macro->getName()) - ->setCaption( - pht('This word or phrase will be replaced with the image.')) - ->setError($e_name)); - - if (!$macro->getID()) { - if ($current_file) { - $current_file_view = id(new PhabricatorFileLinkView()) - ->setViewer($viewer) - ->setFilePHID($current_file->getPHID()) - ->setFileName($current_file->getName()) - ->setFileViewable(true) - ->setFileViewURI($current_file->getBestURI()) - ->render(); - $form->addHiddenInput('phid', $current_file->getPHID()); - $form->appendChild( - id(new AphrontFormMarkupControl()) - ->setLabel(pht('Selected File')) - ->setValue($current_file_view)); - - $other_label = pht('Change File'); - } else { - $other_label = pht('File'); - } - - $form->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('URL')) - ->setName('url') - ->setValue($request->getStr('url')) - ->setError($request->getFileExists('file') ? false : $e_file)); - - $form->appendChild( - id(new AphrontFormFileControl()) - ->setLabel($other_label) - ->setName('file') - ->setError($request->getStr('url') ? false : $e_file)); - } - - - $view_uri = $this->getApplicationURI('/view/'.$macro->getID().'/'); - - if ($macro->getID()) { - $cancel_uri = $view_uri; - } else { - $cancel_uri = $this->getApplicationURI(); - } - - $form - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Save Image Macro')) - ->addCancelButton($cancel_uri)); - - $crumbs = $this->buildApplicationCrumbs(); - - if ($macro->getID()) { - $title = pht('Edit Macro: %s', $macro->getName()); - $crumb = pht('Edit Macro'); - $header_icon = 'fa-pencil'; - - $crumbs->addTextCrumb(pht('Macro: %s', $macro->getName()), $view_uri); - } else { - $title = pht('Create Image Macro'); - $crumb = pht('Create Macro'); - $header_icon = 'fa-plus-square'; - } - - $crumbs->addTextCrumb($crumb, $request->getRequestURI()); - $crumbs->setBorder(true); - - $upload = null; - if ($macro->getID()) { - $upload_form = id(new AphrontFormView()) - ->setEncType('multipart/form-data') - ->setUser($request->getUser()); - - $upload_form->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('URL')) - ->setName('url') - ->setValue($request->getStr('url'))); - - $upload_form - ->appendChild( - id(new AphrontFormFileControl()) - ->setLabel(pht('File')) - ->setName('file')) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Upload File'))); - - $upload = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Upload New File')) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->setForm($upload_form); - } - - $form_box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Macro')) - ->setFormErrors($errors) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->setForm($form); - - $header = id(new PHUIHeaderView()) - ->setHeader($title) - ->setHeaderIcon($header_icon); - - $view = id(new PHUITwoColumnView()) - ->setHeader($header) - ->setFooter(array( - $form_box, - $upload, - )); - - return $this->newPage() - ->setTitle($title) - ->setCrumbs($crumbs) - ->appendChild($view); - + return id(new PhabricatorMacroEditEngine()) + ->setController($this) + ->buildResponse(); } - } diff --git a/src/applications/macro/editor/PhabricatorMacroEditEngine.php b/src/applications/macro/editor/PhabricatorMacroEditEngine.php new file mode 100644 index 0000000000..3f472ff0ce --- /dev/null +++ b/src/applications/macro/editor/PhabricatorMacroEditEngine.php @@ -0,0 +1,88 @@ +getViewer(); + return PhabricatorFileImageMacro::initializeNewFileImageMacro($viewer); + } + + protected function newObjectQuery() { + return new PhabricatorMacroQuery(); + } + + protected function getObjectCreateTitleText($object) { + return pht('Create New Macro'); + } + + protected function getObjectEditTitleText($object) { + return pht('Edit %s', $object->getName()); + } + + protected function getObjectEditShortText($object) { + return $object->getName(); + } + + protected function getObjectCreateShortText() { + return pht('Create Macro'); + } + + protected function getObjectName() { + return pht('Macro'); + } + + protected function getObjectViewURI($object) { + return $object->getViewURI(); + } + + protected function getEditorURI() { + return $this->getApplication()->getApplicationURI('edit/'); + } + + protected function getCreateNewObjectPolicy() { + return $this->getApplication()->getPolicy( + PhabricatorMacroManageCapability::CAPABILITY); + } + + protected function buildCustomEditFields($object) { + + return array( + id(new PhabricatorTextEditField()) + ->setKey('name') + ->setLabel(pht('Name')) + ->setDescription(pht('Macro name.')) + ->setConduitDescription(pht('Rename the macro.')) + ->setConduitTypeDescription(pht('New macro name.')) + ->setTransactionType(PhabricatorMacroNameTransaction::TRANSACTIONTYPE) + ->setValue($object->getName()), + id(new PhabricatorFileEditField()) + ->setKey('filePHID') + ->setLabel(pht('Image File')) + ->setDescription(pht('Image file to import.')) + ->setTransactionType(PhabricatorMacroFileTransaction::TRANSACTIONTYPE) + ->setConduitDescription(pht('File PHID to import.')) + ->setConduitTypeDescription(pht('File PHID.')), + ); + + } + +} diff --git a/src/applications/macro/editor/PhabricatorMacroEditor.php b/src/applications/macro/editor/PhabricatorMacroEditor.php index 797fa702a9..d9067c5367 100644 --- a/src/applications/macro/editor/PhabricatorMacroEditor.php +++ b/src/applications/macro/editor/PhabricatorMacroEditor.php @@ -11,11 +11,12 @@ final class PhabricatorMacroEditor return pht('Macros'); } - public function getTransactionTypes() { - $types = parent::getTransactionTypes(); - $types[] = PhabricatorTransactions::TYPE_COMMENT; + public function getCreateObjectTitle($author, $object) { + return pht('%s created this macro.', $author); + } - return $types; + public function getCreateObjectTitleForFeed($author, $object) { + return pht('%s created %s.', $author, $object); } protected function applyCustomExternalTransaction( diff --git a/src/applications/macro/storage/PhabricatorFileImageMacro.php b/src/applications/macro/storage/PhabricatorFileImageMacro.php index bc23e639d7..0cd6726f5f 100644 --- a/src/applications/macro/storage/PhabricatorFileImageMacro.php +++ b/src/applications/macro/storage/PhabricatorFileImageMacro.php @@ -41,6 +41,12 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO return $this->assertAttached($this->audio); } + public static function initializeNewFileImageMacro(PhabricatorUser $actor) { + $macro = id(new self()) + ->setAuthorPHID($actor->getPHID()); + return $macro; + } + protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -80,6 +86,10 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO return parent::save(); } + public function getViewURI() { + return '/macro/view/'.$this->getID().'/'; + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ @@ -128,11 +138,19 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } public function getPolicy($capability) { - return PhabricatorPolicies::getMostOpenPolicy(); + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return PhabricatorPolicies::getMostOpenPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + $app = PhabricatorApplication::getByClass( + 'PhabricatorMacroApplication'); + return $app->getPolicy(PhabricatorMacroManageCapability::CAPABILITY); + } } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { diff --git a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php index 40e51fe1df..77c9f24dd9 100644 --- a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php @@ -26,6 +26,41 @@ final class PhabricatorMacroFileTransaction $this->renderObject()); } + public function validateTransactions($object, array $xactions) { + $errors = array(); + $viewer = $this->getActor(); + + foreach ($xactions as $xaction) { + $file_phid = $xaction->getNewValue(); + + if ($this->isEmptyTextTransaction($file_phid, $xactions)) { + $errors[] = $this->newRequiredError( + pht('Image macros must have a file.')); + } + + $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(); + $errors[] = $this->newInvalidError( + pht('File mime type of "%s" is not a valid viewable image.', + $mime_type)); + } + } + + } + + return $errors; + } + public function getIcon() { return 'fa-file-image-o'; } diff --git a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php index ebd81530f5..5b7b6f4417 100644 --- a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php +++ b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php @@ -32,6 +32,7 @@ final class PhabricatorMacroNameTransaction public function validateTransactions($object, array $xactions) { $errors = array(); + $viewer = $this->getActor(); if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { $errors[] = $this->newRequiredError( @@ -40,13 +41,37 @@ final class PhabricatorMacroNameTransaction $max_length = $object->getColumnMaximumByteLength('name'); foreach ($xactions as $xaction) { + $old_value = $this->generateOldValue($object); $new_value = $xaction->getNewValue(); + $new_length = strlen($new_value); if ($new_length > $max_length) { $errors[] = $this->newInvalidError( pht('The name can be no longer than %s characters.', new PhutilNumber($max_length))); } + + if (!preg_match('/^[a-z0-9:_-]{3,}\z/', $new_value)) { + $errors[] = $this->newInvalidError( + pht('Macro name "%s" be at least three characters long and contain '. + 'only lowercase letters, digits, hyphens, colons and '. + 'underscores.', + $new_value)); + } + + // Check name is unique when updating / creating + if ($old_value != $new_value) { + $macro = id(new PhabricatorMacroQuery()) + ->setViewer($viewer) + ->withNames(array($new_value)) + ->executeOne(); + + if ($macro) { + $errors[] = $this->newInvalidError( + pht('Macro "%s" already exists.', $new_value)); + } + } + } return $errors;