-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor Mark all as read in notifications #20649
Conversation
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Quality Gate passedIssues Measures |
WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20649 +/- ##
=======================================
Coverage 40.46% 40.46%
=======================================
Files 1484 1485 +1
Lines 68413 68441 +28
Branches 11307 11312 +5
=======================================
+ Hits 27683 27696 +13
- Misses 38230 38244 +14
- Partials 2500 2501 +1 ☔ 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.
Awesome work @jarvislin!
The app works as expected for me and the code changes look good 🎉
Thank you for this refactoring and for adding tests🏅
If I'm not mistaken #20247 also works as expected now. Feel free to close that too if that's the case :)
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Refactor #20066
The previous implementation used a loop to make API requests to mark each note as read, this will be like DDoS if many users are marking their notes read at the same time. The new implementation only uses one API request to mark all notes as read.
To Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):