From 32e4a7a37f1fd1820ff7032b3a4689e6eb2d97b5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Jan 2013 18:14:41 -0800 Subject: [PATCH] Use transactions to show edit history for Configuration Summary: Use ApplicationTransactions in Config to create an edit history. Resolves T2256. Test Plan: {F28477} Reviewers: btrahan, codeblock Reviewed By: codeblock CC: aran Maniphest Tasks: T2256 Differential Revision: https://secure.phabricator.com/D4314 --- .../sql/patches/20130101.confxaction.sql | 20 ++++ src/__phutil_library_map__.php | 6 ++ .../PhabricatorConfigEditController.php | 68 +++++++++++--- .../config/editor/PhabricatorConfigEditor.php | 90 ++++++++++++++++++ .../PhabricatorConfigTransactionQuery.php | 10 ++ .../storage/PhabricatorConfigTransaction.php | 94 +++++++++++++++++++ .../macro/editor/PhabricatorMacroEditor.php | 1 + .../pholio/editor/PholioMockEditor.php | 1 + ...habricatorApplicationTransactionEditor.php | 4 +- .../patch/PhabricatorBuiltinPatchList.php | 4 + 10 files changed, 281 insertions(+), 17 deletions(-) create mode 100644 resources/sql/patches/20130101.confxaction.sql create mode 100644 src/applications/config/editor/PhabricatorConfigEditor.php create mode 100644 src/applications/config/query/PhabricatorConfigTransactionQuery.php create mode 100644 src/applications/config/storage/PhabricatorConfigTransaction.php diff --git a/resources/sql/patches/20130101.confxaction.sql b/resources/sql/patches/20130101.confxaction.sql new file mode 100644 index 0000000000..81105d961b --- /dev/null +++ b/resources/sql/patches/20130101.confxaction.sql @@ -0,0 +1,20 @@ +CREATE TABLE {$NAMESPACE}_config.config_transaction ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + phid VARCHAR(64) NOT NULL COLLATE utf8_bin, + authorPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + viewPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + editPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + commentPHID VARCHAR(64) COLLATE utf8_bin, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) NOT NULL COLLATE utf8_bin, + oldValue LONGTEXT NOT NULL COLLATE utf8_bin, + newValue LONGTEXT NOT NULL COLLATE utf8_bin, + contentSource LONGTEXT NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + + UNIQUE KEY `key_phid` (phid), + KEY `key_object` (objectPHID) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b05d327542..4b563f0277 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -687,6 +687,7 @@ phutil_register_library_map(array( 'PhabricatorConfigDefaultSource' => 'infrastructure/env/PhabricatorConfigDefaultSource.php', 'PhabricatorConfigDictionarySource' => 'infrastructure/env/PhabricatorConfigDictionarySource.php', 'PhabricatorConfigEditController' => 'applications/config/controller/PhabricatorConfigEditController.php', + 'PhabricatorConfigEditor' => 'applications/config/editor/PhabricatorConfigEditor.php', 'PhabricatorConfigEntry' => 'applications/config/storage/PhabricatorConfigEntry.php', 'PhabricatorConfigEntryDAO' => 'applications/config/storage/PhabricatorConfigEntryDAO.php', 'PhabricatorConfigFileSource' => 'infrastructure/env/PhabricatorConfigFileSource.php', @@ -701,6 +702,8 @@ phutil_register_library_map(array( 'PhabricatorConfigProxySource' => 'infrastructure/env/PhabricatorConfigProxySource.php', 'PhabricatorConfigSource' => 'infrastructure/env/PhabricatorConfigSource.php', 'PhabricatorConfigStackSource' => 'infrastructure/env/PhabricatorConfigStackSource.php', + 'PhabricatorConfigTransaction' => 'applications/config/storage/PhabricatorConfigTransaction.php', + 'PhabricatorConfigTransactionQuery' => 'applications/config/query/PhabricatorConfigTransactionQuery.php', 'PhabricatorContentSource' => 'applications/metamta/contentsource/PhabricatorContentSource.php', 'PhabricatorContentSourceView' => 'applications/metamta/contentsource/PhabricatorContentSourceView.php', 'PhabricatorController' => 'applications/base/controller/PhabricatorController.php', @@ -2023,6 +2026,7 @@ phutil_register_library_map(array( 'PhabricatorConfigDefaultSource' => 'PhabricatorConfigProxySource', 'PhabricatorConfigDictionarySource' => 'PhabricatorConfigSource', 'PhabricatorConfigEditController' => 'PhabricatorConfigController', + 'PhabricatorConfigEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorConfigEntry' => 'PhabricatorConfigEntryDAO', 'PhabricatorConfigEntryDAO' => 'PhabricatorLiskDAO', 'PhabricatorConfigFileSource' => 'PhabricatorConfigProxySource', @@ -2040,6 +2044,8 @@ phutil_register_library_map(array( ), 'PhabricatorConfigProxySource' => 'PhabricatorConfigSource', 'PhabricatorConfigStackSource' => 'PhabricatorConfigSource', + 'PhabricatorConfigTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorContentSourceView' => 'AphrontView', 'PhabricatorController' => 'AphrontController', 'PhabricatorCountdownController' => 'PhabricatorController', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index caeb4e0205..4106a0b25e 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -61,15 +61,26 @@ final class PhabricatorConfigEditController $errors = array(); if ($request->isFormPost()) { - list($e_value, $value_errors, $display_value) = $this->readRequest( + $result = $this->readRequest( $option, - $config_entry, $request); + list($e_value, $value_errors, $display_value, $xaction) = $result; $errors = array_merge($errors, $value_errors); if (!$errors) { - $config_entry->save(); + + $editor = id(new PhabricatorConfigEditor()) + ->setActor($user) + ->setContinueOnNoEffect(true) + ->setContentSource( + PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_WEB, + array( + 'ip' => $request->getRemoteAddr(), + ))) + ->applyTransactions($config_entry, array($xaction)); + return id(new AphrontRedirectResponse())->setURI($done_uri); } } else { @@ -148,12 +159,23 @@ final class PhabricatorConfigEditController ->setName($this->key) ->setHref('/config/edit/'.$this->key)); + + $xactions = id(new PhabricatorConfigTransactionQuery()) + ->withObjectPHIDs(array($config_entry->getPHID())) + ->setViewer($user) + ->execute(); + + $xaction_view = id(new PhabricatorApplicationTransactionView()) + ->setUser($user) + ->setTransactions($xactions); + return $this->buildApplicationPage( array( $crumbs, id(new PhabricatorHeaderView())->setHeader($title), $error_view, $form, + $xaction_view, ), array( 'title' => $title, @@ -163,41 +185,49 @@ final class PhabricatorConfigEditController private function readRequest( PhabricatorConfigOption $option, - PhabricatorConfigEntry $entry, AphrontRequest $request) { + $xaction = new PhabricatorConfigTransaction(); + $xaction->setTransactionType(PhabricatorConfigTransaction::TYPE_EDIT); + $e_value = null; $errors = array(); $value = $request->getStr('value'); if (!strlen($value)) { $value = null; - $entry->setValue($value); - $entry->setIsDeleted(true); - return array($e_value, $errors, $value); - } else { - $entry->setIsDeleted(false); + + $xaction->setNewValue( + array( + 'deleted' => true, + 'value' => null, + )); + + return array($e_value, $errors, $value, $xaction); } $type = $option->getType(); + $set_value = null; + switch ($type) { case 'int': if (preg_match('/^-?[0-9]+$/', trim($value))) { - $entry->setValue((int)$value); + $set_value = (int)$value; } else { $e_value = pht('Invalid'); $errors[] = pht('Value must be an integer.'); } break; case 'string': + $set_value = (string)$value; break; case 'bool': switch ($value) { case 'true': - $entry->setValue(true); + $set_value = true; break; case 'false': - $entry->setValue(false); + $set_value = false; break; default: $e_value = pht('Invalid'); @@ -212,12 +242,22 @@ final class PhabricatorConfigEditController $errors[] = pht( 'The given value must be valid JSON. This means, among '. 'other things, that you must wrap strings in double-quotes.'); - $entry->setValue($json); + $set_value = $json; } break; } - return array($e_value, $errors, $value); + if (!$errors) { + $xaction->setNewValue( + array( + 'deleted' => false, + 'value' => $set_value, + )); + } else { + $xaction = null; + } + + return array($e_value, $errors, $value, $xaction); } private function getDisplayValue( diff --git a/src/applications/config/editor/PhabricatorConfigEditor.php b/src/applications/config/editor/PhabricatorConfigEditor.php new file mode 100644 index 0000000000..667ae6e237 --- /dev/null +++ b/src/applications/config/editor/PhabricatorConfigEditor.php @@ -0,0 +1,90 @@ +getTransactionType()) { + case PhabricatorConfigTransaction::TYPE_EDIT: + return array( + 'deleted' => (bool)$object->getIsDeleted(), + 'value' => $object->getValue(), + ); + } + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorConfigTransaction::TYPE_EDIT: + return $xaction->getNewValue(); + } + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorConfigTransaction::TYPE_EDIT: + $v = $xaction->getNewValue(); + + $object->setIsDeleted($v['deleted']); + $object->setValue($v['value']); + break; + } + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + return; + } + + protected function mergeTransactions( + PhabricatorApplicationTransaction $u, + PhabricatorApplicationTransaction $v) { + + $type = $u->getTransactionType(); + switch ($type) { + case PhabricatorConfigTransaction::TYPE_EDIT: + return $v; + } + + return parent::mergeTransactions($u, $v); + } + + protected function transactionHasEffect( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + $type = $xaction->getTransactionType(); + switch ($type) { + case PhabricatorConfigTransaction::TYPE_EDIT: + // If an edit deletes an already-deleted entry, no-op it. + if (idx($old, 'deleted') && idx($new, 'deleted')) { + return false; + } + break; + } + + return parent::transactionHasEffect($object, $xaction); + } + +} diff --git a/src/applications/config/query/PhabricatorConfigTransactionQuery.php b/src/applications/config/query/PhabricatorConfigTransactionQuery.php new file mode 100644 index 0000000000..817096eaa1 --- /dev/null +++ b/src/applications/config/query/PhabricatorConfigTransactionQuery.php @@ -0,0 +1,10 @@ +getAuthorPHID(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case self::TYPE_EDIT: + + // TODO: After T2213 show the actual values too; for now, we don't + // have the tools to do it without making a bit of a mess of it. + + $old_del = idx($old, 'deleted'); + $new_del = idx($new, 'deleted'); + if ($old_del && !$new_del) { + return pht( + '%s created this configuration entry.', + $this->renderHandleLink($author_phid)); + } else if (!$old_del && $new_del) { + return pht( + '%s deleted this configuration entry.', + $this->renderHandleLink($author_phid)); + } else if ($old_del && $new_del) { + // This is a bug. + return pht( + '%s deleted this configuration entry (again?).', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s edited this configuration entry.', + $this->renderHandleLink($author_phid)); + } + break; + } + + return parent::getTitle(); + } + + + public function getIcon() { + switch ($this->getTransactionType()) { + case self::TYPE_EDIT: + return 'edit'; + } + + return parent::getIcon(); + } + + public function getColor() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case self::TYPE_EDIT: + $old_del = idx($old, 'deleted'); + $new_del = idx($new, 'deleted'); + + if ($old_del && !$new_del) { + return PhabricatorTransactions::COLOR_GREEN; + } else if (!$old_del && $new_del) { + return PhabricatorTransactions::COLOR_RED; + } else { + return PhabricatorTransactions::COLOR_BLUE; + } + break; + } + } + +} + diff --git a/src/applications/macro/editor/PhabricatorMacroEditor.php b/src/applications/macro/editor/PhabricatorMacroEditor.php index e10046f8cc..93d7af6d4e 100644 --- a/src/applications/macro/editor/PhabricatorMacroEditor.php +++ b/src/applications/macro/editor/PhabricatorMacroEditor.php @@ -6,6 +6,7 @@ final class PhabricatorMacroEditor public function getTransactionTypes() { $types = parent::getTransactionTypes(); + $types[] = PhabricatorTransactions::TYPE_COMMENT; $types[] = PhabricatorMacroTransactionType::TYPE_NAME; $types[] = PhabricatorMacroTransactionType::TYPE_DISABLED; $types[] = PhabricatorMacroTransactionType::TYPE_FILE; diff --git a/src/applications/pholio/editor/PholioMockEditor.php b/src/applications/pholio/editor/PholioMockEditor.php index 770dcc84f9..36ab5a847a 100644 --- a/src/applications/pholio/editor/PholioMockEditor.php +++ b/src/applications/pholio/editor/PholioMockEditor.php @@ -8,6 +8,7 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { public function getTransactionTypes() { $types = parent::getTransactionTypes(); + $types[] = PhabricatorTransactions::TYPE_COMMENT; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index e5ccdebc8c..c9510f636d 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -58,9 +58,7 @@ abstract class PhabricatorApplicationTransactionEditor } public function getTransactionTypes() { - $types = array( - PhabricatorTransactions::TYPE_COMMENT, - ); + $types = array(); if ($this->object instanceof PhabricatorSubscribableInterface) { $types[] = PhabricatorTransactions::TYPE_SUBSCRIBERS; diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 3c38dc6514..cc909fc4b1 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1072,6 +1072,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20121226.config.sql'), ), + '20130101.confxaction.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130101.confxaction.sql'), + ), ); }