Make yellow "draft" bubbles more generic

Summary:
Fixes T12095. Ref T6660. The old code for this was specific to Differential, using the `DifferentialDraft` table.

Instead, make the `EditEngine` / `VersionedDraft` code create and remove a `<objectPHID, authorPHID>` edge when a particular author creates drafts.

Some applications have drafts beyond `VersionedDrafts`, notably inline comments. Before writing "yes, draft" or "no, no draft", ask the object if it has any custom draft stuff we need to know about.

This should fix all the yellow bubble bugs I created in T11114 and allow us to bring the feature to Audit fairly easily.

Test Plan: Created and deleted comments and inlines, reloading the list view after each change. Couldn't find a way to break the list view anymore.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12095, T6660

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

src/__phutil_library_map__.php

@@ -519,6 +519,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php',
+    'DifferentialRevisionDraftEngine' => 'applications/differential/engine/DifferentialRevisionDraftEngine.php',
     'DifferentialRevisionEditConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php',
     'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php',
     'DifferentialRevisionEditEngine' => 'applications/differential/editor/DifferentialRevisionEditEngine.php',
@@ -2055,6 +2056,7 @@ phutil_register_library_map(array(
     'PhabricatorBotUser' => 'infrastructure/daemon/bot/target/PhabricatorBotUser.php',
     'PhabricatorBotWhatsNewHandler' => 'infrastructure/daemon/bot/handler/PhabricatorBotWhatsNewHandler.php',
     'PhabricatorBritishEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorBritishEnglishTranslation.php',
+    'PhabricatorBuiltinDraftEngine' => 'applications/transactions/draft/PhabricatorBuiltinDraftEngine.php',
     'PhabricatorBuiltinPatchList' => 'infrastructure/storage/patch/PhabricatorBuiltinPatchList.php',
     'PhabricatorBulkContentSource' => 'infrastructure/daemon/contentsource/PhabricatorBulkContentSource.php',
     'PhabricatorBusyUIExample' => 'applications/uiexample/examples/PhabricatorBusyUIExample.php',
@@ -2550,6 +2552,8 @@ phutil_register_library_map(array(
     'PhabricatorDoorkeeperApplication' => 'applications/doorkeeper/application/PhabricatorDoorkeeperApplication.php',
     'PhabricatorDraft' => 'applications/draft/storage/PhabricatorDraft.php',
     'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php',
+    'PhabricatorDraftEngine' => 'applications/transactions/draft/PhabricatorDraftEngine.php',
+    'PhabricatorDraftInterface' => 'applications/transactions/draft/PhabricatorDraftInterface.php',
     'PhabricatorDrydockApplication' => 'applications/drydock/application/PhabricatorDrydockApplication.php',
     'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php',
     'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php',
@@ -3095,6 +3099,7 @@ phutil_register_library_map(array(
     'PhabricatorObjectHasAsanaSubtaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaSubtaskEdgeType.php',
     'PhabricatorObjectHasAsanaTaskEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasAsanaTaskEdgeType.php',
     'PhabricatorObjectHasContributorEdgeType' => 'applications/transactions/edges/PhabricatorObjectHasContributorEdgeType.php',
+    'PhabricatorObjectHasDraftEdgeType' => 'applications/transactions/edges/PhabricatorObjectHasDraftEdgeType.php',
     'PhabricatorObjectHasFileEdgeType' => 'applications/transactions/edges/PhabricatorObjectHasFileEdgeType.php',
     'PhabricatorObjectHasJiraIssueEdgeType' => 'applications/doorkeeper/edge/PhabricatorObjectHasJiraIssueEdgeType.php',
     'PhabricatorObjectHasSubscriberEdgeType' => 'applications/transactions/edges/PhabricatorObjectHasSubscriberEdgeType.php',
@@ -5210,6 +5215,7 @@ phutil_register_library_map(array(
       'PhabricatorProjectInterface',
       'PhabricatorFulltextInterface',
       'PhabricatorConduitResultInterface',
+      'PhabricatorDraftInterface',
     ),
     'DifferentialRevisionAbandonTransaction' => 'DifferentialRevisionActionTransaction',
     'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionReviewTransaction',
@@ -5226,6 +5232,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'Phobject',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'PhabricatorEdgeType',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'PhabricatorEdgeType',
+    'DifferentialRevisionDraftEngine' => 'PhabricatorDraftEngine',
     'DifferentialRevisionEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod',
     'DifferentialRevisionEditController' => 'DifferentialController',
     'DifferentialRevisionEditEngine' => 'PhabricatorEditEngine',
@@ -6981,6 +6988,7 @@ phutil_register_library_map(array(
     'PhabricatorBotUser' => 'PhabricatorBotTarget',
     'PhabricatorBotWhatsNewHandler' => 'PhabricatorBotHandler',
     'PhabricatorBritishEnglishTranslation' => 'PhutilTranslation',
+    'PhabricatorBuiltinDraftEngine' => 'PhabricatorDraftEngine',
     'PhabricatorBuiltinPatchList' => 'PhabricatorSQLPatchList',
     'PhabricatorBulkContentSource' => 'PhabricatorContentSource',
     'PhabricatorBusyUIExample' => 'PhabricatorUIExample',
@@ -7559,6 +7567,7 @@ phutil_register_library_map(array(
     'PhabricatorDoorkeeperApplication' => 'PhabricatorApplication',
     'PhabricatorDraft' => 'PhabricatorDraftDAO',
     'PhabricatorDraftDAO' => 'PhabricatorLiskDAO',
+    'PhabricatorDraftEngine' => 'Phobject',
     'PhabricatorDrydockApplication' => 'PhabricatorApplication',
     'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants',
     'PhabricatorEdgeConstants' => 'Phobject',
@@ -8172,6 +8181,7 @@ phutil_register_library_map(array(
     'PhabricatorObjectHasAsanaSubtaskEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorObjectHasAsanaTaskEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorObjectHasContributorEdgeType' => 'PhabricatorEdgeType',
+    'PhabricatorObjectHasDraftEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorObjectHasFileEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorObjectHasJiraIssueEdgeType' => 'PhabricatorEdgeType',
     'PhabricatorObjectHasSubscriberEdgeType' => 'PhabricatorEdgeType',

src/applications/differential/controller/DifferentialInlineCommentEditController.php

@@ -152,36 +152,23 @@ final class DifferentialInlineCommentEditController
 
   protected function deleteComment(PhabricatorInlineCommentInterface $inline) {
     $inline->openTransaction();
-
       $inline->setIsDeleted(1)->save();
-      DifferentialDraft::deleteHasDraft(
+      $this->syncDraft();
-        $inline->getAuthorPHID(),
-        $inline->getRevisionPHID(),
-        $inline->getPHID());
-
     $inline->saveTransaction();
   }
 
   protected function undeleteComment(
     PhabricatorInlineCommentInterface $inline) {
     $inline->openTransaction();
-
       $inline->setIsDeleted(0)->save();
-      DifferentialDraft::markHasDraft(
+      $this->syncDraft();
-        $inline->getAuthorPHID(),
-        $inline->getRevisionPHID(),
-        $inline->getPHID());
-
     $inline->saveTransaction();
   }
 
   protected function saveComment(PhabricatorInlineCommentInterface $inline) {
     $inline->openTransaction();
       $inline->save();
-      DifferentialDraft::markHasDraft(
+      $this->syncDraft();
-        $inline->getAuthorPHID(),
-        $inline->getRevisionPHID(),
-        $inline->getPHID());
     $inline->saveTransaction();
   }
 
@@ -224,4 +211,14 @@ final class DifferentialInlineCommentEditController
       $ids);
   }
 
+  private function syncDraft() {
+    $viewer = $this->getViewer();
+    $revision = $this->loadRevision();
+
+    $revision->newDraftEngine()
+      ->setObject($revision)
+      ->setViewer($viewer)
+      ->synchronize();
+  }
+
 }

src/applications/differential/engine/DifferentialRevisionDraftEngine.php

@@ -0,0 +1,17 @@
+<?php
+
+final class DifferentialRevisionDraftEngine
+  extends PhabricatorDraftEngine {
+
+  protected function hasCustomDraftContent() {
+    $viewer = $this->getViewer();
+    $revision = $this->getObject();
+
+    $inlines = DifferentialTransactionQuery::loadUnsubmittedInlineComments(
+      $viewer,
+      $revision);
+
+    return (bool)$inlines;
+  }
+
+}

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -473,15 +473,33 @@ final class DifferentialRevisionQuery
     }
 
     if ($this->needDrafts) {
-      $drafts = id(new DifferentialDraft())->loadAllWhere(
+      $viewer_phid = $viewer->getPHID();
-        'authorPHID = %s AND objectPHID IN (%Ls)',
+      $draft_type = PhabricatorObjectHasDraftEdgeType::EDGECONST;
-        $viewer->getPHID(),
+
-        mpull($revisions, 'getPHID'));
+      if (!$viewer_phid) {
-      $drafts = mgroup($drafts, 'getObjectPHID');
+        // Viewers without a valid PHID can never have drafts.
-      foreach ($revisions as $revision) {
+        foreach ($revisions as $revision) {
-        $revision->attachDrafts(
+          $revision->attachHasDraft($viewer, false);
-          $viewer,
+        }
-          idx($drafts, $revision->getPHID(), array()));
+      } else {
+        $edge_query = id(new PhabricatorEdgeQuery())
+          ->withSourcePHIDs(mpull($revisions, 'getPHID'))
+          ->withEdgeTypes(
+            array(
+              $draft_type,
+            ))
+          ->withDestinationPHIDs(array($viewer_phid));
+
+        $edge_query->execute();
+
+        foreach ($revisions as $revision) {
+          $has_draft = (bool)$edge_query->getDestinationPHIDs(
+            array(
+              $revision->getPHID(),
+            ));
+
+          $revision->attachHasDraft($viewer, $has_draft);
+        }
       }
     }
 
@@ -621,12 +639,13 @@ final class DifferentialRevisionQuery
     }
 
     if ($this->draftAuthors) {
-      $differential_draft = new DifferentialDraft();
       $joins[] = qsprintf(
         $conn_r,
-        'JOIN %T has_draft ON has_draft.objectPHID = r.phid '.
+        'JOIN %T has_draft ON has_draft.srcPHID = r.phid
-        'AND has_draft.authorPHID IN (%Ls)',
+          AND has_draft.type = %s
-        $differential_draft->getTableName(),
+          AND has_draft.dstPHID IN (%Ls)',
+        PhabricatorEdgeConfig::TABLE_NAME_EDGE,
+        PhabricatorObjectHasDraftEdgeType::EDGECONST,
         $this->draftAuthors);
     }
 

src/applications/differential/storage/DifferentialDraft.php

@@ -20,46 +20,4 @@ final class DifferentialDraft extends DifferentialDAO {
     )  + parent::getConfiguration();
   }
 
-  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 (AphrontDuplicateKeyQueryException $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/DifferentialRevision.php

@@ -15,7 +15,8 @@ final class DifferentialRevision extends DifferentialDAO
     PhabricatorDestructibleInterface,
     PhabricatorProjectInterface,
     PhabricatorFulltextInterface,
-    PhabricatorConduitResultInterface {
+    PhabricatorConduitResultInterface,
+    PhabricatorDraftInterface {
 
   protected $title = '';
   protected $originalTitle;
@@ -488,12 +489,12 @@ final class DifferentialRevision extends DifferentialDAO
     return $this;
   }
 
-  public function getDrafts(PhabricatorUser $viewer) {
+  public function getHasDraft(PhabricatorUser $viewer) {
-    return $this->assertAttachedKey($this->drafts, $viewer->getPHID());
+    return $this->assertAttachedKey($this->drafts, $viewer->getCacheFragment());
   }
 
-  public function attachDrafts(PhabricatorUser $viewer, array $drafts) {
+  public function attachHasDraft(PhabricatorUser $viewer, $has_draft) {
-    $this->drafts[$viewer->getPHID()] = $drafts;
+    $this->drafts[$viewer->getCacheFragment()] = $has_draft;
     return $this;
   }
 
@@ -735,4 +736,12 @@ final class DifferentialRevision extends DifferentialDAO
     return array();
   }
 
+
+/* -(  PhabricatorDraftInterface  )------------------------------------------ */
+
+
+  public function newDraftEngine() {
+    return new DifferentialRevisionDraftEngine();
+  }
+
 }

src/applications/differential/view/DifferentialRevisionListView.php

@@ -92,7 +92,7 @@ final class DifferentialRevisionListView extends AphrontView {
           '');
       }
 
-      if ($revision->getDrafts($viewer)) {
+      if ($revision->getHasDraft($viewer)) {
         $icons['draft'] = true;
       }
 

src/applications/transactions/draft/PhabricatorBuiltinDraftEngine.php

@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorBuiltinDraftEngine
+  extends PhabricatorDraftEngine {}

src/applications/transactions/draft/PhabricatorDraftEngine.php

@@ -0,0 +1,98 @@
+<?php
+
+abstract class PhabricatorDraftEngine
+  extends Phobject {
+
+  private $viewer;
+  private $object;
+  private $hasVersionedDraft;
+  private $versionedDraft;
+
+  final public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  final public function getViewer() {
+    return $this->viewer;
+  }
+
+  final public function setObject($object) {
+    $this->object = $object;
+    return $this;
+  }
+
+  final public function getObject() {
+    return $this->object;
+  }
+
+  final public function setVersionedDraft(
+    PhabricatorVersionedDraft $draft = null) {
+    $this->hasVersionedDraft = true;
+    $this->versionedDraft = $draft;
+    return $this;
+  }
+
+  final public function getVersionedDraft() {
+    if (!$this->hasVersionedDraft) {
+      $draft = PhabricatorVersionedDraft::loadDraft(
+        $this->getObject()->getPHID(),
+        $this->getViewer()->getPHID());
+      $this->setVersionedDraft($draft);
+    }
+
+    return $this->versionedDraft;
+  }
+
+  protected function hasVersionedDraftContent() {
+    $draft = $this->getVersionedDraft();
+    if (!$draft) {
+      return false;
+    }
+
+    if ($draft->getProperty('comment')) {
+      return true;
+    }
+
+    if ($draft->getProperty('actions')) {
+      return true;
+    }
+
+    return false;
+  }
+
+  protected function hasCustomDraftContent() {
+    return false;
+  }
+
+  final protected function hasAnyDraftContent() {
+    if ($this->hasVersionedDraftContent()) {
+      return true;
+    }
+
+    if ($this->hasCustomDraftContent()) {
+      return true;
+    }
+
+    return false;
+  }
+
+  final public function synchronize() {
+    $object_phid = $this->getObject()->getPHID();
+    $viewer_phid = $this->getViewer()->getPHID();
+
+    $has_draft = $this->hasAnyDraftContent();
+
+    $draft_type = PhabricatorObjectHasDraftEdgeType::EDGECONST;
+    $editor = id(new PhabricatorEdgeEditor());
+
+    if ($has_draft) {
+      $editor->addEdge($object_phid, $draft_type, $viewer_phid);
+    } else {
+      $editor->removeEdge($object_phid, $draft_type, $viewer_phid);
+    }
+
+    $editor->save();
+  }
+
+}

src/applications/transactions/draft/PhabricatorDraftInterface.php

@@ -0,0 +1,7 @@
+<?php
+
+interface PhabricatorDraftInterface {
+
+  public function newDraftEngine();
+
+}

src/applications/transactions/edges/PhabricatorObjectHasDraftEdgeType.php

@@ -0,0 +1,8 @@
+<?php
+
+final class PhabricatorObjectHasDraftEdgeType
+  extends PhabricatorEdgeType {
+
+  const EDGECONST = 64;
+
+}

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1746,10 +1746,19 @@ abstract class PhabricatorEditEngine
           $viewer->getPHID(),
           $current_version);
 
+        $is_empty = (!strlen($comment_text) && !$actions);
+
         $draft
           ->setProperty('comment', $comment_text)
           ->setProperty('actions', $actions)
           ->save();
+
+        $draft_engine = $this->newDraftEngine($object);
+        if ($draft_engine) {
+          $draft_engine
+            ->setVersionedDraft($draft)
+            ->synchronize();
+        }
       }
     }
 
@@ -1831,6 +1840,13 @@ abstract class PhabricatorEditEngine
         $object->getPHID(),
         $viewer->getPHID(),
         $this->loadDraftVersion($object));
+
+      $draft_engine = $this->newDraftEngine($object);
+      if ($draft_engine) {
+        $draft_engine
+          ->setVersionedDraft(null)
+          ->synchronize();
+      }
     }
 
     if ($request->isAjax() && $is_preview) {
@@ -1847,6 +1863,20 @@ abstract class PhabricatorEditEngine
     }
   }
 
+  protected function newDraftEngine($object) {
+    $viewer = $this->getViewer();
+
+    if ($object instanceof PhabricatorDraftInterface) {
+      $engine = $object->newDraftEngine();
+    } else {
+      $engine = new PhabricatorBuiltinDraftEngine();
+    }
+
+    return $engine
+      ->setObject($object)
+      ->setViewer($viewer);
+  }
+
 
 /* -(  Conduit  )------------------------------------------------------------ */