Skip to content

feat(notifications): open the exact page for every notification - #233

Closed
Clupai8o0 wants to merge 16 commits into
ui/a11y-auditfrom
ui/notification-links
Closed

Clupai8o0 wants to merge 16 commits into
ui/a11y-auditfrom
ui/notification-links

Conversation

@Clupai8o0

Copy link
Copy Markdown

What this does

Adds a single resolver that decides where a notification should open, for both students and staff, and wires the notification bell and the full notifications page to call it instead of following the notification's raw link directly.

Stacked PR

The diff shown here is only the range ui/a11y-audit..ui/notification-links (57 files, +3,618/-1,236), not the whole branch.

The stack is a DAG, not a chain. Checked with git merge-base --is-ancestor:

  • ui/a11y-audit is an ancestor of this branch. Do not merge this before it lands.
  • ui/panel-layout is also an ancestor of this branch. The merge base is f7b72aa1f, which is that branch's own tip.
  • ui/panel-layout is not an ancestor of ui/a11y-audit. They are siblings forked from cc4a834e6.
  • ui/notification-links is an ancestor of ui/inbox-extension-card, which merges it at 78902d4f7.

This branch was not cut plainly off ui/a11y-audit. Its own history carries 7972ea62a "Merge branch 'ui/panel-layout' into ui/inbox-extension-card" and 3c19fb377 "Merge branch 'ui/a11y-audit' into ui/inbox-extension-card", so it sits on the inbox-extension-card integration line after both siblings were already merged there. That is why the shared panel layout work shows up in this range.

Merge order that follows from the graph: theme/status-and-globals first. Then ui/panel-layout and ui/a11y-audit in either order, since neither is an ancestor of the other. One caveat on that pair. Both already contain cc4a834e6 ("Restyle extension request card and add full-screen comments to the task inbox"), which is not on theme/status-and-globals, so that inbox-extension-card commit lands with whichever of the two merges first. Then this branch. Then ui/inbox-extension-card.

What changed and why

Notification routing (the point of this branch)

  • notification-target.ts is a new pure function, notificationTarget(), that reads a notification's event, notificationType, link and four of the ids now sent alongside link (unitId, projectId, studentId, taskDefinitionAbbr) and decides one of four outcomes: a route to navigate to, the old raw link (for an api that hasn't been updated to send ids), "unavailable" (the thing it pointed at is gone), or "none" (nothing to open).
  • Which side it routes to is decided by comparing the notification's studentId with the signed in user's id, not by that user's account role. A tutor who is also enrolled as a student elsewhere still lands on the right side depending on whose project the notification is about.
  • Students go to the task, its comments panel (for a comment, a discussion request, or an assessed extension), the groups page, the tutorials page, or the portfolio view, depending on the event. Staff go to the task inbox for that student and task, the staff portfolio view for a submission event, or the unit's student list when there is no task abbreviation and it is not a portfolio event. The staff inbox route sets students: 'all' in the query params so a unit convenor who doesn't tutor that student isn't filtered out of the list before the row can be selected.
  • notification-open.service.ts is the piece that needs the running app. It calls notificationTarget(), then for a staff destination checks the signed-in user still holds a role in that unit before navigating (Admin and Auditor skip that check, and that role does come off the account), waiting for unit roles to finish loading first since they load once after sign-in. If the role check fails, or the navigation itself fails or gets refused, it shows a snackbar through AlertService.error and leaves the reader where they were, instead of a blank page or /unauthorised.
  • notification-bell.component.ts and notifications-page.component.ts drop their direct NotificationRouteService.navigate(notification.link) call in favour of NotificationOpenService.open(notification). Mark-as-read behaviour itself is unchanged. One wiring change worth naming: both used to call navigate only inside if (notification.link), and both now call open() on every click, because whether there is anywhere to go is the opener's decision. Both specs were updated so a row with no link still expects one open call.
  • notification-route.service.ts gets a new navigateToTarget() next to the existing navigate(). It's needed because navigate() screens its target through an allow-list built for the api's own link shapes, and the staff inbox/portfolio routes built from ids don't match that allow-list.
  • notification.ts and notification.service.ts add and map eight id fields: unitId, projectId, studentId, taskDefinitionId, taskDefinitionAbbr, taskId, commentId, groupId. The resolver reads four of them. The other four are mapped and not yet used. They are deliberately left without defaults so an older api sending nothing stays distinguishable from a newer one sending null.
  • A new event, portfolio_submitted, gets its own icon and label, separate from the student-facing portfolio_received, so the staff side of a submission reads as a different notification in the list.
  • The bell's dropdown gets xPosition="before" and yPosition="below" at every width. Under 600px a media rule also sets transform-origin: top right, a max-width and a width of calc(100vw - 16px), so the panel grows from under the bell instead of animating in from where it would have sat before the overlay pushed it back on screen.

Shared panel layout (already in this branch's history, extended here)

  • app-panel-layout / app-panel is a shared component pair for a page built from collapsible, resizable, full-screen-able columns. PanelStateService remembers each panel's collapsed state and width per page in localStorage, with every read and write wrapped in a try/catch since a private window or a full quota can refuse silently.
  • The staff task inbox drops its own hand-rolled CDK-drag resize logic (a manual drag Subject stream with auditTime, hand-clamped widths, the CdkDragMove imports) for this shared layout, and the student project dashboard moves onto it too. The split-pane-resizing body class is not gone. It moved into panel.component.ts, which adds and removes it around a drag, and styles.scss still carries its rules.
  • This branch adds app-panel-collapse-button, which finds its parent panel and renders into that panel's own control row. It is placed in four templates: beside Refresh tasks in the staff task list header, beside the filters button in the shared task list, and in the tab rows of the staff inbox dashboard and the student task dashboard. The floating edge toggle it replaces is removed outright, not just repositioned. panel.component.html no longer renders .app-panel__edge-toggle, and the panel layout spec asserts it is absent.
  • Panels keep a minimum width through dragging, keyboard resizing, and a restored remembered width, all through one clamp(). When the page can't fit every expanded panel the layout rails the first collapsible panels in order, the list and then comments, and never writes that over the user's remembered choice.

Task list and task dashboard styling

  • The status filter in f-unit-task-list is now a themed mat-select showing each status's own colour and icon beside its label, with "All statuses" first, in place of the plain browser <select>. The filter value, route binding and aria-label are unchanged. The old <label> wrapper and its visible "Task status" text were dropped with the native select, so the control is now named by its aria-label alone. This component is used by both the student project dashboard and the staff task viewer, and the filter row is not gated on mode, so the picker changes in both.
  • A follow-up commit, titled to keep the selected sort option readable in dark mode, swaps the selected sort option from a hardcoded text-formatif-blue to the themed text-ot-link. It also swaps a separate reset button from text-formatif-blue disabled:text-gray-400 to text-ot-link disabled:text-ot-muted.
  • The task status card's three action buttons get icons and a 44px minimum height. The task details card's buttons already had icons and gain the 44px height plus wrapping. The details card measures its own width with a CSS container query and, under 440px, switches its start/due/feedback timeline from one column per date to a single vertical line and gives each action button a full-width row.
  • src/styles.scss restyles every outlined button in the app: a token outline over a faint primary wash, primary-coloured icons, a token radius, and no shadow in place of the old grey fill and drop shadow.

Smaller, unrelated fixes carried on this branch

  • The startup splash screen replaces a looping 70vw Lottie animation with a small themed logo mark, a glow, and a loading bar, and drops the ngx-lottie options from the component.
  • The signed-out pages drop a 1rem gutter that had been framing the content card in its own background colour on top of the card's own padding. The safe-area insets stay.
  • The Unit Hub page adds day-grouped "Today" / "Tomorrow" sections for upcoming sessions, loading skeletons, and a menu for starting an announcement or a session.

Deliberately not changed

  • The status picker swap keeps the filter value, the route binding and the accessible name. The commit says so. The one visible change beyond the control itself is the dropped "Task status" label text described above.
  • The panel layout's auto-rail behaviour (railing the list, then comments, when the page is too narrow for every expanded panel) doesn't touch what's remembered for the user. Their collapsed/expanded choice and saved widths stay in localStorage exactly as they set them. The layout component's own comment says the same, and a spec covers it.
  • A notification from an older api that sends no ids at all still follows its raw link, unchanged, and nothing had to be backfilled for those. That is the only fallback to the old path. Once the api does send ids, a notification whose projectId is null but which has a link is reported as unavailable, and one with no link at all opens nothing.

Testing

No test suite was run for this PR. What follows is the spec coverage that exists in the diff, counted from it.

  • notification-target.spec.ts (new): 8 it blocks plus three it.each tables of 7, 3 and 5 rows, 23 cases in all. They cover the student task and comments-pane events, the groups, tutorials and student portfolio routes, the five staff inbox events, the staff portfolio view, the staff fallback to the unit's student list, the deleted-task and deleted-project "unavailable" outcomes, the "never had a page" case and the older-api link fallback.
  • notification-open.service.spec.ts (new): 10 cases, covering the student comments destination, the staff inbox destination, waiting for unit roles to load, the staff role check and the admin bypass, the deleted-target snackbar, navigation refused and navigation failed, the older-api link fallback, and the nothing-to-open case.
  • notification-presentation.spec.ts: 1 new case, for the portfolio_submitted icon, label and tone.
  • panel-layout.component.spec.ts (new): 13 cases for the shared panel layout, covering collapse and expand, the in-content collapse button, the absence of the edge toggle, stacked tabs, resize clamping, a restored width out of range, auto-railing without touching storage, storage keys, storage that throws, stored values of the wrong shape, and full screen with Esc.
  • unit-task-list.component.spec.ts: 1 new case asserting no <select> remains, that the mat-options carry the right label and status-icon, and that selection calls setStatusFilter.
  • notification-bell.component.spec.ts and notifications-page.component.spec.ts: existing click-to-open tests rewritten against NotificationOpenService in place of NotificationRouteService, including the no-link row, which now expects open to be called.
  • project-dashboard.component.spec.ts and project-dashboard.mobile.spec.ts: existing tests rewritten for fullscreenPanel and app-panel-layout markup, replacing the old commentsFullscreen flag, the .comments-sidebar selector and the comments-breakpoint stub. Two assertions went with that rewrite and are not replaced: the toggle half of the full-screen test, and the "keeps a full-screen chat open when the window narrows" case.

Review guidance

Given the size, the actual new decision-making is small.

  • notification-target.ts (151 lines) and notification-open.service.ts (91 lines) carry all the new logic. Both are short and side-effect-light, and the two new spec files exercise them, so a logic read plus a skim of the tests should cover them.
  • panel.component.ts (287 lines) and panel-layout.component.ts (264 lines) are the largest new source files and carry the min-width, resize and auto-rail math. panel-layout.component.spec.ts is larger still at 290 lines. Worth a closer look, though as the Stacked PR note says, most of this arrived through the ui/panel-layout merge rather than being written on this branch. The commits written here are 7f9966acd and 8763d0934.
  • The notification.ts model and notification.service.ts field list additions are plain mapping changes, no logic, safe to skim. Four of the eight new fields have no reader yet.
  • Everything else, the app-wide outlined-button CSS, the status-picker markup, the dark-mode colour-token fix, the bell menu position fix and the wiring changes in the bell and notifications-page components, is markup or styling with the existing specs updated alongside.

Merge order, and what this diff actually contains

Read this before judging the size. The branch history is a diamond, not a line.

theme/status-and-globals (open as #192) forks at cc4a834e6 into ui/panel-layout and ui/a11y-audit. Those two rejoin at merge commits 7972ea62a and 3c19fb377, and this branch is cut after both. So the diff GitHub shows against ui/a11y-audit also contains the whole of ui/panel-layout, because ui/a11y-audit does not contain it.

  • Shown against ui/a11y-audit: 57 files, +3,618 / -1,236.
  • Of that, ui/panel-layout accounts for 21 files, +1,521 / -653, and is reviewed in its own PR.
  • Work unique to this slice, measured from the merge point 7972ea62a: 61 files, +1,564 / -227.

Merge order: #192, then the panel layout and accessibility PRs in either order, then this one, then the submission and Unit Hub PR last.

…widths

Replace the floating edge toggle with app-panel-collapse-button, which finds
its parent panel and sits in the column's existing control row: beside refresh
in the staff task list, beside the filter button in the student task list, and
in the centre tab bars. Panels now hold a minimum width through drag, keyboard
resize and restored widths, and the layout shows the list, then comments, as a
rail when the page cannot fit every expanded panel, without changing what is
remembered.
Swap the native status select for a mat-select that shows each status's
circle and colour beside its label, with All statuses first. The filter value,
route binding and accessible label are unchanged.
…ow column

Outlined buttons drop the grey fill and drop shadow for a token outline, a faint
primary wash and a primary icon, in both themes, using the current Material
button tokens. The task details card and status card buttons get icons, a 44px
touch height, and wrap to full-width rows on a narrow card, and the key dates
run down the card when there is no room for one column per date.
…students and staff

One resolver decides the target from the event and whether the notification
is about the viewer's own project. Staff go to the task inbox or the staff
portfolio view, students to the task, its comments, groups, tutorials or
portfolio. A deleted target or a unit the viewer no longer has a role in shows
a snackbar and stays put. An api that sends no ids still follows the link.
@Clupai8o0

Copy link
Copy Markdown
Author

Closing: this work is now on integration/t2-2026 (web 8fcf4135a), together with #224, the rest of the UI stack and feature/ui-polish. PR intake has closed for the trimester, so the integration branch is the base for the demo and for the upstream PRs to thoth-tech. The full web suite (2235 tests), lint and build pass on it.

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.

1 participant