-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate event tracking and incorrect foreground event logging for push notifications #22956
Fix duplicate event tracking and incorrect foreground event logging for push notifications #22956
Conversation
… notification is received and when it is tapped
Generated by 🚫 Danger |
@@ -186,9 +186,6 @@ final public class PushNotificationsManager: NSObject { | |||
return | |||
} | |||
|
|||
// Analytics | |||
trackNotification(with: userInfo) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this line from PushNotificationsManager.handleNotification
resolved the issue of duplicated push_notification_alert_tapped
analytics events. The idea is that handleNotification
should only be concerned with handling the notification, irrespective of the context that triggered it (whether the notification was received in the background, or the notification alert was tapped). Analytics tracking has now been moved to the caller level.
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
return response.actionIdentifier == UNNotificationDefaultActionIdentifier ? .pushNotificationAlertPressed : nil | ||
} | ||
return .pushNotificationReceived | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic fixes the bug where the event push_notification_alert_tapped
is tracked when the app is in foreground. So the event was tracked even though the user doesn't tap the alert, which is misleading.
This event is now only tracked when the user taps the notification alert.
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this @salimbraksa 🙇
I confirm that the behavior introduced with this PR aligns with how the Android push_notification_received
and push_notification_alert_tapped
events work.
Fixes #22935
Description
This PR fixes an issue where the
push_notification_alert_tapped
event was being tracked multiple times. It ensures thatpush_notification_received
is consistently logged upon the arrival of any notification, andpush_notification_alert_tapped
is recorded exclusively when the user taps the notification alert. These changes also shift the responsibility of event tracking to the caller level, thereby improving the precision of our analytics.Test Instructions
push_notification_received
is tracked, regardless of the app's state ( foreground or background ).push_notification_alert_tapped
is tracked, and is not duplicated.Regression Notes
Potential unintended areas of impact
This PR impacts only the analytics tracking when a push notification is received or tapped.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.