Differential - add DifferentialDraft to track whether revisions have draft feedback or not

Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.

Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3496

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

resources/sql/autopatches/20140218.differentialdraft.sql

@@ -0,0 +1,9 @@
+CREATE TABLE {$NAMESPACE}_differential.differential_draft(
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  authorPHID VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  draftKey VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_unique` (objectPHID, authorPHID, draftKey)
+) ENGINE=InnoDB, COLLATE utf8_general_ci;

src/__phutil_library_map__.php

@@ -378,6 +378,7 @@ phutil_register_library_map(array(
     'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php',
     'DifferentialDiffViewPolicyFieldSpecification' => 'applications/differential/field/specification/DifferentialDiffViewPolicyFieldSpecification.php',
     'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php',
+    'DifferentialDraft' => 'applications/differential/storage/DifferentialDraft.php',
     'DifferentialEditPolicyFieldSpecification' => 'applications/differential/field/specification/DifferentialEditPolicyFieldSpecification.php',
     'DifferentialException' => 'applications/differential/exception/DifferentialException.php',
     'DifferentialExceptionMail' => 'applications/differential/mail/DifferentialExceptionMail.php',
@@ -2914,6 +2915,7 @@ phutil_register_library_map(array(
     'DifferentialDiffViewController' => 'DifferentialController',
     'DifferentialDiffViewPolicyFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher',
+    'DifferentialDraft' => 'DifferentialDAO',
     'DifferentialEditPolicyFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialException' => 'Exception',
     'DifferentialExceptionMail' => 'DifferentialMail',
@@ -3957,7 +3959,7 @@ phutil_register_library_map(array(
     'PhabricatorCalendarEventOverlapException' => 'Exception',
     'PhabricatorCalendarEventQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorCalendarEventSearchEngine' => 'PhabricatorApplicationSearchEngine',
-    'PhabricatorCalendarEventViewController' => 'PhabricatorDashboardController',
+    'PhabricatorCalendarEventViewController' => 'PhabricatorCalendarController',
     'PhabricatorCalendarHoliday' => 'PhabricatorCalendarDAO',
     'PhabricatorCalendarHolidayTestCase' => 'PhabricatorTestCase',
     'PhabricatorCalendarPHIDTypeEvent' => 'PhabricatorPHIDType',

src/applications/differential/controller/DifferentialCommentPreviewController.php

@@ -119,12 +119,24 @@ final class DifferentialCommentPreviewController
       $metadata['action'] = $action;
     }
 
-    id(new PhabricatorDraft())
+    $draft_key = 'differential-comment-'.$this->id;
+    $draft = id(new PhabricatorDraft())
       ->setAuthorPHID($viewer->getPHID())
-      ->setDraftKey('differential-comment-'.$this->id)
+      ->setDraftKey($draft_key)
       ->setDraft($request->getStr('content'))
       ->setMetadata($metadata)
       ->replaceOrDelete();
+    if ($draft->isDeleted()) {
+      DifferentialDraft::deleteHasDraft(
+        $viewer->getPHID(),
+        $revision->getPHID(),
+        $draft_key);
+    } else {
+      DifferentialDraft::markHasDraft(
+        $viewer->getPHID(),
+        $revision->getPHID(),
+        $draft_key);
+    }
 
     return id(new AphrontAjaxResponse())
       ->setContent((string)phutil_implode_html('', $view->buildEvents()));

src/applications/differential/controller/DifferentialCommentSaveController.php

@@ -74,13 +74,15 @@ final class DifferentialCommentSaveController extends DifferentialController {
 
     // TODO: Diff change detection?
 
+    $user = $request->getUser();
     $draft = id(new PhabricatorDraft())->loadOneWhere(
       'authorPHID = %s AND draftKey = %s',
-      $request->getUser()->getPHID(),
+      $user->getPHID(),
       'differential-comment-'.$revision->getID());
     if ($draft) {
       $draft->delete();
     }
+    DifferentialDraft::deleteAllDrafts($user->getPHID(), $revision->getPHID());
 
     return id(new AphrontRedirectResponse())
       ->setURI('/D'.$revision->getID());

src/applications/differential/controller/DifferentialInlineCommentEditController.php

@@ -74,4 +74,23 @@ final class DifferentialInlineCommentEditController
     return true;
   }
 
+  protected function deleteComment(PhabricatorInlineCommentInterface $inline) {
+    $inline->openTransaction();
+      DifferentialDraft::deleteHasDraft(
+        $inline->getAuthorPHID(),
+        $inline->getRevisionPHID(),
+        $inline->getPHID());
+      $inline->delete();
+    $inline->saveTransaction();
+  }
+
+  protected function saveComment(PhabricatorInlineCommentInterface $inline) {
+    $inline->openTransaction();
+      $inline->save();
+      DifferentialDraft::markHasDraft(
+        $inline->getAuthorPHID(),
+        $inline->getRevisionPHID(),
+        $inline->getPHID());
+    $inline->saveTransaction();
+  }
 }

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -36,7 +36,6 @@ final class DifferentialRevisionQuery
   private $responsibles = array();
   private $branches = array();
   private $arcanistProjectPHIDs = array();
-  private $draftRevisions = array();
   private $repositoryPHIDs;
 
   private $order            = 'order-modified';
@@ -468,39 +467,6 @@ final class DifferentialRevisionQuery
     $table = new DifferentialRevision();
     $conn_r = $table->establishConnection('r');
 
-    if ($this->draftAuthors) {
-      $this->draftRevisions = array();
-
-      $draft_key = 'differential-comment-';
-      $drafts = id(new PhabricatorDraft())->loadAllWhere(
-        'authorPHID IN (%Ls) AND draftKey LIKE %> AND draft != %s',
-        $this->draftAuthors,
-        $draft_key,
-        '');
-      $len = strlen($draft_key);
-      foreach ($drafts as $draft) {
-        $this->draftRevisions[] = substr($draft->getDraftKey(), $len);
-      }
-
-      // TODO: Restore this after drafts are sorted out. It's now very
-      // expensive to get revision IDs.
-
-      /*
-
-      $inlines = id(new DifferentialInlineCommentQuery())
-        ->withDraftsByAuthors($this->draftAuthors)
-        ->execute();
-      foreach ($inlines as $inline) {
-        $this->draftRevisions[] = $inline->getRevisionID();
-      }
-
-      */
-
-      if (!$this->draftRevisions) {
-        return array();
-      }
-    }
-
     $selects = array();
 
     // NOTE: If the query includes "responsiblePHIDs", we execute it as a
@@ -632,6 +598,16 @@ final class DifferentialRevisionQuery
         $this->reviewers);
     }
 
+    if ($this->draftAuthors) {
+      $differential_draft = new DifferentialDraft();
+      $joins[] = qsprintf(
+        $conn_r,
+        'JOIN %T has_draft ON has_draft.objectPHID = r.phid '.
+        'AND has_draft.authorPHID IN (%Ls)',
+        $differential_draft->getTableName(),
+        $this->draftAuthors);
+    }
+
     $joins = implode(' ', $joins);
 
     return $joins;
@@ -665,13 +641,6 @@ final class DifferentialRevisionQuery
         $this->authors);
     }
 
-    if ($this->draftRevisions) {
-      $where[] = qsprintf(
-        $conn_r,
-        'r.id IN (%Ld)',
-        $this->draftRevisions);
-    }
-
     if ($this->revIDs) {
       $where[] = qsprintf(
         $conn_r,

src/applications/differential/storage/DifferentialDraft.php

@@ -0,0 +1,51 @@
+<?php
+
+final class DifferentialDraft extends DifferentialDAO {
+
+  protected $objectPHID;
+  protected $authorPHID;
+  protected $draftKey;
+
+  public static function markHasDraft(
+    $author_phid,
+    $object_phid,
+    $draft_key) {
+    try {
+      id(new DifferentialDraft())
+        ->setObjectPHID($object_phid)
+        ->setAuthorPHID($author_phid)
+        ->setDraftKey($draft_key)
+        ->save();
+    } catch (AphrontQueryDuplicateKeyException $ex) {
+      // no worries
+    }
+  }
+
+  public static function deleteHasDraft(
+    $author_phid,
+    $object_phid,
+    $draft_key) {
+    $draft = id(new DifferentialDraft())->loadOneWhere(
+      'objectPHID = %s AND authorPHID = %s AND draftKey = %s',
+      $object_phid,
+      $author_phid,
+      $draft_key);
+    if ($draft) {
+      $draft->delete();
+    }
+  }
+
+  public static function deleteAllDrafts(
+    $author_phid,
+    $object_phid) {
+
+    $drafts = id(new DifferentialDraft())->loadAllWhere(
+      'objectPHID = %s AND authorPHID = %s',
+      $object_phid,
+      $author_phid);
+    foreach ($drafts as $draft) {
+      $draft->delete();
+    }
+  }
+
+}

src/applications/differential/storage/DifferentialInlineComment.php

@@ -28,6 +28,13 @@ final class DifferentialInlineComment
     return $this->proxy;
   }
 
+  public function openTransaction() {
+    $this->proxy->openTransaction();
+  }
+
+  public function saveTransaction() {
+    $this->proxy->saveTransaction();
+  }
 
   public function save() {
     $this->getTransactionCommentForSave()->save();
@@ -45,6 +52,10 @@ final class DifferentialInlineComment
     return $this->proxy->getID();
   }
 
+  public function getPHID() {
+    return $this->proxy->getPHID();
+  }
+
   public static function newFromModernComment(
     DifferentialTransactionComment $comment) {
 
@@ -141,6 +152,10 @@ final class DifferentialInlineComment
     return $this;
   }
 
+  public function getRevisionPHID() {
+    return $this->proxy->getRevisionPHID();
+  }
+
   // Although these are purely transitional, they're also *extra* dumb.
 
   public function setRevisionID($revision_id) {

src/applications/diffusion/controller/DiffusionInlineCommentController.php

@@ -76,4 +76,12 @@ final class DiffusionInlineCommentController
     return true;
   }
 
+  protected function deleteComment(PhabricatorInlineCommentInterface $inline) {
+    return $inline->delete();
+  }
+
+  protected function saveComment(PhabricatorInlineCommentInterface $inline) {
+    return $inline->save();
+  }
+
 }

src/applications/draft/storage/PhabricatorDraft.php

@@ -7,6 +7,8 @@ final class PhabricatorDraft extends PhabricatorDraftDAO {
   protected $draft;
   protected $metadata = array();
 
+  private $deleted = false;
+
   public function getConfiguration() {
     return array(
       self::CONFIG_SERIALIZATION => array(
@@ -23,11 +25,20 @@ final class PhabricatorDraft extends PhabricatorDraftDAO {
         $this->getTableName(),
         $this->authorPHID,
         $this->draftKey);
+      $this->deleted = true;
       return $this;
     }
     return parent::replace();
   }
 
+  protected function didDelete() {
+    $this->deleted = true;
+  }
+
+  public function isDeleted() {
+    return $this->deleted;
+  }
+
   public static function newFromUserAndKey(PhabricatorUser $user, $key) {
     if ($user->getPHID() && strlen($key)) {
       $draft = id(new PhabricatorDraft())->loadOneWhere(

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -6,6 +6,10 @@ abstract class PhabricatorInlineCommentController
   abstract protected function createComment();
   abstract protected function loadComment($id);
   abstract protected function loadCommentForEdit($id);
+  abstract protected function deleteComment(
+    PhabricatorInlineCommentInterface $inline);
+  abstract protected function saveComment(
+    PhabricatorInlineCommentInterface $inline);
 
   private $changesetID;
   private $isNewFile;
@@ -60,7 +64,7 @@ abstract class PhabricatorInlineCommentController
         $inline = $this->loadCommentForEdit($this->getCommentID());
 
         if ($request->isFormPost()) {
-          $inline->delete();
+          $this->deleteComment($inline);
           return $this->buildEmptyResponse();
         }
 
@@ -86,12 +90,12 @@ abstract class PhabricatorInlineCommentController
         if ($request->isFormPost()) {
           if (strlen($text)) {
             $inline->setContent($text);
-            $inline->save();
+            $this->saveComment($inline);
             return $this->buildRenderedCommentResponse(
               $inline,
               $this->getIsOnRight());
           } else {
-            $inline->delete();
+            $this->deleteComment($inline);
             return $this->buildEmptyResponse();
           }
         }
@@ -122,8 +126,8 @@ abstract class PhabricatorInlineCommentController
           ->setLineNumber($this->getLineNumber())
           ->setLineLength($this->getLineLength())
           ->setIsNewFile($this->getIsNewFile())
-          ->setContent($text)
+          ->setContent($text);
-          ->save();
+        $this->saveComment($inline);
 
         return $this->buildRenderedCommentResponse(
           $inline,