From 501c001520eab5616edd4dfd70278eac59d3c21b Mon Sep 17 00:00:00 2001 From: tuomaspelkonen Date: Thu, 9 Jun 2011 18:56:47 -0700 Subject: [PATCH] Added a big warning if reviewer is about to accept a diff with lint or unit errors. Summary: Make sure reviewers know what they are doing. Test Plan: Tested with different diffs that had lint and unit problems. Reviewed By: epriestley Reviewers: epriestley, jungejason CC: grglr, aran, epriestley, tuomaspelkonen Differential Revision: 432 --- src/__celerity_resource_map__.php | 67 ++++++++++-------- .../addcomment/DifferentialAddCommentView.php | 68 +++++++++++++++++++ .../differential/view/addcomment/__init__.php | 3 + src/view/form/error/AphrontErrorView.php | 17 +++-- .../behavior-accept-with-errors.js | 28 ++++++++ 5 files changed, 151 insertions(+), 32 deletions(-) create mode 100644 webroot/rsrc/js/application/differential/behavior-accept-with-errors.js diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index c77c2c199c..ef2ed1f58b 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -16,15 +16,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/aphront/attached-file-view.css', ), - 'aphront-crumbs-view-css' => - array( - 'uri' => '/res/9009e6bd/rsrc/css/aphront/crumbs-view.css', - 'type' => 'css', - 'requires' => - array( - ), - 'disk' => '/rsrc/css/aphront/crumbs-view.css', - ), 'aphront-dark-console-css' => array( 'uri' => '/res/e7011594/rsrc/css/aphront/dark-console.css', @@ -34,15 +25,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/aphront/dark-console.css', ), - 'aphront-dialog-view-css' => - array( - 'uri' => '/res/61a58113/rsrc/css/aphront/dialog-view.css', - 'type' => 'css', - 'requires' => - array( - ), - 'disk' => '/rsrc/css/aphront/dialog-view.css', - ), 'aphront-error-view-css' => array( 'uri' => '/res/e4c5e4ed/rsrc/css/aphront/error-view.css', @@ -52,6 +34,34 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/aphront/error-view.css', ), + 0 => + array( + 'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-install', + ), + 'disk' => '/rsrc/js/javelin/docs/Base.js', + ), + 'aphront-crumbs-view-css' => + array( + 'uri' => '/res/9009e6bd/rsrc/css/aphront/crumbs-view.css', + 'type' => 'css', + 'requires' => + array( + ), + 'disk' => '/rsrc/css/aphront/crumbs-view.css', + ), + 'aphront-dialog-view-css' => + array( + 'uri' => '/res/61a58113/rsrc/css/aphront/dialog-view.css', + 'type' => 'css', + 'requires' => + array( + ), + 'disk' => '/rsrc/css/aphront/dialog-view.css', + ), 'aphront-form-view-css' => array( 'uri' => '/res/8ee16aba/rsrc/css/aphront/form-view.css', @@ -360,6 +370,17 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/core/behavior-dark-console.js', ), + 'javelin-behavior-differential-accept-with-errors' => + array( + 'uri' => '/res/41c4685b/rsrc/js/application/differential/behavior-accept-with-errors.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-dom', + ), + 'disk' => '/rsrc/js/application/differential/behavior-accept-with-errors.js', + ), 'javelin-behavior-differential-add-reviewers' => array( 'uri' => '/res/dc79790c/rsrc/js/application/differential/behavior-add-reviewers.js', @@ -590,16 +611,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/owners/owners-path-editor.js', ), - 0 => - array( - 'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-install', - ), - 'disk' => '/rsrc/js/javelin/docs/Base.js', - ), 'javelin-behavior-phabricator-keyboard-shortcuts' => array( 'uri' => '/res/5a23bcc8/rsrc/js/application/core/behavior-keyboard-shortcuts.js', diff --git a/src/applications/differential/view/addcomment/DifferentialAddCommentView.php b/src/applications/differential/view/addcomment/DifferentialAddCommentView.php index 30d582a30c..5ebadc8699 100644 --- a/src/applications/differential/view/addcomment/DifferentialAddCommentView.php +++ b/src/applications/differential/view/addcomment/DifferentialAddCommentView.php @@ -47,6 +47,22 @@ final class DifferentialAddCommentView extends AphrontView { return $this; } + private function generateWarningView( + $status, + array $titles, + $id, + $content) { + + $warning = new AphrontErrorView(); + $warning->setSeverity(AphrontErrorView::SEVERITY_ERROR); + $warning->setWidth(AphrontErrorView::WIDTH_WIDE); + $warning->setID($id); + $warning->appendChild($content); + $warning->setTitle(idx($titles, $status, 'Warning')); + + return $warning; + } + public function render() { require_celerity_resource('differential-revision-add-comment-css'); @@ -96,6 +112,52 @@ final class DifferentialAddCommentView extends AphrontView { 'row' => 'add-reviewers', )); + $diff = $revision->loadActiveDiff(); + $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' + ); + $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' + ); + $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, + )); + $rev_id = $revision->getID(); Javelin::initBehavior( @@ -112,6 +174,12 @@ final class DifferentialAddCommentView extends AphrontView { $panel_view = new AphrontPanelView(); $panel_view->appendChild($form); + if ($lint_warning) { + $panel_view->appendChild($lint_warning); + } + if ($unit_warning) { + $panel_view->appendChild($unit_warning); + } $panel_view->setHeader('Leap Into Action'); $panel_view->addClass('aphront-panel-accent'); $panel_view->addClass('aphront-panel-flush'); diff --git a/src/applications/differential/view/addcomment/__init__.php b/src/applications/differential/view/addcomment/__init__.php index 4af0523ead..6ab7fb2cda 100644 --- a/src/applications/differential/view/addcomment/__init__.php +++ b/src/applications/differential/view/addcomment/__init__.php @@ -7,6 +7,8 @@ phutil_require_module('phabricator', 'applications/differential/constants/action'); +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/javelin/api'); phutil_require_module('phabricator', 'view/base'); @@ -15,6 +17,7 @@ phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/control/textarea'); phutil_require_module('phabricator', 'view/form/control/tokenizer'); +phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phutil', 'utils'); diff --git a/src/view/form/error/AphrontErrorView.php b/src/view/form/error/AphrontErrorView.php index 47fa413b2c..e9971e03e0 100644 --- a/src/view/form/error/AphrontErrorView.php +++ b/src/view/form/error/AphrontErrorView.php @@ -30,6 +30,7 @@ final class AphrontErrorView extends AphrontView { private $errors; private $severity; private $width; + private $id; public function setTitle($title) { $this->title = $title; @@ -51,6 +52,11 @@ final class AphrontErrorView extends AphrontView { return $this; } + public function setID($id) { + $this->id = $id; + return $this; + } + final public function render() { require_celerity_resource('aphront-error-view-css'); @@ -85,11 +91,14 @@ final class AphrontErrorView extends AphrontView { $more_classes = implode(' ', $more_classes); return - '
'. + phutil_render_tag( + 'div', + array( + 'id' => $this->id, + 'class' => 'aphront-error-view '.$more_classes, + ), $title. $this->renderChildren(). - $list. - '
'; - + $list); } } diff --git a/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js b/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js new file mode 100644 index 0000000000..45b3c7693c --- /dev/null +++ b/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js @@ -0,0 +1,28 @@ +/** + * @provides javelin-behavior-differential-accept-with-errors + * @requires javelin-behavior + * javelin-dom + */ + +JX.behavior('differential-accept-with-errors', function(config) { + + function toggleWarning(control) { + if (control) { + JX.DOM.hide(JX.$(control)); + JX.DOM.listen( + JX.$(config.select), + 'change', + null, + function(e) { + if (JX.$(config.select).value == 'accept') { + JX.DOM.show(JX.$(control)); + } else { + JX.DOM.hide(JX.$(control)); + } + }); + } + } + + toggleWarning(config.unit_warning); + toggleWarning(config.lint_warning); +});