Use "CommentPro" controller instead of "Comment" controller

Summary: Ref T2222. This will probabaly have a few rough edges too, but seems to work well.

Test Plan:
  - Made a bunch of comments while building this.
  - Made some new comments.
  - Verified that the Asana/JIRA integration is only a little bit janky, not completely broken.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/__phutil_library_map__.php

@@ -360,7 +360,6 @@ phutil_register_library_map(array(
     'DifferentialCommentPreviewController' => 'applications/differential/controller/DifferentialCommentPreviewController.php',
     'DifferentialCommentQuery' => 'applications/differential/query/DifferentialCommentQuery.php',
     'DifferentialCommentSaveController' => 'applications/differential/controller/DifferentialCommentSaveController.php',
-    'DifferentialCommentSaveControllerPro' => 'applications/differential/controller/DifferentialCommentSaveControllerPro.php',
     'DifferentialCommitsField' => 'applications/differential/customfield/DifferentialCommitsField.php',
     'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/DifferentialCommitsFieldSpecification.php',
     'DifferentialConflictsFieldSpecification' => 'applications/differential/field/specification/DifferentialConflictsFieldSpecification.php',
@@ -2925,7 +2924,6 @@ phutil_register_library_map(array(
     'DifferentialCommentPreviewController' => 'DifferentialController',
     'DifferentialCommentQuery' => 'PhabricatorOffsetPagedQuery',
     'DifferentialCommentSaveController' => 'DifferentialController',
-    'DifferentialCommentSaveControllerPro' => 'DifferentialController',
     'DifferentialCommitsField' => 'DifferentialCustomField',
     'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialConflictsFieldSpecification' => 'DifferentialFieldSpecification',

src/applications/differential/application/PhabricatorApplicationDifferential.php

@@ -55,8 +55,7 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication {
           => 'DifferentialRevisionLandController',
         'comment/' => array(
           'preview/(?P<id>[1-9]\d*)/' => 'DifferentialCommentPreviewController',
-          'save/' => 'DifferentialCommentSaveController',
+          'save/(?P<id>[1-9]\d*)/' => 'DifferentialCommentSaveController',
-          'savepro/(?P<id>[1-9]\d*)/' => 'DifferentialCommentSaveControllerPro',
           'inline/' => array(
             'preview/(?P<id>[1-9]\d*)/'
               => 'DifferentialInlineCommentPreviewController',

src/applications/differential/controller/DifferentialCommentSaveController.php

@@ -1,79 +1,144 @@
 <?php
 
-final class DifferentialCommentSaveController extends DifferentialController {
+final class DifferentialCommentSaveController
+  extends DifferentialController {
+
+  private $id;
+
+  public function willProcessRequest(array $data) {
+    $this->id = $data['id'];
+  }
 
   public function processRequest() {
     $request = $this->getRequest();
+    $viewer = $request->getUser();
+
     if (!$request->isFormPost()) {
       return new Aphront400Response();
     }
 
-    $viewer = $request->getUser();
-
-    $revision_id = $request->getInt('revision_id');
     $revision = id(new DifferentialRevisionQuery())
       ->setViewer($viewer)
-      ->withIDs(array($revision_id))
+      ->withIDs(array($this->id))
+      ->needReviewerStatus(true)
+      ->needReviewerAuthority(true)
       ->executeOne();
     if (!$revision) {
-      return new Aphront400Response();
+      return new Aphront404Response();
     }
 
+    $type_action = DifferentialTransaction::TYPE_ACTION;
+    $type_subscribers = PhabricatorTransactions::TYPE_SUBSCRIBERS;
+    $type_edge = PhabricatorTransactions::TYPE_EDGE;
+    $type_comment = PhabricatorTransactions::TYPE_COMMENT;
+    $type_inline = DifferentialTransaction::TYPE_INLINE;
+
+    $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
+
+    $xactions = array();
+
+    $action = $request->getStr('action');
+    switch ($action) {
+      case DifferentialAction::ACTION_COMMENT:
+      case DifferentialAction::ACTION_ADDREVIEWERS:
+      case DifferentialAction::ACTION_ADDCCS:
+        // These transaction types have no direct effect, they just
+        // accompany other transaction types which can have an effect.
+        break;
+      default:
+        $xactions[] = id(new DifferentialTransaction())
+          ->setTransactionType($type_action)
+          ->setNewValue($request->getStr('action'));
+        break;
+    }
+
+    $ccs = $request->getArr('ccs');
+    if ($ccs) {
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($type_subscribers)
+        ->setNewValue(array('+' => $ccs));
+    }
+
+    $current_reviewers = mpull(
+      $revision->getReviewerStatus(),
+      null,
+      'getReviewerPHID');
+
+    $reviewer_edges = array();
+    $add_reviewers = $request->getArr('reviewers');
+    foreach ($add_reviewers as $reviewer_phid) {
+      if (isset($current_reviewers[$reviewer_phid])) {
+        continue;
+      }
+      $reviewer = new DifferentialReviewer(
+        $reviewer_phid,
+        array(
+          'status' => DifferentialReviewerStatus::STATUS_ADDED,
+        ));
+      $reviewer_edges[$reviewer_phid] = array(
+        'data' => $reviewer->getEdgeData(),
+      );
+    }
+
+    if ($add_reviewers) {
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($type_edge)
+        ->setMetadataValue('edge:type', $edge_reviewer)
+        ->setNewValue(array('+' => $reviewer_edges));
+    }
+
+    $inline_phids = $this->loadUnsubmittedInlinePHIDs($revision);
+    if ($inline_phids) {
+      $inlines = id(new PhabricatorApplicationTransactionCommentQuery())
+        ->setTemplate(new DifferentialTransactionComment())
+        ->setViewer($viewer)
+        ->withPHIDs($inline_phids)
+        ->execute();
+    } else {
+      $inlines = array();
+    }
+
+    foreach ($inlines as $inline) {
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($type_inline)
+        ->attachComment($inline);
+    }
+
+    // NOTE: If there are no other transactions, add an empty comment
+    // transaction so that we'll raise a more user-friendly error message,
+    // to the effect of "you can not post an empty comment".
+    $no_xactions = !$xactions;
+
     $comment = $request->getStr('comment');
-    $action     = $request->getStr('action');
+    if (strlen($comment) || $no_xactions) {
-    $reviewers  = $request->getArr('reviewers');
+      $xactions[] = id(new DifferentialTransaction())
-    $ccs        = $request->getArr('ccs');
+        ->setTransactionType($type_comment)
+        ->attachComment(
+          id(new DifferentialTransactionComment())
+            ->setRevisionPHID($revision->getPHID())
+            ->setContent($comment));
+    }
 
-    $editor = new DifferentialCommentEditor(
-      $revision,
-      $action);
 
-    $content_source = PhabricatorContentSource::newForSource(
+    $editor = id(new DifferentialTransactionEditor())
-      PhabricatorContentSource::SOURCE_WEB,
+      ->setActor($viewer)
-      array(
+      ->setContentSourceFromRequest($request)
-        'ip' => $request->getRemoteAddr(),
+      ->setContinueOnMissingFields(true)
-      ));
+      ->setContinueOnNoEffect($request->isContinueRequest());
+
+    $revision_uri = '/D'.$revision->getID();
 
     try {
-      $editor
+      $editor->applyTransactions($revision, $xactions);
-        ->setActor($request->getUser())
+    } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
-        ->setMessage($comment)
+      return id(new PhabricatorApplicationTransactionNoEffectResponse())
-        ->setContentSource($content_source)
+        ->setCancelURI($revision_uri)
-        ->setAttachInlineComments(true)
+        ->setException($ex);
-        ->setAddedReviewers($reviewers)
+    } catch (PhabricatorApplicationTransactionValidationException $ex) {
-        ->setAddedCCs($ccs)
+      // TODO: Provide a nice Response for rendering these in a clean way.
-        ->save();
+      throw $ex;
-    } catch (DifferentialActionHasNoEffectException $no_effect) {
-      $has_inlines = id(new DifferentialInlineCommentQuery())
-        ->withDraftComments($request->getUser()->getPHID(), $revision->getID())
-        ->execute();
-
-      $dialog = new AphrontDialogView();
-      $dialog->setUser($request->getUser());
-      $dialog->addCancelButton('/D'.$revision_id);
-
-      $dialog->addHiddenInput('revision_id',  $revision_id);
-      $dialog->addHiddenInput('action',       'none');
-      $dialog->addHiddenInput('reviewers',    $reviewers);
-      $dialog->addHiddenInput('ccs',          $ccs);
-      $dialog->addHiddenInput('comment',      $comment);
-
-      $dialog->setTitle(pht('Action Has No Effect'));
-      $dialog->appendChild(
-        phutil_tag('p', array(), $no_effect->getMessage()));
-
-      if (strlen($comment) || $has_inlines) {
-        $dialog->addSubmitButton(pht('Post as Comment'));
-        $dialog->appendChild(phutil_tag('br'));
-        $dialog->appendChild(phutil_tag('p', array(), pht(
-          'Do you want to post your feedback anyway, as a normal comment?')));
     }
 
-      return id(new AphrontDialogResponse())->setDialog($dialog);
-    }
-
-    // TODO: Diff change detection?
-
     $user = $request->getUser();
     $draft = id(new PhabricatorDraft())->loadOneWhere(
       'authorPHID = %s AND draftKey = %s',
@@ -88,4 +153,30 @@ final class DifferentialCommentSaveController extends DifferentialController {
       ->setURI('/D'.$revision->getID());
   }
 
+
+  private function loadUnsubmittedInlinePHIDs(DifferentialRevision $revision) {
+    $viewer = $this->getRequest()->getUser();
+
+    // TODO: This probably needs to move somewhere more central as we move
+    // away from DifferentialInlineCommentQuery, but
+    // PhabricatorApplicationTransactionCommentQuery is currently `final` and
+    // I'm not yet decided on how to approach that. For now, just get the PHIDs
+    // and then execute a PHID-based query through the standard stack.
+
+    $table = new DifferentialTransactionComment();
+    $conn_r = $table->establishConnection('r');
+
+    $phids = queryfx_all(
+      $conn_r,
+      'SELECT phid FROM %T
+        WHERE revisionPHID = %s
+          AND authorPHID = %s
+          AND transactionPHID IS NULL',
+      $table->getTableName(),
+      $revision->getPHID(),
+      $viewer->getPHID());
+
+    return ipull($phids, 'phid');
+  }
+
 }

src/applications/differential/controller/DifferentialCommentSaveControllerPro.php

@@ -1,182 +0,0 @@
-<?php
-
-final class DifferentialCommentSaveControllerPro
-  extends DifferentialController {
-
-  private $id;
-
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-
-    if (!$request->isFormPost()) {
-      return new Aphront400Response();
-    }
-
-    $revision = id(new DifferentialRevisionQuery())
-      ->setViewer($viewer)
-      ->withIDs(array($this->id))
-      ->needReviewerStatus(true)
-      ->needReviewerAuthority(true)
-      ->executeOne();
-    if (!$revision) {
-      return new Aphront404Response();
-    }
-
-    $type_action = DifferentialTransaction::TYPE_ACTION;
-    $type_subscribers = PhabricatorTransactions::TYPE_SUBSCRIBERS;
-    $type_edge = PhabricatorTransactions::TYPE_EDGE;
-    $type_comment = PhabricatorTransactions::TYPE_COMMENT;
-    $type_inline = DifferentialTransaction::TYPE_INLINE;
-
-    $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
-
-    $xactions = array();
-
-    $action = $request->getStr('action');
-    switch ($action) {
-      case DifferentialAction::ACTION_COMMENT:
-      case DifferentialAction::ACTION_ADDREVIEWERS:
-      case DifferentialAction::ACTION_ADDCCS:
-        // These transaction types have no direct effect, they just
-        // accompany other transaction types which can have an effect.
-        break;
-      default:
-        $xactions[] = id(new DifferentialTransaction())
-          ->setTransactionType($type_action)
-          ->setNewValue($request->getStr('action'));
-        break;
-    }
-
-    $ccs = $request->getArr('ccs');
-    if ($ccs) {
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType($type_subscribers)
-        ->setNewValue(array('+' => $ccs));
-    }
-
-    $current_reviewers = mpull(
-      $revision->getReviewerStatus(),
-      null,
-      'getReviewerPHID');
-
-    $reviewer_edges = array();
-    $add_reviewers = $request->getArr('reviewers');
-    foreach ($add_reviewers as $reviewer_phid) {
-      if (isset($current_reviewers[$reviewer_phid])) {
-        continue;
-      }
-      $reviewer = new DifferentialReviewer(
-        $reviewer_phid,
-        array(
-          'status' => DifferentialReviewerStatus::STATUS_ADDED,
-        ));
-      $reviewer_edges[$reviewer_phid] = array(
-        'data' => $reviewer->getEdgeData(),
-      );
-    }
-
-    if ($add_reviewers) {
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType($type_edge)
-        ->setMetadataValue('edge:type', $edge_reviewer)
-        ->setNewValue(array('+' => $reviewer_edges));
-    }
-
-    $inline_phids = $this->loadUnsubmittedInlinePHIDs($revision);
-    if ($inline_phids) {
-      $inlines = id(new PhabricatorApplicationTransactionCommentQuery())
-        ->setTemplate(new DifferentialTransactionComment())
-        ->setViewer($viewer)
-        ->withPHIDs($inline_phids)
-        ->execute();
-    } else {
-      $inlines = array();
-    }
-
-    foreach ($inlines as $inline) {
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType($type_inline)
-        ->attachComment($inline);
-    }
-
-    // NOTE: If there are no other transactions, add an empty comment
-    // transaction so that we'll raise a more user-friendly error message,
-    // to the effect of "you can not post an empty comment".
-    $no_xactions = !$xactions;
-
-    $comment = $request->getStr('comment');
-    if (strlen($comment) || $no_xactions) {
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType($type_comment)
-        ->attachComment(
-          id(new DifferentialTransactionComment())
-            ->setRevisionPHID($revision->getPHID())
-            ->setContent($comment));
-    }
-
-
-    $editor = id(new DifferentialTransactionEditor())
-      ->setActor($viewer)
-      ->setContentSourceFromRequest($request)
-      ->setContinueOnMissingFields(true)
-      ->setContinueOnNoEffect($request->isContinueRequest());
-
-    $revision_uri = '/D'.$revision->getID();
-
-    try {
-      $editor->applyTransactions($revision, $xactions);
-    } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
-      return id(new PhabricatorApplicationTransactionNoEffectResponse())
-        ->setCancelURI($revision_uri)
-        ->setException($ex);
-    } catch (PhabricatorApplicationTransactionValidationException $ex) {
-      // TODO: Provide a nice Response for rendering these in a clean way.
-      throw $ex;
-    }
-
-    $user = $request->getUser();
-    $draft = id(new PhabricatorDraft())->loadOneWhere(
-      'authorPHID = %s AND draftKey = %s',
-      $user->getPHID(),
-      'differential-comment-'.$revision->getID());
-    if ($draft) {
-      $draft->delete();
-    }
-    DifferentialDraft::deleteAllDrafts($user->getPHID(), $revision->getPHID());
-
-    return id(new AphrontRedirectResponse())
-      ->setURI('/D'.$revision->getID());
-  }
-
-
-  private function loadUnsubmittedInlinePHIDs(DifferentialRevision $revision) {
-    $viewer = $this->getRequest()->getUser();
-
-    // TODO: This probably needs to move somewhere more central as we move
-    // away from DifferentialInlineCommentQuery, but
-    // PhabricatorApplicationTransactionCommentQuery is currently `final` and
-    // I'm not yet decided on how to approach that. For now, just get the PHIDs
-    // and then execute a PHID-based query through the standard stack.
-
-    $table = new DifferentialTransactionComment();
-    $conn_r = $table->establishConnection('r');
-
-    $phids = queryfx_all(
-      $conn_r,
-      'SELECT phid FROM %T
-        WHERE revisionPHID = %s
-          AND authorPHID = %s
-          AND transactionPHID IS NULL',
-      $table->getTableName(),
-      $revision->getPHID(),
-      $viewer->getPHID());
-
-    return ipull($phids, 'phid');
-  }
-
-}

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -339,13 +339,8 @@ final class DifferentialRevisionViewController extends DifferentialController {
       // TODO: Restore the ability for fields to add accept warnings.
 
       $comment_form->setActions($this->getRevisionCommentActions($revision));
-
-      $action_uri = '/differential/comment/save/';
-      if (false) {
-        // TODO: Temporary for testing the new comment workflow.
       $action_uri = $this->getApplicationURI(
-          'comment/savepro/'.$revision->getID().'/');
+        'comment/save/'.$revision->getID().'/');
-      }
 
       $comment_form->setActionURI($action_uri);
       $comment_form->setUser($user);

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -732,6 +732,10 @@ final class DifferentialTransactionEditor
     return parent::requireCapabilities($object, $xaction);
   }
 
+  protected function supportsFeed() {
+    return true;
+  }
+
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {