mw.Upload: Refactor error handling for the umpteenth time
authorBartosz Dziewoński <matma.rex@gmail.com>
Thu, 8 Oct 2015 20:10:46 +0000 (22:10 +0200)
committerJames D. Forrester <jforrester@wikimedia.org>
Mon, 12 Oct 2015 19:26:18 +0000 (15:26 -0400)
When I started, I just wanted mw.Upload.BookletLayout to be able to
display more information about errors than just the default message
(like it can do for warnings). And down the rabbit hole I went...

mediawiki/api/upload.js:
* Simply throw errors when our methods are called with bad parameters,
  rather than return a rejected promise.
* Always call .notify( 1 ) when upload is complete, regardless of
  whether it succeeded or failed.
* Reject promises with error code and error details, for consistency
  with api.js. Previous behavior meant that we did not let callers
  know the details in some cases. It was also problematic when we
  passed-through promises rejected in api.js (which had different
  parameters given).
  * Made some effort to return sane codes when something intricate
    fails in iframe upload, but no guarantee that this works well. The
    codes are inspired by what api.js returns in similar circumstances.
  * When rejecting because of warnings, use the first warning's key as
    error code.
  * Always ignore the warnings when uploading to stash and 'filekey'
    is present in response, never ignore when uploading directly.
* When the upload succeeds, never check for 'result.upload.error'
  (which just isn't a thing) nor for 'result.error' (which api.js
  detects and rejects the promise before we get to it). We only need
  to check for 'result.upload.warnings'.

mediawiki.Upload.js:
* Update for the above changes in mediawiki/api/upload.js.
* More reliably distinguish warnings from errors in all cases, not
  only when finishing a stash upload.
* Store machine-readable error codes, not mw.Message objects. This
  lets callers do something sensible when we encounter an unknown
  error (especially one that has no corresponding message).
* Store full result as state details for warnings, as well as errors.

mediawiki.Upload.BookletLayout.js:
* Update for the above changes in mediawiki.Upload.js.
* Give errors/warnings generated during upload to stash the same
  loving treatment as errors/warnings during publishing.
  * Extract the code to a new method getErrorMessageForStateDetails().
* Handle 'stashfailed' warning (which is really an error).
* Handle unknown errors, now that mw.Upload lets us do something
  sensible with them. (See, this is the thing I set out to do.)

Bug: T114940
Change-Id: I4c0f619a4e483cca296c2fa2907ed1f81a99fdd6

resources/Resources.php
resources/src/mediawiki/api/upload.js
resources/src/mediawiki/mediawiki.Upload.BookletLayout.js
resources/src/mediawiki/mediawiki.Upload.js
tests/qunit/suites/resources/mediawiki.api/mediawiki.api.upload.test.js

index c635f0e..4d1b5ef 100644 (file)
@@ -1135,54 +1135,6 @@ return array(
                        'dom-level2-shim',
                        'mediawiki.api.upload',
                ),
-               'messages' => array(
-                       'api-error-badaccess-groups',
-                       'api-error-badtoken',
-                       'api-error-copyuploaddisabled',
-                       'api-error-duplicate',
-                       'api-error-duplicate-archive',
-                       'api-error-empty-file',
-                       'api-error-emptypage',
-                       'api-error-fetchfileerror',
-                       'api-error-fileexists-forbidden',
-                       'api-error-fileexists-shared-forbidden',
-                       'api-error-file-too-large',
-                       'api-error-filename-tooshort',
-                       'api-error-filetype-banned',
-                       'api-error-filetype-banned-type',
-                       'api-error-filetype-missing',
-                       'api-error-hookaborted',
-                       'api-error-http',
-                       'api-error-illegal-filename',
-                       'api-error-internal-error',
-                       'api-error-invalid-file-key',
-                       'api-error-missingparam',
-                       'api-error-missingresult',
-                       'api-error-mustbeloggedin',
-                       'api-error-mustbeposted',
-                       'api-error-noimageinfo',
-                       'api-error-nomodule',
-                       'api-error-ok-but-empty',
-                       'api-error-overwrite',
-                       'api-error-stashfailed',
-                       'api-error-publishfailed',
-                       'api-error-stasherror',
-                       'api-error-stashedfilenotfound',
-                       'api-error-stashpathinvalid',
-                       'api-error-stashfilestorage',
-                       'api-error-stashzerolength',
-                       'api-error-stashnotloggedin',
-                       'api-error-stashwrongowner',
-                       'api-error-stashnosuchfilekey',
-                       'api-error-timeout',
-                       'api-error-unclassified',
-                       'api-error-unknown-code',
-                       'api-error-unknown-error',
-                       'api-error-unknown-warning',
-                       'api-error-unknownerror',
-                       'api-error-uploaddisabled',
-                       'api-error-verification-error',
-               ),
        ),
        'mediawiki.ForeignUpload' => array(
                'scripts' => 'resources/src/mediawiki/mediawiki.ForeignUpload.js',
@@ -1231,6 +1183,52 @@ return array(
                        'upload-form-label-infoform-description',
                        'upload-form-label-usage-title',
                        'upload-form-label-usage-filename',
+                       'api-error-unknownerror',
+                       'api-error-unknown-warning',
+                       'api-error-badaccess-groups',
+                       'api-error-badtoken',
+                       'api-error-copyuploaddisabled',
+                       'api-error-duplicate',
+                       'api-error-duplicate-archive',
+                       'api-error-empty-file',
+                       'api-error-emptypage',
+                       'api-error-fetchfileerror',
+                       'api-error-fileexists-forbidden',
+                       'api-error-fileexists-shared-forbidden',
+                       'api-error-file-too-large',
+                       'api-error-filename-tooshort',
+                       'api-error-filetype-banned',
+                       'api-error-filetype-banned-type',
+                       'api-error-filetype-missing',
+                       'api-error-hookaborted',
+                       'api-error-http',
+                       'api-error-illegal-filename',
+                       'api-error-internal-error',
+                       'api-error-invalid-file-key',
+                       'api-error-missingparam',
+                       'api-error-missingresult',
+                       'api-error-mustbeloggedin',
+                       'api-error-mustbeposted',
+                       'api-error-noimageinfo',
+                       'api-error-nomodule',
+                       'api-error-ok-but-empty',
+                       'api-error-overwrite',
+                       'api-error-stashfailed',
+                       'api-error-publishfailed',
+                       'api-error-stasherror',
+                       'api-error-stashedfilenotfound',
+                       'api-error-stashpathinvalid',
+                       'api-error-stashfilestorage',
+                       'api-error-stashzerolength',
+                       'api-error-stashnotloggedin',
+                       'api-error-stashwrongowner',
+                       'api-error-stashnosuchfilekey',
+                       'api-error-timeout',
+                       'api-error-unclassified',
+                       'api-error-unknown-code',
+                       'api-error-unknown-error',
+                       'api-error-uploaddisabled',
+                       'api-error-verification-error',
                        'fileexists',
                        'filepageexists',
                        'filename-bad-prefix',
index 4d6b34c..614c001 100644 (file)
                return nonce++;
        }
 
+       /**
+        * @private
+        * Given a non-empty object, return one of its keys.
+        *
+        * @param {Object} obj
+        * @return {string}
+        */
+       function getFirstKey( obj ) {
+               for ( var key in obj ) {
+                       if ( obj.hasOwnProperty( key ) ) {
+                               return key;
+                       }
+               }
+       }
+
        /**
         * @private
         * Get new iframe object for an upload.
                        }
 
                        if ( !file ) {
-                               return $.Deferred().reject( 'No file' );
+                               throw new Error( 'No file' );
                        }
 
                        canUseFormData = formDataAvailable() && file instanceof window.File;
 
                        if ( !isFileInput && !canUseFormData ) {
-                               return $.Deferred().reject( 'Unsupported argument type passed to mw.Api.upload' );
+                               throw new Error( 'Unsupported argument type passed to mw.Api.upload' );
                        }
 
                        if ( canUseFormData ) {
                        $iframe.one( 'load', function () {
                                $iframe.one( 'load', function () {
                                        var result = processIframeResult( iframe );
+                                       deferred.notify( 1 );
 
                                        if ( !result ) {
-                                               deferred.reject( 'No response from API on upload attempt.' );
-                                       } else if ( result.error || result.warnings ) {
-                                               if ( result.error && result.error.code === 'badtoken' ) {
+                                               deferred.reject( 'ok-but-empty', 'No response from API on upload attempt.' );
+                                       } else if ( result.error ) {
+                                               if ( result.error.code === 'badtoken' ) {
                                                        api.badToken( 'edit' );
                                                }
 
-                                               deferred.reject( result.error || result.warnings );
+                                               deferred.reject( result.error.code, result );
+                                       } else if ( result.upload && result.upload.warnings ) {
+                                               deferred.reject( getFirstKey( result.upload.warnings ), result );
                                        } else {
-                                               deferred.notify( 1 );
                                                deferred.resolve( result );
                                        }
                                } );
                        } );
 
                        $iframe.error( function ( error ) {
-                               deferred.reject( 'iframe failed to load: ' + error );
+                               deferred.reject( 'http', error );
                        } );
 
                        $iframe.prop( 'src', 'about:blank' ).hide();
                        } );
 
                        if ( !data.filename && !data.stash ) {
-                               return $.Deferred().reject( 'Filename not included in file data.' );
+                               throw new Error( 'Filename not included in file data.' );
                        }
 
                        if ( this.needToken() ) {
                        data.file = file;
 
                        if ( !data.filename && !data.stash ) {
-                               return $.Deferred().reject( 'Filename not included in file data.' );
+                               throw new Error( 'Filename not included in file data.' );
                        }
 
                        // Use this.postWithEditToken() or this.post()
                                }
                        } )
                                .done( function ( result ) {
-                                       if ( result.error || result.warnings ) {
-                                               deferred.reject( result.error || result.warnings );
+                                       deferred.notify( 1 );
+                                       if ( result.upload && result.upload.warnings ) {
+                                               deferred.reject( getFirstKey( result.upload.warnings ), result );
                                        } else {
-                                               deferred.notify( 1 );
                                                deferred.resolve( result );
                                        }
                                } )
-                               .fail( function ( result ) {
-                                       deferred.reject( result );
+                               .fail( function ( errorCode, result ) {
+                                       deferred.notify( 1 );
+                                       deferred.reject( errorCode, result );
                                } );
 
                        return deferred.promise();
                                api = this;
 
                        if ( !data.filename ) {
-                               return $.Deferred().reject( 'Filename not included in file data.' );
+                               throw new Error( 'Filename not included in file data.' );
                        }
 
                        function finishUpload( moreData ) {
                                data.format = 'json';
 
                                if ( !data.filename ) {
-                                       return $.Deferred().reject( 'Filename not included in file data.' );
+                                       throw new Error( 'Filename not included in file data.' );
                                }
 
                                return api.postWithEditToken( data ).then( function ( result ) {
-                                       if ( result.upload && ( result.upload.error || result.upload.warnings ) ) {
-                                               return $.Deferred().reject( result.upload.error || result.upload.warnings ).promise();
+                                       if ( result.upload && result.upload.warnings ) {
+                                               return $.Deferred().reject( getFirstKey( result.upload.warnings ), result ).promise();
                                        }
                                        return result;
                                } );
                        }
 
-                       return this.upload( file, { stash: true, filename: data.filename } ).then( function ( result ) {
-                               if ( result && result.upload && result.upload.filekey ) {
+                       return this.upload( file, { stash: true, filename: data.filename } ).then(
+                               function ( result ) {
                                        filekey = result.upload.filekey;
-                               } else if ( result && ( result.error || result.warning ) ) {
-                                       return $.Deferred().reject( result );
+                                       return finishUpload;
+                               },
+                               function ( errorCode, result ) {
+                                       if ( result && result.upload && result.upload.filekey ) {
+                                               // Ignore any warnings if 'filekey' was returned, that's all we care about
+                                               filekey = result.upload.filekey;
+                                               return $.Deferred().resolve( finishUpload );
+                                       }
+                                       return $.Deferred().reject( errorCode, result );
                                }
-
-                               return finishUpload;
-                       } );
+                       );
                },
 
                needToken: function () {
index c4b8c09..05afbe3 100644 (file)
                        deferred.resolve();
                        layout.emit( 'fileUploaded' );
                }, function () {
-                       // These errors will be thrown while the user is on the info page
-                       if ( layout.upload.getState() === mw.Upload.State.ERROR ) {
-                               deferred.reject( new OO.ui.Error( layout.upload.getStateDetails(), {
-                                       recoverable: false
-                               } ) );
-                               return false;
-                       }
-                       if ( layout.upload.getState() === mw.Upload.State.WARNING ) {
-                               deferred.reject( new OO.ui.Error( layout.upload.getStateDetails(), {
-                                       recoverable: false
-                               } ) );
-                               return false;
-                       }
+                       // These errors will be thrown while the user is on the info page.
+                       // Pretty sure it's impossible to get a warning other than 'stashfailed' here, which should
+                       // really be an error...
+                       var errorMessage = layout.getErrorMessageForStateDetails();
+                       deferred.reject( errorMessage );
                } );
 
                // If there is an error in uploading, come back to the upload page
                                deferred.resolve();
                                layout.emit( 'fileSaved' );
                        }, function () {
-                               var stateDetails = layout.upload.getStateDetails();
-
-                               if ( layout.upload.getState() === mw.Upload.State.ERROR ) {
-                                       deferred.reject( new OO.ui.Error( stateDetails, {
-                                               recoverable: false
-                                       } ) );
-                                       return false;
-                               }
-
-                               if ( layout.upload.getState() === mw.Upload.State.WARNING ) {
-                                       // We could get more than one of these errors, these are in order
-                                       // of importance. For example fixing the thumbnail like file name
-                                       // won't help the fact that the file already exists.
-                                       if ( stateDetails.exists !== undefined ) {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'filepageexists', stateDetails.exists ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       } else if ( stateDetails.duplicate !== undefined ) {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'fileexists', stateDetails.duplicate[ 0 ] ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       } else if ( stateDetails[ 'thumb-name' ] !== undefined ) {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'filename-thumb-name' ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       } else if ( stateDetails[ 'bad-prefix' ] !== undefined ) {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'filename-bad-prefix', stateDetails[ 'bad-prefix' ] ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       } else if ( stateDetails[ 'duplicate-archive' ] !== undefined ) {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'api-error-duplicate-archive', stateDetails[ 'duplicate-archive' ] ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       } else if ( stateDetails.badfilename !== undefined ) {
-                                               // Change the name if the current name isn't acceptable
-                                               layout.filenameWidget.setValue( stateDetails.badfilename );
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               mw.message( 'badfilename', stateDetails.badfilename ).parse()
-                                                       )
-                                               ) );
-                                       } else {
-                                               deferred.reject( new OO.ui.Error(
-                                                       $( '<p>' ).html(
-                                                               // Let's get all the help we can if we can't pin point the error
-                                                               mw.message( 'api-error-unknown-warning', JSON.stringify( stateDetails ) ).parse()
-                                                       ),
-                                                       { recoverable: false }
-                                               ) );
-                                       }
-
-                                       return false;
-                               }
+                               var errorMessage = layout.getErrorMessageForStateDetails();
+                               deferred.reject( errorMessage );
                        } );
                } );
 
                return deferred.promise();
        };
 
+       /**
+        * Get an error message (as OO.ui.Error object) that should be displayed to the user for current
+        * state and state details.
+        *
+        * @protected
+        * @returns {OO.ui.Error} Error to display for given state and details.
+        */
+       mw.Upload.BookletLayout.prototype.getErrorMessageForStateDetails = function () {
+               var message,
+                       state = this.upload.getState(),
+                       stateDetails = this.upload.getStateDetails(),
+                       error = stateDetails.error,
+                       warnings = stateDetails.upload && stateDetails.upload.warnings;
+
+               if ( state === mw.Upload.State.ERROR ) {
+                       message = mw.message( 'api-error-' + error.code );
+                       if ( !message.exists() ) {
+                               message = mw.message( 'api-error-unknownerror', JSON.stringify( stateDetails ) );
+                       }
+                       return new OO.ui.Error(
+                               $( '<p>' ).html(
+                                       message.parse()
+                               ),
+                               { recoverable: false }
+                       );
+               }
+
+               if ( state === mw.Upload.State.WARNING ) {
+                       // We could get more than one of these errors, these are in order
+                       // of importance. For example fixing the thumbnail like file name
+                       // won't help the fact that the file already exists.
+                       if ( warnings.stashfailed !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'api-error-stashfailed' ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings.exists !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'filepageexists', warnings.exists ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings.duplicate !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'fileexists', warnings.duplicate[ 0 ] ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings[ 'thumb-name' ] !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'filename-thumb-name' ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings[ 'bad-prefix' ] !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'filename-bad-prefix', warnings[ 'bad-prefix' ] ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings[ 'duplicate-archive' ] !== undefined ) {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'api-error-duplicate-archive', warnings[ 'duplicate-archive' ] ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       } else if ( warnings.badfilename !== undefined ) {
+                               // Change the name if the current name isn't acceptable
+                               // TODO This might not really be the best place to do this
+                               this.filenameWidget.setValue( warnings.badfilename );
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               mw.message( 'badfilename', warnings.badfilename ).parse()
+                                       )
+                               );
+                       } else {
+                               return new OO.ui.Error(
+                                       $( '<p>' ).html(
+                                               // Let's get all the help we can if we can't pin point the error
+                                               mw.message( 'api-error-unknown-warning', JSON.stringify( stateDetails ) ).parse()
+                                       ),
+                                       { recoverable: false }
+                               );
+                       }
+               }
+       };
+
        /* Form renderers */
 
        /**
index 007c855..1432912 100644 (file)
         * Sets the state and state details (if any) of the upload.
         *
         * @param {mw.Upload.State} state
-        * @param {string|Object} stateDetails
+        * @param {Object} stateDetails
         */
        UP.setState = function ( state, stateDetails ) {
                this.state = state;
                        upload.setState( Upload.State.UPLOADED );
                        upload.imageinfo = result.upload.imageinfo;
                        return result;
-               }, function () {
-                       upload.setState( Upload.State.ERROR );
+               }, function ( errorCode, result ) {
+                       if ( result && result.upload && result.upload.warnings ) {
+                               upload.setState( Upload.State.WARNING, result );
+                       } else {
+                               upload.setState( Upload.State.ERROR, result );
+                       }
+                       return $.Deferred().reject( errorCode, result );
                } );
        };
 
                } ).then( function ( finishStash ) {
                        upload.setState( Upload.State.STASHED );
                        return finishStash;
-               }, function ( result ) {
-                       upload.setState( Upload.State.ERROR, mw.message( 'api-error-' + result ) );
+               }, function ( errorCode, result ) {
+                       if ( result && result.upload && result.upload.warnings ) {
+                               upload.setState( Upload.State.WARNING, result );
+                       } else {
+                               upload.setState( Upload.State.ERROR, result );
+                       }
+                       return $.Deferred().reject( errorCode, result );
                } );
 
                return this.stashPromise;
                                upload.setState( Upload.State.UPLOADED );
                                upload.imageinfo = result.upload.imageinfo;
                                return result;
-                       }, function ( result ) {
-                               // Errors are strings that can be used to get error message
-                               if ( typeof result === 'string' ) {
-                                       upload.setState( Upload.State.ERROR, mw.message( 'api-error-' + result ) );
-                                       return;
-                               }
-
-                               // Warnings come in the form of objects
-                               if ( $.isPlainObject( result ) ) {
+                       }, function ( errorCode, result ) {
+                               if ( result && result.upload && result.upload.warnings ) {
                                        upload.setState( Upload.State.WARNING, result );
-                                       return;
+                               } else {
+                                       upload.setState( Upload.State.ERROR, result );
                                }
-
-                               // Throw an empty error if we can't figure it out
-                               upload.setState( Upload.State.ERROR );
+                               return $.Deferred().reject( errorCode, result );
                        } );
                } );
        };
index 030e703..10fcd5d 100644 (file)
@@ -5,8 +5,9 @@
                QUnit.expect( 2 );
                var api = new mw.Api();
                assert.ok( api.upload );
-               // The below will return a rejected deferred, but that's OK.
-               assert.ok( api.upload() );
+               assert.throws( function () {
+                       api.upload();
+               } );
        } );
 
        QUnit.test( 'Set up iframe upload', function ( assert ) {