Closed jaclync closed 2 years ago
@jaclync My WordPress.com sandbox was deleted a while ago. I requested a new one pMz3w-e4d-p2. It'll be a while for me to review this since I'll be AFK for a couple of days. Please re-assign if this will be a problem!
You can trigger an installable build for these changes by visiting CircleCI here.
Thanks for the heads up @shiki! No worries, I'll reassign the reviewer 😄
Trying @pmusolino or @ealeksandrov if you have a dev sandbox, and have some time Friday or Monday to review this 😃 we're hoping to get this in for release 7.9, but the status of the backend PR is still unknown so we might also just wait for another release cycle. @astralbodies might be able to help with testing as well!
We decided to move this feature to release 8.0, so take your time reviewing 🙂
You can trigger optional UI/connected tests for these changes by visiting CircleCI here.
In Dev MC push notifications debug console (link in p91TBi-6eM-p2#testing-push-notifications), resend the last 2 store order notifications
I'm not sure if I'm testing this right. 😐 I only see one push notification about the order in dev-mc
And I think that's expected. So, when I try to resend the push notifications of the last 2 orders, I get the push notifications for the same order only:
Also, they don't seem to be grouped together. What could I be missing?
@pmusolino or @ealeksandrov feel free to still review this even though I'm commenting. I have a feeling my problem is because I'm a noob. 😅
These 2 notifications should be grouped in Notification Center (they might not be grouped immediately, you can check back in a minute)
Update. 🤣 So it looks like the notifications will be grouped if I received the push notifications while the device is locked.
Thanks a lot for testing with dev MC after requesting sandbox @shiki 🙏 Updated the PR and responded to your comments if you'd like another look!
I only see one push notification about the order in dev-mc
Ah that's correct, each new order notification replaces the previous one for the same store. We can still send the same order notification multiple times from dev MC.
@pmusolino or @ealeksandrov feel free to still review this even though I'm commenting. I have a feeling my problem is because I'm a noob. 😅
I'm struggling for the second time in a month with our developer provisioning profiles 😰
Warnings | |
---|---|
:warning: | This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone |
Generated by :no_entry_sign: dangerJS
The backend changes D68195 were deployed, and I tested the PR branch with the app version set to 8.0.0.0 and also 7.9.0.0 with the feature flag off. Merging this PR now!
@jaclync 👏
Hey there! Tested what I could today without setting up DevMC - not sure if that impacts what's noted below.
I was able to experience all features except the following:
Tapping on push notifications In the app, go to Settings > Switch Store. Switch to a store that is not related to the push notifications you resent from Dev MC above Tap on a store order notification --> the app should switch to the store of the notification, and display order details. an in-app notice should show about the store switch "Switched to {{store name}}" Tap on a store review notification --> the app should display review details
App Open to STORE1, notification for STORE1:
App Open to STORE2, notification for STORE1:
iPhone XR - iOS 14.8.1, WooCommerce App 8.0
Fixes #5032 (the last part) Project thread p91TBi-5Oe-p2
Just one reviewer is required for code review.
⚠️ Checklist before merging ⚠️
Why
To support the new notification content for multi-store push notifications, we made all the notification payload changes in server side (D68195). Since the payload format also changes, we need to update how we parse the payload when handling the notification tap action. The new payload format also includes a new field
subtitle
, and this PR updated the in-app notice UI to show the subtitle.New payload format
Currently, we only have one body text
alert
in the payload:The new payload replaces the
alert
value from a string with a dictionary withtitle
,subtitle
, andbody
(the full list ofalert
keys). Additionally, it has a thread ID that groups notifications by type and site (no client side changes required):Changes
pushNotificationsForAllStores
feature flagsubtitle
toPushNotification
struct, and parsedPushNotification
based on the feature flagTesting
Because the backend changes are still pending review and not deployed yet, please test it with dev MC for now. I'm hoping for the feature to launch in release 7.9.
Prerequisites: To test it with dev MC, please refer to the steps in p91TBi-6eM-p2#testing-push-notifications Dev MC only works for a8c WP.com accounts, please make sure your a8c WP.com account is connected to more than one store.
Receiving push notifications in the background
arc patch D68195
)You have a new order! 🎉
(behavior in production)You have a new order! 🎉
in the title, andNew order for {{order amount}} on {{site name}}
in the body. These 2 notifications should be grouped in Notification Center (they might not be grouped immediately, you can check back in a minute){{customer name}} left a review on {{product name}}
(behavior in production)You have a new review! ⭐️
in the title, store name in the subtitle, and{{customer name}} left a review on {{product name}}: {{review content}}
in the body. These 2 notifications should be grouped in Notification Center (they might not be grouped immediately, you can check back in a minute)Receiving push notifications in the foreground
You have a new order! 🎉
in the title, andNew order for {{order amount}} on {{site name}}
in the bodyYou have a new review! ⭐️
in the title, store name in the subtitle, and{{customer name}} left a review on {{product name}}: {{review content}}
in the bodyTapping on push notifications
Regression (for release 7.8 and if this feature does not ship in time for 7.9)
Feel free to skip this part. @jaclync will make sure to test this throughout the review process.
DefaultFeatureFlagService
, returntrue
forpushNotificationsForAllStores
feature flagYou have a new order! 🎉
{{customer name}} left a review on {{product name}}
. This push notification should be in the same group as the store order notification aboveExample screenshots
Update release notes:
RELEASE-NOTES.txt
if necessary.