diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 98e5e3997f..a7519fa877 100644 --- 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', diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 9fe5d9761b..82926b5bd2 100644 --- 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); diff --git a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php index e0953b1a3b..7237c9b1e3 100644 --- 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 )------------------------------ */ diff --git a/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php b/src/applications/differential/field/specification/lint/DifferentialLintFieldSpecification.php index f76c74ab69..b1154ba128 100644 --- 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 = + "

This diff was created without running lint. 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.

"; + } + $lint_warning = id(new AphrontErrorView()) + ->setSeverity(AphrontErrorView::SEVERITY_ERROR) + ->setWidth(AphrontErrorView::WIDTH_WIDE) + ->appendChild($content) + ->setTitle(idx($titles, $diff->getLintStatus(), 'Warning')); + } + return $lint_warning; + } + } diff --git a/src/applications/differential/field/specification/lint/__init__.php b/src/applications/differential/field/specification/lint/__init__.php index d8295f4821..09352cfde5 100644 --- 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'); diff --git a/src/applications/differential/field/specification/unit/DifferentialUnitFieldSpecification.php b/src/applications/differential/field/specification/unit/DifferentialUnitFieldSpecification.php index 7d72734578..1bd0574642 100644 --- 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 = + "

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) + ->setWidth(AphrontErrorView::WIDTH_WIDE) + ->appendChild($content) + ->setTitle(idx($titles, $diff->getUnitStatus(), 'Warning')); + } + return $unit_warning; + } + } diff --git a/src/applications/differential/field/specification/unit/__init__.php b/src/applications/differential/field/specification/unit/__init__.php index fa65658b2e..2c8522b008 100644 --- 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'); diff --git a/src/applications/differential/view/addcomment/DifferentialAddCommentView.php b/src/applications/differential/view/addcomment/DifferentialAddCommentView.php index 97a425eda6..a9fa4afb63 100644 --- 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 = - "

This diff was created without running lint. 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.

"; - } - $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 = - "

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 = $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 = '
'; + foreach ($warnings as $warning) { + if ($warning) { + $warning_container .= $warning->render(); + } + } + $warning_container .= '
'; + $panel_view->appendChild($warning_container); if ($lint_warning) { $panel_view->appendChild($lint_warning); } diff --git a/src/applications/differential/view/addcomment/__init__.php b/src/applications/differential/view/addcomment/__init__.php index fed62292bc..3eae5581d2 100644 --- 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'); diff --git a/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js b/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js index 45b3c7693c..e1c434f1fb 100644 --- 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); });