Matthias Mullie has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/326134 )
Change subject: [WIP] Simplify error handling
......................................................................
[WIP] Simplify error handling
This uses the new API error message formats to i18n the message.
This way, we no longer have to try to i18n on the clientside,
which was problematic in some cases since we didn't have all
params.
I'm also getting rid of all api-error-* & related messages in
i18n. There's no point in building messages ourselves, let alone
keep copies of those same messages.
Bug: T115946
Bug: T152607
Change-Id: I58b9a7ed054ad0084bcd7180d92bd69ccc7ed129
---
M resources/handlers/mw.ApiUploadFormDataHandler.js
M resources/handlers/mw.ApiUploadPostHandler.js
M resources/mw.UploadWizard.js
M resources/mw.UploadWizardDetails.js
M resources/mw.UploadWizardUpload.js
M resources/mw.UploadWizardUploadInterface.js
M resources/transports/mw.FirefoggTransport.js
M resources/transports/mw.FormDataTransport.js
M resources/uw.EventFlowLogger.js
M resources/uw.FieldLayout.js
10 files changed, 139 insertions(+), 155 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/UploadWizard
refs/changes/34/326134/1
diff --git a/resources/handlers/mw.ApiUploadFormDataHandler.js
b/resources/handlers/mw.ApiUploadFormDataHandler.js
index 57a30df..a11b71a 100644
--- a/resources/handlers/mw.ApiUploadFormDataHandler.js
+++ b/resources/handlers/mw.ApiUploadFormDataHandler.js
@@ -67,19 +67,19 @@
handler.upload.setTransportProgress( fraction );
}
} ).then( function ( result ) {
- if ( !result || result.error ||
( result.upload && result.upload.warnings ) ) {
+ if ( !result || result.errors
|| ( result.upload && result.upload.warnings ) ) {
uw.eventFlowLogger.logApiError( 'file', result );
}
handler.upload.setTransported(
result );
}, function ( result ) {
- if ( !result || result.error ||
( result.upload && result.upload.warnings ) ) {
+ if ( !result || result.errors
|| ( result.upload && result.upload.warnings ) ) {
uw.eventFlowLogger.logApiError( 'file', result );
}
handler.upload.setTransported(
result );
} );
}, function ( code, info, result ) {
uw.eventFlowLogger.logApiError( 'file', result
);
- handler.upload.setError( code, info );
+ handler.upload.setError( code, info.errors[ 0
].html );
} );
}
};
diff --git a/resources/handlers/mw.ApiUploadPostHandler.js
b/resources/handlers/mw.ApiUploadPostHandler.js
index 49b0084..f648f0a 100644
--- a/resources/handlers/mw.ApiUploadPostHandler.js
+++ b/resources/handlers/mw.ApiUploadPostHandler.js
@@ -27,7 +27,7 @@
} )
.fail( function ( code, info, result ) {
uw.eventFlowLogger.logApiError( 'file',
result );
- handler.upload.setError( code, info );
+ handler.upload.setError( code,
info.errors[ 0 ].html );
} )
.done( function ( result ) {
if ( !result || result.error || (
result.upload && result.upload.warnings ) ) {
diff --git a/resources/mw.UploadWizard.js b/resources/mw.UploadWizard.js
index 616092c..4db1ce8 100644
--- a/resources/mw.UploadWizard.js
+++ b/resources/mw.UploadWizard.js
@@ -6,7 +6,7 @@
mw.UploadWizard = function ( config ) {
var maxSimPref, wizard = this;
- this.api = new mw.Api( { ajax: { timeout: 0 } } );
+ this.api = this.getApi( { ajax: { timeout: 0 } } );
// making a sort of global for now, should be done by passing
in config or fragments of config
// when needed elsewhere
@@ -104,6 +104,66 @@
},
/**
+ * mw.Api's ajax calls are not very consistent in their error
handling.
+ * As long as the response comes back, the response will be
fine: it'll
+ * get rejected with the error details there. However, if no
response
+ * comes back for whatever reason, things can get confusing.
+ * I'll monkeypatch around such cases so that we can always
rely on the
+ * error response the way we want it to be.
+ *
+ * @param {Object} options
+ */
+ getApi: function ( options ) {
+ var api = new mw.Api( options );
+
+ api.ajax = function ( parameters, ajaxOptions ) {
+ $.extend( parameters, {
+ errorformat: 'html',
+ uselang: mw.config.get(
'wgUserLanguage' ),
+ formatversion: 2
+ } );
+
+ return mw.Api.prototype.ajax.apply( this, [
parameters, ajaxOptions ] ).then(
+ null, // done handler - doesn't need
overriding
+ function ( code, info, result ) { //
fail handler
+ var response = {
+ errors: [ {
+ code: code,
+ html: 'unknown'
+ } ]
+ };
+
+ if ( info && info.errors &&
info.errors[ 0 ] ) {
+ // in case of
success-but-has-errors, we have a valid result
+ response = info;
+ } else if ( info &&
info.textStatus ) {
+ // in case of
$.ajax.fail(), info will be an object literal
+ // with this data
+ response.errors[ 0
].html = info.textStatus;
+
+ if ( info.textStatus
=== 'timeout' ) {
+
response.errors[ 0 ].html = mw.message( 'timeout' ).text();
+ }
+ } else if ( info instanceof
String ) {
+ // in case of empty
response with 200 status code, info will
+ // be a string
+ response.errors[ 0
].html = info;
+ }
+
+ if ( code === 'http' && result
&& result.xhr && result.xhr.status === 0 ) {
+ // Failed to even
connect to server
+ code = 'offline';
+ }
+
+ return $.Deferred().reject(
code, response, result );
+ }
+ );
+ };
+
+ return api;
+ },
+
+ /**
* Helper function to check whether the upload process is
totally
* complete and we can safely leave the window.
*/
diff --git a/resources/mw.UploadWizardDetails.js
b/resources/mw.UploadWizardDetails.js
index 262808a..8d0c187 100644
--- a/resources/mw.UploadWizardDetails.js
+++ b/resources/mw.UploadWizardDetails.js
@@ -769,11 +769,12 @@
submitInternal: function ( params ) {
var
details = this,
- apiPromise =
details.upload.api.postWithEditToken( params );
+ apiPromise = this.upload.api.postWithEditToken(
params );
+
return apiPromise
.then(
function ( result ) {
- if ( !result || result.error ||
( result.upload && result.upload.warnings ) ) {
+ if ( !result || result.errors
|| ( result.upload && result.upload.warnings ) ) {
uw.eventFlowLogger.logApiError( 'details', result );
}
return
details.handleSubmitResult( result, params );
@@ -782,7 +783,7 @@
uw.eventFlowLogger.logApiError(
'details', result );
details.upload.state = 'error';
details.processError( code,
info );
- return $.Deferred().reject(
code, info );
+ return $.Deferred().reject(
code, info, result );
}
)
.promise( { abort: apiPromise.abort } );
@@ -907,7 +908,7 @@
/**
* Create a recoverable error -- show the form again, and
highlight the problematic field.
*
- * @param {mw.Message} errorMessage Error message to show.
+ * @param {string} errorMessage Error message to show.
* @param {string} errorCode
*/
recoverFromError: function ( errorMessage, errorCode ) {
@@ -921,12 +922,12 @@
* Show error state, possibly using a recoverable error form
*
* @param {string} code Error code
- * @param {string} statusLine Status line
+ * @param {string} html Error html
*/
- showError: function ( code, statusLine ) {
- uw.eventFlowLogger.logError( 'details', { code: code,
message: statusLine } );
+ showError: function ( code, html ) {
+ uw.eventFlowLogger.logError( 'details', { code: code,
message: html } );
this.showIndicator( 'error' );
- this.setStatus( statusLine );
+ this.setStatus( html );
},
/**
@@ -936,37 +937,13 @@
* @param {Mixed} result Result from ajax call
*/
processError: function ( code, result ) {
- var statusKey, comma, promise,
- statusLine = mw.message(
'api-error-unclassified' ).text(),
- titleBlacklistMessageMap = {
- senselessimagename:
'senselessimagename', // TODO This is probably never hit?
- 'titleblacklist-custom-filename':
'hosting',
- 'titleblacklist-custom-SVG-thumbnail':
'thumbnail',
- 'titleblacklist-custom-thumbnail':
'thumbnail',
-
'titleblacklist-custom-double-apostrophe': 'double-apostrophe'
- },
- titleErrorMap = {
- 'fileexists-shared-forbidden':
'fileexists-shared-forbidden',
- protectedpage: 'protected'
- };
-
if ( code === 'badtoken' ) {
this.api.badToken( 'csrf' );
// TODO Automatically try again instead of
requiring the user to bonk the button
}
if ( code === 'abusefilter-disallowed' || code ===
'abusefilter-warning' || code === 'spamblacklist' ) {
- // 'amenableparser' will expand templates and
parser functions server-side.
- // We still do the rest of wikitext parsing
here (throught jqueryMsg).
- promise = this.api.loadMessagesIfMissing( [
result.error.message.key ], { amenableparser: true } );
- this.recoverFromError( mw.message( 'api-error-'
+ code, function () {
- promise.done( function () {
- mw.errorDialog( $( '<div>'
).msg(
-
result.error.message.key,
-
result.error.message.params
- ) );
- } );
- } ), code );
+ this.recoverFromError( result.errors[ 0 ].html,
code );
return;
}
@@ -979,41 +956,18 @@
// This could potentially also come up when an
upload is removed by the user, but in that
// case the UI is invisible anyway, so whatever.
code = 'ratelimited';
- } else if ( code === 'http' && result && result.xhr &&
result.xhr.status === 0 ) {
- // Failed to even connect to server
- code = 'offline';
}
- if ( result && code ) {
- if ( titleErrorMap[ code ] ) {
- this.recoverFromError( mw.message(
'mwe-upwiz-error-title-' + titleErrorMap[ code ] ), 'title-' + titleErrorMap[
code ] );
- return;
- } else if ( code === 'titleblacklist-forbidden'
) {
- this.recoverFromError( mw.message(
'mwe-upwiz-error-title-' + titleBlacklistMessageMap[ result.error.message.key ]
), 'title-' + titleBlacklistMessageMap[ result.error.message.key ] );
- return;
- } else {
- statusKey = 'api-error-' + code;
- if ( code === 'filetype-banned' &&
result.error.blacklisted ) {
- comma = mw.message(
'comma-separator' ).text();
- code = 'filetype-banned-type';
- statusLine = mw.message(
'api-error-filetype-banned-type',
-
result.error.blacklisted.join( comma ),
-
result.error.allowed.join( comma ),
-
result.error.allowed.length,
-
result.error.blacklisted.length
- ).text();
- } else if ( result.error &&
result.error.info ) {
- statusLine = mw.message(
statusKey, result.error.info ).text();
- } else {
- statusLine = mw.message(
statusKey, '[no error info]' ).text();
- }
- }
+ if ( [ 'fileexists-shared-forbidden', 'protectedpage',
'titleblacklist-forbidden' ].indexOf( code ) > -1 ) {
+ this.recoverFromError( result.errors[ 0 ].html,
code );
+ return;
}
- this.showError( code, statusLine );
+
+ this.showError( code, result.errors[ 0 ].html );
},
- setStatus: function ( s ) {
- this.div.find( '.mwe-upwiz-file-status-line' ).text( s
).show();
+ setStatus: function ( html ) {
+ this.div.find( '.mwe-upwiz-file-status-line' ).html(
html ).show();
},
showIndicator: function ( statusStr ) {
diff --git a/resources/mw.UploadWizardUpload.js
b/resources/mw.UploadWizardUpload.js
index 2e42aeb..225f26b 100644
--- a/resources/mw.UploadWizardUpload.js
+++ b/resources/mw.UploadWizardUpload.js
@@ -116,15 +116,15 @@
/**
* Stop the upload -- we have failed for some reason
*/
- mw.UploadWizardUpload.prototype.setError = function ( code, info,
additionalStatus ) {
+ mw.UploadWizardUpload.prototype.setError = function ( code, html,
$additionalStatus ) {
if ( this.state === 'aborted' ) {
// There's no point in reporting an error anymore.
return;
}
this.state = 'error';
this.transportProgress = 0;
- this.ui.showError( code, info, additionalStatus );
- uw.eventFlowLogger.logError( 'file', { code: code, message:
info } );
+ this.ui.showError( code, html, $additionalStatus );
+ uw.eventFlowLogger.logError( 'file', { code: code, message:
html } );
};
/**
@@ -142,20 +142,17 @@
*/
mw.UploadWizardUpload.prototype.setTransported = function ( result ) {
// default error state
- var comma, warnCode, promise,
- code = 'unknown',
- info = 'unknown',
+ var warnCode,
+ error = result.errors && result.errors[ 0 ] ?
result.errors[ 0 ] : undefined,
+ code = error && error.code ? error.code : 'unknown',
+ html = error && error.html ? error.html : 'unknown',
$extra;
if ( this.state === 'aborted' ) {
return;
}
- if ( result.error ) {
- // If there was an error, we can't really do anything
else, so let's get out while we can.
- if ( result.error.code ) {
- code = result.error.code;
- }
+ if ( error ) {
if ( code === 'badtoken' ) {
this.api.badToken( 'csrf' );
// Try again once
@@ -164,45 +161,20 @@
return;
}
}
- if ( code === 'filetype-banned' &&
result.error.blacklisted ) {
- code = 'filetype-banned-type';
- comma = mw.message( 'comma-separator' ).text();
- info = [
- result.error.blacklisted.join( comma ),
- result.error.allowed.join( comma ),
- result.error.allowed.length,
- result.error.blacklisted.length
- ];
- } else if ( code === 'abusefilter-disallowed' || code
=== 'abusefilter-warning' || code === 'spamblacklist' ) {
- // 'amenableparser' will expand templates and
parser functions server-side.
- // We still do the rest of wikitext parsing
here (throught jqueryMsg).
- promise = this.api.loadMessagesIfMissing( [
result.error.message.key ], { amenableparser: true } );
- info = [
- function () {
- promise.done( function () {
- mw.errorDialog( $(
'<div>' ).msg(
-
result.error.message.key,
-
result.error.message.params
- ) );
- } );
- }
- ];
- if ( code === 'abusefilter-warning' ) {
- $extra = new OO.ui.ButtonWidget( {
- label: mw.message(
'mwe-upwiz-override' ).text(),
- title: mw.message(
'mwe-upwiz-override-upload' ).text(),
- flags: 'progressive',
- framed: false
- } ).on( 'click', function () {
- // No need to ignore the error,
AbuseFilter will only return it once
- this.start();
- }.bind( this ) ).$element;
- }
- } else if ( result.error.info ) {
- info = result.error.info;
+ if ( code === 'abusefilter-warning' ) {
+ $extra = new OO.ui.ButtonWidget( {
+ label: mw.message( 'mwe-upwiz-override'
).text(),
+ title: mw.message(
'mwe-upwiz-override-upload' ).text(),
+ flags: 'progressive',
+ framed: false
+ } ).on( 'click', function () {
+ // No need to ignore the error,
AbuseFilter will only return it once
+ this.start();
+ }.bind( this ) ).$element;
}
- this.setError( code, info );
+
+ this.setError( code, html );
return;
}
@@ -227,13 +199,14 @@
default:
// we have an unknown warning,
so let's say what we know
code = 'unknown-warning';
+ html = mw.message( 'api-error-'
+ code ).text();
if ( typeof
result.upload.warnings[ warnCode ] === 'string' ) {
// tack the original
error code onto the warning info
- info = warnCode +
mw.message( 'colon-separator' ).text() + result.upload.warnings[ warnCode ];
+ html += warnCode +
mw.message( 'colon-separator' ).text() + result.upload.warnings[ warnCode ];
} else {
- info =
result.upload.warnings[ warnCode ];
+ html +=
result.upload.warnings[ warnCode ];
}
- this.setError( code, info );
+ this.setError( code, html );
break;
}
}
@@ -244,12 +217,12 @@
if ( result.upload.imageinfo ) {
this.setSuccess( result );
} else {
- this.setError( 'noimageinfo', info );
+ this.setError( 'noimageinfo', html );
}
} else if ( result.upload && result.upload.result ===
'Warning' ) {
throw new Error( 'Your browser got back a
Warning result from the server. Please file a bug.' );
} else {
- this.setError( code, info );
+ this.setError( code, html );
}
}
};
@@ -307,7 +280,7 @@
$extra = $extra.add( uploadDuplicate.$element );
}
- this.setError( code, [ duplicates.length ], $extra );
+ this.setError( code, mw.message( 'api-error-' + code,
duplicates.length ).text(), $extra );
};
/**
diff --git a/resources/mw.UploadWizardUploadInterface.js
b/resources/mw.UploadWizardUploadInterface.js
index f557105..7c48146 100644
--- a/resources/mw.UploadWizardUploadInterface.js
+++ b/resources/mw.UploadWizardUploadInterface.js
@@ -113,6 +113,15 @@
};
/**
+ * Set status line directly with html content. Make sure the HTML is
safe!
+ *
+ * @param {string} html
+ */
+ mw.UploadWizardUploadInterface.prototype.setStatusHtml = function (
html ) {
+ $( this.div ).find( '.mwe-upwiz-file-status' ).html( html
).show();
+ };
+
+ /**
* Set status line directly with a string
*
* @param {string} s
@@ -169,34 +178,12 @@
* Show that transport has failed
*
* @param {string} code Error code from API
- * @param {string|Object} info Extra info
+ * @param {string} html Error message
* @param {jQuery} [$additionalStatus]
*/
- mw.UploadWizardUploadInterface.prototype.showError = function ( code,
info, $additionalStatus ) {
- var msgKey, args, moreErrorCodes = [
- 'unknown-warning',
- 'abusefilter-disallowed',
- 'abusefilter-warning',
- 'spamblacklist',
- 'offline',
- 'parsererror'
- ];
-
+ mw.UploadWizardUploadInterface.prototype.showError = function ( code,
html, $additionalStatus ) {
this.showIndicator( 'error' );
- // is this an error that we expect to have a message for?
-
- if ( code === 'http' && info.textStatus === 'timeout' ) {
- code = 'timeout';
- }
-
- if ( $.inArray( code, mw.Api.errors ) !== -1 || $.inArray(
code, moreErrorCodes ) !== -1 ) {
- msgKey = 'api-error-' + code;
- args = $.makeArray( info );
- } else {
- msgKey = 'api-error-unknown-code';
- args = [ code ].concat( $.makeArray( info ) );
- }
- this.setStatus( msgKey, args );
+ this.setStatusHtml( html );
this.setAdditionalStatus( $additionalStatus );
};
diff --git a/resources/transports/mw.FirefoggTransport.js
b/resources/transports/mw.FirefoggTransport.js
index e2e76b7..28a3975 100644
--- a/resources/transports/mw.FirefoggTransport.js
+++ b/resources/transports/mw.FirefoggTransport.js
@@ -41,10 +41,10 @@
} else {
// encoding failed
deferred.reject( {
- error: {
+ errors: [ {
code:
'firefogg',
- info: 'Encoding
failed'
- }
+ html: 'Encoding
failed'
+ } ]
} );
}
}, function ( progress ) {
diff --git a/resources/transports/mw.FormDataTransport.js
b/resources/transports/mw.FormDataTransport.js
index 6208577..74dc987 100644
--- a/resources/transports/mw.FormDataTransport.js
+++ b/resources/transports/mw.FormDataTransport.js
@@ -87,6 +87,11 @@
formData.append( 'offset', offset || 0 );
+ // request parsed errors, in user language
+ formData.append( 'errorformat', 'html' );
+ formData.append( 'uselang', mw.config.get( 'wgUserLanguage' ) );
+ formData.append( 'formatversion', 2 );
+
return formData;
};
@@ -426,11 +431,12 @@
// Let's check what caused this, too.
window.console.error( 'parsererror', evt );
}
+
response = {
- error: {
- code: evt.target.status !== 0 ?
'parsererror' : 'offline',
- info: evt.target.responseText
- }
+ errors: [ {
+ code: 'parsererror',
+ html: mw.message( evt.target.status !==
0 ? 'api-error-parsererror' : 'api-error-offline' ).text()
+ } ]
};
}
diff --git a/resources/uw.EventFlowLogger.js b/resources/uw.EventFlowLogger.js
index 8802602..c4048dc 100644
--- a/resources/uw.EventFlowLogger.js
+++ b/resources/uw.EventFlowLogger.js
@@ -157,7 +157,11 @@
var code, message;
if ( !result ) {
code = 'api/empty';
+ } else if ( result.errors ) {
+ // formatversion=2
+ code = 'api/error/' + result.errors[0].code;
} else if ( result.error ) {
+ // formatversion=1
code = 'api/error/' + result.error.code;
} else if ( result.upload ) {
code = 'api/warning/' + Object.keys(
result.upload.warnings || {} ).sort().join( ',' );
diff --git a/resources/uw.FieldLayout.js b/resources/uw.FieldLayout.js
index 1e24a04..8074f61 100644
--- a/resources/uw.FieldLayout.js
+++ b/resources/uw.FieldLayout.js
@@ -87,7 +87,7 @@
*/
uw.FieldLayout.prototype.makeMessage = function ( kind, msg ) {
var
- content = msg.parseDom(),
+ content = msg instanceof mw.Message ? msg.parseDom() :
msg,
$listItem =
uw.FieldLayout.parent.prototype.makeMessage.call( this, kind, content );
$listItem.addClass( 'mwe-upwiz-fieldLayout-' + kind + '-' +
msg.key );
return $listItem;
--
To view, visit https://gerrit.wikimedia.org/r/326134
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I58b9a7ed054ad0084bcd7180d92bd69ccc7ed129
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/UploadWizard
Gerrit-Branch: master
Gerrit-Owner: Matthias Mullie <mmul...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
