Improve UX for Diffusion mail fields

Summary:
  - Give the fields names and descriptions.
  - When new, default-disabled fields are added, disable them by default even if there's already a config.
  - Be a bit less hacky about `$faux_spec`.

Test Plan: {F432383}

Reviewers: joshuaspence, fabe

Reviewed By: fabe

Subscribers: epriestley

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

src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php

@@ -145,9 +145,7 @@ final class PhabricatorDiffusionConfigOptions
           id(new PhabricatorRepositoryCommit())
             ->getCustomFieldBaseClass())
         ->setDescription(
-          pht(
+          pht('Select and reorder Diffusion fields.')),
-            "Select and reorder diffusion fields.\n\n".
-            "These will primarily show up in Mail Notifications.")),
     );
   }
 

src/applications/repository/customfield/PhabricatorCommitBranchesField.php

@@ -7,6 +7,14 @@ final class PhabricatorCommitBranchesField
     return 'diffusion:branches';
   }
 
+  public function getFieldName() {
+    return pht('Branches');
+  }
+
+  public function getFieldDescription() {
+    return pht('Shows branches a commit appears on in email.');
+  }
+
   public function shouldAppearInTransactionMail() {
     return true;
   }

src/applications/repository/customfield/PhabricatorCommitMergedCommitsField.php

@@ -7,6 +7,14 @@ final class PhabricatorCommitMergedCommitsField
     return 'diffusion:mergedcommits';
   }
 
+  public function getFieldName() {
+    return pht('Merged Commits');
+  }
+
+  public function getFieldDescription() {
+    return pht('For merge commits, shows merged changes in email.');
+  }
+
   public function shouldDisableByDefault() {
     return true;
   }

src/applications/repository/customfield/PhabricatorCommitRepositoryField.php

@@ -7,6 +7,14 @@ final class PhabricatorCommitRepositoryField
     return 'diffusion:repository';
   }
 
+  public function getFieldName() {
+    return pht('Repository');
+  }
+
+  public function getFieldDescription() {
+    return pht('Shows repository in email.');
+  }
+
   public function shouldDisableByDefault() {
     return true;
   }

src/applications/repository/customfield/PhabricatorCommitTagsField.php

@@ -7,6 +7,14 @@ final class PhabricatorCommitTagsField
     return 'diffusion:tags';
   }
 
+  public function getFieldName() {
+    return pht('Tags');
+  }
+
+  public function getFieldDescription() {
+    return pht('Shows commit tags in email.');
+  }
+
   public function shouldAppearInTransactionMail() {
     return true;
   }

src/infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php

@@ -34,20 +34,16 @@ final class PhabricatorCustomFieldConfigOptionType
       $field_spec = PhabricatorEnv::getEnvConfig($option->getKey());
     }
 
-    // Get all of the fields (including disabled fields) by querying for them
-    // with a faux spec where no fields are disabled.
-    $faux_spec = $field_spec;
-    foreach ($faux_spec as $key => $spec) {
-      unset($faux_spec[$key]['disabled']);
-    }
-
     // TODO: We might need to build a real object here eventually.
     $faux_object = null;
 
     $fields = PhabricatorCustomField::buildFieldList(
       $field_base_class,
-      $faux_spec,
+      $field_spec,
-      $faux_object);
+      $faux_object,
+      array(
+        'withDisabled' => true,
+      ));
 
     $list_id = celerity_generate_unique_node_id();
     $input_id = celerity_generate_unique_node_id();
@@ -63,7 +59,8 @@ final class PhabricatorCustomFieldConfigOptionType
         ->addAttribute($field->getFieldDescription())
         ->setHeader($field->getFieldName());
 
-      $is_disabled = !empty($field_spec[$key]['disabled']);
+      $spec = idx($field_spec, $key, array());
+      $is_disabled = idx($spec, 'disabled', $field->shouldDisableByDefault());
 
       $disabled_item = clone $item;
       $enabled_item = clone $item;

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -105,7 +105,18 @@ abstract class PhabricatorCustomField {
   /**
    * @task apps
    */
-  public static function buildFieldList($base_class, array $spec, $object) {
+  public static function buildFieldList(
+    $base_class,
+    array $spec,
+    $object,
+    array $options = array()) {
+
+    PhutilTypeSpec::checkMap(
+      $options,
+      array(
+        'withDisabled' => 'optional bool',
+      ));
+
     $field_objects = id(new PhutilSymbolLoader())
       ->setAncestorClass($base_class)
       ->loadObjects();
@@ -135,13 +146,16 @@ abstract class PhabricatorCustomField {
 
     $fields = array_select_keys($fields, array_keys($spec)) + $fields;
 
-    foreach ($spec as $key => $config) {
+    if (empty($options['withDisabled'])) {
-      if (empty($fields[$key])) {
+      foreach ($fields as $key => $field) {
-        continue;
+        $config = idx($spec, $key, array()) + array(
-      }
+          'disabled' => $field->shouldDisableByDefault(),
-      if (!empty($config['disabled'])) {
+        );
-        if ($fields[$key]->canDisableField()) {
+
-          unset($fields[$key]);
+        if (!empty($config['disabled'])) {
+          if ($field->canDisableField()) {
+            unset($fields[$key]);
+          }
         }
       }
     }