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
Introducing the REFERENCE_CLICK action that will fetch and show the
preview for the clicked reference right away without any delay.
The main goal of the new chain of events introduced with the reference
click is showing the reference preview right away. The actions triggered
by the dwelling include delays in multiple parts of the process.
If there's a dwell action-chain in progress when the click action is
executed, the related promise ( that might still include steps with
delays ) and the reference preview is retrieved and shown right away
re-using the token.
In the case where there's no dwell action running ( e.g. when the click
was triggered via touch ) we create a new token and start from scratch.
In either case we want to avoid, that multiple clicks trigger multiple
actions and abort early when there's already a click action in progress.
Bug: T218765
Change-Id: I073a93be2d17a21178aebe12267765f60a2811b9
Pretty much all usages of this function do *not* use the second
parameter to pass a page name prefixed with a namespace, but pass the
page name only.
This patch here removes the redundancy. The namespace prefix is now a
generated one, saying "Namespace <number>:…". It turns out no test
relies on this so far.
Bug: T220097
Change-Id: Ibd45d49c91e86a2647afe676a5e3bb07dfeab6ed
This patch also removes misplaced empty lines at the beginning of a
scope. In PHP code we even have a sniff for these. In JavaScript we
don't, but I suggest to be consistent about this.
Change-Id: Ic104ae8fe176da1dafa9bc783402adecb71de1f0
It's not exclusively about page summaries any more.
We had a few suggestions in mind:
* get, fetch, request, or issueRequest. But I feel these are all to
generic and don't describe well what the method does. As a reminder:
It expects a Title object and returns a promise, which returns a
PreviewModel object, which contains an HTML "extract".
* fetchPreview? I feel this can still mean to many things.
* fetchPreviewModel? But we don't really need to repeat that it will
return a model object.
So I went for fetchPreviewForTitle. What do you think?
Bug: T213415
Change-Id: Icb32c63cec82f72453dc1507c9f8b8d461fd4f4c
This is split from the current draft patch Ie0ccb03. This is part of a
series of very small patches that prepare the code for new types of popups.
Bug: T213415
Change-Id: I00d46a716c0e6ada82ffc0034a7dd5582363c657
Currently the Popups schema is disabled but if we were to re-enable
it, the page tokens would now be consistent with the ReadingDepth
schema
The getToken function is kept to allow generation of the
pageInteractionToken's relating to a link interaction, for which
there is no centralised API.
Additional changes:
* I've updated various tests to use the central mw.user stub to make
clearer where tokens come from.
Bug: T203013
Change-Id: If746bea5aeed2b4c192a9b8a02feb1fe06480633
Let's improve our documentation by linting it and ensuring it
is complete and matches guidelines
This fixes offenders
Change-Id: I7c829b375705e763085cf731e9a77cc14339af67
Whe user moves mouse away and we abort the http request we shouldn't
count that request as a FETCH_FAILED. The reasoning behind is that
FETCH_FAILED state increments the counter.PagePreviewsApiFailure.
Our StatsD graph gets polluted with lots of aborted requests and it
becomes unsuable. It doesn't show only the failed requests.
Changes:
- introduced new state: FETCH_ABORTED
- switch to FETCH_ABORTED when browser aborts the request
Bug: T199482
Change-Id: I58047eb80f0700b78b2991daff9395ecc92553b8
Whenever an HTTP request sequence is started, i.e. wait for the fetch
start time, issue a network request, and return the result, abort the
process if the results are known to no longer be needed. This occurs
when a user has dwelt upon one link and then abandoned it either during
the fetch start wait time or during the fetch network request itself.
This change is accomplished by preserving the pending promises in two
actions, LINK_DWELL and FETCH_START, and whenever the ABANDON_START
action is issued, it now aborts any previously pending XHR-like promise,
called a "AbortPromise" which is just a thenable with an abort() method.
There is a similar concept in Core:
ecc812f06e/resources/src/mediawiki.api/index.js.
Aborting pending requests has big implications for client and server
logging as requests are quickly canceled, especially on slower
connections. These differences can be observed on the network tab of
DevTools and the log in Redux DevTools.
Consider, for instance, the scenario of dwelling upon and quickly
abandoning a single link prior to this patch:
BOOT EVENT_LOGGED LINK_DWELL FETCH_START ABANDON_START FETCH_END STATSV_LOGGED ABANDON_END EVENT_LOGGED FETCH_COMPLETE
And after this patch when the fetch timer is canceled (prior to an
actual network request):
BOOT EVENT_LOGGED LINK_DWELL ABANDON_START ABANDON_END EVENT_LOGGED
In the above sequence, FETCH_* and STATSV_LOGGED actions never occur.
And after this patch when the network request itself is canceled:
BOOT EVENT_LOGGED LINK_DWELL FETCH_START ABANDON_START FETCH_FAILED STATSV_LOGGED FETCH_COMPLETE ABANDON_END EVENT_LOGGED
FETCH_FAILED occurs intentionally, STATSV_LOGGED and FETCH_COMPLETE
still happen even though the fetch didn't complete successfully, and
FETCH_END doesn't.
Additionally, since less data is transmitted, it's possible that the
timing and success rate of logging will improve on low bandwidth
connections.
Also, this patch tries to revise the JSDocs where possible to support
type checking and fix a call to the missing assert.fail() function in
changeListener.test.js.
Bug: T197700
Change-Id: I9a73b3086fc8fb0edd897a347b5497d5362e20ef
Via jscodeshift:
jscodeshift \
-t jscodeshift-recipes/src/qunit-assert-equal-to-strictEqual.js \
Popups/tests
Also, some very minor manual clean up.
https://github.com/niedzielski/jscodeshift-recipes/blob/5944e50/src/qunit-assert-equal-to-strictEqual.js
Additional change:
* Drop redundant clipPath parameter from createThumbnailElement - this
parameter does not exist in the function signature.
Change-Id: I209ecf2d54b6f5c17767aa2041d8f11cb368a9b5
Replace all test assertions for calledOnce / Twice with callCount.
assert.ok( calledOnce / Twice ) only lets the dev know that a test
fails. assert.strictEqual( callCount, EXPECTATION ) starts the debugging
process when it fails since it provides the difference in the failure
output. strictEqual() was deliberately used since it's a saner default
and the codebase already favors === equivalency checks.
find tests -name \*.js|
xargs -rd\\n sed -ri '
s%ok\(([^,]+)calledOnce%strictEqual(\1callCount, 1%g;
s%ok\(([^,]+)calledTwice%strictEqual(\1callCount, 2%g;
'
Change-Id: I07c3c6d20e07c5b8107583a01d820e3fbd68a4e1
This change updates the schema and begins to log
additional information such as source_namespace, id
and title. This information is provided inside the
boot action.
Additional changes:
* Allow camel case in bundle artifact
Bug: T184793
Bug: T186728
Change-Id: I425ffecc018bef2958d0dfe957a40a065e3e6c56
Pageview is consistent with verbiage used by Research and Analytics
Engineering in their reports and documentation, e.g.
https://wikitech.wikimedia.org/wiki/Analytics/Pageviews.
Bug: T184793
Change-Id: I8ae085b4af85aa72f234f3db27f0cac2c4d014e5
* New action added PREVIEW_SEEN
* The action will be used to signal that a page view needs
to be recorded.
* PREVIEW_SEEN is a delayed action which is triggered
as a side-effect of the previewShow action. It is only dispatched
if the user is still previewing the same card and the page
related to the card has preview type `page`
* The pageview changelistener is added when
$wgPopupsVirtualPageViews is set to true.
* The page view changelistener listens for page views and logs
them using EventLogging when needed using
https://meta.wikimedia.org/wiki/Schema:VirtualPageView
Note:
* Currently if a user has enabled the DNT header, the
event will not be logged. There is ongoing discussion on the
ticket and fixing this will be addressed separately.
* Only title and referrer are logged in the initial version.
The task demands that "namespace" is logged but this information
is not provided by the summary endpoints we use so will need
to be added later (if indeed needed) either via a change to that
endpoint of by using JavaScript to parse the URL.
Bug: T184793
Change-Id: Id1fe34e4bdada3a41f0d888a753af366d4756590
This change fixes some issues with assertions not running, removes
unnecessary promise dances, and improves legibility and some code
patterns in the action and integration node tests mainly.
Detailed changes:
* actions.js
* Fully migrate out of jQuery 1 promises (no done/fail)
* Fix linkDwell action not returning the fetch action promise
* actions.test.js
* Simplify setupWait for the tests
* It always autoresolves immediately the wait call to ensure speedy tests
* No waitDeferreds or waitPromises array coordination, rely on action
returned promises instead
* Get rid of that = this in favor of arrow functions
* Rename generic "p" promises to meaningful names
* Add assert.expect for more solid tests (so that we don't skip assertions in
the future if we change them)
* Fix some assertions that weren't being run because of the incorrect promise
being returned (p.then, and then just returning p)
* Get rid of $.when stubbing in favor of waiting for the promise returned from
the action
* Get rid of hacky setTimeout(..., 0) to run assertions after the promises
* integration.test.js
* Get rid of wait(0) calls to hack around asynchronous actions
* Use the action returned promises instead of the waitPromises/Deferreds
* Remove unused "el" parameter being passed to this.abandon in several tests
* Remove unnecessary test helper this.abandonPreview (it was the same as
this.abandon)
* Clarify a bit the last and more complex test with some comments and variable
name changes
* Get rid of that=this in favor of arrow functions
* container.test.js
* Get rid of that=this in favor of arrow functions
* previewBehavior.test.js
* Get rid of that=this in favor of arrow functions
* Get rid of $.each in favor of .forEach
Bug: T170807
Change-Id: I06fafd92a1712f143018e2ddff24fadf1a6882b3
Returning promises from the `linkDwell` and `abandon` thunks and
removing some of the `wait` stubs in the unit test for these actions.
Also converting a fetch callback from a `.fail` to a Promise A+
compatible `.catch`.
Bug: T170807
Change-Id: I4bbf2863db090e222ba926d3bc36a99da4bdb601
Enforce it with eslint.
Ignore:
* Comment lines with eslint disable directives
* QUnit test lines as they contain long subjects (QUnit.* (only, test,
module, skip, etc)
* Strings, since long strings are used extensively in tests
* Ignore template literals for similar reasons
* Regex literals as they may be too long, but can't be easily
split in several lines
* Long urls
See bug for more general proposal for eslint-wikimedia-config.
Bug: T185295
Change-Id: I3aacaf46e61a4d96547c513073e179ef997deb09
Test subject was changed and stopped matching the implementation. In
this particular change the test (a bit convoluted but) tests that wait
is called appropriately, which is why the subject read "should delay
dispatching ..."
Change-Id: I3c8d9d8769f3d1c2869a267af105b9489df86cf5
Functional changes
- Show the default / error preview for all extract request failures
except those due to network circumstances (such as CORS) or no
connectivity (offline). Previously, the error preview was displayed
only for missing pages.
- FETCH_COMPLETE was previously only dispatched after FETCH_END. Now
it's also dispatched after FETCH_FAILED. The additional "fetch
complete" is not expected to impact logging. The states of success
are: START, END, COMPLETE. The new failure states are consistent with
success: START, FAILED, COMPLETE.
Testing
Errors may be stimulated in a number of ways including:
- Timeout: add a timeout field to RESTBaseGateway /
MediaWikiGateway.fetch().
http://api.jquery.com/jquery.ajax/
- Bad request: change MediaWikiGateway.fetch's action field to
`Math.random() > 0.5 ? 'query' : 'fail'` and RESTBaseGateway.fetch's
url field to
`RESTBASE_ENDPOINT + ( Math.random() > 0.5 ? encodeURIComponent( title ) : '%%%' )`.
- Desired Gateway can be configured in Gateway#createGateway().
- Note: T184534 describes a circumstance where cached previews may not
appear when offline. Disable browser caching to avoid confusion.
Bug: T183151
Bug: T184534
Change-Id: I7332284da0e0fb1ecd234a6f1e146ebd9ad8d81f
Instead of using the non-standard old Promise implementation by relying
on .fail, migrate it to .catch.
In this specific case, where we were chaining promises with $.when, the
semantics from going from fail to catch actually change. .fail keeps the
promise in a rejected state, while .catch will change it to resolved
unless another error is thrown.
As such, when changing it to .catch, the catched error will be re-thrown
to keep the promise in a rejected state, so that $.when.then is not
executed.
Additional changes:
* Make actions.js#fetch return its promise for ease of testing.
* Test FETCH_FAILED, which was fully untested.
Bug: T173819
Change-Id: Ibd380b714586979c6e60b4c969d17f36a0796b52
Simplify all our tests to return to promises
Use catch rather than fail when testing error cases.
Bug: T170812
Change-Id: I37c4e3f86343052c946d8586f8ff840a81f631f8
Why: Because they are the approved standard by TC39 and Ecma for
JavaScript modules.
Changes:
* Wrap mw-node-qunit in run.js to register babel to transpile modules
for node v6
* Change all sources in src/ to use ES modules
* Change constants.js to be able to run without
jQuery.bracketedDevicePixelRatio given ES modules are hoisted to
the top by spec so we can't patch globals before importing it
* Change all tests in tests/node-qunit/ to use ES modules
* Drop usage of mock-require given ES modules are easy to stub with
sinon
Additional changes:
* Rename tests/node-qunit/renderer.js to renderer.test.js to follow
the convention of all the other files
* Make npm run test:node run only .test.js test files so that it
doesn't run the stubs.js or run.js file.
Bug: T171951
Change-Id: I17a0b76041d5e2fd18e2d54950d9d7c0db99a941
...instead of 1 event per link
Supporting changes:
* Delegate events on the container when booting up
* Check eligibility of title on event triggered
* Pass the title from the event handlers into the actions instead of
storing it in the dom
* Add title.fromElement as sugar over isvalid(gettitle())
* Not tested as it is sugar over the other 2 functions
* Fix action tests and integration tests
processLinks to be removed next
Bug: T165572
Change-Id: I4d9837706dc77ec64121ac94410c0d2da83692e4
This fixes a bug in I8a63d82, where the pageTitleHover and
namespaceIdHover properties of EventLogging events weren't being set.
Bug: T164256
Change-Id: Ie2c2d253f6508b89d48129fd17a902e5ded7cad5
Action changes:
* Include the namespace ID in LINK_DWELL.
Reducer changes:
* Add the createEvent helper, which adds the pageTitleHover and
namespaceIdHover properties in the event.
* Create "dismissed", "dwelledButAbandoned", and "opened" events using
the createEvent helper.
Additional changes:
* Store the target page's associated mw.Title using jQuery.
* Update the eventLogging reducer's test cases so that all LINK_DWELL
representations include title and namespaceID attributes.
* Create the createStubTitle factory function, which returns a minimal
usable mw.Title stub, and use it in the actions and integration test
cases.
Bug: T164256
Change-Id: I8a63d82a65324680dff9176020a8ea97695428c4
The setup and teardown hooks on QUnit.module are deprecated on the
latest versions.
See https://api.qunitjs.com/QUnit/module
mw-node-qunit supports them but we shouldn't be using them.
Bug: T160406
Change-Id: I32c07f22d01d16449a6e37f46ff20c577a1f14c6
If the user CmdOrCtrl+Clicks on a link, then the link will remain
focussed. If a preview is shown, clicks another element on the page, and
then there'll be no token to include in ABANDON_* with.
Bug: T162924
Change-Id: Ie2237aa55ea9a11070498b66c73b8bf1898d8d44
Mirroring all other actions that are dispatched after some delay, add
the token to the PREVIEW_SHOW action.
Supporting changes:
* Pass the token to ext.popups.Preview#show so that it can be passed to
actions#previewShow.
Bug: T159490
Change-Id: I128fd56e770ed09d5d0dc55db73d11b013049c79
Require that two promises are resolved (or one is rejected) before the
FETCH_COMPLETE action is dispatched. The first promise represents the
gateway request and the second represents an arbitrarily long delay. If
the first resolves before the second, then there'll be a delay until the
second resolves; whereas if the first rejects, then there's no delay.
Change-Id: I496fe317337745c593594efff26688c46d661bf3
The delay/timeout logic in actions#fetch will require more than one wait
call, for example.
Changes:
* Update the stub created in setupWait to store all deferred-promise
pairs and update the integration tests so that they don't use the stub
accidentally.
* Remove all references to the waitDeferred/waitPromise properties.
* Fix tests that relied on the waitDeferred/waitPromise properties being
available regardless of whether wait had been called.
* Update outdated or brittle - in the sense that it didn't reference
constants but their values - inline documentation.
Change-Id: I94345cdf4126b6c540d4fb8135a7a7e4d0507bed
Mixing in the delay was introduced in If3f1a06f so that the total RTT
for an API request could be calculated. Now that the FETCH_END action is
dispatched when the gateway request ends and not when the preview model
is resolved, this additional information (state) is redundant.
Change-Id: I7e6ffe0945ffedd9425525fa7da855e729d50b77
