Allow "Test Plan" to be disabled in config

Summary:
This is a somewhat common request, and far more difficult than necessary currently.

I think the field is useful enough to leave it default-enabled, but there's wide diversity in testing philosophy.

Test Plan: Verified "test plan" field appeared. Disabled config. Verified "test plan" field vanished.

Reviewers: btrahan, vrana, jungejason

Reviewed By: vrana

CC: aran, asouza

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

conf/default.conf.php

@@ -675,6 +675,18 @@ return array(
   // enable them.
   'differential.show-host-field'  => false,
 
+  // Differential has a required "Test Plan" field by default, which requires
+  // authors to fill out information about how they verified the correctness of
+  // their changes when sending code for review. If you'd prefer not to use
+  // this field, you can disable it here. You can also make it optional
+  // (instead of required) below.
+  'differential.show-test-plan-field' => true,
+
+  // Differential has a required "Test Plan" field by default. You can make it
+  // optional by setting this to false. You can also completely remove it above,
+  // if you prefer.
+  'differential.require-test-plan-field' => true,
+
   // If you set this to true, users can "!accept" revisions via email (normally,
   // they can take other actions but can not "!accept"). This action is disabled
   // by default because email authentication can be configured to be very weak,

src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php

@@ -23,26 +23,30 @@ final class DifferentialDefaultFieldSelector
     $fields = array(
       new DifferentialTitleFieldSpecification(),
       new DifferentialSummaryFieldSpecification(),
-      new DifferentialTestPlanFieldSpecification(),
-      new DifferentialRevisionStatusFieldSpecification(),
-      new DifferentialAuthorFieldSpecification(),
-      new DifferentialReviewersFieldSpecification(),
-      new DifferentialReviewedByFieldSpecification(),
-      new DifferentialCCsFieldSpecification(),
-      new DifferentialUnitFieldSpecification(),
-      new DifferentialLintFieldSpecification(),
-      new DifferentialCommitsFieldSpecification(),
-      new DifferentialDependenciesFieldSpecification(),
-      new DifferentialManiphestTasksFieldSpecification(),
     );
 
+    if (PhabricatorEnv::getEnvConfig('differential.show-test-plan-field')) {
+      $fields[] = new DifferentialTestPlanFieldSpecification();
+    }
+
+    $fields = array_merge(
+      $fields,
+      array(
+        new DifferentialRevisionStatusFieldSpecification(),
+        new DifferentialAuthorFieldSpecification(),
+        new DifferentialReviewersFieldSpecification(),
+        new DifferentialReviewedByFieldSpecification(),
+        new DifferentialCCsFieldSpecification(),
+        new DifferentialUnitFieldSpecification(),
+        new DifferentialLintFieldSpecification(),
+        new DifferentialCommitsFieldSpecification(),
+        new DifferentialDependenciesFieldSpecification(),
+        new DifferentialManiphestTasksFieldSpecification(),
+      ));
+
     if (PhabricatorEnv::getEnvConfig('differential.show-host-field')) {
-      $fields = array_merge(
+      $fields[] = new DifferentialHostFieldSpecification();
-        $fields,
+      $fields[] = new DifferentialPathFieldSpecification();
-        array(
-          new DifferentialHostFieldSpecification(),
-          new DifferentialPathFieldSpecification(),
-        ));
     }
 
     $fields = array_merge(

src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,7 +20,9 @@ final class DifferentialTestPlanFieldSpecification
   extends DifferentialFieldSpecification {
 
   private $plan;
-  private $error = true;
+
+  // NOTE: This means "uninitialized".
+  private $error = false;
 
   public function shouldAppearOnEdit() {
     return true;
@@ -37,6 +39,14 @@ final class DifferentialTestPlanFieldSpecification
   }
 
   public function renderEditControl() {
+    if ($this->error === false) {
+      if ($this->isRequired()) {
+        $this->error = true;
+      } else {
+        $this->error = null;
+      }
+    }
+
     return id(new AphrontFormTextAreaControl())
       ->setLabel('Test Plan')
       ->setName('testplan')
@@ -49,10 +59,12 @@ final class DifferentialTestPlanFieldSpecification
   }
 
   public function validateField() {
-    if (!strlen($this->plan)) {
+    if ($this->isRequired()) {
-      $this->error = 'Required';
+      if (!strlen($this->plan)) {
-      throw new DifferentialFieldValidationException(
+        $this->error = 'Required';
-        "You must provide a test plan.");
+        throw new DifferentialFieldValidationException(
+          "You must provide a test plan.");
+      }
     }
   }
 
@@ -85,4 +97,8 @@ final class DifferentialTestPlanFieldSpecification
     return $value;
   }
 
+  private function isRequired() {
+    return PhabricatorEnv::getEnvConfig('differential.require-test-plan-field');
+  }
+
 }

src/applications/differential/field/specification/testplan/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/differential/field/exception/validation');
 phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/control/textarea');
 
 phutil_require_module('phutil', 'utils');