jenkins-bot has submitted this change and it was merged.
Change subject: Fix duplicate impression logging
......................................................................
Fix duplicate impression logging
We have a wrapper around logInteraction() called logNotificationImpressions()
that logs impressions uniquely (i.e. logs each notification impression only
once),
but in addition to calling that (from NotificationsWidget), we were also
manually
calling logInteraction() to log impressions from NotificationBadgeWidget
and NotificationGroupItemWidget. This resulted in two impression events
for every notification, each with different data, one of which is logged
only once and one of which can be logged multiple times.
Remove the manual logImpression() calls and route everything through
logNotificationImpressions(), which is called from only one place:
NotificationsWidget. Add support for logging foreign wikis to
logNotificationImpressions(), as it was previously missing.
This causes us to lose the notification type information in these
events, but that can also be derived after the fact by looking
up the event_id in the echo_event table.
Whether impression logging is even useful is another question,
but it certainly isn't useful if we log duplicate impression
events with different data.
Change-Id: I19b76a4ce796b21e9347dd9392af24918db82e18
---
M modules/logger/mw.echo.Logger.js
M modules/ooui/mw.echo.ui.NotificationBadgeWidget.js
M modules/ooui/mw.echo.ui.NotificationGroupItemWidget.js
M modules/ooui/mw.echo.ui.NotificationsWidget.js
4 files changed, 20 insertions(+), 45 deletions(-)
Approvals:
Mooeypoo: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/logger/mw.echo.Logger.js b/modules/logger/mw.echo.Logger.js
index 0eaf01c..84261cf 100644
--- a/modules/logger/mw.echo.Logger.js
+++ b/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;
}
}
};
diff --git a/modules/ooui/mw.echo.ui.NotificationBadgeWidget.js
b/modules/ooui/mw.echo.ui.NotificationBadgeWidget.js
index 840ba12..8804285 100644
--- a/modules/ooui/mw.echo.ui.NotificationBadgeWidget.js
+++ b/modules/ooui/mw.echo.ui.NotificationBadgeWidget.js
@@ -300,9 +300,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();
@@ -311,20 +308,6 @@
// Mark notifications as 'read' if
markReadWhenSeen is set to true
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()
- );
- }
}
}
diff --git a/modules/ooui/mw.echo.ui.NotificationGroupItemWidget.js
b/modules/ooui/mw.echo.ui.NotificationGroupItemWidget.js
index 68509f9..b277ee2 100644
--- a/modules/ooui/mw.echo.ui.NotificationGroupItemWidget.js
+++ b/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
diff --git a/modules/ooui/mw.echo.ui.NotificationsWidget.js
b/modules/ooui/mw.echo.ui.NotificationsWidget.js
index b34f136..41b5b2c 100644
--- a/modules/ooui/mw.echo.ui.NotificationsWidget.js
+++ b/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()
+ );
}
};
--
To view, visit https://gerrit.wikimedia.org/r/274342
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I19b76a4ce796b21e9347dd9392af24918db82e18
Gerrit-PatchSet: 3
Gerrit-Project: mediawiki/extensions/Echo
Gerrit-Branch: master
Gerrit-Owner: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: Mooeypoo <mor...@gmail.com>
Gerrit-Reviewer: Sbisson <sbis...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
