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

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;

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',

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);
+      $xaction->setNewValue(
-      return array($e_value, $errors, $value);
+        array(
-    } else {
+          'deleted' => true,
-      $entry->setIsDeleted(false);
+          '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(

src/applications/config/editor/PhabricatorConfigEditor.php

@@ -0,0 +1,90 @@
+<?php
+
+final class PhabricatorConfigEditor
+  extends PhabricatorApplicationTransactionEditor {
+
+  public function getTransactionTypes() {
+    $types = parent::getTransactionTypes();
+
+    $types[] = PhabricatorConfigTransaction::TYPE_EDIT;
+
+    return $types;
+  }
+
+  protected function getCustomTransactionOldValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->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);
+  }
+
+}

src/applications/config/query/PhabricatorConfigTransactionQuery.php

@@ -0,0 +1,10 @@
+<?php
+
+final class PhabricatorConfigTransactionQuery
+  extends PhabricatorApplicationTransactionQuery {
+
+  protected function getTemplateApplicationTransaction() {
+    return new PhabricatorConfigTransaction();
+  }
+
+}

src/applications/config/storage/PhabricatorConfigTransaction.php

@@ -0,0 +1,94 @@
+<?php
+
+final class PhabricatorConfigTransaction
+  extends PhabricatorApplicationTransaction {
+
+  const TYPE_EDIT = 'config:edit';
+
+  public function getApplicationName() {
+    return 'config';
+  }
+
+  public function getApplicationTransactionType() {
+    return PhabricatorPHIDConstants::PHID_TYPE_CONF;
+  }
+
+  public function getApplicationTransactionCommentObject() {
+    return null;
+  }
+
+  public function getApplicationObjectTypeName() {
+    return pht('config');
+  }
+
+
+  public function getTitle() {
+    $author_phid = $this->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;
+    }
+  }
+
+}
+

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;

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;
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -58,9 +58,7 @@ abstract class PhabricatorApplicationTransactionEditor
   }
 
   public function getTransactionTypes() {
-    $types = array(
+    $types = array();
-      PhabricatorTransactions::TYPE_COMMENT,
-    );
 
     if ($this->object instanceof PhabricatorSubscribableInterface) {
       $types[] = PhabricatorTransactions::TYPE_SUBSCRIBERS;

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'),
+      ),
     );
   }