From dcd7a316d239cccc5ba5b0891e2ad351ff562d53 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Tue, 18 Feb 2014 16:25:16 -0800 Subject: [PATCH] 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 --- .../20140218.differentialdraft.sql | 9 ++++ src/__phutil_library_map__.php | 4 +- .../DifferentialCommentPreviewController.php | 16 +++++- .../DifferentialCommentSaveController.php | 4 +- ...ifferentialInlineCommentEditController.php | 19 +++++++ .../query/DifferentialRevisionQuery.php | 51 ++++--------------- .../storage/DifferentialDraft.php | 51 +++++++++++++++++++ .../storage/DifferentialInlineComment.php | 15 ++++++ .../DiffusionInlineCommentController.php | 8 +++ .../draft/storage/PhabricatorDraft.php | 11 ++++ .../PhabricatorInlineCommentController.php | 14 +++-- 11 files changed, 152 insertions(+), 50 deletions(-) create mode 100644 resources/sql/autopatches/20140218.differentialdraft.sql create mode 100644 src/applications/differential/storage/DifferentialDraft.php diff --git a/resources/sql/autopatches/20140218.differentialdraft.sql b/resources/sql/autopatches/20140218.differentialdraft.sql new file mode 100644 index 0000000000..7421e08c98 --- /dev/null +++ b/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; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index eb76217a3d..4269d031a4 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialCommentPreviewController.php b/src/applications/differential/controller/DifferentialCommentPreviewController.php index b413b9306f..627d02d2bc 100644 --- a/src/applications/differential/controller/DifferentialCommentPreviewController.php +++ b/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())); diff --git a/src/applications/differential/controller/DifferentialCommentSaveController.php b/src/applications/differential/controller/DifferentialCommentSaveController.php index 6e888947a9..89eed0e093 100644 --- a/src/applications/differential/controller/DifferentialCommentSaveController.php +++ b/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()); diff --git a/src/applications/differential/controller/DifferentialInlineCommentEditController.php b/src/applications/differential/controller/DifferentialInlineCommentEditController.php index 619990b731..1bda9e05d2 100644 --- a/src/applications/differential/controller/DifferentialInlineCommentEditController.php +++ b/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(); + } } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index c0258dfd52..c942fad813 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/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, diff --git a/src/applications/differential/storage/DifferentialDraft.php b/src/applications/differential/storage/DifferentialDraft.php new file mode 100644 index 0000000000..4831d8a5f9 --- /dev/null +++ b/src/applications/differential/storage/DifferentialDraft.php @@ -0,0 +1,51 @@ +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(); + } + } + +} diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php index 928104d9d6..c2dec0d510 100644 --- a/src/applications/differential/storage/DifferentialInlineComment.php +++ b/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) { diff --git a/src/applications/diffusion/controller/DiffusionInlineCommentController.php b/src/applications/diffusion/controller/DiffusionInlineCommentController.php index 5cc0c384d5..78df83292d 100644 --- a/src/applications/diffusion/controller/DiffusionInlineCommentController.php +++ b/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(); + } + } diff --git a/src/applications/draft/storage/PhabricatorDraft.php b/src/applications/draft/storage/PhabricatorDraft.php index 21daa14b12..5b6a6e1aee 100644 --- a/src/applications/draft/storage/PhabricatorDraft.php +++ b/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( diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 22550936cf..780a152799 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/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) - ->save(); + ->setContent($text); + $this->saveComment($inline); return $this->buildRenderedCommentResponse( $inline,