[Bugfix:Notifications] Misc Notification UI Bugfixes#12233
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12233 +/- ##
=========================================
Coverage 21.69% 21.69%
Complexity 9617 9617
=========================================
Files 268 268
Lines 36113 36113
Branches 478 478
=========================================
Hits 7835 7835
Misses 27804 27804
Partials 474 474
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Rkoester47
left a comment
There was a problem hiding this comment.
I tested these hotfixes and can confirm that the issue with displaying 0 older notifications is no longer present for me. I attempted to recreate the race issue you described at the bottom of the PR, but was unable to reproduce it on my end. The other features of notifications, such as mark all as seen and marking as seen within course notifications, seem to be untouched. I think the changes in the code to make it more consistent with previous Submitty code is a good choice. Overall, great work quickly addressing these bugs!
RyanStyron
left a comment
There was a problem hiding this comment.
None of the changes appear to have created unintended bugs based on testing. I appreciate the updates for consistency. Great job.
Additional note: I was able to reproduce the issue described with respect to there not being a dynamic update after clearing notifications when there are initially many notifications, but I agree that it is not urgent.
The original PR, #12233, left this refactor incomplete, preventing dynamic mark all as seen.
### Why is this Change Important & Necessary? In #12233, I made an effort to refactor the notifications system to use `term` instead of `semester`, to maintain consistency with the rest of the site. Unfortunately, I missed several files, resulting in each notification having an `undefined` term. As a result, `markAllSeen()`, which uses `term` to find the notifications to clear, was unable to update the page dynamically. ### What is the New Behavior? Convertedall instances of `semester` to `term` within the notification system. ### What steps should a reviewer take to reproduce or test the bug or new feature? **On main:** Observe that `markAllSeen` works upon reload, but does not update the page dynamically. If you want to take it a step further, you can do `console.log(n.term)` inside of the function, and observe that it is always `undefined`. **On my branch:** Observer that `markAllSeen`, and all other functionality of the notifications UI is dynamic. `n.term` is now `s26`. ### Automated Testing & Documentation There is currently no complete E2E testing of the notification system (#11908). With the new user interface now complete, this will be my next priority. ### Other information This is not a breaking change.
Why is this Change Important & Necessary?
There are several small bugs and code convention errors in the notification UI, mostly introduced by #12227. If necessary, this PR can be split into multiple smaller ones, but since all changes are minor and primarily contained in
NotificationsDisplay.vue, I figured I would just address all of them in a single PR.UI Issues
Unseen notification count displayed when 0
In the original PR, [Feature:Notifications] Home Page Additional Unseen Count #12227, I did not have a condition for when older notifications is equal to 0. This meant if the user had an unseen notification displayed, the older notifications count would always be visible, even when it was 0.
"No unseen notifications" not appearing dynamically when course notifications are cleared
If the user marks an individual course notification as seen and then clears all of them, the "No unseen notifications" text would not appear until after a reload. This is because the course notification's prop for unseenCount is set to 0, meaning it becomes negative if notifications are marked seen, and thus preventing the text from displaying.
Developer Convention Issues
NotificationsDisplay.vuecontains 3 notification lists:localNotifications- all notifications sent from the backendfilteredNotifications- filtered version oflocalNotificationsthat is either unseen only, or all based on local storage variablevisibleNotifications- filtered version offilteredNotifications- all for courses, 10 most recent for home page.When referring to displayed notifications, you should use
visibleNotifications. When manipulating the data in notifications (i.e., mark seen), you should uselocalNotifications, since the updates will propogate to the other arrays from there.The logic introduced in #12227 uses
filteredNotifications. I don't think this affects the functionality, but it is still not the correct convention.Inconsistent Logic
The individual mark seen logic does not match the mark all as seen logic.
Using
semesterinstead oftermSubmitty uses term, not semester. In [Feature:Notifications] Display All Notifications on Home Page #11914, the first PR for the new notifications UI, I used semester. In more recent PRs such as [Feature:Notifications] Home Page Mark All Seen Popup #12219, I used the correct Submitty convention, term, leading to code inconsistencies.
What is the New Behavior?
Added a condition to ensure the count is not displayed when it is 0.
Before
After
"No unseen notifications" not appearing dynamically when course notifications are cleared
Rather than checking if
unseenCountis equal to 0, check ifunseenCountis <= 0.Using Incorrect Notification Arrays
Use
visibleNotificationsfor the unseen count logic instead offilteredNotifications. Move arrays to be next to each other and adjust comments to make it more apparent which one should be used.Inconsistent Logic
Use a for loop for both individual mark seen and mark all as seen.
Using
semesterinstead oftermChanged all instances of
semesterin the notification UI toterm.What steps should a reviewer take to reproduce or test the bug or new feature?
Play around with the notifications UI and ensure there are no other odd behaviors.
Other information
This is not a breaking change.
Minor Bug Discovery
In my testing, I discovered a possible race condition with clearing individual notifications. When the user has many notifications and attempts to rapidly clear them one at a time, it is possible for a notification to appear as seen, but persist as the 11th notification in the unseen only panel.
This seems to occur when there is a high volume of mark_seen network requests, causing them to return out of order. The solution would be to block each individual mark seen button until each request is completed, but I believe that would be too much overhead for such a minor issue.
This should not occur often, as users with many notifications should clear them using
Mark as seen. Additionally, the issue can be solved with a simple reload, and does not occur if the user is using the notification UI as intended / not individually clearing 10+ notifications at a time.