From 5a158b5b193276a822ca179c329eeee3948cfcef Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 2 Jul 2014 04:58:51 -0700
Subject: [PATCH] Move revision header warnings into custom fields

Summary:
Ref T5495. We currently show one warning in revision headers, about not having any reviewers.

I want to add a second warning (for missing Legalpad signatures). At least one install would like to add custom warnings (see T5495) which are so specific that we can't reasonably cover them in the upstream.

Generalize these header warnings by moving them to CustomField, so I can implement the Legalpad stuff without making a mess and the install in T5495 can use an extension.

Test Plan:
Hit all three header states, they look exactly like they did before this change:

{F173265}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5495

Differential Revision: https://secure.phabricator.com/D9793
---
 .../DifferentialRevisionViewController.php    | 63 ++++++++++---------
 .../customfield/DifferentialCustomField.php   |  8 +++
 .../DifferentialReviewersField.php            | 30 +++++++++
 3 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index aa89286025..a4efd83c41 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -115,6 +115,22 @@ final class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $revision,
+      PhabricatorCustomField::ROLE_VIEW);
+
+    $field_list->setViewer($user);
+    $field_list->readFieldsFromStorage($revision);
+
+    $warning_handle_map = array();
+    foreach ($field_list->getFields() as $key => $field) {
+      $req = $field->getRequiredHandlePHIDsForRevisionHeaderWarnings();
+      foreach ($req as $phid) {
+        $warning_handle_map[$key][] = $phid;
+        $object_phids[] = $phid;
+      }
+    }
+
     $handles = $this->loadViewerHandles($object_phids);
 
     $request_uri = $request->getRequestURI();
@@ -169,13 +185,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $visible_changesets = $changesets;
     }
 
-    $field_list = PhabricatorCustomField::getObjectFields(
-      $revision,
-      PhabricatorCustomField::ROLE_VIEW);
-
-    $field_list->setViewer($user);
-    $field_list->readFieldsFromStorage($revision);
-
 
     // TODO: This should be in a DiffQuery or similar.
     $need_props = array();
@@ -245,8 +254,15 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $revision_detail_box = $revision_detail->render();
 
-    $revision_warnings = $this->buildRevisionWarnings($revision, $handles);
+    $revision_warnings = $this->buildRevisionWarnings(
+      $revision,
+      $field_list,
+      $warning_handle_map,
+      $handles);
     if ($revision_warnings) {
+      $revision_warnings = id(new AphrontErrorView())
+        ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
+        ->setErrors($revision_warnings);
       $revision_detail_box->setErrorView($revision_warnings);
     }
 
@@ -935,32 +951,21 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
   private function buildRevisionWarnings(
     DifferentialRevision $revision,
+    PhabricatorCustomFieldList $field_list,
+    array $warning_handle_map,
     array $handles) {
 
-    $status_needs_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-    if ($revision->getStatus() != $status_needs_review) {
-      return;
-    }
-
-    foreach ($revision->getReviewers() as $reviewer) {
-      if (!$handles[$reviewer]->isDisabled()) {
-        return;
+    $warnings = array();
+    foreach ($field_list->getFields() as $key => $field) {
+      $phids = idx($warning_handle_map, $key, array());
+      $field_handles = array_select_keys($handles, $phids);
+      $field_warnings = $field->getWarningsForRevisionHeader($field_handles);
+      foreach ($field_warnings as $warning) {
+        $warnings[] = $warning;
       }
     }
 
-    $warnings = array();
-    if ($revision->getReviewers()) {
-      $warnings[] = pht(
-        'This revision needs review, but all specified reviewers are '.
-        'disabled or inactive.');
-    } else {
-      $warnings[] = pht(
-        'This revision needs review, but there are no reviewers specified.');
-    }
-
-    return id(new AphrontErrorView())
-      ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
-      ->setErrors($warnings);
+    return $warnings;
   }
 
 }
diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php
index 7264ddc89a..088e7beeac 100644
--- a/src/applications/differential/customfield/DifferentialCustomField.php
+++ b/src/applications/differential/customfield/DifferentialCustomField.php
@@ -74,6 +74,14 @@ abstract class DifferentialCustomField
     return array();
   }
 
+  public function getRequiredHandlePHIDsForRevisionHeaderWarnings() {
+    return array();
+  }
+
+  public function getWarningsForRevisionHeader(array $handles) {
+    return array();
+  }
+
 /* -(  Integration with Commit Messages  )----------------------------------- */
 
 
diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php
index eac18ff293..5ff0a2f9f2 100644
--- a/src/applications/differential/customfield/DifferentialReviewersField.php
+++ b/src/applications/differential/customfield/DifferentialReviewersField.php
@@ -191,5 +191,35 @@ final class DifferentialReviewersField
     }
   }
 
+  public function getRequiredHandlePHIDsForRevisionHeaderWarnings() {
+    return mpull($this->getValue(), 'getReviewerPHID');
+  }
+
+  public function getWarningsForRevisionHeader(array $handles) {
+    $revision = $this->getObject();
+
+    $status_needs_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
+    if ($revision->getStatus() != $status_needs_review) {
+      return array();
+    }
+
+    foreach ($this->getValue() as $reviewer) {
+      if (!$handles[$reviewer->getReviewerPHID()]->isDisabled()) {
+        return array();
+      }
+    }
+
+    $warnings = array();
+    if ($this->getValue()) {
+      $warnings[] = pht(
+        'This revision needs review, but all specified reviewers are '.
+        'disabled or inactive.');
+    } else {
+      $warnings[] = pht(
+        'This revision needs review, but there are no reviewers specified.');
+    }
+
+    return $warnings;
+  }
 
 }