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

src/applications/phriction/controller/PhrictionEditController.php

@@ -93,120 +93,6 @@ 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())) {
@@ -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'))
+      ->setHeaderText($panel_header)
-      ->setFormErrors($errors)
+      ->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,
       ));
   }
 

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;
   }