Refactor some Herald code

Summary:
I was reading herald code for a task and realized that the method was
really long. So I refactor it to shorter methods.

Test Plan:
was still able to create a differential rule and commit rule; and
verified that dry-run still worked.

Reviewers: epriestley, tuomaspelkonen

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 1077

src/applications/herald/controller/rule/HeraldRuleController.php

@@ -71,6 +71,133 @@ class HeraldRuleController extends HeraldController {
     $e_name = true;
     $errors = array();
     if ($request->isFormPost() && $request->getStr('save')) {
+      list($e_name, $errors) = $this->saveRule($rule, $request);
+
+      if (!$errors) {
+        $uri = '/herald/view/'.$rule->getContentType().'/';
+        return id(new AphrontRedirectResponse())
+          ->setURI($uri);
+      }
+    }
+
+    if ($errors) {
+      $error_view = new AphrontErrorView();
+      $error_view->setTitle('Form Errors');
+      $error_view->setErrors($errors);
+    } else {
+      $error_view = null;
+    }
+
+    $must_match_selector = $this->getMustMatchSelector($rule);
+    $repetition_selector = $this->getRepetitionSelector($rule);
+
+    $handles = $this->loadHandles($rule);
+
+    require_celerity_resource('herald-css');
+
+    $type_name = $content_type_map[$rule->getContentType()];
+
+    $form = id(new AphrontFormView())
+      ->setUser($user)
+      ->setID('herald-rule-edit-form')
+      ->addHiddenInput('type', $rule->getContentType())
+      ->addHiddenInput('save', 1)
+      ->appendChild(
+        // Build this explicitly so we can add a sigil to it.
+        javelin_render_tag(
+          'input',
+          array(
+            'type'  => 'hidden',
+            'name'  => 'rule',
+            'sigil' => 'rule',
+          )))
+      ->appendChild(
+        id(new AphrontFormTextControl())
+          ->setLabel('Rule Name')
+          ->setName('name')
+          ->setError($e_name)
+          ->setValue($rule->getName()))
+      ->appendChild(
+        id(new AphrontFormStaticControl())
+          ->setLabel('Author')
+          ->setValue($handles[$rule->getAuthorPHID()]->getName()))
+      ->appendChild(
+        id(new AphrontFormMarkupControl())
+          ->setValue(
+            "This rule triggers for <strong>{$type_name}</strong>."))
+      ->appendChild(
+        '<h1>Conditions</h1>'.
+        '<div class="aphront-form-inset">'.
+          '<div style="float: right;">'.
+            javelin_render_tag(
+              'a',
+              array(
+                'href' => '#',
+                'class' => 'button green',
+                'sigil' => 'create-condition',
+                'mustcapture' => true,
+              ),
+              'Create New Condition').
+          '</div>'.
+          '<p>When '.$must_match_selector.' these conditions are met:</p>'.
+          '<div style="clear: both;"></div>'.
+          javelin_render_tag(
+            'table',
+            array(
+              'sigil' => 'rule-conditions',
+              'class' => 'herald-condition-table',
+            ),
+            '').
+        '</div>')
+      ->appendChild(
+        '<h1>Action</h1>'.
+        '<div class="aphront-form-inset">'.
+          '<div style="float: right;">'.
+          javelin_render_tag(
+            'a',
+            array(
+              'href' => '#',
+              'class' => 'button green',
+              'sigil' => 'create-action',
+              'mustcapture' => true,
+            ),
+            'Create New Action').
+          '</div>'.
+          '<p>'.
+            'Take these actions '.$repetition_selector.' this rule matches:'.
+          '</p>'.
+          '<div style="clear: both;"></div>'.
+          javelin_render_tag(
+            'table',
+            array(
+              'sigil' => 'rule-actions',
+              'class' => 'herald-action-table',
+            ),
+            '').
+        '</div>')
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+          ->setValue('Save Rule')
+          ->addCancelButton('/herald/view/'.$rule->getContentType().'/'));
+
+    $this->setupEditorBehavior($rule, $handles);
+
+    $panel = new AphrontPanelView();
+    $panel->setHeader('Edit Herald Rule');
+    $panel->setWidth(AphrontPanelView::WIDTH_WIDE);
+    $panel->appendChild($form);
+
+    return $this->buildStandardPageResponse(
+      array(
+        $error_view,
+        $panel,
+      ),
+      array(
+        'title' => 'Edit Rule',
+      ));
+  }
+
+  private function saveRule($rule, $request) {
     $rule->setName($request->getStr('name'));
     $rule->setMustMatchAll(($request->getStr('must_match') == 'all'));
 
@@ -79,6 +206,9 @@ class HeraldRuleController extends HeraldController {
       HeraldRepetitionPolicyConfig::toInt($repetition_policy_param)
     );
 
+    $e_name = true;
+    $errors = array();
+
     if (!strlen($rule->getName())) {
       $e_name = "Required";
       $errors[] = "Rule must have a name.";
@@ -191,207 +321,16 @@ class HeraldRuleController extends HeraldController {
           $rule->saveActions($actions);
 //          $rule->saveTransaction();
 
-          $uri = '/herald/view/'.$rule->getContentType().'/';
-
-          return id(new AphrontRedirectResponse())
-            ->setURI($uri);
       } catch (AphrontQueryDuplicateKeyException $ex) {
         $e_name = "Not Unique";
         $errors[] = "Rule name is not unique. Choose a unique name.";
       }
     }
 
+    return array($e_name, $errors);
   }
 
-    $phids = array();
-    $phids[] = $rule->getAuthorPHID();
-
-    foreach ($rule->getActions() as $action) {
-      if (!is_array($action->getTarget())) {
-        continue;
-      }
-      foreach ($action->getTarget() as $target) {
-        $target = (array)$target;
-        foreach ($target as $phid) {
-          $phids[] = $phid;
-        }
-      }
-    }
-
-    foreach ($rule->getConditions() as $condition) {
-      $value = $condition->getValue();
-      if (is_array($value)) {
-        foreach ($value as $phid) {
-          $phids[] = $phid;
-        }
-      }
-    }
-
-    $handles = id(new PhabricatorObjectHandleData($phids))
-      ->loadHandles();
-
-    if ($errors) {
-      $error_view = new AphrontErrorView();
-      $error_view->setTitle('Form Errors');
-      $error_view->setErrors($errors);
-    } else {
-      $error_view = null;
-    }
-
-    $options = array(
-      'all' => 'all of',
-      'any' => 'any of',
-    );
-
-    $selected = $rule->getMustMatchAll() ? 'all' : 'any';
-
-    $must_match = array();
-    foreach ($options as $key => $option) {
-      $must_match[] = phutil_render_tag(
-        'option',
-        array(
-          'selected' => ($selected == $key) ? 'selected' : null,
-          'value' => $key,
-        ),
-        phutil_escape_html($option));
-    }
-    $must_match =
-      '<select name="must_match">'.
-        implode("\n", $must_match).
-      '</select>';
-
-    if ($rule->getID()) {
-      $action = '/herald/rule/'.$rule->getID().'/';
-    } else {
-      $action = '/herald/rule/'.$rule->getID().'/';
-    }
-
-    // Make the selector for choosing how often this rule should be repeated
-    $repetition_selector = "";
-    $repetition_policy = HeraldRepetitionPolicyConfig::toString(
-      $rule->getRepetitionPolicy()
-    );
-    $repetition_options = HeraldRepetitionPolicyConfig::getMapForContentType(
-      $rule->getContentType()
-    );
-
-    if (empty($repetition_options)) {
-      // default option is 'every time'
-      $repetition_selector = idx(
-        HeraldRepetitionPolicyConfig::getMap(),
-        HeraldRepetitionPolicyConfig::EVERY
-      );
-    } else if (count($repetition_options) == 1) {
-      // if there's only 1 option, just pick it for the user
-      $repetition_selector = reset($repetition_options);
-    } else {
-      // give the user all the options for this rule type
-      $tags = array();
-
-      foreach ($repetition_options as $name => $option) {
-        $tags[] = phutil_render_tag(
-          'option',
-          array (
-            'selected'  => ($repetition_policy == $name) ? 'selected' : null,
-            'value'     => $name,
-          ),
-          phutil_escape_html($option)
-        );
-      }
-
-      $repetition_selector =
-        '<select name="repetition_policy">'.
-          implode("\n", $tags).
-        '</select>';
-    }
-
-    require_celerity_resource('herald-css');
-
-    $type_name = $content_type_map[$rule->getContentType()];
-
-    $form = id(new AphrontFormView())
-      ->setUser($user)
-      ->setID('herald-rule-edit-form')
-      ->addHiddenInput('type', $rule->getContentType())
-      ->addHiddenInput('save', 1)
-      ->appendChild(
-        // Build this explicitly so we can add a sigil to it.
-        javelin_render_tag(
-          'input',
-          array(
-            'type'  => 'hidden',
-            'name'  => 'rule',
-            'sigil' => 'rule',
-          )))
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel('Rule Name')
-          ->setName('name')
-          ->setError($e_name)
-          ->setValue($rule->getName()))
-      ->appendChild(
-        id(new AphrontFormStaticControl())
-          ->setLabel('Author')
-          ->setValue($handles[$rule->getAuthorPHID()]->getName()))
-      ->appendChild(
-        id(new AphrontFormMarkupControl())
-          ->setValue(
-            "This rule triggers for <strong>{$type_name}</strong>."))
-      ->appendChild(
-        '<h1>Conditions</h1>'.
-        '<div class="aphront-form-inset">'.
-          '<div style="float: right;">'.
-            javelin_render_tag(
-              'a',
-              array(
-                'href' => '#',
-                'class' => 'button green',
-                'sigil' => 'create-condition',
-                'mustcapture' => true,
-              ),
-              'Create New Condition').
-          '</div>'.
-          '<p>When '.$must_match.' these conditions are met:</p>'.
-          '<div style="clear: both;"></div>'.
-          javelin_render_tag(
-            'table',
-            array(
-              'sigil' => 'rule-conditions',
-              'class' => 'herald-condition-table',
-            ),
-            '').
-        '</div>')
-      ->appendChild(
-        '<h1>Action</h1>'.
-        '<div class="aphront-form-inset">'.
-          '<div style="float: right;">'.
-          javelin_render_tag(
-            'a',
-            array(
-              'href' => '#',
-              'class' => 'button green',
-              'sigil' => 'create-action',
-              'mustcapture' => true,
-            ),
-            'Create New Action').
-          '</div>'.
-          '<p>'.
-            'Take these actions '.$repetition_selector.' this rule matches:'.
-          '</p>'.
-          '<div style="clear: both;"></div>'.
-          javelin_render_tag(
-            'table',
-            array(
-              'sigil' => 'rule-actions',
-              'class' => 'herald-action-table',
-            ),
-            '').
-        '</div>')
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->setValue('Save Rule')
-          ->addCancelButton('/herald/view/'.$rule->getContentType().'/'));
-
+  private function setupEditorBehavior($rule, $handles) {
     $serial_conditions = array(
       array('default', 'default', ''),
     );
@@ -474,27 +413,111 @@ class HeraldRuleController extends HeraldController {
       'herald-rule-editor',
       array(
         'root' => 'herald-rule-edit-form',
-        'conditions' => (object) $serial_conditions,
-        'actions' => (object) $serial_actions,
+        'conditions' => (object)$serial_conditions,
+        'actions' => (object)$serial_actions,
         'template' => $this->buildTokenizerTemplates() + array(
           'rules' => $all_rules,
         ),
         'info' => $config_info,
       ));
+  }
 
-    $panel = new AphrontPanelView();
-    $panel->setHeader('Edit Herald Rule');
-    $panel->setWidth(AphrontPanelView::WIDTH_WIDE);
-    $panel->appendChild($form);
+  private function loadHandles($rule) {
+    $phids = array();
+    $phids[] = $rule->getAuthorPHID();
 
-    return $this->buildStandardPageResponse(
+    foreach ($rule->getActions() as $action) {
+      if (!is_array($action->getTarget())) {
+        continue;
+      }
+      foreach ($action->getTarget() as $target) {
+        $target = (array)$target;
+        foreach ($target as $phid) {
+          $phids[] = $phid;
+        }
+      }
+    }
+
+    foreach ($rule->getConditions() as $condition) {
+      $value = $condition->getValue();
+      if (is_array($value)) {
+        foreach ($value as $phid) {
+          $phids[] = $phid;
+        }
+      }
+    }
+
+    $handles = id(new PhabricatorObjectHandleData($phids))
+      ->loadHandles();
+    return $handles;
+  }
+
+  private function getMustMatchSelector($rule) {
+    $options = array(
+      'all' => 'all of',
+      'any' => 'any of',
+    );
+
+    $selected = $rule->getMustMatchAll() ? 'all' : 'any';
+
+    $must_match = array();
+    foreach ($options as $key => $option) {
+      $must_match[] = phutil_render_tag(
+        'option',
         array(
-        $error_view,
-        $panel,
+          'selected' => ($selected == $key) ? 'selected' : null,
+          'value' => $key,
         ),
+        phutil_escape_html($option));
+    }
+    $must_match =
+      '<select name="must_match">' .
+      implode("\n", $must_match) .
+      '</select>';
+    return $must_match;
+  }
+
+  private function getRepetitionSelector($rule) {
+    // Make the selector for choosing how often this rule should be repeated
+    $repetition_policy = HeraldRepetitionPolicyConfig::toString(
+      $rule->getRepetitionPolicy()
+    );
+    $repetition_options = HeraldRepetitionPolicyConfig::getMapForContentType(
+      $rule->getContentType()
+    );
+
+    if (empty($repetition_options)) {
+      // default option is 'every time'
+      $repetition_selector = idx(
+        HeraldRepetitionPolicyConfig::getMap(),
+        HeraldRepetitionPolicyConfig::EVERY
+      );
+      return $repetition_selector;
+    } else if (count($repetition_options) == 1) {
+      // if there's only 1 option, just pick it for the user
+      $repetition_selector = reset($repetition_options);
+      return $repetition_selector;
+    } else {
+      // give the user all the options for this rule type
+      $tags = array();
+
+      foreach ($repetition_options as $name => $option) {
+        $tags[] = phutil_render_tag(
+          'option',
           array(
-        'title' => 'Edit Rule',
-      ));
+            'selected' => ($repetition_policy == $name) ? 'selected' : null,
+            'value' => $name,
+          ),
+          phutil_escape_html($option)
+        );
+      }
+
+      $repetition_selector =
+        '<select name="repetition_policy">' .
+        implode("\n", $tags) .
+        '</select>';
+      return $repetition_selector;
+    }
   }
 
   protected function buildTokenizerTemplates() {