Differential - finesse Differential diff view controller

Summary:
Fixes T7229. Some usability issues around this controller - basically you can't leave comments with it and its not particular useful compared to the revision page.

Ergo, if there is a revision associated with a given diff, just re-direct back to the revision page with the proper diff loaded.

Test Plan: Tried to view a diff on the standalone controller attached to a revision and instead was re-directed to the revision view page with the proper diff loaded.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7229

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

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -24,82 +24,75 @@ final class DifferentialDiffViewController extends DifferentialController {
       return new Aphront404Response();
     }
 
-    $error_view = id(new PHUIErrorView())
-        ->setSeverity(PHUIErrorView::SEVERITY_NOTICE);
     if ($diff->getRevisionID()) {
-      $error_view->appendChild(
+      return id(new AphrontRedirectResponse())
-          pht(
+        ->setURI('/D'.$diff->getRevisionID().'?id='.$diff->getID());
-            'This diff belongs to revision %s.',
-            phutil_tag(
-              'a',
-              array(
-                'href' => '/D'.$diff->getRevisionID(),
-              ),
-              'D'.$diff->getRevisionID())));
-    } else {
-      // TODO: implement optgroup support in AphrontFormSelectControl?
-      $select = array();
-      $select[] = hsprintf('<optgroup label="%s">', pht('Create New Revision'));
-      $select[] = phutil_tag(
-        'option',
-        array('value' => ''),
-        pht('Create a new Revision...'));
-      $select[] = hsprintf('</optgroup>');
-
-      $revisions = id(new DifferentialRevisionQuery())
-        ->setViewer($viewer)
-        ->withAuthors(array($viewer->getPHID()))
-        ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
-        ->execute();
-
-      if ($revisions) {
-        $select[] = hsprintf(
-          '<optgroup label="%s">',
-          pht('Update Existing Revision'));
-        foreach ($revisions as $revision) {
-          $select[] = phutil_tag(
-            'option',
-            array(
-              'value' => $revision->getID(),
-            ),
-            id(new PhutilUTF8StringTruncator())
-            ->setMaximumGlyphs(128)
-            ->truncateString(
-              'D'.$revision->getID().' '.$revision->getTitle()));
-        }
-        $select[] = hsprintf('</optgroup>');
-      }
-
-      $select = phutil_tag(
-        'select',
-        array('name' => 'revisionID'),
-        $select);
-
-      $form = id(new AphrontFormView())
-        ->setUser($request->getUser())
-        ->setAction('/differential/revision/edit/')
-        ->addHiddenInput('diffID', $diff->getID())
-        ->addHiddenInput('viaDiffView', 1)
-        ->addHiddenInput(
-          id(new DifferentialRepositoryField())->getFieldKey(),
-          $diff->getRepositoryPHID())
-        ->appendRemarkupInstructions(
-          pht(
-            'Review the diff for correctness. When you are satisfied, either '.
-            '**create a new revision** or **update an existing revision**.'))
-        ->appendChild(
-          id(new AphrontFormMarkupControl())
-          ->setLabel(pht('Attach To'))
-          ->setValue($select))
-        ->appendChild(
-          id(new AphrontFormSubmitControl())
-          ->setValue(pht('Continue')));
-
-        $error_view->appendChild($form);
     }
 
+    $error_view = id(new PHUIErrorView())
+        ->setSeverity(PHUIErrorView::SEVERITY_NOTICE);
+    // TODO: implement optgroup support in AphrontFormSelectControl?
+    $select = array();
+    $select[] = hsprintf('<optgroup label="%s">', pht('Create New Revision'));
+    $select[] = phutil_tag(
+      'option',
+      array('value' => ''),
+      pht('Create a new Revision...'));
+    $select[] = hsprintf('</optgroup>');
+
+    $revisions = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withAuthors(array($viewer->getPHID()))
+      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->execute();
+
+    if ($revisions) {
+      $select[] = hsprintf(
+        '<optgroup label="%s">',
+        pht('Update Existing Revision'));
+      foreach ($revisions as $revision) {
+        $select[] = phutil_tag(
+          'option',
+          array(
+            'value' => $revision->getID(),
+          ),
+          id(new PhutilUTF8StringTruncator())
+          ->setMaximumGlyphs(128)
+          ->truncateString(
+            'D'.$revision->getID().' '.$revision->getTitle()));
+      }
+      $select[] = hsprintf('</optgroup>');
+    }
+
+    $select = phutil_tag(
+      'select',
+      array('name' => 'revisionID'),
+      $select);
+
+    $form = id(new AphrontFormView())
+      ->setUser($request->getUser())
+      ->setAction('/differential/revision/edit/')
+      ->addHiddenInput('diffID', $diff->getID())
+      ->addHiddenInput('viaDiffView', 1)
+      ->addHiddenInput(
+        id(new DifferentialRepositoryField())->getFieldKey(),
+        $diff->getRepositoryPHID())
+      ->appendRemarkupInstructions(
+        pht(
+          'Review the diff for correctness. When you are satisfied, either '.
+          '**create a new revision** or **update an existing revision**.'))
+      ->appendChild(
+        id(new AphrontFormMarkupControl())
+        ->setLabel(pht('Attach To'))
+        ->setValue($select))
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+        ->setValue(pht('Continue')));
+
+    $error_view->appendChild($form);
+
     $props = id(new DifferentialDiffProperty())->loadAllWhere(
-      'diffID = %d',
+    'diffID = %d',
       $diff->getID());
     $props = mpull($props, 'getData', 'getName');
 

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php

@@ -62,6 +62,7 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
     }
 
     $max_id = $diff->getID();
+    $revision_id = $diff->getRevisionID();
 
     $idx = 0;
     $rows = array();
@@ -169,12 +170,21 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
       }
       $last_base = $base;
 
-      $id_link = phutil_tag(
+      if ($revision_id) {
-        'a',
+        $id_link = phutil_tag(
-        array(
+          'a',
-          'href' => '/differential/diff/'.$id.'/',
+          array(
-        ),
+            'href' => '/D'.$revision_id.'?id='.$id,
-        $id);
+          ),
+          $id);
+      } else {
+        $id_link = phutil_tag(
+          'a',
+          array(
+            'href' => '/differential/diff/'.$id.'/',
+          ),
+          $id);
+      }
 
       $rows[] = array(
         $name,

src/applications/differential/view/DifferentialTransactionView.php

@@ -143,15 +143,13 @@ final class DifferentialTransactionView
 
           $is_visible = ($changeset_diff_id == $visible_diff_id);
           if (!$is_visible) {
-            $item['where'] = pht('(On Diff #%d)', $changeset_diff_id);
-
             $revision_id = $this->getRevision()->getID();
             $comment_id = $comment->getID();
-
             $item['href'] =
               '/D'.$revision_id.
               '?id='.$changeset_diff_id.
               '#inline-'.$comment_id;
+            $item['where'] = pht('(On Diff #%d)', $changeset_diff_id);
           }
 
           $items[] = $item;