From a49fec39be84ed255a534a49a2788ddd92b91c6c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 9 Mar 2014 13:18:17 -0700 Subject: [PATCH] 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 | 1 - src/__phutil_library_map__.php | 2 - .../DifferentialRevisionViewController.php | 13 ++++++ .../customfield/DifferentialCustomField.php | 6 +++ .../customfield/DifferentialLintField.php | 21 ++++++++++ .../customfield/DifferentialUnitField.php | 20 +++++++++ .../DifferentialLintFieldSpecification.php | 42 ------------------- .../DifferentialUnitFieldSpecification.php | 38 ----------------- .../view/DifferentialAddCommentView.php | 33 +++++++-------- .../behavior-accept-with-errors.js | 25 ----------- 10 files changed, 74 insertions(+), 127 deletions(-) delete mode 100644 src/applications/differential/field/specification/DifferentialLintFieldSpecification.php delete mode 100644 src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php delete mode 100644 webroot/rsrc/js/application/differential/behavior-accept-with-errors.js diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index da471882ad..3633e722c7 100644 --- a/resources/celerity/packages.php +++ b/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', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ed64f026f5..8552113e3d 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 026f664dad..78781bc352 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/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)); diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php index 3ad9e56a0c..97f9a72da3 100644 --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/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 )----------------------------------- */ diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index e9102dd3ce..b726089715 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/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; + } + } diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index ab3daf6e62..fb813f7077 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/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; + } + + } diff --git a/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php b/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php deleted file mode 100644 index ecacb4fd5f..0000000000 --- a/src/applications/differential/field/specification/DifferentialLintFieldSpecification.php +++ /dev/null @@ -1,42 +0,0 @@ -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')); - } - -} diff --git a/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php b/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php deleted file mode 100644 index 58f14d9b9d..0000000000 --- a/src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php +++ /dev/null @@ -1,38 +0,0 @@ -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; - } - -} diff --git a/src/applications/differential/view/DifferentialAddCommentView.php b/src/applications/differential/view/DifferentialAddCommentView.php index f261f0ff90..1c39882b6d 100644 --- a/src/applications/differential/view/DifferentialAddCommentView.php +++ b/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); } } diff --git a/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js b/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js deleted file mode 100644 index 676967f6fc..0000000000 --- a/webroot/rsrc/js/application/differential/behavior-accept-with-errors.js +++ /dev/null @@ -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)); - } - } - -});