Touch up PHP/JS interactions for inline comments

Summary:
Ref T1460. Overall:

  - Pass `objectOwnerPHID` consistently.
  - Pass viewer consistently.
  - Set the correct draft state for checkboxes on the client.

Test Plan:
  - Made inline comments in Differential.
  - Made inline comments in Diffusion.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T1460

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

resources/celerity/map.php

@@ -11,7 +11,7 @@ return array(
     'core.pkg.js' => '75599122',
     'darkconsole.pkg.js' => '8ab24e01',
     'differential.pkg.css' => '571b1cc1',
-    'differential.pkg.js' => 'e324301d',
+    'differential.pkg.js' => 'c0506961',
     'diffusion.pkg.css' => '591664fa',
     'diffusion.pkg.js' => 'bfc0737b',
     'maniphest.pkg.css' => '68d4dd3d',
@@ -57,7 +57,7 @@ return array(
     'rsrc/css/application/differential/add-comment.css' => 'c47f8c40',
     'rsrc/css/application/differential/changeset-view.css' => 'f36406b1',
     'rsrc/css/application/differential/core.css' => '7ac3cabc',
-    'rsrc/css/application/differential/phui-inline-comment.css' => '17e89126',
+    'rsrc/css/application/differential/phui-inline-comment.css' => 'a96c315d',
     'rsrc/css/application/differential/results-table.css' => '181aa9d9',
     'rsrc/css/application/differential/revision-comment.css' => '024dda6b',
     'rsrc/css/application/differential/revision-history.css' => '0e8eb855',
@@ -365,7 +365,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
     'rsrc/js/application/differential/ChangesetViewManager.js' => '58562350',
-    'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'b3412377',
+    'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '2529c82d',
     'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
     'rsrc/js/application/differential/behavior-comment-preview.js' => '8e1389b5',
@@ -523,7 +523,7 @@ return array(
     'conpherence-widget-pane-css' => '1979ee8c',
     'differential-changeset-view-css' => 'f36406b1',
     'differential-core-view-css' => '7ac3cabc',
-    'differential-inline-comment-editor' => 'b3412377',
+    'differential-inline-comment-editor' => '2529c82d',
     'differential-results-table-css' => '181aa9d9',
     'differential-revision-add-comment-css' => 'c47f8c40',
     'differential-revision-comment-css' => '024dda6b',
@@ -790,7 +790,7 @@ return array(
     'phui-image-mask-css' => '5a8b09c8',
     'phui-info-panel-css' => '27ea50a1',
     'phui-info-view-css' => 'c6f0aef8',
-    'phui-inline-comment-view-css' => '17e89126',
+    'phui-inline-comment-view-css' => 'a96c315d',
     'phui-list-view-css' => '2e25ebfb',
     'phui-object-box-css' => 'd68ce5dc',
     'phui-object-item-list-view-css' => '9db65899',
@@ -995,6 +995,14 @@ return array(
       'javelin-workflow',
       'javelin-util',
     ),
+    '2529c82d' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-request',
+      'javelin-workflow',
+    ),
     '2818f5ce' => array(
       'javelin-install',
       'javelin-util',
@@ -1652,14 +1660,6 @@ return array(
       'javelin-uri',
       'javelin-request',
     ),
-    'b3412377' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-request',
-      'javelin-workflow',
-    ),
     'b3a4b884' => array(
       'javelin-behavior',
       'phabricator-prefab',

src/applications/differential/controller/DifferentialInlineCommentEditController.php

@@ -3,28 +3,31 @@
 final class DifferentialInlineCommentEditController
   extends PhabricatorInlineCommentController {
 
-  private $revisionID;
+  private function getRevisionID() {
-
+    return $this->getRequest()->getURIData('id');
-  public function willProcessRequest(array $data) {
-    $this->revisionID = $data['id'];
   }
 
-  protected function createComment() {
+  private function loadRevision() {
+    $viewer = $this->getViewer();
+    $revision_id = $this->getRevisionID();
 
-    // Verify revision and changeset correspond to actual objects.
-    $revision_id = $this->revisionID;
-    $changeset_id = $this->getChangesetID();
-
-    $viewer = $this->getRequest()->getUser();
     $revision = id(new DifferentialRevisionQuery())
       ->setViewer($viewer)
       ->withIDs(array($revision_id))
       ->executeOne();
-
+   if (!$revision) {
-    if (!$revision) {
+      throw new Exception(pht('Invalid revision ID "%s".', $revision_id));
-      throw new Exception('Invalid revision ID!');
     }
 
+    return $revision;
+  }
+
+  protected function createComment() {
+    // Verify revision and changeset correspond to actual objects.
+    $changeset_id = $this->getChangesetID();
+
+    $revision = $this->loadRevision();
+
     if (!id(new DifferentialChangeset())->load($changeset_id)) {
       throw new Exception('Invalid changeset ID!');
     }
@@ -113,7 +116,7 @@ final class DifferentialInlineCommentEditController
     }
 
     // Inline must be attached to the active revision.
-    if ($inline->getRevisionID() != $this->revisionID) {
+    if ($inline->getRevisionID() != $this->getRevisionID()) {
       return false;
     }
 
@@ -139,4 +142,10 @@ final class DifferentialInlineCommentEditController
         $inline->getPHID());
     $inline->saveTransaction();
   }
+
+  protected function loadObjectOwnerPHID(
+    PhabricatorInlineCommentInterface $inline) {
+    return $this->loadRevision()->getAuthorPHID();
+  }
+
 }

src/applications/differential/controller/DifferentialInlineCommentPreviewController.php

@@ -3,20 +3,30 @@
 final class DifferentialInlineCommentPreviewController
 extends PhabricatorInlineCommentPreviewController {
 
-  private $revisionID;
-
-  public function willProcessRequest(array $data) {
-    $this->revisionID = $data['id'];
-  }
-
   protected function loadInlineComments() {
-    $user = $this->getRequest()->getUser();
+    $viewer = $this->getViewer();
 
-    $inlines = id(new DifferentialInlineCommentQuery())
+    return id(new DifferentialInlineCommentQuery())
-      ->withDraftComments($user->getPHID(), $this->revisionID)
+      ->withDraftComments($viewer->getPHID(), $this->getRevisionID())
       ->execute();
-
-    return $inlines;
   }
 
+  protected function loadObjectOwnerPHID() {
+    $viewer = $this->getViewer();
+
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->getRevisionID()))
+      ->executeOne();
+    if (!$revision) {
+      return null;
+    }
+
+    return $revision->getAuthorPHID();
+  }
+
+
+  private function getRevisionID() {
+    return $this->getRequest()->getURIData('id');
+  }
 }

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php

@@ -459,6 +459,7 @@ abstract class DifferentialChangesetHTMLRenderer
     $allow_done = !$comment->isDraft() && $this->getCanMarkDone();
 
     return id(new PHUIDiffInlineCommentDetailView())
+      ->setUser($user)
       ->setInlineComment($comment)
       ->setIsOnRight($on_right)
       ->setHandles($this->getHandles())

src/applications/diffusion/controller/DiffusionBrowseFileController.php

@@ -845,7 +845,11 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
     $rows = array();
     foreach ($inlines as $inline) {
+
+      // TODO: This should use modern scaffolding code.
+
       $inline_view = id(new PHUIDiffInlineCommentDetailView())
+        ->setUser($this->getViewer())
         ->setMarkupEngine($engine)
         ->setInlineComment($inline)
         ->render();

src/applications/diffusion/controller/DiffusionInlineCommentController.php

@@ -3,39 +3,41 @@
 final class DiffusionInlineCommentController
   extends PhabricatorInlineCommentController {
 
-  private $commitPHID;
+  private function getCommitPHID() {
+    return $this->getRequest()->getURIData('phid');
+  }
 
-  public function willProcessRequest(array $data) {
+  private function loadCommit() {
-    $this->commitPHID = $data['phid'];
+    $viewer = $this->getViewer();
+    $commit_phid = $this->getCommitPHID();
+
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($commit_phid))
+      ->executeOne();
+    if (!$commit) {
+      throw new Exception(pht('Invalid commit PHID "%s"!', $commit_phid));
+    }
+
+    return $commit;
   }
 
   protected function createComment() {
-
+    $commit = $this->loadCommit();
-    // Verify commit and path correspond to actual objects.
-    $commit_phid = $this->commitPHID;
-    $path_id = $this->getChangesetID();
-
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'phid = %s',
-      $commit_phid);
-    if (!$commit) {
-      throw new Exception('Invalid commit ID!');
-    }
 
     // TODO: Write a real PathQuery object?
-
+    $path_id = $this->getChangesetID();
     $path = queryfx_one(
       id(new PhabricatorRepository())->establishConnection('r'),
       'SELECT path FROM %T WHERE id = %d',
       PhabricatorRepository::TABLE_PATH,
       $path_id);
-
     if (!$path) {
       throw new Exception('Invalid path ID!');
     }
 
     return id(new PhabricatorAuditInlineComment())
-      ->setCommitPHID($commit_phid)
+      ->setCommitPHID($commit->getPHID())
       ->setPathID($path_id);
   }
 
@@ -98,7 +100,7 @@ final class DiffusionInlineCommentController
     }
 
     // Inline must be attached to the active revision.
-    if ($inline->getCommitPHID() != $this->commitPHID) {
+    if ($inline->getCommitPHID() != $this->getCommitPHID()) {
       return false;
     }
 
@@ -113,4 +115,10 @@ final class DiffusionInlineCommentController
     return $inline->save();
   }
 
+  protected function loadObjectOwnerPHID(
+    PhabricatorInlineCommentInterface $inline) {
+    return $this->loadCommit()->getAuthorPHID();
+  }
+
+
 }

src/applications/diffusion/controller/DiffusionInlineCommentPreviewController.php

@@ -3,19 +3,30 @@
 final class DiffusionInlineCommentPreviewController
   extends PhabricatorInlineCommentPreviewController {
 
-  private $commitPHID;
-
-  public function willProcessRequest(array $data) {
-    $this->commitPHID = $data['phid'];
-  }
-
   protected function loadInlineComments() {
-    $user = $this->getRequest()->getUser();
+    $viewer = $this->getViewer();
 
-    $inlines = PhabricatorAuditInlineComment::loadDraftComments(
+    return PhabricatorAuditInlineComment::loadDraftComments(
-      $user,
+      $viewer,
-      $this->commitPHID);
+      $this->getCommitPHID());
-
-    return $inlines;
   }
+
+  protected function loadObjectOwnerPHID() {
+    $viewer = $this->getViewer();
+
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($this->getCommitPHID()))
+      ->executeOne();
+    if (!$commit) {
+      return null;
+    }
+
+    return $commit->getAuthorPHID();
+  }
+
+  private function getCommitPHID() {
+    return $this->getRequest()->getURIData('phid');
+  }
+
 }

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -8,6 +8,8 @@ abstract class PhabricatorInlineCommentController
   abstract protected function loadCommentForEdit($id);
   abstract protected function loadCommentForDone($id);
   abstract protected function loadCommentByPHID($phid);
+  abstract protected function loadObjectOwnerPHID(
+    PhabricatorInlineCommentInterface $inline);
   abstract protected function deleteComment(
     PhabricatorInlineCommentInterface $inline);
   abstract protected function saveComment(
@@ -88,6 +90,7 @@ abstract class PhabricatorInlineCommentController
         }
         $inline = $this->loadCommentForDone($this->getCommentID());
 
+        $is_draft_state = false;
         switch ($inline->getFixedState()) {
           case PhabricatorInlineCommentInterface::STATE_DRAFT:
             $next_state = PhabricatorInlineCommentInterface::STATE_UNDONE;
@@ -97,16 +100,22 @@ abstract class PhabricatorInlineCommentController
             break;
           case PhabricatorInlineCommentInterface::STATE_DONE:
             $next_state = PhabricatorInlineCommentInterface::STATE_UNDRAFT;
+            $is_draft_state = true;
             break;
           default:
           case PhabricatorInlineCommentInterface::STATE_UNDONE:
             $next_state = PhabricatorInlineCommentInterface::STATE_DRAFT;
+            $is_draft_state = true;
             break;
         }
 
         $inline->setFixedState($next_state)->save();
 
-        return $this->buildEmptyResponse();
+        return id(new AphrontAjaxResponse())
+          ->setContent(
+            array(
+              'draftState' => $is_draft_state,
+            ));
       case 'delete':
       case 'undelete':
       case 'refdelete':
@@ -306,12 +315,10 @@ abstract class PhabricatorInlineCommentController
     $phids = array($user->getPHID());
 
     $handles = $this->loadViewerHandles($phids);
-
+    $object_owner_phid = $this->loadObjectOwnerPHID($inline);
-    // TODO: This is not correct, but figuring it out is a little bit
-    // involved and it only affects drafts.
-    $object_owner_phid = null;
 
     $view = id(new PHUIDiffInlineCommentDetailView())
+      ->setUser($user)
       ->setInlineComment($inline)
       ->setIsOnRight($on_right)
       ->setMarkupEngine($engine)

src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php

@@ -4,6 +4,7 @@ abstract class PhabricatorInlineCommentPreviewController
   extends PhabricatorController {
 
   abstract protected function loadInlineComments();
+  abstract protected function loadObjectOwnerPHID();
 
   public function processRequest() {
     $request = $this->getRequest();
@@ -23,13 +24,12 @@ abstract class PhabricatorInlineCommentPreviewController
 
     $phids = array($viewer->getPHID());
     $handles = $this->loadViewerHandles($phids);
+    $object_owner_phid = $this->loadObjectOwnerPHID();
 
     $views = array();
     foreach ($inlines as $inline) {
-      // TODO: This is incorrect, but figuring it out is somewhat involved.
-      $object_owner_phid = null;
-
       $view = id(new PHUIDiffInlineCommentDetailView())
+        ->setUser($viewer)
         ->setInlineComment($inline)
         ->setMarkupEngine($engine)
         ->setHandles($handles)

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -130,9 +130,14 @@ final class PHUIDiffInlineCommentDetailView
     if ($inline->getReplyToCommentPHID()) {
       $classes[] = 'inline-comment-is-reply';
     }
-    // Might break?
+
-    if ($this->getCanMarkDone()) {
+    $viewer_phid = $this->getUser()->getPHID();
-      $classes[] = 'viewer-is-diff-author';
+    $owner_phid = $this->getObjectOwnerPHID();
+
+    if ($viewer_phid) {
+      if ($viewer_phid == $owner_phid) {
+        $classes[] = 'viewer-is-object-owner';
+      }
     }
 
     $action_buttons = new PHUIButtonBarView();

webroot/rsrc/css/application/differential/phui-inline-comment.css

@@ -244,6 +244,11 @@
   cursor: pointer;
 }
 
+.inline-state-is-draft .differential-inline-done-label {
+  /* TODO: Placeholder style. */
+  border-style: dashed;
+}
+
 .differential-inline-done-label .differential-inline-done {
   margin: 0 6px 0 0;
   display: inline;

webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js

@@ -301,7 +301,7 @@ JX.install('DifferentialInlineCommentEditor', {
       };
 
       new JX.Workflow(this._uri, data)
-        .setHandler(JX.bind(this, function() {
+        .setHandler(JX.bind(this, function(r) {
           checkbox.checked = !checkbox.checked;
 
           var comment = JX.DOM.findAbove(
@@ -309,8 +309,7 @@ JX.install('DifferentialInlineCommentEditor', {
             'div',
             'differential-inline-comment');
           JX.DOM.alterClass(comment, 'inline-is-done', !!checkbox.checked);
-
+          JX.DOM.alterClass(comment, 'inline-state-is-draft', r.draftState);
-          // TODO: Dynamically update the "inline-state-is-draft" class.
 
           this._didUpdate();
         }))