Improve Phriction page move dialog

Summary:
Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.

  - The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
  - Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
  - When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
  - Don't prefill the edit note (this feels inconsistent/unusual).
  - On the form, label the input "Path" instead of "URI".
  - Show the old path, to help remind the user what the input should look like.
  - When a user tries to do a no-op move, show a more tailored message.
  - When the user tries to do an overwriting move, explain how they can fix it.
  - When normalizing a slug like `/question/???/mark/`, make it normalize to `/question/_/mark`.

Test Plan:
  - Tried to move a document to itself.
  - Tried to overwrite a document.
  - Did a bad-path move, accepted corrected path.
  - Did a good-path move.
  - Did a path move with a weird component like `/???/`.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5492

Differential Revision: https://secure.phabricator.com/D10838

src/applications/phriction/application/PhabricatorPhrictionApplication.php

@@ -52,7 +52,7 @@ final class PhabricatorPhrictionApplication extends PhabricatorApplication {
         'edit/(?:(?P<id>[1-9]\d*)/)?' => 'PhrictionEditController',
         'delete/(?P<id>[1-9]\d*)/'    => 'PhrictionDeleteController',
         'new/'                        => 'PhrictionNewController',
-        'move/(?:(?P<id>[1-9]\d*)/)?' => 'PhrictionMoveController',
+        'move/(?P<id>[1-9]\d*)/' => 'PhrictionMoveController',
 
         'preview/' => 'PhabricatorMarkupPreviewController',
         'diff/(?P<id>[1-9]\d*)/' => 'PhrictionDiffController',

src/applications/phriction/controller/PhrictionMoveController.php

@@ -2,20 +2,12 @@
 
 final class PhrictionMoveController extends PhrictionController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
 
-  public function willProcessRequest(array $data) {
-    $this->id = idx($data, 'id');
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    if ($this->id) {
     $document = id(new PhrictionDocumentQuery())
-        ->setViewer($user)
+      ->setViewer($viewer)
-        ->withIDs(array($this->id))
+      ->withIDs(array($request->getURIData('id')))
       ->needContent(true)
       ->requireCapabilities(
         array(
@@ -23,58 +15,59 @@ final class PhrictionMoveController extends PhrictionController {
           PhabricatorPolicyCapability::CAN_EDIT,
         ))
       ->executeOne();
-    } else {
-      $slug = PhabricatorSlug::normalize(
-        $request->getStr('slug'));
-      if (!$slug) {
-        return new Aphront404Response();
-      }
-
-      $document = id(new PhrictionDocumentQuery())
-        ->setViewer($user)
-        ->withSlugs(array($slug))
-        ->needContent(true)
-        ->requireCapabilities(
-          array(
-            PhabricatorPolicyCapability::CAN_VIEW,
-            PhabricatorPolicyCapability::CAN_EDIT,
-          ))
-        ->executeOne();
-    }
-
     if (!$document) {
       return new Aphront404Response();
     }
 
-    if (!isset($slug)) {
     $slug = $document->getSlug();
-    }
-
-    $target_slug = PhabricatorSlug::normalize(
-      $request->getStr('new-slug', $slug));
-
-    $submit_uri = $request->getRequestURI()->getPath();
     $cancel_uri = PhrictionDocument::getSlugURI($slug);
 
-    $e_url = true;
+    $v_slug = $slug;
-    $validation_exception = null;
+    $e_slug = null;
-    $content = $document->getContent();
 
+    $v_note = '';
+
+    $validation_exception = null;
     if ($request->isFormPost()) {
+      $v_note = $request->getStr('description');
+      $v_slug = $request->getStr('slug');
+
+      // If what the user typed isn't what we're actually using, warn them
+      // about it.
+      if (strlen($v_slug)) {
+        $normal_slug = PhabricatorSlug::normalize($v_slug);
+        if ($normal_slug !== $v_slug) {
+          return $this->newDialog()
+            ->setTitle(pht('Adjust Path'))
+            ->appendParagraph(
+              pht(
+                'The path you entered (%s) is not a valid wiki document '.
+                'path. Paths may not contain special characters.',
+                phutil_tag('strong', array(), $v_slug)))
+            ->appendParagraph(
+              pht(
+                'Would you like to use the path %s instead?',
+                phutil_tag('strong', array(), $normal_slug)))
+            ->addHiddenInput('slug', $normal_slug)
+            ->addHiddenInput('description', $v_note)
+            ->addCancelButton($cancel_uri)
+            ->addSubmitButton(pht('Accept Path'));
+        }
+      }
 
       $editor = id(new PhrictionTransactionEditor())
-        ->setActor($user)
+        ->setActor($viewer)
         ->setContentSourceFromRequest($request)
         ->setContinueOnNoEffect(true)
-        ->setDescription($request->getStr('description'));
+        ->setDescription($v_note);
 
       $xactions = array();
       $xactions[] = id(new PhrictionTransaction())
         ->setTransactionType(PhrictionTransaction::TYPE_MOVE_TO)
         ->setNewValue($document);
       $target_document = PhrictionDocument::initializeNewDocument(
-        $user,
+        $viewer,
-        $target_slug);
+        $v_slug);
       try {
         $editor->applyTransactions($target_document, $xactions);
         $redir_uri = PhrictionDocument::getSlugURI(
@@ -82,40 +75,40 @@ final class PhrictionMoveController extends PhrictionController {
         return id(new AphrontRedirectResponse())->setURI($redir_uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
         $validation_exception = $ex;
-        $e_url = $ex->getShortMessage(PhrictionTransaction::TYPE_MOVE_TO);
+        $e_slug = $ex->getShortMessage(PhrictionTransaction::TYPE_MOVE_TO);
       }
     }
 
-    $form = id(new PHUIFormLayoutView())
+
-      ->setUser($user)
+    $form = id(new AphrontFormView())
+      ->setUser($viewer)
       ->appendChild(
         id(new AphrontFormStaticControl())
           ->setLabel(pht('Title'))
-        ->setValue($content->getTitle()))
+          ->setValue($document->getContent()->getTitle()))
       ->appendChild(
         id(new AphrontFormTextControl())
-        ->setLabel(pht('New URI'))
+          ->setLabel(pht('Current Path'))
-        ->setValue($target_slug)
+          ->setDisabled(true)
-        ->setError($e_url)
+          ->setValue($slug))
-        ->setName('new-slug')
+      ->appendChild(
-        ->setCaption(pht('The new location of the document.')))
+        id(new AphrontFormTextControl())
+          ->setLabel(pht('New Path'))
+          ->setValue($v_slug)
+          ->setError($e_slug)
+          ->setName('slug'))
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setLabel(pht('Edit Notes'))
-        ->setValue(pht('Moving document to a new location.'))
+          ->setValue($v_note)
-        ->setError(null)
           ->setName('description'));
 
-    $dialog = id(new AphrontDialogView())
+    return $this->newDialog()
-      ->setUser($user)
-      ->setValidationException($validation_exception)
       ->setTitle(pht('Move Document'))
-      ->appendChild($form)
+      ->setValidationException($validation_exception)
-      ->setSubmitURI($submit_uri)
+      ->appendForm($form)
       ->addSubmitButton(pht('Move Document'))
       ->addCancelButton($cancel_uri);
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/applications/phriction/editor/PhrictionTransactionEditor.php

@@ -537,13 +537,24 @@ final class PhrictionTransactionEditor
             ->needContent(true)
             ->executeOne();
 
-          // Considering to overwrite existing docs? Nuke this!
+          // Prevent overwrites and no-op moves.
           $exists = PhrictionDocumentStatus::STATUS_EXISTS;
-          if ($target_document && $target_document->getStatus() == $exists) {
+          if ($target_document) {
+            if ($target_document->getSlug() == $source_document->getSlug()) {
+              $message = pht(
+                'You can not move a document to its existing location. '.
+                'Choose a different location to move the document to.');
+            } else if ($target_document->getStatus() == $exists) {
+              $message = pht(
+                'You can not move this document there, because it would '.
+                'overwrite an existing document which is already at that '.
+                'location. Move or delete the existing document first.');
+            }
+
             $error = new PhabricatorApplicationTransactionValidationError(
               $type,
-              pht('Can not move document.'),
+              pht('Invalid'),
-              pht('Can not overwrite existing target document.'),
+              $message,
               $xaction);
             $errors[] = $error;
           }

src/infrastructure/util/PhabricatorSlug.php

@@ -8,7 +8,17 @@ final class PhabricatorSlug {
     $slug = phutil_utf8_strtolower($slug);
     $slug = preg_replace("@[\\x00-\\x19#%&+=\\\\?<> ]+@", '_', $slug);
     $slug = preg_replace('@_+@', '_', $slug);
-    $slug = trim($slug, '_');
+
+    // Remove leading and trailing underscores from each component, if the
+    // component has not been reduced to a single underscore. For example, "a?"
+    // converts to "a", but "??" converts to "_".
+    $parts = explode('/', $slug);
+    foreach ($parts as $key => $part) {
+      if ($part != '_') {
+        $parts[$key] = trim($part, '_');
+      }
+    }
+    $slug = implode('/', $parts);
 
     // Specifically rewrite these slugs. It's OK to have a slug like "a..b",
     // but not a slug which is only "..".

src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php

@@ -7,7 +7,7 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase {
       ''                  => '/',
       '/'                 => '/',
       '//'                => '/',
-      '&&&'               => '/',
+      '&&&'               => '_/',
       '/derp/'            => 'derp/',
       'derp'              => 'derp/',
       'derp//derp'        => 'derp/derp/',
@@ -27,6 +27,13 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase {
       '../a'              => 'dotdot/a/',
       'a/..'              => 'a/dotdot/',
       'a/../'             => 'a/dotdot/',
+      'a?'                => 'a/',
+      '??'                => '_/',
+      'a/?'               => 'a/_/',
+      '??/a/??'           => '_/a/_/',
+      'a/??/c'            => 'a/_/c/',
+      'a/?b/c'            => 'a/b/c/',
+      'a/b?/c'            => 'a/b/c/',
     );
 
     foreach ($slugs as $slug => $normal) {