I6a38a261 made the eventLogging change listener count and discard
duplicate events and count duplicate tokens.
While we isolate the issue(s) that lead to duplication (reuse, likely)
of tokens, make the eventLogging change listener discard events with
duplicate tokens as well.
Bug: T161769
Bug: T163198
Change-Id: I0dbb16c37814d39d7aec35c8fb7cc7309704c550
Binding the behavior has been left out as it requires some refactoring.
Ideally, that functionality should be tested via integration tests, which
is already done.
Bug: T133022
Change-Id: If2fd472847eb3557de97c7ec9619e8831e9bda6d
Per the Popups schema [0], the "dismissed" action expects the
perceivedWait property to be set.
Happily, the time until the preview was shown is already recorded and
used to determine whether or not to create a "dismissed" or
"dwelledButAbandoned" event.
Reducer changes:
* When creating the "dismissed" event, set the perceivedWait property to
the already accumulated timeToPreviewShow.
[0] https://meta.wikimedia.org/wiki/Schema:Popups
Bug: T164256
Change-Id: I3fd253f1c2ff5fa8e24c9225060d728ffd8dfd27
Since Ieea378c9 all QUnit tests are run in Node.js and not in the
browser. Tidy up references to QUnit inside of the codebase and tooling.
Changes:
* Don't exclude src/processLinks.js in Istanbul code coverage reports.
* Don't test for window.QUnit in createSchema. We no longer run the risk
of sending beacons while running the QUnit test suite as it's no
longer run in the browser.
Bug: T160406
Change-Id: Ifb6adb84b8019dd69231b50af00bf978b708fc60
The tests were removed in Iae0a78d0b8a13353de70794b67387f2c3bab44c6.
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Some of the tests have been removed as the functionality covered by
those were moved to model.js in I20f29657fcf94101a71ed13c0920508db71292ce.
Bug: T133022
Change-Id: I7b20324dd5fe8a428cdd96959b65bc82d44fb515
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Bug: T133022
Change-Id: I3ecbfb9bb3ac9c63fdd40df502796748c62949fe
The tests were removed in Iae0a78d0b8a13353de70794b67387f2c3bab44c6.
The ultimate goal is to refactor the renderer code and make it
testable, but before doing so we need to add tests to cover the existing
code. This will give us confidence that we won’t accidentally break
anything when we refactor.
Bug: T133022
Change-Id: I897276a1a953f6be62e4c2d4a24e0f22fc6ef141
With the final goal to remove the real mw stubs, move the processLinks
tests away from test cases, and split getTitle to be tested standalone.
Supporting changes:
* Move getTitle out to it's own module
* Will be tested separately in a followup commit
* Remove global declaration of mw.popups.processLinks (not needed any more)
Bug: T160406
Change-Id: Ieebd1257a2476081c67a318d3f05dffa1d3b9bdd
The eventLogging change listener is responsible for ensuring that the
internal state of Page Previews matches its external state (that
perceived by the user and UA). It does this by logging events with
ext.eventLog.Schema#log. This makes it the perfect place to track and
discard duplicate events enqueued by the Page Previews codebase observed
in T161769.
Make the change listener track events that it's logged by storing hashes
of the dynamic parts of them in memory. If the eventLogging change
listener sees the same event more than once, then it discards it and
increments a counter in StatsD.
This behaviour should be enabled for a matter of days as we should see
whether the duplicate events are being enqueued by the Page Previews
codebase immediately.
Bug: T163198
Change-Id: I6a38a2619d777a76dd45eb7300079e1f07b07b12
The statsv change listener depended on both the analytics tracking
function and whether it should log metrics to StatsD. We can simplify
the behaviour of the change listener by passing in a function which
doesn't log metrics to StatsD if such logging is disabled.
The change listener is now more isolated from other components.
Moreover, sharing the analytics tracking function with other components
is simpler as there's no repeated code.
Bug: T163198
Change-Id: Ibf4785fa4c27c1ad4739f02410f57412f56ff481
If the user dwells on a link for long enough, then the gateway makes a
request, which is allowed to complete regardless of whether or not the
response is required.
If the user abandons the link but the request completes before the
abandon completes - currently, ABANDON_END_DELAY is 300 ms - then the
preview will be rendered temporarily.
Fix this by not rendering the preview if the user has started to abandon
the link.
Bug: T163350
Change-Id: I154dde4e3ccaed3d11cb023c85c44451fc0ad957
When the user dwells on a link and there's enough room to display a
preview above it, then the preview should rest atop the link rather than
above the mouse.
Bug: T161366
Change-Id: Ia7266f6e5c272817581bdbcb3710429b266556e4
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
I09d8776 introduced a bug in the eventLogging reducer where reducing
LINK_CLICK would remove any accumulated interaction state but not close
the interaction.
Update the associated tests so that they correctly test the reduction of
this action.
Bug: T162924
Change-Id: Ia03e719c228ee96f279c1fa89252dc6b6371a8e9
... the interaction.
Following on from Iccba3c4c, LINK_CLICK shouldn't be considered the end
of an interaction because the link or preview can still be abandoned by
the user. Unlike ABANDON_END and LINK_DWELL, therefore, LINK_CLICK
musn't destroy the interaction but indicate that no more events should
be enqueued for the interaction.
More concretely, if the user has clicked the link or the preview, then
a "dwelledButAbandoned" or "dismissed" event shouldn't be logged.
Changes:
* Distinguish between "finalizing" and "closing" an interaction, where
the latter is the current behavior of ABANDON_END, LINK_DWELL, and
LINK_CLICK, in the eventLogging reducer and associated tests.
* If the interaction is finalized, then either the "dwelledButAbandoned"
or "dismissed" events shouldn't be logged.
Bug: T162924
Change-Id: I09d8776da992053f89a77508e29a7cde3cfeeac6
As noted in T160406, the only QUnit tests that requires a running
MediaWiki instance is the test for mw.popups.processLinks. The function
itself is isolated from the rest of the codebase.
Now, as noted in T162876#3182198, during boot the
ext.eventLogging.Schema module is loaded asynchronously with
mw.loader.using. Since boot is unconditional and happens ASAP this
happens when the tests are loaded and run.
In the short term this can be avoided by not making the tests depend on
the entire codebase. The long term solution is laid out in T160406.
Supporting changes:
* Bundle assets with webpack@2.4.1.
Bug: T162876
Change-Id: If1ee1853ba7a9b2a66b24bb93b4e6062b92b0dba
... in the eventLogging reducer.
Like ABANDON_END, the LINK_CLICK should be considered the end of the
interaction in the context of the EventLogging instrumentation, i.e. no
events should be logged after the user clicks the preview or the link.
See T159490#3172692 for additional context.
Bug: T159490
Change-Id: Iccba3c4c2b6121016ff7923c11b1622bc046ad6b
Like the FETCH_COMPLETE and ABANDON_END actions, the PREVIEW_SHOW action
was delayed but not conditionally reduced. As ABANDON_END is delayed,
there's a potential for a race and if ABANDON_END is reduced before
PREVIEW_SHOW, then there's no interaction to reduce the action into,
which causes an error, e.g. T159490#3165276 and T162373. Making
PREVIEW_SHOW require a token stops the error occurring in this scenario.
An alternative would be to clear the timeout created in
ext.popups.Preview#show in #hide. However, this would be inconsistent
with actions#fetch and actions#abandon.
Bug: T159490
Change-Id: Ibd2c0c6f45e4392582cc6ed08517f6ca1146d57a
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
... instead of using the active element.
In the case of the eventLogging reducer, this fixes scenario 4.1 from
T159490#3150331, which was caused by late FETCH_COMPLETE actions being
reduced regardless of whether interaction had been finalised or a new
interaction had started.
Bug: T159490
Change-Id: If9d718625b0302ea2f75a778005643b4eef62bde
Treat these responses not as an API failure. Show a generic
preview whenever the server responds with a 404.
Bug: T160744
Change-Id: Id6169d9d4c7493f5b6511cc78fe65d448cdadc03
I6d9ff52b introduced a regression where if a logged out user clicks the
settings cog then an error is thrown.
For now, passing the event to the behavior for further processing is
required. However, it's clear that this makes the
ext.Popups.PreviewBehavior abstraction leaky.
Bug: T162324
Change-Id: I9dea04eb7435f9349e60d477f5701ec5dd655ebd
We have to be careful about the namespaces here, and then we don't
need the awful `.html( .html() )` hack. (I honestly have no idea
why that even worked for some browsers, it really shouldn't have.
The comment next to it is wrong.)
* Construct the 'svg:svg' element with the right namespace
* Set 'xlink:href' attribute on 'svg:image' element with the right namespace
Doing this correctly makes the thumbnails work in Opera 12, and it also
works as before in (at least) Chromium 57, Firefox 53, IE 11 and Edge.
I can't find out what version of Safari the other hack here was
supposed to apply to, but the code was wrong in both cases, and the
hack was mistakenly also applied to modern Chromium.
Useful resources for dealing with SVG embedded in HTML while scripting:
* http://stackoverflow.com/questions/6701705/programmatically-creating-an-svg-image-element-with-javascript
* http://jsfiddle.net/UVFBj/8/
* https://www.w3.org/Graphics/SVG/WG/wiki/Href#xlink:href
Bug: T161799
Change-Id: I30b2a1291811296424018e013bd07055ae7551d7
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
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
If the user has disabled PP via the settings dialog or they aren't in
the experimental condition, then link titles shouldn't be emptied.
Because this behavior has to respond to the user enabling/disabling PP
within the same page session, change the linkTitle change listener
rather than conditionally registering it.
Bug: T161277
Change-Id: I53c1a1d3e4436e2ffe08da27da388f394f4e8817
Binding the behavior to the preview before it's shown means that the
application will respond to user interactions with the preview even
though it's transparent.
This fixes scenario 4 from T159490.
Bug: T159490
Change-Id: Ia2d06869868d07af60bdeb49d46612a4a0dc02e9
If the user abandons link A (or preview A) and immediately dwells on
link B, then log a "dismissed" or "dwelledButAbandoned" event.
In this context, "immediately" means before the ABANDON_END action is
dispatched, which, currently, is 300 ms after the ABANDON_START action
is dispatched.
Bug: T159490
Change-Id: I49f0f5dfb3e6c08844f1794fee8cb6170e93981b
Reducer changes:
* Add tests for ABANDON_END case.
* Extract the body of the ABANDON_END case into the createAbandonEvent
helper function.
Additional changes:
* totalInteractionProperty -> totalInteractionTime elsewhere in the same
file.
Bug: T159490
Change-Id: Ifff34271395f330b83cfe487e84800fe2d6f3811
Reducer changes:
* Make the eventLogging reducer queue a "tapped settings cog" event when
reducing the SETTINGS_SHOW action.
This was discovered while testing I6ce7d72b.
Bug: T159490
Change-Id: I6ce7d72b364d20c71b0e2cfed98e99f7997895e5
Like dwelling and abandoning, clicking on a preview is the same as
clicking on a link.
This fixes scenario 3 from T159490.
Bug: T159490
Change-Id: I6d9ff52b62bec93ebfcc9b6d267a46cf961852fb
For now, mirror the interaction modelling in the preview reducer in the
eventLogging reducer to handle the user either:
* Repeatedly dwelling on and abandoning a link.
* (Repeatedly) moving their mouse between the link and the preview.
This fixes scenarios 1, 2, 5, and the general issue from T159490.
Bug: T159490
Change-Id: Ia771f325e541c107348b16b47c5b786c97847652
Step 1 of T161284. Given that the median API response time (as measured
by the client) is ~115 ms [0] and the API response is artificially
delayed so that the preview starts fading in at 500 ms, we can increase
the API request delay to 150 ms without affecting the current UX while
decreasing the number of incidental HTTP requests triggered by the user
glancing their mouse over a link to another page.
[0] https://grafana.wikimedia.org/dashboard/db/reading-web-page-previews
Bug: T161284
Change-Id: I4c4a766467cdb4cd47c4231c1106c35bab67855e
When EventLogging is unavailable do not initialise the EL-related code
or try to send any events.
When EL is enabled for a brand new user we request an additional module
during boot causing an additional HTTP request. Page Previews continues
to boot normally regardless of whether the request fails.
This approach doesn't impact boot or first paint time. Once the module
is loaded once it should be cached locally, subject to the
ResourceLoader's policy. Moreover, the RL will not attempt to load the
module twice so this doesn't impact the performance of other modules.
Bug: T158999
Change-Id: I7ed7f00d52279151ece23e5aced4f2adb0f7fdc3
Changes:
- remove focus events listeners as they are triggered after switching tabs
- show PagePreview on keyup event
Bug: T158631
Change-Id: I7533f896604e0e0a8ea6e900ae4f7d12b6458836
IE doesn't appear to update/redraw the SVG image element when Setting
its clip-path attribute to '', not removing it. This is problematic as
the attribute is always set to a default value (in the createThumbnail
function) before the preview is laid out.
Bug: T160237
Change-Id: I4559ff5018b8f4ecf06f6f5d9462a999d9726b94
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
... thumbnails.
A good example of the difference in behaviour of the PageImages API is
here <T156800#3087507>. The API defers to File#transform, which scales
the largest dimension of an image, not always the width, e.g. if an
image is 1000px x 2000px and the request is for a thumbnail "of 1800px",
then the thumbnail will be 900px x 1800px.
Bug: T156800
Change-Id: I64bc2244ee78a594298d8637233b0da1083700eb
Keep all configuration-like values in one file.
Changes:
- moved EXTRACT_LENGTH to constants.js file
Change-Id: Ibe5ecfc60f2c09a30a9ecb3586bc5fb6a7365476
Webpack 1.14.0 is an old version, switch to using latest stable which
has better documentation, tree shaking, ES2015 modules and a core team
of contributors with funding. See https://webpack.js.org/
Additional changes:
* Recompile the frontend assets
Change-Id: I2c5940276e99dee104d04c6a0b83d8ab36a99df5
If the image isn't an SVG then it shouldn't be scaled past its original
dimensions. Handle the case where the requested thumbnail can't be
generated on the server as the original is too small ( < 320px,
currently [0]) in the same way.
Moreover, if the image happens to have the exact dimensions that we're
requesting (300px or 600px wide, currently [1]), then use the original
image to avoid unnecessary work/pressure on caches.
Supporting changes:
* Update the SVG_RESTBASE_RESPONSE fixture to use the extension returned
by RESTBase (and the PageImages extension) for the thumbnail:
.svg.png.
[0] https://github.com/wikimedia/restbase/blob/master/v1/summary.yaml#L121
[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/constants.js#L2
Bug: T156800
Change-Id: I5d0aa161e80869e4b4f5425d906d1e753047a3a3
Instead of importing the modules from sources (which you should do if
you properly define NODE_ENV and use uglifyjs from webpack) import the
already compiled files on the npm packages.
This results in 5kb less on the compiled bundle.
Change-Id: I83732ea79a59b611c117ddcf4c262948c795f642
Now that most unit tests are run in node with common.js for loading
sources there is no reason to keep global variables around exposing all
the sources.
Only exception is the only qunit integration test processLinks.test.js
which still consumes mw.popups.processLinks, which is the only global
variable remaining in the codebase.
Changes:
* Remove references to mw.popups in code comments and reference the JS
file instead
* Remove popups.js which exposes all common.js modules as global
variables
* Export mw.popups.processLinks in processLinks.js for testing in
processLinks.test.js
Change-Id: I91066654b9282f73a80eb1ba5018bd091656c61d
We used to query the MediaWiki API to only return non-free images.
This patch allows us to query the API for images with any license.
The RESTBase end point is already returning images with any license.
Bug: T158632
Change-Id: I9ac60b6f74a7f7eb2cb160ee522c2c3a26dd0858
All the other masks use an 8px offset for the triangle. But this
specific mask used 250 - 243 = 7px offset.
Bug: T153840
Change-Id: Ib72842d18bd844ff37509cf5bf1dedd4e0f99dbc
Rather than manipulating the URL of the original image to get the URL of
the appropriately sized thumbnail, manipulate the URL of the thumbnail
image.
While we could manipulate either the thumbnail or original image URL,
there are subtle differences between the two, so manipulating the latter
makes the generateThumbnailData function as simple as possible, e.g. we
don't have to splice in "/thumb" after "/commons".
Also, ensure that the thumbnail's dimensions are scaled appropriately.
Bug: T156800
Change-Id: I6825bad14b1131dc81f23dcf120cf8ffb7d7b4f6
* Logged in users bypass bucketing. They keep working as before.
* When Page Previews is configured as a beta feature, logged out users
won't see the feature.
* If an anonymous user has enabled/disabled the feature using
the settings cog then they will see or not see the feature
depending on the value of their setting.
* The other anonymous users are bucketed. By default 90% of these
users see the feature, the other 10% don't. These numbers can be
controlled by the config variable `wgPopupsAnonsEnabledSamplingRate`.
Bug: T157700
Change-Id: I5307587b10f4849c4e82d3b064ff759121c2de67
mw.storage#get doesn't take a default value to return if the underlying
storage is disabled or the key is missing. In the former case it'll
return false and in the latter it'll return null, i.e. in neither case
will it return undefined.
Bug: T157700
Change-Id: I3f653c11468e17b64765e85ebb3b8f03e311352a
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
The "checkin" part of the Popups schema was superseded by the
ReadingDepth schema, the implementation of which is tracked by T155639.
As well as removing all checkin-related code, update the Popups schema
to the latest version - the version that doesn't have the checkin
property.
Bug: T155639
Depends-On: I762ec3fc91decf3cffa869dbd783faf62f01329a
Change-Id: If764917b6e121e1f9db980a4efa30c0f7a166197
The render change listener is hard coupled to the renderer file, so in
order to migrate the test, instead of stubbing a global variable, we had
to either inject the renderer into the change listener factory as done
everywhere else, or mock the require call.
In order to do one thing per commit, we're mocking the require call
here to get the migration done, but added a FIXME to use dependency
injection instead in a future change.
Change-Id: I50f82cdc9664d34b8a8ccc1ff368f7209404159d
In index.js. Instead of using the global variable/object popups, require
things from their files so that we can remove the global variables when
we can run qunit tests with commonjs in node.
Change-Id: I85408f01eca27f97cf46b2076176fcc16c037829
This change resizes thumbnails to the appropriate width
based on the value of mw.popups.gateway.THUMBNAIL_SIZE
Tests cover
* When requested thumbnail is < than original size
* When requested thumbnail is > than original size
* When requested thumbnail is an svg and originalimage
smaller than requested thumb size
Bug: T156800
Change-Id: Ib375b97e2bc959e91de5177efc3df1f2ded54a5b
This way, src contains sources, and dist contains distribution files.
Also, add some documentation about the folders in the README and an adr.
Change-Id: Ie0b9f6475b8423b90e927633d883bde3cd5d5e4d
In order to automatically verify in CI that the built assets are up to
date with the commited sources, we need to keep the built assets in
a folder separate from the RL assets.
* Rename the compiled assets folder to industry standard `dist`
Change-Id: I8c5898f9bb29fee7164a7038b835a5f7efd33dbc
