Reduce Lint/Unit rendering duplication

Summary: Ref T8096. No functional changes, just a bit less code.

Test Plan: Viewed some revisions, saw the same stuff as before.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8096

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

src/__phutil_library_map__.php

@@ -376,6 +376,7 @@ phutil_register_library_map(array(
     'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php',
     'DifferentialGitHubLandingStrategy' => 'applications/differential/landing/DifferentialGitHubLandingStrategy.php',
     'DifferentialGitSVNIDField' => 'applications/differential/customfield/DifferentialGitSVNIDField.php',
+    'DifferentialHarbormasterField' => 'applications/differential/customfield/DifferentialHarbormasterField.php',
     'DifferentialHiddenComment' => 'applications/differential/storage/DifferentialHiddenComment.php',
     'DifferentialHostField' => 'applications/differential/customfield/DifferentialHostField.php',
     'DifferentialHostedGitLandingStrategy' => 'applications/differential/landing/DifferentialHostedGitLandingStrategy.php',
@@ -3743,6 +3744,7 @@ phutil_register_library_map(array(
     'DifferentialGetWorkingCopy' => 'Phobject',
     'DifferentialGitHubLandingStrategy' => 'DifferentialLandingStrategy',
     'DifferentialGitSVNIDField' => 'DifferentialCustomField',
+    'DifferentialHarbormasterField' => 'DifferentialCustomField',
     'DifferentialHiddenComment' => 'DifferentialDAO',
     'DifferentialHostField' => 'DifferentialCustomField',
     'DifferentialHostedGitLandingStrategy' => 'DifferentialLandingStrategy',
@@ -3768,7 +3770,7 @@ phutil_register_library_map(array(
     'DifferentialLandingStrategy' => 'Phobject',
     'DifferentialLegacyHunk' => 'DifferentialHunk',
     'DifferentialLineAdjustmentMap' => 'Phobject',
-    'DifferentialLintField' => 'DifferentialCustomField',
+    'DifferentialLintField' => 'DifferentialHarbormasterField',
     'DifferentialLintStatus' => 'Phobject',
     'DifferentialLocalCommitsView' => 'AphrontView',
     'DifferentialManiphestTasksField' => 'DifferentialCoreCustomField',
@@ -3844,7 +3846,7 @@ phutil_register_library_map(array(
     'DifferentialTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
     'DifferentialTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'DifferentialTransactionView' => 'PhabricatorApplicationTransactionView',
-    'DifferentialUnitField' => 'DifferentialCustomField',
+    'DifferentialUnitField' => 'DifferentialHarbormasterField',
     'DifferentialUnitStatus' => 'Phobject',
     'DifferentialUnitTestResult' => 'Phobject',
     'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod',

src/applications/differential/customfield/DifferentialHarbormasterField.php

@@ -0,0 +1,96 @@
+<?php
+
+abstract class DifferentialHarbormasterField
+  extends DifferentialCustomField {
+
+  abstract protected function getDiffPropertyKeys();
+  abstract protected function loadHarbormasterTargetMessages(
+    array $target_phids);
+  abstract protected function getLegacyProperty();
+  abstract protected function newModernMessage(array $message);
+  abstract protected function renderHarbormasterStatus(
+    DifferentialDiff $diff,
+    array $messages);
+  abstract protected function newHarbormasterMessageView(array $messages);
+
+  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
+    // TODO: This load is slightly inefficient, but most of this is moving
+    // to Harbormaster and this simplifies the transition. Eat 1-2 extra
+    // queries for now.
+    $keys = $this->getDiffPropertyKeys();
+
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d AND name IN (%Ls)',
+      $diff->getID(),
+      $keys);
+    $properties = mpull($properties, 'getData', 'getName');
+
+    foreach ($keys as $key) {
+      $diff->attachProperty($key, idx($properties, $key));
+    }
+
+    $messages = array();
+
+    $buildable = $diff->getBuildable();
+    if ($buildable) {
+      $target_phids = array();
+      foreach ($buildable->getBuilds() as $build) {
+        foreach ($build->getBuildTargets() as $target) {
+          $target_phids[] = $target->getPHID();
+        }
+      }
+
+      if ($target_phids) {
+        $messages = $this->loadHarbormasterTargetMessages($target_phids);
+      }
+    }
+
+    if (!$messages) {
+      // No Harbormaster messages, so look for legacy messages and make them
+      // look like modern messages.
+      $legacy_messages = $diff->getProperty($this->getLegacyProperty());
+      if ($legacy_messages) {
+        // Show the top 100 legacy lint messages. Previously, we showed some
+        // by default and let the user toggle the rest. With modern messages,
+        // we can send the user to the Harbormaster detail page. Just show
+        // "a lot" of messages in legacy cases to try to strike a balance
+        // between implementation simplicitly and compatibility.
+        $legacy_messages = array_slice($legacy_messages, 0, 100);
+
+        foreach ($legacy_messages as $message) {
+          try {
+            $modern = $this->newModernMessage($message);
+            $messages[] = $modern;
+          } catch (Exception $ex) {
+            // Ignore any poorly formatted messages.
+          }
+        }
+      }
+    }
+
+    $status = $this->renderHarbormasterStatus($diff, $messages);
+
+    if ($messages) {
+      $path_map = mpull($diff->loadChangesets(), 'getID', 'getFilename');
+      foreach ($path_map as $path => $id) {
+        $href = '#C'.$id.'NL';
+
+        // TODO: When the diff is not the right-hand-size diff, we should
+        // ideally adjust this URI to be absolute.
+
+        $path_map[$path] = $href;
+      }
+
+      $view = $this->newHarbormasterMessageView($messages)
+        ->setPathURIMap($path_map);
+    } else {
+      $view = null;
+    }
+
+    return array(
+      $status,
+      $view,
+    );
+  }
+
+}

src/applications/differential/customfield/DifferentialLintField.php

@@ -1,7 +1,7 @@
 <?php
 
 final class DifferentialLintField
-  extends DifferentialCustomField {
+  extends DifferentialHarbormasterField {
 
   public function getFieldKey() {
     return 'differential:lint';
@@ -31,91 +31,32 @@ final class DifferentialLintField
     return $this->getFieldName();
   }
 
-  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
-    // TODO: This load is slightly inefficient, but most of this is moving
-    // to Harbormaster and this simplifies the transition. Eat 1-2 extra
-    // queries for now.
-    $keys = array(
+  protected function getLegacyProperty() {
+    return 'arc:lint';
+  }
+
+  protected function getDiffPropertyKeys() {
+    return array(
       'arc:lint',
       'arc:lint-excuse',
     );
+  }
 
-    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
-      'diffID = %d AND name IN (%Ls)',
-      $diff->getID(),
-      $keys);
-    $properties = mpull($properties, 'getData', 'getName');
+  protected function loadHarbormasterTargetMessages(array $target_phids) {
+    return id(new HarbormasterBuildLintMessage())->loadAllWhere(
+      'buildTargetPHID IN (%Ls) LIMIT 25',
+      $target_phids);
+  }
 
-    foreach ($keys as $key) {
-      $diff->attachProperty($key, idx($properties, $key));
-    }
+  protected function newHarbormasterMessageView(array $messages) {
+    return id(new HarbormasterLintPropertyView())
+      ->setLintMessages($messages);
+  }
 
-    $status = $this->renderLintStatus($diff);
-
-    $lint = array();
-
-    $buildable = $diff->getBuildable();
-    if ($buildable) {
-      $target_phids = array();
-      foreach ($buildable->getBuilds() as $build) {
-        foreach ($build->getBuildTargets() as $target) {
-          $target_phids[] = $target->getPHID();
-        }
-      }
-
-      $lint = id(new HarbormasterBuildLintMessage())->loadAllWhere(
-        'buildTargetPHID IN (%Ls) LIMIT 25',
-        $target_phids);
-    }
-
-    if (!$lint) {
-      // No Harbormaster messages, so look for legacy messages and make them
-      // look like modern messages.
-      $legacy_lint = $diff->getProperty('arc:lint');
-      if ($legacy_lint) {
-        // Show the top 100 legacy lint messages. Previously, we showed some
-        // by default and let the user toggle the rest. With modern messages,
-        // we can send the user to the Harbormaster detail page. Just show
-        // "a lot" of messages in legacy cases to try to strike a balance
-        // between implementation simplicitly and compatibility.
-        $legacy_lint = array_slice($legacy_lint, 0, 100);
-
-        $target = new HarbormasterBuildTarget();
-        foreach ($legacy_lint as $message) {
-          try {
-            $modern = HarbormasterBuildLintMessage::newFromDictionary(
-              $target,
-              $this->getModernLintMessageDictionary($message));
-            $lint[] = $modern;
-          } catch (Exception $ex) {
-            // Ignore any poorly formatted messages.
-          }
-        }
-      }
-    }
-
-    if ($lint) {
-      $path_map = mpull($diff->loadChangesets(), 'getID', 'getFilename');
-      foreach ($path_map as $path => $id) {
-        $href = '#C'.$id.'NL';
-
-        // TODO: When the diff is not the right-hand-size diff, we should
-        // ideally adjust this URI to be absolute.
-
-        $path_map[$path] = $href;
-      }
-
-      $view = id(new HarbormasterLintPropertyView())
-        ->setPathURIMap($path_map)
-        ->setLintMessages($lint);
-    } else {
-      $view = null;
-    }
-
-    return array(
-      $status,
-      $view,
-    );
+  protected function newModernMessage(array $message) {
+    return HarbormasterBuildLintMessage::newFromDictionary(
+      new HarbormasterBuildTarget(),
+      $this->getModernLintMessageDictionary($message));
   }
 
   public function getWarningsForDetailView() {
@@ -141,7 +82,10 @@ final class DifferentialLintField
     return $warnings;
   }
 
-  private function renderLintStatus(DifferentialDiff $diff) {
+  protected function renderHarbormasterStatus(
+    DifferentialDiff $diff,
+    array $messages) {
+
     $colors = array(
       DifferentialLintStatus::LINT_NONE => 'grey',
       DifferentialLintStatus::LINT_OKAY => 'green',

src/applications/differential/customfield/DifferentialUnitField.php

@@ -1,7 +1,7 @@
 <?php
 
 final class DifferentialUnitField
-  extends DifferentialCustomField {
+  extends DifferentialHarbormasterField {
 
   public function getFieldKey() {
     return 'differential:unit';
@@ -31,84 +31,32 @@ final class DifferentialUnitField
     return $this->getFieldName();
   }
 
-  public function renderDiffPropertyViewValue(DifferentialDiff $diff) {
-    // TODO: See DifferentialLintField.
-    $keys = array(
+  protected function getLegacyProperty() {
+    return 'arc:unit';
+  }
+
+  protected function getDiffPropertyKeys() {
+    return array(
       'arc:unit',
       'arc:unit-excuse',
     );
+  }
 
-    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
-      'diffID = %d AND name IN (%Ls)',
-      $diff->getID(),
-      $keys);
-    $properties = mpull($properties, 'getData', 'getName');
+  protected function loadHarbormasterTargetMessages(array $target_phids) {
+    return id(new HarbormasterBuildUnitMessage())->loadAllWhere(
+      'buildTargetPHID IN (%Ls) LIMIT 25',
+      $target_phids);
+  }
 
-    foreach ($keys as $key) {
-      $diff->attachProperty($key, idx($properties, $key));
-    }
+  protected function newModernMessage(array $message) {
+    return HarbormasterBuildUnitMessage::newFromDictionary(
+      new HarbormasterBuildTarget(),
+      $this->getModernUnitMessageDictionary($message));
+  }
 
-    $status = $this->renderUnitStatus($diff);
-
-    $unit = array();
-
-    $buildable = $diff->getBuildable();
-    if ($buildable) {
-      $target_phids = array();
-      foreach ($buildable->getBuilds() as $build) {
-        foreach ($build->getBuildTargets() as $target) {
-          $target_phids[] = $target->getPHID();
-        }
-      }
-
-      $unit = id(new HarbormasterBuildUnitMessage())->loadAllWhere(
-        'buildTargetPHID IN (%Ls) LIMIT 25',
-        $target_phids);
-    }
-
-    if (!$unit) {
-      $legacy_unit = $diff->getProperty('arc:unit');
-      if ($legacy_unit) {
-        // Show the top 100 legacy unit messages.
-        $legacy_unit = array_slice($legacy_unit, 0, 100);
-
-        $target = new HarbormasterBuildTarget();
-        foreach ($legacy_unit as $message) {
-          try {
-            $modern = HarbormasterBuildUnitMessage::newFromDictionary(
-              $target,
-              $this->getModernUnitMessageDictionary($message));
-            $unit[] = $modern;
-          } catch (Exception $ex) {
-            // Just ignore it if legacy messages aren't formatted like
-            // we expect.
-          }
-        }
-      }
-    }
-
-    if ($unit) {
-      $path_map = mpull($diff->loadChangesets(), 'getID', 'getFilename');
-      foreach ($path_map as $path => $id) {
-        $href = '#C'.$id.'NL';
-
-        // TODO: When the diff is not the right-hand-size diff, we should
-        // ideally adjust this URI to be absolute.
-
-        $path_map[$path] = $href;
-      }
-
-      $view = id(new HarbormasterUnitPropertyView())
-        ->setPathURIMap($path_map)
-        ->setUnitMessages($unit);
-    } else {
-      $view = null;
-    }
-
-    return array(
-      $status,
-      $view,
-    );
+  protected function newHarbormasterMessageView(array $messages) {
+    return id(new HarbormasterUnitPropertyView())
+      ->setUnitMessages($messages);
   }
 
   public function getWarningsForDetailView() {
@@ -132,8 +80,10 @@ final class DifferentialUnitField
     return $warnings;
   }
 
+  protected function renderHarbormasterStatus(
+    DifferentialDiff $diff,
+    array $messages) {
 
-  private function renderUnitStatus(DifferentialDiff $diff) {
     $colors = array(
       DifferentialUnitStatus::UNIT_NONE => 'grey',
       DifferentialUnitStatus::UNIT_OKAY => 'green',