Fix action reducer forgetting *all* duplicate dwell actions

I was able to track the issue down to the changes made in this patch:
https://gerrit.wikimedia.org/r/331563

This patch is mostly a simplification of ce8a2d4
(I9a73b3086fc8fb0edd897a347b5497d5362e20ef):

- Don't make wait#wait() abortable. This adds complexity and isn't
  needed since the token is rechecked in the .then() of
  actions#linkDwell(). The request is permitted to continue and fetch if
  that token still matches and is never issued otherwise.

  Once a request has been issued, that request is still abortable.
  However, note that calling XHR.abort() is just a request to abort and
  may not be granted. Whether or not XHR.catch() is invoked is what
  dispatches the FETCH_ABORTED action (or doesn't if the request to
  abort was denied).

- Remove the abort tests for wait#wait() since the functionality is no
  longer provided.

- Pass the preview token in the FETCH_ABORTED action and reduce
  FETCH_ABORTED the same way as ABANDON_COMPLETE in reducers/preview.

The follow-up patch, I3597b8025f7a12db0cf5d83cce5a77abace9bae3, adds
integration tests for the specific bug fix. Note that these Selenium
tests are incompatible with the content proxy, so it is probably best to
simply unset $wgPopupsRestGatewayEndpoint and $wgPopupsGateway and allow
the defaults to be used. Also note that the tests are incompatible with
recent versions of Node.js (so use NVM) and emit many deprecation
warnings (so set deprecationWarnings to false in
tests/selenium/wdio.conf.js) An example run of the tests looks something
like:

  chromedriver --url-base=wd/hub --port=4444 &
  # If any changes are made locally, also run `npm -s start &`.
  MW_SERVER=http://localhost:8181 \
  MEDIAWIKI_USER=foo \
  MEDIAWIKI_PASSWORD=bar \
  DISPLAY= \
  npm -s run selenium-test

Live testing may be performed as well. Remember that RESTBase requests
are incompatible with MobileFrontend's content proxy hack so ensure to
comment it out if $wgPopupsGateway is configured for RESTBase (see
T218159).

1. Open the DevTools network tab.
2. Disable the browser cache. Chromium, at least, won't abort requests
   coming form the cache.
3. Hover back and forth quickly over a preview. In Chromium, canceled
   requests are labeled and appear red. This is a good scenario to test.
   With the patch, a preview should always be shown when ultimately
   resting on a link. Without the patch, it is possible to rest upon the
   link with no preview showing. This may require several attempts.

Bug: T219434
Change-Id: I9da84b0296dd14e9ce69cb35f1ca475272fb249a
This commit is contained in:
Thiemo Kreuz 2019-02-05 18:38:22 +01:00 committed by WMDE-Fisch
parent 9c1f918440
commit 065a8e9631
7 changed files with 20 additions and 66 deletions

Binary file not shown.

Binary file not shown.

View file

@ -157,7 +157,8 @@ export function fetch( gateway, title, el, token, type ) {
exception.data = data;
dispatch( {
type,
el
el,
token
} );
// Keep the request promise in a rejected status since it failed.
throw exception;
@ -276,14 +277,16 @@ export function abandon() {
return $.Deferred().resolve().promise();
}
// Immediately abandon any outstanding fetch request. Do not wait.
promise.abort();
dispatch( timedAction( {
type: types.ABANDON_START,
token
} ) );
if ( 'abort' in promise ) {
// Immediately request that any outstanding fetch request be abandoned. Do not wait.
promise.abort();
}
return wait( ABANDON_END_DELAY )
.then( () => {
dispatch( {

View file

@ -34,8 +34,8 @@ export default function preview( state, action ) {
} );
case actionTypes.LINK_DWELL:
// New interaction
if ( action.el !== state.activeLink ) {
// New interaction
return nextState( state, {
activeLink: action.el,
activeEvent: action.event,
@ -52,7 +52,7 @@ export default function preview( state, action ) {
promise: action.promise
} );
}
// Dwelling back into the same link
// Dwelling back into the same link.
return nextState( state, {
isUserDwelling: true
} );
@ -65,6 +65,7 @@ export default function preview( state, action ) {
wasClicked: true
} );
case actionTypes.FETCH_ABORTED:
case actionTypes.ABANDON_END:
if ( action.token === state.activeToken && !state.isUserDwelling ) {
return nextState( state, {

View file

@ -4,13 +4,6 @@
const $ = jQuery;
/**
* A Promise usually for a long running or costly request that is abortable.
* @template T
* @typedef {JQuery.Promise<T>} AbortPromise
* @prop {Function(): void} abort
*/
/**
* Sugar around `window.setTimeout`.
*
@ -23,19 +16,10 @@ const $ = jQuery;
* } );
*
* @param {number} delay The number of milliseconds to wait
* @return {AbortPromise<void>}
* @return {jQuery.Promise}
*/
export default function wait( delay ) {
const deferred = $.Deferred();
const timer = setTimeout( () => {
deferred.resolve();
}, delay );
deferred.catch( () => clearTimeout( timer ) );
return deferred.promise( {
abort() {
deferred.reject();
}
} );
setTimeout( () => deferred.resolve(), delay );
return deferred.promise();
}

View file

@ -130,7 +130,7 @@ QUnit.test( '#linkDwell', function ( assert ) {
timestamp: mw.now(),
title: 'Foo',
namespaceId: 0,
promise: $.Deferred().promise( { abort() {} } )
promise: $.Deferred().promise()
},
'The dispatcher was called with the correct arguments.'
);
@ -360,7 +360,8 @@ QUnit.test( 'it should dispatch the FETCH_FAILED action when the request fails',
this.dispatch.getCall( 1 ).args[ 0 ],
{
type: actionTypes.FETCH_FAILED,
el: this.el
el: this.el,
token: this.token
},
'The dispatcher was called with the correct arguments.'
);
@ -381,7 +382,8 @@ QUnit.test( 'it should dispatch the FETCH_FAILED action when the request fails e
this.dispatch.getCall( 1 ).args[ 0 ],
{
type: actionTypes.FETCH_FAILED,
el: this.el
el: this.el,
token: this.token
},
'The dispatcher was called with the correct arguments.'
);
@ -408,7 +410,8 @@ QUnit.test( 'it should dispatch the FETCH_ABORTED action when the request is abo
this.dispatch.getCall( 1 ).args[ 0 ],
{
type: actionTypes.FETCH_ABORTED,
el: this.el
el: this.el,
token: this.token
},
'The dispatcher was called with the correct arguments.'
);

View file

@ -12,40 +12,3 @@ QUnit.test( 'it should resolve after waiting', function ( assert ) {
);
} );
} );
QUnit.test( 'it should not catch after resolving', function ( assert ) {
return wait( 0 ).catch( () => {
assert.ok( false, 'It never calls a catchable after resolution' );
} ).then( () => {
assert.ok( true, 'It resolves' );
} );
} );
QUnit.test( 'it should not resolve after aborting', function ( assert ) {
const deferred = wait( 0 );
const chain = deferred.then( () => {
assert.ok( false, 'It never calls a thenable after rejection' );
} ).catch( () => {
assert.ok( true, 'It calls the catchable' );
} );
deferred.abort();
return chain;
} );
QUnit.test( 'it should catch after resolving and aborting', function ( assert ) {
const deferred = wait( 0 );
const chain = deferred.catch( () => {
assert.ok( true, 'It calls the catchable' );
} );
deferred.then( () => {
assert.ok( true, 'It resolves' );
deferred.abort();
} );
return chain;
} );