Commit graph

111 commits

Author SHA1 Message Date
joakin 98d9415361 Hygiene: Rename builder vars on require preview/model
preview/model is just a module/namespace object, not a builder class or
similar.

This patch changes a couple of imports to reference some of the exposed
functions of the preview/model where required.

In a shiny future, this pattern would be:

    const { createModel, createNullModel } =
      require( '../preview/model' )

Bug: T165018
Change-Id: If6ad4611538ca4f24e2443c0c3ed433275e995a6
2017-06-16 14:50:19 +02:00
joakin 010a4d91a6 Hygiene: Simplify gateways
gateway/*/rest were copies of gateway/restProvider just passing
a different provider. Docs were the same, they were untested, and
looking at them they seemed like unnecessary abstraction.

This patch removes the plain vs html structure, and separates gateways
like before, by endpoint.

There is a light utility in gateway/restFormatters.js that adapts the
call from the rest gateway to use formatters.js functions. It needs
testing, that I'll add in the next patch.

The flow for creating a gateway ends up as follows:

1. index.js calls gateway/index#createGateway( mw.config )
2. createGateway chooses based on wgPopupsGateway and invokes
  * mediawiki.js#createMediaWikiApiGateway or
  * rest.js#createRESTBaseGateway w/ restFormatters.js#parsePlainTextResponse or
  * rest.js#createRESTBaseGateway w/ restFormatters.js#parseHTMLResponse

Changes:
* Removed src/gateway/{plain,html}/rest.js
  * Extracted formatter functions to src/gateway/restFormatters.js
* src/gateway/plain/mediawiki.js -> src/gateway/mediawiki.js
         * tests/node-qunit/gateway/plain/mediawiki.test.js ->
           tests/node-qunit/gateway/mediawiki.test.js
* gateway/restProvider{,.test}.js -> gateway/rest{,.test}.js
* Change gateway/index.js#createGateway to properly call the rest
  gateways with the rest formatters

Bug: T165018
Change-Id: Ia75695dfc192aad5bc581a68882514bad6c29646
2017-06-16 14:49:59 +02:00
joakin b12599c871 Hygiene: Move createGateway to gateway/index.js
And add tests, given it is growing in complexity.

Additional changes:
* Interface ext.popups.Gateway -> Gateway in docs

Bug: T165018
Change-Id: I8a12333ad9d14d6a7fbde11afc42f607881e8ea3
2017-06-16 12:46:05 +02:00
joakin 490583bcb9 Hygiene: Capture jQuery at construction
and not from global scope all the time.

Bug: T165018
Change-Id: I90b55a65a7ca25c2998c811a98401feaeced165e
2017-06-15 20:10:08 +02:00
Sam Smith 6159af3151 i13n: Extract experiments module
... from the statsvInstrumentation module so that the bucketing logic
can be shared with other instrumentation modules.

Change-Id: I5732fa539a3911939fa85fa88c102fa8dcfa5613
2017-06-14 11:04:32 -07:00
Piotr Miazga f2fbef6ec7 Implement html/rest.js gateway which handles HTML Restbase responses
Refactor existing Restbase gateway and extract shared logic into
shared Restbase provider. Also introduced new createNullModel()
which defines an empty preview model.

Additionally improve naming in new gateways/formatter so function
names are more explicity.
 * Htmlize() became formatPlainTextExtract() as it should be used
   only with plain text extracts
 * removeEllipsis() became  removeTrailingEllipsis() as it removes
   only trailing ellipsis.
 * src/gateway/index.js defines gateways by configuration name stored
   in extension.json

Bug: T165018
Change-Id: Ibe54dddfc1080e94814d1562d41e85cb6b43bfc1
Depends-On: I4f42c61b155a37c5dd42bc40034583865abd5d7a
2017-06-13 20:19:05 +02:00
Timo Tijhof 4996886eb9 eventLogging: Use base 32 instead of 16 for fnv-encoded hash
Follows-up 79f3b318d0.

Number#toString supports up to Base 32.
Same collision behaviour but with a shorter string.
Typical length with Base 32: 7 (max: 11)
Typical length with Base 16: 9 (max: 14)

Change-Id: I91e91341cbecdec24549ace6a6300550f5b449ee
2017-06-12 21:43:05 +01:00
joakin 38b061a468 Return empty extract if string is blank after formatting
Bug: T167626
Change-Id: Icb89cbe447226850231c3d7d74e61e5f6a5db809
2017-06-12 12:59:31 +02:00
Sam Smith fb8d54c7ce actions/rest: Use DB-key version of title
This reduces the number of 301 Redirect responses when fetching previews
from RESTBase.

Bug: T167633
Change-Id: I830947ab79e72dcc023193412c8d5bcee986e23f
2017-06-12 11:22:55 +01:00
Piotr Miazga ef283c2509 Extract rendering/parsing mediawiki responses into separate class
Page Previews should be able to consume HTML response generated by
MediaWiki. First we need to move out plain text crunching from
renderer.js and model.js. Mediawiki and Restbase gateways will have
to parse/htmlize plaintext into nice HTML by themselves.

Bug: T165018
Change-Id: I5d7e9f610bb809aa9fb035a4a9f96e9e8796c9d8
2017-06-09 18:34:25 +02: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
joakin 356678ffcd Add title#isValid
Which checks if a title is an eligible one for showing previews

Bug: T165572
Change-Id: I5ade3fb84d400293d24de05e10119996e711b41e
2017-06-08 12:29:33 -07:00
Sam Smith 343506bdfb gateway: Fix Accept header sent by rest gateway
... and update the RESTBASE_PROFILE constant to the latest "stable"
profile for the endpoint.

Prior to this change the Accept header sent by the rest gateway was

  application/json; charset=utf-8profile="..."

This was discovered while responding to T166605.

Change-Id: I00f277e724c561634b26c9ab10bd35332c6dba91
2017-06-08 12:19:32 -07:00
joakin ef2f99ef65 Rename getTitle.js to title.js
In order to create a title#shouldShowPreview function next in the
module.

Bug: T165572
Change-Id: I9e59bb0f525d2698f882543ca0d4a1bde6b2d5d2
2017-06-08 12:10:14 +02:00
Sam Smith 98864a7ce3 eventLogging: Add missing properties to "tapped settings cog" event
When there's an interaction, then the "tapped settings cog" event should
have the same properties set as the other interaction-specific events.

This was discovered while QAing T164256.

Bug: T164256
Change-Id: I4749b52656203c7e0c42ae742556ee996eee322a
2017-06-07 10:13:23 +01:00
Sam Smith 66234e021e eventLogging: Add perceivedWait prop to all events
... and the previewType property as well.

Per the Popups schema [0], the "opened" action should have the
perceivedWait and previewType properties set.

Bug: T166323
Change-Id: I957d123434a6b750aff6f5279865321a08367382
2017-05-25 17:00:13 -04:00
Sam Smith 0670597434 doc: Document gateway modules
Additional changes:
* Fix the summary not showing up for the Container interface.

Bug: T158236
Change-Id: I7c5dee1b4525c2db28b89e57604d2d073620293d
2017-05-25 14:34:25 +01:00
Sam Smith e4f4041846 doc: Document statsvInstrumentation module
Bug: T158236
Change-Id: I138a1ef5305c0b49415d5f2a914ba45fd6aa869b
2017-05-24 10:45:49 +01:00
Sam Smith 445318111b doc: Document counts module
Bug: T158236
Change-Id: I27106436e693289aa0c4140cef8e3d6210bcd8fb
2017-05-23 13:26:56 +01:00
Sam Smith 5d9561bc91 doc: Document preview/model module
Changes:
* Assign exports to exports rather than reassigning module.exports so
  that JSDoc can guess inner members.
* Tidy up parameter types with JSDoc's "nullable" syntax.

Bug: T158236
Change-Id: I7261d1bb3924c9f14301490f54d7813dcffc959b
2017-05-23 13:26:56 +01:00
joakin 293d7ebe8d Clear interaction after an event for it is logged in EL
Given that interactions end up with an event logged, there shouldn't be
a reason to keep an interaction active after it's corresponding final
event has been logged. See Tbayer's state graph.

This patch removes the current interaction if an event with that token
is logged, effectively finalizing it and making it impossible for the
token to be reused from the state tree again.

Additional changes:
* Pass the logged event with the action EVENT_LOGGED so that the reducer
  can determine if it needs to do anything else.
* Since the interaction is removed, when undefined, guard against
  actions that use state.interaction freely. (Only allow BOOT,
  LINK_PREVIEW, and EVENT_LOGGED)

Bug: T161769
Bug: T163198
Change-Id: I99fd5716dc17da32929b6e8ae4aa164f9d84c387
2017-05-16 11:25:41 +02: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
joakin 7bd29bb058 Enable production settings for the production bundle
It enables certain optimizations, see:
* https://webpack.js.org/guides/production-build/

Code is minified by uglifyjs.

Aditional changes:
* Add banner so that sources are not minified by ResourceLoader

Bug: T160061
Change-Id: I50c9148ebf2d860db42a24225bc128bfcfe56927
2017-05-11 10:37:45 +02:00
Sam Smith c44fddf8cd eventLogging: Discard events with duplicate tokens
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
2017-05-10 11:43:05 +01:00
Baha d6424cb59d QA: Test renderer#show
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
2017-05-09 17:15:01 -04:00
Baha 108f205e12 QA: Test renderer#layoutPreview
Bug: T133022
Change-Id: I7b7aa544b850e5487a621c522b28653db6056633
2017-05-09 17:13:16 -04:00
Sam Smith 2d3f5333b5 eventLogging: Round perceivedWait property
The perceivedWait property, introduced in I3fd253f1, must be an
integer.

Bug: T164256
Change-Id: If19e8694e5ac111af0f27146cf4ac03bd1a6ff82
2017-05-09 19:00:41 +01:00
Sam Smith 35cfc38b0b eventLogging: Add missing perceivedWait property
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
2017-05-09 06:51:05 +01:00
Baha 6d95bbf630 Refactor and test renderer#createLayout
* Make function dependencies explicit;
* Only pass data that's needed, no more.

Bug: T133022
Change-Id: Ia973bd9636a477a76db907a37e2a0689daf4e3fa
2017-05-05 14:20:46 -04:00
Ed Sanders 81a6d49e99 build: Update eslint to 0.4.0 and make pass
Change-Id: I7d301049f0c91f79e82f996b2dce770840c27e72
2017-05-05 05:43:16 +01:00
Baha 0b169321d8 QA: Test renderer#bindBehavior
Bug: T133022
Change-Id: I3b0b199010f0460d83f80c57e4bab8e3606a4b4f
2017-05-04 15:48:10 -04:00
Sam Smith 742f341e5c Hygiene: Tidy up QUnit references
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
2017-05-04 15:53:44 +01:00
Baha d95badc614 QA: Test renderer#hide
Bug: T133022
Change-Id: I752c4266b6be1909a3265d8292e7c5229e5724fb
2017-05-03 13:26:19 -04:00
Baha eb9ceb6238 QA: Test renderer#createPreview
Bug: T133022
Change-Id: Ied8e798851d5aebf2910aba5866f8a801c20923b
2017-05-03 13:23:43 -04:00
Baha a46a5fc02f QA: Test renderer#createEmptyPreview
Bug: T133022
Change-Id: If93d2c679bd7edcf98946e1cb688bc5c3744f69b
2017-05-03 13:21:24 -04:00
Baha ccca8b7a90 QA: Test renderer#createThumbnailElement
Bug: T133022
Change-Id: I7909c208e3d69b2ee510dcd8bfbcafc0118397ca
2017-05-03 16:25:28 +00:00
Baha 7f89341cc6 QA: Test renderer#getClasses
Bug: T133022
Change-Id: If3b8888a948f6e14d8847fdc1d4ea16e42329bb8
2017-05-03 16:24:18 +00:00
Baha 872691e293 QA: Bring back renderer#renderExtract tests
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
2017-05-02 12:09:11 -04:00
Baha 234282d1a2 QA: Test renderer#createPokeyMasks
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
2017-05-01 11:00:42 -04:00
Baha 2030cb6b06 QA: Bring back renderer#getClosestYPosition tests
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
2017-05-01 10:56:41 -04:00
joakin 1da916ee73 Tests: Refactor processlinks test
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
2017-04-28 22:39:49 +01:00
Sam Smith 79f3b318d0 Track and discard duplicate enqueued events
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
2017-04-28 15:12:40 +01:00
Sam Smith 3d0899c0a0 Remove isLoggingEnabled with Null Object pattern
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
2017-04-27 14:29:04 +01:00
Sam Smith 7f91068ecf Don't show preview if user has abandoned link
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
2017-04-24 15:35:07 +01:00
Sam Smith 5c5872d31f Don't occlude link when preview is above mouse
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
2017-04-20 16:41:19 +01: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 2c171d7f25 reducers: Don't destroy interaction on LINK_CLICK
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
2017-04-18 19:43:31 +01:00
Sam Smith bb43c9c2e6 reducers: Update eventLogging documentation
Bug: T159490
Change-Id: Ica8363ffb166cf4e1583b481a7055f6d1678ccf4
2017-04-17 15:06:54 -07:00
Sam Smith 56aeeccb0d reducers: Make LINK_CLICK finalize but not close
... 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
2017-04-17 15:06:00 -07:00