From b82863d972a512da2cee1fc3cada5edb7d7efc3e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Dec 2015 12:25:24 -0800 Subject: [PATCH] Implement versioned drafts in EditEngine comment forms Summary: Ref T9132. Fixes T5031. This approximately implements the plan described in T5031#67988: When we recieve a preview request, don't write a draft if the form is from a version of the object before the last update the viewer made. This should fix the race-related (?) zombie draft comments that sometimes show up. I just added a new object for this stuff to make it easier to do stacked actions (or whatever we end up with) a little later, since I needed to do some schema adjustments anyway. Test Plan: - Typed some text. - Reloaded page. - Draft stayed there. - Tried real hard to get it to ghost by submitting stuff in multiple windows and typing a lot and couldn't, although I didn't bother specifically narrowing down the race condition. Reviewers: chad Reviewed By: chad Maniphest Tasks: T5031, T9132 Differential Revision: https://secure.phabricator.com/D14640 --- .../autopatches/20151202.versioneddraft.1.sql | 10 +++ src/__phutil_library_map__.php | 2 + .../storage/PhabricatorVersionedDraft.php | 83 +++++++++++++++++++ .../editengine/PhabricatorEditEngine.php | 74 +++++++++++++++-- ...catorApplicationTransactionCommentView.php | 31 +++++++ 5 files changed, 193 insertions(+), 7 deletions(-) create mode 100644 resources/sql/autopatches/20151202.versioneddraft.1.sql create mode 100644 src/applications/draft/storage/PhabricatorVersionedDraft.php diff --git a/resources/sql/autopatches/20151202.versioneddraft.1.sql b/resources/sql/autopatches/20151202.versioneddraft.1.sql new file mode 100644 index 0000000000..2ad4332732 --- /dev/null +++ b/resources/sql/autopatches/20151202.versioneddraft.1.sql @@ -0,0 +1,10 @@ +CREATE TABLE {$NAMESPACE}_draft.draft_versioneddraft ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + version INT UNSIGNED NOT NULL, + properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_object` (objectPHID, authorPHID, version) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a67662ba3c..b6d48f60fc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3226,6 +3226,7 @@ phutil_register_library_map(array( 'PhabricatorUsersPolicyRule' => 'applications/people/policyrule/PhabricatorUsersPolicyRule.php', 'PhabricatorUsersSearchField' => 'applications/people/searchfield/PhabricatorUsersSearchField.php', 'PhabricatorVCSResponse' => 'applications/repository/response/PhabricatorVCSResponse.php', + 'PhabricatorVersionedDraft' => 'applications/draft/storage/PhabricatorVersionedDraft.php', 'PhabricatorVeryWowEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorVeryWowEnglishTranslation.php', 'PhabricatorViewerDatasource' => 'applications/people/typeahead/PhabricatorViewerDatasource.php', 'PhabricatorWatcherHasObjectEdgeType' => 'applications/transactions/edges/PhabricatorWatcherHasObjectEdgeType.php', @@ -7536,6 +7537,7 @@ phutil_register_library_map(array( 'PhabricatorUsersPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorUsersSearchField' => 'PhabricatorSearchTokenizerField', 'PhabricatorVCSResponse' => 'AphrontResponse', + 'PhabricatorVersionedDraft' => 'PhabricatorDraftDAO', 'PhabricatorVeryWowEnglishTranslation' => 'PhutilTranslation', 'PhabricatorViewerDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorWatcherHasObjectEdgeType' => 'PhabricatorEdgeType', diff --git a/src/applications/draft/storage/PhabricatorVersionedDraft.php b/src/applications/draft/storage/PhabricatorVersionedDraft.php new file mode 100644 index 0000000000..145f24a3fb --- /dev/null +++ b/src/applications/draft/storage/PhabricatorVersionedDraft.php @@ -0,0 +1,83 @@ + array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'version' => 'uint32', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_object' => array( + 'columns' => array('objectPHID', 'authorPHID', 'version'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public static function loadDraft( + $object_phid, + $viewer_phid) { + + return id(new PhabricatorVersionedDraft())->loadOneWhere( + 'objectPHID = %s AND authorPHID = %s ORDER BY version DESC LIMIT 1', + $object_phid, + $viewer_phid); + } + + public static function loadOrCreateDraft( + $object_phid, + $viewer_phid, + $version) { + + $draft = self::loadDraft($object_phid, $viewer_phid); + if ($draft) { + return $draft; + } + + return id(new PhabricatorVersionedDraft()) + ->setObjectPHID($object_phid) + ->setAuthorPHID($viewer_phid) + ->setVersion($version) + ->save(); + } + + public static function purgeDrafts( + $object_phid, + $viewer_phid, + $version) { + + $draft = new PhabricatorVersionedDraft(); + $conn_w = $draft->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE objectPHID = %s AND authorPHID = %s + AND version <= %d', + $draft->getTableName(), + $object_phid, + $viewer_phid, + $version); + } + +} diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index fb766317a0..47de6224d8 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -879,21 +879,56 @@ abstract class PhabricatorEditEngine $header_text = $this->getCommentViewHeaderText($object); $button_text = $this->getCommentViewButtonText($object); - // TODO: Drafts. - // $draft = PhabricatorDraft::newFromUserAndKey( - // $viewer, - // $object_phid); - $comment_uri = $this->getEditURI($object, 'comment/'); - return id(new PhabricatorApplicationTransactionCommentView()) + $view = id(new PhabricatorApplicationTransactionCommentView()) ->setUser($viewer) ->setObjectPHID($object_phid) ->setHeaderText($header_text) ->setAction($comment_uri) ->setSubmitButtonName($button_text); + + $draft = PhabricatorVersionedDraft::loadDraft( + $object_phid, + $viewer->getPHID()); + if ($draft) { + $view->setVersionedDraft($draft); + } + + $view->setCurrentVersion($this->loadDraftVersion($object)); + + return $view; } + protected function loadDraftVersion($object) { + $viewer = $this->getViewer(); + + if (!$viewer->isLoggedIn()) { + return null; + } + + $template = $object->getApplicationTransactionTemplate(); + $conn_r = $template->establishConnection('r'); + + // Find the most recent transaction the user has written. We'll use this + // as a version number to make sure that out-of-date drafts get discarded. + $result = queryfx_one( + $conn_r, + 'SELECT id AS version FROM %T + WHERE objectPHID = %s AND authorPHID = %s + ORDER BY id DESC LIMIT 1', + $template->getTableName(), + $object->getPHID(), + $viewer->getPHID()); + + if ($result) { + return (int)$result['version']; + } else { + return null; + } + } + + /* -( Responding to HTTP Parameter Requests )------------------------------ */ @@ -970,13 +1005,31 @@ abstract class PhabricatorEditEngine $template = $object->getApplicationTransactionTemplate(); $comment_template = $template->getApplicationTransactionCommentObject(); + $comment_text = $request->getStr('comment'); + + if ($is_preview) { + $version_key = PhabricatorVersionedDraft::KEY_VERSION; + $request_version = $request->getInt($version_key); + $current_version = $this->loadDraftVersion($object); + if ($request_version >= $current_version) { + $draft = PhabricatorVersionedDraft::loadOrCreateDraft( + $object->getPHID(), + $viewer->getPHID(), + $current_version); + + // TODO: This is just a proof of concept. + $draft->setProperty('temporary.comment', $comment_text); + $draft->save(); + } + } + $xactions = array(); $xactions[] = id(clone $template) ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) ->attachComment( id(clone $comment_template) - ->setContent($request->getStr('comment'))); + ->setContent($comment_text)); $editor = $object->getApplicationTransactionEditor() ->setActor($viewer) @@ -992,6 +1045,13 @@ abstract class PhabricatorEditEngine ->setException($ex); } + if (!$is_preview) { + PhabricatorVersionedDraft::purgeDrafts( + $object->getPHID(), + $viewer->getPHID(), + $this->loadDraftVersion($object)); + } + if ($request->isAjax() && $is_preview) { return id(new PhabricatorApplicationTransactionResponse()) ->setViewer($viewer) diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index cbc3739cfb..8e46f335f8 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -20,6 +20,9 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { private $objectPHID; private $headerText; + private $currentVersion; + private $versionedDraft; + public function setObjectPHID($object_phid) { $this->objectPHID = $object_phid; return $this; @@ -46,6 +49,25 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { return $this->requestURI; } + public function setCurrentVersion($current_version) { + $this->currentVersion = $current_version; + return $this; + } + + public function getCurrentVersion() { + return $this->currentVersion; + } + + public function setVersionedDraft( + PhabricatorVersionedDraft $versioned_draft) { + $this->versionedDraft = $versioned_draft; + return $this; + } + + public function getVersionedDraft() { + return $this->versionedDraft; + } + public function setDraft(PhabricatorDraft $draft) { $this->draft = $draft; return $this; @@ -148,10 +170,18 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { $draft_key = $this->getDraft()->getDraftKey(); } + $versioned_draft = $this->getVersionedDraft(); + if ($versioned_draft) { + $draft_comment = $versioned_draft->getProperty('temporary.comment', ''); + } + if (!$this->getObjectPHID()) { throw new PhutilInvalidStateException('setObjectPHID', 'render'); } + $version_key = PhabricatorVersionedDraft::KEY_VERSION; + $version_value = $this->getCurrentVersion(); + return id(new AphrontFormView()) ->setUser($this->getUser()) ->addSigil('transaction-append') @@ -163,6 +193,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { ->setAction($this->getAction()) ->setID($this->getFormID()) ->addHiddenInput('__draft__', $draft_key) + ->addHiddenInput($version_key, $version_value) ->appendChild( id(new PhabricatorRemarkupControl()) ->setID($this->getCommentID())