-
Notifications
You must be signed in to change notification settings - Fork 138
Add push notification tracking for Milestone 1 #15239
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
base: trunk
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
JorgeMucientes
left a comment
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.
Nicely done @irfano.
Tracking works as expected, however I left a suggestion that might be worth checking, as that would expand the support for common properties to the rest of the tracking events.
Also, it would be a nice opportunity to do some legacy properties clean up.
| this["is_jetpack_installed"] = site.isJetpackInstalled | ||
| this["is_jetpack_connected"] = site.isJetpackConnected | ||
| this["is_jetpack_cp_connected"] = site.isJetpackCPConnected | ||
| this["is_ciab"] = site.isCIABSite() |
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.
Suggestion.
I think we could set this properties in AnalyticsTracker.ktclass instead. In particular inbuildFinalProperties()function. This goes in line with what was suggeted in the P2 tracking plan comments. At the very least I'd addis_ciabandgarden_partnerthere. And while you are at it, remove the propertyfinalProperties[KEY_WAS_ECOMMERCE_TRIAL] = it.wasEcommerceTrial` from there. It doesn't make sense to track that anymore.
| this["is_jetpack_connected"] = site.isJetpackConnected | ||
| this["is_jetpack_cp_connected"] = site.isJetpackCPConnected | ||
| this["is_ciab"] = site.isCIABSite() | ||
| site.gardenPartner?.let { this["garden_partner"] = it } |
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.
I haven't checked the API response yet, but in all my tests garde_partner was always null. I wonder if they have removed this field from the site model. I wouldn't be surprised tbh
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Description
Implements Milestone 1 push notification tracking as outlined in the pe5sF9-4Xn-p2.
New Events:
woo_push_token_register_success- Tracked when Woo Core push token registration succeedswoo_push_token_register_error- Tracked when registration fails, includes error detailswpcom_device_disable_push_notifications_success- Tracked when WPCOM notifications are disabled*wpcom_device_disable_push_notifications_error- Tracked when disabling fails*Updated Events:
push_notification_received- Added common properties (is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner)
push_notification_tapped- Added common propertiesRefactoring:
NotificationAnalyticsTrackernow usessiteIdparameter instead ofSiteModeltrackError()method for error event tracking with error detailsSiteStoredependency fromNotificationMessageHandlerTest Steps
1. Test
woo_push_token_register_successPreconditions:
WOO_PUSH_NOTIFICATIONS_SYSTEMenabledSteps:
Verify:
woocommerceandroid_woo_push_token_register_successProperties:is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner.2. Test
woo_push_token_register_errorPreconditions:
WOO_PUSH_NOTIFICATIONS_SYSTEMenabledSteps:
Verify:
woocommerceandroid_woo_push_token_register_errorProperties:is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner,error_description,error_type,error_code.3. Test
wpcom_device_disable_push_notifications_successPreconditions:
Steps:
Verify:
woocommerceandroid_wpcom_device_disable_push_notifications_successProperties:is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner.4. Test
wpcom_device_disable_push_notifications_errorPreconditions:
Steps:
Verify:
woocommerceandroid_wpcom_device_disable_push_notifications_errorProperties:error_description("Device not registered."),error_type("UnregisteredDevice"),is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner.5. Test
push_notification_receivedPreconditions:
Steps:
Verify:
woocommerceandroid_push_notification_receivedProperties:is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partner6. Test
push_notification_tappedPreconditions:
Steps:
Verify:
woocommerceandroid_push_notification_tapped: Properties:
is_jetpack_installed,is_jetpack_connected,is_jetpack_cp_connected,is_ciab,garden_partnerImages/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.