Create option for differential custom fields to display warning on accept

Summary: This adds support to differential fields to display warnings before a revision gets accepted. Since lint and unit are differential fields, the code for their warnings was moved into their respective field specification classes, so there is only one code path for warnings (lint, unit, or custom). Test Plan: Select 'Accept' on a revision with lint/unit warnings and see messages appear like they used to. Change it back to 'Comment' and they go away. Repeat with a revision without lint/unit warnings and see no warnings appear. Checked darkconsole to see no errors due to this. Reviewers: jungejason, epriestley, vrana Reviewed By: epriestley CC: aran, Koolvin Differential Revision: https://secure.phabricator.com/D2363
2025-01-18 10:41:08 +01:00 · 2012-03-26 22:16:46 -07:00 · 2012-03-26 22:16:46 -07:00 · 6039ca6fb5
commit 6039ca6fb5
parent 81dd92fcdc
10 changed files with 118 additions and 78 deletions
--- a/src/__celerity_resource_map__.php
+++ b/src/__celerity_resource_map__.php
@ -857,7 +857,7 @@ celerity_register_resource_map(array(
  ),
  'javelin-behavior-differential-accept-with-errors' =>
  array(
-    'uri' => '/res/41c4685b/rsrc/js/application/differential/behavior-accept-with-errors.js',
+    'uri' => '/res/ba5144c5/rsrc/js/application/differential/behavior-accept-with-errors.js',
    'type' => 'js',
    'requires' =>
    array(
@ -2525,7 +2525,7 @@ celerity_register_resource_map(array(
      'uri' => '/res/pkg/27683aba/differential.pkg.css',
      'type' => 'css',
    ),
-    70509835 =>
+    '5e9ae855' =>
    array(
      'name' => 'differential.pkg.js',
      'symbols' =>
@ -2549,7 +2549,7 @@ celerity_register_resource_map(array(
        16 => 'javelin-behavior-differential-dropdown-menus',
        17 => 'javelin-behavior-buoyant',
      ),
-      'uri' => '/res/pkg/70509835/differential.pkg.js',
+      'uri' => '/res/pkg/5e9ae855/differential.pkg.js',
      'type' => 'js',
    ),
    'c8ce2d88' =>
@ -2657,7 +2657,7 @@ celerity_register_resource_map(array(
    'aphront-typeahead-control-css' => '9c4e265b',
    'differential-changeset-view-css' => '27683aba',
    'differential-core-view-css' => '27683aba',
-    'differential-inline-comment-editor' => '70509835',
+    'differential-inline-comment-editor' => '5e9ae855',
    'differential-local-commits-view-css' => '27683aba',
    'differential-results-table-css' => '27683aba',
    'differential-revision-add-comment-css' => '27683aba',
@ -2670,21 +2670,21 @@ celerity_register_resource_map(array(
    'inline-comment-summary-css' => '27683aba',
    'javelin-behavior' => '8a5de8a3',
    'javelin-behavior-aphront-basic-tokenizer' => '97f65640',
-    'javelin-behavior-aphront-drag-and-drop' => '70509835',
-    'javelin-behavior-aphront-drag-and-drop-textarea' => '70509835',
+    'javelin-behavior-aphront-drag-and-drop' => '5e9ae855',
+    'javelin-behavior-aphront-drag-and-drop-textarea' => '5e9ae855',
    'javelin-behavior-aphront-form-disable-on-submit' => '0c96375e',
    'javelin-behavior-audit-preview' => '5e68be89',
-    'javelin-behavior-buoyant' => '70509835',
-    'javelin-behavior-differential-accept-with-errors' => '70509835',
-    'javelin-behavior-differential-add-reviewers-and-ccs' => '70509835',
-    'javelin-behavior-differential-comment-jump' => '70509835',
-    'javelin-behavior-differential-diff-radios' => '70509835',
-    'javelin-behavior-differential-dropdown-menus' => '70509835',
-    'javelin-behavior-differential-edit-inline-comments' => '70509835',
-    'javelin-behavior-differential-feedback-preview' => '70509835',
-    'javelin-behavior-differential-keyboard-navigation' => '70509835',
-    'javelin-behavior-differential-populate' => '70509835',
-    'javelin-behavior-differential-show-more' => '70509835',
+    'javelin-behavior-buoyant' => '5e9ae855',
+    'javelin-behavior-differential-accept-with-errors' => '5e9ae855',
+    'javelin-behavior-differential-add-reviewers-and-ccs' => '5e9ae855',
+    'javelin-behavior-differential-comment-jump' => '5e9ae855',
+    'javelin-behavior-differential-diff-radios' => '5e9ae855',
+    'javelin-behavior-differential-dropdown-menus' => '5e9ae855',
+    'javelin-behavior-differential-edit-inline-comments' => '5e9ae855',
+    'javelin-behavior-differential-feedback-preview' => '5e9ae855',
+    'javelin-behavior-differential-keyboard-navigation' => '5e9ae855',
+    'javelin-behavior-differential-populate' => '5e9ae855',
+    'javelin-behavior-differential-show-more' => '5e9ae855',
    'javelin-behavior-diffusion-commit-graph' => '5e68be89',
    'javelin-behavior-diffusion-pull-lastmodified' => '5e68be89',
    'javelin-behavior-maniphest-batch-selector' => '7707de41',
@ -2694,12 +2694,12 @@ celerity_register_resource_map(array(
    'javelin-behavior-maniphest-transaction-preview' => '7707de41',
    'javelin-behavior-phabricator-autofocus' => '0c96375e',
    'javelin-behavior-phabricator-keyboard-shortcuts' => '0c96375e',
-    'javelin-behavior-phabricator-object-selector' => '70509835',
+    'javelin-behavior-phabricator-object-selector' => '5e9ae855',
    'javelin-behavior-phabricator-oncopy' => '0c96375e',
    'javelin-behavior-phabricator-tooltips' => '0c96375e',
    'javelin-behavior-phabricator-watch-anchor' => '0c96375e',
    'javelin-behavior-refresh-csrf' => '0c96375e',
-    'javelin-behavior-repository-crossreference' => '70509835',
+    'javelin-behavior-repository-crossreference' => '5e9ae855',
    'javelin-behavior-workflow' => '0c96375e',
    'javelin-dom' => '8a5de8a3',
    'javelin-event' => '8a5de8a3',
@ -2725,7 +2725,7 @@ celerity_register_resource_map(array(
    'phabricator-core-buttons-css' => '9c4e265b',
    'phabricator-core-css' => '9c4e265b',
    'phabricator-directory-css' => '9c4e265b',
-    'phabricator-drag-and-drop-file-upload' => '70509835',
+    'phabricator-drag-and-drop-file-upload' => '5e9ae855',
    'phabricator-dropdown-menu' => '0c96375e',
    'phabricator-flag-css' => '9c4e265b',
    'phabricator-jump-nav' => '9c4e265b',
@ -2737,7 +2737,7 @@ celerity_register_resource_map(array(
    'phabricator-prefab' => '0c96375e',
    'phabricator-project-tag-css' => '7839ae2d',
    'phabricator-remarkup-css' => '9c4e265b',
-    'phabricator-shaped-request' => '70509835',
+    'phabricator-shaped-request' => '5e9ae855',
    'phabricator-standard-page-view' => '9c4e265b',
    'phabricator-tooltip' => '0c96375e',
    'phabricator-transaction-view-css' => '9c4e265b',
--- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php
@ -312,6 +312,7 @@ final class DifferentialRevisionViewController extends DifferentialController {

      $comment_form = new DifferentialAddCommentView();
      $comment_form->setRevision($revision);
+      $comment_form->setAuxFields($aux_fields);
      $comment_form->setActions($this->getRevisionCommentActions($revision));
      $comment_form->setActionURI('/differential/comment/save/');
      $comment_form->setUser($user);
--- a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php
+++ b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php
@ -265,6 +265,18 @@ abstract class DifferentialFieldSpecification {
  }


+  /**
+   * Return a markup block representing a warning to display with the comment
+   * box when preparing to accept a diff. A return value of null indicates no
+   * warning box should be displayed for this field.
+   *
+   * @return string|null Display markup for warning box, or null for no warning
+   */
+  public function renderWarningBoxForRevisionAccept() {
+    return null;
+  }
+
+
 /* -(  Extending the Revision List Interface  )------------------------------ */


--- a/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php
+++ b/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php
@ -197,4 +197,32 @@ final class DifferentialLintFieldSpecification
    return "Show Full Lint Results (".implode(', ', $show).")";
  }

+  public function renderWarningBoxForRevisionAccept() {
+    $diff = $this->getDiff();
+    $lint_warning = null;
+    if ($diff->getLintStatus() >= DifferentialLintStatus::LINT_WARN) {
+      $titles =
+        array(
+          DifferentialLintStatus::LINT_WARN => 'Lint Warning',
+          DifferentialLintStatus::LINT_FAIL => 'Lint Failure',
+          DifferentialLintStatus::LINT_SKIP => 'Lint Skipped'
+        );
+      if ($diff->getLintStatus() == DifferentialLintStatus::LINT_SKIP) {
+        $content =
+          "<p>This diff was created without running lint. Make sure you are ".
+          "OK with that before you accept this diff.</p>";
+      } else {
+        $content =
+          "<p>This diff has Lint Problems. Make sure you are OK with them ".
+          "before you accept this diff.</p>";
+      }
+      $lint_warning = id(new AphrontErrorView())
+        ->setSeverity(AphrontErrorView::SEVERITY_ERROR)
+        ->setWidth(AphrontErrorView::WIDTH_WIDE)
+        ->appendChild($content)
+        ->setTitle(idx($titles, $diff->getLintStatus(), 'Warning'));
+    }
+    return $lint_warning;
+  }
+
 }
--- a/src/applications/differential/field/specification/lint/init.php
+++ b/src/applications/differential/field/specification/lint/init.php
@ -8,9 +8,11 @@

 phutil_require_module('arcanist', 'lint/severity');

+phutil_require_module('phabricator', 'applications/differential/constants/lintstatus');
 phutil_require_module('phabricator', 'applications/differential/field/specification/base');
 phutil_require_module('phabricator', 'applications/differential/view/resultstableview');
 phutil_require_module('phabricator', 'applications/differential/view/revisionupdatehistory');
+phutil_require_module('phabricator', 'view/form/error');

 phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
--- a/src/applications/differential/field/specification/unit/DifferentialUnitFieldSpecification.php
+++ b/src/applications/differential/field/specification/unit/DifferentialUnitFieldSpecification.php
@ -182,4 +182,38 @@ final class DifferentialUnitFieldSpecification
    return "Show Full Unit Results (".implode(', ', $show).")";
  }

+  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 =
+          "<p>This diff has postponed unit tests. The results should be ".
+          "coming in soon. You should probably wait for them before accepting ".
+          "this diff.</p>";
+      } else if ($diff->getUnitStatus() == DifferentialUnitStatus::UNIT_SKIP) {
+        $content =
+          "<p>Unit tests were skipped when this diff was created. Make sure ".
+          "you are OK with that before you accept this diff.</p>";
+      } else {
+        $content =
+          "<p>This diff has Unit Test Problems. Make sure you are OK with ".
+          "them before you accept this diff.</p>";
+      }
+      $unit_warning = id(new AphrontErrorView())
+        ->setSeverity(AphrontErrorView::SEVERITY_ERROR)
+        ->setWidth(AphrontErrorView::WIDTH_WIDE)
+        ->appendChild($content)
+        ->setTitle(idx($titles, $diff->getUnitStatus(), 'Warning'));
+    }
+    return $unit_warning;
+  }
+
 }
--- a/src/applications/differential/field/specification/unit/init.php
+++ b/src/applications/differential/field/specification/unit/init.php
@ -8,10 +8,12 @@

 phutil_require_module('arcanist', 'unit/result');

+phutil_require_module('phabricator', 'applications/differential/constants/unitstatus');
 phutil_require_module('phabricator', 'applications/differential/field/specification/base');
 phutil_require_module('phabricator', 'applications/differential/view/resultstableview');
 phutil_require_module('phabricator', 'applications/differential/view/revisionupdatehistory');
 phutil_require_module('phabricator', 'applications/markup/engine');
+phutil_require_module('phabricator', 'view/form/error');

 phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
--- a/src/applications/differential/view/addcomment/DifferentialAddCommentView.php
+++ b/src/applications/differential/view/addcomment/DifferentialAddCommentView.php
@ -23,12 +23,19 @@ final class DifferentialAddCommentView extends AphrontView {
  private $actionURI;
  private $user;
  private $draft;
+  private $auxFields;

  public function setRevision($revision) {
    $this->revision = $revision;
    return $this;
  }

+  public function setAuxFields(array $aux_fields) {
+    assert_instances_of($aux_fields, 'DifferentialFieldSpecification');
+    $this->auxFields = $aux_fields;
+    return $this;
+  }
+
  public function setActions(array $actions) {
    $this->actions = $actions;
    return $this;
@ -144,66 +151,15 @@ final class DifferentialAddCommentView extends AphrontView {
      ));

    $diff = $revision->loadActiveDiff();
+    $warnings = mpull($this->auxFields, 'renderWarningBoxForRevisionAccept');
    $lint_warning = null;
    $unit_warning = null;
-    if ($diff->getLintStatus() >= DifferentialLintStatus::LINT_WARN) {
-      $titles =
-        array(
-          DifferentialLintStatus::LINT_WARN => 'Lint Warning',
-          DifferentialLintStatus::LINT_FAIL => 'Lint Failure',
-          DifferentialLintStatus::LINT_SKIP => 'Lint Skipped'
-        );
-      if ($diff->getLintStatus() == DifferentialLintStatus::LINT_SKIP) {
-        $content =
-          "<p>This diff was created without running lint. Make sure you are ".
-          "OK with that before you accept this diff.</p>";
-      } else {
-        $content =
-          "<p>This diff has Lint Problems. Make sure you are OK with them ".
-          "before you accept this diff.</p>";
-      }
-      $lint_warning = $this->generateWarningView(
-        $diff->getLintStatus(),
-        $titles,
-        'lint-warning',
-        $content);
-    }
-
-    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 =
-          "<p>This diff has postponed unit tests. The results should be ".
-          "coming in soon. You should probably wait for them before accepting ".
-          "this diff.</p>";
-      } else if ($diff->getUnitStatus() == DifferentialUnitStatus::UNIT_SKIP) {
-        $content =
-          "<p>Unit tests were skipped when this diff was created. Make sure ".
-          "you are OK with that before you accept this diff.</p>";
-      } else {
-        $content =
-          "<p>This diff has Unit Test Problems. Make sure you are OK with ".
-          "them before you accept this diff.</p>";
-      }
-      $unit_warning = $this->generateWarningView(
-        $diff->getUnitStatus(),
-        $titles,
-        'unit-warning',
-        $content);
-    }

    Javelin::initBehavior(
      'differential-accept-with-errors',
      array(
        'select' => 'comment-action',
-        'lint_warning' => $lint_warning ? 'lint-warning' : null,
-        'unit_warning' => $unit_warning ? 'unit-warning' : null,
+        'warnings' => 'warnings',
      ));

    $rev_id = $revision->getID();
@ -226,6 +182,14 @@ final class DifferentialAddCommentView extends AphrontView {

    $panel_view = new AphrontPanelView();
    $panel_view->appendChild($form);
+    $warning_container = '<div id="warnings">';
+    foreach ($warnings as $warning) {
+      if ($warning) {
+        $warning_container .= $warning->render();
+      }
+    }
+    $warning_container .= '</div>';
+    $panel_view->appendChild($warning_container);
    if ($lint_warning) {
      $panel_view->appendChild($lint_warning);
    }
--- a/src/applications/differential/view/addcomment/init.php
+++ b/src/applications/differential/view/addcomment/init.php
@ -6,8 +6,6 @@



-phutil_require_module('phabricator', 'applications/differential/constants/lintstatus');
-phutil_require_module('phabricator', 'applications/differential/constants/unitstatus');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
 phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'infrastructure/javelin/api');
--- a/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js
+++ b/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js
@ -23,6 +23,5 @@ JX.behavior('differential-accept-with-errors', function(config) {
    }
  }

-  toggleWarning(config.unit_warning);
-  toggleWarning(config.lint_warning);
+  toggleWarning(config.warnings);
 });