r77100 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77099‎ | r77100 | r77101 >
Date:09:08, 22 November 2010
Author:neilk
Status:ok (Comments)
Tags:
Comment:
decent error messaging
Modified paths:
  • /trunk/extensions/UploadWizard/UploadWizard.i18n.php (modified) (history)
  • /trunk/extensions/UploadWizard/resources/combined.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/combined.min.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.Api.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.ApiUploadHandler.js (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.UploadWizard.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizard.i18n.php
——@@ -31,7 +31,34 @@
3232 'mwe-upwiz-step-deeds' => 'Release rights',
3333 'mwe-upwiz-step-details' => 'Describe',
3434 'mwe-upwiz-step-thanks' => 'Use',
35 - 'mwe-upwiz-api-error-code' => 'API error code: $1',
 35+ 'mwe-upwiz-api-error-http' => 'There a problem connecting to the service.',
 36+ 'mwe-upwiz-api-error-ok-but-empty' => 'The server didn\'t return any information about the upload.',
 37+ 'mwe-upwiz-api-error-unknown-code' => 'The server returned an error we did not understand: "$1"',
 38+ 'mwe-upwiz-api-error-uploaddisabled' => 'Uploading is disabled on this wiki.',
 39+ 'mwe-upwiz-api-error-nomodule' => 'The wiki did not know how to handle this upload.',
 40+ 'mwe-upwiz-api-error-mustbeposted' => 'There\'s a bug in this software; it\'s not using the proper HTTP method.',
 41+ 'mwe-upwiz-api-error-badaccess-groups' => 'You aren\'t permitted to upload files to this wiki. Check what access groups you belong to.',
 42+ 'mwe-upwiz-api-error-stashfailed' => 'The wiki could not store the file.',
 43+ 'mwe-upwiz-api-error-missingresult' => 'We could not determine if the copy succeeded.',
 44+ 'mwe-upwiz-api-error-missingparam' => 'The upload didn\'t have all the required information (probably a bug in this uploader.)',
 45+ 'mwe-upwiz-api-error-invalid-session-key' => 'The server couldn\'t find that file in your uploaded files.',
 46+ 'mwe-upwiz-api-error-copyuploaddisabled' => 'Uploads by copying are disabled.',
 47+ 'mwe-upwiz-api-error-mustbeloggedin' => 'You are not properly logged in.',
 48+ 'mwe-upwiz-api-error-empty-file' => 'The file you submitted was empty.',
 49+ 'mwe-upwiz-api-error-file-too-large' => 'The file you submitted was too large.',
 50+ 'mwe-upwiz-api-error-filetype-missing' => 'The file is missing an extension.',
 51+ 'mwe-upwiz-api-error-filetype-banned' => 'This type of file is banned.',
 52+ 'mwe-upwiz-api-error-filename-tooshort' => 'The filename is too short.',
 53+ 'mwe-upwiz-api-error-illegal-filename' => 'The filename is not allowed.',
 54+ 'mwe-upwiz-api-error-verification-error' => 'This file might be corrupt, or have the wrong extension.',
 55+ 'mwe-upwiz-api-error-hookaborted' => 'The modification you tried to make was aborted by an extension hook.',
 56+ 'mwe-upwiz-api-error-unknown-error' => 'Something (we don\'t know what) went wrong when trying to upload your file.',
 57+ 'mwe-upwiz-api-error-internal-error' => 'Something went wrong with processing your upload on the wiki.',
 58+ 'mwe-upwiz-api-error-overwrite' => 'Overwriting an existing file is not allowed.',
 59+ 'mwe-upwiz-api-error-badtoken' => 'The "token" we use to identify you to the server was bad.',
 60+ 'mwe-upwiz-api-error-fetchfileerror' => 'Something went wrong while fetching the file.',
 61+ 'mwe-upwiz-api-warning-duplicate' => 'There is another file already on the wiki with the same content',
 62+ 'mwe-upwiz-api-warning-exists' => 'There is another file already on the wiki with the same filename',
3663 'mwe-upwiz-tutorial-error-localized-file-missing' => 'Sorry, we could not find a tutorial in your language. The English one is shown instead.',
3764 'mwe-upwiz-tutorial-error-file-missing' => 'Sorry, we could not find any files for the tutorial that is supposed to go here. Please contact the system administrators.',
3865 'mwe-upwiz-tutorial-error-cannot-transform' => 'Sorry, we could not get a scaled image of the tutorial to fit this screen. This may be a temporary problem with Wikimedia Commons; try again later.',
Index: trunk/extensions/UploadWizard/resources/mw.Api.js
——@@ -126,13 +126,13 @@
127127 ajaxOptions.data = parameters;
128128
129129 ajaxOptions.error = function( xhr, textStatus, exception ) {
130 - ajaxOptions.err( 'http-' + textStatus, { xhr: xhr, exception: exception } );
 130+ ajaxOptions.err( 'http', { xhr: xhr, textStatus: textStatus, exception: exception } );
131131 };
132132
133133 /* success just means 200 OK; also check for output and API errors */
134134 ajaxOptions.success = function( result ) {
135135 if ( mw.isEmpty( result ) ) {
136 - ajaxOptions.err( "empty", "OK response but empty result (check HTTP headers?)" );
 136+ ajaxOptions.err( "ok-but-empty", "OK response but empty result (check HTTP headers?)" );
137137 } else if ( result.error ) {
138138 var code = mw.isDefined( result.error.code ) ? result.error.code : "unknown";
139139 ajaxOptions.err( code, result );
——@@ -145,6 +145,48 @@
146146
147147 }
148148
149 - }
 149+ };
150150
 151+ /**
 152+ * This is a list of errors we might receive from the API.
 153+ * For now, this just documents our expectation that there should be similar messages
 154+ * available.
 155+ */
 156+ mw.Api.errors = [
 157+ 'uploaddisabled',
 158+ 'nomodule',
 159+ 'mustbeposted',
 160+ 'badaccess-groups',
 161+ 'stashfailed',
 162+ 'missingresult',
 163+ 'missingparam',
 164+ 'invalid-session-key',
 165+ 'copyuploaddisabled',
 166+ 'mustbeloggedin',
 167+ 'empty-file',
 168+ 'file-too-large',
 169+ 'filetype-missing',
 170+ 'filetype-banned',
 171+ 'filename-tooshort',
 172+ 'illegal-filename',
 173+ 'verification-error',
 174+ 'hookaborted',
 175+ 'unknown-error',
 176+ 'internal-error',
 177+ 'overwrite',
 178+ 'badtoken',
 179+ 'fetchfileerror'
 180+ ];
 181+
 182+ /**
 183+ * This is a list of warnings we might receive from the API.
 184+ * For now, this just documents our expectation that there should be similar messages
 185+ * available.
 186+ */
 187+
 188+ mw.Api.warnings = [
 189+ 'duplicate',
 190+ 'exists'
 191+ ];
 192+
151193 }) ( window.mw, jQuery );
Index: trunk/extensions/UploadWizard/resources/combined.js
——@@ -5837,13 +5837,13 @@
58385838 ajaxOptions.data = parameters;
58395839
58405840 ajaxOptions.error = function( xhr, textStatus, exception ) {
5841 - ajaxOptions.err( 'http-' + textStatus, { xhr: xhr, exception: exception } );
 5841+ ajaxOptions.err( 'http', { xhr: xhr, textStatus: textStatus, exception: exception } );
58425842 };
58435843
58445844 /* success just means 200 OK; also check for output and API errors */
58455845 ajaxOptions.success = function( result ) {
58465846 if ( mw.isEmpty( result ) ) {
5847 - ajaxOptions.err( "empty", "OK response but empty result (check HTTP headers?)" );
 5847+ ajaxOptions.err( "ok-but-empty", "OK response but empty result (check HTTP headers?)" );
58485848 } else if ( result.error ) {
58495849 var code = mw.isDefined( result.error.code ) ? result.error.code : "unknown";
58505850 ajaxOptions.err( code, result );
——@@ -5856,8 +5856,50 @@
58575857
58585858 }
58595859
5860 - }
 5860+ };
58615861
 5862+ /**
 5863+ * This is a list of errors we might receive from the API.
 5864+ * For now, this just documents our expectation that there should be similar messages
 5865+ * available.
 5866+ */
 5867+ mw.Api.errors = [
 5868+ 'uploaddisabled',
 5869+ 'nomodule',
 5870+ 'mustbeposted',
 5871+ 'badaccess-groups',
 5872+ 'stashfailed',
 5873+ 'missingresult',
 5874+ 'missingparam',
 5875+ 'invalid-session-key',
 5876+ 'copyuploaddisabled',
 5877+ 'mustbeloggedin',
 5878+ 'empty-file',
 5879+ 'file-too-large',
 5880+ 'filetype-missing',
 5881+ 'filetype-banned',
 5882+ 'filename-tooshort',
 5883+ 'illegal-filename',
 5884+ 'verification-error',
 5885+ 'hookaborted',
 5886+ 'unknown-error',
 5887+ 'internal-error',
 5888+ 'overwrite',
 5889+ 'badtoken',
 5890+ 'fetchfileerror'
 5891+ ];
 5892+
 5893+ /**
 5894+ * This is a list of warnings we might receive from the API.
 5895+ * For now, this just documents our expectation that there should be similar messages
 5896+ * available.
 5897+ */
 5898+
 5899+ mw.Api.warnings = [
 5900+ 'duplicate',
 5901+ 'exists'
 5902+ ];
 5903+
58625904 }) ( window.mw, jQuery );
58635905 // library to assist with edits
58645906
——@@ -8005,7 +8047,7 @@
80068048 _this.$form.submit();
80078049 };
80088050 var err = function( code, info ) {
8009 - _this.upload.setFailed( code, info );
 8051+ _this.upload.setError( code, info );
80108052 };
80118053 this.configureEditToken( ok, err );
80128054 }
——@@ -9202,7 +9244,6 @@
92039245 * Stop the upload -- we have failed for some reason
92049246 */
92059247 setError: function( code, info ) {
9206 - /* stop the upload progress */
92079248 this.state = 'error';
92089249 this.transportProgress = 0;
92099250 this.ui.showError( code, info );
——@@ -9584,11 +9625,21 @@
95859626
95869627 /**
95879628 * Show that transport has failed
 9629+ * @param String code: error code from API
 9630+ * @param {String|Object} info: extra info
95889631 */
95899632 showError: function( code, info ) {
9590 - // XXX TODO use code
95919633 this.showIndicator( 'error' );
9592 - // create a status message for the error
 9634+ // is this an error that we expect to have a message for?
 9635+ var msgKey = 'mwe-upwiz-api-error-unknown-code'
 9636+ var args = [ code ];
 9637+ if ( $j.inArray( code, mw.Api.errors ) !== -1 ) {
 9638+ var msgKey = 'mwe-upwiz-api-error-' + code;
 9639+ // args may change base on particular error messages.
 9640+ // for instance, we are throwing away the extra info right now. Might be nice to surface that in a debug mode
 9641+ args = [];
 9642+ }
 9643+ this.setStatus( msgKey, args );
95939644 },
95949645
95959646 /**
Index: trunk/extensions/UploadWizard/resources/mw.UploadWizard.js
——@@ -82,7 +82,6 @@
8383 * Stop the upload -- we have failed for some reason
8484 */
8585 setError: function( code, info ) {
86 - /* stop the upload progress */
8786 this.state = 'error';
8887 this.transportProgress = 0;
8988 this.ui.showError( code, info );
——@@ -464,11 +463,21 @@
465464
466465 /**
467466 * Show that transport has failed
 467+ * @param String code: error code from API
 468+ * @param {String|Object} info: extra info
468469 */
469470 showError: function( code, info ) {
470 - // XXX TODO use code
471471 this.showIndicator( 'error' );
472 - // create a status message for the error
 472+ // is this an error that we expect to have a message for?
 473+ var msgKey = 'mwe-upwiz-api-error-unknown-code'
 474+ var args = [ code ];
 475+ if ( $j.inArray( code, mw.Api.errors ) !== -1 ) {
 476+ var msgKey = 'mwe-upwiz-api-error-' + code;
 477+ // args may change base on particular error messages.
 478+ // for instance, we are throwing away the extra info right now. Might be nice to surface that in a debug mode
 479+ args = [];
 480+ }
 481+ this.setStatus( msgKey, args );
473482 },
474483
475484 /**
Index: trunk/extensions/UploadWizard/resources/mw.ApiUploadHandler.js
——@@ -114,7 +114,7 @@
115115 _this.$form.submit();
116116 };
117117 var err = function( code, info ) {
118 - _this.upload.setFailed( code, info );
 118+ _this.upload.setError( code, info );
119119 };
120120 this.configureEditToken( ok, err );
121121 }
Index: trunk/extensions/UploadWizard/resources/combined.min.js
——@@ -5837,13 +5837,13 @@
58385838 ajaxOptions.data=parameters;
58395839
58405840 ajaxOptions.error=function(xhr,textStatus,exception){
5841 -ajaxOptions.err('http-'+textStatus,{xhr:xhr,exception:exception});
 5841+ajaxOptions.err('http',{xhr:xhr,textStatus:textStatus,exception:exception});
58425842 };
58435843
58445844
58455845 ajaxOptions.success=function(result){
58465846 if(mw.isEmpty(result)){
5847 -ajaxOptions.err("empty","OK response but empty result (check HTTP headers?)");
 5847+ajaxOptions.err("ok-but-empty","OK response but empty result (check HTTP headers?)");
58485848 }else if(result.error){
58495849 var code=mw.isDefined(result.error.code)?result.error.code:"unknown";
58505850 ajaxOptions.err(code,result);
——@@ -5856,8 +5856,50 @@
58575857
58585858 }
58595859
5860 -}
 5860+};
58615861
 5862+
 5863+
 5864+
 5865+
 5866+
 5867+mw.Api.errors=[
 5868+'uploaddisabled',
 5869+'nomodule',
 5870+'mustbeposted',
 5871+'badaccess-groups',
 5872+'stashfailed',
 5873+'missingresult',
 5874+'missingparam',
 5875+'invalid-session-key',
 5876+'copyuploaddisabled',
 5877+'mustbeloggedin',
 5878+'empty-file',
 5879+'file-too-large',
 5880+'filetype-missing',
 5881+'filetype-banned',
 5882+'filename-tooshort',
 5883+'illegal-filename',
 5884+'verification-error',
 5885+'hookaborted',
 5886+'unknown-error',
 5887+'internal-error',
 5888+'overwrite',
 5889+'badtoken',
 5890+'fetchfileerror'
 5891+];
 5892+
 5893+
 5894+
 5895+
 5896+
 5897+
 5898+
 5899+mw.Api.warnings=[
 5900+'duplicate',
 5901+'exists'
 5902+];
 5903+
58625904 })(window.mw,jQuery);
58635905
58645906
——@@ -8005,7 +8047,7 @@
80068048 _this.$form.submit();
80078049 };
80088050 var err=function(code,info){
8009 -_this.upload.setFailed(code,info);
 8051+_this.upload.setError(code,info);
80108052 };
80118053 this.configureEditToken(ok,err);
80128054 }
——@@ -9202,7 +9244,6 @@
92039245
92049246
92059247 setError:function(code,info){
9206 -
92079248 this.state='error';
92089249 this.transportProgress=0;
92099250 this.ui.showError(code,info);
——@@ -9585,10 +9626,20 @@
95869627
95879628
95889629
 9630+
 9631+
95899632 showError:function(code,info){
9590 -
95919633 this.showIndicator('error');
95929634
 9635+var msgKey='mwe-upwiz-api-error-unknown-code'
 9636+var args=[code];
 9637+if($j.inArray(code,mw.Api.errors)!==-1){
 9638+var msgKey='mwe-upwiz-api-error-'+code;
 9639+
 9640+
 9641+args=[];
 9642+}
 9643+this.setStatus(msgKey,args);
95939644 },
95949645
95959646

Follow-up revisions

RevisionCommit summaryAuthorDate
r81509First pass through tweaking human-readable forms of API error messages for Up...brion04:34, 4 February 2011

Comments

#Comment by Nikerabbit (talk | contribs)   09:47, 22 November 2010

+ 'mwe-upwiz-api-error-http' => 'There a problem connecting to the service.', ?Unable to connect to the service. 

+	'mwe-upwiz-api-error-ok-but-empty' => 'The server didn\'t return any information about the upload.',

MediaWiki style doesn't use contractions. Does this mean we didn't get any reply from the server? 

+	'mwe-upwiz-api-error-unknown-code' => 'The server returned an error we did not understand: "$1"',

?Unknown error

Is $1 a error code or explanation (in English?) 

+	'mwe-upwiz-api-error-uploaddisabled' => 'Uploading is disabled on this wiki.',

?Uploading of files 

+	'mwe-upwiz-api-error-nomodule' => 'The wiki did not know how to handle this upload.',

How to handle the file format or something else? The user should know what he can do, or nothing if that is the case. 

+	'mwe-upwiz-api-error-mustbeposted' => 'There\'s a bug in this software; it\'s not using the proper HTTP method.',

Again, what can the user do in this unlikely situation? 

+	'mwe-upwiz-api-error-badaccess-groups' => 'You aren\'t permitted to upload files to this wiki. Check what access groups you belong to.',

Would be nice to fetch this information for the user. I haven't seen term access group used anywhere else. 

+	'mwe-upwiz-api-error-stashfailed' => 'The wiki could not store the file.',

Temporarily or always? 

+	'mwe-upwiz-api-error-missingparam' => 'The upload didn\'t have all the required information (probably a bug in this uploader.)',

Should it print also the param to aid in bug reporting? Is it possible to automatically log these cases to somewhere? 

+	'mwe-upwiz-api-error-invalid-session-key' => 'The server couldn\'t find that file in your uploaded files.',

Which file? We have quite standard wording for session errors in MediaWiki itself. 

+	'mwe-upwiz-api-error-copyuploaddisabled' => 'Uploads by copying are disabled.',

Should try to say more clearly what Upload by copy is. Is it the same thing as upload from url? 

+	'mwe-upwiz-api-error-mustbeloggedin' => 'You are not properly logged in.',

properly? is there some kind of middle state? 

+	'mwe-upwiz-api-error-file-too-large' => 'The file you submitted was too large.',

Can this be catched before upload? Are the limits shown anywhere? 

+	'mwe-upwiz-api-error-filetype-banned' => 'This type of file is banned.',

?Files of this type cannot be uploaded? 

+	'mwe-upwiz-api-error-illegal-filename' => 'The filename is not allowed.',

Does the user after seeing this error have any idea how to fix it? 

+	'mwe-upwiz-api-error-overwrite' => 'Overwriting an existing file is not allowed.',

Is this true? I think some users can do that. Also, in quick reading of the API code, I did not find place where this error could be detected. 

+	'mwe-upwiz-api-error-badtoken' => 'The "token" we use to identify you to the server was bad.',

"token"? Is it a real token or not? 

+	'mwe-upwiz-api-warning-duplicate' => 'There is another file already on the wiki with the same content',

Can we provide the filename(s) too?

#Comment by NeilK (talk | contribs)   11:52, 22 November 2010

General notes:

  • The whole point of error messages in this interface is to give the user some way to guess whether they should try uploading the file again or give up and move on. So, we don't always need 100% of the information.
  • many of these entries are included only for completeness; these are pretty much all the errors you can possibly have.
  • yes a lot of these error messages are almost useless to the average user, but they should be incredibly rare or impossible. Nevertheless we might as well have something in the code, since it is theoretically possible for the server to emit these messages.
  • many of these errors will have additional info in API response that the user could act upon, but I am deferring doing anything about that. That might imply a "more info..." interface for the error, and I haven't thought that through yet.
  • As for wording, we can argue.

Now, on to individual comments:

  • Does this mean we didn't get any reply from the server?

It means that the server returned 200 OK but the response was entirely blank. This happens sometimes, for reasons I haven't figured out.

  • Is $1 a error code or explanation (in English?)

It's the error code. This could occur if an error code is added or changed on the server for which we have no response on the client. This should never happen, but if it does there's at least some hope of fixing it if this can be reported correctly.

  • "Uploading of files"

I just disagree, that doesn't sound nice to my ears.

  • "Upload by copy"

This is included for sheer completeness.

  • nomodule

This should never ever happen; it would mean that the UploadFromFile module was missing.

  • badaccess-groups

Feel free to revise wording

  • missing-param

Yes, it should print the param. This info is available, I just wrote this error handler in a hurry. Again, this error should virtually NEVER happen.

  • stashfailed -- temporarily or always?

As mentioned above, the user will have the opportunity to try again. If it keeps failing maybe they can just give up and move on.

  • invalid-session-key

So where is this standard wording?

Also, I don't want to use "session" here as we may not always depend on session functionality. It's just one way of associating the file with the user.

  • file-too-large -- can this be catched before upload? Are the limits shown anywhere?

In older browsers, we have no idea what the file size is before getting a response from the server. We don't actually have an "HTML5" upload handler yet, but I want to do that once the first cut of this is finally out there.

That's a good point that the limits should be shown somewhere.

  • illegal-filename

Just a placeholder message for now. Probably the bad filename, and maybe even the reason, is returned in info. Have not tested this significantly.

  • filetype-banned

This wording was taken directly from the API, it's just that the API hardcodes it in english

  • overwrite

I am unsure what this message refers to. It may be obsolete.

  • badtoken -- why use quotation marks?

This probably is the first time the user has ever heard of the token, so I wanted to indicate that it was okay if they didn't know what it meant. The idea is to give them an error message that isn't completely baffling that a bug report can make use of. This error should never occur, since before each upload is posted we check for stale tokens.

  • duplicate -- can we provide the filenames

This is available in the extra info from the api return, probably we can, yes

#Comment by Nikerabbit (talk | contribs)   12:19, 22 November 2010
yes a lot of these error messages are almost useless to the average user, but they should be incredibly rare or impossible.

But we know some of them will happen sooner or later :) That's why I asked if it is possible to log them to save the user from the hassle.

The actual wording of error messages is not that significant for the end users (they will probably ignore it anyway and go away or try again), but vague and ambiguous wording will drive translators mad. This set of messages is quite ok from that point of view, fortunately.

Re "token": I read this kind of quoting as something like token, but not really a token. There are even sites like [1] making fun of people using quotes for other purposes. You can avoid the word token altogether by rewording it, if necessary.

#Comment by NeilK (talk | contribs)   20:04, 22 November 2010

I know what you mean by "unnecessary" quotes. I'll see if I can come up with something better. Ideally avoid mentioning the token at all.

I agree that more details ought to be shoved into a debug log of some kind, perhaps accessed by the user via a more info link... right now I haven't worked out the details of how the interface for that works.

#Comment by NeilK (talk | contribs)   10:22, 23 November 2010

Hey, give me a hint as to what messaging you find most problematic. You marked it fixme but some of your comments are more like questions.

#Comment by MarkAHershberger (talk | contribs)   02:51, 2 February 2011

removing fixme. Nikerabbit, could you open a bug with your specific complaints?

#Comment by MarkAHershberger (talk | contribs)   02:59, 2 February 2011

See bug #27098

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:52, 7 February 2011

Realizing that there are further questions about this, there's no pressing issues as far as I see, so I've marked it OK.

Status & tagging log