From bd7420c4bb7e8557f0a9c0232b4ca97d4c3d821b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 4 May 2014 11:11:46 -0700 Subject: [PATCH] Allow pastes to be edited Summary: Fixes T4814. Test Plan: Edited pastes from the web UI. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4814 Differential Revision: https://secure.phabricator.com/D8970 --- .../sql/patches/20130801.pastexactions.php | 2 +- .../ConduitAPI_paste_create_Method.php | 2 +- .../PhabricatorPasteEditController.php | 56 +++++-------- .../paste/editor/PhabricatorPasteEditor.php | 20 +++-- .../paste/mail/PasteCreateMailReceiver.php | 2 +- .../paste/query/PhabricatorPasteQuery.php | 8 +- .../storage/PhabricatorPasteTransaction.php | 84 +++++++++++++++---- .../PhabricatorApplicationTransaction.php | 5 +- 8 files changed, 115 insertions(+), 64 deletions(-) diff --git a/resources/sql/patches/20130801.pastexactions.php b/resources/sql/patches/20130801.pastexactions.php index b04edddda5..1f48d99784 100644 --- a/resources/sql/patches/20130801.pastexactions.php +++ b/resources/sql/patches/20130801.pastexactions.php @@ -30,7 +30,7 @@ foreach ($rows as $row) { $row['phid'], 'public', $row['authorPHID'], - PhabricatorPasteTransaction::TYPE_CREATE, + PhabricatorPasteTransaction::TYPE_CONTENT, 'null', $row['filePHID'], PhabricatorContentSource::newForSource( diff --git a/src/applications/paste/conduit/ConduitAPI_paste_create_Method.php b/src/applications/paste/conduit/ConduitAPI_paste_create_Method.php index a2d08ecd24..f09f650db1 100644 --- a/src/applications/paste/conduit/ConduitAPI_paste_create_Method.php +++ b/src/applications/paste/conduit/ConduitAPI_paste_create_Method.php @@ -51,7 +51,7 @@ final class ConduitAPI_paste_create_Method extends ConduitAPI_paste_Method { $xactions = array(); $xactions[] = id(new PhabricatorPasteTransaction()) - ->setTransactionType(PhabricatorPasteTransaction::TYPE_CREATE) + ->setTransactionType(PhabricatorPasteTransaction::TYPE_CONTENT) ->setNewValue($file->getPHID()); $xactions[] = id(new PhabricatorPasteTransaction()) diff --git a/src/applications/paste/controller/PhabricatorPasteEditController.php b/src/applications/paste/controller/PhabricatorPasteEditController.php index 61dd106af1..76df630b20 100644 --- a/src/applications/paste/controller/PhabricatorPasteEditController.php +++ b/src/applications/paste/controller/PhabricatorPasteEditController.php @@ -42,6 +42,7 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { } $paste->setAuthorPHID($user->getPHID()); + $paste->attachRawContent(''); } else { $is_create = false; @@ -53,6 +54,7 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { PhabricatorPolicyCapability::CAN_EDIT, )) ->withIDs(array($this->id)) + ->needRawContent(true) ->executeOne(); if (!$paste) { return new Aphront404Response(); @@ -69,22 +71,20 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { } else { $v_title = $paste->getTitle(); $v_language = $paste->getLanguage(); - $v_text = ''; + $v_text = $paste->getRawContent(); } $v_policy = $paste->getViewPolicy(); if ($request->isFormPost()) { $xactions = array(); - if ($is_create) { - $v_text = $request->getStr('text'); - if (!strlen($v_text)) { - $e_text = pht('Required'); - $errors[] = pht('The paste may not be blank.'); - } else { - $e_text = null; - } - } + $v_text = $request->getStr('text'); + if (!strlen($v_text)) { + $e_text = pht('Required'); + $errors[] = pht('The paste may not be blank.'); + } else { + $e_text = null; + } $v_title = $request->getStr('title'); $v_language = $request->getStr('language'); @@ -94,14 +94,14 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { // so it's impossible for them to choose an invalid policy. if (!$errors) { - if ($is_create) { + if ($is_create || ($v_text !== $paste->getRawContent())) { $file = PhabricatorPasteEditor::initializeFileForPaste( $user, $v_title, $v_text); $xactions[] = id(new PhabricatorPasteTransaction()) - ->setTransactionType(PhabricatorPasteTransaction::TYPE_CREATE) + ->setTransactionType(PhabricatorPasteTransaction::TYPE_CONTENT) ->setNewValue($file->getPHID()); } @@ -161,31 +161,15 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { ->setPolicies($policies) ->setName('can_view')); - if ($is_create) { - $form - ->appendChild( - id(new AphrontFormTextAreaControl()) - ->setLabel(pht('Text')) - ->setError($e_text) - ->setValue($v_text) - ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) - ->setCustomClass('PhabricatorMonospaced') - ->setName('text')); - } else { - $fork_link = phutil_tag( - 'a', - array( - 'href' => $this->getApplicationURI('?parent='.$paste->getID()) - ), - pht('Fork')); - $form - ->appendChild( - id(new AphrontFormMarkupControl()) + $form + ->appendChild( + id(new AphrontFormTextAreaControl()) ->setLabel(pht('Text')) - ->setValue(pht( - 'Paste text can not be edited. %s to create a new paste.', - $fork_link))); - } + ->setError($e_text) + ->setValue($v_text) + ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) + ->setCustomClass('PhabricatorMonospaced') + ->setName('text')); $submit = new AphrontFormSubmitControl(); diff --git a/src/applications/paste/editor/PhabricatorPasteEditor.php b/src/applications/paste/editor/PhabricatorPasteEditor.php index 599cdd4d4b..1c9eded342 100644 --- a/src/applications/paste/editor/PhabricatorPasteEditor.php +++ b/src/applications/paste/editor/PhabricatorPasteEditor.php @@ -23,7 +23,7 @@ final class PhabricatorPasteEditor public function getTransactionTypes() { $types = parent::getTransactionTypes(); - $types[] = PhabricatorPasteTransaction::TYPE_CREATE; + $types[] = PhabricatorPasteTransaction::TYPE_CONTENT; $types[] = PhabricatorPasteTransaction::TYPE_TITLE; $types[] = PhabricatorPasteTransaction::TYPE_LANGUAGE; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; @@ -37,8 +37,8 @@ final class PhabricatorPasteEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: - return null; + case PhabricatorPasteTransaction::TYPE_CONTENT: + return $object->getFilePHID(); case PhabricatorPasteTransaction::TYPE_TITLE: return $object->getTitle(); case PhabricatorPasteTransaction::TYPE_LANGUAGE: @@ -51,7 +51,7 @@ final class PhabricatorPasteEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case PhabricatorPasteTransaction::TYPE_CONTENT: case PhabricatorPasteTransaction::TYPE_TITLE: case PhabricatorPasteTransaction::TYPE_LANGUAGE: return $xaction->getNewValue(); @@ -63,7 +63,7 @@ final class PhabricatorPasteEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case PhabricatorPasteTransaction::TYPE_CONTENT: $object->setFilePHID($xaction->getNewValue()); return; case PhabricatorPasteTransaction::TYPE_TITLE: @@ -72,6 +72,9 @@ final class PhabricatorPasteEditor case PhabricatorPasteTransaction::TYPE_LANGUAGE: $object->setLanguage($xaction->getNewValue()); return; + case PhabricatorTransactions::TYPE_VIEW_POLICY: + $object->setViewPolicy($xaction->getNewValue()); + return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -82,9 +85,10 @@ final class PhabricatorPasteEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case PhabricatorPasteTransaction::TYPE_CONTENT: case PhabricatorPasteTransaction::TYPE_TITLE: case PhabricatorPasteTransaction::TYPE_LANGUAGE: + case PhabricatorTransactions::TYPE_VIEW_POLICY: return; } @@ -96,7 +100,7 @@ final class PhabricatorPasteEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case PhabricatorPasteTransaction::TYPE_CONTENT: return array($xaction->getNewValue()); } @@ -108,7 +112,7 @@ final class PhabricatorPasteEditor array $xactions) { foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case PhabricatorPasteTransaction::TYPE_CONTENT: return false; default: break; diff --git a/src/applications/paste/mail/PasteCreateMailReceiver.php b/src/applications/paste/mail/PasteCreateMailReceiver.php index 885c518990..5693a0e768 100644 --- a/src/applications/paste/mail/PasteCreateMailReceiver.php +++ b/src/applications/paste/mail/PasteCreateMailReceiver.php @@ -44,7 +44,7 @@ final class PasteCreateMailReceiver $xactions = array(); $xactions[] = id(new PhabricatorPasteTransaction()) - ->setTransactionType(PhabricatorPasteTransaction::TYPE_CREATE) + ->setTransactionType(PhabricatorPasteTransaction::TYPE_CONTENT) ->setNewValue($file->getPHID()); $xactions[] = id(new PhabricatorPasteTransaction()) diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index c4db19e1ff..a1b88d1102 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -157,7 +157,13 @@ final class PhabricatorPasteQuery } private function getContentCacheKey(PhabricatorPaste $paste) { - return 'P'.$paste->getID().':content/'.$paste->getLanguage(); + return implode( + ':', + array( + 'P'.$paste->getID(), + $paste->getFilePHID(), + $paste->getLanguage(), + )); } private function loadRawContent(array $pastes) { diff --git a/src/applications/paste/storage/PhabricatorPasteTransaction.php b/src/applications/paste/storage/PhabricatorPasteTransaction.php index a62f6be5f2..acb06b22ac 100644 --- a/src/applications/paste/storage/PhabricatorPasteTransaction.php +++ b/src/applications/paste/storage/PhabricatorPasteTransaction.php @@ -3,7 +3,7 @@ final class PhabricatorPasteTransaction extends PhabricatorApplicationTransaction { - const TYPE_CREATE = 'paste.create'; + const TYPE_CONTENT = 'paste.create'; const TYPE_TITLE = 'paste.title'; const TYPE_LANGUAGE = 'paste.language'; @@ -23,7 +23,7 @@ final class PhabricatorPasteTransaction $phids = parent::getRequiredHandlePHIDs(); switch ($this->getTransactionType()) { - case self::TYPE_CREATE: + case self::TYPE_CONTENT: $phids[] = $this->getObjectPHID(); break; } @@ -36,14 +36,14 @@ final class PhabricatorPasteTransaction switch ($this->getTransactionType()) { case self::TYPE_TITLE: case self::TYPE_LANGUAGE: - return $old === null; + return ($old === null); } return parent::shouldHide(); } public function getIcon() { switch ($this->getTransactionType()) { - case self::TYPE_CREATE: + case self::TYPE_CONTENT: return 'fa-plus'; break; case self::TYPE_TITLE: @@ -63,11 +63,16 @@ final class PhabricatorPasteTransaction $type = $this->getTransactionType(); switch ($type) { - case PhabricatorPasteTransaction::TYPE_CREATE: - return pht( - '%s created "%s".', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid)); + case PhabricatorPasteTransaction::TYPE_CONTENT: + if ($old === null) { + return pht( + '%s created this paste.', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s edited the content of this paste.', + $this->renderHandleLink($author_phid)); + } break; case PhabricatorPasteTransaction::TYPE_TITLE: return pht( @@ -94,11 +99,18 @@ final class PhabricatorPasteTransaction $type = $this->getTransactionType(); switch ($type) { - case PhabricatorPasteTransaction::TYPE_CREATE: - return pht( - '%s created %s.', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid)); + case PhabricatorPasteTransaction::TYPE_CONTENT: + if ($old === null) { + return pht( + '%s created %s.', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid)); + } else { + return pht( + '%s edited %s.', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid)); + } break; case PhabricatorPasteTransaction::TYPE_TITLE: return pht( @@ -122,10 +134,52 @@ final class PhabricatorPasteTransaction $new = $this->getNewValue(); switch ($this->getTransactionType()) { - case PhabricatorPasteTransaction::TYPE_CREATE: + case self::TYPE_CONTENT: return PhabricatorTransactions::COLOR_GREEN; } return parent::getColor(); } + + + public function hasChangeDetails() { + switch ($this->getTransactionType()) { + case self::TYPE_CONTENT: + return ($this->getOldValue() !== null); + } + + return parent::hasChangeDetails(); + } + + public function renderChangeDetails(PhabricatorUser $viewer) { + switch ($this->getTransactionType()) { + case self::TYPE_CONTENT: + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $files = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array_filter(array($old, $new))) + ->execute(); + $files = mpull($files, null, 'getPHID'); + + $old_text = ''; + if (idx($files, $old)) { + $old_text = $files[$old]->loadFileData(); + } + + $new_text = ''; + if (idx($files, $new)) { + $new_text = $files[$new]->loadFileData(); + } + + return $this->renderTextCorpusChangeDetails( + $viewer, + $old_text, + $new_text); + } + + return parent::renderChangeDetails($viewer); + } + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index d074f3f799..aa0f24ab7b 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -822,7 +822,10 @@ abstract class PhabricatorApplicationTransaction break; } - return $this->renderTextCorpusChangeDetails(); + return $this->renderTextCorpusChangeDetails( + $viewer, + $this->getOldValue(), + $this->getNewValue()); } public function renderTextCorpusChangeDetails(