Implement "Test Plan" as a CustomField

Summary: Ref T3886. Moves some of the "required" logic to the base class ("DifferentialCoreField") so Title and Test Plan can share it.

Test Plan: Edited revisions using "pro" editor, saw test plan transactions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886

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

src/__phutil_library_map__.php

@@ -465,6 +465,7 @@ phutil_register_library_map(array(
     'DifferentialSummaryField' => 'applications/differential/customfield/DifferentialSummaryField.php',
     'DifferentialSummaryFieldSpecification' => 'applications/differential/field/specification/DifferentialSummaryFieldSpecification.php',
     'DifferentialTasksAttacher' => 'applications/differential/DifferentialTasksAttacher.php',
+    'DifferentialTestPlanField' => 'applications/differential/customfield/DifferentialTestPlanField.php',
     'DifferentialTestPlanFieldSpecification' => 'applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php',
     'DifferentialTitleField' => 'applications/differential/customfield/DifferentialTitleField.php',
     'DifferentialTitleFieldSpecification' => 'applications/differential/field/specification/DifferentialTitleFieldSpecification.php',
@@ -3008,6 +3009,7 @@ phutil_register_library_map(array(
     'DifferentialSubscribeController' => 'DifferentialController',
     'DifferentialSummaryField' => 'DifferentialCoreCustomField',
     'DifferentialSummaryFieldSpecification' => 'DifferentialFreeformFieldSpecification',
+    'DifferentialTestPlanField' => 'DifferentialCoreCustomField',
     'DifferentialTestPlanFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialTitleField' => 'DifferentialCoreCustomField',
     'DifferentialTitleFieldSpecification' => 'DifferentialFreeformFieldSpecification',

src/applications/differential/customfield/DifferentialCoreCustomField.php

@@ -18,10 +18,53 @@ abstract class DifferentialCoreCustomField
     DifferentialRevision $revision,
     $value);
 
-  public function isCoreFieldRequired() {
+  protected function isCoreFieldRequired() {
     return false;
   }
 
+  protected function isCoreFieldValueEmpty($value) {
+    if (is_array($value)) {
+      return !$value;
+    }
+    return !strlen(trim($value));
+  }
+
+  protected function getCoreFieldRequiredErrorString() {
+    throw new PhabricatorCustomFieldImplementationIncompleteException($this);
+  }
+
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+
+    $this->setFieldError(null);
+
+    $errors = parent::validateApplicationTransactions(
+      $editor,
+      $type,
+      $xactions);
+
+    $transaction = null;
+    foreach ($xactions as $xaction) {
+      $value = $xaction->getNewValue();
+      if ($this->isCoreFieldRequired()) {
+        if ($this->isCoreFieldValueEmpty($value)) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            $this->getCoreFieldRequiredErrorString(),
+            $xaction);
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+          $this->setFieldError(pht('Required'));
+        }
+      }
+    }
+
+    return $errors;
+  }
+
   public function canDisableField() {
     return false;
   }

src/applications/differential/customfield/DifferentialTestPlanField.php

@@ -0,0 +1,95 @@
+<?php
+
+final class DifferentialTestPlanField
+  extends DifferentialCoreCustomField {
+
+  public function getFieldKey() {
+    return 'differential:test-plan';
+  }
+
+  public function getFieldName() {
+    return pht('Test Plan');
+  }
+
+  public function getFieldDescription() {
+    return pht('Actions performed to verify the behavior of the change.');
+  }
+
+  protected function readValueFromRevision(
+    DifferentialRevision $revision) {
+    return $revision->getTestPlan();
+  }
+
+  protected function writeValueToRevision(
+    DifferentialRevision $revision,
+    $value) {
+    $revision->setTestPlan($value);
+  }
+
+  protected function isCoreFieldRequired() {
+    return PhabricatorEnv::getEnvConfig('differential.require-test-plan-field');
+  }
+
+  public function canDisableField() {
+    return true;
+  }
+
+  protected function getCoreFieldRequiredErrorString() {
+    return pht(
+      'You must provide a test plan: describe the actions you performed '.
+      'to verify the behvaior of this change.');
+  }
+
+  public function readValueFromRequest(AphrontRequest $request) {
+    $this->setValue($request->getStr($this->getFieldKey()));
+  }
+
+  public function renderEditControl() {
+    return id(new PhabricatorRemarkupControl())
+      ->setName($this->getFieldKey())
+      ->setValue($this->getValue())
+      ->setError($this->getFieldError())
+      ->setLabel($this->getFieldName());
+  }
+
+  public function getApplicationTransactionTitle(
+    PhabricatorApplicationTransaction $xaction) {
+    $author_phid = $xaction->getAuthorPHID();
+    $old = $xaction->getOldValue();
+    $new = $xaction->getNewValue();
+
+    return pht(
+      '%s updated the test plan for this revision.',
+      $xaction->renderHandleLink($author_phid));
+  }
+
+  public function getApplicationTransactionTitleForFeed(
+    PhabricatorApplicationTransaction $xaction,
+    PhabricatorFeedStory $story) {
+
+    $object_phid = $xaction->getObjectPHID();
+    $author_phid = $xaction->getAuthorPHID();
+    $old = $xaction->getOldValue();
+    $new = $xaction->getNewValue();
+
+    return pht(
+      '%s updated the test plan for %s.',
+      $xaction->renderHandleLink($author_phid),
+      $xaction->renderHandleLink($object_phid));
+  }
+
+  public function getApplicationTransactionHasChangeDetails(
+    PhabricatorApplicationTransaction $xaction) {
+    return true;
+  }
+
+  public function getApplicationTransactionChangeDetails(
+    PhabricatorApplicationTransaction $xaction,
+    PhabricatorUser $viewer) {
+    return $xaction->renderTextCorpusChangeDetails(
+      $viewer,
+      $xaction->getOldValue(),
+      $xaction->getNewValue());
+  }
+
+}

src/applications/differential/customfield/DifferentialTitleField.php

@@ -26,36 +26,18 @@ final class DifferentialTitleField
     $revision->setTitle($value);
   }
 
-  public function validateApplicationTransactions(
+  protected function getCoreFieldRequiredErrorString() {
-    PhabricatorApplicationTransactionEditor $editor,
+    return pht('You must choose a title for this revision.');
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateApplicationTransactions(
-      $editor,
-      $type,
-      $xactions);
-
-    $transaction = null;
-    foreach ($xactions as $xaction) {
-      $value = $xaction->getNewValue();
-      if (!strlen($value)) {
-        $error = new PhabricatorApplicationTransactionValidationError(
-          $type,
-          pht('Required'),
-          pht('You must choose a title for this revision.'),
-          $xaction);
-        $error->setIsMissingFieldError(true);
-        $errors[] = $error;
-        $this->setFieldError(pht('Required'));
-      }
-    }
   }
 
   public function readValueFromRequest(AphrontRequest $request) {
     $this->setValue($request->getStr($this->getFieldKey()));
   }
 
+  protected function isCoreFieldRequired() {
+    return true;
+  }
+
   public function renderEditControl() {
     return id(new AphrontFormTextAreaControl())
       ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT)