Jdlrobson has uploaded a new change for review.
https://gerrit.wikimedia.org/r/315610
Change subject: Move logic for dwelledButAbandoned to central log method
......................................................................
Move logic for dwelledButAbandoned to central log method
Add tests.
Change-Id: Ieba557c9adb544db6200aecdf141587c49ebb2e2
---
M resources/ext.popups.core.js
M resources/ext.popups.renderer/desktopRenderer.js
M resources/ext.popups.targets/desktopTarget.js
M tests/qunit/ext.popups.core.test.js
4 files changed, 60 insertions(+), 31 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Popups
refs/changes/10/315610/1
diff --git a/resources/ext.popups.core.js b/resources/ext.popups.core.js
index 42d5418..9964937 100644
--- a/resources/ext.popups.core.js
+++ b/resources/ext.popups.core.js
@@ -1,7 +1,16 @@
( function ( $, mw ) {
'use strict';
- var previewCountStorageKey = 'ext.popups.core.previewCount',
+ var DWELL_EVENTS_MIN_INTERACTION_TIME,
+ previewCountStorageKey = 'ext.popups.core.previewCount',
popupsEnabledStorageKey = 'mwe-popups-enabled';
+
+ /**
+ * Minimum time to log dwelledButAbandoned events. Initially considered
as
+ * 250ms to avoid accidental hovers being logged. Now logging all
events to
+ * verify data. See T145379
+ * @property DWELL_EVENTS_MIN_INTERACTION_TIME
+ */
+ DWELL_EVENTS_MIN_INTERACTION_TIME = 0;
/**
* @class mw.popups
@@ -36,6 +45,25 @@
];
/**
+ * Decide whether to log an event
+ *
+ * @method log
+ * @param {string} action that is being logged
+ * @param {Object} [logData]
+ */
+ function shouldLog( action, logData ) {
+ if ( action === 'dwelledButAbandoned' &&
+ logData.dwellStartTime &&
+ logData.linkInteractionToken &&
+ mw.now() - logData.dwellStartTime <
DWELL_EVENTS_MIN_INTERACTION_TIME
+ ) {
+ return false;
+ } else {
+ return true;
+ }
+ }
+
+ /**
* Log events
*
* @method log
@@ -44,13 +72,16 @@
*/
mw.popups.log = function ( action, logData ) {
logData = logData || {};
- mw.track( 'ext.popups.schemaPopups',
- $.extend( {}, logData, {
- action: action,
- // Some events e.g. pageLoaded do not have an
interaction time.
- totalInteractionTime: logData.dwellStartTime ?
Math.round( mw.now() - logData.dwellStartTime ) : undefined
- } )
- );
+
+ if ( shouldLog( action, logData ) ) {
+ mw.track( 'ext.popups.schemaPopups',
+ $.extend( {}, logData, {
+ action: action,
+ // Some events e.g. pageLoaded do not
have an interaction time.
+ totalInteractionTime:
logData.dwellStartTime ? Math.round( mw.now() - logData.dwellStartTime ) :
undefined
+ } )
+ );
+ }
};
/**
diff --git a/resources/ext.popups.renderer/desktopRenderer.js
b/resources/ext.popups.renderer/desktopRenderer.js
index 7b574d5..1005f56 100644
--- a/resources/ext.popups.renderer/desktopRenderer.js
+++ b/resources/ext.popups.renderer/desktopRenderer.js
@@ -75,14 +75,6 @@
mw.popups.render.API_DELAY = 50;
/**
- * Minimum time to log dwelledButAbandoned events. Initially considered
as
- * 250ms to avoid accidental hovers being logged. Now logging all
events to
- * verify data. See T145379
- * @property DWELL_EVENTS_MIN_INTERACTION_TIME
- */
- mw.popups.render.DWELL_EVENTS_MIN_INTERACTION_TIME = 0;
-
- /**
* Cache of all the popups that were opened in this session
* @property {Object} cache
*/
@@ -372,12 +364,7 @@
function leaveInactive() {
var $activeLink = getActiveLink();
- if ( logData.dwellStartTime &&
- logData.linkInteractionToken &&
- mw.now() - logData.dwellStartTime >=
mw.popups.render.DWELL_EVENTS_MIN_INTERACTION_TIME
- ) {
- mw.popups.log( 'dwelledButAbandoned', logData );
- }
+ mw.popups.log( 'dwelledButAbandoned', logData );
// TODO: should `blur` also be here?
$activeLink.off( 'mouseleave', leaveInactive );
if ( openTimer ) {
diff --git a/resources/ext.popups.targets/desktopTarget.js
b/resources/ext.popups.targets/desktopTarget.js
index 3982147..58cd372 100644
--- a/resources/ext.popups.targets/desktopTarget.js
+++ b/resources/ext.popups.targets/desktopTarget.js
@@ -20,15 +20,11 @@
$this.off( 'mouseleave blur', onLinkAbandon );
- if ( data.dwellStartTime && data.linkInteractionToken &&
- mw.now() - data.dwellStartTime >=
mw.popups.render.DWELL_EVENTS_MIN_INTERACTION_TIME
- ) {
- mw.popups.log( 'dwelledButAbandoned', {
- pageTitleHover: $this.attr( 'title' ),
- linkInteractionToken: data.linkInteractionToken,
- hovercardsSuppressedByGadget:
data.hovercardsSuppressedByGadget
- } );
- }
+ mw.popups.log( 'dwelledButAbandoned', {
+ pageTitleHover: $this.attr( 'title' ),
+ linkInteractionToken: data.linkInteractionToken,
+ hovercardsSuppressedByGadget:
data.hovercardsSuppressedByGadget
+ } );
}
/**
diff --git a/tests/qunit/ext.popups.core.test.js
b/tests/qunit/ext.popups.core.test.js
index 360c2ee..b167146 100644
--- a/tests/qunit/ext.popups.core.test.js
+++ b/tests/qunit/ext.popups.core.test.js
@@ -6,6 +6,21 @@
}
} ) );
+
+ QUnit.test( 'log#dwelledButAbandoned', 2, function ( assert ) {
+ var spy = this.sandbox.spy( mw, 'track' );
+ mw.popups.log( 'dwelledButAbandoned', {} );
+ assert.ok( spy.called );
+ spy.reset();
+ mw.popups.log( 'dwelledButAbandoned', {
+ // make this in the future so answer is negative
+ dwellStartTime: mw.now() + 1000,
+ linkInteractionToken: 'yes'
+ } );
+ assert.ok( !spy.called,
+ 'Should not be called if event is shorter than
DWELL_EVENTS_MIN_INTERACTION_TIME (currently equal to 0)' );
+ } );
+
QUnit.test( 'getTitle', function ( assert ) {
var cases, i, expected, actual;
--
To view, visit https://gerrit.wikimedia.org/r/315610
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieba557c9adb544db6200aecdf141587c49ebb2e2
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Popups
Gerrit-Branch: master
Gerrit-Owner: Jdlrobson <jrob...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
