Provide a smoother "update diff" web workflow

Summary: Fixes T1102. If you don't use `arc`, the web workflow requires some extra needless steps when updating diffs. Provide a more streamlined "Update Diff" workflow. Test Plan: {F347750} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T1102 Differential Revision: https://secure.phabricator.com/D12131
2015-03-23 10:44:33 -07:00
parent 1c32c9b965
commit d0b3f199bb
6 changed files with 135 additions and 46 deletions
--- a/src/applications/differential/application/PhabricatorDifferentialApplication.php
+++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php
@@ -63,12 +63,16 @@ EOTEXT
          'create/' => 'DifferentialDiffCreateController',
        ),
        'changeset/' => 'DifferentialChangesetViewController',
-        'revision/edit/(?:(?P<id>[1-9]\d*)/)?'
+        'revision/' => array(
+          'edit/(?:(?P<id>[1-9]\d*)/)?'
            => 'DifferentialRevisionEditController',
-        'revision/land/(?:(?P<id>[1-9]\d*))/(?P<strategy>[^/]+)/'
+          'land/(?:(?P<id>[1-9]\d*))/(?P<strategy>[^/]+)/'
            => 'DifferentialRevisionLandController',
-        'revision/closedetails/(?P<phid>[^/]+)/'
+          'closedetails/(?P<phid>[^/]+)/'
            => 'DifferentialRevisionCloseDetailsController',
+          'update/(?P<revisionID>[1-9]\d*)/'
+            => 'DifferentialDiffCreateController',
+        ),
        'comment/' => array(
          'preview/(?P<id>[1-9]\d*)/' => 'DifferentialCommentPreviewController',
          'save/(?P<id>[1-9]\d*)/' => 'DifferentialCommentSaveController',
--- a/src/applications/differential/controller/DifferentialController.php
+++ b/src/applications/differential/controller/DifferentialController.php
@@ -2,18 +2,6 @@

 abstract class DifferentialController extends PhabricatorController {

-  protected function buildApplicationCrumbs() {
-    $crumbs = parent::buildApplicationCrumbs();
-
-    $crumbs->addAction(
-      id(new PHUIListItemView())
-        ->setHref($this->getApplicationURI('/diff/create/'))
-        ->setName(pht('Create Diff'))
-        ->setIcon('fa-plus-square'));
-
-    return $crumbs;
-  }
-
  public function buildSideNavView($for_app = false) {
    $viewer = $this->getRequest()->getUser();

--- a/src/applications/differential/controller/DifferentialDiffCreateController.php
+++ b/src/applications/differential/controller/DifferentialDiffCreateController.php
@@ -2,10 +2,27 @@

 final class DifferentialDiffCreateController extends DifferentialController {

-  public function processRequest() {
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();

-    $request = $this->getRequest();
-    $viewer = $request->getUser();
+    // If we're on the "Update Diff" workflow, load the revision we're going
+    // to update.
+    $revision = null;
+    $revision_id = $request->getURIData('revisionID');
+    if ($revision_id) {
+      $revision = id(new DifferentialRevisionQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($revision_id))
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
+        ->executeOne();
+      if (!$revision) {
+        return new Aphront404Response();
+      }
+    }

    $diff = null;
    // This object is just for policy stuff
@@ -32,8 +49,8 @@ final class DifferentialDiffCreateController extends DifferentialController {

      if (!strlen($diff)) {
        $errors[] = pht(
-          'You can not create an empty diff. Copy/paste a diff, or upload a '.
-          'diff file.');
+          'You can not create an empty diff. Paste a diff or upload a '.
+          'file containing a diff.');
        $e_diff = pht('Required');
        $e_file = pht('Required');
      }
@@ -48,8 +65,16 @@ final class DifferentialDiffCreateController extends DifferentialController {
              'viewPolicy' => $request->getStr('viewPolicy'),));
          $call->setUser($viewer);
          $result = $call->execute();
-          $path = id(new PhutilURI($result['uri']))->getPath();
-          return id(new AphrontRedirectResponse())->setURI($path);
+
+          $diff_id = $result['id'];
+
+          $uri = $this->getApplicationURI("diff/{$diff_id}/");
+          $uri = new PhutilURI($uri);
+          if ($revision) {
+            $uri->setQueryParam('revisionID', $revision->getID());
+          }
+
+          return id(new AphrontRedirectResponse())->setURI($uri);
        } catch (PhabricatorApplicationTransactionValidationException $ex) {
          $validation_exception = $ex;
        }
@@ -64,7 +89,7 @@ final class DifferentialDiffCreateController extends DifferentialController {
        'href' => $arcanist_href,
        'target' => '_blank',
      ),
-      'Arcanist');
+      'Learn More');

    $cancel_uri = $this->getApplicationURI();

@@ -77,19 +102,53 @@ final class DifferentialDiffCreateController extends DifferentialController {
      ->setObject($diff_object)
      ->execute();

-    $form
-      ->setAction('/differential/diff/create/')
-      ->setEncType('multipart/form-data')
-      ->setUser($viewer)
-      ->appendInstructions(
+    $info_view = null;
+    if (!$request->isFormPost()) {
+      $info_view = id(new PHUIInfoView())
+        ->setSeverity(PHUIInfoView::SEVERITY_NOTICE)
+        ->setErrors(
+          array(
+            array(
              pht(
-          'The best way to create a Differential diff is by using %s, but you '.
-          'can also just paste a diff (for example, from %s, %s or %s) into '.
-          'this box, or upload a diff file.',
+                'The best way to create a diff is to use the Arcanist '.
+                'command-line tool.'),
+              ' ',
              $arcanist_link,
+            ),
+            pht(
+              'You can also paste a diff below, or upload a file '.
+              'containing a diff (for example, from %s, %s or %s).',
              phutil_tag('tt', array(), 'svn diff'),
              phutil_tag('tt', array(), 'git diff'),
-          phutil_tag('tt', array(), 'hg diff --git')))
+              phutil_tag('tt', array(), 'hg diff --git')),
+          ));
+    }
+
+    if ($revision) {
+      $title = pht('Update Diff');
+      $header = pht('Update Diff');
+      $button = pht('Continue');
+    } else {
+      $title = pht('Create Diff');
+      $header = pht('Create New Diff');
+      $button = pht('Create Diff');
+    }
+
+    $form
+      ->setEncType('multipart/form-data')
+      ->setUser($viewer);
+
+    if ($revision) {
+      $revision_handles = $this->loadViewerHandles(array($revision->getPHID()));
+      $revision_handle = head($revision_handles);
+
+      $form->appendChild(
+        id(new AphrontFormMarkupControl())
+          ->setLabel(pht('Updating Revision'))
+          ->setValue($revision_handle->renderLink()));
+    }
+
+    $form
      ->appendChild(
        id(new AphrontFormTextAreaControl())
          ->setLabel(pht('Raw Diff'))
@@ -119,16 +178,25 @@ final class DifferentialDiffCreateController extends DifferentialController {
      ->appendChild(
        id(new AphrontFormSubmitControl())
          ->addCancelButton($cancel_uri)
-          ->setValue(pht('Create Diff')));
+          ->setValue($button));

    $form_box = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Create New Diff'))
+      ->setHeaderText($header)
      ->setValidationException($validation_exception)
      ->setForm($form)
      ->setFormErrors($errors);

+    if ($info_view) {
+      $form_box->setInfoView($info_view);
+    }
+
    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(pht('Create Diff'));
+    if ($revision) {
+      $crumbs->addTextCrumb(
+        $revision->getMonogram(),
+        '/'.$revision->getMonogram());
+    }
+    $crumbs->addTextCrumb($title);

    return $this->buildApplicationPage(
      array(
@@ -136,7 +204,7 @@ final class DifferentialDiffCreateController extends DifferentialController {
        $form_box,
      ),
      array(
-        'title' => pht('Create Diff'),
+        'title' => $title,
      ));
  }

--- a/src/applications/differential/controller/DifferentialDiffViewController.php
+++ b/src/applications/differential/controller/DifferentialDiffViewController.php
@@ -29,8 +29,6 @@ final class DifferentialDiffViewController extends DifferentialController {
        ->setURI('/D'.$diff->getRevisionID().'?id='.$diff->getID());
    }

-    $error_view = id(new PHUIInfoView())
-        ->setSeverity(PHUIInfoView::SEVERITY_NOTICE);
    // TODO: implement optgroup support in AphrontFormSelectControl?
    $select = array();
    $select[] = hsprintf('<optgroup label="%s">', pht('Create New Revision'));
@@ -44,17 +42,31 @@ final class DifferentialDiffViewController extends DifferentialController {
      ->setViewer($viewer)
      ->withAuthors(array($viewer->getPHID()))
      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
      ->execute();

+    $selected_id = $request->getInt('revisionID');
+
    if ($revisions) {
      $select[] = hsprintf(
        '<optgroup label="%s">',
        pht('Update Existing Revision'));
      foreach ($revisions as $revision) {
+        if ($selected_id == $revision->getID()) {
+          $selected = 'selected';
+        } else {
+          $selected = null;
+        }
+
        $select[] = phutil_tag(
          'option',
          array(
            'value' => $revision->getID(),
+            'selected' => $selected,
          ),
          id(new PhutilUTF8StringTruncator())
          ->setMaximumGlyphs(128)
@@ -89,8 +101,6 @@ final class DifferentialDiffViewController extends DifferentialController {
        id(new AphrontFormSubmitControl())
        ->setValue(pht('Continue')));

-    $error_view->appendChild($form);
-
    $props = id(new DifferentialDiffProperty())->loadAllWhere(
    'diffID = %d',
      $diff->getID());
@@ -129,7 +139,7 @@ final class DifferentialDiffViewController extends DifferentialController {
    $prop_box = id(new PHUIObjectBoxView())
      ->setHeader($property_head)
      ->addPropertyList($property_view)
-      ->setInfoView($error_view);
+      ->appendChild($form);

    return $this->buildApplicationPage(
      array(
--- a/src/applications/differential/controller/DifferentialRevisionListController.php
+++ b/src/applications/differential/controller/DifferentialRevisionListController.php
@@ -21,4 +21,16 @@ final class DifferentialRevisionListController extends DifferentialController {
    return $this->delegateToController($controller);
  }

+  protected function buildApplicationCrumbs() {
+    $crumbs = parent::buildApplicationCrumbs();
+
+    $crumbs->addAction(
+      id(new PHUIListItemView())
+        ->setHref($this->getApplicationURI('/diff/create/'))
+        ->setName(pht('Create Diff'))
+        ->setIcon('fa-plus-square'));
+
+    return $crumbs;
+  }
+
 }
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -525,6 +525,13 @@ final class DifferentialRevisionViewController extends DifferentialController {
      ->setDisabled(!$can_edit)
      ->setWorkflow(!$can_edit);

+    $actions[] = id(new PhabricatorActionView())
+      ->setIcon('fa-upload')
+      ->setHref("/differential/revision/update/{$revision_id}/")
+      ->setName(pht('Update Diff'))
+      ->setDisabled(!$can_edit)
+      ->setWorkflow(!$can_edit);
+
    $this->requireResource('phabricator-object-selector-css');
    $this->requireResource('javelin-behavior-phabricator-object-selector');