From 3b1294cf4599a86aa4789cb0618e004e4210b521 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Aug 2018 09:53:18 -0700 Subject: [PATCH] Store Phriction max version on Document, improve editing rules for editing documents with drafts Summary: Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object. Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version. Test Plan: - Ran migration. - Edited a draft page without hitting any weird version errors. - Checked database for sensible `maxVersion` values. Reviewers: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13077 Differential Revision: https://secure.phabricator.com/D19625 --- .../20180830.phriction.01.maxversion.sql | 2 ++ .../20180830.phriction.02.maxes.php | 30 +++++++++++++++++++ .../PhrictionDocumentController.php | 7 +---- .../controller/PhrictionEditController.php | 26 ++++++++++------ .../editor/PhrictionTransactionEditor.php | 5 ++-- .../phriction/storage/PhrictionDocument.php | 3 ++ 6 files changed, 56 insertions(+), 17 deletions(-) create mode 100644 resources/sql/autopatches/20180830.phriction.01.maxversion.sql create mode 100644 resources/sql/autopatches/20180830.phriction.02.maxes.php diff --git a/resources/sql/autopatches/20180830.phriction.01.maxversion.sql b/resources/sql/autopatches/20180830.phriction.01.maxversion.sql new file mode 100644 index 0000000000..f6f24e8333 --- /dev/null +++ b/resources/sql/autopatches/20180830.phriction.01.maxversion.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phriction.phriction_document + ADD maxVersion INT UNSIGNED NOT NULL; diff --git a/resources/sql/autopatches/20180830.phriction.02.maxes.php b/resources/sql/autopatches/20180830.phriction.02.maxes.php new file mode 100644 index 0000000000..97abf010db --- /dev/null +++ b/resources/sql/autopatches/20180830.phriction.02.maxes.php @@ -0,0 +1,30 @@ +establishConnection('w'); + +$iterator = new LiskRawMigrationIterator( + $conn, + $document_table->getTableName()); +foreach ($iterator as $row) { + $content = queryfx_one( + $conn, + 'SELECT MAX(version) max FROM %T WHERE documentPHID = %s', + $content_table->getTableName(), + $row['phid']); + if (!$content) { + continue; + } + + queryfx( + $conn, + 'UPDATE %T SET maxVersion = %d WHERE id = %d', + $document_table->getTableName(), + $content['max'], + $row['id']); +} diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 6e9edc1346..0a4dfdd070 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -66,12 +66,7 @@ final class PhrictionDocumentController ->addAction($create_button); } else { - $draft_content = id(new PhrictionContentQuery()) - ->setViewer($viewer) - ->withDocumentPHIDs(array($document->getPHID())) - ->setLimit(1) - ->executeOne(); - $max_version = (int)$draft_content->getVersion(); + $max_version = (int)$document->getMaxVersion(); $version = $request->getInt('v'); if ($version) { diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index 675b1cd630..2c47187007 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -7,7 +7,7 @@ final class PhrictionEditController $viewer = $request->getViewer(); $id = $request->getURIData('id'); - $current_version = null; + $max_version = null; if ($id) { $is_new = false; $document = id(new PhrictionDocumentQuery()) @@ -24,7 +24,7 @@ final class PhrictionEditController return new Aphront404Response(); } - $current_version = $document->getContent()->getVersion(); + $max_version = $document->getMaxVersion(); $revert = $request->getInt('revert'); if ($revert) { @@ -37,9 +37,12 @@ final class PhrictionEditController return new Aphront404Response(); } } else { - $content = $document->getContent(); + $content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withDocumentPHIDs(array($document->getPHID())) + ->setLimit(1) + ->executeOne(); } - } else { $slug = $request->getStr('slug'); $slug = PhabricatorSlug::normalize($slug); @@ -54,8 +57,13 @@ final class PhrictionEditController ->executeOne(); if ($document) { - $content = $document->getContent(); - $current_version = $content->getVersion(); + $content = id(new PhrictionContentQuery()) + ->setViewer($viewer) + ->withDocumentPHIDs(array($document->getPHID())) + ->setLimit(1) + ->executeOne(); + + $max_version = $document->getMaxVersion(); $is_new = false; } else { $document = PhrictionDocument::initializeNewDocument($viewer, $slug); @@ -128,7 +136,7 @@ final class PhrictionEditController $title = $request->getStr('title'); $content_text = $request->getStr('content'); $notes = $request->getStr('description'); - $current_version = $request->getInt('contentVersion'); + $max_version = $request->getInt('contentVersion'); $v_view = $request->getStr('viewPolicy'); $v_edit = $request->getStr('editPolicy'); $v_cc = $request->getArr('cc'); @@ -168,7 +176,7 @@ final class PhrictionEditController ->setContinueOnNoEffect(true) ->setDescription($notes) ->setProcessContentVersionError(!$request->getBool('overwrite')) - ->setContentVersion($current_version); + ->setContentVersion($max_version); try { $editor->applyTransactions($document, $xactions); @@ -232,7 +240,7 @@ final class PhrictionEditController ->setUser($viewer) ->addHiddenInput('slug', $document->getSlug()) ->addHiddenInput('nodraft', $request->getBool('nodraft')) - ->addHiddenInput('contentVersion', $current_version) + ->addHiddenInput('contentVersion', $max_version) ->addHiddenInput('overwrite', $overwrite) ->appendChild( id(new AphrontFormTextControl()) diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index d91165ab1d..3c045e16ba 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -468,7 +468,7 @@ final class PhrictionTransactionEditor $error = null; if ($this->getContentVersion() && - ($object->getContent()->getVersion() != $this->getContentVersion())) { + ($object->getMaxVersion() != $this->getContentVersion())) { $error = new PhabricatorApplicationTransactionValidationError( $type, pht('Edit Conflict'), @@ -519,6 +519,7 @@ final class PhrictionTransactionEditor $document->setContentPHID($content_phid); $document->attachContent($content); $document->setEditedEpoch(PhabricatorTime::getNow()); + $document->setMaxVersion($content->getVersion()); $this->newContent = $content; } @@ -539,7 +540,7 @@ final class PhrictionTransactionEditor $content->setDescription($this->getDescription()); } - $content->setVersion($this->getOldContent()->getVersion() + 1); + $content->setVersion($document->getMaxVersion() + 1); return $content; } diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 626d02c1db..b6dcd6d56d 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -23,6 +23,7 @@ final class PhrictionDocument extends PhrictionDAO protected $editPolicy; protected $spacePHID; protected $editedEpoch; + protected $maxVersion; private $contentObject = self::ATTACHABLE; private $ancestors = array(); @@ -36,6 +37,7 @@ final class PhrictionDocument extends PhrictionDAO 'depth' => 'uint32', 'status' => 'text32', 'editedEpoch' => 'epoch', + 'maxVersion' => 'uint32', ), self::CONFIG_KEY_SCHEMA => array( 'slug' => array( @@ -89,6 +91,7 @@ final class PhrictionDocument extends PhrictionDAO } $document->setEditedEpoch(PhabricatorTime::getNow()); + $document->setMaxVersion(0); return $document; }