Commit graph

394 commits

Author SHA1 Message Date
jdlrobson 11d631a975 Delete unimplemented browser tests
These browser tests were added in I55f311b6b8845e6ebf4cc5698758afd1f9042a45
but broken in Ibc0d07e8394aeb640f9efaabfe10be5317bf4b8c

Let's remove these while porting over to the Node.js version
of these browser tests since they are not run anywhere.

(Try bundle exec cucumber tests/browser/features/ --dry-run)

Bug: T177262
Change-Id: Ica745d084acbb56ec66d88a1785e7557cb12ad6e
2017-10-03 18:17:04 +00:00
smarita 3ec185cb01 Consider using more common image sizes for Page previews
Modified necessary files to increase size of popup from 300px to 320px

Bug: T173434
Change-Id: I47bbe9defe961008163551d5be4fc7b1ca08d0d1
2017-10-01 22:02:29 +05:30
Bartosz Dziewoński 2b637e42ef Improve how we render help messages in preferences
* The 'help' parameter expects a string, not a Message object. It
  seemed to be working, but caused fatals when switching the
  preferences form to the OOUI implementation (T117781).
* Use 'help-message' instead, which also avoids using the global
  request context for Message objects.

Bug: T117781
Change-Id: I5b2e44df35a2696da0e7ba8394eccf41914f6dda
2017-09-12 17:07:37 +00:00
jdlrobson f18c8e4eaf Run settings browser tests and make sure jQuery 3 not enabled
Change-Id: I599c8c002c570d4911b7c86294f714279833e80b
2017-09-01 10:16:17 -04:00
Piotr Miazga 0de054cd79 Use canonical name for NS_SPECIAL titles when checking the blacklist
Changes
 - when verifying title use canonical names for pages in
 special namespace
 - improve unit tests to verify canonical names and translated titles

Bug: T170169
Change-Id: I49592133eb8286eacf04fd3034df091f7ef2aa50
2017-08-25 15:56:02 +02: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
jenkins-bot 8bb322b15d Merge "Hygiene: Use promises A/A+ everywhere" 2017-08-22 14:34:26 +00:00
joakin 4d8364a999 Hygiene: Use promises A/A+ everywhere
Remove usages of deprecated methods like .done which make jquery
promises fall back to non-standard behavior

Additional changes:
* Rename var promise to a more descriptive name in tests

Bug: T173819
Change-Id: I7b041d0a7a8c42a8eac947295d265e898085c60a
2017-08-22 13:36:00 +02:00
jdlrobson e4e9bb3bd6 Popups A/B test infrastructure
Introduce PopupsAnonsExperimentalGroupSize config variable. This defines
a population size who will be subject to experimentation. If the group
size is undefined or 0 (default) and PopupsBetaFeature is false
(default value) Popups will be enabled for everyone. If it is any other
value, half that group will see page previews.

Drop the config variable PopupsSchemaSamplingRate - we will now only
EventLog when an experiment is occuring. This means we can simplify the
MWEventLogger class as shouldLog will always be truthy. Given server
side eventlogging is only used for preference changes
traffic should be low and not need sampling.

Introduce getUserBucket which determines whether a user is in a bucket
on, off or control based on the value of
PopupsAnonsExperimentalGroupSize. Add tests showing how these
buckets are calculated.

Caution:
A kill switch wgPopupsEventLogging is provided for safety.
It defaults to false. Before merging, please check if any config changes
are necessary.

Bug: T171853
Change-Id: If2a0c5fceae78262c44cb522af38a925cc5919d3
2017-08-17 21:07:07 +00:00
Piotr Miazga 910dd0408f if preview count stored in localStorage is not a number override it
There are some cases when the preview count stored in local storage
evaluates to NaN. When this happens we should override the value
to zero, store it in localStorage, and return it.

Bug: T168371
Change-Id: Ic44b7c7b5b716f6a0859f33278d56d2d95bbfb3e
2017-08-17 18:29:56 +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
Baha 3ac3769aa7 Remove event logging duplication detection and logging
We haven't seen the PP EventLogging instrumentation produce duplicate events
for weeks.

Bug: T172106
Change-Id: I6f3d7c0cdbf23161f63259e4d20d8a710376468b
2017-08-16 11:46:35 -04:00
Piotr Miazga d07441ec7f getPreviewCountBucket should return unknown when no bucket is found
Under some unknown circumstances getPreviewCountBucket() is called
with a value that is not a -1 or a natural number. When that happens
function returns 'undefined bucket' which causes eventLogging to
fail. I wasn't able to reproduce the issue, it might be specific
to browser/os. The safest way is to return 'unknown' for any other
case.

Bug: T168371
Change-Id: I374bb629762a86ac06a18e775d3c1a14682c9f55
2017-08-15 16:12:26 +02:00
joakin f37b76f8b4 Hygiene: make integrations/mwpopups pure
Instead of registering global variables in a function, make it pure
return the external interface and set it to mw.popups in the
src/index.js entry point.

Explicitly comment on index.js what is being set and why.

Bug: T171287
Change-Id: I94d467bfa7fa6e56033dd254518ad50b5dde5bfc
2017-08-09 16:07:08 +02:00
Piotr Miazga 8510b4b942 Allow 3rd party to check Popups enabled state by accessing mw.popups object
Changes:
 - introduced js module defining mw.popups object
 - introduced isEnabled() method which checks the redux store to retrieve
 isEnabled status

Bug: T171287
Change-Id: I523369831e2aa8a915ed1cb001b35d13b770f9da
2017-08-08 15:38:01 +02: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 02507fb74d Hygiene: Move settingsDialog UI code to ui/
Bug: T171951
Change-Id: I58f77737456e1f4b9db6631f83e4b0f14212c939
2017-07-31 19:14:18 +00:00
joakin 31fa60d32c Hygiene: Move ui renderer.js to ui/ folder
Seems appropiate to group the UI portions of the source under a ui
folder.

Bug: T171951
Change-Id: I6d4317abea4e2a8e273e13fc40a7445bb54628ef
2017-07-31 19:11:41 +00:00
Baha 29770a3ff7 Disable Previews on blacklisted pages
Introduce a config variable `PopupsPageBlacklist`. Previews code won't
be shipped to pages listed in the config variable.

Bug: T170169
Change-Id: Ia8342b55c682f444ba79e959dcc1180527a31374
2017-07-31 09:10:30 -04:00
Sam Smith ba3c0b7f76 Hygiene: i13n: Return false over not sampling
Previously, if the browser didn't support the Beacon API, then
instrumentation/eventLogging#isEnabled would bucket the user with a
sampling rate of 0, which is equivalent to returning false. This change
simply does the latter.

Additional changes:
* Update the documented module names of the instrumentation/eventLogging
  and statsv modules.

Bug: T168847
Change-Id: I7ae5c10da42ca614b5b1a6619f9555e5665344cf
2017-07-26 17:06:58 +00:00
Piotr Miazga 3d0c5b1cc3 Hygiene: Dependency Injection for Popups
Changes:
 - removed ugly PopupsContext::getInstance
 - removed ugly PopupsContextTestWrapper helper
 - defined all services inside ServiceWirings
 - fixed unit tests to test classes directly or use MediawikiServices

Change-Id: Ie27e28bb07aebe01014848c290369b1b1f098e9b
2017-07-24 22:41:28 +00:00
Sam Smith 8e00ecc5d1 i13n: popupEnabled = false for disabled event
... for consistency with the server-sent disabled event introduced in
I63faecb0.

Bug: T167365
Change-Id: I3a96df5279f6f0f4e573765735ab0e1fc6f406a8
2017-07-24 17:31:59 +00:00
jenkins-bot 22d9ef3e24 Merge "Re-enable MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment sniff" 2017-07-13 17:42:21 +00:00
Piotr Miazga b8e7a79a20 Re-enable MediaWiki.Commenting.FunctionComment.MissingParamComment sniff
Bug: T168384
Change-Id: I8b29f0ec6804e24ea4d61fb1bf1a528dddaf8fb8
2017-07-13 11:33:04 +01:00
Piotr Miazga 18b6675b4a Re-enable MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment sniff
Bug: T168384
Change-Id: Id64269f5950d6da5fee3f18825ce2d713d0446b0
2017-07-12 22:44:12 +02:00
jenkins-bot 311466cad0 Merge "Send disabled event when user disables Page Preview" 2017-07-12 14:16:36 +00:00
Piotr Miazga 8bba8c1417 Send disabled event when user disables Page Preview
Changes:
 - introduced new UserPreferencesChangeHandler class that listens to
 PreferencesFormPreSave hook
 - introduced wrapper for EventLogging extension plus NullLogger when
 EventLogging extension is not availalbe
 - when user changes PagePreview to disabled system will trigger
 disabled event

Bug: T167365
Change-Id: I63faecb0495eb30a9fc2763e7ddf3944baf7f55a
2017-07-12 15:59:42 +02:00
Piotr Miazga 426356e822 Remove duplicate events filtering
We had instrumentation for over 4 weeks and duplicate events rate
was very low. We want to keep stats so we check the duplicate events
rate but there is no need to filter those.

Bug: T167365
Change-Id: I72585beb21e9db589e45eeace657ef25f432abc9
2017-07-06 17:38:42 +02:00
Piotr Miazga 7e2c79ae0d Send disabled event from settings windows
Changes:
 - introduced new event 'disabled', sent from settings popup
 - added unit tests for 'disabled' event handling

Bug: T167365
Change-Id: I048b38122b8843199c86fd1ed9ec2ff21767e114
2017-07-04 19:08:37 +02:00
Piotr Miazga a82e54bf2d Override eventLogging to enabled when debug flag is on
In order to debug the EventLogging instrumentation in production
environments, we want to be able to bucket ourselves at will.
When the debug flag (?debug=true) is passed send all events
for given page view.

Bug: T168847
Change-Id: Id1b13b0ecaa791b4f26be4d1151bdbbe5270b64d
2017-06-30 18:52:51 +00:00
Piotr Miazga 450e6bc34c Allow events without linkInteractionToken to be logged
Changes:
 - when event doesn't have linkInteractionToken do not check for
   duplicated tokens
 - hygiene, move event duplication logic into separate functions
   for better readability

Bug: T168449
Change-Id: I3ae197567ec9f67e104af109d4f1a1c1a6769d32
2017-06-30 18:03:40 +02:00
jenkins-bot 86075fbaf2 Merge "Hygiene: Group instrumentation modules" 2017-06-20 11:06:10 +00:00
jenkins-bot 34dcf587ac Merge "build: Updating mediawiki/mediawiki-codesniffer to 0.9.0" 2017-06-20 11:05:14 +00:00
Sam Smith dcf8532cdf Hygiene: Group instrumentation modules
Following on from I4f653bba, since the schema and statsvInstrumentation
modules are similar, let's group/rename them:

  schema -> instrumentation/eventLogging
  statsvInstrumentation -> instrumentation/statsv

Change-Id: Ic59e0da7d4917f6733fd090f15d3c269af863f05
2017-06-20 11:41:37 +01:00
Kunal Mehta ba9e3c68f6 build: Updating mediawiki/mediawiki-codesniffer to 0.9.0
The following sniffs are failing and were disabled:
* MediaWiki.Commenting.FunctionComment.MissingParamComment
* MediaWiki.Commenting.FunctionComment.MissingParamTag
* MediaWiki.Commenting.FunctionComment.MissingReturn
* MediaWiki.Commenting.FunctionComment.ParamNameNoMatch
* MediaWiki.FunctionComment.Missing.Public
* MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment

Change-Id: Id9e98b7a4e87d00c63e7b509506c23f12cf0d380
2017-06-20 00:19:34 -07:00
Sam Smith 67eb3b1dcf i13n: Log EL events with mw.track
Currently, the mw.eventLog.Schema class samples per pageview. However,
we expect that if a user is bucketed for a session, then all
EventLogging events logged during that session are in the sample.

Moreover, loading the class in the way that we did - asynchronously,
using mw.loader#using - introduced an issue where the eventLogging
change listener would subscribe in the next tick of the JavaScript VM's
event loop and miss the "pageLoaded" event being queued (see T167273).

Changes:
* Make the schema module follow the form of the statsvInstrumentation
  module, i.e. make it expose the #isEnabled method, and add the
  associated getEventLoggingTracker function.
* Update the eventLogging change listener accept the tracker returned by
  getEventLoggingTracker.
* Update/fix related JSDoc documentation.

Bug: T167236
Bug: T167273
Change-Id: I4f653bbaf1bbc2c2f70327e338080e17cd3443d4
2017-06-17 00:51:32 +00: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 8f408e2937 Fix the npm script test:node
npm scripts are run under sh. globstar support is only available in bash
> 4 with globstar enabled, so **/*.js was actually being resolved to
*/*.js only running tests 2 directories deep.

This made the test mediawiki.test.js not run since it was moved to the
3rd level deep folder (and actually broke), resulting in:

  208 tests
  403 passed

With this change, it fixes getting the test files, and fixes the
mediawiki.test.js, resulting in:

  215 tests
  413 passed

instead.

This makes it work everywhere and as nested as it will get for now. Will
consider adding glob support to mw-node-qunit later for easier use in
other projects too.

Bug: T165018
Change-Id: Id0164b2673c8afe8a24fd0eb4aa255c134253862
2017-06-16 14:49:33 +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
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
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 b16a6fe735 Remove unused files processLinks{,.test}.js
Bug: T165572
Change-Id: I3139167e7caec7b7b2707c648e777e448b7de426
2017-06-08 19:37:37 +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
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 4728c63342 Add service container
Change-Id: I554fa160e1848a0398e32c796578138e4cc506ec
2017-05-23 06:25:44 +01:00
Piotr Miazga 49c4cb4d42 Introduce PHPCS check in CI
Introduced PHPCS check in CI - using same configuration as in
MobileFrontend. Additionally fixed wrong code style.

Change-Id: I0c879553d355c2a277fcc4349a93e85c65eb2291
2017-05-16 19:59:29 +02: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
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 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
Baha da2e8c82b0 QA: Improve renderer#createThumbnail tests
* Test for cases when a thumbnail won't be generated;
* Test thumbnail widths and heights;
* Test thumbnail coordinates.

Bug: T133022
Change-Id: I0f7d94816a2faab488b84f55b4462c8732d1031d
2017-05-05 10:43:59 -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 76d323da60 QA: Test renderer#createThumbnail
Bug: T133022
Change-Id: Ia6f4de8cbb7ba3b389fb326007db0f859ae1d80a
2017-05-03 13:24:54 -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
Bmansurov 520c57e30d Merge "QA: Test renderer#getClasses" 2017-05-03 16:24:28 +00:00
Baha 7f89341cc6 QA: Test renderer#getClasses
Bug: T133022
Change-Id: If3b8888a948f6e14d8847fdc1d4ea16e42329bb8
2017-05-03 16:24:18 +00:00
jenkins-bot a656d7278b Merge "QA: Bring back renderer#renderExtract tests" 2017-05-03 16:22:45 +00:00
Ed Sanders 2a4a235dec Fix ,->; typos
Change-Id: I9b490fcb35459bc0318506e049fbd573bf17e050
2017-05-03 10:19:19 +01: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 406d41d9ae Hygiene: Tests: Remove unused stubs
Bug: T160406
Change-Id: Ifa58ebfd53c037ac483f4b480efc581f18ecb936
2017-04-28 22:39:49 +01:00
joakin f2039605d5 Tests: Unit test getTitle
Real stubs on stubs.js to be removed in followup

Bug: T160406
Change-Id: I0b56e70f3ff95fc5c0f6f6dc4cf0a6625f1e5fe4
2017-04-28 22:39:49 +01: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
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
joakin c9d325d01e Tests: Migrate processLinks.test.js to node-qunit
Tests are basically unchanged, except for some stubs on beforeEach.

Supporting changes:
* Bring stubs from the mediawiki library for mw.Uri,
  mw.Title.newFromText and mw.RegExp into stubs.js
* Remove hook onResourceLoaderTestModules given there are no resource
  loader test modules after migrating processLinks.test.js

Why bring stubs from real source? This is not optimal. It could be the
case that the stubs would need to be updated at some point in the
future. That's why in the comment of each stub, it is specified where it
came from, and what was changed to make it work. It is not optimal but
it should help with a future update if necessary.

Also checked the history of the stubs and these three stubs are very
stable with a small commits per year, usually adding some extra
functionality (not breaking changes) (the rest of the commits are
docs/format stuff), so the core behavior that we rely on here shouldn't
change in a fundamental way. See the github links:

* https://github.com/wikimedia/mediawiki/commits/master/resources/src/mediawiki/mediawiki.Uri.js
* https://github.com/wikimedia/mediawiki/commits/master/resources/src/mediawiki/mediawiki.Title.js
* https://github.com/wikimedia/mediawiki/commits/master/resources/src/mediawiki/mediawiki.RegExp.js

Right now this stubs allow us to bring the test to run in isolation in
node.

The initial plan was to do change the test to be less test-case oriented
with dependencies on mediawiki.*.js and not to bring fake "real" stubs,
but after looking into it, given that:
1. the test cases in the test seem pretty informative showing the kind
   of links that popups accepts
2. the stubs are acceptably easy to bring in, and are pretty stable
I decided to go with this approach initially to finish the migration
without changing the meaning of the tests.

If we want to remove the stubs and morph the test to verify stub calls
and move the test cases to documentation on the source, I'll tackle that
on a future commit.

Bug: T160406
Change-Id: Ieea378c9b7fec9116222b4a099c226d1f1131f65
2017-04-26 12:26:43 +02:00
joakin 3a39dfd07d Hygiene: Clear global stub after the test
Bug: T160406
Change-Id: I3d8b644a3be12841ba31402fa12dd71e100dbed4
2017-04-25 14:13:29 +02: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 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 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
Piotr Miazga 9590284c70 Sanitize gadget name
MediaWikiGadgetsDefinition does some basic gadget name sanitization
and we have to do the same when checking "is gadget enabled for user"

Changes:
 - sanitize gadget name same way as
   MediaWikiGadgetsDefinitionRepo::newFromDefinition() does.
 - add try{} catch() when loading gadget as getGadget might throw an
   exception

Bug: T160081
Change-Id: Ia7a57e9dcfa3b25129d6d2bf75795372fad2b251
2017-04-17 17:29:32 +00:00
Sam Smith 83e32c255d reducers: Make LINK_CLICK finalize interaction
... 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
2017-04-12 11:45:04 +01:00
Sam Smith 76b8c18ca5 reducers: Make PREVIEW_SHOW require a token
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
2017-04-11 14:06:04 +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 91b1d5da42 Hygiene: Reduce nesting of test cases
The ABANDON_START action test cases were (accidentally?) nested under
the FETCH_COMPLETE action test cases...

Bug: T162373
Change-Id: Ia7866178162512602dedd7f70d62c17995ee5076
2017-04-07 14:11:52 +01:00
Sam Smith 87be4be855 reducers: Reduce FETCH_COMPLETE if token matches
... 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
2017-04-07 14:06:14 +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
Baha 5b613b1698 Disable Previews when Navigation Popups Gadget is enabled
Only showing a preview is disabled, EL and other stuff should work.

Bug: T160081
Change-Id: Ia837816081781dcaea9a8b4c722f8df5b3e3ca03
2017-04-06 13:11:17 -04:00
Baha e3fde6e360 Handle RESTBase 404
Treat these responses not as an API failure. Show a generic
preview whenever the server responds with a 404.

Bug: T160744
Change-Id: Id6169d9d4c7493f5b6511cc78fe65d448cdadc03
2017-04-06 18:03:11 +02:00