jenkins-bot has submitted this change and it was merged.

Change subject: Show warnings in HTMLForm and warnings as warnings on 
Login/Signup form
......................................................................


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
---
M includes/Status.php
M includes/auth/AuthenticationResponse.php
M includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
M includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
M includes/htmlform/HTMLForm.php
M includes/htmlform/OOUIHTMLForm.php
M includes/libs/StatusValue.php
M includes/specialpage/LoginSignupSpecialPage.php
M resources/src/mediawiki.less/mediawiki.ui/variables.less
M resources/src/mediawiki.ui/components/forms.less
M tests/phpunit/includes/StatusTest.php
M tests/phpunit/includes/auth/AuthenticationResponseTest.php
12 files changed, 222 insertions(+), 30 deletions(-)

Approvals:
  Bartosz Dziewoński: Looks good to me, but someone else must approve
  Gergő Tisza: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/Status.php b/includes/Status.php
index e578873..a6348b7 100644
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -106,6 +106,26 @@
        }
 
        /**
+        * 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
         *
         * @param bool $ok Whether the operation completed
@@ -314,6 +334,7 @@
 
        /**
         * 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 0339e45..6684fb9 100644
--- a/includes/auth/AuthenticationResponse.php
+++ b/includes/auth/AuthenticationResponse.php
@@ -81,6 +81,9 @@
        /** @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 @@
                $ret = new AuthenticationResponse;
                $ret->status = AuthenticationResponse::FAIL;
                $ret->message = $msg;
+               $ret->messageType = 'error';
                return $ret;
        }
 
@@ -172,18 +176,23 @@
        /**
         * @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 beb11f4..7f121cd 100644
--- a/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
+++ b/includes/auth/ConfirmLinkSecondaryAuthenticationProvider.php
@@ -64,7 +64,8 @@
                $req = new ConfirmLinkAuthenticationRequest( $maybeLink );
                return AuthenticationResponse::newUI(
                        [ $req ],
-                       wfMessage( 'authprovider-confirmlink-message' )
+                       wfMessage( 'authprovider-confirmlink-message' ),
+                       'warning'
                );
        }
 
@@ -150,7 +151,8 @@
                                        '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 dd97830..f11a12c 100644
--- a/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
+++ b/includes/auth/ResetPasswordSecondaryAuthenticationProvider.php
@@ -112,17 +112,17 @@
 
                $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 3c88594..567e692 100644
--- a/includes/htmlform/HTMLForm.php
+++ b/includes/htmlform/HTMLForm.php
@@ -1014,7 +1014,8 @@
                $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 @@
         *
         * @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 0b22727..bbd3473 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 @@
        }
 
        /**
-        * @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 @@
                }
 
                // Used in getBody()
-               $this->oouiErrors = $errors;
+               if ( $elementsType === 'error' ) {
+                       $this->oouiErrors = $errors;
+               } else {
+                       $this->oouiWarnings = $errors;
+               }
                return '';
        }
 
@@ -236,7 +247,10 @@
                        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 @@
                                                [
                                                        'align' => 'top',
                                                        'errors' => 
$this->oouiErrors,
+                                                       'notices' => 
$this->oouiWarnings,
                                                        'classes' => $classes,
                                                ]
                                        )
diff --git a/includes/libs/StatusValue.php b/includes/libs/StatusValue.php
index 1d23f9d..f45dc41 100644
--- a/includes/libs/StatusValue.php
+++ b/includes/libs/StatusValue.php
@@ -80,6 +80,34 @@
        }
 
        /**
+        * 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 c3d43df..9e93970 100644
--- a/includes/specialpage/LoginSignupSpecialPage.php
+++ b/includes/specialpage/LoginSignupSpecialPage.php
@@ -354,7 +354,7 @@
                                $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 @@
 
                $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 507109a..b6f6568 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 cc96a5c..aedec5b 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 474a481..ebc2d10 100644
--- a/tests/phpunit/includes/StatusTest.php
+++ b/tests/phpunit/includes/StatusTest.php
@@ -645,4 +645,66 @@
                ];
        }
 
+       /**
+        * @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 477b161..194b49e 100644
--- a/tests/phpunit/includes/auth/AuthenticationResponseTest.php
+++ b/tests/phpunit/includes/auth/AuthenticationResponseTest.php
@@ -16,6 +16,7 @@
        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 @@
                        [ 'newFail', [ $msg ], [
                                'status' => AuthenticationResponse::FAIL,
                                'message' => $msg,
+                               'messageType' => 'error',
                        ] ],
 
                        [ 'newRestart', [ $msg ], [
@@ -66,6 +68,21 @@
                                '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' )

-- 
To view, visit https://gerrit.wikimedia.org/r/296938
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9a27911613e62b5c4cb86bea40696cb37c4f49c2
Gerrit-PatchSet: 21
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Florianschmidtwelzow <florian.schmidt.stargatewis...@gmail.com>
Gerrit-Reviewer: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Bartosz Dziewoński <matma....@gmail.com>
Gerrit-Reviewer: Florianschmidtwelzow <florian.schmidt.stargatewis...@gmail.com>
Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org>
Gerrit-Reviewer: VolkerE <volke...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to