Skip to content

Add Notifications page#5610

Open
AlexVelezLl wants to merge 15 commits intolearningequality:unstablefrom
AlexVelezLl:notifications-page
Open

Add Notifications page#5610
AlexVelezLl wants to merge 15 commits intolearningequality:unstablefrom
AlexVelezLl:notifications-page

Conversation

@AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Dec 17, 2025

Summary

  • Added Notifications page.
    • Displays this page as a StudioImmersivePage.
    • To persist the state when reloading the page, I have implemented this so that it is opened if a Modal=NOTIFICATIONS query 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.
    • Built the NotificationsList component in a way that each row corresponds to a notification type, and each notification type has its own renderer. So that we can extend this later if we want to support more notification types.
  • Ended up implementing the "red dot" by adding two new datetime fields in the User model: last_read_notification_date and newest_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 :).
  • Composables
    • Moved useFilter, useFetch, useKeywordSearch, and useQueryParams to the shared folder.
    • Modified useFilter composable to comply with the current KSelect API.
    • Added useSnackbar as a wrapper of the current Vuex Snackbar module, until this gets migrated.
    • Added useStore as sugar syntax to get the store from the currentInstance.
    • Added useCommunityLibraryUpdates composable 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

  • Check the new notifications page on the profile menu in the Appbar or by opening the main menu.
  • To create new notifications, create new Submissions.
  • If you want to see approved submissions/rejected submissions notifications, you can use the admin_communitylibrary_submission/{id}/resolve endpoint.

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 VDialog and Popper, 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 the ImmersivePage into Studio before releasing the Community Library, or if we prefer spending more time trying to fix this bug. There is now a StudioImmersivePage! (Thanks @MisRob 😄 )

@AlexVelezLl AlexVelezLl force-pushed the notifications-page branch 3 times, most recently from c7df896 to 9a59b96 Compare December 19, 2025 16:13
@AlexVelezLl AlexVelezLl requested a review from rtibbles December 19, 2025 16:13
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
UPDATE_SESSION_FROM_INDEXEDDB(state, { id, ...mods }) {
if (id === state.currentUser.id) {
UPDATE_SESSION_FROM_INDEXEDDB(state, { CURRENT_USER, ...mods }) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MisRob
Copy link
Member

MisRob commented Jan 15, 2026

Hi @AlexVelezLl, I didn't review, just noticed the note about FullscreenModal (assuming you refer to shared/views/FullscreenModal.vue). If so, would you instead please use non-Vuetify alternative StudioImmersiveModal? We built it recently as a replacement for FullscreenModal. Of course, feel free to adjust StudioImmersiveModal to this new use-case if it's needed.

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.

@MisRob
Copy link
Member

MisRob commented Jan 15, 2026

And also StudioPage + StudioOfflineAlert will be handy :) The first one takes care of consistent paddings on all screens - I hope we get to using it everywhere for these kinds of layouts in Studio.

@AlexVelezLl
Copy link
Member Author

Thanks a lot @MisRob! I did look for an alternative on Studio, but didn't find it. Will use the StudioImmersiveModal instead. Thanks!

@MisRob
Copy link
Member

MisRob commented Jan 15, 2026

Yeah the naming doesn't help - when possible I try to rename in line with Kolibri conventions :) Glad it helps.

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — the catch (error) parameter shadows the outer error ref, so error state is never written to the ref
  • notify_update_to_channel_editors() is called before super().save() for new submissions, but date_updated is auto_now=True and hasn't been set yet — the notification date will be None
  • notify_users() reads user.newest_notification_date from stale in-memory objects after .update() — the change events will push the old values
  • Approval/rejection notification components pass notification.date_created instead of notification.date to NotificationBase, showing the wrong date
  • NotificationFilters receives a lastReadFilter prop 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!

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.jscatch (error) shadows the outer error ref (see inline)
  • notify_update_to_channel_editors() called before super().save() on creation — date_updated is None for auto_now fields until saved (see inline)
  • Approval/rejection notification components pass notification.date_created instead of notification.date to NotificationBase (see inline)

Suggestions:

  • lastReadFilter prop is passed to NotificationFilters but never declared or used in that component (see inline)

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — the error ref is never set on catch
  • suggestion: Unused prop lastReadFilter passed to NotificationFilters
  • nitpick: Using array index as v-for key in notification list

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 bulk update(), so newest_notification_date in the change events may be stale. See inline comment on models.py:706.
  • suggestion (3): useFetch.js catch block shadows outer error variable; mark_notifications_read uses Greatest on an in-memory assignment instead of DB-level update; notify_update_to_channel_editors is called before super().save() so date_updated may be None for new submissions.
  • nitpick (2): Mutating input argument in getSubmissionsUpdates; minor typo in comment.
  • strings: This PR adds new i18n strings and targets the unstable branch (which is the default branch), so that's appropriate.

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 more buttons not wired up
  • nitpick (x1): getStartOfWeek locale assumption

See inline comments for details.

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_read leaves the in-memory model field as a Greatest expression after save
  • nitpick: flaggedNotification string 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

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 response
  • MainNavigationDrawer.vue:238: Drawer not closed when opening notifications modal (unlike language modal)
  • CommunityLibrarySubmissionRejection.vue:54: resolved_by_name may be null, could render literal "null" in title
  • index.vue:25,32: Redundant @click handlers on VTabs when v-model already manages selection

Nitpicks:

  • useCommunityLibraryUpdates.js:125: Redundant new Date() wrapping on values already of type Date
  • index.vue:167: Typo "precisision" → "precision" (now on line 167 after latest commit)

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: the data.value = null move changes behavior for all consumers (see inline)
  • mark_notifications_read: unused pk parameter
  • useCommunityLibraryUpdates.js: isLoadingMore state could be stale on error path
  • NotificationFilters.vue: date filter values computed once at component creation

Nitpicks:

  • CommunityLibraryStatusChip.vue: props declared after setup (non-standard order)
  • useStore.js: no guard for missing instance

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in models.py)
  • "View more" buttons render on all 3 notification types but have no click handler

Suggestions:

  • WithNotificationIndicator needs screen reader support for the red dot
  • getStartOfWeek() uses US-centric Sunday start — locale concern for international users
  • AppBar showNotificationsModal missing trackClick analytics (present in drawer version)
  • NotificationList has no fallback for unknown notification types

See inline comments for details.

Copy link

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_editors on resolve doesn't exclude the resolving admin — self-notification (inline)
  • suggestion: handleNotificationsRead relies solely on reactive chain for refresh — timeout path leaves stale data (inline)
  • nitpick: CommunityLibraryStatusChip mixes Options API with Composition API (inline)
  • nitpick: Test description says date_updated__lte but tests date_updated__gte (inline)

resolved_by=request.user,
)

submission.notify_update_to_channel_editors()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement red dot in username and sidebar to flag new notifications Implement Community Library notifications page

4 participants