From 37bc0474feea91657f3579854a022a19dd272a33 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 6 Nov 2014 14:02:52 -0800 Subject: [PATCH] Phriction - consolidate edit business logic into Editor Summary: Ref T4029. Some business logic lives outside the editor. This revision moves that logic from the edit controller into the editor proper. This makes re-using that business logic across other endpoints - say like a conduit end point - possible. This is also part of the general modernization quest for phriction I am on. This diff also restores the functionality where you can delete a document by wiping out the content and saving. Test Plan: tried to make a document with no title or content and saw errors. opened a document for edit with user 1, then made edits with user 2, then saw an error when i made the edit with user 1. clicking "overwrite changes" then worked. deleted a document by wiping out the body and clicking save. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T4029 Differential Revision: https://secure.phabricator.com/D10795 --- .../controller/PhrictionEditController.php | 208 ++++++++---------- .../editor/PhrictionTransactionEditor.php | 138 +++++++++++- 2 files changed, 223 insertions(+), 123 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index 850e5f47fc..e85b7bc788 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -93,123 +93,9 @@ final class PhrictionEditController $draft_key); } - require_celerity_resource('phriction-document-css'); - - $e_title = true; - $notes = null; - $errors = array(); - - if ($request->isFormPost()) { - - $overwrite = $request->getBool('overwrite'); - if (!$overwrite) { - $edit_version = $request->getStr('contentVersion'); - if ($edit_version != $current_version) { - $dialog = $this->newDialog() - ->setTitle(pht('Edit Conflict!')) - ->appendParagraph( - pht( - 'Another user made changes to this document after you began '. - 'editing it. Do you want to overwrite their changes?')) - ->appendParagraph( - pht( - 'If you choose to overwrite their changes, you should review '. - 'the document edit history to see what you overwrote, and '. - 'then make another edit to merge the changes if necessary.')) - ->addSubmitButton(pht('Overwrite Changes')) - ->addCancelButton($request->getRequestURI()); - - $dialog->addHiddenInput('overwrite', 'true'); - foreach ($request->getPassthroughRequestData() as $key => $value) { - $dialog->addHiddenInput($key, $value); - } - - return $dialog; - } - } - - - $title = $request->getStr('title'); - $notes = $request->getStr('description'); - - if (!strlen($title)) { - $e_title = pht('Required'); - $errors[] = pht('Document title is required.'); - } else { - $e_title = null; - } - - if ($document->getID()) { - if ($content->getTitle() == $title && - $content->getContent() == $request->getStr('content')) { - - $dialog = new AphrontDialogView(); - $dialog->setUser($user); - $dialog->setTitle(pht('No Edits')); - $dialog->appendChild(phutil_tag('p', array(), pht( - 'You did not make any changes to the document.'))); - $dialog->addCancelButton($request->getRequestURI()); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } - } else if (!strlen($request->getStr('content'))) { - - // We trigger this only for new pages. For existing pages, deleting - // all the content counts as deleting the page. - - $dialog = new AphrontDialogView(); - $dialog->setUser($user); - $dialog->setTitle(pht('Empty Page')); - $dialog->appendChild(phutil_tag('p', array(), pht( - 'You can not create an empty document.'))); - $dialog->addCancelButton($request->getRequestURI()); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - if (!count($errors)) { - - $xactions = array(); - $xactions[] = id(new PhrictionTransaction()) - ->setTransactionType(PhrictionTransaction::TYPE_TITLE) - ->setNewValue($title); - $xactions[] = id(new PhrictionTransaction()) - ->setTransactionType(PhrictionTransaction::TYPE_CONTENT) - ->setNewValue($request->getStr('content')); - - $editor = id(new PhrictionTransactionEditor()) - ->setActor($user) - ->setContentSourceFromRequest($request) - ->setContinueOnNoEffect(true) - ->setDescription($notes) - ->applyTransactions($document, $xactions); - - if ($draft) { - $draft->delete(); - } - - $uri = PhrictionDocument::getSlugURI($document->getSlug()); - return id(new AphrontRedirectResponse())->setURI($uri); - } - } - - if ($document->getID()) { - $panel_header = pht('Edit Phriction Document'); - $submit_button = pht('Save Changes'); - } else { - $panel_header = pht('Create New Phriction Document'); - $submit_button = pht('Create Document'); - } - - $uri = $document->getSlug(); - $uri = PhrictionDocument::getSlugURI($uri); - $uri = PhabricatorEnv::getProductionURI($uri); - - $cancel_uri = PhrictionDocument::getSlugURI($document->getSlug()); - if ($draft && - strlen($draft->getDraft()) && - ($draft->getDraft() != $content->getContent())) { + strlen($draft->getDraft()) && + ($draft->getDraft() != $content->getContent())) { $content_text = $draft->getDraft(); $discard = phutil_tag( @@ -230,17 +116,94 @@ final class PhrictionEditController $draft_note = null; } + require_celerity_resource('phriction-document-css'); + + $e_title = true; + $e_content = true; + $validation_exception = null; + $notes = null; + $title = $content->getTitle(); + $overwrite = false; + + if ($request->isFormPost()) { + + $title = $request->getStr('title'); + $content_text = $request->getStr('content'); + $notes = $request->getStr('description'); + $current_version = $request->getInt('contentVersion'); + + $xactions = array(); + $xactions[] = id(new PhrictionTransaction()) + ->setTransactionType(PhrictionTransaction::TYPE_TITLE) + ->setNewValue($title); + $xactions[] = id(new PhrictionTransaction()) + ->setTransactionType(PhrictionTransaction::TYPE_CONTENT) + ->setNewValue($content_text); + + $editor = id(new PhrictionTransactionEditor()) + ->setActor($user) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setDescription($notes) + ->setProcessContentVersionError(!$request->getBool('overwrite')) + ->setContentVersion($current_version); + + try { + $editor->applyTransactions($document, $xactions); + + if ($draft) { + $draft->delete(); + } + + $uri = PhrictionDocument::getSlugURI($document->getSlug()); + return id(new AphrontRedirectResponse())->setURI($uri); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + $e_title = $ex->getShortMessage( + PhrictionTransaction::TYPE_TITLE); + $e_content = $ex->getShortMessage( + PhrictionTransaction::TYPE_CONTENT); + + // if we're not supposed to process the content version error, then + // overwrite that content...! + if (!$editor->getProcessContentVersionError()) { + $overwrite = true; + } + + // TODO - remember to set policy to what the user tried to set it to + } + } + + if ($document->getID()) { + $panel_header = pht('Edit Phriction Document'); + $page_title = pht('Edit Document'); + if ($overwrite) { + $submit_button = pht('Overwrite Changes'); + } else { + $submit_button = pht('Save Changes'); + } + } else { + $panel_header = pht('Create New Phriction Document'); + $submit_button = pht('Create Document'); + $page_title = pht('Create Document'); + } + + $uri = $document->getSlug(); + $uri = PhrictionDocument::getSlugURI($uri); + $uri = PhabricatorEnv::getProductionURI($uri); + + $cancel_uri = PhrictionDocument::getSlugURI($document->getSlug()); + $form = id(new AphrontFormView()) ->setUser($user) - ->setWorkflow(true) - ->setAction($request->getRequestURI()->getPath()) ->addHiddenInput('slug', $document->getSlug()) ->addHiddenInput('nodraft', $request->getBool('nodraft')) ->addHiddenInput('contentVersion', $current_version) + ->addHiddenInput('overwrite', $overwrite) ->appendChild( id(new AphrontFormTextControl()) ->setLabel(pht('Title')) - ->setValue($content->getTitle()) + ->setValue($title) ->setError($e_title) ->setName('title')) ->appendChild( @@ -251,6 +214,7 @@ final class PhrictionEditController id(new PhabricatorRemarkupControl()) ->setLabel(pht('Content')) ->setValue($content_text) + ->setError($e_content) ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) ->setName('content') ->setID('document-textarea') @@ -267,8 +231,8 @@ final class PhrictionEditController ->setValue($submit_button)); $form_box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Edit Document')) - ->setFormErrors($errors) + ->setHeaderText($panel_header) + ->setValidationException($validation_exception) ->setForm($form); $preview = id(new PHUIRemarkupPreviewPanel()) @@ -295,7 +259,7 @@ final class PhrictionEditController $preview, ), array( - 'title' => pht('Edit Document'), + 'title' => $page_title, )); } diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index d97b9e0844..c8bd4096de 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -8,6 +8,8 @@ final class PhrictionTransactionEditor private $newContent; private $moveAwayDocument; private $skipAncestorCheck; + private $contentVersion; + private $processContentVersionError = true; public function setDescription($description) { $this->description = $description; @@ -45,6 +47,24 @@ final class PhrictionTransactionEditor return $this->skipAncestorCheck; } + public function setContentVersion($version) { + $this->contentVersion = $version; + return $this; + } + + public function getContentVersion() { + return $this->contentVersion; + } + + public function setProcessContentVersionError($process) { + $this->processContentVersionError = $process; + return $this; + } + + public function getProcessContentVersionError() { + return $this->processContentVersionError; + } + public function getEditorApplicationClass() { return 'PhabricatorPhrictionApplication'; } @@ -162,6 +182,30 @@ final class PhrictionTransactionEditor } } + protected function expandTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $xactions = parent::expandTransaction($object, $xaction); + switch ($xaction->getTransactionType()) { + case PhrictionTransaction::TYPE_CONTENT: + if ($this->getIsNewObject()) { + break; + } + $content = $xaction->getNewValue(); + if ($content === '') { + $xactions[] = id(new PhrictionTransaction()) + ->setTransactionType(PhrictionTransaction::TYPE_DELETE) + ->setNewValue(true); + } + break; + default: + break; + } + + return $xactions; + } + protected function applyCustomExternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -251,7 +295,8 @@ final class PhrictionTransactionEditor ->setMetadataValue('stub:create:phid', $object->getPHID()); $stub_xactions[] = id(new PhrictionTransaction()) ->setTransactionType(PhrictionTransaction::TYPE_CONTENT) - ->setNewValue(''); + ->setNewValue('') + ->setMetadataValue('stub:create:phid', $object->getPHID()); $sub_editor = id(new PhrictionTransactionEditor()) ->setActor($this->getActor()) ->setContentSource($this->getContentSource()) @@ -366,6 +411,97 @@ final class PhrictionTransactionEditor return $phids; } + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + foreach ($xactions as $xaction) { + switch ($type) { + case PhrictionTransaction::TYPE_TITLE: + $title = $object->getContent()->getTitle(); + $missing = $this->validateIsEmptyTextField( + $title, + $xactions); + + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('Document title is required.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + } else if ($this->getProcessContentVersionError()) { + $error = $this->validateContentVersion($object, $type, $xaction); + if ($error) { + $this->setProcessContentVersionError(false); + $errors[] = $error; + } + } + break; + + case PhrictionTransaction::TYPE_CONTENT: + if ($xaction->getMetadataValue('stub:create:phid')) { + continue; + } + + $missing = false; + if ($this->getIsNewObject()) { + $content = $object->getContent()->getContent(); + $missing = $this->validateIsEmptyTextField( + $content, + $xactions); + } + + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('Document content is required.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + } else if ($this->getProcessContentVersionError()) { + $error = $this->validateContentVersion($object, $type, $xaction); + if ($error) { + $this->setProcessContentVersionError(false); + $errors[] = $error; + } + } + break; + } + } + + return $errors; + } + + private function validateContentVersion( + PhabricatorLiskDAO $object, + $type, + PhabricatorApplicationTransaction $xaction) { + + $error = null; + if ($this->getContentVersion() && + ($object->getContent()->getVersion() != $this->getContentVersion())) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Edit Conflict'), + pht( + 'Another user made changes to this document after you began '. + 'editing it. Do you want to overwrite their changes? '. + '(If you choose to overwrite their changes, you should review '. + 'the document edit history to see what you overwrote, and '. + 'then make another edit to merge the changes if necessary.)'), + $xaction); + } + return $error; + } + protected function supportsSearch() { return true; }