-
Notifications
You must be signed in to change notification settings - Fork 136
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
Issue/12539 notification type api improvements #12580
Issue/12539 notification type api improvements #12580
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
BLAZE_CANCELLED_NOTE, | ||
BLAZE_PERFORMED_NOTE, | ||
sealed interface WooNotificationType : Parcelable { | ||
val trackingValue: String |
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.
Had to add this property to ensure we keep the same tracking values we were previously using in NotificationAnalyticsTracker.
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #12580 +/- ##
=========================================
Coverage 40.62% 40.63%
+ Complexity 5669 5666 -3
=========================================
Files 1228 1228
Lines 68944 68943 -1
Branches 9549 9549
=========================================
+ Hits 28010 28012 +2
- Misses 38357 38358 +1
+ Partials 2577 2573 -4 ☔ View full report in Codecov by Sentry. |
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.
Nice work, thanks @JorgeMucientes for taking care of this.
I left a question about a change to the trackingValue
for Blaze notifications, please check it out before merging this.
sealed interface BlazeStatusUpdate : WooNotificationType, Parcelable { | ||
@Parcelize | ||
data object BlazeApprovedNote : BlazeStatusUpdate { | ||
@IgnoredOnParcel override val trackingValue: String = "blaze_approved_note" |
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.
Question, is having these lowercase for Blaze expected? I think they were using upper case previously.
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.
Good catch @hichamboushaba. Yes, previously we were tracking upper case values for Blaze pushes. While it wasn't a big deal, that was actually different from what we agreed and defined in the PT tracking section for both platforms: pffQ75-GL-p2 .
So, given that I was refactoring and fixing this, I decided to set the values correctly. I don't think it will be a big deal that for a couple versions we tracked the Blaze values in upper case style.
API improvement of the current implementation fow: #12539
Description
Coming from this feedback: #12564 (comment) this changes aim to improve the API to handle
WooNotificationType
. The main change is convertingWooNotificationType
fromenum class
tosealed interface
Testing information
Passing the build checks should be enough. But in case you want to ensure everything works well feel free to smoke tests the push notifications.
The tests that have been performed
Smoke-tested push notifications
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: