From 6039ca6fb554bedf1b3fb835347c6fdae2fc2587 Mon Sep 17 00:00:00 2001 From: Nick Harper Date: Mon, 26 Mar 2012 22:16:46 -0700 Subject: [PATCH] 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 --- src/__celerity_resource_map__.php | 42 +++++------ .../DifferentialRevisionViewController.php | 1 + .../base/DifferentialFieldSpecification.php | 12 ++++ .../DifferentialLintFieldSpecification.php | 28 ++++++++ .../field/specification/lint/__init__.php | 2 + .../DifferentialUnitFieldSpecification.php | 34 +++++++++ .../field/specification/unit/__init__.php | 2 + .../addcomment/DifferentialAddCommentView.php | 70 +++++-------------- .../differential/view/addcomment/__init__.php | 2 - .../behavior-accept-with-errors.js | 3 +- 10 files changed, 118 insertions(+), 78 deletions(-) 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); });