Add Notifications page#5610
Conversation
c7df896 to
9a59b96
Compare
There was a problem hiding this comment.
These tests were moved to be their own test files
| grid-template-columns: repeat(auto-fit, minmax(250px, 1fr)); | ||
| gap: 6px 12px; | ||
|
|
||
| /* TODO: Open a KDS follow up to fix KTextbox feedback message alignment */ |
There was a problem hiding this comment.
Follow up: learningequality/kolibri-design-system#1187
| }, | ||
| UPDATE_SESSION_FROM_INDEXEDDB(state, { id, ...mods }) { | ||
| if (id === state.currentUser.id) { | ||
| UPDATE_SESSION_FROM_INDEXEDDB(state, { CURRENT_USER, ...mods }) { |
There was a problem hiding this comment.
This was a weird error, the key field in the session indexedDB table is CURRENT_USER and its only value could be CURRENT_USER so it doesnt make sense to look at the id and if id === state.currentUser.id for this table. Therefore, until now, this function wasn't doing anything.
|
Hi @AlexVelezLl, I didn't review, just noticed the note about If there are any other places like that, please let me know and we can chat. This tracker should be quite up-to-date so it may too help. |
|
And also |
|
Thanks a lot @MisRob! I did look for an alternative on Studio, but didn't find it. Will use the StudioImmersiveModal instead. Thanks! |
|
Yeah the naming doesn't help - when possible I try to rename in line with Kolibri conventions :) Glad it helps. |
9a59b96 to
61f2f65
Compare
61f2f65 to
4fef186
Compare
4fef186 to
eee90ba
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Great work on this notifications feature! This is a well-structured PR with clean composable architecture, good test coverage, and thoughtful design decisions (like using query params for modal persistence and the extensible notification type renderer pattern). The video demo looks polished.
There are a few issues to address before merging:
Blocking issues:
- Variable shadowing bug in
useFetch.js— thecatch (error)parameter shadows the outererrorref, so error state is never written to the ref notify_update_to_channel_editors()is called beforesuper().save()for new submissions, butdate_updatedisauto_now=Trueand hasn't been set yet — the notification date will beNonenotify_users()readsuser.newest_notification_datefrom stale in-memory objects after.update()— the change events will push the old values- Approval/rejection notification components pass
notification.date_createdinstead ofnotification.datetoNotificationBase, showing the wrong date NotificationFiltersreceives alastReadFilterprop from the parent but never declares or uses it
Visual inspection: The UI video shows proper layout, working tabs, filters, and notification list rendering. The immersive modal and responsive design look good.
CI: All checks pass.
Looking forward to the updates — this is a great addition to Studio!
contentcuration/contentcuration/frontend/shared/composables/useFetch.js
Outdated
Show resolved
Hide resolved
...end/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionApproval.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationList.vue
Outdated
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/administration/pages/Channels/ChannelTable.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid feature implementation — the notifications page, composables migration, and red dot indicator are well-architected.
CI passing. Video demo verified: layout, filters, chips, and tab switching look correct.
Blocking:
- Variable shadowing bug in
useFetch.js—catch (error)shadows the outererrorref (see inline) notify_update_to_channel_editors()called beforesuper().save()on creation —date_updatedisNoneforauto_nowfields until saved (see inline)- Approval/rejection notification components pass
notification.date_createdinstead ofnotification.datetoNotificationBase(see inline)
Suggestions:
lastReadFilterprop is passed toNotificationFiltersbut never declared or used in that component (see inline)
contentcuration/contentcuration/frontend/shared/composables/useFetch.js
Outdated
Show resolved
Hide resolved
...end/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionApproval.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid implementation of the notifications page and red dot indicator. Architecture is well-considered — the composable-based approach, extensible notification type rendering, and timestamp-based unread tracking are good design choices. CI passing. Video demo verified.
- blocking: Wrong date displayed for approval/rejection notifications (shows creation date instead of status-change date)
- blocking: Variable shadowing bug in
useFetch— theerrorref is never set on catch - suggestion: Unused prop
lastReadFilterpassed toNotificationFilters - nitpick: Using array index as
v-forkey in notification list
...end/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionApproval.vue
Outdated
Show resolved
Hide resolved
...nd/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionRejection.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/composables/useFetch.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationList.vue
Outdated
Show resolved
Hide resolved
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid implementation of the notifications page and red-dot indicator. The architecture — timestamp-based unread tracking, composable-driven filtering, per-type notification renderers — is well thought out and extensible.
CI passing. Video demo reviewed; UI looks clean with proper layout, filter behavior, and empty states.
Findings:
- blocking (1): Race condition in
notify_users— queryset is read after a bulkupdate(), sonewest_notification_datein the change events may be stale. See inline comment onmodels.py:706. - suggestion (3):
useFetch.jscatch block shadows outererrorvariable;mark_notifications_readusesGreateston an in-memory assignment instead of DB-level update;notify_update_to_channel_editorsis called beforesuper().save()sodate_updatedmay beNonefor new submissions. - nitpick (2): Mutating input argument in
getSubmissionsUpdates; minor typo in comment. - strings: This PR adds new i18n strings and targets the
unstablebranch (which is the default branch), so that's appropriate.
contentcuration/contentcuration/frontend/shared/composables/useFetch.js
Outdated
Show resolved
Hide resolved
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
eee90ba to
4431baa
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Well-structured PR that adds a solid notifications system. The composable architecture is extensible and the UI is clean.
CI passing. Video demo reviewed — layout, filters, tabs, and red dot all look correct.
Findings:
- suggestion (x3): Date filter eagerness, self-notification on submission creation,
View morebuttons not wired up - nitpick (x1):
getStartOfWeeklocale assumption
See inline comments for details.
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Outdated
Show resolved
Hide resolved
...end/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionApproval.vue
Show resolved
Hide resolved
4431baa to
acd221f
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid implementation of the notifications page and red dot indicator. The architecture — composable-based data fetching, extensible notification type renderers, and timestamp-based unread tracking — is well-considered.
CI passing. Video demo verified: notifications page layout, tabs, filters, chips, and empty states all look correct.
Findings:
- suggestion: Date filter values are computed at component setup time, not at selection time (NotificationFilters.vue)
- suggestion:
mark_notifications_readleaves the in-memory model field as aGreatestexpression after save - nitpick:
flaggedNotificationstring uses{ variable }(with spaces) inconsistent with existing{variable}pattern - nitpick:
getStartOfWeek()treats Sunday as week start (US convention) — may not match expectations for an international platform - nitpick: New strings are added targeting
unstable(the default branch), which is appropriate
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/strings/communityChannelsStrings.js
Outdated
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Outdated
Show resolved
Hide resolved
acd221f to
0b5acd8
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Good work on the notifications page. The architecture is clean: notification types map to dedicated renderer components, the composable handles data transformation well, and the Vuex session bug fix is correct.
CI: Python tests in progress. Video demo verified — layout, filters, tabs, and chips all look correct.
Suggestions:
useCommunityLibraryUpdates.js:75-76: Direct mutation of submission objects from API responseMainNavigationDrawer.vue:238: Drawer not closed when opening notifications modal (unlike language modal)CommunityLibrarySubmissionRejection.vue:54:resolved_by_namemay be null, could render literal "null" in titleindex.vue:25,32: Redundant@clickhandlers on VTabs whenv-modelalready manages selection
Nitpicks:
useCommunityLibraryUpdates.js:125: Redundantnew Date()wrapping on values already of type Dateindex.vue:167: Typo "precisision" → "precision" (now on line 167 after latest commit)
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/MainNavigationDrawer.vue
Show resolved
Hide resolved
...nd/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionRejection.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Outdated
Show resolved
Hide resolved
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Show resolved
Hide resolved
rtibblesbot
left a comment
There was a problem hiding this comment.
Thorough implementation of the notifications page and red dot indicator for community library submissions.
CI passing. Video demo verified — layout, filters, tabs, and status chips all look correct.
Suggestions:
useFetch.js: thedata.value = nullmove changes behavior for all consumers (see inline)mark_notifications_read: unusedpkparameteruseCommunityLibraryUpdates.js:isLoadingMorestate could be stale on error pathNotificationFilters.vue: date filter values computed once at component creation
Nitpicks:
CommunityLibraryStatusChip.vue:propsdeclared aftersetup(non-standard order)useStore.js: no guard for missing instance
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Outdated
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Show resolved
Hide resolved
...ration/contentcuration/frontend/shared/views/communityLibrary/CommunityLibraryStatusChip.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/index.vue
Show resolved
Hide resolved
0b5acd8 to
78c22f1
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid feature implementation. The notifications page, red dot indicator, composable migration, and backend notification infrastructure are well-structured and follow established project patterns.
CI: Python tests still in progress.
Video demo reviewed — UI looks clean: notifications list, search, filters (date/status), UNREAD/ALL tabs, and red dot indicator all function as expected.
Blocking:
Greatest(NULL, value)returns NULL on PostgreSQL — first notification for any user will silently fail (2 locations inmodels.py)- "View more" buttons render on all 3 notification types but have no click handler
Suggestions:
WithNotificationIndicatorneeds screen reader support for the red dotgetStartOfWeek()uses US-centric Sunday start — locale concern for international users- AppBar
showNotificationsModalmissingtrackClickanalytics (present in drawer version) NotificationListhas no fallback for unknown notification types
See inline comments for details.
...end/shared/views/NotificationsModal/notificationTypes/CommunityLibrarySubmissionApproval.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/WithNotificationIndicator.vue
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationFilters.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/NotificationsModal/NotificationList.vue
Outdated
Show resolved
Hide resolved
...tcuration/frontend/shared/views/NotificationsModal/composables/useCommunityLibraryUpdates.js
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/composables/useFetch.js
Outdated
Show resolved
Hide resolved
78c22f1 to
66d2a5f
Compare
66d2a5f to
a4f2b13
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Well-structured PR that implements the notifications page and red dot indicator for community library submissions.
CI passing. Video demo reviewed — UI layout, tabs, filters, and empty states look clean.
Findings:
- suggestion:
notify_update_to_channel_editorson resolve doesn't exclude the resolving admin — self-notification (inline) - suggestion:
handleNotificationsReadrelies solely on reactive chain for refresh — timeout path leaves stale data (inline) - nitpick:
CommunityLibraryStatusChipmixes Options API with Composition API (inline) - nitpick: Test description says
date_updated__ltebut testsdate_updated__gte(inline)
| resolved_by=request.user, | ||
| ) | ||
|
|
||
| submission.notify_update_to_channel_editors() |
There was a problem hiding this comment.
suggestion: When a submission is resolved by an admin, notify_update_to_channel_editors() is called here without exclude_user_id. In the model's save() method, the submission author is excluded via exclude_user_id=self.author_id, but here the resolving admin receives a notification about their own action.
Consider:
submission.notify_update_to_channel_editors(exclude_user_id=request.user.id)|
|
||
| // Refresh the notifications list after notifications read timestamp is updated | ||
| // in the vuex store so that the lastRead filter gets updated and the list refetched | ||
| await waitForLastReadUpdate(); |
There was a problem hiding this comment.
suggestion: The refresh after "Clear all" relies on the reactive chain: markNotificationsRead → IndexedDB sync → Vuex store update → lastReadNotificationDate change → queryParams change → watcher calls fetchData(). If the 5-second timeout on waitForLastReadUpdate fires before the store update arrives, resolve() is called but lastReadNotificationDate hasn't changed — so the watcher never re-triggers, leaving stale notifications visible.
Consider calling fetchData() explicitly after waitForLastReadUpdate() as a safety net, so the list always refreshes regardless of whether the store update arrived in time.
| import { communityChannelsStrings } from 'shared/strings/communityChannelsStrings'; | ||
| import { CommunityLibraryStatus } from 'shared/constants'; | ||
|
|
||
| export default { |
There was a problem hiding this comment.
nitpick: This component uses the Options API (export default {}) with a setup() function, while the other new components in this PR (Chip.vue, CommunityLibraryChip.vue, WithNotificationIndicator.vue) use <script setup>. Not a functional issue, but mixing styles within the same PR is slightly inconsistent.
| }); | ||
| }); | ||
|
|
||
| it('should take the newest date between date_updated__lte and lastRead - 1', async () => { |
There was a problem hiding this comment.
nitpick: Test description says date_updated__lte but the test actually verifies date_updated__gte vs lastRead behavior. Same issue in the companion test at line 374.
Summary
Modal=NOTIFICATIONSquery param is set in the route. Since this modal will be shown across the entire application, I chose this option rather than creating a new route for this page on all app routers.Usermodel:last_read_notification_dateandnewest_notification_date, the first representing the date of the last notification read by the user, the second the date of the most recent notification. If newest_notification_date > last_read_notification_date, the user has a new notification :).useFilter,useFetch,useKeywordSearch, anduseQueryParamsto the shared folder.useFiltercomposable to comply with the current KSelect API.useSnackbaras a wrapper of the current Vuex Snackbar module, until this gets migrated.useStoreas sugar syntax to get the store from the currentInstance.useCommunityLibraryUpdatescomposable to manage the data fetching and transformation of submissions into updates/notifications.Grabacion.de.pantalla.2025-12-17.a.la.s.10.41.42.a.m.mov
References
Closes #5457
Closes #5458
Reviewer guidance
notificationspage on the profile menu in the Appbar or by opening the main menu.admin_communitylibrary_submission/{id}/resolveendpoint.Tech debt introduced
Right now, notification filter selections don't close automatically when clicking outside the select input. This is a bug due to some weird interaction of. There is now aVDialogandPopper, I did not want to spend too much time trying to solve this because we will end up replacing Vuetify in Studio anyways, but noting that this bug will be present until then. We can coordinate later if we can prioritize migrating theImmersivePageinto Studio before releasing the Community Library, or if we prefer spending more time trying to fix this bugStudioImmersivePage! (Thanks @MisRob 😄 )