-
Notifications
You must be signed in to change notification settings - Fork 135
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
Optimize the logic of "no blaze campaign" local notification #12585
Optimize the logic of "no blaze campaign" local notification #12585
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.
|
📲 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 #12585 +/- ##
============================================
- Coverage 40.60% 40.60% -0.01%
- Complexity 5671 5676 +5
============================================
Files 1228 1228
Lines 69025 69062 +37
Branches 9562 9569 +7
============================================
+ Hits 28028 28043 +15
- Misses 38420 38438 +18
- Partials 2577 2581 +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.
Excellent job @irfano! In all my test scheduling and cancelling happened as expected. Code looks good too!
Ready to ship. (I'm approving but leaving the merge up to you in case you want to apply the suggestion)
|
||
val notification = BlazeNoCampaignReminderNotification( | ||
siteId = selectedSite.get().siteId, | ||
delay = notificationTime - Calendar.getInstance().time.time |
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.
Just a minor suggestion. No need to apply.
Wdyt of converting/rounding this milliseconds to DAYS and use DAYS
as delayUnit
for this. Makes debugging and checking the logs easier.
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 advice! However, while I’m confident it would work, it’s still an extra calculation that may result in rounding by ±1 day. I won't add it just for debugging purposes and will stay on the safe side.
…mpaign-notfication # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefs.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefsWrapper.kt
Closes: #12574
Description
The current logic is not optimal as it schedules the same notification multiple times and does not effectively handle evergreen notifications. This PR updates the logic based on the flowchart below:
Steps to reproduce
Each time a new campaign list is fetched, the logic for scheduling the reminder is triggered. I won’t provide specific testing instructions. Please test it on your own and check
Local notification canceled: blaze_no_campaign_reminder
andLocal notification scheduled: type=blaze_no_campaign_reminder, delay=2592000000MILLISECONDS
logs to verify the scheduled notifications. You don’t need to test the actual notification, as it’s outside the scope of this PR.Testing information
Please test by adding endless and limited campaigns, and removing them.
The tests that have been performed
I tested each step in the flowchart by checking logs.
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: