Matthias Mullie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/206827
Change subject: Refactor flowEventsMixinApiRequestInteractiveHandler
......................................................................
Refactor flowEventsMixinApiRequestInteractiveHandler
That method was (and still is) a huge monster :)
It was hard to consistently do things in there (e.g. error
handling or post-processing cleaning-up)
I've made it a chain of promises. The biggest annoyance is
that the handlers don't return data in a way that would
allow them to easily be chained, so all of those have a
small wrapper around them to re-format the data. That way,
this refactor didn't have to touch api(Pre)Handlers code.
I think this drastically improved to understand what's
happening where.
And now the .flow-api-inprogress is only removed after
*everything* has run, including the apiHandlers.
Bug: T96811
Change-Id: Ibb9a6e846437cbc8ed9b851b9d4ed45f00f8f90d
---
M modules/engine/components/common/flow-component-events.js
M modules/engine/misc/flow-api.js
2 files changed, 136 insertions(+), 123 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/27/206827/1
diff --git a/modules/engine/components/common/flow-component-events.js
b/modules/engine/components/common/flow-component-events.js
index 226fbd4..6f629e5 100644
--- a/modules/engine/components/common/flow-component-events.js
+++ b/modules/engine/components/common/flow-component-events.js
@@ -226,8 +226,8 @@
/**
* Utility to get error message for API result.
*
- * @param string code
- * @param Object result
+ * @param {string} code
+ * @param {Object} result
* @returns string
*/
function flowGetApiErrorMessage( code, result ) {
@@ -255,11 +255,9 @@
* @returns {$.Promise}
*/
function flowEventsMixinApiRequestInteractiveHandler( event ) {
- var $deferred,
- $handlerDeferred,
- handlerPromises = [],
+ var $deferred = $.Deferred(),
+ deferreds = [$deferred],
$target,
- preHandlerReturn,
self = event.currentTarget || event.delegateTarget ||
event.target,
$this = $( self ),
flowComponent = mw.flow.getPrototypeMethod(
'component', 'getInstanceByElement' )( $this ),
@@ -271,7 +269,8 @@
component: flowComponent
},
args = Array.prototype.slice.call( arguments, 0 ),
- queryMap = flowComponent.Api.getQueryMap( self.href ||
self );
+ queryMap = flowComponent.Api.getQueryMap( self.href ||
self ),
+ preHandlers = [];
event.preventDefault();
@@ -292,136 +291,107 @@
// Make sure an API call is not already in progress for this
target
if ( $target.closest( '.flow-api-inprogress' ).length ) {
- flowComponent.debug( false, 'apiRequest already in
progress', arguments );
- return $.Deferred().reject().promise();
+ $deferred.reject( 'fail-api-inprogress', { error: {
info: 'apiRequest already in progress' } } );
+ } else {
+ $deferred.resolve( args );
}
- // Mark the target node as "in progress" to disallow any
further API calls until it finishes
- $target.addClass( 'flow-api-inprogress' );
- $this.addClass( 'flow-api-inprogress' );
+ $deferred.then( function ( args ) {
+ // Mark the target node as "in progress" to disallow
any further API calls until it finishes
+ $target.addClass( 'flow-api-inprogress' );
+ $this.addClass( 'flow-api-inprogress' );
- // Let generic pre-handler take care of edit conflicts
- $.each( flowComponent.UI.events.globalApiPreHandlers, function(
key, callbackArray ) {
- $.each( callbackArray, function ( i, callbackFn ) {
- queryMap = callbackFn.apply( self, args );
- } );
+ // Remove existing errors from previous attempts
+ flowComponent.emitWithReturn( 'removeError', $this );
} );
- // We'll return a deferred object that won't resolve before
apiHandlers
- // are resolved
- $handlerDeferred = $.Deferred();
+ // chain apiPreHandler callbacks
+ preHandlers = _getApiPreHandlers( self, handlerName );
+ $.each( preHandlers, function ( i, callback ) {
+ $deferred = $deferred.then( callback );
+ } );
- // Use the pre-callback to find out if we should process this
- if ( flowComponent.UI.events.apiPreHandlers[ handlerName ] ) {
- // apiPreHandlers can return FALSE to prevent
processing,
- // nothing at all to proceed,
- // or new queryMap (= API params)
- $.each( flowComponent.UI.events.apiPreHandlers[
handlerName ], function ( i, callbackFn ) {
- // make sure queryMap is up to date (may have
been altered by
- // previous preHandler)
- args.splice( 2, 1, queryMap );
- preHandlerReturn = callbackFn.apply( self, args
);
+ // execute API call
+ $deferred = $deferred.then( function ( args ) {
+ var queryMap = args[2];
+ return flowComponent.Api.requestFromNode( self,
queryMap );
+ } );
- if ( preHandlerReturn === false ) {
- // Callback returned false; break out
of this loop
- return false;
- } else {
- queryMap = preHandlerReturn;
+ // alter API response: apiHandler expects a 1st param info (that
+ // includes 'status') & `this` being the target element
+ $deferred = $deferred.then(
+ // success
+ function () {
+ var args = Array.prototype.slice.call(
arguments, 0 );
+ info.status = 'done';
+ args.unshift( info );
+ return $.Deferred().resolveWith( self, args );
+ },
+ // failure
+ function ( code, result ) {
+ var errorMsg,
+ args = Array.prototype.slice.call(
arguments, 0 ),
+ $form = $this.closest( 'form' );
+
+ info.status = 'fail';
+ args.unshift( info );
+
+ /*
+ * In the event of edit conflicts, store the
previous
+ * revision id so we can re-submit an edit
against the
+ * current id later.
+ */
+ if ( result.error && result.error.prev_revision
) {
+ $form.data( 'flow-prev-revision',
result.error.prev_revision.revision_id );
}
- } );
- if ( preHandlerReturn === false ) {
- // Last callback returned false
- flowComponent.debug( false, 'apiPreHandler
returned false', handlerName, args );
+ /*
+ * Generic error handling: displays error
message in the
+ * nearest error container.
+ *
+ * Errors returned by MW/Flow should always be
in the
+ * same format. If the request failed without a
specific
+ * error message, just fall back to some
default error.
+ */
+ errorMsg =
flowComponent.constructor.static.getApiErrorMessage( code, result );
+ flowComponent.debug( false, errorMsg,
handlerName, args );
+ flowComponent.emitWithReturn( 'showError',
$this, errorMsg );
- // Abort any old request in flight; this is
normally done automatically by requestFromNode
flowComponent.Api.abortOldRequestFromNode(
self, queryMap, null );
- // @todo support for multiple indicators on
same target
- $target.removeClass( 'flow-api-inprogress' );
- $this.removeClass( 'flow-api-inprogress' );
-
- return $.Deferred().reject().promise();
+ // keep going & process those apiHandlers;
based on info.status,
+ // they'll know if they're dealing with
successful submissions,
+ // or cleaning up after error
+ return $.Deferred().rejectWith( self, args );
}
+ );
+
+ // chain apiHandler callbacks: run apiHandlers in both success
& failure
+ // cases, it can distinguish in how it needs to wrap up
depending on
+ // info.status
+ if ( flowComponent.UI.events.apiHandlers[ handlerName ] ) {
+ $.each( flowComponent.UI.events.apiHandlers[
handlerName ], function ( i, callback ) {
+ /*
+ * apiHandlers will return promises that won't
resolve until
+ * the apiHandler has completed all it needs to
do.
+ * These handlers aren't chainable, though
(although we only
+ * have 1 per call, AFAIK), they don't return
the same data the
+ * next handler assumes.
+ * In order to suspend something until all of
these apiHandlers
+ * have completed, we'll combine them in an
array which we can
+ * keep tabs on until all of these promises are
done ($.when)
+ */
+ deferreds.push( $deferred.then( callback,
callback ) );
+ } );
}
- // Make the request
- $deferred = flowComponent.Api.requestFromNode( self, queryMap );
- if ( !$deferred ) {
- mw.flow.debug( '[FlowApi] [interactiveHandlers]
apiRequest element is not anchor or form element' );
- $deferred = $.Deferred();
- $deferred.rejectWith( { error: { info: 'Not an anchor
or form' } } );
- }
-
- // Remove the load indicator
- $deferred.always( function () {
- // @todo support for multiple indicators on same target
+ // cleanup
+ $.when.apply( $, deferreds ).always( function() {
$target.removeClass( 'flow-api-inprogress' );
$this.removeClass( 'flow-api-inprogress' );
} );
- // Remove existing errors from previous attempts
- flowComponent.emitWithReturn( 'removeError', $this );
-
- // We'll return a deferred object that won't resolve before
apiHandlers
- // are resolved
- $handlerDeferred = $.Deferred();
-
- // If this has a special api handler, bind it to the callback.
- if ( flowComponent.UI.events.apiHandlers[ handlerName ] ) {
- $deferred
- .done( function () {
- var args = Array.prototype.slice.call(
arguments, 0 );
- info.status = 'done';
- args.unshift( info );
- $.each(
flowComponent.UI.events.apiHandlers[ handlerName ], function ( i, callbackFn ) {
- handlerPromises.push(
callbackFn.apply( self, args ) );
- } );
- } )
- .fail( function ( code, result ) {
- var errorMsg,
- args =
Array.prototype.slice.call( arguments, 0 ),
- $form = $this.closest( 'form' );
-
- info.status = 'fail';
- args.unshift( info );
-
- /*
- * In the event of edit conflicts,
store the previous
- * revision id so we can re-submit an
edit against the
- * current id later.
- */
- if ( result.error &&
result.error.prev_revision ) {
- $form.data(
'flow-prev-revision', result.error.prev_revision.revision_id );
- }
-
- /*
- * Generic error handling: displays
error message in the
- * nearest error container.
- *
- * Errors returned by MW/Flow should
always be in the
- * same format. If the request failed
without a specific
- * error message, just fall back to
some default error.
- */
- errorMsg =
flowComponent.constructor.static.getApiErrorMessage( code, result );
- flowComponent.emitWithReturn(
'showError', $this, errorMsg );
-
- $.each(
flowComponent.UI.events.apiHandlers[ handlerName ], function ( i, callbackFn ) {
- handlerPromises.push(
callbackFn.apply( self, args ) );
- } );
- } )
- .always( function() {
- // Resolve/reject the promised
deferreds when all apiHandler
- // deferreds have been resolved/rejected
- $.when.apply( $, handlerPromises )
- .done( $handlerDeferred.resolve
)
- .fail( $handlerDeferred.reject
);
- } );
- }
-
- // Return an aggregate promise that resolves when all are
resolved, or
- // rejects once one of them is rejected
- return $handlerDeferred.promise();
+ return $deferred.promise();
}
FlowComponentEventsMixin.UI.events.interactiveHandlers.apiRequest =
flowEventsMixinApiRequestInteractiveHandler;
@@ -902,6 +872,49 @@
return $result;
}
+ /**
+ * @param {Element} target
+ * @param {String} handlerName
+ * @returns {Array<function>}
+ * @private
+ */
+ function _getApiPreHandlers( target, handlerName ) {
+ var flowComponent = mw.flow.getPrototypeMethod( 'component',
'getInstanceByElement' )( $( target ) ),
+ preHandlers = [];
+
+ // Compile a list of all preHandlers to be run
+ $.each( flowComponent.UI.events.globalApiPreHandlers, function(
key, callbackArray ) {
+ preHandlers.concat( callbackArray );
+ } );
+ if ( flowComponent.UI.events.apiPreHandlers[ handlerName ] ) {
+ preHandlers = flowComponent.UI.events.apiPreHandlers[
handlerName ];
+ }
+
+ $.each( preHandlers, function ( key, callback ) {
+ /**
+ * apiPreHandlers aren't properly set up to serve as
chained promise
+ * callbacks (they'll return false instead of returning
a rejected
+ * promise, the incoming & outgoing params don't line
up)
+ * This will wrap all those callbacks into callbacks we
can chain.
+ *
+ * @param {object} args
+ * @returns {object}
+ */
+ preHandlers[key] = function ( args ) {
+ var queryMap = callback.apply( target, args );
+ if ( queryMap === false ) {
+ return $.Deferred().reject(
'fail-prehandler', { error: { info: 'apiPreHandler returned false' } } );
+ }
+
+ // update changed queryMap
+ args.splice( 2, 0, queryMap );
+ return args;
+ };
+ } );
+
+ return preHandlers;
+ }
+
// Copy static and prototype from mixin to main class
mw.flow.mixinComponent( 'component', FlowComponentEventsMixin );
}( jQuery, mediaWiki ) );
diff --git a/modules/engine/misc/flow-api.js b/modules/engine/misc/flow-api.js
index 1949950..5b9af60 100644
--- a/modules/engine/misc/flow-api.js
+++ b/modules/engine/misc/flow-api.js
@@ -35,7 +35,7 @@
/**
* Makes the actual API call and returns
* @param {Object|String} [params] May be a JSON object string
- * @param {String} [pageName]
+ * @param {String} [method]
* @returns {$.Promise}
*/
function flowApiCall( params, method ) {
@@ -60,11 +60,11 @@
if ( !params.action ) {
mw.flow.debug( '[FlowApi] apiCall error:
missing action string', arguments );
- return $deferred.rejectWith({ error: 'Invalid
action' }).promise();
+ return $deferred.reject( 'fail-apirequest', {
error: { info: 'Invalid action' } } ).promise();
}
if ( !params.page ) {
mw.flow.debug( '[FlowApi] apiCall error:
missing page string', [ mw.config.get( 'wgPageName' ) ], arguments );
- return $deferred.rejectWith({ error: 'Invalid
title' } ).promise();
+ return $deferred.reject( 'fail-apirequest', {
error: { info: 'Invalid title' } } ).promise();
}
if ( method === 'POST' ) {
@@ -78,7 +78,7 @@
return mwApi.postWithToken( tokenType, params );
} else if ( method !== 'GET' ) {
- return $deferred.rejectWith({ error: 'Unknown
submission method: ' + method }).promise();
+ return $deferred.reject( 'fail-apirequest', {
error: { info: 'Unknown submission method: ' + method } } ).promise();
} else {
return mwApi.get( params );
}
@@ -290,7 +290,7 @@
} else if ( $node.is( 'form, input, button, textarea, select,
option' ) ) {
return this.requestFromForm.apply( this, arguments );
} else {
- return $.Deferred().reject();
+ return $.Deferred().reject( 'fail-apirequest', { error:
{ info: 'apiRequest element is not anchor or form element' } } );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/206827
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb9a6e846437cbc8ed9b851b9d4ed45f00f8f90d
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Matthias Mullie <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
