Commit graph

22 commits

Author SHA1 Message Date
Stephen Niedzielski 9114981380 Update: show placeholder preview for more failures
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
2018-01-16 18:44:00 -06:00
joakin e865409593 Hygiene: Don't rely on .fail, use Promises/A+
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
2017-08-23 19:36:05 +02:00
jdlrobson 08331ba199 Return promises from QUnit test
Simplify all our tests to return to promises
Use catch rather than fail when testing error cases.

Bug: T170812
Change-Id: I37c4e3f86343052c946d8586f8ff840a81f631f8
2017-08-17 14:34:39 +00:00
joakin e6081106f1 Use EcmaScript modules instead of common.js modules
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
2017-07-31 23:05:44 +00:00
joakin 002f4c8e0c Use delegated events in container
...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
2017-06-08 12:31:06 -07:00
Sam Smith cf0ea9db7b actions: Mix title and namespaceID into LINK_DWELL
This fixes a bug in I8a63d82, where the pageTitleHover and
namespaceIdHover properties of EventLogging events weren't being set.

Bug: T164256
Change-Id: Ie2c2d253f6508b89d48129fd17a902e5ded7cad5
2017-05-15 19:02:27 +01:00
Sam Smith 35bf613964 eventLogging: Add missing *Hover properties
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
2017-05-13 08:27:58 +01:00
Ed Sanders 2a4a235dec Fix ,->; typos
Change-Id: I9b490fcb35459bc0318506e049fbd573bf17e050
2017-05-03 10:19:19 +01:00
joakin 698a93f5bc Hygiene: QUnit setup -> beforeEach & teardown -> afterEach
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
2017-04-26 12:34:12 +02:00
joakin 2516299bd2 Hygiene: Lint JS files on tests/node-qunit too
For consistency.

Bug: T160406
Change-Id: I2fd67649e9dc4bdb3b963b8ee70ed0234dbcb5ce
2017-04-26 12:34:12 +02:00
Sam Smith 8c611d069f actions: Conditionally dispatch ABANDON_*
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
2017-04-18 20:03:58 +01:00
Sam Smith 77943704f8 actions: Add token to PREVIEW_SHOW
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
2017-04-11 09:26:22 +01:00
Sam Smith d3d9100637 actions: Add token to FETCH_COMPLETE
Bug: T159490
Change-Id: Ic71dd3ce5e7933273f84a9a64d41e7f3a4cb03f4
2017-04-07 14:06:11 +01:00
Sam Smith f86f388fd3 Hygiene: Remove trailing whitespace
... from tests/node-qunit/actions.test.js.

Spotted during the review of Ic3165620.

Change-Id: I0fb78d2dc3aadc6b42bd9f441bca9a72300f720c
2017-04-06 21:29:07 +01:00
Sam Smith 52d128863e actions: Correctly delay FETCH_COMPLETE
I496fe317 caused a regression where the FETCH_COMPLETE was delayed for a
total of 650 ms rather than 500 ms. This is evidenced by a 150 ms step
in the median Time To Preview immediately after today's (Thursday, 6th
April) MediaWiki train [0].

[0] https://grafana.wikimedia.org/dashboard/db/reading-web-page-previews?refresh=1m&orgId=1&from=1491505806387&to=1491507027263

Change-Id: Ic31656208671766f2c08cfaf55babba64455a614
2017-04-06 21:17:44 +01:00
Sam Smith 1e199b67f0 actions: Simplify delaying FETCH_COMPLETE
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
2017-03-30 17:48:58 -07:00
Sam Smith a7d353767e tests: Don't assume 1 wait call per test
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
2017-03-31 00:48:23 +00:00
Sam Smith 6042000eb1 actions: Don't mix delay into FETCH_COMPLETE
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
2017-03-30 17:48:05 -07:00
Sam Smith 3646b04876 actions: Dispatch FETCH_END
... when the gateway request ends, per I62c9cb04.

Change-Id: Ifbed65d6b97877e859e81f256fa44344a64fc64f
2017-03-30 17:47:39 -07:00
Sam Smith 8b311aa159 Hygiene: FETCH_END -> FETCH_COMPLETE
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
2017-03-30 17:47:13 -07:00
Baha 9a94300858 Log events to statsv for monitoring PagePreviews performance
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
2017-03-14 08:51:10 +00:00
joakin 82e315b124 Tests: Migrate {integration,actions}.test.js to node qunit
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
2017-02-27 18:17:28 +01:00