Use CustomFields in differential.createrevision

Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.

This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.

Test Plan: Used `arc diff --create` to create several revisions with different data.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8452

src/applications/differential/conduit/ConduitAPI_differential_Method.php

@@ -43,5 +43,109 @@ abstract class ConduitAPI_differential_Method extends ConduitAPIMethod {
     );
   }
 
+  protected function applyFieldEdit(
+    ConduitAPIRequest $request,
+    DifferentialRevision $revision,
+    DifferentialDiff $diff,
+    array $fields,
+    $message) {
+
+    $viewer = $request->getUser();
+
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $revision,
+      DifferentialCustomField::ROLE_COMMITMESSAGEEDIT);
+
+    $field_list
+      ->setViewer($viewer)
+      ->readFieldsFromStorage($revision);
+    $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');
+
+    $xactions = array();
+
+    $xactions[] = id(new DifferentialTransaction())
+      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+      ->setNewValue($diff->getPHID());
+
+    $values = $request->getValue('fields', array());
+    foreach ($values as $key => $value) {
+      $field = idx($field_map, $key);
+      if (!$field) {
+        // NOTE: We're just ignoring fields we don't know about. This isn't
+        // ideal, but the way the workflow currently works involves us getting
+        // several read-only fields, like the revision ID field, which we should
+        // just skip.
+        continue;
+      }
+
+      $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS;
+      if (!$field->shouldEnableForRole($role)) {
+        throw new Exception(
+          pht(
+            'Request attempts to update field "%s", but that field can not '.
+            'perform transactional updates.',
+            $key));
+      }
+
+      // TODO: This is fairly similar to PhabricatorCustomField's
+      // buildFieldTransactionsFromRequest() method, but that's currently not
+      // easy to reuse.
+
+      $transaction_type = $field->getApplicationTransactionType();
+      $xaction = id(new DifferentialTransaction())
+        ->setTransactionType($transaction_type);
+
+      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
+        // For TYPE_CUSTOMFIELD transactions only, we provide the old value
+        // as an input.
+        $old_value = $field->getOldValueForApplicationTransactions();
+        $xaction->setOldValue($old_value);
+      }
+
+      // The transaction itself will be validated so this is somewhat
+      // redundant, but this validator will sometimes give us a better error
+      // message or a better reaction to a bad value type.
+      $field->validateCommitMessageValue($value);
+      $field->readValueFromCommitMessage($value);
+
+      $xaction
+        ->setNewValue($field->getNewValueForApplicationTransactions());
+
+      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
+        // For TYPE_CUSTOMFIELD transactions, add the field key in metadata.
+        $xaction->setMetadataValue('customfield:key', $field->getFieldKey());
+      }
+
+      $metadata = $field->getApplicationTransactionMetadata();
+      foreach ($metadata as $meta_key => $meta_value) {
+        $xaction->setMetadataValue($meta_key, $meta_value);
+      }
+
+      $xactions[] = $xaction;
+    }
+
+    $message = $request->getValue('message');
+    if (strlen($message)) {
+      // This is a little awkward, and should maybe move inside the transaction
+      // editor. It largely exists for legacy reasons.
+      $first_line = head(phutil_split_lines($message, false));
+      $diff->setDescription($first_line);
+      $diff->save();
+
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
+        ->attachComment(
+          id(new DifferentialTransactionComment())
+            ->setContent($message));
+    }
+
+    $editor = id(new DifferentialTransactionEditor())
+      ->setActor($viewer)
+      ->setContentSourceFromConduitRequest($request)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true);
+
+    $editor->applyTransactions($revision, $xactions);
+  }
 
 }

src/applications/differential/conduit/ConduitAPI_differential_createrevision_Method.php

@@ -1,13 +1,10 @@
 <?php
 
-/**
- * @group conduit
- */
 final class ConduitAPI_differential_createrevision_Method
-  extends ConduitAPIMethod {
+  extends ConduitAPI_differential_Method {
 
   public function getMethodDescription() {
-    return "Create a new Differential revision.";
+    return pht("Create a new Differential revision.");
   }
 
   public function defineParamTypes() {
@@ -31,20 +28,25 @@ final class ConduitAPI_differential_createrevision_Method
   }
 
   protected function execute(ConduitAPIRequest $request) {
-    $fields = $request->getValue('fields');
+    $viewer = $request->getUser();
 
     $diff = id(new DifferentialDiffQuery())
-      ->setViewer($request->getUser())
+      ->setViewer($viewer)
       ->withIDs(array($request->getValue('diffid')))
       ->executeOne();
     if (!$diff) {
       throw new ConduitException('ERR_BAD_DIFF');
     }
 
-    $revision = DifferentialRevisionEditor::newRevisionFromConduitWithDiff(
+    $revision = DifferentialRevision::initializeNewRevision($viewer);
-      $fields,
+    $revision->attachReviewerStatus(array());
+
+    $this->applyFieldEdit(
+      $request,
+      $revision,
       $diff,
-      $request->getUser());
+      $request->getValue('fields', array()),
+      $message = null);
 
     return array(
       'revisionid'  => $revision->getID(),

src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php

@@ -1,7 +1,7 @@
 <?php
 
 final class ConduitAPI_differential_updaterevision_Method
-  extends ConduitAPIMethod {
+  extends ConduitAPI_differential_Method {
 
   public function getMethodDescription() {
     return pht("Update a Differential revision.");
@@ -59,100 +59,12 @@ final class ConduitAPI_differential_updaterevision_Method
       throw new ConduitException('ERR_CLOSED');
     }
 
-    $field_list = PhabricatorCustomField::getObjectFields(
+    $this->applyFieldEdit(
+      $request,
       $revision,
-      DifferentialCustomField::ROLE_COMMITMESSAGEEDIT);
+      $diff,
-
+      $request->getValue('fields', array()),
-    $field_list
+      $request->getValue('message'));
-      ->setViewer($viewer)
-      ->readFieldsFromStorage($revision);
-    $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');
-
-    $xactions = array();
-
-    $xactions[] = id(new DifferentialTransaction())
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-      ->setNewValue($diff->getPHID());
-
-    $values = $request->getValue('fields', array());
-    foreach ($values as $key => $value) {
-      $field = idx($field_map, $key);
-      if (!$field) {
-        // NOTE: We're just ignoring fields we don't know about. This isn't
-        // ideal, but the way the workflow currently works involves us getting
-        // several read-only fields, like the revision ID field, which we should
-        // just skip.
-        continue;
-      }
-
-      $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS;
-      if (!$field->shouldEnableForRole($role)) {
-        throw new Exception(
-          pht(
-            'Request attempts to update field "%s", but that field can not '.
-            'perform transactional updates.',
-            $key));
-      }
-
-      // TODO: This is fairly similar to PhabricatorCustomField's
-      // buildFieldTransactionsFromRequest() method, but that's currently not
-      // easy to reuse.
-
-      $transaction_type = $field->getApplicationTransactionType();
-      $xaction = id(new DifferentialTransaction())
-        ->setTransactionType($transaction_type);
-
-      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
-        // For TYPE_CUSTOMFIELD transactions only, we provide the old value
-        // as an input.
-        $old_value = $field->getOldValueForApplicationTransactions();
-        $xaction->setOldValue($old_value);
-      }
-
-      // The transaction itself will be validated so this is somewhat
-      // redundant, but this validator will sometimes give us a better error
-      // message or a better reaction to a bad value type.
-      $field->validateCommitMessageValue($value);
-      $field->readValueFromCommitMessage($value);
-
-      $xaction
-        ->setNewValue($field->getNewValueForApplicationTransactions());
-
-      if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) {
-        // For TYPE_CUSTOMFIELD transactions, add the field key in metadata.
-        $xaction->setMetadataValue('customfield:key', $field->getFieldKey());
-      }
-
-      $metadata = $field->getApplicationTransactionMetadata();
-      foreach ($metadata as $meta_key => $meta_value) {
-        $xaction->setMetadataValue($meta_key, $meta_value);
-      }
-
-      $xactions[] = $xaction;
-    }
-
-    $message = $request->getValue('message');
-    if (strlen($message)) {
-      // This is a little awkward, and should maybe move inside the transaction
-      // editor. It largely exists for legacy reasons.
-      $first_line = head(phutil_split_lines($message, false));
-      $diff->setDescription($first_line);
-      $diff->save();
-
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-        ->attachComment(
-          id(new DifferentialTransactionComment())
-            ->setContent($message));
-    }
-
-    $editor = id(new DifferentialTransactionEditor())
-      ->setActor($viewer)
-      ->setContentSourceFromConduitRequest($request)
-      ->setContinueOnNoEffect(true)
-      ->setContinueOnMissingFields(true);
-
-    $editor->applyTransactions($revision, $xactions);
 
     return array(
       'revisionid'  => $revision->getID(),

src/applications/differential/customfield/DifferentialSummaryField.php

@@ -21,6 +21,9 @@ final class DifferentialSummaryField
 
   protected function readValueFromRevision(
     DifferentialRevision $revision) {
+    if (!$revision->getID()) {
+      return null;
+    }
     return $revision->getSummary();
   }
 
@@ -82,6 +85,11 @@ final class DifferentialSummaryField
       $xaction->getNewValue());
   }
 
+  public function shouldHideInApplicationTransactions(
+    PhabricatorApplicationTransaction $xaction) {
+    return ($xaction->getOldValue() === null);
+  }
+
   public function shouldAppearInGlobalSearch() {
     return true;
   }

src/applications/differential/customfield/DifferentialTestPlanField.php

@@ -21,6 +21,9 @@ final class DifferentialTestPlanField
 
   protected function readValueFromRevision(
     DifferentialRevision $revision) {
+    if (!$revision->getID()) {
+      return null;
+    }
     return $revision->getTestPlan();
   }
 
@@ -96,6 +99,10 @@ final class DifferentialTestPlanField
       $xaction->getNewValue());
   }
 
+  public function shouldHideInApplicationTransactions(
+    PhabricatorApplicationTransaction $xaction) {
+    return ($xaction->getOldValue() === null);
+  }
 
   public function shouldAppearInGlobalSearch() {
     return true;

src/applications/differential/storage/DifferentialTransaction.php

@@ -20,10 +20,23 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
   }
 
   public function shouldHide() {
-    switch ($this->getTransactionType()) {
-      case PhabricatorTransactions::TYPE_EDGE:
     $old = $this->getOldValue();
     $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_UPDATE:
+        // Older versions of this transaction have an ID for the new value,
+        // and/or do not record the old value. Only hide the transaction if
+        // the new value is a PHID, indicating that this is a newer style
+        // transaction.
+        if ($old === null) {
+          if (phid_get_type($new) == DifferentialPHIDTypeDiff::TYPECONST) {
+            return true;
+          }
+        }
+        break;
+
+      case PhabricatorTransactions::TYPE_EDGE:
         $add = array_diff_key($new, $old);
         $rem = array_diff_key($old, $new);
 
@@ -38,7 +51,7 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
         break;
     }
 
-    return false;
+    return parent::shouldHide();
   }
 
   public function shouldHideForMail(array $xactions) {

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -356,6 +356,11 @@ abstract class PhabricatorApplicationTransaction
           return false;
         }
         break;
+      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+        $field = $this->getTransactionCustomField();
+        if ($field) {
+          return $field->shouldHideInApplicationTransactions($this);
+        }
     }
 
     return false;

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -991,6 +991,14 @@ abstract class PhabricatorCustomField {
     return array();
   }
 
+  public function shouldHideInApplicationTransactions(
+    PhabricatorApplicationTransaction $xaction) {
+    if ($this->proxy) {
+      return $this->proxy->shouldHideInApplicationTransactions($xaction);
+    }
+    return false;
+  }
+
 
 /* -(  Edit View  )---------------------------------------------------------- */