From 7276af6a81f49bbdc14ace064aab50afbeb79cfc Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 Jan 2017 06:36:25 -0800 Subject: [PATCH] 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 `` 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 | 10 ++ ...ifferentialInlineCommentEditController.php | 29 +++--- .../DifferentialRevisionDraftEngine.php | 17 ++++ .../query/DifferentialRevisionQuery.php | 45 ++++++--- .../storage/DifferentialDraft.php | 42 -------- .../storage/DifferentialRevision.php | 19 +++- .../view/DifferentialRevisionListView.php | 2 +- .../draft/PhabricatorBuiltinDraftEngine.php | 4 + .../draft/PhabricatorDraftEngine.php | 98 +++++++++++++++++++ .../draft/PhabricatorDraftInterface.php | 7 ++ .../PhabricatorObjectHasDraftEdgeType.php | 8 ++ .../editengine/PhabricatorEditEngine.php | 30 ++++++ 12 files changed, 234 insertions(+), 77 deletions(-) create mode 100644 src/applications/differential/engine/DifferentialRevisionDraftEngine.php create mode 100644 src/applications/transactions/draft/PhabricatorBuiltinDraftEngine.php create mode 100644 src/applications/transactions/draft/PhabricatorDraftEngine.php create mode 100644 src/applications/transactions/draft/PhabricatorDraftInterface.php create mode 100644 src/applications/transactions/edges/PhabricatorObjectHasDraftEdgeType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e0bcc82c8d..617365f333 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 42b96920eb..926158cb41 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/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( - $inline->getAuthorPHID(), - $inline->getRevisionPHID(), - $inline->getPHID()); - + $this->syncDraft(); $inline->saveTransaction(); } protected function undeleteComment( PhabricatorInlineCommentInterface $inline) { $inline->openTransaction(); - $inline->setIsDeleted(0)->save(); - DifferentialDraft::markHasDraft( - $inline->getAuthorPHID(), - $inline->getRevisionPHID(), - $inline->getPHID()); - + $this->syncDraft(); $inline->saveTransaction(); } protected function saveComment(PhabricatorInlineCommentInterface $inline) { $inline->openTransaction(); $inline->save(); - DifferentialDraft::markHasDraft( - $inline->getAuthorPHID(), - $inline->getRevisionPHID(), - $inline->getPHID()); + $this->syncDraft(); $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(); + } + } diff --git a/src/applications/differential/engine/DifferentialRevisionDraftEngine.php b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php new file mode 100644 index 0000000000..94669b0182 --- /dev/null +++ b/src/applications/differential/engine/DifferentialRevisionDraftEngine.php @@ -0,0 +1,17 @@ +getViewer(); + $revision = $this->getObject(); + + $inlines = DifferentialTransactionQuery::loadUnsubmittedInlineComments( + $viewer, + $revision); + + return (bool)$inlines; + } + +} diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 6dd690d179..a4e446ad0d 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -473,15 +473,33 @@ final class DifferentialRevisionQuery } if ($this->needDrafts) { - $drafts = id(new DifferentialDraft())->loadAllWhere( - 'authorPHID = %s AND objectPHID IN (%Ls)', - $viewer->getPHID(), - mpull($revisions, 'getPHID')); - $drafts = mgroup($drafts, 'getObjectPHID'); - foreach ($revisions as $revision) { - $revision->attachDrafts( - $viewer, - idx($drafts, $revision->getPHID(), array())); + $viewer_phid = $viewer->getPHID(); + $draft_type = PhabricatorObjectHasDraftEdgeType::EDGECONST; + + if (!$viewer_phid) { + // Viewers without a valid PHID can never have drafts. + foreach ($revisions as $revision) { + $revision->attachHasDraft($viewer, false); + } + } 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 '. - 'AND has_draft.authorPHID IN (%Ls)', - $differential_draft->getTableName(), + 'JOIN %T has_draft ON has_draft.srcPHID = r.phid + AND has_draft.type = %s + AND has_draft.dstPHID IN (%Ls)', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + PhabricatorObjectHasDraftEdgeType::EDGECONST, $this->draftAuthors); } diff --git a/src/applications/differential/storage/DifferentialDraft.php b/src/applications/differential/storage/DifferentialDraft.php index 371698937c..7dbe2f68bd 100644 --- a/src/applications/differential/storage/DifferentialDraft.php +++ b/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(); - } - } - } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 1e90b12dc4..df43043d6d 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/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) { - return $this->assertAttachedKey($this->drafts, $viewer->getPHID()); + public function getHasDraft(PhabricatorUser $viewer) { + return $this->assertAttachedKey($this->drafts, $viewer->getCacheFragment()); } - public function attachDrafts(PhabricatorUser $viewer, array $drafts) { - $this->drafts[$viewer->getPHID()] = $drafts; + public function attachHasDraft(PhabricatorUser $viewer, $has_draft) { + $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(); + } + } diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php index 7ab0f2d183..811e74fd65 100644 --- a/src/applications/differential/view/DifferentialRevisionListView.php +++ b/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; } diff --git a/src/applications/transactions/draft/PhabricatorBuiltinDraftEngine.php b/src/applications/transactions/draft/PhabricatorBuiltinDraftEngine.php new file mode 100644 index 0000000000..590ebea90c --- /dev/null +++ b/src/applications/transactions/draft/PhabricatorBuiltinDraftEngine.php @@ -0,0 +1,4 @@ +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(); + } + +} diff --git a/src/applications/transactions/draft/PhabricatorDraftInterface.php b/src/applications/transactions/draft/PhabricatorDraftInterface.php new file mode 100644 index 0000000000..69b2edf8a0 --- /dev/null +++ b/src/applications/transactions/draft/PhabricatorDraftInterface.php @@ -0,0 +1,7 @@ +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 )------------------------------------------------------------ */