From cda1ffe425c6ef47394f65feabeb666f50f3f6bd Mon Sep 17 00:00:00 2001
From: Baha <bmansurov@wikimedia.org>
Date: Thu, 12 May 2016 18:37:19 -0400
Subject: [PATCH] Use mw.eventLog.Schema to log EventLogging events

This change is an intermediate step in our transition
to logging a variety of events easily.

Also:
  * Some events may need sendBeacon support and may not be
    logged if the navigator does not support sendBeacon.
  * Do not load schema code if EventLogging is not available.

Bug: T131315
Change-Id: Iff939577f65f1c6c71701dd6967939445385fb70
---
 Popups.hooks.php                              |  24 +++-
 extension.json                                |   4 +-
 resources/ext.popups.core.js                  |  21 +++
 resources/ext.popups.logger.js                |  64 ---------
 .../ext.popups.renderer/desktopRenderer.js    |  53 ++++---
 resources/ext.popups.schemaPopups.js          |  15 ++
 resources/ext.popups.targets/desktopTarget.js |  20 +--
 tests/qunit/ext.popups.core.test.js           |  24 ++++
 tests/qunit/ext.popups.logger.test.js         | 133 ------------------
 9 files changed, 122 insertions(+), 236 deletions(-)
 delete mode 100644 resources/ext.popups.logger.js
 create mode 100644 resources/ext.popups.schemaPopups.js
 delete mode 100644 tests/qunit/ext.popups.logger.test.js

diff --git a/Popups.hooks.php b/Popups.hooks.php
index 8af73dade..e868af7ce 100644
--- a/Popups.hooks.php
+++ b/Popups.hooks.php
@@ -64,14 +64,28 @@ class PopupsHooks {
 			'ext.popups.targets.desktopTarget',
 		);
 
-		// If EventLogging is present, add the schema as a dependency.
-		if ( class_exists( 'ResourceLoaderSchemaModule' ) ) {
-			$moduleDependencies[] = "schema.Popups";
+		// Create a schema module and add it as a dependency of `ext.popups.desktop`.
+		$schemaPopups = [
+			'remoteExtPath' => 'Popups',
+			'localBasePath' => __DIR__,
+			'targets' => [ 'desktop' ],
+		];
+
+		if ( class_exists( 'EventLogging' ) ) {
+			$schemaPopups += [
+				'dependencies' => [
+					'schema.Popups',
+				],
+				'scripts' => [
+					'resources/ext.popups.schemaPopups.js',
+				]
+			];
 		}
+		$rl->register('ext.popups.schemaPopups', $schemaPopups );
+		$moduleDependencies[] = 'ext.popups.schemaPopups';
 
 		$rl->register( "ext.popups.desktop", array(
 			'scripts' => array(
-				'resources/ext.popups.logger.js',
 				'resources/ext.popups.renderer.article.js',
 				'resources/ext.popups.disablenavpop.js',
 				'resources/ext.popups.settings.js',
@@ -206,7 +220,6 @@ class PopupsHooks {
 			'scripts' => array(
 				'tests/qunit/ext.popups.renderer.article.test.js',
 				'tests/qunit/ext.popups.core.test.js',
-				'tests/qunit/ext.popups.logger.test.js',
 				'tests/qunit/ext.popups.settings.test.js',
 			),
 			'dependencies' => array( 'ext.popups.desktop' ),
@@ -222,5 +235,6 @@ class PopupsHooks {
 	public static function onResourceLoaderGetConfigVars( array &$vars ) {
 		$conf = ConfigFactory::getDefaultInstance()->makeConfig( 'popups' );
 		$vars['wgPopupsSurveyLink'] = $conf->get( 'PopupsSurveyLink' );
+		$vars['wgPopupsSchemaPopupsSamplingRate'] = $conf->get( 'SchemaPopupsSamplingRate' );
 	}
 }
diff --git a/extension.json b/extension.json
index 980a25eec..22767b8b6 100644
--- a/extension.json
+++ b/extension.json
@@ -47,7 +47,9 @@
 		"PopupsBetaFeature": false,
 		"@PopupsSurveyLink": "@var bool|string: When defined a link will be rendered at the bottom of the popup for the user to provide feedback. The URL must start with https or http. If not, then an error is thrown client-side. The link is annotated with `rel=\"noreferrer\"` so no referrer information or `window.opener` is leaked to the survey hosting site (see https://html.spec.whatwg.org/multipage/semantics.html#link-type-noreferrer for more information).",
 		"PopupsSurveyLink": false,
-		"EnablePopupsMobile": false
+		"EnablePopupsMobile": false,
+		"@SchemaPopupsSamplingRate": "@var number: Sample rate for logging events to Schema:Popups.",
+		"SchemaPopupsSamplingRate": 0.1
 	},
 	"DefaultUserOptions": {
 		"popupsmobile": "1"
diff --git a/resources/ext.popups.core.js b/resources/ext.popups.core.js
index 46487cf3f..80900c338 100644
--- a/resources/ext.popups.core.js
+++ b/resources/ext.popups.core.js
@@ -140,4 +140,25 @@
 			} );
 	};
 
+	/**
+	 * Get action based on click event
+	 *
+	 * @method getAction
+	 * @param {Object} event
+	 * @return {string}
+	 */
+	mw.popups.getAction = function ( event ) {
+		if ( event.which === 2 ) { // middle click
+			return 'opened in new tab';
+		} else if ( event.which === 1 ) {
+			if ( event.ctrlKey || event.metaKey ) {
+				return 'opened in new tab';
+			} else if ( event.shiftKey ) {
+				return 'opened in new window';
+			} else {
+				return 'opened in same tab';
+			}
+		}
+	};
+
 } )( jQuery, mediaWiki );
diff --git a/resources/ext.popups.logger.js b/resources/ext.popups.logger.js
deleted file mode 100644
index b1b2c0bc8..000000000
--- a/resources/ext.popups.logger.js
+++ /dev/null
@@ -1,64 +0,0 @@
-( function ( $, mw ) {
-
-	/**
-	 * @class mw.popups.logger
-	 * @singleton
-	 */
-	var logger = {};
-
-	/**
-	 * Sampling rate at which events are logged
-	 * @property samplingRate
-	 */
-	logger.samplingRate = 10;
-
-	/**
-	 * Get action based on click event
-	 *
-	 * @method getAction
-	 * @param {Object} event
-	 * @return {string}
-	 */
-	logger.getAction = function ( event ) {
-		if ( event.which === 2 ) { // middle click
-			return 'opened in new tab';
-		} else if ( event.which === 1 ) {
-			if ( event.ctrlKey || event.metaKey ) {
-				return 'opened in new tab';
-			} else if ( event.shiftKey ) {
-				return 'opened in new window';
-			} else {
-				return 'opened in same tab';
-			}
-		}
-	};
-
-	/**
-	 * Logs the popup event as defined in the following schema-
-	 * https://meta.wikimedia.org/wiki/Schema:Popups
-	 *
-	 * @method log
-	 * @param {Object} event
-	 * @return {jQuery.Promise}
-	 */
-	logger.log = function ( event ) {
-		if (
-			mw.eventLog === undefined ||
-			Math.floor( Math.random() * logger.samplingRate ) !== 0
-		) {
-			return $.Deferred().resolve();
-		}
-
-		// Get duration from  time
-		if ( $.isNumeric( event.time ) ) {
-			event.duration = Math.floor( mw.now() - event.time );
-			// FIXME: the time property should not be sent to the back-end regardless of its value.
-			delete event.time;
-		}
-
-		return mw.eventLog.logEvent( 'Popups', event );
-	};
-
-	mw.popups.logger = logger;
-
-} )( jQuery, mediaWiki );
diff --git a/resources/ext.popups.renderer/desktopRenderer.js b/resources/ext.popups.renderer/desktopRenderer.js
index 4dfea221f..2d4701206 100644
--- a/resources/ext.popups.renderer/desktopRenderer.js
+++ b/resources/ext.popups.renderer/desktopRenderer.js
@@ -185,16 +185,13 @@
 		cache.process( link );
 
 		// Event logging
-		if ( mw.popups.logger ) {
-			mw.popups.render.logEvent = {
-				pageTitleHover: cache.settings.title,
-				pageTitleSource: mw.config.get( 'wgTitle' ),
-				popupEnabled: mw.popups.enabled,
-				time: mw.now(),
-				action: 'dismissed'
-			};
-			mw.popups.$popup.find( 'a.mwe-popups-extract, a.mwe-popups-discreet' ).click( mw.popups.render.clickHandler );
-		}
+		mw.popups.logData = {
+			pageTitleHover: cache.settings.title,
+			pageTitleSource: mw.config.get( 'wgTitle' ),
+			popupEnabled: mw.popups.enabled,
+			time: mw.now()
+		};
+		mw.popups.$popup.find( 'a.mwe-popups-extract, a.mwe-popups-discreet' ).click( mw.popups.render.clickHandler );
 
 		link
 			.off( 'mouseleave blur', mw.popups.render.leaveInactive )
@@ -210,12 +207,16 @@
 	 * @param {Object} event
 	 */
 	mw.popups.render.clickHandler = function ( event ) {
-		mw.popups.render.logEvent.action = mw.popups.logger.getAction( event );
-		if ( mw.popups.render.logEvent.action === 'opened in same tab' ) {
-			event.preventDefault();
-			mw.popups.logger.log( mw.popups.render.logEvent ).then( function () {
-				window.location.href = mw.popups.render.currentLink.attr( 'href' );
+		var action = mw.popups.getAction( event ),
+			logData;
+
+		if ( action === 'opened in same tab' ) {
+			logData = $.extend( {}, mw.popups.logData, {
+				action: action
 			} );
+			computeDurationFromTime( logData );
+			mw.track( 'ext.popups.schemaPopups', logData );
+			window.location.href = mw.popups.render.currentLink.attr( 'href' );
 		}
 	};
 
@@ -226,16 +227,18 @@
 	 * @method closePopup
 	 */
 	mw.popups.render.closePopup = function () {
-		var fadeInClass, fadeOutClass;
+		var fadeInClass, fadeOutClass,
+			logData = $.extend( {}, mw.popups.logData, {
+				action: 'dismissed'
+			} );
 
 		if ( mw.popups.render.currentLink === undefined ) {
 			return false;
 		}
 
 		// Event logging
-		if ( mw.popups.logger ) {
-			mw.popups.logger.log( mw.popups.render.logEvent );
-		}
+		computeDurationFromTime( logData );
+		mw.track( 'ext.popups.schemaPopups', logData );
 
 		$( mw.popups.render.currentLink ).off( 'mouseleave blur', mw.popups.render.leaveActive );
 
@@ -347,4 +350,16 @@
 		mw.popups.render.closeTimer = undefined;
 	};
 
+	/**
+	 * Utility function that computes duration from time.
+	 * Modifies the data so that it can be logged with EL.
+	 *
+	 * @ignore
+	 * @param {Object} data
+	 */
+	function computeDurationFromTime( data ) {
+		data.duration = Math.floor( mw.now() - data.time );
+		delete data.time;
+	}
+
 } )( jQuery, mediaWiki );
diff --git a/resources/ext.popups.schemaPopups.js b/resources/ext.popups.schemaPopups.js
new file mode 100644
index 000000000..9deb2fba2
--- /dev/null
+++ b/resources/ext.popups.schemaPopups.js
@@ -0,0 +1,15 @@
+( function ( $, mw ) {
+	/**
+	 * Log the popup event as defined in the schema
+	 *
+	 * https://meta.wikimedia.org/wiki/Schema:Popups
+	 */
+	var schemaPopups = new mw.eventLog.Schema(
+		'Popups',
+		mw.config.get( 'wgPopupsSchemaPopupsSamplingRate', 0 )
+	);
+
+	mw.trackSubscribe( 'ext.popups.schemaPopups', function ( topic, data ) {
+		schemaPopups.log( data );
+	} );
+} )( jQuery, mediaWiki );
diff --git a/resources/ext.popups.targets/desktopTarget.js b/resources/ext.popups.targets/desktopTarget.js
index 1168c4d10..2df503e76 100644
--- a/resources/ext.popups.targets/desktopTarget.js
+++ b/resources/ext.popups.targets/desktopTarget.js
@@ -108,28 +108,20 @@
 			// Events are logged even when Hovercards are disabled
 			// See T88166 for details
 			$elements.on( 'click', function ( event ) {
-				var $this, href, action, logEvent, logPromise;
+				var $this = $( this ),
+					action = mw.popups.getAction( event ),
+					href = $this.attr( 'href' );
 
-				if ( mw.popups.logger === undefined ) {
-					return true;
-				}
-
-				$this = $( this );
-				href = $this.attr( 'href' );
-				action = mw.popups.logger.getAction( event );
-				logEvent = {
+				mw.track( 'ext.popups.schemaPopups', {
 					pageTitleHover: $this.attr( 'title' ),
 					pageTitleSource: mw.config.get( 'wgTitle' ),
 					popupEnabled: mw.popups.enabled,
 					action: action
-				};
-				logPromise = mw.popups.logger.log( logEvent );
+				} );
 
 				if ( action  === 'opened in same tab' ) {
 					event.preventDefault();
-					logPromise.then( function () {
-						window.location.href = href;
-					} );
+					window.location.href = href;
 				}
 			} );
 		}
diff --git a/tests/qunit/ext.popups.core.test.js b/tests/qunit/ext.popups.core.test.js
index 78e7a4019..4ecb90a29 100644
--- a/tests/qunit/ext.popups.core.test.js
+++ b/tests/qunit/ext.popups.core.test.js
@@ -161,4 +161,28 @@
 		$link.remove();
 	} );
 
+	QUnit.test( 'getAction', function ( assert ) {
+		var i, expected, actual,
+			// 0 - main button, 1 - middle button
+			cases = [
+				[ { button: 0 }, 'opened in same tab' ],
+				[ { button: 0, ctrlKey: true }, 'opened in new tab' ],
+				[ { button: 0, metaKey: true }, 'opened in new tab' ],
+				[ { button: 0, ctrlKey: true, shiftKey: true }, 'opened in new tab' ],
+				[ { button: 0, metaKey: true, shiftKey: true }, 'opened in new tab' ],
+				[ { button: 0, ctrlKey: true, metaKey: true, shiftKey: true }, 'opened in new tab' ],
+				[ { button: 0, shiftKey: true }, 'opened in new window' ],
+				[ { button: 1 }, 'opened in new tab' ],
+				[ { button: 1, shiftKey: true }, 'opened in new tab' ]
+			];
+
+		QUnit.expect( cases.length );
+
+		for ( i = 0; i < cases.length; i++ ) {
+			expected = cases[ i ][ 1 ];
+			actual = mw.popups.getAction( new MouseEvent( 'CustomEvent', cases[ i ][ 0 ] ) );
+			assert.equal( actual, expected );
+		}
+	} );
+
 } )( jQuery, mediaWiki );
diff --git a/tests/qunit/ext.popups.logger.test.js b/tests/qunit/ext.popups.logger.test.js
deleted file mode 100644
index 7843e2c03..000000000
--- a/tests/qunit/ext.popups.logger.test.js
+++ /dev/null
@@ -1,133 +0,0 @@
-( function ( $, mw ) {
-	QUnit.module( 'ext.popups.logger' );
-
-	QUnit.test( 'samplingRate', function ( assert ) {
-		QUnit.expect( 1 );
-
-		// make sure the sampling rate is not accidentally changed
-		assert.equal( mw.popups.logger.samplingRate, 10 );
-	} );
-
-	QUnit.test( 'getAction', function ( assert ) {
-		var i, expected, actual,
-			// 0 - main button, 1 - middle button
-			cases = [
-				[ { button: 0 }, 'opened in same tab' ],
-				[ { button: 0, ctrlKey: true }, 'opened in new tab' ],
-				[ { button: 0, metaKey: true }, 'opened in new tab' ],
-				[ { button: 0, ctrlKey: true, shiftKey: true }, 'opened in new tab' ],
-				[ { button: 0, metaKey: true, shiftKey: true }, 'opened in new tab' ],
-				[ { button: 0, ctrlKey: true, metaKey: true, shiftKey: true }, 'opened in new tab' ],
-				[ { button: 0, shiftKey: true }, 'opened in new window' ],
-				[ { button: 1 }, 'opened in new tab' ],
-				[ { button: 1, shiftKey: true }, 'opened in new tab' ]
-			];
-
-		QUnit.expect( cases.length );
-
-		for ( i = 0; i < cases.length; i++ ) {
-			expected = cases[ i ][ 1 ];
-			actual = mw.popups.logger.getAction( new MouseEvent( 'CustomEvent', cases[ i ][ 0 ] ) );
-			assert.equal( actual, expected );
-		}
-	} );
-
-	QUnit.module( 'ext.popups.logger (with EventLogging)', {
-		setup: function () {
-			this.eventLog = mw.eventLog;
-			if ( !mw.eventLog ) {
-				mw.eventLog = { logEvent: $.noop };
-			}
-			this.sandbox.stub( mw.eventLog, 'logEvent' );
-			this.logData = {
-				pageTitleHover: 'Main Page',
-				pageTitleSource: 'Popups test page',
-				popupEnabled: true,
-				action: 'opened in same tab',
-				time: new Date().getTime()
-			};
-		},
-		teardown: function () {
-			mw.eventLog.logEvent.restore();
-			mw.eventLog = this.eventLog;
-		}
-	} );
-
-	QUnit.test( 'log', function ( assert ) {
-		QUnit.expect( 6 );
-
-		// not sampled
-		this.sandbox.stub( Math, 'random' ).returns( 1 );
-		mw.popups.logger.log( $.extend( {}, this.logData ) ).done( function ( result ) {
-			assert.equal(
-				result,
-				undefined,
-				'Logger resolves with `undefined` when the page is not sampled.'
-			);
-		} );
-		Math.random.restore();
-
-		// Sampled
-		this.sandbox.stub( Math, 'random' ).returns( 0 );
-		mw.popups.logger.log( $.extend( {}, this.logData ) );
-		assert.ok(
-			mw.eventLog.logEvent.firstCall.args[ 1 ].hasOwnProperty( 'duration' ),
-			'The `duration` property has been added when `time` is a number.'
-		);
-		assert.notOk(
-			mw.eventLog.logEvent.firstCall.args[ 1 ].hasOwnProperty( 'time' ),
-			'The `time` property has been removed when it is a number.'
-		);
-
-		delete this.logData.time;
-		mw.popups.logger.log( this.logData );
-		assert.notOk(
-			mw.eventLog.logEvent.secondCall.args[ 1 ].hasOwnProperty( 'duration' ),
-			'The `duration` property has not been added when `time` is `undefined`.'
-		);
-
-		this.logData.time = 'September, 2046';
-		mw.popups.logger.log( this.logData );
-		assert.notOk(
-			mw.eventLog.logEvent.thirdCall.args[ 1 ].hasOwnProperty( 'duration' ),
-			'The `duration` property has not been added when `time` is non-numeric.'
-		);
-		assert.ok(
-			mw.eventLog.logEvent.thirdCall.args[ 1 ].hasOwnProperty( 'time' ),
-			'The `time` property has not been removed when it is non-numeric.'
-		);
-		Math.random.restore();
-
-	} );
-
-	QUnit.module( 'ext.popups.logger (without EventLogging)', {
-		setup: function () {
-			this.eventLog = mw.eventLog;
-			delete mw.eventLog;
-			// make sure we're sampled
-			this.sandbox.stub( Math, 'random' ).returns( 0 );
-			this.logData = {
-				pageTitleHover: 'Main Page',
-				pageTitleSource: 'Popups test page',
-				popupEnabled: true,
-				action: 'opened in same tab',
-				time: new Date().getTime()
-			};
-		},
-		teardown: function () {
-			mw.eventLog = this.eventLog;
-			Math.random.restore();
-		}
-	} );
-	QUnit.test( 'log', function ( assert ) {
-		QUnit.expect( 1 );
-
-		mw.popups.logger.log( this.logData ).done( function ( result ) {
-			assert.equal(
-				result,
-				undefined,
-				'Logger resolves with `undefined` when mw.eventLog is not available.'
-			);
-		} );
-	} );
-} )( jQuery, mediaWiki );