Move lint/unit test warning code forward to Transactions

Summary: Ref T2222. Makes the "lint/unit errors" warnings work again.

Test Plan: Viewed some revisions with and without these warnings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

resources/celerity/packages.php

@@ -138,7 +138,6 @@ return array(
     'javelin-behavior-differential-populate',
     'javelin-behavior-differential-show-more',
     'javelin-behavior-differential-diff-radios',
-    'javelin-behavior-differential-accept-with-errors',
     'javelin-behavior-differential-comment-jump',
     'javelin-behavior-differential-add-reviewers-and-ccs',
     'javelin-behavior-differential-keyboard-navigation',

src/__phutil_library_map__.php

@@ -399,7 +399,6 @@ phutil_register_library_map(array(
     'DifferentialLandingToHostedGit' => 'applications/differential/landing/DifferentialLandingToHostedGit.php',
     'DifferentialLandingToHostedMercurial' => 'applications/differential/landing/DifferentialLandingToHostedMercurial.php',
     'DifferentialLintField' => 'applications/differential/customfield/DifferentialLintField.php',
-    'DifferentialLintFieldSpecification' => 'applications/differential/field/specification/DifferentialLintFieldSpecification.php',
     'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php',
     'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php',
     'DifferentialMail' => 'applications/differential/mail/DifferentialMail.php',
@@ -452,7 +451,6 @@ phutil_register_library_map(array(
     'DifferentialTransactionQuery' => 'applications/differential/query/DifferentialTransactionQuery.php',
     'DifferentialTransactionView' => 'applications/differential/view/DifferentialTransactionView.php',
     'DifferentialUnitField' => 'applications/differential/customfield/DifferentialUnitField.php',
-    'DifferentialUnitFieldSpecification' => 'applications/differential/field/specification/DifferentialUnitFieldSpecification.php',
     'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php',
     'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php',
     'DifferentialViewPolicyField' => 'applications/differential/customfield/DifferentialViewPolicyField.php',

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -336,6 +336,19 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $comment_form = new DifferentialAddCommentView();
       $comment_form->setRevision($revision);
 
+      $review_warnings = array();
+      foreach ($field_list->getFields() as $field) {
+        $review_warnings[] = $field->getWarningsForDetailView();
+      }
+      $review_warnings = array_mergev($review_warnings);
+
+      if ($review_warnings) {
+        $review_warnings_panel = id(new AphrontErrorView())
+          ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
+          ->setErrors($review_warnings);
+        $comment_form->setErrorView($review_warnings_panel);
+      }
+
       // TODO: Restore the ability for fields to add accept warnings.
 
       $comment_form->setActions($this->getRevisionCommentActions($revision));

src/applications/differential/customfield/DifferentialCustomField.php

@@ -76,6 +76,12 @@ abstract class DifferentialCustomField
     return implode(', ', $out);
   }
 
+  public function getWarningsForDetailView() {
+    if ($this->getProxy()) {
+      return $this->getProxy()->getWarningsForDetailView();
+    }
+    return array();
+  }
 
 /* -(  Integration with Commit Messages  )----------------------------------- */
 

src/applications/differential/customfield/DifferentialLintField.php

@@ -233,4 +233,25 @@ final class DifferentialLintField
 
     return "Show Full Lint Results (".implode(', ', $show).")";
   }
+
+  public function getWarningsForDetailView() {
+    $status = $this->getObject()->getActiveDiff()->getLintStatus();
+    if ($status < DifferentialLintStatus::LINT_WARN) {
+      return array();
+    }
+
+    $warnings = array();
+    if ($status == DifferentialLintStatus::LINT_SKIP) {
+      $warnings[] = pht(
+        'Lint was skipped when generating these changes.');
+    } else if ($status == DifferentialLintStatus::LINT_POSTPONED) {
+      $warnings[] = pht(
+        'Background linting has not finished executing on these changes.');
+    } else {
+      $warnings[] = pht('These changes have lint problems.');
+    }
+
+    return $warnings;
+  }
+
 }

src/applications/differential/customfield/DifferentialUnitField.php

@@ -203,4 +203,24 @@ final class DifferentialUnitField
     return "Show Full Unit Results (".implode(', ', $show).")";
   }
 
+  public function getWarningsForDetailView() {
+    $status = $this->getObject()->getActiveDiff()->getUnitStatus();
+
+    $warnings = array();
+    if ($status < DifferentialUnitStatus::UNIT_WARN) {
+      // Don't show any warnings.
+    } else if ($status == DifferentialUnitStatus::UNIT_POSTPONED) {
+      $warnings[] = pht(
+        'Background tests have not finished executing on these changes.');
+    } else if ($status == DifferentialUnitStatus::UNIT_SKIP) {
+      $warnings[] = pht(
+        'Unit tests were skipped when generating these changes.');
+    } else {
+      $warnings[] = pht('These changes have unit test problems.');
+    }
+
+    return $warnings;
+  }
+
+
 }

src/applications/differential/field/specification/DifferentialLintFieldSpecification.php

@@ -1,42 +0,0 @@
-<?php
-
-final class DifferentialLintFieldSpecification {
-
-  public function renderWarningBoxForRevisionAccept() {
-    $status = $this->getDiff()->getLintStatus();
-    if ($status < DifferentialLintStatus::LINT_WARN) {
-      return null;
-    }
-
-    $severity = AphrontErrorView::SEVERITY_ERROR;
-    $titles = array(
-      DifferentialLintStatus::LINT_WARN => 'Lint Warning',
-      DifferentialLintStatus::LINT_FAIL => 'Lint Failure',
-      DifferentialLintStatus::LINT_SKIP => 'Lint Skipped',
-      DifferentialLintStatus::LINT_POSTPONED => 'Lint Postponed',
-    );
-
-    if ($status == DifferentialLintStatus::LINT_SKIP) {
-      $content =
-        "This diff was created without running lint. Make sure you are ".
-        "OK with that before you accept this diff.";
-
-    } else if ($status == DifferentialLintStatus::LINT_POSTPONED) {
-      $severity = AphrontErrorView::SEVERITY_WARNING;
-      $content =
-        "Postponed linters didn't finish yet. Make sure you are OK with ".
-        "that before you accept this diff.";
-
-    } else {
-      $content =
-        "This diff has Lint Problems. Make sure you are OK with them ".
-        "before you accept this diff.";
-    }
-
-    return id(new AphrontErrorView())
-      ->setSeverity($severity)
-      ->appendChild(phutil_tag('p', array(), $content))
-      ->setTitle(idx($titles, $status, 'Warning'));
-  }
-
-}

src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php

@@ -1,38 +0,0 @@
-<?php
-
-final class DifferentialUnitFieldSpecification {
-
-  public function renderWarningBoxForRevisionAccept() {
-    $diff = $this->getDiff();
-    $unit_warning = null;
-    if ($diff->getUnitStatus() >= DifferentialUnitStatus::UNIT_WARN) {
-      $titles =
-        array(
-          DifferentialUnitStatus::UNIT_WARN => 'Unit Tests Warning',
-          DifferentialUnitStatus::UNIT_FAIL => 'Unit Tests Failure',
-          DifferentialUnitStatus::UNIT_SKIP => 'Unit Tests Skipped',
-          DifferentialUnitStatus::UNIT_POSTPONED => 'Unit Tests Postponed'
-        );
-      if ($diff->getUnitStatus() == DifferentialUnitStatus::UNIT_POSTPONED) {
-        $content =
-          "This diff has postponed unit tests. The results should be ".
-          "coming in soon. You should probably wait for them before accepting ".
-          "this diff.";
-      } else if ($diff->getUnitStatus() == DifferentialUnitStatus::UNIT_SKIP) {
-        $content =
-          "Unit tests were skipped when this diff was created. Make sure ".
-          "you are OK with that before you accept this diff.";
-      } else {
-        $content =
-          "This diff has Unit Test Problems. Make sure you are OK with ".
-          "them before you accept this diff.";
-      }
-      $unit_warning = id(new AphrontErrorView())
-        ->setSeverity(AphrontErrorView::SEVERITY_ERROR)
-        ->appendChild(phutil_tag('p', array(), $content))
-        ->setTitle(idx($titles, $diff->getUnitStatus(), 'Warning'));
-    }
-    return $unit_warning;
-  }
-
-}

src/applications/differential/view/DifferentialAddCommentView.php

@@ -8,6 +8,16 @@ final class DifferentialAddCommentView extends AphrontView {
   private $draft;
   private $reviewers = array();
   private $ccs = array();
+  private $errorView;
+
+  public function setErrorView(AphrontErrorView $error_view) {
+    $this->errorView = $error_view;
+    return $this;
+  }
+
+  public function getErrorView() {
+    return $this->errorView;
+  }
 
   public function setRevision($revision) {
     $this->revision = $revision;
@@ -129,15 +139,6 @@ final class DifferentialAddCommentView extends AphrontView {
       ));
 
     $diff = $revision->loadActiveDiff();
-    $warnings = array();
-
-    Javelin::initBehavior(
-      'differential-accept-with-errors',
-      array(
-        'select' => 'comment-action',
-        'warnings' => 'warnings',
-      ));
-
     $rev_id = $revision->getID();
 
     Javelin::initBehavior(
@@ -156,13 +157,6 @@ final class DifferentialAddCommentView extends AphrontView {
         'inline'    => 'inline-comment-preview',
       ));
 
-    $warning_container = array();
-    foreach ($warnings as $warning) {
-      if ($warning) {
-        $warning_container[] = $warning->render();
-      }
-    }
-
     $header = id(new PHUIHeaderView())
       ->setHeader($is_serious ? pht('Add Comment') : pht('Leap Into Action'));
 
@@ -170,8 +164,6 @@ final class DifferentialAddCommentView extends AphrontView {
         ->setAnchorName('comment')
         ->setNavigationMarker(true);
 
-    $warn = phutil_tag('div', array('id' => 'warnings'), $warning_container);
-
     $loading = phutil_tag(
       'span',
       array('class' => 'aphront-panel-preview-loading-text'),
@@ -188,9 +180,12 @@ final class DifferentialAddCommentView extends AphrontView {
     $comment_box = id(new PHUIObjectBoxView())
       ->setHeader($header)
       ->appendChild($anchor)
-      ->appendChild($warn)
       ->appendChild($form);
 
+    if ($this->errorView) {
+      $comment_box->setErrorView($this->errorView);
+    }
+
     return array($comment_box, $preview);
   }
 }

webroot/rsrc/js/application/differential/behavior-accept-with-errors.js

@@ -1,25 +0,0 @@
-/**
- * @provides javelin-behavior-differential-accept-with-errors
- * @requires javelin-behavior
- *           javelin-dom
- */
-
-JX.behavior('differential-accept-with-errors', function(config) {
-  if (config.warnings) {
-    toggleWarning();
-    JX.DOM.listen(
-      JX.$(config.select),
-      'change',
-      null,
-      toggleWarning);
-  }
-
-  function toggleWarning() {
-    if (JX.$(config.select).value == 'accept') {
-      JX.DOM.show(JX.$(config.warnings));
-    } else {
-      JX.DOM.hide(JX.$(config.warnings));
-    }
-  }
-
-});