[Bugfix:Notifications] Completed Semester => Term Refactor#12337
[Bugfix:Notifications] Completed Semester => Term Refactor#12337
Conversation
The original PR, #12233, left this refactor incomplete, preventing dynamic mark all as seen.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12337 +/- ##
=========================================
Coverage 21.69% 21.69%
Complexity 9617 9617
=========================================
Files 268 268
Lines 36115 36115
Branches 478 478
=========================================
Hits 7836 7836
Misses 27805 27805
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.
The changes made by this branch appear to be successful. I tested in Firefox, and was able to reproduce the results described in the PR. On main, the act of 'Mark as Seen' does not refresh the page dynamically, and requires a manual refresh. However, on this branch, I was able to Mark as Seen and see that the page automatically refreshes, showing the notifications cleared as intended.
Good work!
|
The changes appear to be successful for automatically updating notifications. On the original branch the notification list was not updated until the website was reloaded. On this PR's branch the notifications were removed as soon as 'Mark as Seen' was pressed. Looking at the code, there are a few variables named 'term' remaining. Do any of these need to be changed as well? |
I'm assuming you mean |
Why is this Change Important & Necessary?
In #12233, I made an effort to refactor the notifications system to use
terminstead ofsemester, to maintain consistency with the rest of the site. Unfortunately, I missed several files, resulting in each notification having anundefinedterm. As a result,markAllSeen(), which usestermto find the notifications to clear, was unable to update the page dynamically.What is the New Behavior?
Convertedall instances of
semestertotermwithin the notification system.What steps should a reviewer take to reproduce or test the bug or new feature?
On main:
Observe that
markAllSeenworks upon reload, but does not update the page dynamically. If you want to take it a step further, you can doconsole.log(n.term)inside of the function, and observe that it is alwaysundefined.On my branch:
Observer that
markAllSeen, and all other functionality of the notifications UI is dynamic.n.termis nows26.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.