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
Ideally, the preview model is resolved after 500 ms, regardless of
whether the internal gateway takes 100 or 300 ms. Given this, there's an
important distinction to be made between the "fetch" ending and it
completing and their associated actions.
Changes:
* Dispatch the FETCH_COMPLETE action when the preview model is resolved.
* Update the reducers accordingly.
Change-Id: I62c9cb0430284b76338ea80bd170cac5af4be9d0
For logging to work:
1. $wgWMEStatsdBaseUri needs to point to a valid statsv endpoint,
e.g. 'https://en.wikipedia.org/beacon/statsv'.
2. $wgPopupsStatsvSamplingRate needs to be set. Note that the codebase
already contains the EventLogging functionality, which is configured
separately. Separately configuring different logging mechanisms
allows us to avoid sampling mistakes that may arise while choosing
one or the other. For example, let's say we want to use EventLogging for
10% of users and statsv for 5%. We'd sample all users into two
buckets: 50/50. And then we'd have to set the sampling rates as
20% and 10% respectively, only because of the bucketing above. To avoid
this kind of complications, separate sampling rates are used for each
logging mechanism. This, of course, may result in situations where a
session is logged via both EventLogging and statsv.
3. The WikimediaEvents extension needs to be installed. The extension
adds the `ext.wikimediaEvents` module to the output page. The
logging functionality is delegated to this module.
Notable changes:
* The FETCH_START and FETCH_END actions are converted to a timed action.
* The experiments stub used in tests has been extracted to the stubs
file.
Logged data is visualized at
https://grafana.wikimedia.org/dashboard/db/reading-web-page-previews
Bug: T157111
Change-Id: If3f1a06f1f623e8e625b6c30a48b7f5aa9de24db
Because of the globals mw.popups.wait usage and mocking in both actions
and integration, they need to be migrated in a single step, fixing them
both to require wait.js and mock using mock-require instead of the
global variable.
Additional changes:
* Fix FIXMEs about actions.js using the global mw.popups.wait instead of
the require one.
* Fix the unit tests to use require mocking for wait.js instead of
global variable mocking in both integration and actions tests
* Change tests that use deferreds and promises to be async qunit tests
(Deferreds are asynchronous with jQuery in node, apparently they
weren't in the browser)
* Change integration.test.js to use require on Redux and ReduxThunk
Change-Id: I8e3e87b158bd11c9620e77d0a73e611cf9e82183
