Merge "Fix duplicate impression logging"

modules/logger/mw.echo.Logger.js

@@ -88,7 +88,7 @@
 	 * @param {int} [eventId] Notification event id
 	 * @param {string} [eventType] notification type
 	 * @param {boolean} [mobile] True if interaction was on a mobile device
-	 * @param {string} [notifWiki] Wiki the notification came from
+	 * @param {string} [notifWiki] Foreign wiki the notification came from
 	 */
 	mw.echo.Logger.prototype.logInteraction = function ( action, context, eventId, eventType, mobile, notifWiki ) {
 		var myEvt;
@@ -115,7 +115,7 @@
 			myEvt.mobile = mobile;
 		}
 
-		if ( notifWiki && notifWiki !== mw.config.get( 'wgDBname' ) ) {
+		if ( notifWiki && notifWiki !== mw.config.get( 'wgDBname' ) && notifWiki !== 'local' ) {
 			myEvt.notifWiki = notifWiki;
 		}
 
@@ -130,17 +130,22 @@
 	 * @param {string} type Notification type; 'alert' or 'message'
 	 * @param {number[]} notificationIds Array of notification ids
 	 * @param {string} context 'flyout'/'archive' or undefined for the badge
-	 * @param {boolean} [mobile] True if interaction was on a mobile device
+	 * @param {string} [notifWiki='local'] Foreign wiki the notifications came from
+	 * @param {boolean} [mobile=false] True if interaction was on a mobile device
 	 */
-	mw.echo.Logger.prototype.logNotificationImpressions = function ( type, notificationIds, context, mobile ) {
-		var i, len;
+	mw.echo.Logger.prototype.logNotificationImpressions = function ( type, notificationIds, context, notifWiki, mobile ) {
+		var i, len, key;
 
 		for ( i = 0, len = notificationIds.length; i < len; i++ ) {
-			if ( !this.notificationsIdCache[ notificationIds[ i ] ] ) {
+			key = notifWiki && notifWiki !== mw.config.get( 'wgDBname' ) && notifWiki !== 'local' ?
+				notificationIds[ i ] + '-' + notifWiki :
+				notificationIds[ i ];
+
+			if ( !this.notificationsIdCache[ key ] ) {
 				// Log notification impression
-				this.logInteraction( 'notification-impression', context, notificationIds[ i ], type, mobile );
+				this.logInteraction( 'notification-impression', context, notificationIds[ i ], type, mobile, notifWiki );
 				// Cache
-				this.notificationsIdCache[ notificationIds[ i ] ] = true;
+				this.notificationsIdCache[ key ] = true;
 			}
 		}
 	};

modules/ooui/mw.echo.ui.NotificationBadgeWidget.js

@@ -301,9 +301,6 @@
 		// the case where the promise is already underway.
 		this.notificationsModel.fetchNotifications()
 			.then( function () {
-				var i,
-					items = widget.notificationsWidget.getItems();
-
 				if ( widget.popup.isVisible() ) {
 					widget.popup.clip();
 
@@ -313,20 +310,6 @@
 					if ( widget.markReadWhenSeen ) {
 						widget.notificationsModel.autoMarkAllRead();
 					}
-
-					// Log impressions
-					// TODO: Only log the impressions of notifications that
-					// are actually visible
-					for ( i = 0; i < items.length; i++ ) {
-						if ( items[ i ].getModel ) {
-							mw.echo.logger.logInteraction(
-								mw.echo.Logger.static.actions.notificationImpression,
-								mw.echo.Logger.static.context.popup,
-								items[ i ].getModel().getId(),
-								items[ i ].getModel().getCategory()
-							);
-						}
-					}
 				}
 
 				widget.emit( 'finishLoading' );

modules/ooui/mw.echo.ui.NotificationGroupItemWidget.js

@@ -125,26 +125,8 @@
 
 			// Query all sources
 			this.model.fetchAllNotificationsInGroups()
-				.then( function ( /* Groups */ ) {
-					var source, items, i,
-						models = widget.model.getSubModels();
-
+				.then( function () {
 					widget.popPending();
-
-					// Log impressions of all items from each group
-					for ( source in models ) {
-						items = models[ source ].getItems();
-						for ( i = 0; i < items.length; i++ ) {
-							mw.echo.logger.logInteraction(
-								mw.echo.Logger.static.actions.notificationImpression,
-								mw.echo.Logger.static.context.popup,
-								items[ i ].getId(),
-								items[ i ].getCategory(),
-								false,
-								source
-							);
-						}
-					}
 				} );
 
 			// Log the expand action

modules/ooui/mw.echo.ui.NotificationsWidget.js

@@ -86,7 +86,12 @@
 
 		if ( isSuccess ) {
 			// Log impressions
-			mw.echo.logger.logNotificationImpressions( this.type, result.ids, mw.echo.Logger.static.context.popup );
+			mw.echo.logger.logNotificationImpressions(
+				undefined, // type: we don't know
+				result.ids,
+				mw.echo.Logger.static.context.popup,
+				this.getModel().getSource()
+			);
 		}
 	};