From 3706dcb5c712bfe7d93d7aa472be1139a2a5022a Mon Sep 17 00:00:00 2001 From: Florian Date: Fri, 1 Jul 2016 18:26:20 +0200 Subject: [PATCH] Show warnings in HTMLForm and warnings as warnings on Login/Signup form This commit changes the way how HTMLForm handles a Status object when executed from a request. It now handles, beside the errors, also the warnings of a Status object and prints them out, wrapped in a warning box. The LoginSignupPage uses this feature to show informative warnings actually as warnings and not as more disturbing error messages. Error messages should be reserved for errors and only for erros. An AuthenticationProvider, which returns an UI AuthenticationResponse can choose, if the given message is an error or a warning message. This commit also addds a new function to Status, which allows a developer to split the object into two new Status objects, where one only contains the errors and the other only the warnings of the origin Status object (splitByErrorType). StatusValue also has a new function, splitByErrorType(), to support this. Bug: T139179 Change-Id: I9a27911613e62b5c4cb86bea40696cb37c4f49c2 --- includes/Status.php | 21 +++++++ includes/auth/AuthenticationResponse.php | 11 +++- ...irmLinkSecondaryAuthenticationProvider.php | 6 +- ...asswordSecondaryAuthenticationProvider.php | 6 +- includes/htmlform/HTMLForm.php | 46 ++++++++++---- includes/htmlform/OOUIHTMLForm.php | 35 ++++++++--- includes/libs/StatusValue.php | 28 +++++++++ .../specialpage/LoginSignupSpecialPage.php | 10 ++- .../mediawiki.ui/variables.less | 1 + .../src/mediawiki.ui/components/forms.less | 9 ++- tests/phpunit/includes/StatusTest.php | 62 +++++++++++++++++++ .../auth/AuthenticationResponseTest.php | 17 +++++ 12 files changed, 222 insertions(+), 30 deletions(-) diff --git a/includes/Status.php b/includes/Status.php index e5788733cf..a6348b7165 100644 --- a/includes/Status.php +++ b/includes/Status.php @@ -105,6 +105,26 @@ class Status { return new self( $sv ); } + /** + * Splits this Status object into two new Status objects, one which contains only + * the error messages, and one that contains the warnings, only. The returned array is + * defined as: + * array( + * 0 => object(Status) # the Status with error messages, only + * 1 => object(Status) # The Status with warning messages, only + * ) + * + * @return array + */ + public function splitByErrorType() { + list( $errorsOnlyStatusValue, $warningsOnlyStatusValue ) = $this->sv->splitByErrorType(); + $errorsOnlyStatus = new Status( $errorsOnlyStatusValue ); + $warningsOnlyStatus = new Status( $warningsOnlyStatusValue ); + $errorsOnlyStatus->cleanCallback = $warningsOnlyStatus->cleanCallback = $this->cleanCallback; + + return [ $errorsOnlyStatus, $warningsOnlyStatus ]; + } + /** * Change operation result * @@ -314,6 +334,7 @@ class Status { /** * Return the message for a single error. + * * @param mixed $error With an array & two values keyed by * 'message' and 'params', use those keys-value pairs. * Otherwise, if its an array, just use the first value as the diff --git a/includes/auth/AuthenticationResponse.php b/includes/auth/AuthenticationResponse.php index 0339e451b3..6684fb958f 100644 --- a/includes/auth/AuthenticationResponse.php +++ b/includes/auth/AuthenticationResponse.php @@ -81,6 +81,9 @@ class AuthenticationResponse { /** @var Message|null I18n message to display in case of UI or FAIL */ public $message = null; + /** @var string Whether the $message is an error or warning message, for styling reasons */ + public $messageType = 'warning'; + /** * @var string|null Local user name from authentication. * May be null if the authentication passed but no local user is known. @@ -144,6 +147,7 @@ class AuthenticationResponse { $ret = new AuthenticationResponse; $ret->status = AuthenticationResponse::FAIL; $ret->message = $msg; + $ret->messageType = 'error'; return $ret; } @@ -172,18 +176,23 @@ class AuthenticationResponse { /** * @param AuthenticationRequest[] $reqs AuthenticationRequests needed to continue * @param Message $msg + * @param string $msgtype * @return AuthenticationResponse * @see AuthenticationResponse::UI */ - public static function newUI( array $reqs, Message $msg ) { + public static function newUI( array $reqs, Message $msg, $msgtype = 'warning' ) { if ( !$reqs ) { throw new \InvalidArgumentException( '$reqs may not be empty' ); } + if ( $msgtype !== 'warning' && $msgtype !== 'error' ) { + throw new \InvalidArgumentException( $msgtype . ' is not a valid message type.' ); + } $ret = new AuthenticationResponse; $ret->status = AuthenticationResponse::UI; $ret->neededRequests = $reqs; $ret->message = $msg; + $ret->messageType = $msgtype; return $ret; } diff --git a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php index beb11f4331..7f121cdef1 100644 --- a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php +++ b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php @@ -64,7 +64,8 @@ class ConfirmLinkSecondaryAuthenticationProvider extends AbstractSecondaryAuthen $req = new ConfirmLinkAuthenticationRequest( $maybeLink ); return AuthenticationResponse::newUI( [ $req ], - wfMessage( 'authprovider-confirmlink-message' ) + wfMessage( 'authprovider-confirmlink-message' ), + 'warning' ); } @@ -150,7 +151,8 @@ class ConfirmLinkSecondaryAuthenticationProvider extends AbstractSecondaryAuthen 'linkOk', wfMessage( 'ok' ), wfMessage( 'authprovider-confirmlink-ok-help' ) ) ], - $combinedStatus->getMessage( 'authprovider-confirmlink-failed' ) + $combinedStatus->getMessage( 'authprovider-confirmlink-failed' ), + 'error' ); } } diff --git a/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php b/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php index dd97830dae..f11a12c421 100644 --- a/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php +++ b/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php @@ -112,17 +112,17 @@ class ResetPasswordSecondaryAuthenticationProvider extends AbstractSecondaryAuth $req = AuthenticationRequest::getRequestByClass( $reqs, get_class( $needReq ) ); if ( !$req || !array_key_exists( 'retype', $req->getFieldInfo() ) ) { - return AuthenticationResponse::newUI( $needReqs, $data->msg ); + return AuthenticationResponse::newUI( $needReqs, $data->msg, 'warning' ); } if ( $req->password !== $req->retype ) { - return AuthenticationResponse::newUI( $needReqs, new \Message( 'badretype' ) ); + return AuthenticationResponse::newUI( $needReqs, new \Message( 'badretype' ), 'error' ); } $req->username = $user->getName(); $status = $this->manager->allowsAuthenticationDataChange( $req ); if ( !$status->isGood() ) { - return AuthenticationResponse::newUI( $needReqs, $status->getMessage() ); + return AuthenticationResponse::newUI( $needReqs, $status->getMessage(), 'error' ); } $this->manager->changeAuthenticationData( $req ); diff --git a/includes/htmlform/HTMLForm.php b/includes/htmlform/HTMLForm.php index 3c88594eac..567e692edd 100644 --- a/includes/htmlform/HTMLForm.php +++ b/includes/htmlform/HTMLForm.php @@ -1014,7 +1014,8 @@ class HTMLForm extends ContextSource { $this->getOutput()->addModuleStyles( 'mediawiki.htmlform.styles' ); $html = '' - . $this->getErrors( $submitResult ) + . $this->getErrorsOrWarnings( $submitResult, 'error' ) + . $this->getErrorsOrWarnings( $submitResult, 'warning' ) . $this->getHeaderText() . $this->getBody() . $this->getHiddenFields() @@ -1230,23 +1231,46 @@ class HTMLForm extends ContextSource { * * @param string|array|Status $errors * + * @deprecated since 1.28, use getErrorsOrWarnings() instead + * * @return string */ public function getErrors( $errors ) { - if ( $errors instanceof Status ) { - if ( $errors->isOK() ) { - $errorstr = ''; + wfDeprecated( __METHOD__ ); + return $this->getErrorsOrWarnings( $errors, 'error' ); + } + + /** + * Returns a formatted list of errors or warnings from the given elements. + * + * @param string|array|Status $elements The set of errors/warnings to process. + * @param string $elementsType Should warnings or errors be returned. This is meant + * for Status objects, all other valid types are always considered as errors. + * @return string + */ + public function getErrorsOrWarnings( $elements, $elementsType ) { + if ( !in_array( $elementsType, [ 'error', 'warning' ], true ) ) { + throw new DomainException( $elementsType . ' is not a valid type.' ); + } + $elementstr = false; + if ( $elements instanceof Status ) { + list( $errorStatus, $warningStatus ) = $elements->splitByErrorType(); + $status = $elementsType === 'error' ? $errorStatus : $warningStatus; + if ( $status->isGood() ) { + $elementstr = ''; } else { - $errorstr = $this->getOutput()->parse( $errors->getWikiText() ); + $elementstr = $this->getOutput()->parse( + $status->getWikiText() + ); } - } elseif ( is_array( $errors ) ) { - $errorstr = $this->formatErrors( $errors ); - } else { - $errorstr = $errors; + } elseif ( is_array( $elements ) && $elementsType === 'error' ) { + $elementstr = $this->formatErrors( $elements ); + } elseif ( $elementsType === 'error' ) { + $elementstr = $elements; } - return $errorstr - ? Html::rawElement( 'div', [ 'class' => 'error' ], $errorstr ) + return $elementstr + ? Html::rawElement( 'div', [ 'class' => $elementsType ], $elementstr ) : ''; } diff --git a/includes/htmlform/OOUIHTMLForm.php b/includes/htmlform/OOUIHTMLForm.php index 0b227278a3..bbd3473af9 100644 --- a/includes/htmlform/OOUIHTMLForm.php +++ b/includes/htmlform/OOUIHTMLForm.php @@ -26,6 +26,7 @@ */ class OOUIHTMLForm extends HTMLForm { private $oouiErrors; + private $oouiWarnings; public function __construct( $descriptor, $context = null, $messagePrefix = '' ) { parent::__construct( $descriptor, $context, $messagePrefix ); @@ -185,28 +186,34 @@ class OOUIHTMLForm extends HTMLForm { } /** - * @param string|array|Status $err + * @param string|array|Status $elements + * @param string $elementsType * @return string */ - function getErrors( $err ) { - if ( !$err ) { + function getErrorsOrWarnings( $elements, $elementsType ) { + if ( !in_array( $elementsType, [ 'error', 'warning' ] ) ) { + throw new DomainException( $elementsType . ' is not a valid type.' ); + } + if ( !$elements ) { $errors = []; - } elseif ( $err instanceof Status ) { - if ( $err->isOK() ) { + } elseif ( $elements instanceof Status ) { + if ( $elements->isGood() ) { $errors = []; } else { - $errors = $err->getErrorsByType( 'error' ); + $errors = $elements->getErrorsByType( $elementsType ); foreach ( $errors as &$error ) { // Input: array( 'message' => 'foo', 'errors' => array( 'a', 'b', 'c' ) ) // Output: array( 'foo', 'a', 'b', 'c' ) $error = array_merge( [ $error['message'] ], $error['params'] ); } } - } else { - $errors = $err; + } elseif ( $elementsType === 'errors' ) { + $errors = $elements; if ( !is_array( $errors ) ) { $errors = [ $errors ]; } + } else { + $errors = []; } foreach ( $errors as &$error ) { @@ -215,7 +222,11 @@ class OOUIHTMLForm extends HTMLForm { } // Used in getBody() - $this->oouiErrors = $errors; + if ( $elementsType === 'error' ) { + $this->oouiErrors = $errors; + } else { + $this->oouiWarnings = $errors; + } return ''; } @@ -236,7 +247,10 @@ class OOUIHTMLForm extends HTMLForm { if ( $this->oouiErrors ) { $classes[] = 'mw-htmlform-ooui-header-errors'; } - if ( $this->mHeader || $this->oouiErrors ) { + if ( $this->oouiWarnings ) { + $classes[] = 'mw-htmlform-ooui-header-warnings'; + } + if ( $this->mHeader || $this->oouiErrors || $this->oouiWarnings ) { // if there's no header, don't create an (empty) LabelWidget, simply use a placeholder if ( $this->mHeader ) { $element = new OOUI\LabelWidget( [ 'label' => new OOUI\HtmlSnippet( $this->mHeader ) ] ); @@ -249,6 +263,7 @@ class OOUIHTMLForm extends HTMLForm { [ 'align' => 'top', 'errors' => $this->oouiErrors, + 'notices' => $this->oouiWarnings, 'classes' => $classes, ] ) diff --git a/includes/libs/StatusValue.php b/includes/libs/StatusValue.php index 1d23f9d368..f45dc41248 100644 --- a/includes/libs/StatusValue.php +++ b/includes/libs/StatusValue.php @@ -79,6 +79,34 @@ class StatusValue { return $result; } + /** + * Splits this StatusValue object into two new StatusValue objects, one which contains only + * the error messages, and one that contains the warnings, only. The returned array is + * defined as: + * array( + * 0 => object(StatusValue) # the StatusValue with error messages, only + * 1 => object(StatusValue) # The StatusValue with warning messages, only + * ) + * + * @return array + */ + public function splitByErrorType() { + $errorsOnlyStatusValue = clone $this; + $warningsOnlyStatusValue = clone $this; + $warningsOnlyStatusValue->ok = true; + + $errorsOnlyStatusValue->errors = $warningsOnlyStatusValue->errors = []; + foreach ( $this->errors as $item ) { + if ( $item['type'] === 'warning' ) { + $warningsOnlyStatusValue->errors[] = $item; + } else { + $errorsOnlyStatusValue->errors[] = $item; + } + }; + + return [ $errorsOnlyStatusValue, $warningsOnlyStatusValue ]; + } + /** * Returns whether the operation completed and didn't have any error or * warnings diff --git a/includes/specialpage/LoginSignupSpecialPage.php b/includes/specialpage/LoginSignupSpecialPage.php index c3d43df266..9e9397017c 100644 --- a/includes/specialpage/LoginSignupSpecialPage.php +++ b/includes/specialpage/LoginSignupSpecialPage.php @@ -354,7 +354,7 @@ abstract class LoginSignupSpecialPage extends AuthManagerSpecialPage { $this->authAction = $this->isSignup() ? AuthManager::ACTION_CREATE_CONTINUE : AuthManager::ACTION_LOGIN_CONTINUE; $this->authRequests = $response->neededRequests; - $this->mainLoginForm( $response->neededRequests, $response->message, 'warning' ); + $this->mainLoginForm( $response->neededRequests, $response->message, $response->messageType ); break; default: throw new LogicException( 'invalid AuthenticationResponse' ); @@ -494,7 +494,13 @@ abstract class LoginSignupSpecialPage extends AuthManagerSpecialPage { $form = $this->getAuthForm( $requests, $this->authAction, $msg, $msgtype ); $form->prepareForm(); - $formHtml = $form->getHTML( $msg ? Status::newFatal( $msg ) : false ); + $submitStatus = Status::newGood(); + if ( $msg && $msgtype === 'warning' ) { + $submitStatus->warning( $msg ); + } elseif ( $msg && $msgtype === 'error' ) { + $submitStatus->fatal( $msg ); + } + $formHtml = $form->getHTML( $submitStatus ); $out->addHTML( $this->getPageHtml( $formHtml ) ); } diff --git a/resources/src/mediawiki.less/mediawiki.ui/variables.less b/resources/src/mediawiki.less/mediawiki.ui/variables.less index 507109ae2c..b6f656878c 100644 --- a/resources/src/mediawiki.less/mediawiki.ui/variables.less +++ b/resources/src/mediawiki.less/mediawiki.ui/variables.less @@ -51,6 +51,7 @@ @colorButtonTextActive: @colorGray7; @colorDisabledText: @colorGray12; @colorErrorText: #c00; +@colorWarningText: #705000; // UI colors @colorFieldBorder: @colorGray12; diff --git a/resources/src/mediawiki.ui/components/forms.less b/resources/src/mediawiki.ui/components/forms.less index cc96a5c09c..aedec5b9e9 100644 --- a/resources/src/mediawiki.ui/components/forms.less +++ b/resources/src/mediawiki.ui/components/forms.less @@ -103,6 +103,7 @@ // // Styleguide 5.2. .error, + .warning, .errorbox, .warningbox, .successbox { @@ -118,7 +119,13 @@ color: @colorErrorText; border: 1px solid #fac5c5; background-color: #fae3e3; - text-shadow: 0 1px #fae3e3; + } + + // Colours taken from those for .warningbox in shared.css + .warning { + color: @colorWarningText; + border: 1px solid #fde29b; + background-color: #fdf1d1; } // This specifies styling for individual field validation error messages. diff --git a/tests/phpunit/includes/StatusTest.php b/tests/phpunit/includes/StatusTest.php index 474a481a6e..ebc2d1080c 100644 --- a/tests/phpunit/includes/StatusTest.php +++ b/tests/phpunit/includes/StatusTest.php @@ -645,4 +645,66 @@ class StatusTest extends MediaWikiLangTestCase { ]; } + /** + * @dataProvider provideErrorsWarningsOnly + * @covers Status::getErrorsOnlyStatus + * @covers Status::getWarningsOnlyStatus + */ + public function testGetErrorsWarningsOnlyStatus( $errorText, $warningText, $type, $errorResult, + $warningResult + ) { + $status = Status::newGood(); + if ( $errorText ) { + $status->fatal( $errorText ); + } + if ( $warningText ) { + $status->warning( $warningText ); + } + $testStatus = $status->splitByErrorType()[$type]; + $this->assertEquals( $errorResult, $testStatus->getErrorsByType( 'error' ) ); + $this->assertEquals( $warningResult, $testStatus->getErrorsByType( 'warning' ) ); + } + + public static function provideErrorsWarningsOnly() { + return [ + [ + 'Just an error', + 'Just a warning', + 0, + [ + 0 => [ + 'type' => 'error', + 'message' => 'Just an error', + 'params' => [] + ], + ], + [], + ], [ + 'Just an error', + 'Just a warning', + 1, + [], + [ + 0 => [ + 'type' => 'warning', + 'message' => 'Just a warning', + 'params' => [] + ], + ], + ], [ + null, + null, + 1, + [], + [], + ], [ + null, + null, + 0, + [], + [], + ] + ]; + } + } diff --git a/tests/phpunit/includes/auth/AuthenticationResponseTest.php b/tests/phpunit/includes/auth/AuthenticationResponseTest.php index 477b1617f5..194b49e01a 100644 --- a/tests/phpunit/includes/auth/AuthenticationResponseTest.php +++ b/tests/phpunit/includes/auth/AuthenticationResponseTest.php @@ -16,6 +16,7 @@ class AuthenticationResponseTest extends \MediaWikiTestCase { public function testConstructors( $constructor, $args, $expect ) { if ( is_array( $expect ) ) { $res = new AuthenticationResponse(); + $res->messageType = 'warning'; foreach ( $expect as $field => $value ) { $res->$field = $value; } @@ -51,6 +52,7 @@ class AuthenticationResponseTest extends \MediaWikiTestCase { [ 'newFail', [ $msg ], [ 'status' => AuthenticationResponse::FAIL, 'message' => $msg, + 'messageType' => 'error', ] ], [ 'newRestart', [ $msg ], [ @@ -66,6 +68,21 @@ class AuthenticationResponseTest extends \MediaWikiTestCase { 'status' => AuthenticationResponse::UI, 'neededRequests' => [ $req ], 'message' => $msg, + 'messageType' => 'warning', + ] ], + + [ 'newUI', [ [ $req ], $msg, 'warning' ], [ + 'status' => AuthenticationResponse::UI, + 'neededRequests' => [ $req ], + 'message' => $msg, + 'messageType' => 'warning', + ] ], + + [ 'newUI', [ [ $req ], $msg, 'error' ], [ + 'status' => AuthenticationResponse::UI, + 'neededRequests' => [ $req ], + 'message' => $msg, + 'messageType' => 'error', ] ], [ 'newUI', [ [], $msg ], new \InvalidArgumentException( '$reqs may not be empty' ) -- 2.20.1