diff --git a/docs/service-worker.md b/docs/service-worker.md new file mode 100644 index 0000000000..818a6c5579 --- /dev/null +++ b/docs/service-worker.md @@ -0,0 +1,147 @@ +# The service worker in development + +Push notifications are received by a service worker. Until MN-F03, the service +worker only ran in production builds, so **no push could ever arrive while +developing** and nobody could test any push work. + +This describes what changed, what it costs, and how to get out of trouble. + +## What changed + +Two halves. Doing only one of them leaves the app worse than before. + +1. **`angular.json`** — the `development` build configuration now sets + `"serviceWorker": "ngsw-config.json"`. Before this it was set only on + `production`, so a development build never generated `ngsw-worker.js` at all. +2. **`doubtfire-angular.module.ts`** — `ServiceWorkerModule.register` now reads + `environment.production || environment.enableServiceWorker` instead of + `environment.production`. + +Order matters. Flipping the flag without the first half leaves the app asking the +server for a file that does not exist. That fails quietly and looks exactly like +push being broken. + +## Turning it off + +`src/environments/environment.ts`: + +```ts +enableServiceWorker: false, +``` + +Then reload with the worker cleared (below). Production is unaffected either way, +because `production: true` already enables it. + +## Which build configuration is actually in use + +`package.json` runs `ng serve --configuration $NODE_ENV`, so **the configuration +depends on an environment variable and is not necessarily `development`.** +"I added it to `development`" does not mean `development` is the one running. + +Check what `$NODE_ENV` is: + + docker exec doubtfire-web printenv NODE_ENV + +In the Docker stack it is `docker`. That is a **serve** configuration in +`angular.json`, and it maps to the `development` **build** configuration: + +```json +"docker": { "buildTarget": "doubtfire:build:development" } +``` + +So the change does apply in Docker. If you add another serve configuration, point +it at a build configuration that has `serviceWorker` set, or push silently stops +working for anyone using it. + +## Registration is delayed six seconds + +`doubtfire-angular.module.ts` sets: + +```ts +registrationStrategy: () => interval(6000).pipe(take(1)), +``` + +The worker registers **six seconds after bootstrap**, not at bootstrap. Anything +that asks for the service worker during app init finds nothing there. Wait on +`navigator.serviceWorker.ready` rather than assuming it exists — MN-C01 depends +on this. + +## What it costs + +**Measured** on the Docker stack, Angular 22, `ng serve`: + +- `ngsw-worker.js` and `ngsw.json` are served (they were 404 before). The dev + server generates them, so no separate `ng build` step is needed. +- `ngsw.json` lists 158 hashed files, and the `app` asset group prefetches + `/index.html`, `/main.js`, `/styles.css`, `/polyfills.js` and `/scripts.js`. + **The whole app bundle is cached.** +- A source change does regenerate the manifest: after editing a file under `src/` + the `/main.js` hash in `ngsw.json` changed, so the worker can see there is an + update. +- API calls are not cached. The `api` data group in `ngsw-config.json` uses + `"strategy": "freshness"` with `maxAge: 0u` and `maxSize: 0`. + +**Confirmed in a browser** on 2026-08-02: the worker registers, and a push sent +from the api arrives as a desktop notification. So the two halves above are +enough to make push work in development. + +What still has not been measured is how live reload behaves once the worker is +serving from its cache over a long session. Angular's worker normally picks up a +new version on a later page load rather than the current one, which would mean +**after saving a file you reload and still see the old code**. Treat that as +expected until somebody hits it. + +The cost to watch for is stale app code, not stale data — API responses are not +cached. If something you just changed is not showing up, clear the worker before +assuming the change is wrong. + +## Push subscription rotation + +Angular 22's worker handles the browser's `pushsubscriptionchange` event and +forwards it through `SwPush.pushSubscriptionChanges`. The app starts that +listener with its other push lifecycle services. When the browser supplies a +replacement, the app POSTs it to `/api/push_subscriptions` first and removes the +old endpoint only after the replacement is stored. A keys-only rotation keeps +the same endpoint and updates the existing api row in place. + +The focused service tests simulate this event because browsers do not provide a +reliable way to force a real rotation. They verify the POST-before-DELETE order, +keys-only updates, teardown, and that one failed api request does not stop later +rotations. A change event with no replacement cannot be repaired automatically: +creating a fresh subscription may require a user gesture, so the user must use +the existing opt-in control again; the api removes the dead row after a failed +delivery. + +## Clearing a stuck service worker + +Fastest, in the browser console: + +```js +(await navigator.serviceWorker.getRegistrations()).forEach((r) => r.unregister()); +const keys = await caches.keys(); +await Promise.all(keys.map((k) => caches.delete(k))); +location.reload(); +``` + +Through dev tools instead: + +1. Application → Service Workers → **Unregister**. +2. Application → Storage → **Clear site data**. +3. Reload. + +While actively working on the app, Application → Service Workers → **Bypass for +network** stops the worker serving cached responses without unregistering it. A +normal hard reload is not enough on its own, because the worker still intercepts. + +## Checking it is working + + curl -s -o /dev/null -w "%{http_code}\n" http://localhost:4200/ngsw-worker.js + +200 is what you want. 404 means the build configuration in use has no +`serviceWorker` entry — see "Which build configuration is actually in use". + +In the browser: dev tools → Application → Service Workers. It should be +registered and activated roughly six seconds after the page loads. + +Then follow `doubtfire-api/docs/notifications/push-setup.md` to register the +browser and send a push. diff --git a/ngsw-config.json b/ngsw-config.json index c1e00b6bf7..0b77d11053 100644 --- a/ngsw-config.json +++ b/ngsw-config.json @@ -35,10 +35,7 @@ "installMode": "lazy", "updateMode": "prefetch", "resources": { - "files": [ - "/assets/**", - "/*.(eot|svg|cur|jpg|png|png?default=blank&size=25webp|gif|otf|ttf|woff|woff2|ani)" - ] + "files": ["/assets/**", "/*.(eot|svg|cur|jpg|png|webp|gif|otf|ttf|woff|woff2|ani)"] } }, { @@ -60,6 +57,8 @@ "!/beta/**", "!/beta", "!/legacy", - "!/legacy/**" + "!/legacy/**", + "!/api", + "!/api/**" ] } diff --git a/src/app/api/models/notification.ts b/src/app/api/models/notification.ts new file mode 100644 index 0000000000..0fe9ede99d --- /dev/null +++ b/src/app/api/models/notification.ts @@ -0,0 +1,67 @@ +import {Entity} from 'ngx-entity-service'; + +/** + * Something that happened which the signed in user should be told about. + * + * `notificationType` is the category the user's preferences switch on, one of + * task, feedback, portfolio, extension, general or unit_hub. The api gates + * delivery on task, feedback, portfolio and unit_hub against the + * receive_*_notifications columns, so the web app never has to check a + * preference before rendering one of these. + * + * `event` is the stable, specific hook (for example `task_comment_created`). + * Presentation can vary by event without inferring meaning from message text. + */ +export class Notification extends Entity { + id: number; + notificationType: string; + event: string; + message: string; + + /** + * Where to send the user when they click this, or null when there is nowhere + * to go. Treat it as a route within the app, not an absolute url. + * + * The column is nullable on the api, so guard it before calling anything on + * it. The union is documentation for now, the repo builds with strict off. + */ + link: string | null; + + /** + * The ids of the page this is about, sent beside link by newer apis. + * + * Left undefined when the api did not send them at all, which is how an older + * api is told apart from a newer one saying the record is gone (null). Do not + * give these a default, or that difference is lost. + */ + unitId?: number | null; + projectId?: number | null; + studentId?: number | null; + taskDefinitionId?: number | null; + taskDefinitionAbbr?: string | null; + taskId?: number | null; + commentId?: number | null; + groupId?: number | null; + + /** + * The Unit Hub announcement or session a unit_hub notification is about. + * Null once it has been deleted, undefined from an api that predates them. + */ + announcementId?: number | null; + sessionId?: number | null; + + /** + * When the user read this, or null while it is still unread. + */ + readAt: Date | null; + + /** + * Never null. The api column is NOT NULL, which is why this one can go + * through the plain date mapping and readAt cannot. + */ + createdAt: Date; + + public get isRead(): boolean { + return this.readAt != null; + } +} diff --git a/src/app/api/services/notification-route.service.ts b/src/app/api/services/notification-route.service.ts new file mode 100644 index 0000000000..8af00ad2aa --- /dev/null +++ b/src/app/api/services/notification-route.service.ts @@ -0,0 +1,153 @@ +// MN-C03: one security boundary for every notification destination. +import {Injectable} from '@angular/core'; +import {Router} from '@angular/router'; +import {AuthReturnUrlService} from 'src/app/security/auth-return-url.service'; +import {AuthenticationService} from './authentication.service'; +import { + NotificationFeedbackRouteIntent, + NotificationFeedbackRouteIntentService, +} from './notification-feedback-route-intent.service'; + +export const NOTIFICATION_ROUTE_FALLBACK = '/notifications'; + +const MAX_NOTIFICATION_ROUTE_LENGTH = 256; +const CONTROL_CHARACTER_MAX = 0x1f; +const DELETE_CHARACTER = 0x7f; +const FORBIDDEN_ROUTE_TEXT = /[\s\\?#%]/; +const PROJECT_ROOT_ROUTE = /^\/projects\/[1-9]\d*\/(?:dashboard|groups)$/; +const PROJECT_TASK_ROUTE = + /^\/projects\/[1-9]\d*\/dashboard\/[A-Za-z0-9][A-Za-z0-9._-]{0,31}(?:\/feedback)?$/; +// The one destination allowed a query string, and only as two numeric ids. +const UNIT_HUB_ROUTE = /^\/unit-hub\?unit=[1-9]\d{0,9}&(?:announcement|session)=[1-9]\d{0,9}$/; +const PROJECT_FEEDBACK_ROUTE = + /^\/projects\/([1-9]\d*)\/dashboard\/([A-Za-z0-9][A-Za-z0-9._-]{0,31})\/feedback$/; + +function hasControlCharacters(value: string): boolean { + return Array.from(value).some((character) => { + const characterCode = character.charCodeAt(0); + return characterCode <= CONTROL_CHARACTER_MAX || characterCode === DELETE_CHARACTER; + }); +} + +@Injectable({providedIn: 'root'}) +export class NotificationRouteService { + constructor( + private router: Router, + private authentication: AuthenticationService, + private authReturnUrl: AuthReturnUrlService, + private feedbackIntents?: NotificationFeedbackRouteIntentService, + ) {} + + public resolve(link: unknown): string { + if (typeof link !== 'string') { + return NOTIFICATION_ROUTE_FALLBACK; + } + if (link.length === 0 || link.length > MAX_NOTIFICATION_ROUTE_LENGTH) { + return NOTIFICATION_ROUTE_FALLBACK; + } + if (link !== link.trim()) { + return NOTIFICATION_ROUTE_FALLBACK; + } + if (!link.startsWith('/') || link.startsWith('//')) { + return NOTIFICATION_ROUTE_FALLBACK; + } + if (UNIT_HUB_ROUTE.test(link)) { + return link; + } + if (hasControlCharacters(link) || FORBIDDEN_ROUTE_TEXT.test(link)) { + return NOTIFICATION_ROUTE_FALLBACK; + } + + if (link === NOTIFICATION_ROUTE_FALLBACK || PROJECT_ROOT_ROUTE.test(link)) { + return link; + } + + if (!PROJECT_TASK_ROUTE.test(link)) { + return NOTIFICATION_ROUTE_FALLBACK; + } + + return link; + } + + public navigate(link: unknown): Promise { + return this.navigateToTarget(this.resolve(link)); + } + + /** + * Go to an in-app url that was built here from ids, not taken from a link. + * + * NotificationTargetService builds these with the router from numeric ids and + * a task abbreviation the router encodes, so there is no raw text to screen + * and the allow-list above, which only knows the api's link shapes, would + * turn away the staff pages it needs. + */ + public navigateToTarget(target: string): Promise { + const feedbackIntent = this.createFeedbackIntent(target); + + // A service-worker click can reach an already-open anonymous client after + // the one-off startup authentication check has finished. Save the already + // allow-listed notification target and enter the normal sign-in flow now, + // rather than waiting for a protected request to fail. + if (!this.authentication.isAuthenticated()) { + this.authReturnUrl.remember(target); + if (this.currentPath() === '/sign_in') { + return Promise.resolve(true); + } + return this.finishNavigation(this.router.navigateByUrl('/sign_in'), feedbackIntent); + } + + if (this.currentPath() === target) { + return Promise.resolve(true); + } + return this.finishNavigation(this.router.navigateByUrl(target), feedbackIntent); + } + + private createFeedbackIntent(target: string): NotificationFeedbackRouteIntent | null { + if (!this.feedbackIntents) { + return null; + } + + // A later notification click supersedes any earlier route intent, including + // one waiting behind sign-in. This prevents an old task from revealing when + // a different destination eventually resolves. + this.feedbackIntents.clear(); + + const match = target.match(PROJECT_FEEDBACK_ROUTE); + if (!match) { + return null; + } + + return this.feedbackIntents.request({ + projectId: Number(match[1]), + taskAbbreviation: match[2], + }); + } + + private async finishNavigation( + navigation: Promise, + feedbackIntent: NotificationFeedbackRouteIntent | null, + ): Promise { + try { + const navigated = await navigation; + if (!navigated && feedbackIntent) { + this.feedbackIntents?.cancel(feedbackIntent); + } + return navigated; + } catch (error) { + if (feedbackIntent) { + this.feedbackIntents?.cancel(feedbackIntent); + } + throw error; + } + } + + private currentPath(): string { + const routerUrl = typeof this.router.url === 'string' ? this.router.url : '/'; + const withoutFragment = routerUrl.split('#', 1)[0]; + const withoutQuery = withoutFragment.split('?', 1)[0]; + if (!withoutQuery) { + return '/'; + } + return withoutQuery.length > 1 ? withoutQuery.replace(/\/+$/, '') : withoutQuery; + } +} diff --git a/src/app/api/services/notification.service.ts b/src/app/api/services/notification.service.ts new file mode 100644 index 0000000000..de9a6092e6 --- /dev/null +++ b/src/app/api/services/notification.service.ts @@ -0,0 +1,365 @@ +import {CachedEntityService, RequestOptions} from 'ngx-entity-service'; +import {HttpClient} from '@angular/common/http'; +import {Injectable} from '@angular/core'; +import {BehaviorSubject, Observable, Subject, map, takeUntil, tap} from 'rxjs'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {Notification} from '../models/notification'; +import {MappingFunctions} from './mapping-fn'; + +/** + * Reads and updates the signed in user's notifications. + * + * Three of the five endpoints are ordinary entity work and go through + * CachedEntityService. The other two answer `{count: n}` and `{success: true}`, + * which are not entities and would not survive the mapping, so those two use + * HttpClient directly. Each one says so at its own definition. + * + * Provided in root rather than declared in doubtfire-angular.module.ts. Every + * other entity service is registered in that module, but IN-01 to IN-05 are + * five separate pull requests into the same feature branch and would otherwise + * all edit the same providers array. PushNotificationService, already merged on + * this branch, sets the same precedent. Being a root singleton is also why + * reset() exists, see the note there. + */ +@Injectable({providedIn: 'root'}) +export class NotificationService extends CachedEntityService { + protected readonly endpointFormat = 'notifications/:id:'; + private readonly markReadEndpointFormat = 'notifications/:id:/read'; + + private readonly unreadCountSubject: BehaviorSubject = new BehaviorSubject(0); + + /** + * Fires once per sign out, from reset(). + * + * Every request this service returns ends with takeUntil on it, so this + * unsubscribes the lot. Unsubscribing aborts the underlying request, which is + * the part that matters: nothing in ngx-entity-service subscribes internally, + * so the cache writes all live in taps on the chain and an aborted request + * never reaches them. Without this, one still in flight at sign out would + * land afterwards and put the previous user's rows back into a cache that is + * shared by the whole app. + */ + private readonly sessionEnded: Subject = new Subject(); + + /** + * Which unread count request is which. + * + * Every mutation asks for the count again, so several are often in the air at + * once, and nothing makes them answer in the order they were sent. The older + * answer is the wrong one, it was computed before the newer mutation applied. + * Each request takes the next number on the way out and only a number higher + * than the last one applied is allowed to move the subject. + */ + private issuedUnreadCountRequests = 0; + private appliedUnreadCountRequest = 0; + + constructor(private apiHttpClient: HttpClient) { + super(apiHttpClient, API_URL); + + this.mapping.addKeys( + 'id', + 'notificationType', + 'event', + 'message', + 'link', + 'unitId', + 'projectId', + 'studentId', + 'taskDefinitionId', + 'taskDefinitionAbbr', + 'taskId', + 'commentId', + 'groupId', + { + // Not MappingFunctions.mapDate. read_at is null on every unread + // notification and new Date(null) is the epoch, not null, so mapDate + // would report every unread notification as read. + keys: 'readAt', + toEntityFn: (data: object, key: string) => (data[key] ? new Date(data[key]) : null), + }, + { + keys: 'createdAt', + toEntityFn: MappingFunctions.mapDate, + }, + ); + + // No mapAllKeysToJsonExcept on purpose. Not one of the five endpoints reads + // a request body, so this entity is never serialised on the way out. + } + + public createInstanceFrom(_json: object): Notification { + return new Notification(); + } + + /** + * How many unread notifications the signed in user has. + * + * A subject and not a request, so the bell subscribes once and every later + * mark read or delete moves the number without another round trip. It starts + * at zero and stays there until refreshUnreadCount() is called, so call that + * on sign in and on navigation rather than trusting the seed. + */ + public get unreadCount$(): Observable { + return this.unreadCountSubject.asObservable(); + } + + /** + * Every notification for the signed in user, newest first. + * + * fetchAll and not query, because query answers straight from the cache once + * it has run, and a notification list that never changes after first paint is + * worse than useless. fetchAll always goes to the server. + * + * The cache is passed explicitly and that is not optional. buildInstance only + * reuses an entity already in the cache when options.cache is set, and + * fetchAll does not set it the way query does. Without this line every call + * would build a fresh set of objects, so a component holding the result of an + * earlier call would stop seeing later updates to the same rows. + * + * There is no paging. GET /notifications returns the lot in one response, it + * takes unread_only and nothing else, so page client side over what comes + * back. If real paging is wanted it is an api change, not a change here. + */ + public list(unreadOnly = false): Observable { + const options: RequestOptions = {cache: this.cache}; + + if (unreadOnly) { + options.params = {unread_only: true}; + } + + return this.fetchAll(undefined, options).pipe( + tap((notifications) => { + if (!unreadOnly) { + this.evictMissing(notifications); + } + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Ask the api how many are unread and push the answer to unreadCount$. + * + * Direct HttpClient. unread_count answers `{count: n}`, which is not a + * Notification. + * + * Only the newest request in flight is allowed to move the subject, see + * appliedUnreadCountRequest. The returned observable still emits whatever the + * server said either way, because the caller asked a question and that is the + * answer to it. unreadCount$ is the one the bell reads and the one that has to + * stay right. + */ + public refreshUnreadCount(): Observable { + const request = ++this.issuedUnreadCountRequests; + + return this.apiHttpClient.get<{count: number}>(`${API_URL}/notifications/unread_count`).pipe( + map((response) => response.count), + tap((count) => { + if (request > this.appliedUnreadCountRequest) { + this.appliedUnreadCountRequest = request; + this.unreadCountSubject.next(count); + } + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Mark one notification as read. + * + * The api answers with the updated entity, so this goes through update() and + * the cache corrects itself. The body is empty and the endpoint ignores it. + */ + public markRead(notification: Notification): Observable { + const wasUnread = !notification.isRead; + + const options: RequestOptions = { + endpointFormat: this.markReadEndpointFormat, + entity: notification, + }; + + return super.update({id: notification.id}, options).pipe( + tap(() => { + // Only when it was unread. mark_read! is a no-op on the api for one + // already read, so decrementing again would drift the bell below the + // real count. + if (wasUnread) { + this.adjustUnreadCount(-1); + } + + this.resyncUnreadCount(); + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Mark every unread notification as read. + * + * Direct HttpClient, because read_all answers `{success: true}`. That leaves + * the cache holding rows the server now considers read, so they are corrected + * here. Without it the bell drops to zero while the list still draws every + * row as unread, which reads as an api bug and is not one. + * + * The read time is this machine's clock, because read_all sends none back. It + * is close enough to display and the next list() replaces it with the server's + * value. + */ + public markAllRead(): Observable { + return this.apiHttpClient.put<{success: boolean}>(`${API_URL}/notifications/read_all`, {}).pipe( + map(() => void 0), + tap(() => { + const readAt = new Date(); + + this.cache.currentValues + .filter((notification) => !notification.isRead) + .forEach((notification) => { + notification.readAt = readAt; + + // Writing the property alone announces nothing. Only EntityCache.set + // calls updateCacheArray, so without this cache.values never emits + // and an OnPush list bound to it keeps drawing every row unread. + this.cache.set(notification.key, notification); + }); + + this.setUnreadCount(0); + this.resyncUnreadCount(); + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Delete every notification the user had seen when they confirmed the bulk + * action, without deleting anything that arrived afterwards. + * + * `throughId` is the highest id in the confirmed snapshot. The API applies + * it inside the current user's association, so this is both account-scoped + * and safe against a new notification arriving while the dialog is open. + */ + public deleteAll(throughId: number): Observable { + const removed = this.cache.currentValues.filter((notification) => notification.id <= throughId); + const removedUnread = removed.filter((notification) => !notification.isRead).length; + + return this.apiHttpClient + .delete<{success: boolean; deleted_count: number}>(`${API_URL}/notifications`, { + params: {through_id: throughId}, + }) + .pipe( + map((response) => response.deleted_count), + tap(() => { + removed.map((notification) => notification.key).forEach((key) => this.cache.delete(key)); + this.adjustUnreadCount(-removedUnread); + this.resyncUnreadCount(); + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Delete one notification. + * + * Named remove because delete() is inherited and takes path ids. This wraps it + * so callers pass the entity and the unread count follows. The base class + * evicts it from the cache. + */ + public remove(notification: Notification): Observable { + const wasUnread = !notification.isRead; + + return super.delete<{success: boolean}>(notification.id).pipe( + map(() => void 0), + tap(() => { + if (wasUnread) { + this.adjustUnreadCount(-1); + } + + this.resyncUnreadCount(); + }), + takeUntil(this.sessionEnded), + ); + } + + /** + * Forget everything held for the signed in user. + * + * Call this on sign out. The service is provided in root and the app signs out + * by routing rather than reloading, so without this the cache keeps one + * person's notification messages and the bell keeps their unread count. On a + * shared machine the next person to sign in sees both. AuthenticationService + * already drops the push registration for the same reason. + * + * Clearing is not enough on its own. Anything still in flight would answer + * after the clear and write the previous user's rows straight back, so the + * requests are cancelled first. + */ + public reset(): void { + // Before the clear, not after. This unsubscribes every request the service + // has handed out, which aborts each one, so none of them reach the taps + // that write to the cache or the count. + this.sessionEnded.next(); + + this.cache.clear(); + this.setUnreadCount(0); + } + + /** + * Drop anything the cache holds that the server did not just send back. + * + * fetchAll only ever adds. Without this a notification deleted in another tab + * stays cached for the rest of the session, and markAllRead would walk over a + * row that no longer exists. + * + * Only correct after an unfiltered list. An unread_only response leaves out + * every read notification deliberately, so evicting on that would throw away + * rows that are still there. + */ + private evictMissing(present: Notification[]): void { + const keep = new Set(present.map((notification) => notification.key)); + + // currentValues is a live readonly view, so collect the doomed keys before + // deleting rather than mutating while walking it. + this.cache.currentValues + .filter((notification) => !keep.has(notification.key)) + .map((notification) => notification.key) + .forEach((key) => this.cache.delete(key)); + } + + /** + * Move the unread count without going back to the api. + * + * Floored at zero. A negative badge is a worse thing to show than a stale one. + */ + private adjustUnreadCount(delta: number): void { + this.setUnreadCount(Math.max(0, this.unreadCountSubject.value + delta)); + } + + /** + * Put a locally worked out number on the bell. + * + * A local write counts as newer than every count request already in the air, + * so this retires all of them. Without that, one sent before this line could + * answer after it and undo it. That is the same stale write the sequence + * guard exists to stop, just arriving from the other direction. + */ + private setUnreadCount(count: number): void { + this.appliedUnreadCountRequest = this.issuedUnreadCountRequests; + this.unreadCountSubject.next(count); + } + + /** + * Re-read the count from the api without making the caller wait for it. + * + * Every mutation adjusts the count locally first so the bell moves straight + * away, then calls this. The local arithmetic drifts: two mark read calls + * raised before either answers both see the row as unread and both subtract + * one, and nothing local can know about a notification another tab created or + * deleted. The server number is the real one, so take it while we are already + * talking to the server. + * + * Errors are swallowed on purpose. A count that failed to refresh is not worth + * failing the action the user actually asked for. + */ + private resyncUnreadCount(): void { + this.refreshUnreadCount().subscribe({error: () => undefined}); + } +} diff --git a/src/app/api/services/push-notification-click.service.ts b/src/app/api/services/push-notification-click.service.ts new file mode 100644 index 0000000000..539c968927 --- /dev/null +++ b/src/app/api/services/push-notification-click.service.ts @@ -0,0 +1,67 @@ +// MN-C03: receive Angular service-worker clicks in an already-open client. +import {DOCUMENT} from '@angular/common'; +import {Inject, Injectable} from '@angular/core'; +import {SwPush} from '@angular/service-worker'; +import {Subscription} from 'rxjs'; +import {NotificationRouteService} from './notification-route.service'; + +interface NotificationClickData { + notification_id?: unknown; + link?: unknown; +} + +@Injectable({providedIn: 'root'}) +export class PushNotificationClickService { + private static readonly DUPLICATE_WINDOW_MS = 2000; + + private clickSubscription: Subscription | null = null; + private lastClickKey: string | null = null; + private lastClickAt = 0; + + constructor( + private swPush: SwPush, + private notificationRoutes: NotificationRouteService, + @Inject(DOCUMENT) private appDocument: Document, + ) {} + + public start(): void { + if (this.clickSubscription) { + return; + } + + this.clickSubscription = this.swPush.notificationClicks.subscribe((event) => { + // Angular focuses the most recently focused client before broadcasting the + // click. Only that focused document should navigate when several tabs exist. + if (!this.appDocument.hasFocus()) { + return; + } + + const data = event.notification.data as NotificationClickData | null | undefined; + const target = this.notificationRoutes.resolve(data?.link); + const clickKey = `${this.normalisedId(data?.notification_id)}:${target}`; + const now = Date.now(); + + if ( + clickKey === this.lastClickKey && + now - this.lastClickAt < PushNotificationClickService.DUPLICATE_WINDOW_MS + ) { + return; + } + + this.lastClickKey = clickKey; + this.lastClickAt = now; + void this.notificationRoutes.navigate(target); + }); + } + + public stop(): void { + this.clickSubscription?.unsubscribe(); + this.clickSubscription = null; + this.lastClickKey = null; + this.lastClickAt = 0; + } + + private normalisedId(value: unknown): string { + return typeof value === 'number' || typeof value === 'string' ? String(value) : 'no-id'; + } +} diff --git a/src/app/api/services/push-notification.service.ts b/src/app/api/services/push-notification.service.ts new file mode 100644 index 0000000000..4b47c0d28b --- /dev/null +++ b/src/app/api/services/push-notification.service.ts @@ -0,0 +1,325 @@ +import {HttpClient} from '@angular/common/http'; +import {Injectable} from '@angular/core'; +import {SwPush} from '@angular/service-worker'; +import { + Observable, + Subscription, + catchError, + concatMap, + from, + map, + of, + switchMap, + take, + timeout, +} from 'rxjs'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {DoubtfireConstants} from 'src/app/config/constants/doubtfire-constants'; + +/** How long sign out waits on push clean-up before it carries on without it. */ +export const QUIET_UNSUBSCRIBE_LIMIT_MS = 5000; + +/** + * Why the browser cannot be subscribed to push right now, or null if it can. + */ +export type PushBlocker = + | 'no-service-worker' + | 'not-configured' + | 'permission-denied' + | 'unsupported'; + +/** + * Browsers we have specific unblock instructions for. 'other' covers Safari, + * Opera, and anything else — a generic fallback still gets shown. + */ +export type SupportedBrowser = 'chrome' | 'edge' | 'firefox' | 'other'; + +/** + * Which browser the user agent string identifies as. + * + * Order matters: Edge's user agent also contains "Chrome/", and older Opera + * builds do too, so the more specific token has to be checked first or every + * Edge user gets Chrome's instructions. + */ +export function detectBrowser(userAgent: string): SupportedBrowser { + const ua = userAgent.toLowerCase(); + + if (ua.includes('edg/')) { + return 'edge'; + } + + if (ua.includes('firefox/')) { + return 'firefox'; + } + + if (ua.includes('opr/') || ua.includes('opt/') || ua.includes('opera/')) { + return 'other'; + } + + if (ua.includes('chrome/')) { + return 'chrome'; + } + + return 'other'; +} + +/** + * Step-by-step instructions for reversing a blocked notification permission, + * per browser. A site cannot re-prompt once denied — only the user can undo + * it, in their own browser's settings. + */ +export const PERMISSION_DENIED_INSTRUCTIONS: Record = { + chrome: [ + 'Click the lock (or tune) icon to the left of the address bar.', + 'Select "Site settings".', + 'Set Notifications to "Allow".', + 'Reload this page.', + ], + edge: [ + 'Click the lock icon to the left of the address bar.', + 'Select "Permissions for this site".', + 'Set Notifications to "Allow".', + 'Reload this page.', + ], + firefox: [ + 'Click the lock icon to the left of the address bar.', + 'Select "More Information", then the Permissions tab.', + 'Clear the blocked Notifications permission and set it to "Allow".', + 'Reload this page.', + ], + other: [ + "Open this site's permissions from your browser's address bar menu.", + 'Find Notifications and set it to "Allow".', + 'Reload this page.', + ], +}; + +@Injectable({providedIn: 'root'}) +export class PushNotificationService { + private readonly endpoint = `${API_URL}/push_subscriptions`; + private subscriptionChangeSubscription: Subscription | null = null; + + constructor( + private http: HttpClient, + private swPush: SwPush, + private constants: DoubtfireConstants, + ) {} + + /** + * Emits the current subscription, or null when this browser is not subscribed. + * + * The service worker registers six seconds after bootstrap + * (doubtfire-angular.module.ts), so this is null on load and then changes. + * Subscribe to it rather than reading it once. + */ + public get subscription$(): Observable { + return this.swPush.subscription; + } + + /** + * Whether the service worker is running. False in a build without it, and + * false for the first six seconds of every page load. + */ + public get isEnabled(): boolean { + return this.swPush.isEnabled; + } + + /** + * What is stopping this browser subscribing, or null if nothing is. + */ + public blocker(): PushBlocker | null { + if (typeof Notification === 'undefined' || !('PushManager' in window)) { + return 'unsupported'; + } + if (Notification.permission === 'denied') { + return 'permission-denied'; + } + if (!this.constants.IsPushEnabled.value || !this.constants.VapidPublicKey.value) { + return 'not-configured'; + } + if (!this.swPush.isEnabled) { + return 'no-service-worker'; + } + return null; + } + + /** + * Which browser this is, based on the user agent. Drives which set of + * unblock instructions is shown. + */ + public get browser(): SupportedBrowser { + return detectBrowser(typeof navigator !== 'undefined' ? navigator.userAgent : ''); + } + + /** + * Step-by-step instructions for this browser to reverse a blocked + * notification permission. Only meaningful when blocker() returned + * 'permission-denied'; callers should check that first. + */ + public permissionDeniedInstructions(): string[] { + return PERMISSION_DENIED_INSTRUCTIONS[this.browser]; + } + + /** + * Keep the api registration in step with browser-driven subscription changes. + * + * Angular 22 forwards the service worker's `pushsubscriptionchange` event as + * `pushSubscriptionChanges`. Register the replacement before removing the old + * endpoint so a transient api failure cannot throw away the last server row. + */ + public start(): void { + if (this.subscriptionChangeSubscription) { + return; + } + + this.subscriptionChangeSubscription = this.swPush.pushSubscriptionChanges + .pipe( + concatMap(({oldSubscription, newSubscription}) => { + if (!newSubscription) { + // Some browsers can report invalidation without a replacement. A + // fresh subscribe needs a user gesture, so leave recovery to the + // existing opt-in control and let server delivery remove the dead row. + return of(void 0); + } + + return this.registerSubscription(newSubscription).pipe( + switchMap(() => { + if (!oldSubscription || oldSubscription.endpoint === newSubscription.endpoint) { + return of(void 0); + } + + return this.removeServerSubscription(oldSubscription.endpoint); + }), + // One failed sync must not terminate the long-lived rotation stream. + catchError((error) => { + console.error('Could not update the rotated push subscription', error); + return of(void 0); + }), + ); + }), + ) + .subscribe(); + } + + public stop(): void { + this.subscriptionChangeSubscription?.unsubscribe(); + this.subscriptionChangeSubscription = null; + } + + /** + * Ask the browser for permission, then store the resulting registration + * against the signed in user. + * + * Call this from a click and nowhere else. requestSubscription needs an active + * service worker and a user gesture; called during app init it fails in a way + * that looks nothing like the real cause. + */ + public subscribe(): Observable { + return from( + this.swPush.requestSubscription({ + serverPublicKey: this.constants.VapidPublicKey.value, + }), + ).pipe(switchMap((subscription) => this.registerSubscription(subscription))); + } + + /** + * Stop this browser receiving push, both in the browser and on the api. + * + * The api call goes first and on purpose: once swPush.unsubscribe() has run + * the endpoint is gone from this page, and a row nobody can delete would sit + * there until the push service returned 410 for it. + */ + public unsubscribe(): Observable { + return this.unsubscribeWithin(); + } + + /** + * Unsubscribe, but never fail, and never wait long on anything outside this page. + * + * For sign out, where the caller cannot do anything useful with an error and + * must not be blocked by one. Prefer unsubscribe() anywhere a person is + * waiting on the result and should be told it did not work. + */ + public unsubscribeQuietly(): Observable { + return this.unsubscribeWithin(QUIET_UNSUBSCRIBE_LIMIT_MS).pipe(catchError(() => of(void 0))); + } + + /** + * With a limit, each wait on something outside this page gives up after it: + * finding the subscription, the api delete and the browser unsubscribe. A slow + * api delete still goes on to unsubscribe the browser, which matters more. + */ + private unsubscribeWithin(limitMs?: number): Observable { + const within = (source: Observable, fallback: T): Observable => + limitMs === undefined + ? source + : source.pipe(timeout({first: limitMs, with: () => of(fallback)})); + + // Angular exposes SwPush.subscription as NEVER when service workers are + // disabled, so waiting on it would prevent sign-out from continuing. + if (!this.swPush.isEnabled) { + return of(void 0); + } + + // SwPush only answers once a service worker controls this page, and a page + // can have none with the worker enabled: it registers six seconds after + // bootstrap, a hard reload bypasses it, and the dev server has no worker + // file to register. Waiting on SwPush then never ends, which left sign out + // stuck. The browser's own registration answers at once, with nothing when + // there is none. + const container = typeof navigator === 'undefined' ? undefined : navigator.serviceWorker; + let found: Observable; + let unsubscribeInBrowser: (subscription: PushSubscription) => Promise; + + if (container?.controller) { + // take(1) matters. swPush.subscription never completes, so without it the + // returned observable would stay open forever and re-fire on every change. + // Unsubscribing through SwPush, not the browser, keeps subscription$ current. + found = this.swPush.subscription.pipe(take(1)); + unsubscribeInBrowser = () => this.swPush.unsubscribe(); + } else if (container) { + found = from(container.getRegistration()).pipe( + switchMap((registration) => + registration ? from(registration.pushManager.getSubscription()) : of(null), + ), + ); + unsubscribeInBrowser = (subscription) => subscription.unsubscribe(); + } else { + return of(void 0); + } + + return within(found, null).pipe( + switchMap((subscription) => { + if (!subscription) { + return of(void 0); + } + + return within(this.removeServerSubscription(subscription.endpoint), undefined).pipe( + // Server cleanup failing must not leave the browser subscribed. If + // the delete fails we still unsubscribe locally: the browser stops + // receiving immediately, and the row left behind is removed the + // first time the push service answers 410 for it, which deliver_to + // in PushNotificationService already handles. + catchError(() => of(void 0)), + switchMap(() => + within(from(unsubscribeInBrowser(subscription)).pipe(map(() => void 0)), undefined), + ), + ); + }), + ); + } + + private registerSubscription(subscription: PushSubscription): Observable { + const keys = subscription.toJSON().keys ?? {}; + + return this.http.post(this.endpoint, { + endpoint: subscription.endpoint, + p256dh: keys.p256dh, + auth: keys.auth, + }); + } + + private removeServerSubscription(endpoint: string): Observable { + return this.http.delete(this.endpoint, {params: {endpoint}}); + } +} diff --git a/src/app/api/services/spec/notification-route.service.spec.ts b/src/app/api/services/spec/notification-route.service.spec.ts new file mode 100644 index 0000000000..ae17a4f2e8 --- /dev/null +++ b/src/app/api/services/spec/notification-route.service.spec.ts @@ -0,0 +1,263 @@ +// MN-C03 targeted route-boundary tests. +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {Router} from '@angular/router'; +import {AuthReturnUrlService} from 'src/app/security/auth-return-url.service'; +import {AuthenticationService} from '../authentication.service'; +import {NotificationFeedbackRouteIntentService} from '../notification-feedback-route-intent.service'; +import {NOTIFICATION_ROUTE_FALLBACK, NotificationRouteService} from '../notification-route.service'; + +describe('NotificationRouteService', () => { + let router: {url: string; navigateByUrl: ReturnType}; + let authentication: {isAuthenticated: ReturnType}; + let authReturnUrl: {remember: ReturnType}; + let service: NotificationRouteService; + + beforeEach(() => { + router = { + url: '/home', + navigateByUrl: vi.fn().mockResolvedValue(true), + }; + authentication = {isAuthenticated: vi.fn().mockReturnValue(true)}; + authReturnUrl = {remember: vi.fn()}; + service = new NotificationRouteService( + router as unknown as Router, + authentication as unknown as AuthenticationService, + authReturnUrl as unknown as AuthReturnUrlService, + ); + }); + + it('accepts each approved notification route family', () => { + const approved = [ + '/notifications', + '/projects/1/dashboard', + '/projects/23/groups', + '/projects/23/dashboard/1.1P', + '/projects/23/dashboard/1.1P/feedback', + '/projects/23/dashboard/T1.1', + '/projects/23/dashboard/HD1.2', + '/projects/23/dashboard/10.1H', + '/projects/23/dashboard/A15', + '/projects/23/dashboard/TASK1', + '/projects/23/dashboard/P-2.21', + '/projects/23/dashboard/D-9.568', + '/projects/23/dashboard/C-4.602', + '/unit-hub?unit=3&announcement=12', + '/unit-hub?unit=3&session=40', + ]; + + for (const route of approved) { + expect(service.resolve(route)).toBe(route); + } + }); + + it('uses the safe fallback for missing and malformed values', () => { + const invalid: unknown[] = [ + undefined, + null, + 42, + {}, + '', + ' ', + ' /projects/1/dashboard', + '/projects/1/dashboard ', + '/projects/0/dashboard', + '/projects/-1/dashboard', + '/projects/1//dashboard', + '/projects/1/dashboard/', + '/projects/1/dashboard/1.1P/extra', + '/projects/1/dashboard/1.1P/feedback/extra', + '/projects/1/dashboard/line\nbreak', + `/projects/1/dashboard/${'A1'.repeat(20)}`, + ]; + + for (const value of invalid) { + expect(service.resolve(value)).toBe(NOTIFICATION_ROUTE_FALLBACK); + } + }); + + it('rejects absolute, external, scheme and protocol-relative destinations', () => { + const invalid = [ + 'http://example.test/projects/1/dashboard', + 'https://example.test/projects/1/dashboard', + 'mailto:student@example.test', + 'javascript:alert(1)', + 'data:text/html,unsafe', + 'file:///etc/passwd', + '//example.test/projects/1/dashboard', + '\\example.test\\projects\\1', + '/\\example.test/projects/1', + '/projects\\1\\dashboard', + ]; + + for (const value of invalid) { + expect(service.resolve(value)).toBe(NOTIFICATION_ROUTE_FALLBACK); + } + }); + + it('rejects encoded bypass attempts rather than decoding them', () => { + const invalid = [ + '/%2f%2fexample.test', + '/%5cexample.test', + '/projects/1/dashboard/%31.1P', + '/projects/1/dashboard/1.1P%3ftoken%3dsecret', + '/projects/1/dashboard/1.1P%23feedback', + '/%252f%252fexample.test', + '/projects/1/dashboard/%00', + '/projects/1/dashboard/%', + ]; + + for (const value of invalid) { + expect(service.resolve(value)).toBe(NOTIFICATION_ROUTE_FALLBACK); + } + }); + + it('rejects unexpected route families, queries and fragments', () => { + const invalid = [ + '/home', + '/units/1', + '/projects/1', + '/projects/1/portfolio', + '/projects/1/dashboard/1.1P?token=secret', + '/projects/1/dashboard/1.1P#feedback', + '/notifications?student=Alice', + '/unit-hub', + '/unit-hub?unit=3', + '/unit-hub?announcement=12&unit=3', + '/unit-hub?unit=0&announcement=12', + '/unit-hub?unit=3&announcement=12&next=//evil.test', + '/unit-hub?unit=3&session=40#details', + '/unit-hub?unit=3&task=12', + '/unit-hub?unit=3&session=%34%30', + ]; + + for (const value of invalid) { + expect(service.resolve(value)).toBe(NOTIFICATION_ROUTE_FALLBACK); + } + }); + + it('accepts bounded task segments without guessing their business format', () => { + const approved = [ + '/projects/1/dashboard/85', + '/projects/1/dashboard/Alice1', + '/projects/1/dashboard/BOB1', + '/projects/1/dashboard/1.1ALICE', + '/projects/1/dashboard/1.1BOB', + '/projects/1/dashboard/feedback1', + '/projects/1/dashboard/token123', + '/projects/1/dashboard/mark85', + ]; + + for (const route of approved) { + expect(service.resolve(route)).toBe(route); + } + }); + + it('navigates an open app through Angular Router', async () => { + await expect(service.navigate('/projects/23/dashboard/1.1P')).resolves.toBe(true); + + expect(router.navigateByUrl).toHaveBeenCalledOnce(); + expect(router.navigateByUrl).toHaveBeenCalledWith('/projects/23/dashboard/1.1P'); + }); + + it('navigates unsafe input to the safe fallback', async () => { + await service.navigate('https://example.test/steal'); + + expect(router.navigateByUrl).toHaveBeenCalledWith(NOTIFICATION_ROUTE_FALLBACK); + }); + + it('saves an allow-listed target before sending an anonymous client to sign in', async () => { + authentication.isAuthenticated.mockReturnValue(false); + const target = '/projects/23/dashboard/1.1P/feedback'; + + await service.navigate(target); + + expect(authReturnUrl.remember).toHaveBeenCalledWith(target); + expect(router.navigateByUrl).toHaveBeenCalledWith('/sign_in'); + }); + + it('never saves an unvalidated notification destination', async () => { + authentication.isAuthenticated.mockReturnValue(false); + + await service.navigate('https://evil.example/steal'); + + expect(authReturnUrl.remember).toHaveBeenCalledWith(NOTIFICATION_ROUTE_FALLBACK); + expect(authReturnUrl.remember).not.toHaveBeenCalledWith('https://evil.example/steal'); + }); + + it('does not navigate twice when a closed-app launch already opened the target', async () => { + router.url = '/projects/23/dashboard/1.1P?from=service-worker#top'; + + await expect(service.navigate('/projects/23/dashboard/1.1P')).resolves.toBe(true); + + expect(router.navigateByUrl).not.toHaveBeenCalled(); + }); + + it('does not re-navigate the fallback when notifications is already open', async () => { + router.url = '/notifications'; + + await service.navigate(undefined); + + expect(router.navigateByUrl).not.toHaveBeenCalled(); + }); + + it('carries a validated feedback intent until the project resolves the task definition', async () => { + const intents = new NotificationFeedbackRouteIntentService(); + service = new NotificationRouteService( + router as unknown as Router, + authentication as unknown as AuthenticationService, + authReturnUrl as unknown as AuthReturnUrlService, + intents, + ); + + await service.navigate('/projects/23/dashboard/1.1P/feedback'); + + expect(intents.consume({projectId: 23, taskAbbreviation: '1.1P'})).not.toBeNull(); + }); + + it('emits a new intent without navigating when that feedback route is already open', async () => { + const intents = new NotificationFeedbackRouteIntentService(); + const received = vi.fn(); + intents.requests$.subscribe(received); + service = new NotificationRouteService( + router as unknown as Router, + authentication as unknown as AuthenticationService, + authReturnUrl as unknown as AuthReturnUrlService, + intents, + ); + router.url = '/projects/23/dashboard/1.1P/feedback'; + + await service.navigate('/projects/23/dashboard/1.1P/feedback'); + + expect(received).toHaveBeenCalledOnce(); + expect(router.navigateByUrl).not.toHaveBeenCalled(); + }); + + it('never creates a feedback intent from an unsafe or non-feedback destination', async () => { + const intents = new NotificationFeedbackRouteIntentService(); + service = new NotificationRouteService( + router as unknown as Router, + authentication as unknown as AuthenticationService, + authReturnUrl as unknown as AuthReturnUrlService, + intents, + ); + + await service.navigate('https://example.test/projects/23/dashboard/1.1P/feedback'); + + expect(intents.consume({projectId: 23, taskAbbreviation: '1.1P'})).toBeNull(); + }); + + it('cancels the intent when Angular refuses the navigation', async () => { + const intents = new NotificationFeedbackRouteIntentService(); + router.navigateByUrl.mockResolvedValue(false); + service = new NotificationRouteService( + router as unknown as Router, + authentication as unknown as AuthenticationService, + authReturnUrl as unknown as AuthReturnUrlService, + intents, + ); + + await service.navigate('/projects/23/dashboard/1.1P/feedback'); + + expect(intents.consume({projectId: 23, taskAbbreviation: '1.1P'})).toBeNull(); + }); +}); diff --git a/src/app/api/services/spec/notification.service.spec.ts b/src/app/api/services/spec/notification.service.spec.ts new file mode 100644 index 0000000000..f2c04750a0 --- /dev/null +++ b/src/app/api/services/spec/notification.service.spec.ts @@ -0,0 +1,526 @@ +import {afterEach, beforeEach, describe, expect, it} from 'vitest'; +import { + HttpRequest, + provideHttpClient, + withInterceptorsFromDi, + withXhr, +} from '@angular/common/http'; +import {HttpTestingController, provideHttpClientTesting} from '@angular/common/http/testing'; +import {TestBed} from '@angular/core/testing'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {Notification} from '../../models/notification'; +import {NotificationService} from '../notification.service'; + +const LIST_URL = `${API_URL}/notifications/`; +const COUNT_URL = `${API_URL}/notifications/unread_count`; +const READ_ALL_URL = `${API_URL}/notifications/read_all`; +const DELETE_ALL_URL = `${API_URL}/notifications`; + +/** + * What NotificationEntity actually puts on the wire. Snake case, and read_at is + * null rather than absent while a notification is unread. + */ +function unreadJson(id = 1) { + return { + id, + notification_type: 'task', + event: 'task_due_soon', + message: 'Jane commented on Task 1.1P', + link: '/#/projects/1/task/2', + read_at: null, + created_at: '2026-08-09T04:12:00.000Z', + }; +} + +function readJson(id = 1, readAt = '2026-08-09T05:00:00.000Z') { + return {...unreadJson(id), read_at: readAt}; +} + +describe('NotificationService', () => { + let service: NotificationService; + let httpMock: HttpTestingController; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [], + providers: [ + NotificationService, + provideHttpClient(withXhr(), withInterceptorsFromDi()), + provideHttpClientTesting(), + ], + }); + + service = TestBed.inject(NotificationService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + // A cancelled request is still open as far as the backend is concerned, and + // reset() cancels on purpose. The tests that call it assert the + // cancellation themselves. Nothing is lost by ignoring them here: a request + // cancelled by accident still fails, because the flush that was expecting + // it throws rather than being skipped. + httpMock.verify({ignoreCancelled: true}); + }); + + /** + * Read whatever unreadCount$ is holding right now. It is a BehaviorSubject so + * this emits synchronously. + */ + function currentUnreadCount(): number { + let count: number; + service.unreadCount$.subscribe((value) => (count = value)).unsubscribe(); + return count; + } + + /** + * Put a known count on the subject so the tests below can assert a movement + * rather than an absolute value. + */ + function seedUnreadCount(count: number): void { + service.refreshUnreadCount().subscribe(); + httpMock.expectOne(COUNT_URL).flush({count}); + } + + /** + * Answer the authoritative count re-read that every mutation fires. Leaving it + * unflushed fails the httpMock.verify() in afterEach, which is the point: the + * resync is part of the contract, not an optimisation a change can quietly + * drop. + */ + function flushResync(count: number): void { + httpMock.expectOne(COUNT_URL).flush({count}); + } + + /** + * Put a list in the cache so cache behaviour can be asserted against it. + */ + function primeCache(...rows: object[]): Notification[] { + let listed: Notification[]; + service.list().subscribe((notifications) => (listed = notifications)); + httpMock.expectOne(LIST_URL).flush(rows); + return listed; + } + + describe('list', () => { + it('gets every notification and maps the entity', () => { + let result: Notification[]; + + service.list().subscribe((notifications) => (result = notifications)); + + const req = httpMock.expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(LIST_URL); + expect(request.method).toBe('GET'); + expect(request.params.has('unread_only')).toBe(false); + return true; + }); + req.flush([unreadJson()]); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ + id: 1, + notificationType: 'task', + event: 'task_due_soon', + message: 'Jane commented on Task 1.1P', + link: '/#/projects/1/task/2', + }); + expect(result[0].createdAt).toBeInstanceOf(Date); + }); + + it('reports an unread notification as unread', () => { + const listed = primeCache(unreadJson()); + + // read_at comes back null. new Date(null) is the epoch and would read as + // truthy, so this fails if the mapping ever goes back to mapDate. + expect(listed[0].readAt).toBeNull(); + expect(listed[0].isRead).toBe(false); + }); + + it('updates the same entity objects on a second call instead of rebuilding them', () => { + const first = primeCache(unreadJson(1)); + const second = primeCache(readJson(1)); + + // Same instance, not just equal. fetchAll only reuses cached entities when + // options.cache is passed, so this fails the moment that is dropped, and a + // component holding the first array would silently go stale. + expect(second[0]).toBe(first[0]); + expect(first[0].isRead).toBe(true); + }); + + it('evicts rows the server no longer returns', () => { + primeCache(unreadJson(1), unreadJson(2)); + expect(service.cache.size).toBe(2); + + primeCache(unreadJson(1)); + + // fetchAll only ever adds. Without the eviction pass a notification + // deleted in another tab would sit in the cache for the rest of the + // session and markAllRead would walk over a row that is gone. + expect(service.cache.size).toBe(1); + }); + + it('does not evict when only unread rows were asked for', () => { + primeCache(unreadJson(1), readJson(2)); + expect(service.cache.size).toBe(2); + + service.list(true).subscribe(); + httpMock + .expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(LIST_URL); + expect(request.params.get('unread_only')).toBe('true'); + return true; + }) + .flush([unreadJson(1)]); + + // Notification 2 is missing because it is read, not because it is gone. + expect(service.cache.size).toBe(2); + }); + }); + + describe('refreshUnreadCount', () => { + it('gets the count and pushes it to unreadCount$', () => { + let emitted: number; + + service.refreshUnreadCount().subscribe((count) => (emitted = count)); + + const req = httpMock.expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(COUNT_URL); + expect(request.method).toBe('GET'); + return true; + }); + req.flush({count: 7}); + + expect(emitted).toBe(7); + expect(currentUnreadCount()).toBe(7); + }); + + it('ignores an older count response that lands after a newer one', () => { + service.refreshUnreadCount().subscribe(); + service.refreshUnreadCount().subscribe(); + + const requests = httpMock.match(COUNT_URL); + expect(requests).toHaveLength(2); + + // The second request answers first. + requests[1].flush({count: 4}); + expect(currentUnreadCount()).toBe(4); + + // Then the first one answers, carrying the number from before whatever + // triggered the second. Nothing orders http responses, so without the + // sequence guard this overwrites a correct count with a stale one and the + // bell stays wrong until the next mutation. + requests[0].flush({count: 9}); + + expect(currentUnreadCount()).toBe(4); + }); + + it('ignores a count response older than a local change to the count', () => { + seedUnreadCount(5); + + // In flight before the mutation, so its answer was computed without it. + service.refreshUnreadCount().subscribe(); + const stale = httpMock.expectOne(COUNT_URL); + + const notification = new Notification(); + notification.id = 4; + notification.readAt = null; + service.markRead(notification).subscribe(); + httpMock.expectOne(`${API_URL}/notifications/4/read`).flush(readJson(4)); + + expect(currentUnreadCount()).toBe(4); + + // The local decrement is newer than that request, so it has to retire it. + // Guarding only response against response leaves this hole open, and it + // is the common one: every mutation starts a count request. + stale.flush({count: 5}); + + expect(currentUnreadCount()).toBe(4); + + flushResync(4); + }); + }); + + describe('markRead', () => { + it('puts to the read endpoint and drops the unread count by one', () => { + seedUnreadCount(3); + + const notification = new Notification(); + notification.id = 4; + notification.readAt = null; + + let result: Notification; + service.markRead(notification).subscribe((updated) => (result = updated)); + + const req = httpMock.expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(`${API_URL}/notifications/4/read`); + expect(request.method).toBe('PUT'); + return true; + }); + req.flush(readJson(4)); + + expect(result.isRead).toBe(true); + expect(result.readAt).toBeInstanceOf(Date); + expect(currentUnreadCount()).toBe(2); + + flushResync(2); + }); + + it('leaves the count alone when the notification was already read', () => { + seedUnreadCount(3); + + const notification = new Notification(); + notification.id = 4; + notification.readAt = new Date('2026-08-09T05:00:00.000Z'); + + service.markRead(notification).subscribe(); + httpMock.expectOne(`${API_URL}/notifications/4/read`).flush(readJson(4)); + + // mark_read! is a no-op on the api for one already read, so decrementing + // here would push the bell below the real number. + expect(currentUnreadCount()).toBe(3); + + flushResync(3); + }); + + it('lets the server correct the count when two calls race on the same row', () => { + seedUnreadCount(2); + + const notification = new Notification(); + notification.id = 4; + notification.readAt = null; + + service.markRead(notification).subscribe(); + service.markRead(notification).subscribe(); + + const puts = httpMock.match(`${API_URL}/notifications/4/read`); + expect(puts).toHaveLength(2); + puts.forEach((req) => req.flush(readJson(4))); + + // Both captured the row as unread before either answered, so both + // subtracted one and the local number is now one below the truth. This is + // the known limit of local arithmetic, asserted rather than pretended away. + expect(currentUnreadCount()).toBe(0); + + const resyncs = httpMock.match(COUNT_URL); + expect(resyncs).toHaveLength(2); + resyncs.forEach((req) => req.flush({count: 1})); + + // And this is why every mutation re-reads it. + expect(currentUnreadCount()).toBe(1); + }); + }); + + describe('markAllRead', () => { + it('puts to read_all, zeroes the count and corrects the cached rows', () => { + seedUnreadCount(2); + + const listed = primeCache(unreadJson(1), unreadJson(2)); + expect(listed.every((notification) => !notification.isRead)).toBe(true); + + service.markAllRead().subscribe(); + + const req = httpMock.expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(READ_ALL_URL); + expect(request.method).toBe('PUT'); + return true; + }); + req.flush({success: true}); + + expect(currentUnreadCount()).toBe(0); + // read_all answers {success: true} and not entities, so nothing corrects + // the cache unless the service does it. Without this the bell reads zero + // while every row still draws as unread. + expect(listed.every((notification) => notification.isRead)).toBe(true); + + flushResync(0); + }); + + it('announces the corrected rows on the cache observable', () => { + primeCache(unreadJson(1), unreadJson(2)); + + let emissions = 0; + let latest: Notification[]; + const subscription = service.cache.values.subscribe((values) => { + emissions++; + latest = values; + }); + const emissionsBefore = emissions; + + service.markAllRead().subscribe(); + httpMock.expectOne(READ_ALL_URL).flush({success: true}); + + // Assigning readAt announces nothing on its own. Only EntityCache.set + // calls updateCacheArray, so an OnPush list bound to cache.values would + // never redraw without it. + expect(emissions).toBeGreaterThan(emissionsBefore); + expect(latest.every((notification) => notification.isRead)).toBe(true); + + subscription.unsubscribe(); + flushResync(0); + }); + }); + + describe('remove', () => { + it('deletes the notification and drops the unread count by one', () => { + seedUnreadCount(3); + + const notification = new Notification(); + notification.id = 9; + notification.readAt = null; + + service.remove(notification).subscribe(); + + const req = httpMock.expectOne((request: HttpRequest): boolean => { + expect(request.url).toEqual(`${API_URL}/notifications/9`); + expect(request.method).toBe('DELETE'); + return true; + }); + req.flush({success: true}); + + expect(currentUnreadCount()).toBe(2); + + flushResync(2); + }); + + it('leaves the count alone when deleting one that was already read', () => { + seedUnreadCount(3); + + const notification = new Notification(); + notification.id = 9; + notification.readAt = new Date('2026-08-09T05:00:00.000Z'); + + service.remove(notification).subscribe(); + httpMock.expectOne(`${API_URL}/notifications/9`).flush({success: true}); + + expect(currentUnreadCount()).toBe(3); + + flushResync(3); + }); + + it('evicts the deleted notification from the cache', () => { + primeCache(unreadJson(1), unreadJson(2)); + + const target = service.cache.currentValues.find((notification) => notification.id === 2); + + service.remove(target).subscribe(); + httpMock.expectOne(`${API_URL}/notifications/2`).flush({success: true}); + + expect(service.cache.size).toBe(1); + expect(service.cache.currentValues[0].id).toBe(1); + + flushResync(0); + }); + }); + + describe('deleteAll', () => { + it('deletes only through the confirmed id and evicts that snapshot', () => { + seedUnreadCount(3); + primeCache(unreadJson(3), readJson(4), unreadJson(5)); + + let deletedCount: number; + service.deleteAll(4).subscribe((count) => (deletedCount = count)); + + const request = httpMock.expectOne((candidate: HttpRequest): boolean => { + expect(candidate.url).toBe(DELETE_ALL_URL); + expect(candidate.method).toBe('DELETE'); + expect(candidate.params.get('through_id')).toBe('4'); + return true; + }); + request.flush({success: true, deleted_count: 2}); + + expect(deletedCount).toBe(2); + expect(service.cache.currentValues.map((row) => row.id)).toEqual([5]); + expect(currentUnreadCount()).toBe(2); + + flushResync(2); + }); + + it('does not evict or change the count when the request fails', () => { + seedUnreadCount(2); + primeCache(unreadJson(1), unreadJson(2)); + + service.deleteAll(2).subscribe({error: () => undefined}); + httpMock + .expectOne( + (request: HttpRequest) => + request.url === DELETE_ALL_URL && request.params.get('through_id') === '2', + ) + .flush({error: 'failed'}, {status: 500, statusText: 'Server Error'}); + + expect(service.cache.currentValues.map((row) => row.id)).toEqual([1, 2]); + expect(currentUnreadCount()).toBe(2); + }); + }); + + describe('reset', () => { + it('drops the cache and the unread count', () => { + primeCache(unreadJson(1), unreadJson(2)); + seedUnreadCount(4); + + expect(service.cache.size).toBe(2); + expect(currentUnreadCount()).toBe(4); + + service.reset(); + + // The service is provided in root and sign out routes rather than + // reloading, so without this the next person to sign in on a shared + // machine starts with the previous person's messages and count. + expect(service.cache.size).toBe(0); + expect(currentUnreadCount()).toBe(0); + }); + + it('cancels a list still in flight so it cannot refill the cache', () => { + let emitted = false; + + service.list().subscribe(() => (emitted = true)); + const pending = httpMock.expectOne(LIST_URL); + + service.reset(); + + // Aborted, not merely ignored, and that distinction is the whole fix. + // The cache write lives in a tap inside the base class, upstream of + // anything this service could put a guard in, so the only way to stop it + // is to make sure the response never arrives. + expect(pending.cancelled).toBe(true); + expect(emitted).toBe(false); + expect(service.cache.size).toBe(0); + }); + + it('cancels a count re-sync still in flight so it cannot restore the count', () => { + seedUnreadCount(5); + + const notification = new Notification(); + notification.id = 4; + notification.readAt = null; + + service.markRead(notification).subscribe(); + httpMock.expectOne(`${API_URL}/notifications/4/read`).flush(readJson(4)); + expect(currentUnreadCount()).toBe(4); + + // Nobody holds this one. It is fired and forgotten by every mutation, so + // it is the request most likely to be open when someone signs out. + const resync = httpMock.expectOne(COUNT_URL); + + service.reset(); + + expect(resync.cancelled).toBe(true); + expect(currentUnreadCount()).toBe(0); + }); + + it('cancels a mark read still in flight so it cannot re-add the entity', () => { + primeCache(unreadJson(1)); + + const notification = service.cache.currentValues[0]; + service.markRead(notification).subscribe(); + const pending = httpMock.expectOne(`${API_URL}/notifications/1/read`); + + service.reset(); + + // update() writes the response entity into the cache in a base class tap. + // A sign out between the request and the response would otherwise leave + // one of the previous user's notifications sitting in an empty cache. + expect(pending.cancelled).toBe(true); + expect(service.cache.size).toBe(0); + }); + }); +}); diff --git a/src/app/api/services/spec/push-notification-click.service.spec.ts b/src/app/api/services/spec/push-notification-click.service.spec.ts new file mode 100644 index 0000000000..19a94a8db7 --- /dev/null +++ b/src/app/api/services/spec/push-notification-click.service.spec.ts @@ -0,0 +1,131 @@ +// MN-C03 targeted SwPush click tests. +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {SwPush} from '@angular/service-worker'; +import {Subject} from 'rxjs'; +import {NOTIFICATION_ROUTE_FALLBACK, NotificationRouteService} from '../notification-route.service'; +import {PushNotificationClickService} from '../push-notification-click.service'; + +describe('PushNotificationClickService', () => { + let notificationClicks: Subject; + let notificationRoutes: { + resolve: ReturnType; + navigate: ReturnType; + }; + let appDocument: {hasFocus: ReturnType}; + let service: PushNotificationClickService; + + beforeEach(() => { + notificationClicks = new Subject(); + notificationRoutes = { + resolve: vi.fn((link: unknown) => + typeof link === 'string' && link.startsWith('/projects/') + ? link + : NOTIFICATION_ROUTE_FALLBACK, + ), + navigate: vi.fn().mockResolvedValue(true), + }; + appDocument = {hasFocus: vi.fn().mockReturnValue(true)}; + const swPush = { + notificationClicks: notificationClicks.asObservable(), + } as unknown as SwPush; + + service = new PushNotificationClickService( + swPush, + notificationRoutes as unknown as NotificationRouteService, + appDocument as unknown as Document, + ); + }); + + it('routes an open-app click through the shared boundary', () => { + service.start(); + + notificationClicks.next({ + notification: { + data: {notification_id: 42, link: '/projects/7/dashboard/1.1P'}, + }, + }); + + expect(notificationRoutes.resolve).toHaveBeenCalledWith('/projects/7/dashboard/1.1P'); + expect(notificationRoutes.navigate).toHaveBeenCalledWith('/projects/7/dashboard/1.1P'); + }); + + it('uses the shared fallback for a missing link', () => { + service.start(); + + notificationClicks.next({notification: {data: {notification_id: 43}}}); + + expect(notificationRoutes.resolve).toHaveBeenCalledWith(undefined); + expect(notificationRoutes.navigate).toHaveBeenCalledWith(NOTIFICATION_ROUTE_FALLBACK); + }); + + it('uses the shared fallback for an unsafe link', () => { + service.start(); + + notificationClicks.next({ + notification: { + data: {notification_id: 44, link: 'https://example.test/unsafe'}, + }, + }); + + expect(notificationRoutes.resolve).toHaveBeenCalledWith('https://example.test/unsafe'); + expect(notificationRoutes.navigate).toHaveBeenCalledWith(NOTIFICATION_ROUTE_FALLBACK); + }); + + it('does not navigate a background tab when another client was focused', () => { + appDocument.hasFocus.mockReturnValue(false); + service.start(); + + notificationClicks.next({ + notification: {data: {notification_id: 45, link: '/projects/7/groups'}}, + }); + + expect(notificationRoutes.navigate).not.toHaveBeenCalled(); + }); + + it('subscribes only once when startup calls start more than once', () => { + service.start(); + service.start(); + + notificationClicks.next({ + notification: {data: {notification_id: 46, link: '/projects/7/groups'}}, + }); + + expect(notificationRoutes.navigate).toHaveBeenCalledOnce(); + }); + + it('suppresses a duplicate delivery of the same click event', () => { + service.start(); + const event = { + notification: {data: {notification_id: 47, link: '/projects/7/groups'}}, + }; + + notificationClicks.next(event); + notificationClicks.next(event); + + expect(notificationRoutes.navigate).toHaveBeenCalledOnce(); + }); + + it('does not suppress different notifications that share a route', () => { + service.start(); + + notificationClicks.next({ + notification: {data: {notification_id: 48, link: '/projects/7/groups'}}, + }); + notificationClicks.next({ + notification: {data: {notification_id: 49, link: '/projects/7/groups'}}, + }); + + expect(notificationRoutes.navigate).toHaveBeenCalledTimes(2); + }); + + it('stops receiving clicks after teardown', () => { + service.start(); + service.stop(); + + notificationClicks.next({ + notification: {data: {notification_id: 50, link: '/projects/7/groups'}}, + }); + + expect(notificationRoutes.navigate).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/services/spec/push-notification.service.spec.ts b/src/app/api/services/spec/push-notification.service.spec.ts new file mode 100644 index 0000000000..8fa680edd8 --- /dev/null +++ b/src/app/api/services/spec/push-notification.service.spec.ts @@ -0,0 +1,530 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {provideHttpClient, withInterceptorsFromDi, withXhr} from '@angular/common/http'; +import {HttpTestingController, provideHttpClientTesting} from '@angular/common/http/testing'; +import {TestBed} from '@angular/core/testing'; +import {SwPush} from '@angular/service-worker'; +import {BehaviorSubject, NEVER, Observable, Subject} from 'rxjs'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {DoubtfireConstants} from 'src/app/config/constants/doubtfire-constants'; +import { + PERMISSION_DENIED_INSTRUCTIONS, + PushNotificationService, + QUIET_UNSUBSCRIBE_LIMIT_MS, + detectBrowser, +} from '../push-notification.service'; + +const ENDPOINT = 'https://fcm.googleapis.com/fcm/send/test-browser'; + +/** + * Let pending promise callbacks run. + * + * unsubscribe() ends in from(swPush.unsubscribe()), and that promise settles on + * the microtask queue, so completion is not observable in the same tick as the + * http flush. A macrotask drains everything queued behind it. + */ +function flushMicrotasks(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + +/** + * Enough of a PushSubscription for the service. The real one comes from the + * browser and cannot be constructed in a test environment. + */ +function fakeSubscription( + endpoint: string = ENDPOINT, + p256dh: string = 'BFakePublicKey', + auth: string = 'FakeAuthSecret', +) { + return { + endpoint, + toJSON: () => ({ + endpoint, + keys: {p256dh, auth}, + }), + } as unknown as PushSubscription; +} + +/** Stand in for navigator.serviceWorker, which jsdom does not have. */ +function setServiceWorker(container: { + controller: object | null; + getRegistration?: () => Promise; +}): void { + Object.defineProperty(navigator, 'serviceWorker', {configurable: true, value: container}); +} + +describe('PushNotificationService', () => { + let service: PushNotificationService; + let httpMock: HttpTestingController; + let subscriptionSubject: BehaviorSubject; + let subscriptionChangeSubject: Subject<{ + oldSubscription: PushSubscription | null; + newSubscription: PushSubscription | null; + }>; + let swPush: { + isEnabled: boolean; + subscription: Observable; + pushSubscriptionChanges: Observable<{ + oldSubscription: PushSubscription | null; + newSubscription: PushSubscription | null; + }>; + requestSubscription: ReturnType; + unsubscribe: ReturnType; + }; + let constants: {IsPushEnabled: BehaviorSubject; VapidPublicKey: BehaviorSubject}; + + beforeEach(() => { + subscriptionSubject = new BehaviorSubject(null); + subscriptionChangeSubject = new Subject(); + + swPush = { + isEnabled: true, + subscription: subscriptionSubject, + pushSubscriptionChanges: subscriptionChangeSubject, + requestSubscription: vi.fn().mockResolvedValue(fakeSubscription()), + unsubscribe: vi.fn().mockResolvedValue(undefined), + }; + constants = { + IsPushEnabled: new BehaviorSubject(true), + VapidPublicKey: new BehaviorSubject('BTestVapidPublicKey'), + }; + + // jsdom has neither, and the service checks for both. + vi.stubGlobal('Notification', {permission: 'default'}); + (window as unknown as {PushManager: unknown}).PushManager = class {}; + + // jsdom has no service worker either. Most tests run as a page the worker + // controls, which is when the service goes through SwPush. + setServiceWorker({controller: {}}); + + TestBed.configureTestingModule({ + providers: [ + PushNotificationService, + {provide: SwPush, useValue: swPush}, + {provide: DoubtfireConstants, useValue: constants}, + provideHttpClient(withXhr(), withInterceptorsFromDi()), + provideHttpClientTesting(), + ], + }); + + service = TestBed.inject(PushNotificationService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + vi.unstubAllGlobals(); + vi.useRealTimers(); + delete (navigator as unknown as {serviceWorker?: unknown}).serviceWorker; + }); + + it('posts the browser registration to the api when subscribing', () => { + service.subscribe().subscribe(); + + // requestSubscription resolves a promise, so the POST is a microtask away. + return Promise.resolve().then(() => { + const request = httpMock.expectOne(`${API_URL}/push_subscriptions`); + + expect(request.request.method).toBe('POST'); + expect(request.request.body).toEqual({ + endpoint: ENDPOINT, + p256dh: 'BFakePublicKey', + auth: 'FakeAuthSecret', + }); + request.flush(null); + }); + }); + + it('asks the browser to subscribe with the key the api supplied', () => { + service.subscribe().subscribe(); + + expect(swPush.requestSubscription).toHaveBeenCalledWith({ + serverPublicKey: 'BTestVapidPublicKey', + }); + + return Promise.resolve().then(() => { + httpMock.expectOne(`${API_URL}/push_subscriptions`).flush(null); + }); + }); + + it('stores a rotated registration before deleting the old endpoint', () => { + const oldSubscription = fakeSubscription(`${ENDPOINT}-old`); + const newSubscription = fakeSubscription( + `${ENDPOINT}-new`, + 'BRotatedPublicKey', + 'RotatedAuthSecret', + ); + + service.start(); + subscriptionChangeSubject.next({oldSubscription, newSubscription}); + + const post = httpMock.expectOne( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'POST', + ); + expect(post.request.body).toEqual({ + endpoint: `${ENDPOINT}-new`, + p256dh: 'BRotatedPublicKey', + auth: 'RotatedAuthSecret', + }); + httpMock.expectNone( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'DELETE', + ); + + post.flush(null); + + const removeOld = httpMock.expectOne( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'DELETE', + ); + expect(removeOld.request.params.get('endpoint')).toBe(`${ENDPOINT}-old`); + removeOld.flush(null); + }); + + it('updates keys in place when rotation keeps the same endpoint', () => { + const oldSubscription = fakeSubscription(); + const newSubscription = fakeSubscription(ENDPOINT, 'BRotatedPublicKey', 'RotatedAuthSecret'); + + service.start(); + subscriptionChangeSubject.next({oldSubscription, newSubscription}); + + const post = httpMock.expectOne( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'POST', + ); + expect(post.request.body).toEqual({ + endpoint: ENDPOINT, + p256dh: 'BRotatedPublicKey', + auth: 'RotatedAuthSecret', + }); + post.flush(null); + + httpMock.expectNone( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'DELETE', + ); + }); + + it('keeps listening after one rotated registration fails to reach the api', () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + + service.start(); + subscriptionChangeSubject.next({ + oldSubscription: fakeSubscription(`${ENDPOINT}-old`), + newSubscription: fakeSubscription(`${ENDPOINT}-failed`), + }); + + httpMock + .expectOne( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'POST', + ) + .flush('nope', {status: 500, statusText: 'Server Error'}); + + subscriptionChangeSubject.next({ + oldSubscription: null, + newSubscription: fakeSubscription(`${ENDPOINT}-later`), + }); + + const later = httpMock.expectOne( + (request) => request.url === `${API_URL}/push_subscriptions` && request.method === 'POST', + ); + expect(later.request.body['endpoint']).toBe(`${ENDPOINT}-later`); + later.flush(null); + + expect(consoleError).toHaveBeenCalledOnce(); + }); + + it('ignores an invalidation when the browser supplies no replacement', () => { + service.start(); + subscriptionChangeSubject.next({ + oldSubscription: fakeSubscription(), + newSubscription: null, + }); + + httpMock.expectNone(`${API_URL}/push_subscriptions`); + }); + + it('starts the rotation listener only once and stops it on teardown', () => { + service.start(); + service.start(); + + subscriptionChangeSubject.next({ + oldSubscription: null, + newSubscription: fakeSubscription(`${ENDPOINT}-once`), + }); + + httpMock.expectOne(`${API_URL}/push_subscriptions`).flush(null); + service.stop(); + + subscriptionChangeSubject.next({ + oldSubscription: null, + newSubscription: fakeSubscription(`${ENDPOINT}-stopped`), + }); + + httpMock.expectNone(`${API_URL}/push_subscriptions`); + }); + + it('deletes on the api before unsubscribing in the browser', () => { + subscriptionSubject.next(fakeSubscription()); + + service.unsubscribe().subscribe(); + + const request = httpMock.expectOne( + (r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE', + ); + + // Order matters. Unsubscribing first would throw the endpoint away and leave + // a row on the api that nothing can ever delete. + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + expect(request.request.params.get('endpoint')).toBe(ENDPOINT); + + request.flush(null); + expect(swPush.unsubscribe).toHaveBeenCalled(); + }); + + it('does nothing when unsubscribing a browser that was never subscribed', () => { + subscriptionSubject.next(null); + + service.unsubscribe().subscribe(); + + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + httpMock.expectNone(`${API_URL}/push_subscriptions`); + }); + + it('returns immediately when the service worker is disabled', () => { + swPush.isEnabled = false; + swPush.subscription = NEVER; + let emitted = false; + + service.unsubscribe().subscribe(() => { + emitted = true; + }); + + expect(emitted).toBe(true); + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + httpMock.expectNone(`${API_URL}/push_subscriptions`); + }); + + // Leaving the browser subscribed because the api call failed is the worse of + // the two outcomes. The row on the server is cleaned up when the push service + // returns 410 for it; a browser still receiving is not cleaned up by anything. + it('still unsubscribes locally when the api delete fails', async () => { + subscriptionSubject.next(fakeSubscription()); + let completed = false; + + service.unsubscribe().subscribe({complete: () => (completed = true)}); + + const request = httpMock.expectOne( + (r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE', + ); + request.flush('nope', {status: 500, statusText: 'Server Error'}); + + expect(swPush.unsubscribe).toHaveBeenCalled(); + + // swPush.unsubscribe() returns a promise, so completion lands a tick later. + await flushMicrotasks(); + expect(completed).toBe(true); + }); + + it('still unsubscribes locally when the api is unreachable', () => { + subscriptionSubject.next(fakeSubscription()); + + service.unsubscribe().subscribe(); + + const request = httpMock.expectOne( + (r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE', + ); + request.error(new ProgressEvent('network error')); + + expect(swPush.unsubscribe).toHaveBeenCalled(); + }); + + // The worker is enabled but controls no page: the dev server has no worker + // file, it registers six seconds after bootstrap, and a hard reload bypasses + // it. SwPush then never answers, and sign out used to wait on it for ever. + it('finishes at once when no service worker controls the page', async () => { + swPush.subscription = NEVER; + setServiceWorker({controller: null, getRegistration: () => Promise.resolve(undefined)}); + let completed = false; + + service.unsubscribe().subscribe({complete: () => (completed = true)}); + + await flushMicrotasks(); + expect(completed).toBe(true); + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + httpMock.expectNone(`${API_URL}/push_subscriptions`); + }); + + it('removes a subscription through the browser when no service worker controls the page', async () => { + swPush.subscription = NEVER; + const subscription = fakeSubscription() as PushSubscription & {unsubscribe: () => unknown}; + const browserUnsubscribe = vi.fn().mockResolvedValue(true); + subscription.unsubscribe = browserUnsubscribe; + setServiceWorker({ + controller: null, + getRegistration: () => + Promise.resolve({pushManager: {getSubscription: () => Promise.resolve(subscription)}}), + }); + let completed = false; + + service.unsubscribe().subscribe({complete: () => (completed = true)}); + await flushMicrotasks(); + + const request = httpMock.expectOne( + (r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE', + ); + expect(request.request.params.get('endpoint')).toBe(ENDPOINT); + expect(browserUnsubscribe).not.toHaveBeenCalled(); + + request.flush(null); + await flushMicrotasks(); + + expect(browserUnsubscribe).toHaveBeenCalledOnce(); + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + expect(completed).toBe(true); + }); + + it('unsubscribeQuietly stops waiting after the time limit', () => { + vi.useFakeTimers(); + swPush.subscription = NEVER; + let completed = false; + + service.unsubscribeQuietly().subscribe({complete: () => (completed = true)}); + + vi.advanceTimersByTime(QUIET_UNSUBSCRIBE_LIMIT_MS - 1); + expect(completed).toBe(false); + + vi.advanceTimersByTime(1); + expect(completed).toBe(true); + }); + + // A stalled api delete must not stop the browser unsubscribing, or the next + // person to sign in on this machine could get this user's notifications. + it('unsubscribeQuietly still unsubscribes the browser when the api delete stalls', () => { + vi.useFakeTimers(); + subscriptionSubject.next(fakeSubscription()); + let completed = false; + + service.unsubscribeQuietly().subscribe({complete: () => (completed = true)}); + + httpMock.expectOne((r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE'); + expect(swPush.unsubscribe).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(QUIET_UNSUBSCRIBE_LIMIT_MS); + expect(swPush.unsubscribe).toHaveBeenCalledOnce(); + + return vi.advanceTimersByTimeAsync(0).then(() => { + expect(completed).toBe(true); + }); + }); + + // Sign out calls this and cannot do anything useful with a failure, so it + // must never throw, even when the browser itself refuses to unsubscribe. + it('unsubscribeQuietly swallows a failure from the browser', async () => { + subscriptionSubject.next(fakeSubscription()); + swPush.unsubscribe.mockRejectedValue(new Error('no service worker')); + let errored = false; + let completed = false; + + service.unsubscribeQuietly().subscribe({ + error: () => (errored = true), + complete: () => (completed = true), + }); + + httpMock + .expectOne((r) => r.url === `${API_URL}/push_subscriptions` && r.method === 'DELETE') + .flush(null); + + await flushMicrotasks(); + expect(errored).toBe(false); + expect(completed).toBe(true); + }); + + it('reports nothing blocking when the service worker is running and keys are set', () => { + expect(service.blocker()).toBeNull(); + }); + + it('reports the service worker as the blocker during the first six seconds', () => { + swPush.isEnabled = false; + + expect(service.blocker()).toBe('no-service-worker'); + }); + + it('reports a blocker when the api has no vapid keys', () => { + constants.IsPushEnabled.next(false); + + expect(service.blocker()).toBe('not-configured'); + }); + + it('reports a blocker when the user has denied notifications', () => { + vi.stubGlobal('Notification', {permission: 'denied'}); + + expect(service.blocker()).toBe('permission-denied'); + }); + + it('reports a blocker when the browser cannot do push at all', () => { + delete (window as unknown as {PushManager?: unknown}).PushManager; + + expect(service.blocker()).toBe('unsupported'); + }); + + describe('detectBrowser', () => { + it('recognises Chrome', () => { + const ua = + 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36'; + expect(detectBrowser(ua)).toBe('chrome'); + }); + + it('recognises Edge even though its user agent also contains Chrome', () => { + const ua = + 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36 Edg/120.0.0.0'; + expect(detectBrowser(ua)).toBe('edge'); + }); + + it('recognises Firefox', () => { + const ua = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:120.0) Gecko/20100101 Firefox/120.0'; + expect(detectBrowser(ua)).toBe('firefox'); + }); + + it('uses the generic fallback for Opera even though its user agent contains Chrome', () => { + const ua = + 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 ' + + '(KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36 OPR/106.0.0.0'; + + expect(detectBrowser(ua)).toBe('other'); + }); + + it('falls back to other for an unrecognised browser', () => { + expect(detectBrowser('SomeUnknownBrowser/1.0')).toBe('other'); + }); + }); + + describe('permissionDeniedInstructions', () => { + it('returns Chrome steps in Chrome', () => { + vi.stubGlobal('navigator', {userAgent: 'Chrome/120.0.0.0'}); + expect(service.permissionDeniedInstructions()).toEqual(PERMISSION_DENIED_INSTRUCTIONS.chrome); + }); + + it('returns Edge steps in Edge', () => { + vi.stubGlobal('navigator', {userAgent: 'Chrome/120.0.0.0 Edg/120.0.0.0'}); + expect(service.permissionDeniedInstructions()).toEqual(PERMISSION_DENIED_INSTRUCTIONS.edge); + }); + + it('returns Firefox steps in Firefox', () => { + vi.stubGlobal('navigator', {userAgent: 'Firefox/120.0'}); + expect(service.permissionDeniedInstructions()).toEqual( + PERMISSION_DENIED_INSTRUCTIONS.firefox, + ); + }); + + it('returns generic steps in Opera', () => { + vi.stubGlobal('navigator', { + userAgent: + 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 ' + + '(KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36 OPR/106.0.0.0', + }); + + expect(service.permissionDeniedInstructions()).toEqual(PERMISSION_DENIED_INSTRUCTIONS.other); + }); + + it('returns generic steps in an unrecognised browser', () => { + vi.stubGlobal('navigator', {userAgent: 'SomeUnknownBrowser/1.0'}); + expect(service.permissionDeniedInstructions()).toEqual(PERMISSION_DENIED_INSTRUCTIONS.other); + }); + }); +}); diff --git a/src/app/api/services/spec/task-comment.service.spec.ts b/src/app/api/services/spec/task-comment.service.spec.ts index ae5673b28b..1d8214f0f2 100644 --- a/src/app/api/services/spec/task-comment.service.spec.ts +++ b/src/app/api/services/spec/task-comment.service.spec.ts @@ -1,5 +1,6 @@ -import {afterEach, beforeEach, describe, expect, it} from 'vitest'; +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; import { + HttpEventType, HttpRequest, provideHttpClient, withInterceptorsFromDi, @@ -7,9 +8,11 @@ import { } from '@angular/common/http'; import {HttpTestingController, provideHttpClientTesting} from '@angular/common/http/testing'; import {TestBed} from '@angular/core/testing'; -import {TaskComment} from 'src/app/api/models/doubtfire-model'; +import {Task, TaskComment} from 'src/app/api/models/doubtfire-model'; import {FileDownloaderService} from 'src/app/common/file-downloader/file-downloader.service'; import {EmojiService} from 'src/app/common/services/emoji.service'; +import {AuthenticationService} from '../authentication.service'; +import {NotificationService} from '../notification.service'; import {TaskCommentService} from '../task-comment.service'; import {TestAttemptService} from '../test-attempt.service'; import {UserService} from '../user.service'; @@ -17,28 +20,127 @@ import {UserService} from '../user.service'; describe('TaskCommentService discussion comments', () => { let taskCommentService: TaskCommentService; let httpMock: HttpTestingController; + let downloader: { + downloadFile: ReturnType; + downloadFileWithFeedback: ReturnType; + }; + let notificationService: NotificationService; + let isAuthenticated: ReturnType; beforeEach(() => { + downloader = {downloadFile: vi.fn(), downloadFileWithFeedback: vi.fn()}; + isAuthenticated = vi.fn().mockReturnValue(true); TestBed.configureTestingModule({ providers: [ TaskCommentService, provideHttpClient(withXhr(), withInterceptorsFromDi()), provideHttpClientTesting(), + {provide: AuthenticationService, useValue: {isAuthenticated}}, {provide: EmojiService, useValue: {}}, {provide: UserService, useValue: {cache: {getOrCreate: () => ({})}}}, - {provide: FileDownloaderService, useValue: {}}, + {provide: FileDownloaderService, useValue: downloader}, {provide: TestAttemptService, useValue: {cache: {getOrCreate: () => ({})}}}, ], }); taskCommentService = TestBed.inject(TaskCommentService); httpMock = TestBed.inject(HttpTestingController); + notificationService = TestBed.inject(NotificationService); + }); + + it('refreshes the bell once after reading comments and uses the server count', () => { + const counts: number[] = []; + const subscription = notificationService.unreadCount$.subscribe((count) => counts.push(count)); + notificationService.refreshUnreadCount().subscribe(); + httpMock.expectOne('http://localhost:3000/api/notifications/unread_count').flush({count: 5}); + + const task = {numNewComments: 2} as Task; + let comments: TaskComment[]; + taskCommentService.query({projectId: 1, taskDefinitionId: 2}, task).subscribe((result) => { + comments = result; + }); + httpMock.expectOne('http://localhost:3000/api/projects/1/task_def_id/2/comments/').flush([]); + + expect(comments).toEqual([]); + expect(task.numNewComments).toBe(0); + httpMock.expectOne('http://localhost:3000/api/notifications/unread_count').flush({count: 3}); + expect(counts).toEqual([0, 5, 3]); + subscription.unsubscribe(); + }); + + it('does not request an unread count for a signed-out user', () => { + isAuthenticated.mockReturnValue(false); + const task = {numNewComments: 2} as Task; + taskCommentService.query({projectId: 1, taskDefinitionId: 2}, task).subscribe(); + httpMock.expectOne('http://localhost:3000/api/projects/1/task_def_id/2/comments/').flush([]); + + expect(task.numNewComments).toBe(0); + httpMock.expectNone('http://localhost:3000/api/notifications/unread_count'); + }); + + it('still returns comments when the badge refresh fails', () => { + const task = {numNewComments: 2} as Task; + const next = vi.fn(); + const error = vi.fn(); + taskCommentService.query({projectId: 1, taskDefinitionId: 2}, task).subscribe({next, error}); + httpMock.expectOne('http://localhost:3000/api/projects/1/task_def_id/2/comments/').flush([]); + httpMock.expectOne('http://localhost:3000/api/notifications/unread_count').flush(null, { + status: 503, + statusText: 'Service Unavailable', + }); + + expect(next).toHaveBeenCalledWith([]); + expect(error).not.toHaveBeenCalled(); + }); + + it('does not refresh the badge or clear unread comments when comments fail to load', () => { + const task = {numNewComments: 2} as Task; + const error = vi.fn(); + taskCommentService.query({projectId: 1, taskDefinitionId: 2}, task).subscribe({error}); + httpMock.expectOne('http://localhost:3000/api/projects/1/task_def_id/2/comments/').flush(null, { + status: 503, + statusText: 'Service Unavailable', + }); + + expect(error).toHaveBeenCalledOnce(); + expect(task.numNewComments).toBe(2); + httpMock.expectNone('http://localhost:3000/api/notifications/unread_count'); }); afterEach(() => { httpMock.verify(); }); + it('loads attachment guidance from the authenticated API policy endpoint', () => { + let received: unknown; + taskCommentService.attachmentPolicy().subscribe((policy) => { + received = policy; + }); + const request = httpMock.expectOne('http://localhost:3000/api/task_comments/upload_policy'); + expect(request.request.method).toBe('GET'); + const policy = { + version: 1, + max_bytes_exclusive: 30_000_000, + max_selection_count: 5, + categories: [], + }; + request.flush(policy); + expect(received).toEqual(policy); + }); + + it('uses the authenticated downloader and forces attachment disposition for generic files', () => { + taskCommentService.downloadAttachment({ + id: 1, + attachmentFileName: 'results.xlsx', + attachmentUrl: + 'http://localhost:3000/api/projects/1/task_def_id/2/comments/1?as_attachment=false', + } as TaskComment); + expect(TestBed.inject(FileDownloaderService).downloadFile).toHaveBeenCalledWith( + 'http://localhost:3000/api/projects/1/task_def_id/2/comments/1?as_attachment=true', + 'results.xlsx', + ); + }); + it('posts a discussion reply without expecting an entity response', () => { const replyAudio = new Blob(['reply audio'], {type: 'audio/webm'}); const comment = { @@ -69,4 +171,62 @@ describe('TaskCommentService discussion comments', () => { expect(completed).toBe(true); }); + + it('uploads a staged attachment with its filename and stable request id but no empty caption', () => { + const attachment = new Blob(['document bytes'], {type: 'application/pdf'}); + const refreshCommentData = vi.fn(); + const task = { + project: {id: 12}, + definition: {id: 34}, + refreshCommentData, + } as never; + const states: Array<{state: string; progress: number}> = []; + + taskCommentService + .uploadStagedAttachment( + task, + attachment, + 'Feedback .pdf', + '', + null, + 'stable-request-123', + ) + .subscribe((state) => states.push(state)); + + const req = httpMock.expectOne('http://localhost:3000/api/projects/12/task_def_id/34/comments'); + expect(req.request.method).toBe('POST'); + const formData = req.request.body as FormData; + const uploaded = formData.get('attachment') as File; + expect(uploaded.name).toBe('Feedback .pdf'); + expect(uploaded.type).toBe('application/pdf'); + expect(formData.get('comment')).toBeNull(); + expect(formData.get('reply_to_id')).toBeNull(); + expect(formData.get('client_request_id')).toBe('stable-request-123'); + + req.event({type: HttpEventType.UploadProgress, loaded: 5, total: 10}); + req.flush({id: 99}); + + expect(states).toEqual([ + {state: 'progress', progress: 50}, + {state: 'complete', progress: 100}, + ]); + expect(refreshCommentData).toHaveBeenCalledOnce(); + }); + + it('uses the shared download feedback lifecycle for a feedback attachment', () => { + const comment = { + id: 77, + attachmentUrl: 'http://localhost:3000/api/comments/77?as_attachment=false', + attachmentFileName: 'Tutor feedback.docx', + } as TaskComment; + + taskCommentService.downloadCommentAttachment(comment); + + expect(downloader.downloadFileWithFeedback).toHaveBeenCalledOnce(); + expect(downloader.downloadFileWithFeedback).toHaveBeenCalledWith( + 'http://localhost:3000/api/comments/77?as_attachment=true', + 'Tutor feedback.docx', + {requestKey: 'task-comment-attachment-77'}, + ); + }); }); diff --git a/src/app/api/services/task-comment.service.ts b/src/app/api/services/task-comment.service.ts index f5dd11e222..32300cd284 100644 --- a/src/app/api/services/task-comment.service.ts +++ b/src/app/api/services/task-comment.service.ts @@ -1,8 +1,8 @@ import {CachedEntityService, RequestOptions} from 'ngx-entity-service'; -import {HttpClient} from '@angular/common/http'; +import {HttpClient, HttpEventType} from '@angular/common/http'; import {EventEmitter, Injectable} from '@angular/core'; import {Observable} from 'rxjs'; -import {tap} from 'rxjs/operators'; +import {filter, map, tap} from 'rxjs/operators'; import { ScormComment, Task, @@ -13,10 +13,18 @@ import { import {FileDownloaderService} from 'src/app/common/file-downloader/file-downloader.service'; import {EmojiService} from 'src/app/common/services/emoji.service'; import API_URL from 'src/app/config/constants/apiUrl'; +import {AttachmentPolicy} from '../models/task-comment/attachment-policy'; import {DiscussionComment} from '../models/task-comment/discussion-comment'; import {ExtensionComment} from '../models/task-comment/extension-comment'; import {ScormExtensionComment} from '../models/task-comment/scorm-extension-comment'; +import {AuthenticationService} from './authentication.service'; import {MappingFunctions} from './mapping-fn'; +import {NotificationService} from './notification.service'; + +export interface AttachmentUploadState { + state: 'progress' | 'complete'; + progress: number; +} @Injectable() export class TaskCommentService extends CachedEntityService { @@ -47,6 +55,8 @@ export class TaskCommentService extends CachedEntityService { private userService: UserService, private downloader: FileDownloaderService, private testAttemptService: TestAttemptService, + private authService: AuthenticationService, + private notificationService: NotificationService, ) { super(apiHttpClient, API_URL); @@ -70,6 +80,10 @@ export class TaskCommentService extends CachedEntityService { 'recipientReadTime', 'replyToId', 'isNew', + 'automated', + 'attachmentFileName', + 'attachmentMimeType', + 'attachmentByteSize', { keys: ['text', 'comment'], toEntityFn: (data, _key, _entity) => { @@ -165,16 +179,33 @@ export class TaskCommentService extends CachedEntityService { // Access the task and set the number of new comments to 0 - they are now read on the server const task = other as Task; task.numNewComments = 0; + if (this.authService.isAuthenticated()) { + // Comment reads also mark their notifications read on the server. + // A failed badge refresh must not prevent the comments from loading. + this.notificationService.refreshUnreadCount().subscribe({error: () => undefined}); + } }), ); } + public attachmentPolicy(): Observable { + return this.apiHttpClient.get(`${API_URL}/task_comments/upload_policy`); + } + + public downloadAttachment(comment: TaskComment): void { + this.downloader.downloadFile( + comment.attachmentUrl.replace('as_attachment=false', 'as_attachment=true'), + comment.attachmentFileName || `comment-${comment.id}`, + ); + } + public addComment( task: Task, data: string | File | Blob, commentType: string, originalComment?: TaskComment, prompts?: Blob[], + clientRequestId?: string, ): Observable { const pathId = { projectId: task.project.id, @@ -185,6 +216,9 @@ export class TaskCommentService extends CachedEntityService { if (originalComment) { body.append('reply_to_id', originalComment?.id.toString()); } + if (clientRequestId) { + body.append('client_request_id', clientRequestId); + } const opts: RequestOptions = {endpointFormat: this.commentEndpointFormat}; @@ -212,6 +246,53 @@ export class TaskCommentService extends CachedEntityService { ); } + /** + * Upload one staged attachment and its optional caption as a single comment. + * clientRequestId is stable for the life of the staged item, so retrying after + * a lost response cannot create a second server comment. + */ + public uploadStagedAttachment( + task: Task, + attachment: File | Blob, + fileName: string, + caption: string, + originalComment: TaskComment | null, + clientRequestId: string, + ): Observable { + const body = new FormData(); + body.append('attachment', attachment, fileName); + if (caption.replace(/\s+/g, '').length > 0) { + body.append('comment', caption); + } + if (originalComment) { + body.append('reply_to_id', originalComment.id.toString()); + } + body.append('client_request_id', clientRequestId); + + const url = `${API_URL}/projects/${task.project.id}/task_def_id/${task.definition.id}/comments`; + return this.apiHttpClient + .post(url, body, {observe: 'events', reportProgress: true}) + .pipe( + map((event): AttachmentUploadState | null => { + if (event.type === HttpEventType.UploadProgress) { + const total = event.total ?? attachment.size; + const progress = total > 0 ? Math.round((100 * event.loaded) / total) : 0; + return {state: 'progress', progress}; + } + if (event.type === HttpEventType.Response) { + return {state: 'complete', progress: 100}; + } + return null; + }), + filter((state): state is AttachmentUploadState => state !== null), + tap((state) => { + if (state.state === 'complete') { + task.refreshCommentData(); + } + }), + ); + } + public assessExtension(extension: ExtensionComment): Observable { const opts: RequestOptions = { endpointFormat: this.extensionGrantEndpointFormat, @@ -321,6 +402,14 @@ export class TaskCommentService extends CachedEntityService { ); } + public downloadCommentAttachment(comment: TaskComment): void { + this.downloader.downloadFileWithFeedback( + comment.attachmentUrl.replace('as_attachment=false', 'as_attachment=true'), + comment.attachmentFileName || `comment-${comment.id}`, + {requestKey: `task-comment-attachment-${comment.id}`}, + ); + } + // public getDiscussionComment() -> // DiscussionComment.getDiscussion.get {project_id: task.project.id, task_definition_id: task.definition.id, task_comment_id: commentID}, // (response) -> #success) diff --git a/src/app/app.component.spec.ts b/src/app/app.component.spec.ts new file mode 100644 index 0000000000..01b3c3b6a1 --- /dev/null +++ b/src/app/app.component.spec.ts @@ -0,0 +1,57 @@ +// MN-C03 and MN-C05 targeted startup and teardown test. +import {describe, expect, it, vi} from 'vitest'; +import {Renderer2} from '@angular/core'; +import {Router} from '@angular/router'; +import {Subject} from 'rxjs'; +import {PushNotificationClickService} from 'src/app/api/services/push-notification-click.service'; +import {PushNotificationService} from 'src/app/api/services/push-notification.service'; +import {AppLifecycleService} from 'src/app/common/services/app-lifecycle.service'; +import {AppComponent} from './app.component'; +import {ThemeService} from './common/theme/theme.service'; + +describe('AppComponent push lifecycle', () => { + it('starts push listeners at app startup and stops them at teardown', () => { + const events: Subject = new Subject(); + const router = { + url: '/home', + events: events.asObservable(), + } as unknown as Router; + const renderer = { + setStyle: vi.fn(), + } as unknown as Renderer2; + const clickRouting = { + start: vi.fn(), + stop: vi.fn(), + } as unknown as PushNotificationClickService; + const pushNotifications = { + start: vi.fn(), + stop: vi.fn(), + } as unknown as PushNotificationService; + const appLifecycle = { + start: vi.fn(), + stop: vi.fn(), + } as unknown as AppLifecycleService; + + const theme = {} as unknown as ThemeService; + + const component = new AppComponent( + router, + renderer, + clickRouting, + pushNotifications, + theme, + appLifecycle, + ); + component.ngOnInit(); + + expect(appLifecycle.start).toHaveBeenCalledOnce(); + expect(clickRouting.start).toHaveBeenCalledOnce(); + expect(pushNotifications.start).toHaveBeenCalledOnce(); + + component.ngOnDestroy(); + + expect(appLifecycle.stop).toHaveBeenCalledOnce(); + expect(clickRouting.stop).toHaveBeenCalledOnce(); + expect(pushNotifications.stop).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/app/app.component.ts b/src/app/app.component.ts index b5d35f7f32..79a8e52e7e 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,6 +1,10 @@ import {ChangeDetectionStrategy, Component, OnDestroy, OnInit, Renderer2} from '@angular/core'; import {NavigationEnd, Router} from '@angular/router'; import {Subscription, filter} from 'rxjs'; +import {PushNotificationClickService} from 'src/app/api/services/push-notification-click.service'; +import {PushNotificationService} from 'src/app/api/services/push-notification.service'; +import {AppLifecycleService} from 'src/app/common/services/app-lifecycle.service'; +import {ThemeService} from './common/theme/theme.service'; @Component({ selector: 'app-root', @@ -14,9 +18,18 @@ export class AppComponent implements OnInit, OnDestroy { constructor( private router: Router, private renderer: Renderer2, + private pushNotificationClicks: PushNotificationClickService, + private pushNotifications: PushNotificationService, + // Construct the theme service at startup so every screen, including phones, + // receives the resolved theme before a theme toggle is rendered. + private theme: ThemeService, + private appLifecycle: AppLifecycleService, ) {} ngOnInit(): void { + this.appLifecycle.start(); + this.pushNotificationClicks.start(); + this.pushNotifications.start(); this.setBodyBackground(this.router.url); this.routerSub = this.router.events .pipe(filter((event) => event instanceof NavigationEnd)) @@ -24,12 +37,21 @@ export class AppComponent implements OnInit, OnDestroy { } ngOnDestroy(): void { + this.appLifecycle.stop(); + this.pushNotificationClicks.stop(); + this.pushNotifications.stop(); this.routerSub?.unsubscribe(); } private setBodyBackground(url: string): void { const path = url.split('?')[0].split('#')[0]; - const background = path === '/home' || path === '/' ? '#f5f5f5' : '#fff'; + // THM-M01: was hardcoded #f5f5f5 / #fff set inline, which beats any stylesheet + // and never flipped in dark. Onto the tokens (with a legacy fallback) so they + // follow the resolved-theme marker. + const background = + path === '/home' || path === '/' + ? 'var(--ot-color-page, #f5f5f5)' + : 'var(--ot-color-surface, #fff)'; this.renderer.setStyle(document.body, 'background-color', background); } } diff --git a/src/app/common/header/notification-bell/notification-bell.component.html b/src/app/common/header/notification-bell/notification-bell.component.html new file mode 100644 index 0000000000..49fbc71787 --- /dev/null +++ b/src/app/common/header/notification-bell/notification-bell.component.html @@ -0,0 +1,139 @@ + + + +
+ + @if (unreadCount) { + + } +
+ + + @if (recent.length) { + @if (loadFailed) { +
+ These may be out of date, the last refresh did not work. +
+ } + + @for (notification of recent; track notification.id) { +
+ + + +
+ } + + + + } @else if (loading) { +
+ + + Loading your notifications + +
+ } @else if (loadFailed) { + +
+

We could not load your notifications.

+

Close this and open it again to try once more.

+
+ } @else { +
+

You are all caught up.

+

Anything new will show up here.

+
+ } +
diff --git a/src/app/common/header/notification-bell/notification-bell.component.scss b/src/app/common/header/notification-bell/notification-bell.component.scss new file mode 100644 index 0000000000..493f6562c7 --- /dev/null +++ b/src/app/common/header/notification-bell/notification-bell.component.scss @@ -0,0 +1,276 @@ +// The menu panel is rendered into the cdk overlay container, outside this +// component's element, so component scoped styles never reach it. ::ng-deep on +// the panel class is how unit-dropdown next door sizes its own menu. Everything +// below that line is inside the component's own template, overlay or not, so it +// keeps its encapsulation attribute and needs no ::ng-deep. +// +// The width is a preference and not a floor. Material's own panel sets +// max-width: 280px, which is what this is here to override, but a fixed +// min-width would make the panel wider than a 320px phone and push the page +// sideways. +::ng-deep .notification-dropdown-menu.mat-mdc-menu-panel { + border-radius: var(--ot-radius-md); + max-width: min(380px, calc(100vw - 32px)); + min-width: 0; + // Material's own container shape is 4px. Without clipping, the first and last + // rows square the corners back off again on hover. + overflow: hidden; + width: 380px; +} + +// on a phone the panel is wider than the space left of the bell, so the overlay +// pushes it back on screen. the open animation still grew from where it would +// have been, which read as the menu flying in from the wrong spot. grow it from +// the top right, under the bell, instead +@media (max-width: 599px) { + ::ng-deep .notification-dropdown-menu.mat-mdc-menu-panel { + transform-origin: top right !important; + max-width: calc(100vw - 16px); + width: calc(100vw - 16px); + } +} + +// Put the count on the bell. +// +// Material anchors the badge at the host's outside top right corner, with +// `bottom: 100%` and `left: 100%`, and then pulls it back with a negative +// margin from one of these two tokens. Which token depends on whether the badge +// overlaps, so both are set: leaving either at the theme's -11px leaves the +// number sitting beside the bell rather than on it, which is what the default +// looks like on a 40px icon button. +// +// matBadgeSize="small" is also gone from the template. It is a 16px container, +// fine for a one digit job count and cramped for a two digit unread count; the +// default 22px is the readable one, and a bigger badge needs pulling in further +// to sit on the glyph. +:host { + --mat-badge-container-offset: -20px -20px; + --mat-badge-container-overlap-offset: -20px; + + // The toolbar is a plain flex row with no wrapping, so anything in it that + // can grow pushes the rest towards the edge. This one never does. + // + // The QR action now lives in the profile menu, leaving this fixed-width + // notification entry reachable at every toolbar size. + flex: 0 0 auto; +} + +@media (max-width: 599px) { + .notification-bell-button { + height: 44px; + width: 44px; + } +} + +// A ring in the toolbar's own colour, so the badge cuts itself out from the +// glyph it sits on top of. +// +// A box-shadow and not a border. Material sets box-sizing: border-box on the +// badge and gives it a fixed 22px box with a 22px line-height, so a 2px border +// takes the content box down to 18px while the line box stays 22 and the digit +// is shoved off centre. A shadow draws the same ring and costs no layout. +:host ::ng-deep .mat-badge-content { + box-shadow: 0 0 0 2px var(--ot-color-page); + font-weight: 700; +} + +// mat-menu-item is built for one line of text. It pins itself to 48px, and it +// wraps whatever you put in it in a .mat-mdc-menu-item-text span that clips and +// refuses to wrap. That span is Material's own, so it carries no encapsulation +// attribute and ::ng-deep is the only way to reach it. Without this the row lays +// itself out inside a box that is not the width it looks, which is what pushes +// the glyph off centre. +.notification-row.mat-mdc-menu-item { + height: auto; + line-height: 1.35; + min-height: 0; + padding: 12px 10px 12px 16px; + white-space: normal; + + ::ng-deep .mat-mdc-menu-item-text { + display: block; + overflow: visible; + white-space: normal; + width: 100%; + } +} + +// --------------------------------------------------------------------------- +// Shared row shape. The full notifications page carries a copy of this block, +// deliberately: the two components are on sibling branches so neither can +// import from the other yet, and a row has to look the same in both places. +// Both copies belong in one stylesheet once they have merged. +// --------------------------------------------------------------------------- +.notification-line { + align-items: center; + display: flex; + gap: 12px; + width: 100%; +} + +.notification-body { + display: flex; + flex: 1 1 auto; + flex-direction: column; + gap: 2px; + min-width: 0; + text-align: left; +} + +.notification-meta { + align-items: baseline; + color: var(--ot-color-text-muted); + display: flex; + flex-wrap: wrap; + font-size: 12px; + gap: 4px; + line-height: 1.3; +} + +.notification-kind { + font-weight: 600; +} + +.notification-glyph { + align-items: center; + border-radius: 50%; + display: inline-flex; + flex: 0 0 36px; + height: 36px; + justify-content: center; + width: 36px; + + // mat-icon ships as a 24px inline-block with a 24px line box, and picks up + // margins from Material's list and menu classes. Left alone the glyph is + // nearly as wide as the circle and sits low and left in it, because an inline + // box aligns on its baseline and the margins push it. Making it a block with + // a line-height equal to its own height and no margin takes both out, and the + // flex centring above then does what it looks like it does. + .mat-icon { + // Explicitly, not by inheritance. Material colours icons inside a menu item + // with .mat-mdc-menu-item .mat-icon { color: ... }, so a tone set on the + // circle around it is simply overridden and the glyph comes out black in + // the dropdown while the same glyph is coloured on the page. + color: inherit; + display: block; + font-size: 20px; + height: 20px; + line-height: 20px; + margin: 0; + overflow: visible; + width: 20px; + } + + // One pair per category in Notification::TYPES. The tints are the same hue as + // the glyph at low alpha, so the circle reads as a wash rather than a second + // colour to remember. Theme tokens, so the glyphs stay readable in dark mode. + // + // Feedback takes the link blue, not primary. Dark primary is a fill colour and + // falls to about 2:1 as a glyph on its own wash. In light the two are the same. + &.tone-feedback { + background: color-mix(in srgb, var(--ot-color-link) 12%, transparent); + color: var(--ot-color-link); + } + + &.tone-task { + background: color-mix(in srgb, var(--ot-color-warning) 12%, transparent); + color: var(--ot-color-warning); + } + + &.tone-portfolio { + background: color-mix(in srgb, var(--ot-color-info) 12%, transparent); + color: var(--ot-color-info); + } + + &.tone-extension { + background: color-mix(in srgb, var(--ot-color-success) 12%, transparent); + color: var(--ot-color-success); + } + + &.tone-general { + background: var(--ot-color-hover); + color: var(--ot-color-text-muted); + } + + // Info is already portfolio, so the Unit Hub takes the selected pair, which + // the theme keeps readable against each other in light and dark. + &.tone-unit-hub { + background: var(--ot-color-selected); + color: var(--ot-color-selected-text); + } +} + +.notification-message { + color: var(--ot-color-text); + font-size: 14px; + line-height: 1.35; + white-space: normal; + + // Unread is carried by weight and by the dot, not by colour on its own, and + // the row's aria-label says it in words for anyone who can see neither. + &.unread { + font-weight: 600; + } +} + +.notification-unread-dot { + background: var(--ot-color-primary); + border-radius: 50%; + flex: 0 0 7px; + height: 7px; + width: 7px; + + &.is-read { + visibility: hidden; + } +} + +.notification-time, +.notification-stale { + color: var(--ot-color-text-muted); + font-size: 12px; + line-height: 1.3; +} + +.notification-empty { + color: var(--ot-color-text-muted); + font-size: 14px; +} + +.notification-mark-all.mat-mdc-menu-item { + flex: 0 0 auto; + font-size: 12px; + min-height: 48px; + padding: 0 12px; + width: auto; +} + +.notification-see-all { + font-size: 12px; +} + +// Mark all read and Delete are real menu items so MatMenu's arrow-key manager +// can reach them. Keep Delete compact beside the notification row while +// retaining a 48px keyboard and pointer target. +.notification-delete.mat-mdc-menu-item { + flex: 0 0 48px; + justify-content: center; + min-height: 48px; + padding: 0; + width: 48px; + + // MatMenu renders an empty text wrapper after an icon-only item. Remove it + // so the close icon can sit in the true centre of the button. + ::ng-deep .mat-mdc-menu-item-text { + display: none; + } + + .mat-icon { + display: block; + font-size: 18px; + height: 18px; + line-height: 18px; + margin: 0; + width: 18px; + } +} diff --git a/src/app/common/header/notification-bell/notification-bell.component.spec.ts b/src/app/common/header/notification-bell/notification-bell.component.spec.ts new file mode 100644 index 0000000000..ba1c286efd --- /dev/null +++ b/src/app/common/header/notification-bell/notification-bell.component.spec.ts @@ -0,0 +1,913 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {OverlayContainer} from '@angular/cdk/overlay'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {MatBadge, MatBadgeModule} from '@angular/material/badge'; +import {MatButtonModule} from '@angular/material/button'; +import {MatIconModule} from '@angular/material/icon'; +import {MatListModule} from '@angular/material/list'; +import {MatMenuModule} from '@angular/material/menu'; +import {MatProgressSpinnerModule} from '@angular/material/progress-spinner'; +import {MatTooltipModule} from '@angular/material/tooltip'; +import {By} from '@angular/platform-browser'; +import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {NavigationEnd, Router} from '@angular/router'; +import {BehaviorSubject, Observable, Subject, config, defer, of, throwError} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {AuthenticationService} from 'src/app/api/services/authentication.service'; +import {NotificationService} from 'src/app/api/services/notification.service'; +import {NotificationOpenService} from 'src/app/common/notifications/notification-open.service'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {expectAccessible} from 'src/app/common/testing/accessibility'; +import {ConfirmationModalService} from '../../modals/confirmation-modal/confirmation-modal.service'; +import {NotificationBellComponent} from './notification-bell.component'; + +/** + * The bell has three jobs and each one is a way for it to be wrong: show the + * number the service holds, show nothing at all when that number is zero, and + * ask again as the user moves around the app. The two after those cover the + * ones that are easy to leave out, a count request going out after sign out and + * one still going out after the component is gone. + * + * The dropdown tests open the real menu and read the real overlay rather than + * poking at the component's fields, because "handle the empty state" is a + * question about what is on the screen. The one that matters most is the pair + * about failure: a list that came back empty and a list that failed to come + * back look identical from the outside and mean opposite things. + */ +describe('NotificationBellComponent', () => { + let component: NotificationBellComponent; + let fixture: ComponentFixture; + let overlayContainer: OverlayContainer; + + let unreadCount: BehaviorSubject; + let routerEvents: Subject; + let list: Subject; + let subscribedTo: Set; + let notificationService: { + unreadCount$: BehaviorSubject; + refreshUnreadCount: ReturnType; + list: ReturnType; + markRead: ReturnType; + markAllRead: ReturnType; + remove: ReturnType; + }; + let completeAuthentication: (authenticated: boolean) => void; + + let authenticationService: { + isAuthenticated: ReturnType; + afterAuthCall: ReturnType; + }; + let confirmationModal: {show: ReturnType}; + let alerts: {success: ReturnType; error: ReturnType}; + let router: {events: unknown; navigateByUrl: ReturnType}; + let notificationOpener: {open: ReturnType}; + + /** + * A stand-in that records whether anybody actually subscribed. + * + * A spy that only counts calls is happy either way, and every one of these + * methods returns a cold observable, so dropping the .subscribe() in the + * component sends no request at all while every call count stays the same. + */ + const answers = (name: string, value: T): Observable => + defer(() => { + subscribedTo.add(name); + return of(value); + }); + + const notification = (id: number, over: Partial = {}): Notification => { + const built = new Notification(); + built.id = id; + built.message = `notification ${id}`; + built.link = `/projects/${id}/dashboard`; + built.notificationType = 'feedback'; + built.readAt = null; + built.createdAt = new Date(2026, 7, 17, 9, 0, 0); + return Object.assign(built, over); + }; + + const bell = (): HTMLElement => fixture.nativeElement.querySelector('button'); + + // What is drawn, not what was bound. Material always renders the content + // span and hides the whole badge with a class on the host, so the number on + // screen and whether anyone can see it are two separate questions. + const badgeText = (): string => + fixture.nativeElement.querySelector('.mat-badge-content').textContent.trim(); + + const badgeIsHidden = (): boolean => + fixture.debugElement + .query(By.directive(MatBadge)) + .nativeElement.classList.contains('mat-badge-hidden'); + + const openMenu = (): void => { + bell().click(); + fixture.detectChanges(); + }; + + // Reads the list a second time without going through the trigger, which is + // what reopening the menu does. + const reopenWith = (): Subject => { + const refresh: Subject = new Subject(); + notificationService.list.mockReturnValue(refresh); + component.onMenuOpened(); + fixture.detectChanges(); + return refresh; + }; + + // The menu panel is not inside the component, it is in the cdk overlay + // container, so nothing in the dropdown can be found through the fixture. + const panel = (): HTMLElement => overlayContainer.getContainerElement(); + + const rows = (): HTMLElement[] => Array.from(panel().querySelectorAll('.notification-row')); + + const rowText = (): string[] => + rows().map((row) => row.querySelector('.notification-message').textContent.trim()); + + const glyphIcons = (): string[] => + Array.from(panel().querySelectorAll('.notification-glyph mat-icon')).map((icon) => + icon.textContent.trim(), + ); + + // The tone class, not the computed colour. jsdom applies no stylesheet, and + // the class is the contract between the template and the scss anyway. + const glyphTones = (): string[] => + Array.from(panel().querySelectorAll('.notification-glyph')).map((glyph) => + Array.from(glyph.classList).find((name) => name.startsWith('tone-')), + ); + + const placeholderText = (): string => + Array.from(panel().querySelectorAll('.notification-empty')) + .map((element) => element.textContent.trim()) + .join(' '); + + beforeEach(async () => { + unreadCount = new BehaviorSubject(0); + routerEvents = new Subject(); + list = new Subject(); + subscribedTo = new Set(); + + notificationService = { + unreadCount$: unreadCount, + refreshUnreadCount: vi.fn(() => answers('refreshUnreadCount', 0)), + list: vi.fn().mockReturnValue(list), + markRead: vi.fn(() => answers('markRead', undefined)), + markAllRead: vi.fn(() => answers('markAllRead', undefined)), + remove: vi.fn(() => answers('remove', undefined)), + }; + completeAuthentication = () => undefined; + + authenticationService = { + isAuthenticated: vi.fn().mockReturnValue(true), + afterAuthCall: vi.fn((callback: (authenticated: boolean) => void) => { + completeAuthentication = callback; + }), + }; + confirmationModal = {show: vi.fn()}; + alerts = {success: vi.fn(), error: vi.fn()}; + router = {events: routerEvents.asObservable(), navigateByUrl: vi.fn()}; + notificationOpener = {open: vi.fn().mockResolvedValue(true)}; + + await TestBed.configureTestingModule({ + declarations: [NotificationBellComponent], + imports: [ + MatBadgeModule, + MatButtonModule, + MatIconModule, + MatListModule, + MatMenuModule, + MatProgressSpinnerModule, + MatTooltipModule, + NoopAnimationsModule, + ], + providers: [ + {provide: NotificationService, useValue: notificationService}, + {provide: AuthenticationService, useValue: authenticationService}, + {provide: ConfirmationModalService, useValue: confirmationModal}, + {provide: AlertService, useValue: alerts}, + {provide: Router, useValue: router}, + {provide: NotificationOpenService, useValue: notificationOpener}, + ], + }).compileComponents(); + + overlayContainer = TestBed.inject(OverlayContainer); + fixture = TestBed.createComponent(NotificationBellComponent); + component = fixture.componentInstance; + }); + + afterEach(() => { + fixture.destroy(); + }); + + it('shows how many are unread', () => { + unreadCount.next(3); + fixture.detectChanges(); + + expect(badgeText()).toBe('3'); + expect(badgeIsHidden()).toBe(false); + + // The service is the only source of the number, so a change made anywhere + // in the app has to reach the bell without it asking. + unreadCount.next(4); + fixture.detectChanges(); + + expect(badgeText()).toBe('4'); + }); + + it('shows no badge at all when nothing is unread', () => { + fixture.detectChanges(); + + // Not a badge reading zero. A zero sitting on the bell is a permanent + // little alarm that never means anything. + expect(badgeIsHidden()).toBe(true); + + unreadCount.next(1); + fixture.detectChanges(); + + expect(badgeIsHidden()).toBe(false); + }); + + it('keeps a large unread count compact while announcing the exact count', () => { + unreadCount.next(137); + fixture.detectChanges(); + + expect(badgeText()).toBe('99+'); + expect(bell().getAttribute('aria-label')).toBe('Notifications, 137 unread'); + }); + + it('says how many are unread to a screen reader too', () => { + fixture.detectChanges(); + + expect(bell().getAttribute('aria-label')).toBe('Notifications'); + + unreadCount.next(4); + fixture.detectChanges(); + + // Material renders the badge into a span marked aria-hidden, so without + // this the count is the one thing the control exists to say and the only + // thing it does not say. + expect(bell().getAttribute('aria-label')).toBe('Notifications, 4 unread'); + }); + + it('asks for the count again after every navigation', () => { + fixture.detectChanges(); + + // Once on init, because unreadCount$ starts at zero whether or not that is + // true. + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(1); + expect(subscribedTo.has('refreshUnreadCount')).toBe(true); + + routerEvents.next(new NavigationEnd(1, '/home', '/home')); + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(2); + + routerEvents.next(new NavigationEnd(2, '/units/1/tasks/inbox', '/units/1/tasks/inbox')); + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(3); + }); + + it('does not ask for the count when nobody is signed in', () => { + authenticationService.isAuthenticated.mockReturnValue(false); + + fixture.detectChanges(); + routerEvents.next(new NavigationEnd(1, '/sign_in', '/sign_in')); + + // Sign out hides the header and routes, and the hiding takes a change + // detection pass while the routing event does not, so this component can + // still be alive for it. Anonymous, the request comes back 403, and + // HttpErrorInterceptor treats a 403 on an anonymous user as an expired + // session: an "Authentication timed out" alert and a redirect to /timeout. + expect(notificationService.refreshUnreadCount).not.toHaveBeenCalled(); + }); + + it('stops asking once it is destroyed', () => { + fixture.detectChanges(); + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(1); + + fixture.destroy(); + routerEvents.next(new NavigationEnd(1, '/home', '/home')); + + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(1); + }); + + it('waits for restored authentication before the initial refresh', () => { + authenticationService.isAuthenticated.mockReturnValue(false); + + fixture.detectChanges(); + + expect(authenticationService.afterAuthCall).toHaveBeenCalledTimes(1); + expect(notificationService.refreshUnreadCount).not.toHaveBeenCalled(); + + authenticationService.isAuthenticated.mockReturnValue(true); + completeAuthentication(true); + + expect(notificationService.refreshUnreadCount).toHaveBeenCalledTimes(1); + expect(subscribedTo.has('refreshUnreadCount')).toBe(true); + }); + + it('does not refresh after delayed authentication once destroyed', () => { + authenticationService.isAuthenticated.mockReturnValue(false); + + fixture.detectChanges(); + fixture.destroy(); + + authenticationService.isAuthenticated.mockReturnValue(true); + completeAuthentication(true); + + expect(notificationService.refreshUnreadCount).not.toHaveBeenCalled(); + }); + + it('handles a count request that fails instead of leaving it unhandled', async () => { + // rxjs does not throw an unhandled subscriber error where the subscribe + // call was, it hands it to config.onUnhandledError a macrotask later, and + // the default for that is to rethrow globally. Swapping it for a spy is the + // only way to see the difference from inside a test, and without it this + // passes whether or not the error is handled at all. + const unhandled = vi.fn(); + const previous = config.onUnhandledError; + config.onUnhandledError = unhandled; + + try { + notificationService.refreshUnreadCount.mockReturnValue( + throwError(() => new Error('unread_count failed')), + ); + + fixture.detectChanges(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(unhandled).not.toHaveBeenCalled(); + expect(component.unreadCount).toBe(0); + } finally { + config.onUnhandledError = previous; + } + }); + + describe('the dropdown', () => { + beforeEach(() => { + fixture.detectChanges(); + }); + + const menu = (): HTMLElement => panel().querySelector('[role="menu"]'); + + const expectMenuStructure = (): void => { + expect(menu().getAttribute('aria-label')).toBe('Notifications'); + expect(menu().querySelector('h1, h2, h3, h4, h5, h6, [role="heading"]')).toBeNull(); + const content = menu().querySelector('.mat-mdc-menu-content'); + for (const child of Array.from(content.children)) { + expect(['none', 'menuitem']).toContain(child.getAttribute('role')); + } + for (const wrapper of Array.from(menu().querySelectorAll('[role="none"]'))) { + // A presentational wrapper must not hide its interactive descendants. + expect(wrapper.getAttribute('aria-hidden')).not.toBe('true'); + } + }; + + it('names the menu and exposes loading as an unavailable menu item', () => { + openMenu(); + + expectMenuStructure(); + const state = menu().querySelector('[role="menuitem"][aria-disabled="true"]'); + expect(state.textContent).toContain('Loading your notifications'); + expect(state.querySelector('mat-spinner').getAttribute('aria-hidden')).toBe('true'); + }); + + it('keeps empty and failed states accessible without adding headings or actions', () => { + openMenu(); + list.next([]); + fixture.detectChanges(); + + expectMenuStructure(); + expect(menu().querySelector('[role="menuitem"][aria-disabled="true"]').textContent).toContain( + 'You are all caught up.', + ); + + const refresh = reopenWith(); + refresh.error(new Error('offline')); + fixture.detectChanges(); + + expectMenuStructure(); + expect(menu().querySelector('[role="menuitem"][aria-disabled="true"]').textContent).toContain( + 'We could not load your notifications.', + ); + }); + + it('preserves menu structure when a refresh fails with existing rows', () => { + openMenu(); + list.next([notification(1)]); + fixture.detectChanges(); + const refresh = reopenWith(); + refresh.error(new Error('offline')); + fixture.detectChanges(); + + expectMenuStructure(); + expect(menu().querySelector('[aria-disabled="true"]').textContent).toContain('out of date'); + expect(rows()).toHaveLength(1); + }); + + it('moves through every action with arrow keys and restores the bell on Escape', async () => { + notificationService.list.mockReturnValue(of([notification(1)])); + unreadCount.next(1); + fixture.detectChanges(); + bell().focus(); + bell().dispatchEvent( + new KeyboardEvent('keydown', {key: 'Enter', keyCode: 13, bubbles: true}), + ); + // jsdom does not synthesize the native button click from Enter. + bell().click(); + fixture.detectChanges(); + await fixture.whenStable(); + + expectMenuStructure(); + const actions = Array.from(menu().querySelectorAll('[role="menuitem"]')); + expect(actions).toHaveLength(4); + expect(document.activeElement).toBe(actions[0]); + for (const next of [...actions.slice(1), actions[0]]) { + document.activeElement.dispatchEvent( + new KeyboardEvent('keydown', {key: 'ArrowDown', keyCode: 40, bubbles: true}), + ); + fixture.detectChanges(); + expect(document.activeElement).toBe(next); + } + document.activeElement.dispatchEvent( + new KeyboardEvent('keydown', {key: 'ArrowUp', keyCode: 38, bubbles: true}), + ); + fixture.detectChanges(); + expect(document.activeElement).toBe(actions[3]); + + document.activeElement.dispatchEvent( + new KeyboardEvent('keydown', {key: 'Escape', keyCode: 27, bubbles: true}), + ); + fixture.detectChanges(); + await fixture.whenStable(); + expect(menu()).toBeNull(); + expect(document.activeElement).toBe(bell()); + }); + + it('does not request the list before authentication is ready', () => { + authenticationService.isAuthenticated.mockReturnValue(false); + + openMenu(); + + expect(notificationService.list).not.toHaveBeenCalled(); + expect(placeholderText()).toContain('We could not load your notifications'); + }); + + it('reads the list every time it is opened', () => { + expect(notificationService.list).not.toHaveBeenCalled(); + + openMenu(); + expect(notificationService.list).toHaveBeenCalledTimes(1); + + list.next([notification(1)]); + fixture.detectChanges(); + + // Closing and opening again has to go back to the api. A list fetched + // once at sign in would be wrong for the rest of the session. + component.onMenuOpened(); + expect(notificationService.list).toHaveBeenCalledTimes(2); + }); + + it('shows the newest few, newest first', () => { + openMenu(); + + const at = (hour: number) => new Date(2026, 7, 17, hour, 0, 0); + list.next([ + notification(1, {createdAt: at(9)}), + notification(2, {createdAt: at(14)}), + notification(3, {createdAt: at(11)}), + notification(4, {createdAt: at(15)}), + notification(5, {createdAt: at(10)}), + notification(6, {createdAt: at(13)}), + notification(7, {createdAt: at(12)}), + ]); + fixture.detectChanges(); + + expect(rowText()).toEqual([ + 'notification 4', + 'notification 2', + 'notification 6', + 'notification 7', + 'notification 3', + ]); + }); + + it('makes each row a real button so a keyboard can open it', () => { + openMenu(); + list.next([notification(1)]); + fixture.detectChanges(); + + // Structural, and it has to be. MatMenu's key manager only moves focus, + // it never synthesises a click, so Enter on a row works only because the + // element is natively a button. The test environment does not simulate + // that browser behaviour either, so the element type is the assertion. + expect(rows()[0].tagName).toBe('BUTTON'); + }); + + it('gives each category its own icon and colour', () => { + openMenu(); + list.next([ + notification(1, {notificationType: 'feedback'}), + notification(2, {notificationType: 'task'}), + notification(3, {notificationType: 'portfolio'}), + notification(4, {notificationType: 'extension'}), + notification(5, {notificationType: 'general'}), + ]); + fixture.detectChanges(); + + expect(glyphIcons()).toEqual([ + 'chat_bubble', + 'assignment', + 'collections_bookmark', + 'more_time', + 'campaign', + ]); + + // The tone is what the stylesheet colours on, so a row with the right + // icon and the wrong tone still looks wrong. + expect(glyphTones()).toEqual([ + 'tone-feedback', + 'tone-task', + 'tone-portfolio', + 'tone-extension', + 'tone-general', + ]); + }); + + it('still draws something for a category it has never heard of', () => { + openMenu(); + // Notification::TYPES is validated on the api, so this is a sixth + // category added there reaching a browser running older web code. A row + // with no icon at all is worse than a generic one. + list.next([notification(1, {notificationType: 'announcement'})]); + fixture.detectChanges(); + + expect(glyphIcons()).toEqual(['notifications']); + expect(glyphTones()).toEqual(['tone-general']); + }); + + it('marks the unread rows with a dot and leaves the read ones a gap', () => { + openMenu(); + list.next([notification(1), notification(2, {readAt: new Date(2026, 7, 17, 9, 30, 0)})]); + fixture.detectChanges(); + + const dots = Array.from(panel().querySelectorAll('.notification-unread-dot')); + + // Both rows keep the element. Removing it on read would shuffle the + // delete button sideways from row to row. + expect(dots).toHaveLength(2); + expect(dots[0].classList.contains('is-read')).toBe(false); + expect(dots[1].classList.contains('is-read')).toBe(true); + }); + + it('says unread in words, not only in bold and colour', () => { + openMenu(); + list.next([ + notification(1, {message: 'Andrew Cain commented on 1.1P.'}), + notification(2, { + message: 'Your extension was granted.', + readAt: new Date(2026, 7, 17, 9, 30, 0), + }), + ]); + fixture.detectChanges(); + + // Weight and a dot are both things you have to be able to see. + expect(rows()[0].getAttribute('aria-label')).toContain('Unread.'); + expect(rows()[0].getAttribute('aria-label')).toContain('Andrew Cain commented on 1.1P.'); + expect(rows()[1].getAttribute('aria-label')).toContain('Read.'); + }); + + it('says how long ago each one arrived', () => { + openMenu(); + + const twoHoursAgo = new Date(Date.now() - 2 * 60 * 60 * 1000); + list.next([notification(1, {createdAt: twoHoursAgo})]); + fixture.detectChanges(); + + expect(rows()[0].querySelector('.notification-time').textContent.trim()).toBe('2 hours ago'); + }); + + it('says something friendly when there is nothing to show', () => { + openMenu(); + + list.next([]); + fixture.detectChanges(); + + expect(rows()).toHaveLength(0); + expect(placeholderText()).toContain('You are all caught up'); + }); + + it('says it could not load rather than claiming there is nothing', () => { + openMenu(); + + list.error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + // The whole point of this one. An empty box after a failed request tells + // the user they have no notifications, which is a different and wrong + // thing to say. + expect(placeholderText()).toContain('could not load'); + expect(placeholderText()).not.toContain('caught up'); + }); + + it('shows the list it already has while it reads a fresh one', () => { + openMenu(); + list.next([notification(1)]); + fixture.detectChanges(); + + // Reopening starts a second read. Blanking the panel for it would make + // every open flash, when what is on screen is still nearly right. + reopenWith(); + + expect(rowText()).toEqual(['notification 1']); + }); + + it('warns that the rows are stale when a refresh fails behind them', () => { + openMenu(); + list.next([notification(1)]); + fixture.detectChanges(); + + reopenWith().error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + // Keeping the rows is right, keeping them silently is not. A list nobody + // could refresh looks exactly like a list nothing has happened to. + expect(rowText()).toEqual(['notification 1']); + expect(panel().textContent).toContain('may be out of date'); + }); + + it('marks a row read and opens it when it is clicked', () => { + openMenu(); + list.next([notification(1, {link: '/projects/9/dashboard'})]); + fixture.detectChanges(); + + rows()[0].click(); + + expect(notificationService.markRead).toHaveBeenCalledTimes(1); + expect(notificationService.markRead.mock.calls[0][0].id).toBe(1); + expect(subscribedTo.has('markRead')).toBe(true); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + expect(notificationOpener.open.mock.calls[0][0].id).toBe(1); + }); + + it('does not mark a row read twice', () => { + openMenu(); + list.next([notification(1, {readAt: new Date(2026, 7, 17, 9, 30, 0)})]); + fixture.detectChanges(); + + rows()[0].click(); + + // read_all and mark_read are both no-ops on the api for one already read, + // so a second call would do nothing except tell the service to take one + // off a count that never included it. + expect(notificationService.markRead).not.toHaveBeenCalled(); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + }); + + it('marks a row read even when it has nowhere to go', () => { + openMenu(); + list.next([notification(1, {link: null})]); + fixture.detectChanges(); + + rows()[0].click(); + + // link is nullable on the api. Refusing to mark it read would leave a + // number on the bell that the user has no way to clear. Whether there is + // anywhere to go is the opener's call, so it is still asked. + expect(notificationService.markRead).toHaveBeenCalledTimes(1); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + }); + + it('says so when a notification could not be marked as read', () => { + notificationService.markRead.mockReturnValue(throwError(() => new Error('PUT read failed'))); + + openMenu(); + list.next([notification(1, {link: null})]); + fixture.detectChanges(); + + rows()[0].click(); + + // Nothing else on screen changes when this fails. Swallowing it leaves + // the row unread and the badge where it was with no explanation. + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + it('drops a list read still in flight when a row is opened', () => { + openMenu(); + list.next([notification(1)]); + fixture.detectChanges(); + + // The rows on screen during a refresh are the previous list's, so this is + // the ordinary way to reach this: reopen, click before the read lands. + const refresh = reopenWith(); + expect(refresh.observed).toBe(true); + + rows()[0].click(); + + // That response was worked out before the click and NotificationService + // writes list responses into the shared entity cache, so letting it land + // would put readAt back to null and draw the row unread again. + expect(refresh.observed).toBe(false); + }); + }); + /** + * The split that matters here is which action asks first. Marking read + * destroys nothing and a dialog on it is a step to click through; delete has + * no undo anywhere in this feature and gets one. + */ + describe('the bulk actions', () => { + // The dialog is a callback, not a promise. show() is handed the function to + // run on confirm and the one to run on cancel, so a test drives it by + // reaching for whichever of those it wants. + const confirmed = (): void => confirmationModal.show.mock.calls[0][2](); + const cancelled = (): void => confirmationModal.show.mock.calls[0][3](); + + const deleteButtons = (): HTMLElement[] => + Array.from(panel().querySelectorAll('.notification-delete')); + + const markAllButton = (): HTMLElement => panel().querySelector('.notification-mark-all'); + + const seeAllButton = (): HTMLElement => panel().querySelector('.notification-see-all'); + + beforeEach(() => { + fixture.detectChanges(); + openMenu(); + list.next([notification(1), notification(2, {readAt: new Date(2026, 7, 17, 9, 30, 0)})]); + fixture.detectChanges(); + }); + + it('registers every dropdown action in MatMenu keyboard order', async () => { + unreadCount.next(1); + fixture.detectChanges(); + await fixture.whenStable(); + await expectAccessible(panel()); + + const menuItems = Array.from(panel().querySelectorAll('[role="menuitem"]')); + + expect(menuItems).toHaveLength(6); + expect(menuItems[0]).toBe(markAllButton()); + expect(menuItems[1]).toBe(rows()[0]); + expect(menuItems[2]).toBe(deleteButtons()[0]); + expect(menuItems[3]).toBe(rows()[1]); + expect(menuItems[4]).toBe(deleteButtons()[1]); + expect(menuItems[5]).toBe(seeAllButton()); + + for (const menuItem of menuItems) { + expect(menuItem.classList.contains('mat-mdc-menu-item')).toBe(true); + } + }); + + it('names each delete action after the notification it removes', () => { + expect(deleteButtons().map((button) => button.getAttribute('aria-label'))).toEqual([ + 'Delete notification: notification 1', + 'Delete notification: notification 2', + ]); + }); + + it('marks everything read without asking first', () => { + unreadCount.next(1); + fixture.detectChanges(); + + // MatMenu closes the panel from a click handler on the panel itself, so + // anything that reaches it shuts the menu. The whole reward for this + // button is watching the rows stop being bold, and that is gone if the + // menu closes underneath it. Watching the click arrive is steadier than + // watching the overlay leave, which is animation timing. + const reachedThePanel = vi.fn(); + panel().addEventListener('click', reachedThePanel); + + markAllButton().click(); + fixture.detectChanges(); + + expect(notificationService.markAllRead).toHaveBeenCalledTimes(1); + expect(subscribedTo.has('markAllRead')).toBe(true); + expect(confirmationModal.show).not.toHaveBeenCalled(); + expect(reachedThePanel).not.toHaveBeenCalled(); + }); + + it('does not offer to mark all read when nothing is unread', () => { + unreadCount.next(0); + fixture.detectChanges(); + + // Same reason the badge disappears at zero rather than showing one. An + // action that cannot do anything should not be sitting there. + expect(markAllButton()).toBeNull(); + + unreadCount.next(2); + fixture.detectChanges(); + + expect(markAllButton()).not.toBeNull(); + }); + + it('says so when marking everything read fails', () => { + unreadCount.next(1); + fixture.detectChanges(); + notificationService.markAllRead.mockReturnValue( + throwError(() => new Error('PUT read_all failed')), + ); + + markAllButton().click(); + + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + it('asks before it deletes anything', () => { + deleteButtons()[0].click(); + + expect(confirmationModal.show).toHaveBeenCalledTimes(1); + expect(notificationService.remove).not.toHaveBeenCalled(); + }); + + it('deletes only once the dialog is agreed to', () => { + deleteButtons()[0].click(); + confirmed(); + + expect(notificationService.remove).toHaveBeenCalledTimes(1); + expect(notificationService.remove.mock.calls[0][0].id).toBe(1); + expect(subscribedTo.has('remove')).toBe(true); + + fixture.detectChanges(); + expect(rowText()).toEqual(['notification 2']); + }); + + it('leaves the notification alone when the dialog is cancelled', () => { + deleteButtons()[0].click(); + cancelled(); + fixture.detectChanges(); + + expect(notificationService.remove).not.toHaveBeenCalled(); + expect(rowText()).toEqual(['notification 1', 'notification 2']); + }); + + it('keeps the row when the delete fails', () => { + notificationService.remove.mockReturnValue(throwError(() => new Error('DELETE failed'))); + + deleteButtons()[0].click(); + confirmed(); + fixture.detectChanges(); + + // Taking the row out first and putting it back on failure would be worse + // than leaving it. The user reads the row vanishing as the delete having + // worked. + expect(rowText()).toEqual(['notification 1', 'notification 2']); + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + it('does not open the notification when its delete button is clicked', () => { + deleteButtons()[0].click(); + + // The delete button sits in the row, and the row opens the notification. + // Without stopping the click there, asking to delete one would mark it + // read and navigate away from the menu it was asked in. + expect(notificationService.markRead).not.toHaveBeenCalled(); + expect(router.navigateByUrl).not.toHaveBeenCalled(); + }); + + it('promotes the next notification when a shown one is deleted', () => { + const at = (hour: number) => new Date(2026, 7, 17, hour, 0, 0); + const refresh = reopenWith(); + refresh.next([ + notification(1, {createdAt: at(15)}), + notification(2, {createdAt: at(14)}), + notification(3, {createdAt: at(13)}), + notification(4, {createdAt: at(12)}), + notification(5, {createdAt: at(11)}), + notification(6, {createdAt: at(10)}), + ]); + fixture.detectChanges(); + + expect(rowText()).toHaveLength(5); + expect(rowText()).not.toContain('notification 6'); + + deleteButtons()[0].click(); + confirmed(); + fixture.detectChanges(); + + // Filtering only the five on screen would leave four and a gap, when the + // sixth is already in hand and nothing needs to be asked for. + expect(rowText()).toEqual([ + 'notification 2', + 'notification 3', + 'notification 4', + 'notification 5', + 'notification 6', + ]); + }); + + it('drops a list read still in flight when everything is marked read', () => { + unreadCount.next(1); + fixture.detectChanges(); + + const refresh = reopenWith(); + expect(refresh.observed).toBe(true); + + markAllButton().click(); + + expect(refresh.observed).toBe(false); + }); + + it('offers a way to the rest of them', () => { + // The dropdown shows five and the api sends every one, so without this a + // sixth notification can only be reached by typing a url. + seeAllButton().click(); + + expect(router.navigateByUrl).toHaveBeenCalledWith('/notifications'); + }); + }); +}); diff --git a/src/app/common/header/notification-bell/notification-bell.component.ts b/src/app/common/header/notification-bell/notification-bell.component.ts new file mode 100644 index 0000000000..ddcd626a46 --- /dev/null +++ b/src/app/common/header/notification-bell/notification-bell.component.ts @@ -0,0 +1,381 @@ +import moment from 'moment'; +import {ChangeDetectionStrategy, Component, OnDestroy, OnInit} from '@angular/core'; +import {NavigationEnd, Router} from '@angular/router'; +import {Subscription, filter} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {AuthenticationService} from 'src/app/api/services/authentication.service'; +import {NotificationService} from 'src/app/api/services/notification.service'; +import {NotificationOpenService} from 'src/app/common/notifications/notification-open.service'; +import {presentationFor} from 'src/app/common/notifications/notification-presentation'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {ConfirmationModalService} from '../../modals/confirmation-modal/confirmation-modal.service'; + +/** + * The bell in the header, the unread count on it, and the list behind it. + * + * Its own component rather than more markup in HeaderComponent. The header is + * already eleven injected services long and its spec replaces the template with + * an empty string, so anything added there is untested by construction. This + * also keeps the change to the header itself to one line. + * + * The dropdown lives here rather than in a component of its own, which is the + * same shape as unit-dropdown and task-dropdown next door. Both of those own + * their trigger and their mat-menu in one template, and the reason is not + * tidiness: MatMenu collects its items with a content query, and a content + * query does not reach into another component's template. Rows in a child + * component would still click and still close the menu, but the menu's key + * manager would never see them, so the arrow keys would walk over an empty + * menu. + * + * The count comes from NotificationService.unreadCount$ and not from a request + * this component makes, so marking one read anywhere in the app moves the badge + * without the bell knowing who did it. + */ +@Component({ + selector: 'notification-bell', + templateUrl: './notification-bell.component.html', + styleUrls: ['./notification-bell.component.scss'], + changeDetection: ChangeDetectionStrategy.Eager, + standalone: false, +}) +export class NotificationBellComponent implements OnInit, OnDestroy { + /** + * How many rows the dropdown shows. + * + * The api has no paging, GET /notifications answers with the lot, so this is + * a display limit and not a request size. Somebody with four hundred of them + * still downloads four hundred, and the full page is where they go to read + * them. + */ + public static readonly RECENT_COUNT = 5; + + unreadCount = 0; + + /** + * Everything the last read came back with, newest first. + * + * All of it and not just the five on screen. Deleting one of the five has to + * promote the sixth, and re-reading the list to find out what the sixth was + * would be a request for something already in hand. + */ + notifications: Notification[] = []; + + /** + * The slice actually drawn. + */ + recent: Notification[] = []; + + loading = false; + loadFailed = false; + + private subscriptions: Subscription[] = []; + private listSubscription: Subscription | null = null; + private destroyed = false; + + constructor( + private notificationService: NotificationService, + private authenticationService: AuthenticationService, + private confirmationModal: ConfirmationModalService, + private alerts: AlertService, + private router: Router, + private notificationOpener: NotificationOpenService, + ) {} + + ngOnInit(): void { + this.subscriptions.push( + this.notificationService.unreadCount$.subscribe((count) => { + this.unreadCount = count; + }), + ); + + // The count goes stale on its own. Notifications are created by the api, + // and push only reaches a browser that granted permission and kept the + // service worker alive, so most sessions never hear about a new one. A + // navigation is the cheapest honest excuse to ask again, and it is roughly + // when someone would look at the bell anyway. + this.subscriptions.push( + this.router.events + .pipe(filter((event) => event instanceof NavigationEnd)) + .subscribe(() => this.refresh()), + ); + + // A remembered session is restored after the header is created. If the bell + // asks immediately, the authentication guard can skip the request before the + // restored session is ready, and no later navigation is guaranteed. + if (this.authenticationService.isAuthenticated()) { + this.refresh(); + } else { + this.authenticationService.afterAuthCall((authenticated) => { + if (authenticated && !this.destroyed) { + this.refresh(); + } + }); + } + } + + ngOnDestroy(): void { + this.destroyed = true; + this.listSubscription?.unsubscribe(); + this.subscriptions.forEach((subscription) => subscription.unsubscribe()); + } + + /** + * What a screen reader should call the bell. + * + * The badge is not part of it. Material renders the number into a span marked + * aria-hidden, so a bell with four unread notifications announces itself as + * "Notifications, button" and the count is the one thing the control exists + * to say. + */ + get bellLabel(): string { + return this.unreadCount ? `Notifications, ${this.unreadCount} unread` : 'Notifications'; + } + + get badgeText(): number | string { + return this.unreadCount > 99 ? '99+' : this.unreadCount; + } + + /** + * Read the list again every time the menu is opened. + * + * On the open event and not in ngOnInit. Content written inside a mat-menu is + * built with the surrounding component whether the menu is ever opened or + * not, so an ngOnInit here would fetch once at sign in and then show that + * same list for the rest of the session. + */ + onMenuOpened(): void { + this.loading = true; + this.loadFailed = false; + + if (!this.authenticationService.isAuthenticated()) { + this.loading = false; + this.loadFailed = true; + return; + } + + // Opening twice quickly is easy to do. Without this the first response can + // land after the second and put the older list back. + this.listSubscription?.unsubscribe(); + + this.listSubscription = this.notificationService.list().subscribe({ + next: (notifications) => { + this.notifications = this.newestFirst(notifications); + this.updateRecent(); + this.loading = false; + }, + error: () => { + this.loading = false; + this.loadFailed = true; + }, + }); + } + + /** + * Open one notification: mark it read, then go where it points. + * + * Marking read is not conditional on the link. A notification with nowhere to + * go has still been seen once it is clicked, and leaving it unread would keep + * a number on the bell that the user cannot clear. + */ + open(notification: Notification): void { + if (!notification.isRead) { + // The rows on screen during a refresh are the previous list's, so this is + // a real sequence and not a theoretical one. That response was worked out + // before this click and it is written straight into the shared entity + // cache, so letting it land would put readAt back to null and draw the + // row unread again. + this.cancelPendingList(); + + // Not waited on. The row is already gone from view by the time this + // answers, the badge follows from unreadCount$, and holding the + // navigation for it would make every click feel slow. + this.notificationService.markRead(notification).subscribe({ + error: () => this.alerts.error('That notification could not be marked as read'), + }); + } + + // Where it goes, for a student or for staff, and what happens when the + // thing it was about has gone, is all decided in NotificationOpenService. + void this.notificationOpener.open(notification); + } + + /** + * Mark the lot as read. + * + * No confirmation. Nothing is lost by it and the user can still read every + * one of them afterwards, so a dialog here would only be a step to click + * through. Delete is the one that asks. + * + * The count on the bell is not touched here. NotificationService drops it to + * zero inside markAllRead and pushes that down unreadCount$, which is where + * the badge reads it from, so a second copy of the arithmetic here could only + * ever disagree with it. + */ + markAllRead(event: MouseEvent): void { + // MatMenu closes on any click that reaches the panel, and closing here + // would hide the thing the user just asked to see happen. It also has to + // stop before the row underneath, which would treat this as opening the + // notification. + event.stopPropagation(); + + // A read that went out before this is now out of date, and its response is + // written into the shared cache, so landing late would draw every row + // unread again. + this.cancelPendingList(); + + this.notificationService.markAllRead().subscribe({ + error: () => this.alerts.error('Your notifications could not be marked as read'), + }); + } + + /** + * Ask first, then delete. + * + * Delete is the only one of the five endpoints that destroys anything, and + * there is no undo anywhere in this feature, so it gets the dialog the house + * already has rather than a new one. + * + * The empty cancel function is deliberate. ConfirmationModalService pops a + * green success snackbar reading " action cancelled" when none is + * given, and telling somebody they successfully did not delete something is + * noise. + */ + confirmDelete(notification: Notification, event: MouseEvent): void { + event.stopPropagation(); + + this.confirmationModal.show( + 'Delete notification', + 'This removes the notification. You will not be able to get it back.', + () => this.remove(notification), + () => undefined, + 'Delete', + ); + } + + /** + * Go to the full list. + * + * The dropdown shows five and the api sends every one, so without a way out + * of it a sixth notification is unreachable except by typing a url. + * + * The /notifications route arrives with IN-04, which is a sibling branch, so + * on this branch alone there is nothing at that path yet and app.routes.ts + * sends an unmatched path to /home. Both branches merge into + * feature/notifications before any of this ships. + */ + seeAll(): void { + this.router.navigateByUrl('/notifications'); + } + + iconFor(notification: Notification): string { + return presentationFor(notification).icon; + } + + toneFor(notification: Notification): string { + return presentationFor(notification).tone; + } + + labelFor(notification: Notification): string { + return presentationFor(notification).label; + } + + /** + * What a screen reader should read for a row. + * + * Unread is drawn as bold text and a coloured dot, and both of those are + * things you have to be able to see. The message and the time are already in + * the button, so this only exists to put the state into words. + */ + rowLabel(notification: Notification): string { + const state = notification.isRead ? 'Read' : 'Unread'; + + return `${state}. ${this.labelFor(notification)}. ${notification.message}. ${this.timeAgo(notification)}`; + } + + /** + * How long ago this arrived, in words. + * + * Worked out on each change detection pass rather than stored, so a menu left + * open does not sit there insisting something happened "a few seconds ago" + * ten minutes later. + */ + timeAgo(notification: Notification): string { + return moment(notification.createdAt).fromNow(); + } + + /** + * Retire a list request that is still in the air. + * + * Anything this component changes locally is newer than a read that went out + * before it. NotificationService writes list responses into the shared cache, + * so one arriving late does not merely show stale rows, it undoes the change. + */ + protected cancelPendingList(): void { + this.listSubscription?.unsubscribe(); + this.listSubscription = null; + this.loading = false; + } + + /** + * Delete it for real, once the dialog has been agreed to. + * + * The row is dropped from the held list on success rather than by reading the + * list again. A second GET for something we already know the answer to would + * make the row sit there for a round trip after the user asked for it to go. + * The unread count corrects itself from unreadCount$, the same as everywhere + * else here. + */ + private remove(notification: Notification): void { + this.cancelPendingList(); + + this.notificationService.remove(notification).subscribe({ + next: () => { + this.notifications = this.notifications.filter((row) => row !== notification); + + // Recomputed and not filtered in place. Filtering the five on screen + // leaves four and a gap, when the sixth newest is sitting right here + // waiting to move up. + this.updateRecent(); + this.alerts.success('Notification deleted'); + }, + error: () => this.alerts.error('That notification could not be deleted'), + }); + } + + /** + * Newest first. + * + * The api already answers in this order, and this sorts anyway. Recency is + * the entire meaning of this list, and the copies handed back here come out + * of a cache that other calls also write to, so the order of the array is not + * something worth trusting. + */ + private newestFirst(notifications: Notification[]): Notification[] { + return [...notifications].sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime()); + } + + private updateRecent(): void { + this.recent = this.notifications.slice(0, NotificationBellComponent.RECENT_COUNT); + } + + /** + * Ask the api for the count, but only while somebody is signed in. + * + * The guard is not tidiness. Sign out hides the header and routes to + * /sign_in, and hiding is a change detection away while the navigation event + * fires straight away, so this component can still be alive for it. The + * request would then go out anonymous, come back 403, and + * HttpErrorInterceptor reads a 403 on an anonymous user as an expired session: + * it shows "Authentication timed out" and redirects to /timeout. Signing out + * would look like being kicked out. + * + * Errors are dropped. Nothing the user asked for depends on this. + */ + private refresh(): void { + if (!this.authenticationService.isAuthenticated()) { + return; + } + + this.notificationService.refreshUnreadCount().subscribe({error: () => undefined}); + } +} diff --git a/src/app/common/notification-settings/notification-settings.component.html b/src/app/common/notification-settings/notification-settings.component.html new file mode 100644 index 0000000000..d49180ca1c --- /dev/null +++ b/src/app/common/notification-settings/notification-settings.component.html @@ -0,0 +1,153 @@ +<section + aria-describedby="notification-settings-intro" + aria-labelledby="notification-settings-title" + class="notification-settings" + role="group" +> + <div class="notification-settings__heading"> + <h2 id="notification-settings-title"> + <mat-icon aria-hidden="true">notifications</mat-icon>Notifications + </h2> + + <p class="notification-settings__intro" id="notification-settings-intro"> + Choose which notification categories you want to receive. + </p> + + @if (showNotificationsLink) { + <p> + You can view the notifications controlled by these preferences on the + <a routerLink="/notifications">Notifications page</a>. + </p> + } + </div> + + <div class="notification-setting"> + <mat-checkbox + aria-describedby="task-notification-description" + color="primary" + labelPosition="before" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveTaskNotifications" + (ngModelChange)="preferencesChange.emit()" + > + Task notifications + </mat-checkbox> + <small class="notification-setting__help" id="task-notification-description"> + Due dates, changed dates and task status updates. For staff, also submissions, help requests + and extension requests. + </small> + </div> + + <div class="notification-setting"> + <mat-checkbox + aria-describedby="feedback-notification-description" + color="primary" + labelPosition="before" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveFeedbackNotifications" + (ngModelChange)="preferencesChange.emit()" + > + Feedback notifications + </mat-checkbox> + <small class="notification-setting__help" id="feedback-notification-description"> + New comments, feedback, and review outcomes. + </small> + </div> + + <!-- One email per unit summarising where the student is up to. Its own + control, because it used to ride on feedback notifications and could not + be turned off without losing those too. --> + <div class="notification-setting notification-setting--choice"> + <!-- Named by id, not by label[for]: Material puts role="combobox" on the + select's own element, which a label cannot be associated with. --> + <span class="notification-setting__label" id="digest-frequency-label">Summary email</span> + <mat-form-field + appearance="outline" + class="notification-setting__select" + subscriptSizing="dynamic" + > + <mat-select + aria-describedby="digest-frequency-description" + aria-labelledby="digest-frequency-label" + id="digest-frequency" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.digestFrequency" + (ngModelChange)="preferencesChange.emit()" + > + @for (option of digestOptions; track option.value) { + <mat-option [value]="option.value">{{ option.label }}</mat-option> + } + </mat-select> + </mat-form-field> + <small class="notification-setting__help" id="digest-frequency-description"> + {{ digestHelp }} + </small> + </div> + + <div class="notification-setting"> + <mat-checkbox + aria-describedby="portfolio-notification-description" + color="primary" + labelPosition="before" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receivePortfolioNotifications" + (ngModelChange)="preferencesChange.emit()" + > + Portfolio notifications + </mat-checkbox> + <small class="notification-setting__help" id="portfolio-notification-description"> + Portfolio processing and assessment updates. + </small> + </div> + + <div class="notification-setting"> + <mat-checkbox + aria-describedby="unit-hub-notification-description" + color="primary" + labelPosition="before" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveUnitHubNotifications" + > + Unit Hub updates + </mat-checkbox> + <small class="notification-setting__help" id="unit-hub-notification-description"> + New and changed announcements, and sessions that move or are cancelled. Shown here in OnTrack. + Choose below if you also want them sent to you. + </small> + + <div + aria-label="Also send Unit Hub updates by" + class="notification-setting__channels" + role="group" + > + <mat-checkbox + color="primary" + [disabled]="!user.receiveUnitHubNotifications" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveUnitHubEmailNotifications" + > + Email + </mat-checkbox> + <mat-checkbox + color="primary" + [disabled]="!user.receiveUnitHubNotifications" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveUnitHubPushNotifications" + > + Push + </mat-checkbox> + <mat-checkbox + color="primary" + [disabled]="!user.receiveUnitHubNotifications" + [ngModelOptions]="{standalone: true}" + [(ngModel)]="user.receiveUnitHubSessionReminders" + > + Session reminders + </mat-checkbox> + </div> + <small class="notification-setting__help"> + Push only reaches browsers where push notifications are turned on. Session reminders arrive 30 + minutes before a session starts. + </small> + </div> +</section> diff --git a/src/app/common/notification-settings/notification-settings.component.scss b/src/app/common/notification-settings/notification-settings.component.scss new file mode 100644 index 0000000000..8d9f170a31 --- /dev/null +++ b/src/app/common/notification-settings/notification-settings.component.scss @@ -0,0 +1,70 @@ +@use '../edit-profile-form/profile-form' as profile; + +@include profile.theme; + +:host { + display: block; + min-width: 0; + width: 100%; +} + +.notification-settings { + @include profile.card; + gap: 20px; +} + +.notification-settings__heading { + @include profile.card-heading; + + a { + color: var(--ot-color-link); + text-decoration: underline; + text-underline-offset: 3px; + } +} + +// label and description on the left, the checkbox on the right; one gap between +// rows instead of a box around each +.notification-setting { + @include profile.setting-row; +} + +.notification-setting__channels { + display: flex; + flex-wrap: wrap; + gap: 0 16px; + margin-block: 4px 2px; + // line the boxes up with the text above, past the checkbox's touch padding + margin-inline-start: -11px; +} + +.notification-setting__channels mat-checkbox { + width: auto; +} + +// A choice rather than an on/off, so the control sits on the same line as its +// label the way the checkboxes do, with the help text underneath. +.notification-setting--choice { + display: grid; + align-items: center; + gap: 4px 12px; + grid-template-areas: + 'label control' + 'help help'; + grid-template-columns: minmax(0, 1fr) auto; +} + +.notification-setting__label { + grid-area: label; + color: var(--ot-color-text); + font-size: 0.95rem; +} + +.notification-setting__select { + grid-area: control; + width: 10rem; +} + +.notification-setting--choice .notification-setting__help { + grid-area: help; +} diff --git a/src/app/common/notification-settings/notification-settings.component.spec.ts b/src/app/common/notification-settings/notification-settings.component.spec.ts new file mode 100644 index 0000000000..d720991250 --- /dev/null +++ b/src/app/common/notification-settings/notification-settings.component.spec.ts @@ -0,0 +1,279 @@ +import {beforeEach, describe, expect, it} from 'vitest'; +import {TestbedHarnessEnvironment} from '@angular/cdk/testing/testbed'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {FormsModule} from '@angular/forms'; +import {MatCheckboxModule} from '@angular/material/checkbox'; +import {MatCheckboxHarness} from '@angular/material/checkbox/testing'; +import {MatFormFieldModule} from '@angular/material/form-field'; +import {MatIconModule} from '@angular/material/icon'; +import {MatSelectModule} from '@angular/material/select'; +import {MatSelectHarness} from '@angular/material/select/testing'; +import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {RouterModule} from '@angular/router'; +import {User} from 'src/app/api/models/user/user'; +import {NotificationSettingsComponent} from './notification-settings.component'; + +const makeUser = (): User => + ({ + id: 1, + systemRole: 'Student', + receiveTaskNotifications: false, + receiveFeedbackNotifications: false, + receivePortfolioNotifications: false, + receiveUnitHubNotifications: true, + receiveUnitHubEmailNotifications: false, + receiveUnitHubPushNotifications: false, + receiveUnitHubSessionReminders: false, + }) as User; + +describe('NotificationSettingsComponent', () => { + let component: NotificationSettingsComponent; + let fixture: ComponentFixture<NotificationSettingsComponent>; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [NotificationSettingsComponent], + imports: [ + FormsModule, + MatCheckboxModule, + MatFormFieldModule, + MatIconModule, + MatSelectModule, + NoopAnimationsModule, + RouterModule.forRoot([]), + ], + }).compileComponents(); + + fixture = TestBed.createComponent(NotificationSettingsComponent); + component = fixture.componentInstance; + component.user = makeUser(); + + fixture.detectChanges(); + await fixture.whenStable(); + }); + + it('should create', () => { + expect(component).toBeTruthy(); + }); + + it('shows the four notification categories and the Unit Hub channels', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + const checkboxes = await loader.getAllHarnesses(MatCheckboxHarness); + + expect(await Promise.all(checkboxes.map((checkbox) => checkbox.getLabelText()))).toEqual([ + 'Task notifications', + 'Feedback notifications', + 'Portfolio notifications', + 'Unit Hub updates', + 'Email', + 'Push', + 'Session reminders', + ]); + }); + + it('renders the Unit Hub row like the other categories', () => { + const rows: HTMLElement[] = Array.from( + fixture.nativeElement.querySelectorAll( + '.notification-setting:not(.notification-setting--choice)', + ), + ); + const hubRow = rows[3]; + + expect(rows.length).toBe(4); + expect(hubRow.querySelector('small')?.id).toBe('unit-hub-notification-description'); + expect(hubRow.textContent).toContain('New and changed announcements'); + expect(hubRow.querySelector('[role="group"]')?.getAttribute('aria-label')).toBe( + 'Also send Unit Hub updates by', + ); + }); + + it('starts Unit Hub updates in the app only, and saves each channel to its own field', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + const hub = await loader.getHarness(MatCheckboxHarness.with({label: 'Unit Hub updates'})); + const email = await loader.getHarness(MatCheckboxHarness.with({label: 'Email'})); + const push = await loader.getHarness(MatCheckboxHarness.with({label: 'Push'})); + const reminders = await loader.getHarness( + MatCheckboxHarness.with({label: 'Session reminders'}), + ); + + expect(await hub.isChecked()).toBe(true); + expect(await email.isChecked()).toBe(false); + expect(await push.isChecked()).toBe(false); + expect(await reminders.isChecked()).toBe(false); + + await email.check(); + await reminders.check(); + + expect(component.user.receiveUnitHubEmailNotifications).toBe(true); + expect(component.user.receiveUnitHubPushNotifications).toBe(false); + expect(component.user.receiveUnitHubSessionReminders).toBe(true); + }); + + it('turns the Unit Hub channels off while the category is off', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + const hub = await loader.getHarness(MatCheckboxHarness.with({label: 'Unit Hub updates'})); + + await hub.uncheck(); + fixture.detectChanges(); + await fixture.whenStable(); + + expect(component.user.receiveUnitHubNotifications).toBe(false); + for (const label of ['Email', 'Push', 'Session reminders']) { + const channel = await loader.getHarness(MatCheckboxHarness.with({label})); + expect(await channel.isDisabled()).toBe(true); + } + }); + + it('updates the correct user preference when toggled', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + + const taskCheckbox = await loader.getHarness( + MatCheckboxHarness.with({label: 'Task notifications'}), + ); + const feedbackCheckbox = await loader.getHarness( + MatCheckboxHarness.with({label: 'Feedback notifications'}), + ); + const portfolioCheckbox = await loader.getHarness( + MatCheckboxHarness.with({label: 'Portfolio notifications'}), + ); + + await taskCheckbox.check(); + + expect(component.user.receiveTaskNotifications).toBe(true); + expect(component.user.receiveFeedbackNotifications).toBe(false); + expect(component.user.receivePortfolioNotifications).toBe(false); + + await feedbackCheckbox.check(); + + expect(component.user.receiveFeedbackNotifications).toBe(true); + + await portfolioCheckbox.check(); + + expect(component.user.receivePortfolioNotifications).toBe(true); + }); + + // The profile form listens for this to enable Save profile, because these + // standalone checkboxes never mark that form dirty themselves. + it('reports each toggle so the enclosing form can be marked dirty', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + let changes = 0; + component.preferencesChange.subscribe(() => changes++); + + const taskCheckbox = await loader.getHarness( + MatCheckboxHarness.with({label: 'Task notifications'}), + ); + const portfolioCheckbox = await loader.getHarness( + MatCheckboxHarness.with({label: 'Portfolio notifications'}), + ); + + await taskCheckbox.check(); + await portfolioCheckbox.check(); + + expect(changes).toBe(2); + }); + + it('reports a new summary email cadence so the enclosing form can be marked dirty', async () => { + const loader = TestbedHarnessEnvironment.loader(fixture); + let changes = 0; + component.preferencesChange.subscribe(() => changes++); + + const select = await loader.getHarness(MatSelectHarness); + await select.open(); + await select.clickOptions({text: 'Daily'}); + + expect(component.user.digestFrequency).toBe('daily'); + expect(changes).toBe(1); + }); + + it('shows help text for each notification category', () => { + const text = fixture.nativeElement.textContent; + + expect(text).toContain('Due dates, changed dates and task status updates.'); + expect(text).toContain('help requests and extension requests'); + expect(text).toContain('New comments, feedback, and review outcomes.'); + expect(text).toContain('Portfolio processing and assessment updates.'); + expect(text).toContain('Session reminders arrive 30 minutes before a session starts.'); + }); + + it('associates each checkbox with its help text', () => { + const inputs = fixture.nativeElement.querySelectorAll( + '.notification-setting > mat-checkbox input[type="checkbox"]', + ); + const descriptions = fixture.nativeElement.querySelectorAll( + '.notification-setting:not(.notification-setting--choice) small[id]', + ); + const descriptionIds = [ + 'task-notification-description', + 'feedback-notification-description', + 'portfolio-notification-description', + 'unit-hub-notification-description', + ]; + + expect(inputs.length).toBe(4); + expect(descriptions.length).toBe(4); + + descriptionIds.forEach((id, index) => { + expect(descriptions[index].id).toBe(id); + expect(inputs[index].getAttribute('aria-describedby')).toBe(id); + expect(inputs[index].hasAttribute('name')).toBe(false); + }); + }); + + it('links the notification preferences to the notifications page', () => { + fixture.componentRef.setInput('showNotificationsLink', true); + fixture.detectChanges(); + + const link: HTMLAnchorElement = fixture.nativeElement.querySelector('a[href="/notifications"]'); + + expect(link).not.toBeNull(); + expect(link.textContent.trim()).toBe('Notifications page'); + }); + + // The admin Users dialog and the first-login form render these settings too, + // and neither should link to the viewer's own notifications. + it('leaves the notifications page link out unless asked to show it', () => { + expect(fixture.nativeElement.querySelector('a[href="/notifications"]')).toBeNull(); + expect(fixture.nativeElement.textContent).not.toContain('Notifications page'); + }); + + it('offers a cadence for the summary email and explains the one chosen', () => { + const component = fixture.componentInstance; + + expect(component.digestOptions.map((option) => option.value)).toEqual([ + 'off', + 'daily', + 'weekly', + 'monthly', + ]); + + component.user.digestFrequency = 'monthly'; + expect(component.digestHelp).toBe('How the trimester is going so far.'); + + // Never is a real choice here, separate from feedback notifications. + component.user.digestFrequency = 'off'; + expect(component.digestHelp).toBe('No summary email.'); + }); + + it('describes the cadence control the way the checkboxes are described', () => { + const select = fixture.nativeElement.querySelector('mat-select'); + const help = fixture.nativeElement.querySelector('#digest-frequency-description'); + + expect(select?.getAttribute('aria-describedby')).toBe('digest-frequency-description'); + expect(help).not.toBeNull(); + }); + + // label[for] cannot name the select, because Material puts role="combobox" on + // the select's own element rather than on a form control. + it('names the cadence control with the label beside it', () => { + const select = fixture.nativeElement.querySelector('mat-select'); + const label = fixture.nativeElement.querySelector('#digest-frequency-label'); + + expect(label?.textContent.trim()).toBe('Summary email'); + expect(select?.getAttribute('aria-labelledby')?.trim()).toBe('digest-frequency-label'); + }); + + it('says nothing rather than guessing when the cadence is unknown', () => { + fixture.componentInstance.user.digestFrequency = undefined as unknown as string; + expect(fixture.componentInstance.digestHelp).toBe(''); + }); +}); diff --git a/src/app/common/notification-settings/notification-settings.component.ts b/src/app/common/notification-settings/notification-settings.component.ts new file mode 100644 index 0000000000..045915bdef --- /dev/null +++ b/src/app/common/notification-settings/notification-settings.component.ts @@ -0,0 +1,43 @@ +import {Component, EventEmitter, Input, Output} from '@angular/core'; +import {User} from 'src/app/api/models/user/user'; + +@Component({ + selector: 'f-notification-settings', + templateUrl: './notification-settings.component.html', + styleUrl: './notification-settings.component.scss', + standalone: false, +}) +export class NotificationSettingsComponent { + @Input({required: true}) user!: User; + + /** + * Show the link to the notifications page. Off by default because the same + * settings also render in the admin Users dialog, where the user belongs to + * someone else, and on the first-login setup form. + */ + @Input() showNotificationsLink = false; + + /** + * The summary email's cadence. 'off' stops it without also turning off + * feedback notifications, which used to be the only switch it had. + */ + public readonly digestOptions: {value: string; label: string; help: string}[] = [ + {value: 'off', label: 'Never', help: 'No summary email.'}, + {value: 'daily', label: 'Daily', help: 'What is due next, every morning.'}, + {value: 'weekly', label: 'Weekly', help: 'Deadlines and how you are tracking.'}, + {value: 'monthly', label: 'Monthly', help: 'How the trimester is going so far.'}, + ]; + + public get digestHelp(): string { + return ( + this.digestOptions.find((option) => option.value === this.user?.digestFrequency)?.help ?? '' + ); + } + + /** + * Fires when the user toggles a category. The checkboxes use standalone + * ngModels, so they never join the enclosing profile form, and that form + * needs this to know it has unsaved changes. + */ + @Output() preferencesChange: EventEmitter<void> = new EventEmitter(); +} diff --git a/src/app/common/notifications-page/notifications-page.component.html b/src/app/common/notifications-page/notifications-page.component.html new file mode 100644 index 0000000000..2df2ce6486 --- /dev/null +++ b/src/app/common/notifications-page/notifications-page.component.html @@ -0,0 +1,151 @@ +<div class="notifications-page mx-auto my-6 max-w-3xl px-4"> + <div class="notifications-header mb-4 flex items-center justify-between gap-3"> + <div> + <h1 class="text-2xl">Notifications</h1> + <p class="notification-time"> + Comments on your tasks, feedback and reminders appear here.<br /> + Mark all read marks notifications as read but does not remove them.<br /> + Deleting a notification permanently removes it and cannot be undone. + </p> + </div> + + <!-- + Keep the control rendered once the list is visible, even after the last + unread row changes. Removing a focused button would drop keyboard focus to + the document body. disabledInteractive keeps it focusable while the guard + in markAllRead prevents another request. + --> + @if (!loading && !loadFailed && notifications.length) { + <div aria-label="Notification actions" class="notification-actions"> + <button + class="notification-mark-all" + mat-stroked-button + [disabled]="!hasUnread || markAllReadPending || deleteAllPending" + [disabledInteractive]="true" + (click)="markAllRead()" + > + <mat-icon>done_all</mat-icon> + <span>Mark all read</span> + </button> + <button + class="notification-delete-all" + color="warn" + mat-stroked-button + [disabled]="deleteAllPending" + [disabledInteractive]="true" + (click)="confirmDeleteAll()" + > + <mat-icon>delete_sweep</mat-icon> + <span>Delete all</span> + </button> + </div> + } + </div> + + @if (loading) { + <div class="flex items-center gap-3 py-6"> + <mat-spinner [diameter]="24"></mat-spinner> + <span>Loading your notifications</span> + </div> + } @else if (loadFailed) { + <!-- + Kept apart from the empty state on purpose. "You have none" and "we could + not find out" look the same and mean opposite things, and only one of them + has a way out. + --> + <div class="notifications-placeholder py-6" tabindex="-1"> + <p>We could not load your notifications.</p> + <button class="notifications-retry" mat-stroked-button (click)="load()">Try again</button> + </div> + } @else if (!notifications.length) { + <!-- + Wrapped so that deleting the last notification can move focus here + (focusAfterDelete) instead of dropping it to the document body. + --> + <div class="notifications-placeholder" tabindex="-1"> + <f-empty-state + hint="Comments on your tasks, feedback and reminders will show up here." + icon="notifications_none" + message="You have no notifications yet." + ></f-empty-state> + </div> + } @else { + <!-- + Plain buttons rather than mat-list-item. Material lays a list row out with + its own leading-icon and content wrappers, and the dropdown in the header + is a mat-menu-item with a different set of wrappers again, so the two + never lined up. Both now use the same .notification-line markup and the + same stylesheet block, and Material lays out neither. + + Still a <button>: a div with a click handler is a row only a mouse can + open. + --> + <div class="notification-list"> + @for (notification of visible; track notification.id) { + <!-- + The delete button is a sibling of the row and not inside it. A button + inside a button is not valid html, the browser unnests them, and the + two end up as separate controls in an order nobody chose. Being + siblings is also what stops a click on delete from opening the + notification, with no need to stop the event. + --> + <div class="notification-item flex w-full items-center"> + <button + class="notification-row grow" + type="button" + [attr.aria-label]="rowLabel(notification)" + (click)="open(notification)" + > + <span class="notification-line"> + <!-- + The same event-specific presentation as the dropdown in the + header, so a notification looks like itself wherever it is read. + --> + <span class="notification-glyph" [class]="'tone-' + toneFor(notification)"> + <mat-icon>{{ iconFor(notification) }}</mat-icon> + </span> + + <span class="notification-body"> + <span class="notification-message" [class.unread]="!notification.isRead">{{ + notification.message + }}</span> + <span class="notification-meta"> + <span class="notification-kind">{{ labelFor(notification) }}</span> + <span aria-hidden="true">·</span> + <span class="notification-time">{{ timeAgo(notification) }}</span> + </span> + </span> + + <span + aria-hidden="true" + class="notification-unread-dot" + [class.is-read]="notification.isRead" + ></span> + </span> + </button> + + <button + class="notification-delete" + mat-icon-button + matTooltip="Delete" + [attr.aria-label]="deleteLabel(notification)" + [disabled]="isDeleting(notification)" + [disabledInteractive]="true" + (click)="confirmDelete(notification)" + > + <mat-icon>close</mat-icon> + </button> + </div> + } + </div> + + <mat-paginator + class="mat-elevation-z0" + [length]="notifications.length" + [pageIndex]="pageIndex" + [pageSize]="pageSize" + [pageSizeOptions]="pageSizeOptions" + (page)="onPage($event)" + ></mat-paginator> + } +</div> diff --git a/src/app/common/notifications-page/notifications-page.component.scss b/src/app/common/notifications-page/notifications-page.component.scss new file mode 100644 index 0000000000..e2c4f31072 --- /dev/null +++ b/src/app/common/notifications-page/notifications-page.component.scss @@ -0,0 +1,226 @@ +.notification-list { + border-top: 1px solid var(--ot-color-divider); +} + +// The row and its delete button, side by side. The separator and the hover both +// live out here rather than on the button, so they run the full width of the +// list instead of stopping short of the delete column. +.notification-item { + border-bottom: 1px solid var(--ot-color-divider); + padding-right: 4px; + + &:hover { + background: var(--ot-color-hover); + } +} + +// A plain button carrying the shared row shape. Material lays out neither this +// nor the header dropdown's rows, which is the only way the two match. +.notification-row { + background: none; + border: 0; + cursor: pointer; + display: block; + font: inherit; + // No right padding. The delete button beside it brings its own, and doubling + // up would push the unread dot away from the edge it marks. + padding: 14px 0 14px 12px; + // Without this the row keeps its content width inside the flex line, and a + // list of short messages leaves the delete buttons in a different place on + // every row. + min-width: 0; + + &:focus-visible { + outline: 2px solid var(--ot-color-focus); + outline-offset: -2px; + } +} + +// --------------------------------------------------------------------------- +// Shared row shape. The header's notification-bell carries a copy of this +// block, deliberately: the two components are on sibling branches so neither +// can import from the other yet, and a row has to look the same in both places. +// Both copies belong in one stylesheet once they have merged. Keep them equal. +// --------------------------------------------------------------------------- +.notification-line { + align-items: center; + display: flex; + gap: 12px; + width: 100%; +} + +.notification-body { + display: flex; + flex: 1 1 auto; + flex-direction: column; + gap: 2px; + min-width: 0; + text-align: left; +} + +.notification-meta { + align-items: baseline; + color: var(--ot-color-text-muted); + display: flex; + flex-wrap: wrap; + font-size: 12px; + gap: 4px; + line-height: 1.3; +} + +.notification-kind { + font-weight: 600; +} + +.notification-glyph { + align-items: center; + border-radius: 50%; + display: inline-flex; + flex: 0 0 36px; + height: 36px; + justify-content: center; + width: 36px; + + // mat-icon ships as a 24px inline-block with a 24px line box. Left alone the + // glyph is nearly as wide as the circle and sits low in it, because an inline + // box aligns on its baseline. A block with a line-height equal to its own + // height and no margin takes that out, and the flex centring above then does + // what it looks like it does. + .mat-icon { + // Explicitly, not by inheritance. Material colours icons inside a menu item + // with .mat-mdc-menu-item .mat-icon { color: ... }, so a tone set on the + // circle around it is simply overridden and the glyph comes out black in + // the dropdown while the same glyph is coloured on the page. + color: inherit; + display: block; + font-size: 20px; + height: 20px; + line-height: 20px; + margin: 0; + overflow: visible; + width: 20px; + } + + // The link blue, not primary. Dark primary is a fill colour and falls to about + // 2:1 as a glyph on its own wash. In light the two are the same colour. + &.tone-feedback { + background: color-mix(in srgb, var(--ot-color-link) 12%, transparent); + color: var(--ot-color-link); + } + + &.tone-task { + background: color-mix(in srgb, var(--ot-color-warning) 12%, transparent); + color: var(--ot-color-warning); + } + + &.tone-portfolio { + background: color-mix(in srgb, var(--ot-color-info) 12%, transparent); + color: var(--ot-color-info); + } + + &.tone-extension { + background: color-mix(in srgb, var(--ot-color-success) 12%, transparent); + color: var(--ot-color-success); + } + + &.tone-general { + background: var(--ot-color-hover); + color: var(--ot-color-text-muted); + } + + // Info is already portfolio, so the Unit Hub takes the selected pair, which + // the theme keeps readable against each other in light and dark. + &.tone-unit-hub { + background: var(--ot-color-selected); + color: var(--ot-color-selected-text); + } +} + +.notification-message { + color: var(--ot-color-text); + font-size: 14px; + line-height: 1.35; + white-space: normal; + + &.unread { + font-weight: 600; + } +} + +.notification-unread-dot { + background: var(--ot-color-primary); + border-radius: 50%; + flex: 0 0 7px; + height: 7px; + width: 7px; + + &.is-read { + visibility: hidden; + } +} + +.notification-time { + color: var(--ot-color-text-muted); + font-size: 12px; + line-height: 1.3; +} + +.notifications-placeholder { + color: var(--ot-color-text-muted); +} + +// The delete button competes with the message for attention on a list nobody +// opened to delete things from, so it is drawn down to match the dropdown's. +.notification-delete { + color: var(--ot-color-text-muted); + flex: none; + + .mat-icon { + font-size: 18px; + height: 18px; + width: 18px; + } +} + +// Keep both destructive and non-destructive bulk actions visibly labelled. +// They move below the heading on phones instead of collapsing to ambiguous +// icon-only controls. +.notification-actions { + display: flex; + flex: none; + gap: 8px; + + button { + min-height: 44px; + } + + .mat-icon { + margin-right: 6px; + } +} + +.notifications-placeholder:focus-visible { + outline: 2px solid var(--ot-color-focus); + outline-offset: 4px; +} + +@media (max-width: 599px) { + .notifications-page { + margin-top: 1rem; + } + + .notifications-header { + align-items: stretch; + flex-direction: column; + } + + .notification-actions { + display: grid; + grid-template-columns: repeat(2, minmax(0, 1fr)); + + button { + min-width: 0; + padding-inline: 10px; + } + } +} diff --git a/src/app/common/notifications-page/notifications-page.component.spec.ts b/src/app/common/notifications-page/notifications-page.component.spec.ts new file mode 100644 index 0000000000..7aee90256c --- /dev/null +++ b/src/app/common/notifications-page/notifications-page.component.spec.ts @@ -0,0 +1,801 @@ +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {MatButtonModule} from '@angular/material/button'; +import {MatIconModule} from '@angular/material/icon'; +import {MatListModule} from '@angular/material/list'; +import {MatPaginator, MatPaginatorModule} from '@angular/material/paginator'; +import {MatProgressSpinnerModule} from '@angular/material/progress-spinner'; +import {MatTooltipModule} from '@angular/material/tooltip'; +import {By} from '@angular/platform-browser'; +import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {Router} from '@angular/router'; +import {Observable, Subject, defer, of, throwError} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {AuthenticationService} from 'src/app/api/services/authentication.service'; +import {NotificationService} from 'src/app/api/services/notification.service'; +import {NotificationOpenService} from 'src/app/common/notifications/notification-open.service'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {EmptyStateComponent} from '../empty-state/empty-state.component'; +import {ConfirmationModalService} from '../modals/confirmation-modal/confirmation-modal.service'; +import {NotificationsPageComponent} from './notifications-page.component'; + +/** + * Three of these are the states the ticket names, loading, empty and the list + * itself, and they are tested through the rendered page because that is where + * the difference between them lives. The fourth is the one the ticket does not + * name and is the same trap as in the dropdown: a request that failed must not + * be drawn as a user with no notifications. + * + * The paging tests drive the real paginator rather than calling onPage, so the + * binding between them counts. They hold the api at arm's length deliberately: + * there is one fetch and one only, and every page after the first has to come + * out of what is already here. + */ +describe('NotificationsPageComponent', () => { + let component: NotificationsPageComponent; + let fixture: ComponentFixture<NotificationsPageComponent>; + + let list: Subject<Notification[]>; + let subscribedTo: Set<string>; + let notificationService: { + list: ReturnType<typeof vi.fn>; + markRead: ReturnType<typeof vi.fn>; + markAllRead: ReturnType<typeof vi.fn>; + deleteAll: ReturnType<typeof vi.fn>; + remove: ReturnType<typeof vi.fn>; + }; + let authenticationService: {afterAuthCall: ReturnType<typeof vi.fn>}; + let confirmationModal: {show: ReturnType<typeof vi.fn>}; + let alerts: {success: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn>}; + let router: {navigateByUrl: ReturnType<typeof vi.fn>}; + let notificationOpener: {open: ReturnType<typeof vi.fn>}; + + /** + * A stand-in that records whether anybody actually subscribed. + * + * A spy that only counts calls is happy either way, and markRead returns a + * cold observable, so dropping the .subscribe() sends no request at all while + * the call count stays exactly the same. + */ + const answers = <T>(name: string, value: T): Observable<T> => + defer(() => { + subscribedTo.add(name); + return of(value); + }); + + const notification = (id: number, over: Partial<Notification> = {}): Notification => { + const built = new Notification(); + built.id = id; + built.message = `notification ${id}`; + built.link = `/projects/${id}/dashboard`; + built.notificationType = 'feedback'; + built.readAt = null; + // Newest id first, so a page of them reads in a checkable order. + built.createdAt = new Date(2026, 0, 1, 0, 0, id); + return Object.assign(built, over); + }; + + const many = (count: number): Notification[] => + Array.from({length: count}, (_unused, index) => notification(count - index)); + + const rows = (): HTMLElement[] => + Array.from(fixture.nativeElement.querySelectorAll('.notification-row')); + + const rowText = (): string[] => + rows().map((row) => row.querySelector('.notification-message').textContent.trim()); + + const pageText = (): string => fixture.nativeElement.textContent; + + const paginator = (): MatPaginator => + fixture.debugElement.query(By.directive(MatPaginator)).componentInstance; + + beforeEach(async () => { + list = new Subject<Notification[]>(); + subscribedTo = new Set<string>(); + + notificationService = { + list: vi.fn().mockReturnValue(list), + markRead: vi.fn(() => answers('markRead', undefined)), + markAllRead: vi.fn(() => answers('markAllRead', undefined)), + deleteAll: vi.fn(() => answers('deleteAll', 2)), + remove: vi.fn(() => answers('remove', undefined)), + }; + // Signed in and settled, unless a test says otherwise. + authenticationService = {afterAuthCall: vi.fn((callback) => callback(true))}; + confirmationModal = {show: vi.fn()}; + alerts = {success: vi.fn(), error: vi.fn()}; + router = {navigateByUrl: vi.fn()}; + notificationOpener = {open: vi.fn().mockResolvedValue(true)}; + + await TestBed.configureTestingModule({ + declarations: [NotificationsPageComponent], + imports: [ + EmptyStateComponent, + MatButtonModule, + MatIconModule, + MatListModule, + MatPaginatorModule, + MatProgressSpinnerModule, + MatTooltipModule, + NoopAnimationsModule, + ], + providers: [ + {provide: NotificationService, useValue: notificationService}, + {provide: AuthenticationService, useValue: authenticationService}, + {provide: ConfirmationModalService, useValue: confirmationModal}, + {provide: AlertService, useValue: alerts}, + {provide: Router, useValue: router}, + {provide: NotificationOpenService, useValue: notificationOpener}, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(NotificationsPageComponent); + component = fixture.componentInstance; + }); + + it('says it is loading before the list arrives', () => { + fixture.detectChanges(); + + expect(pageText()).toContain('Loading your notifications'); + expect(rows()).toHaveLength(0); + + list.next([notification(1)]); + fixture.detectChanges(); + + expect(pageText()).not.toContain('Loading your notifications'); + expect(rows()).toHaveLength(1); + }); + + it('says something friendly when there is nothing to show', () => { + fixture.detectChanges(); + + list.next([]); + fixture.detectChanges(); + + expect(pageText()).toContain('You have no notifications yet'); + expect(rows()).toHaveLength(0); + + // Discriminating: this fails if the empty view were skipped in favour of + // falling through to the list branch, which would render zero rows and no + // f-empty-state, but a naive text-only assertion above could still be + // fooled by stray markup. The list branch never renders this element. + expect(fixture.nativeElement.querySelector('f-empty-state')).not.toBeNull(); + }); + + it('does not render the empty state while notifications are loading or the load failed', () => { + fixture.detectChanges(); + + expect(fixture.nativeElement.querySelector('f-empty-state')).toBeNull(); + + list.error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + expect(fixture.nativeElement.querySelector('f-empty-state')).toBeNull(); + }); + + it('explains what notifications are and what the available actions do', () => { + fixture.detectChanges(); + + expect(pageText()).toContain('Comments on your tasks, feedback and reminders appear here.'); + expect(pageText()).toContain( + 'Mark all read marks notifications as read but does not remove them.', + ); + expect(pageText()).toContain( + 'Deleting a notification permanently removes it and cannot be undone.', + ); + }); + + it('keeps the notification guidance visible when loading the list fails', () => { + fixture.detectChanges(); + + list.error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + expect(pageText()).toContain('could not load'); + expect(pageText()).toContain( + 'Mark all read marks notifications as read but does not remove them.', + ); + expect(pageText()).toContain( + 'Deleting a notification permanently removes it and cannot be undone.', + ); + }); + + it('says it could not load rather than claiming there is nothing', () => { + fixture.detectChanges(); + + list.error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + expect(pageText()).toContain('could not load'); + expect(pageText()).not.toContain('You have no notifications yet'); + }); + + it('reads the list again when the retry is clicked', () => { + fixture.detectChanges(); + list.error(new Error('GET /notifications failed')); + fixture.detectChanges(); + + list = new Subject<Notification[]>(); + notificationService.list.mockReturnValue(list); + + fixture.nativeElement.querySelector('.notifications-retry').click(); + fixture.detectChanges(); + + expect(notificationService.list).toHaveBeenCalledTimes(2); + expect(pageText()).toContain('Loading your notifications'); + + list.next([notification(1)]); + fixture.detectChanges(); + + expect(rowText()).toEqual(['notification 1']); + }); + + it('shows the newest first', () => { + fixture.detectChanges(); + + list.next([notification(1), notification(3), notification(2)]); + fixture.detectChanges(); + + expect(rowText()).toEqual(['notification 3', 'notification 2', 'notification 1']); + }); + + it('makes each row a real button so a keyboard can open it', () => { + fixture.detectChanges(); + list.next([notification(1)]); + fixture.detectChanges(); + + // Structural, and it has to be. A mat-list-item on its own is static list + // content: no focus, no Enter, no Space. Only the element type gives the + // row those, and the test environment does not simulate the browser + // behaviour that would let this be asserted any other way. + expect(rows()[0].tagName).toBe('BUTTON'); + }); + + it('gives each category the same icon and colour the dropdown gives it', () => { + fixture.detectChanges(); + list.next([ + notification(5, {notificationType: 'feedback'}), + notification(4, {notificationType: 'task'}), + notification(3, {notificationType: 'portfolio'}), + notification(2, {notificationType: 'extension'}), + notification(1, {notificationType: 'general'}), + ]); + fixture.detectChanges(); + + const glyphs: HTMLElement[] = Array.from( + fixture.nativeElement.querySelectorAll('.notification-glyph'), + ); + + expect(glyphs.map((g) => g.querySelector('mat-icon').textContent.trim())).toEqual([ + 'chat_bubble', + 'assignment', + 'collections_bookmark', + 'more_time', + 'campaign', + ]); + expect(glyphs.map((g) => Array.from(g.classList).find((n) => n.startsWith('tone-')))).toEqual([ + 'tone-feedback', + 'tone-task', + 'tone-portfolio', + 'tone-extension', + 'tone-general', + ]); + }); + + it('still draws something for a category it has never heard of', () => { + fixture.detectChanges(); + list.next([notification(1, {notificationType: 'announcement'})]); + fixture.detectChanges(); + + const glyph = fixture.nativeElement.querySelector('.notification-glyph'); + + expect(glyph.querySelector('mat-icon').textContent.trim()).toBe('notifications'); + expect(Array.from(glyph.classList)).toContain('tone-general'); + }); + + it('says unread in words, not only in bold', () => { + fixture.detectChanges(); + list.next([ + notification(2, {message: 'Andrew Cain commented on 1.1P.'}), + notification(1, {message: 'Your extension was granted.', readAt: new Date(2026, 0, 2)}), + ]); + fixture.detectChanges(); + + expect(rows()[0].getAttribute('aria-label')).toContain('Unread.'); + expect(rows()[1].getAttribute('aria-label')).toContain('Read.'); + }); + + it('shows one page at a time out of a single fetch', () => { + fixture.detectChanges(); + + list.next(many(45)); + fixture.detectChanges(); + + expect(rows()).toHaveLength(20); + expect(rowText()[0]).toBe('notification 45'); + expect(paginator().length).toBe(45); + + // Through the paginator and not through onPage. Calling the handler + // directly passes just as well with the (page) binding deleted, and then + // the real Next button does nothing. + paginator().nextPage(); + fixture.detectChanges(); + + expect(rowText()[0]).toBe('notification 25'); + expect(rows()).toHaveLength(20); + + // The last page is the short one, and it is where an off by one shows up. + paginator().nextPage(); + fixture.detectChanges(); + + expect(rows()).toHaveLength(5); + expect(rowText()).toEqual([ + 'notification 5', + 'notification 4', + 'notification 3', + 'notification 2', + 'notification 1', + ]); + + // One request for the whole thing. Paging is over what is already here, + // because the api has no page parameters to send. + expect(notificationService.list).toHaveBeenCalledTimes(1); + }); + + it('follows a change of page size', () => { + fixture.detectChanges(); + list.next(many(45)); + fixture.detectChanges(); + + paginator()._changePageSize(50); + fixture.detectChanges(); + + expect(rows()).toHaveLength(45); + }); + + it('does not strand the reader past the end when a reload is shorter', () => { + fixture.detectChanges(); + list.next(many(45)); + fixture.detectChanges(); + + paginator().nextPage(); + paginator().nextPage(); + fixture.detectChanges(); + expect(rows()).toHaveLength(5); + + // Same page number, far fewer notifications behind it. Left alone this + // renders an empty page with a paginator insisting it is page three of one. + list = new Subject<Notification[]>(); + notificationService.list.mockReturnValue(list); + component.load(); + list.next(many(4)); + fixture.detectChanges(); + + expect(component.pageIndex).toBe(0); + expect(rows()).toHaveLength(4); + }); + + it('marks a row read and opens it when it is clicked', () => { + fixture.detectChanges(); + list.next([notification(1, {link: '/projects/9/dashboard'})]); + fixture.detectChanges(); + + rows()[0].click(); + + expect(notificationService.markRead).toHaveBeenCalledTimes(1); + expect(notificationService.markRead.mock.calls[0][0].id).toBe(1); + expect(subscribedTo.has('markRead')).toBe(true); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + expect(notificationOpener.open.mock.calls[0][0].id).toBe(1); + }); + + it('does not mark a row read twice', () => { + fixture.detectChanges(); + list.next([notification(1, {readAt: new Date(2026, 0, 2)})]); + fixture.detectChanges(); + + rows()[0].click(); + + expect(notificationService.markRead).not.toHaveBeenCalled(); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + }); + + it('marks a row read even when it has nowhere to go', () => { + fixture.detectChanges(); + list.next([notification(1, {link: null})]); + fixture.detectChanges(); + + rows()[0].click(); + + // link is nullable on the api. Refusing to mark it read would leave a + // number on the bell that the user has no way to clear. Whether there is + // anywhere to go is the opener's call, so it is still asked. + expect(notificationService.markRead).toHaveBeenCalledTimes(1); + expect(notificationOpener.open).toHaveBeenCalledTimes(1); + }); + + it('says so when a notification could not be marked as read', () => { + notificationService.markRead.mockReturnValue(throwError(() => new Error('PUT read failed'))); + + fixture.detectChanges(); + list.next([notification(1, {link: null})]); + fixture.detectChanges(); + + rows()[0].click(); + + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + /** + * The split that matters is which action asks first. Marking read destroys + * nothing and a dialog on it is a step to click through; delete has no undo + * anywhere in this feature and gets one. + * + * The one thing here that the dropdown has no version of is the paging. Five + * rows in a menu have no pages to fall off the end of. + */ + describe('the bulk actions', () => { + // The dialog is a callback, not a promise. show() is handed the function to + // run on confirm and the one to run on cancel, so a test drives it by + // reaching for whichever of those it wants. + const confirmed = (): void => confirmationModal.show.mock.calls[0][2](); + const cancelled = (): void => confirmationModal.show.mock.calls[0][3](); + + const deleteButtons = (): HTMLElement[] => + Array.from(fixture.nativeElement.querySelectorAll('.notification-delete')); + + const markAllButton = (): HTMLElement => + fixture.nativeElement.querySelector('.notification-mark-all'); + + const read = (id: number) => notification(id, {readAt: new Date(2026, 0, 2)}); + + // Swap in a fresh list request and reload through it, so a test can hold the + // second response open or answer it with something different. + const reloadWith = (): Subject<Notification[]> => { + const refresh: Subject<Notification[]> = new Subject(); + notificationService.list.mockReturnValue(refresh); + component.load(); + fixture.detectChanges(); + return refresh; + }; + + beforeEach(() => { + fixture.detectChanges(); + list.next([notification(2), read(1)]); + fixture.detectChanges(); + }); + + it('marks everything read without asking first', () => { + component.markAllRead(); + + expect(notificationService.markAllRead).toHaveBeenCalledTimes(1); + expect(subscribedTo.has('markAllRead')).toBe(true); + expect(confirmationModal.show).not.toHaveBeenCalled(); + expect(alerts.success).toHaveBeenCalledWith('All notifications marked as read'); + }); + + it('shows both bulk actions with visible labels on the page', () => { + expect(pageText()).toContain('Mark all read'); + expect(pageText()).toContain('Delete all'); + }); + + it('asks with the affected count before deleting all', () => { + component.confirmDeleteAll(); + + expect(confirmationModal.show).toHaveBeenCalledTimes(1); + expect(confirmationModal.show.mock.calls[0][0]).toBe('Delete all notifications'); + expect(confirmationModal.show.mock.calls[0][1]).toContain('all 2 notifications'); + expect(notificationService.deleteAll).not.toHaveBeenCalled(); + }); + + it('deletes the confirmed snapshot and reaches the empty state', () => { + component.confirmDeleteAll(); + confirmed(); + fixture.detectChanges(); + + expect(notificationService.deleteAll).toHaveBeenCalledWith(2); + expect(subscribedTo.has('deleteAll')).toBe(true); + expect(component.notifications).toHaveLength(0); + expect(pageText()).toContain('You have no notifications yet'); + expect(alerts.success).toHaveBeenCalledWith('2 notifications deleted'); + }); + + it('does not delete all when the confirmation is cancelled', () => { + component.confirmDeleteAll(); + cancelled(); + + expect(notificationService.deleteAll).not.toHaveBeenCalled(); + expect(component.notifications).toHaveLength(2); + }); + + it('keeps a notification that arrives after the confirmed boundary', () => { + component.confirmDeleteAll(); + component.notifications.unshift(notification(3)); + + confirmed(); + fixture.detectChanges(); + + expect(notificationService.deleteAll).toHaveBeenCalledWith(2); + expect(component.notifications.map((row) => row.id)).toEqual([3]); + }); + + it('keeps every row and reports an error when delete all fails', () => { + notificationService.deleteAll.mockReturnValue( + throwError(() => new Error('DELETE all failed')), + ); + + component.confirmDeleteAll(); + confirmed(); + fixture.detectChanges(); + + expect(component.notifications).toHaveLength(2); + expect(alerts.error).toHaveBeenCalledWith('Your notifications could not be deleted'); + }); + + it('keeps the focused action available but disabled when nothing is unread', () => { + const refresh = reloadWith(); + refresh.next([read(2), read(1)]); + fixture.detectChanges(); + + expect(markAllButton()).not.toBeNull(); + expect(markAllButton().getAttribute('aria-disabled')).toBe('true'); + + component.markAllRead(); + + expect(notificationService.markAllRead).not.toHaveBeenCalled(); + }); + + it('does not send mark all read twice while the request is pending', () => { + const pending: Subject<void> = new Subject(); + notificationService.markAllRead.mockReturnValue(pending); + + component.markAllRead(); + component.markAllRead(); + + expect(notificationService.markAllRead).toHaveBeenCalledTimes(1); + expect(component.markAllReadPending).toBe(true); + + pending.next(); + + expect(component.markAllReadPending).toBe(false); + }); + + it('keeps focus on mark all read after the rows become read', () => { + notificationService.markAllRead.mockImplementation(() => + defer(() => { + component.notifications.forEach((row) => (row.readAt = new Date())); + return of(undefined); + }), + ); + + const button = markAllButton(); + button.focus(); + button.click(); + fixture.detectChanges(); + + expect(document.activeElement).toBe(button); + expect(button.getAttribute('aria-disabled')).toBe('true'); + }); + + it('offers it before the shared unread-count refresh completes', () => { + // The button is drawn from the rows this page loaded and not from + // NotificationService.unreadCount$. That subject seeds at zero and is + // refreshed independently by the bell, so it can still be zero while the + // page has already loaded unread rows. + expect(markAllButton()).not.toBeNull(); + expect(notificationService.list).toHaveBeenCalledTimes(1); + }); + + it('says so when marking everything read fails', () => { + notificationService.markAllRead.mockReturnValue( + throwError(() => new Error('PUT read_all failed')), + ); + + markAllButton().click(); + + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + it('asks before it deletes anything', () => { + deleteButtons()[0].click(); + + expect(confirmationModal.show).toHaveBeenCalledTimes(1); + expect(notificationService.remove).not.toHaveBeenCalled(); + expect(confirmationModal.show.mock.calls[0][1]).toContain('notification 2'); + }); + + it('deletes only once the dialog is agreed to', () => { + deleteButtons()[0].click(); + confirmed(); + + expect(notificationService.remove).toHaveBeenCalledTimes(1); + expect(notificationService.remove.mock.calls[0][0].id).toBe(2); + expect(subscribedTo.has('remove')).toBe(true); + + fixture.detectChanges(); + expect(rowText()).toEqual(['notification 1']); + }); + + it('does not delete the same notification twice while the first request is pending', () => { + const pending: Subject<void> = new Subject(); + notificationService.remove.mockReturnValue(pending); + + deleteButtons()[0].click(); + confirmed(); + confirmed(); + fixture.detectChanges(); + + expect(notificationService.remove).toHaveBeenCalledTimes(1); + expect(deleteButtons()[0].getAttribute('aria-disabled')).toBe('true'); + + pending.next(); + fixture.detectChanges(); + + expect(component.isDeleting(notificationService.remove.mock.calls[0][0])).toBe(false); + }); + + it('moves focus to the row that replaces a deleted row', () => { + const button = deleteButtons()[0]; + button.click(); + button.focus(); + + confirmed(); + + expect(rowText()).toEqual(['notification 1']); + expect(document.activeElement).toBe(rows()[0]); + }); + + it('leaves the notification alone when the dialog is cancelled', () => { + deleteButtons()[0].click(); + cancelled(); + fixture.detectChanges(); + + expect(notificationService.remove).not.toHaveBeenCalled(); + expect(rowText()).toEqual(['notification 2', 'notification 1']); + }); + + it('keeps the row when the delete fails', () => { + notificationService.remove.mockReturnValue(throwError(() => new Error('DELETE failed'))); + + const lastPageDelete = deleteButtons()[0]; + lastPageDelete.click(); + lastPageDelete.focus(); + confirmed(); + fixture.detectChanges(); + + // Taking the row out first and putting it back on failure would be worse + // than leaving it. The user reads the row vanishing as the delete having + // worked. + expect(rowText()).toEqual(['notification 2', 'notification 1']); + expect(alerts.error).toHaveBeenCalledTimes(1); + }); + + it('does not open the notification when its delete button is clicked', () => { + deleteButtons()[0].click(); + + // The delete button is a sibling of the row rather than inside it, and + // that is what keeps this true without stopping the event. Nesting them + // would make asking to delete one mark it read and navigate away from the + // page it was asked on. + expect(notificationService.markRead).not.toHaveBeenCalled(); + expect(notificationOpener.open).not.toHaveBeenCalled(); + }); + + it('names the notification on its delete button', () => { + // Twenty rows means twenty of these buttons. Identical labels leave + // somebody tabbing through them hearing the same three words over and + // over with nothing to tell them apart. + expect(deleteButtons()[0].getAttribute('aria-label')).toBe('Delete: notification 2'); + expect(deleteButtons()[1].getAttribute('aria-label')).toBe('Delete: notification 1'); + }); + + it('pulls the reader back when the last row on the last page is deleted', () => { + const refresh = reloadWith(); + refresh.next(many(21)); + fixture.detectChanges(); + + paginator().nextPage(); + fixture.detectChanges(); + expect(rows()).toHaveLength(1); + + deleteButtons()[0].click(); + confirmed(); + fixture.detectChanges(); + + // Without the clamp the reader is left on page two of a list that now has + // one page. That draws no rows at all and does not take the empty state + // either, because there are still twenty notifications. + expect(component.pageIndex).toBe(0); + expect(rows()).toHaveLength(20); + expect(document.activeElement).toBe(rows()[0]); + }); + + it('shows the empty state once the last one is deleted', () => { + const refresh = reloadWith(); + refresh.next([notification(1)]); + fixture.detectChanges(); + + deleteButtons()[0].click(); + confirmed(); + fixture.detectChanges(); + + expect(pageText()).toContain('You have no notifications yet'); + expect(alerts.success).toHaveBeenCalledTimes(1); + }); + + it('drops a list read still in flight when everything is marked read', () => { + const refresh = reloadWith(); + expect(refresh.observed).toBe(true); + + component.markAllRead(); + + // That response was worked out before the click, and NotificationService + // writes list responses into the shared entity cache, so letting it land + // would put readAt back to null on every row. + expect(refresh.observed).toBe(false); + }); + + it('drops a list read still in flight when one is deleted', () => { + // Driven through the component rather than the dom, and that is worth a + // word. A reload replaces the rows with the spinner, so while one is in + // flight there is no delete button on screen to click, and the sequence + // this guards cannot be produced by clicking today. It is reachable the + // moment anything reloads this page on a timer or a navigation, which is + // what the bell already does to the count, and the response is written + // into a cache the whole app shares. Cheap guard, real hazard. + component.confirmDelete(component.visible[0]); + const refresh = reloadWith(); + expect(refresh.observed).toBe(true); + + confirmed(); + + // Worse here than for a mark read. That response still holds the row that + // was just deleted, so landing late would put it back on screen. + expect(refresh.observed).toBe(false); + }); + + it('does not leave a spinner over the list when a reload is abandoned', () => { + reloadWith(); + expect(pageText()).toContain('Loading your notifications'); + + component.markAllRead(); + fixture.detectChanges(); + + // Cancelling the request without clearing the flag sits a spinner over a + // list that nothing is going to replace now. + expect(pageText()).not.toContain('Loading your notifications'); + expect(rows()).toHaveLength(2); + }); + }); + + describe('before anyone is signed in', () => { + it('asks nothing of the api until authentication has settled', () => { + let resume: (signedIn: boolean) => void; + authenticationService.afterAuthCall.mockImplementation((callback) => { + resume = callback; + }); + + fixture.detectChanges(); + + // An anonymous GET /notifications comes back 403, and HttpErrorInterceptor + // reads a 403 on an anonymous user as an expired session: it alerts + // "Authentication timed out" and redirects to /timeout. Opening a + // bookmark while signed out would look like being thrown out. + expect(notificationService.list).not.toHaveBeenCalled(); + + resume(true); + fixture.detectChanges(); + + expect(notificationService.list).toHaveBeenCalledTimes(1); + }); + + it('sends the visitor to sign in instead of asking the api', () => { + authenticationService.afterAuthCall.mockImplementation((callback) => callback(false)); + + fixture.detectChanges(); + + expect(notificationService.list).not.toHaveBeenCalled(); + expect(router.navigateByUrl).toHaveBeenCalledWith('/sign_in'); + }); + }); +}); diff --git a/src/app/common/notifications-page/notifications-page.component.ts b/src/app/common/notifications-page/notifications-page.component.ts new file mode 100644 index 0000000000..2ea4f2625c --- /dev/null +++ b/src/app/common/notifications-page/notifications-page.component.ts @@ -0,0 +1,439 @@ +import moment from 'moment'; +import { + ChangeDetectionStrategy, + ChangeDetectorRef, + Component, + ElementRef, + OnDestroy, + OnInit, +} from '@angular/core'; +import {PageEvent} from '@angular/material/paginator'; +import {Router} from '@angular/router'; +import {Subscription} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {AuthenticationService} from 'src/app/api/services/authentication.service'; +import {NotificationService} from 'src/app/api/services/notification.service'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {ConfirmationModalService} from '../modals/confirmation-modal/confirmation-modal.service'; +import {NotificationOpenService} from '../notifications/notification-open.service'; +import {presentationFor} from '../notifications/notification-presentation'; + +/** + * The whole notification list, on a page of its own. + * + * Lives under common/ next to the other routed components that are not part of + * a unit or a project, scorm-player and submission-files-download. + * + * Paged here and not by the api. GET /notifications takes unread_only and + * nothing else and answers with every row the user has, so this holds the lot + * and shows a page of it. That is fine at the sizes this feature produces and + * it is wrong at some size, which is an api ticket and not this one. + */ +@Component({ + selector: 'f-notifications-page', + templateUrl: './notifications-page.component.html', + styleUrls: ['./notifications-page.component.scss'], + changeDetection: ChangeDetectionStrategy.Eager, + standalone: false, +}) +export class NotificationsPageComponent implements OnInit, OnDestroy { + public readonly pageSizeOptions: number[] = [10, 20, 50]; + + notifications: Notification[] = []; + + /** + * The rows actually on screen. + * + * Held rather than worked out in the template. A getter that slices would + * build a new array on every change detection pass, and with a click handler + * on every row there are a lot of those. + */ + visible: Notification[] = []; + + loading = true; + loadFailed = false; + + pageIndex = 0; + pageSize = 20; + + private listSubscription: Subscription | null = null; + private readonly deletingNotificationIds: Set<number> = new Set(); + markAllReadPending = false; + deleteAllPending = false; + + constructor( + private notificationService: NotificationService, + private authenticationService: AuthenticationService, + private confirmationModal: ConfirmationModalService, + private alerts: AlertService, + private router: Router, + private changeDetectorRef: ChangeDetectorRef, + private elementRef: ElementRef<HTMLElement>, + private notificationOpener: NotificationOpenService, + ) {} + + /** + * Whether the mark all read button has anything to do. + * + * Worked out from the rows this page is holding, and deliberately not from + * NotificationService.unreadCount$. That subject seeds at zero and is updated + * independently by the bell. Reading it here would make a page action depend + * on whether another component's refresh has completed rather than on the rows + * the action will actually change. + * + * A getter rather than a field kept in step by hand. It has to answer after a + * mark all read, after a delete, after a row is opened and after a reload, and + * a boolean that four call sites have to remember to update is a boolean that + * will eventually be wrong. Nothing is allocated, so unlike the visible slice + * this is safe to recompute on every change detection pass. + */ + get hasUnread(): boolean { + return this.notifications.some((notification) => !notification.isRead); + } + + /** + * Wait for authentication before asking the api for anything. + * + * The route has no guard, so this component is built for whoever opens the + * url, signed in or not. An anonymous GET /notifications comes back 403, and + * HttpErrorInterceptor reads a 403 on an anonymous user as an expired + * session: it shows "Authentication timed out" and redirects to /timeout, + * instead of the sign in page somebody following a stale bookmark should get. + * + * afterAuthCall is what EditProfileComponent does for the same reason, and it + * waits rather than guessing, so a page opened while the session is still + * being restored still loads. + */ + ngOnInit(): void { + this.authenticationService.afterAuthCall((signedIn) => { + if (!signedIn) { + this.router.navigateByUrl('/sign_in'); + return; + } + + this.load(); + }); + } + + ngOnDestroy(): void { + this.listSubscription?.unsubscribe(); + } + + /** + * Read every notification, once. + * + * Also the retry. A failed load is the one state on this page with nothing + * useful on it, so the way out has to be on the page rather than a refresh of + * the browser. + */ + load(): void { + this.loading = true; + this.loadFailed = false; + + this.listSubscription?.unsubscribe(); + + this.listSubscription = this.notificationService.list().subscribe({ + next: (notifications) => { + this.notifications = [...notifications].sort( + (a, b) => b.createdAt.getTime() - a.createdAt.getTime(), + ); + + // A retry after a run of deletes can leave the reader stranded on a + // page that no longer exists, staring at nothing. + this.pageIndex = Math.min(this.pageIndex, this.lastPageIndex()); + this.loading = false; + this.updateVisible(); + }, + error: () => { + this.loading = false; + this.loadFailed = true; + }, + }); + } + + onPage(event: PageEvent): void { + this.pageIndex = event.pageIndex; + this.pageSize = event.pageSize; + this.updateVisible(); + } + + /** + * Open one notification: mark it read, then go where it points. + * + * The same shape as the dropdown in the header. They are on separate branches + * so neither can call the other today, and once both have merged this belongs + * in one place. + */ + open(notification: Notification): void { + if (!notification.isRead) { + // A read that went out before this click is out of date now, and + // NotificationService writes list responses into the shared entity cache, + // so one landing late would put readAt back to null. + this.cancelPendingList(); + + this.notificationService.markRead(notification).subscribe({ + error: () => this.alerts.error('That notification could not be marked as read'), + }); + } + + // Where it goes, for a student or for staff, and what happens when the + // thing it was about has gone, is all decided in NotificationOpenService. + void this.notificationOpener.open(notification); + } + + /** + * Mark the lot as read. + * + * Every one of them and not only the page on screen. read_all is the endpoint + * and it takes no arguments, and a button labelled "mark all read" that left + * pages two onwards unread would be lying about what it did. + * + * No confirmation. Nothing is lost by it and every notification is still here + * to read afterwards, so a dialog would only be a step to click through. + * Delete is the one that asks. + * + * The rows redraw without anything here touching them. NotificationService + * writes readAt onto the entities in its cache, and the rows this page is + * holding came out of that same cache, so they are the same objects. Setting + * readAt again here would be a second copy of that arithmetic that could only + * ever come to disagree with it. + */ + markAllRead(): void { + if (!this.hasUnread || this.markAllReadPending || this.deleteAllPending) { + return; + } + + this.markAllReadPending = true; + + // A list read raised before this is out of date now, and its response goes + // straight into the shared cache, so landing late would draw every row + // unread again. + this.cancelPendingList(); + + this.notificationService.markAllRead().subscribe({ + next: () => { + this.markAllReadPending = false; + this.alerts.success('All notifications marked as read'); + }, + error: () => { + this.markAllReadPending = false; + this.alerts.error('Your notifications could not be marked as read'); + }, + }); + } + + /** + * Ask first, then delete. + * + * Delete is the only one of the five endpoints that destroys anything and + * there is no undo anywhere in this feature, so it gets the dialog the house + * already has rather than a new one. + * + * The empty cancel function is deliberate. ConfirmationModalService pops a + * green success snackbar reading "<title> action cancelled" when none is + * given, and telling somebody they successfully did not delete something is + * noise. + */ + confirmDelete(notification: Notification): void { + if (this.isDeleting(notification) || this.deleteAllPending) { + return; + } + + this.confirmationModal.show( + 'Delete notification', + `Delete "${notification.message}"? This removes the notification permanently.`, + () => this.remove(notification), + () => undefined, + 'Delete', + ); + } + + /** + * Confirm one bounded bulk delete. + * + * The count and highest id come from the same loaded snapshot. The API uses + * the id as an inclusive boundary, so a notification arriving while the + * confirmation is open is not silently deleted. + */ + confirmDeleteAll(): void { + if (!this.notifications.length || this.deleteAllPending) { + return; + } + + const throughId = Math.max(...this.notifications.map((notification) => notification.id)); + const affectedCount = this.notifications.filter( + (notification) => notification.id <= throughId, + ).length; + const noun = affectedCount === 1 ? 'notification' : 'notifications'; + + this.confirmationModal.show( + 'Delete all notifications', + `Delete all ${affectedCount} ${noun}? This cannot be undone. New notifications that arrive after this confirmation will be kept.`, + () => this.removeAll(throughId), + () => undefined, + 'Delete all', + ); + } + + iconFor(notification: Notification): string { + return presentationFor(notification).icon; + } + + toneFor(notification: Notification): string { + return presentationFor(notification).tone; + } + + labelFor(notification: Notification): string { + return presentationFor(notification).label; + } + + /** + * What a screen reader should read for a row. + * + * Unread is drawn as bold text, and that is a thing you have to be able to + * see. The message and the time are already in the button, so this only + * exists to put the state into words. + */ + rowLabel(notification: Notification): string { + const state = notification.isRead ? 'Read' : 'Unread'; + + return `${state}. ${this.labelFor(notification)}. ${notification.message}. ${this.timeAgo(notification)}`; + } + + /** + * What a screen reader should call a delete button. + * + * The message and not a bare "Delete notification". A page of twenty rows has + * twenty of these buttons, and identical labels leave somebody tabbing through + * them hearing the same three words over and over with nothing to tell them + * apart. The dropdown gets away with the generic wording because it shows five + * rows at most and the delete sits beside the row you just heard. + */ + deleteLabel(notification: Notification): string { + return `Delete: ${notification.message}`; + } + + isDeleting(notification: Notification): boolean { + return this.deleteAllPending || this.deletingNotificationIds.has(notification.id); + } + + timeAgo(notification: Notification): string { + return moment(notification.createdAt).fromNow(); + } + + /** + * Retire a list request that is still in the air. + * + * Anything changed from this page is newer than a read that went out before + * it. NotificationService writes list responses into the shared cache, so one + * arriving late does not merely show stale rows, it undoes the change. + * + * loading is cleared with it. A reload that has been abandoned is not still + * loading, and leaving the flag set would sit a spinner over the list nobody + * is going to replace now. + */ + private cancelPendingList(): void { + this.listSubscription?.unsubscribe(); + this.listSubscription = null; + this.loading = false; + } + + /** + * Delete it for real, once the dialog has been agreed to. + * + * The row is dropped from the held list on success rather than by reading the + * list again. A second GET for something already known would leave the row on + * screen for a round trip after the user asked for it to go. + * + * On failure the row stays exactly where it is. Removing it first and putting + * it back would read as the delete having worked and then being undone by + * something, which is a worse story than it not having worked. + */ + private remove(notification: Notification): void { + if (this.isDeleting(notification)) { + return; + } + + this.deletingNotificationIds.add(notification.id); + this.cancelPendingList(); + + const visibleIndex = this.visible.indexOf(notification); + + this.notificationService.remove(notification).subscribe({ + next: () => { + this.deletingNotificationIds.delete(notification.id); + this.notifications = this.notifications.filter((row) => row !== notification); + + // Deleting the only row on the last page leaves the reader on a page + // that no longer exists, staring at nothing while the paginator insists + // there are three of them. Same clamp as a reload does. + this.pageIndex = Math.min(this.pageIndex, this.lastPageIndex()); + this.updateVisible(); + this.alerts.success('Notification deleted'); + this.changeDetectorRef.detectChanges(); + this.restoreFocusAfterDelete(visibleIndex); + }, + error: () => { + this.deletingNotificationIds.delete(notification.id); + this.alerts.error('That notification could not be deleted'); + }, + }); + } + + private removeAll(throughId: number): void { + if (this.deleteAllPending || !this.notifications.length) { + return; + } + + this.deleteAllPending = true; + this.cancelPendingList(); + + this.notificationService.deleteAll(throughId).subscribe({ + next: (deletedCount) => { + this.deleteAllPending = false; + this.notifications = this.notifications.filter( + (notification) => notification.id > throughId, + ); + this.pageIndex = Math.min(this.pageIndex, this.lastPageIndex()); + this.updateVisible(); + + const message = + deletedCount === 1 ? '1 notification deleted' : `${deletedCount} notifications deleted`; + this.alerts.success(message); + this.changeDetectorRef.detectChanges(); + this.restoreFocusAfterDelete(0); + }, + error: () => { + this.deleteAllPending = false; + this.alerts.error('Your notifications could not be deleted'); + }, + }); + } + + private restoreFocusAfterDelete(previousIndex: number): void { + if (document.activeElement !== document.body) { + return; + } + + const rows = Array.from( + this.elementRef.nativeElement.querySelectorAll<HTMLElement>('.notification-row'), + ); + const nextIndex = Math.min(Math.max(previousIndex, 0), rows.length - 1); + + if (rows[nextIndex]) { + rows[nextIndex].focus(); + return; + } + + this.elementRef.nativeElement.querySelector<HTMLElement>('.notifications-placeholder')?.focus(); + } + + private updateVisible(): void { + const start = this.pageIndex * this.pageSize; + this.visible = this.notifications.slice(start, start + this.pageSize); + } + + private lastPageIndex(): number { + return Math.max(0, Math.ceil(this.notifications.length / this.pageSize) - 1); + } +} diff --git a/src/app/common/notifications/notification-open.service.spec.ts b/src/app/common/notifications/notification-open.service.spec.ts new file mode 100644 index 0000000000..dca61ee61b --- /dev/null +++ b/src/app/common/notifications/notification-open.service.spec.ts @@ -0,0 +1,193 @@ +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {Router} from '@angular/router'; +import {BehaviorSubject} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {NotificationRouteService} from 'src/app/api/services/notification-route.service'; +import {UserService} from 'src/app/api/services/user.service'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {GlobalStateService} from 'src/app/projects/states/index/global-state.service'; +import { + NOTIFICATION_UNAVAILABLE_MESSAGE, + NotificationOpenService, +} from './notification-open.service'; + +describe('NotificationOpenService', () => { + let router: { + url: string; + createUrlTree: ReturnType<typeof vi.fn>; + serializeUrl: ReturnType<typeof vi.fn>; + }; + let routes: {navigate: ReturnType<typeof vi.fn>; navigateToTarget: ReturnType<typeof vi.fn>}; + let users: {currentUser: {id: number; role: string}}; + let loading: BehaviorSubject<boolean>; + let unitRoles: {unit: {id: number}}[]; + let alerts: {error: ReturnType<typeof vi.fn>}; + let service: NotificationOpenService; + + function fields(overrides: Partial<Notification>): Notification { + return Object.assign(new Notification(), { + event: 'task_comment_created', + notificationType: 'feedback', + link: '/projects/9/dashboard/2.2C/feedback', + unitId: 3, + projectId: 9, + studentId: 50, + taskDefinitionAbbr: '2.2C', + ...overrides, + }); + } + + beforeEach(() => { + router = { + url: '/notifications', + // Stands in for the real router: joins the commands and appends the query. + createUrlTree: vi.fn((commands: (string | number)[], extras: {queryParams?: object}) => ({ + commands, + queryParams: extras?.queryParams, + })), + serializeUrl: vi.fn((tree: {commands: (string | number)[]; queryParams?: object}) => { + const path = tree.commands.join('/').replace(/^\/+/, '/'); + const query = tree.queryParams + ? `?${new URLSearchParams(tree.queryParams as Record<string, string>).toString()}` + : ''; + return `${path}${query}`; + }), + }; + routes = { + navigate: vi.fn().mockResolvedValue(true), + navigateToTarget: vi.fn().mockResolvedValue(true), + }; + users = {currentUser: {id: 50, role: 'Student'}}; + loading = new BehaviorSubject(false); + unitRoles = []; + alerts = {error: vi.fn()}; + + service = new NotificationOpenService( + router as unknown as Router, + routes as unknown as NotificationRouteService, + users as unknown as UserService, + { + isLoadingSubject: loading, + loadedUnitRoles: { + get currentValues() { + return unitRoles; + }, + }, + } as unknown as GlobalStateService, + alerts as unknown as AlertService, + ); + }); + + it('opens a student comment on the feedback pane', async () => { + await expect(service.open(fields({}))).resolves.toBe(true); + + expect(routes.navigateToTarget).toHaveBeenCalledWith('/projects/9/dashboard/2.2C/feedback'); + expect(alerts.error).not.toHaveBeenCalled(); + }); + + it('opens a staff notification in the inbox when the tutor teaches the unit', async () => { + users.currentUser = {id: 7, role: 'Tutor'}; + unitRoles = [{unit: {id: 3}}]; + + await expect(service.open(fields({event: 'task_submitted'}))).resolves.toBe(true); + + expect(routes.navigateToTarget).toHaveBeenCalledWith( + '/units/3/tasks/inbox/50/2.2C?students=all', + ); + }); + + it('waits for unit roles to load before deciding', async () => { + users.currentUser = {id: 7, role: 'Tutor'}; + loading.next(true); + + const opened = service.open(fields({event: 'task_submitted'})); + await Promise.resolve(); + expect(routes.navigateToTarget).not.toHaveBeenCalled(); + + unitRoles = [{unit: {id: 3}}]; + loading.next(false); + + await expect(opened).resolves.toBe(true); + }); + + it('stays put and says so when staff no longer have a role in the unit', async () => { + users.currentUser = {id: 7, role: 'Tutor'}; + unitRoles = [{unit: {id: 99}}]; + + await expect(service.open(fields({event: 'task_submitted'}))).resolves.toBe(false); + + expect(routes.navigateToTarget).not.toHaveBeenCalled(); + expect(alerts.error).toHaveBeenCalledWith(NOTIFICATION_UNAVAILABLE_MESSAGE); + }); + + it('lets an admin through without a unit role', async () => { + users.currentUser = {id: 7, role: 'Admin'}; + + await expect(service.open(fields({event: 'task_submitted'}))).resolves.toBe(true); + }); + + it('stays put and says so when the task was deleted', async () => { + await expect( + service.open(fields({taskDefinitionAbbr: null, taskDefinitionId: null})), + ).resolves.toBe(false); + + expect(routes.navigateToTarget).not.toHaveBeenCalled(); + expect(alerts.error).toHaveBeenCalledWith(NOTIFICATION_UNAVAILABLE_MESSAGE); + }); + + it('says so when the navigation is refused', async () => { + routes.navigateToTarget.mockResolvedValue(false); + + await expect(service.open(fields({}))).resolves.toBe(false); + + expect(alerts.error).toHaveBeenCalledWith(NOTIFICATION_UNAVAILABLE_MESSAGE); + }); + + it('says so when the navigation fails', async () => { + routes.navigateToTarget.mockRejectedValue(new Error('resolver failed')); + + await expect(service.open(fields({}))).resolves.toBe(false); + + expect(alerts.error).toHaveBeenCalledWith(NOTIFICATION_UNAVAILABLE_MESSAGE); + }); + + it('opens a Unit Hub announcement on the hub without asking for a staff role', async () => { + users.currentUser = {id: 7, role: 'Tutor'}; + loading.next(true); + + const notification = fields({ + event: 'unit_announcement_published', + notificationType: 'unit_hub', + link: '/unit-hub?unit=3&announcement=12', + projectId: null, + studentId: null, + taskDefinitionAbbr: null, + announcementId: 12, + sessionId: null, + }); + + await expect(service.open(notification)).resolves.toBe(true); + + expect(routes.navigateToTarget).toHaveBeenCalledWith('/unit-hub?unit=3&announcement=12'); + }); + + it('follows the link from an older api that sends no ids', async () => { + const legacy = Object.assign(new Notification(), { + event: 'task_comment_created', + link: '/projects/9/dashboard', + }); + + await service.open(legacy); + + expect(routes.navigate).toHaveBeenCalledWith('/projects/9/dashboard'); + expect(routes.navigateToTarget).not.toHaveBeenCalled(); + }); + + it('does nothing for a notification that was never about a page', async () => { + await expect(service.open(fields({link: null, projectId: null}))).resolves.toBe(false); + + expect(routes.navigate).not.toHaveBeenCalled(); + expect(routes.navigateToTarget).not.toHaveBeenCalled(); + expect(alerts.error).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/common/notifications/notification-open.service.ts b/src/app/common/notifications/notification-open.service.ts new file mode 100644 index 0000000000..3c12b930a3 --- /dev/null +++ b/src/app/common/notifications/notification-open.service.ts @@ -0,0 +1,91 @@ +import {Injectable} from '@angular/core'; +import {Router} from '@angular/router'; +import {filter, firstValueFrom, take} from 'rxjs'; +import {Notification} from 'src/app/api/models/notification'; +import {NotificationRouteService} from 'src/app/api/services/notification-route.service'; +import {UserService} from 'src/app/api/services/user.service'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {GlobalStateService} from 'src/app/projects/states/index/global-state.service'; +import {notificationTarget} from './notification-target'; + +export const NOTIFICATION_UNAVAILABLE_MESSAGE = 'That item is no longer available'; + +/** + * Open the page a notification is about, from the bell or the full page. + * + * notificationTarget decides where. This does the parts that need the running + * app: whether a member of staff still has a role in that unit, the navigation + * itself, and the snackbar when either says no. The reader always stays where + * they are in that case, never on a blank page or on /unauthorised. + */ +@Injectable({providedIn: 'root'}) +export class NotificationOpenService { + constructor( + private router: Router, + private routes: NotificationRouteService, + private users: UserService, + private globalState: GlobalStateService, + private alerts: AlertService, + ) {} + + async open(notification: Notification): Promise<boolean> { + const target = notificationTarget(notification, this.users.currentUser); + + switch (target.kind) { + case 'none': + return false; + case 'link': + return this.routes.navigate(target.link); + case 'unavailable': + return this.unavailable(); + } + + if (target.audience === 'staff' && !(await this.canViewUnit(Number(target.commands[1])))) { + return this.unavailable(); + } + + const url = this.router.serializeUrl( + this.router.createUrlTree(target.commands, {queryParams: target.queryParams}), + ); + + try { + const navigated = await this.routes.navigateToTarget(url); + if (!navigated && !this.router.url.startsWith(url)) { + return this.unavailable(); + } + return navigated; + } catch { + return this.unavailable(); + } + } + + /** + * Whether the signed in user still holds a role in the unit. + * + * Asked before navigating because the unit routes answer a missing role by + * redirecting to /unauthorised, which is exactly the dead end this avoids. + * Unit roles load once after sign in, so wait for that first. + */ + private async canViewUnit(unitId: number): Promise<boolean> { + const role = this.users.currentUser?.role; + if (role === 'Admin' || role === 'Auditor') { + return true; + } + + await firstValueFrom( + this.globalState.isLoadingSubject.pipe( + filter((loading) => !loading), + take(1), + ), + ); + + return this.globalState.loadedUnitRoles.currentValues.some( + (unitRole) => unitRole.unit?.id === unitId, + ); + } + + private unavailable(): false { + this.alerts.error(NOTIFICATION_UNAVAILABLE_MESSAGE); + return false; + } +} diff --git a/src/app/common/notifications/notification-target.spec.ts b/src/app/common/notifications/notification-target.spec.ts new file mode 100644 index 0000000000..0c31fa8f79 --- /dev/null +++ b/src/app/common/notifications/notification-target.spec.ts @@ -0,0 +1,256 @@ +import {describe, expect, it} from 'vitest'; +import {Notification} from 'src/app/api/models/notification'; +import {NotificationTarget, notificationTarget} from './notification-target'; + +const STUDENT = {id: 50}; +const TUTOR = {id: 7}; + +const UNIT = 3; +const PROJECT = 9; +const ABBR = '2.2C'; + +function notification(fields: Partial<Notification>): Notification { + return Object.assign(new Notification(), fields); +} + +// What the api sends for a notification about a task on the student's project. +function taskEvent( + event: string, + notificationType: string, + feedback = false, + extra: Partial<Notification> = {}, +): Notification { + return notification({ + event, + notificationType, + link: `/projects/${PROJECT}/dashboard/${ABBR}${feedback ? '/feedback' : ''}`, + unitId: UNIT, + projectId: PROJECT, + studentId: STUDENT.id, + taskDefinitionId: 21, + taskDefinitionAbbr: ABBR, + taskId: 400, + commentId: null, + groupId: null, + ...extra, + }); +} + +function projectEvent(event: string, notificationType: string, path: string): Notification { + return notification({ + event, + notificationType, + link: `/projects/${PROJECT}/${path}`, + unitId: UNIT, + projectId: PROJECT, + studentId: STUDENT.id, + taskDefinitionId: null, + taskDefinitionAbbr: null, + taskId: null, + commentId: null, + groupId: null, + }); +} + +const studentRoute = (...rest: (string | number)[]): NotificationTarget => ({ + kind: 'route', + audience: 'student', + commands: ['/projects', PROJECT, ...rest], +}); + +const staffInbox: NotificationTarget = { + kind: 'route', + audience: 'staff', + commands: ['/units', UNIT, 'tasks', 'inbox', STUDENT.id, ABBR], + queryParams: {students: 'all'}, +}; + +describe('notificationTarget for a student', () => { + it.each([ + ['task_comment_created', 'feedback', true, studentRoute('dashboard', ABBR, 'feedback')], + ['discussion_request_created', 'feedback', true, studentRoute('dashboard', ABBR, 'feedback')], + ['extension_assessed', 'extension', false, studentRoute('dashboard', ABBR, 'feedback')], + ['task_status_changed', 'task', false, studentRoute('dashboard', ABBR)], + ['new_task_available', 'task', false, studentRoute('dashboard', ABBR)], + ['task_due_date_changed', 'task', false, studentRoute('dashboard', ABBR)], + ['task_due_soon', 'task', false, studentRoute('dashboard', ABBR)], + ])('%s opens the task', (event, type, feedback, expected) => { + expect(notificationTarget(taskEvent(event, type, feedback), STUDENT)).toEqual(expected); + }); + + it('opens a new task that has no task row yet from its definition', () => { + const target = notificationTarget( + taskEvent('new_task_available', 'task', false, {taskId: null}), + STUDENT, + ); + + expect(target).toEqual(studentRoute('dashboard', ABBR)); + }); + + it.each([ + ['group_membership_changed', 'general', 'groups', studentRoute('groups')], + ['portfolio_received', 'portfolio', 'dashboard', studentRoute('portfolio')], + ['tutorial_changed', 'general', 'dashboard', studentRoute('tutorials')], + ])('%s opens the project page for it', (event, type, path, expected) => { + expect(notificationTarget(projectEvent(event, type, path), STUDENT)).toEqual(expected); + }); + + it('sends an unknown project event to the dashboard', () => { + expect( + notificationTarget(projectEvent('future_event', 'general', 'dashboard'), STUDENT), + ).toEqual(studentRoute('dashboard')); + }); +}); + +describe('notificationTarget for staff', () => { + it.each([ + ['task_submitted', 'task', false], + ['task_help_requested', 'task', false], + ['task_comment_created', 'feedback', true], + ['extension_requested', 'task', false], + ['discussion_request_created', 'feedback', true], + ])('%s opens the student task in the inbox with its comments', (event, type, feedback) => { + expect(notificationTarget(taskEvent(event, type, feedback), TUTOR)).toEqual(staffInbox); + }); + + it('opens a submitted portfolio in the staff portfolio view', () => { + expect( + notificationTarget(projectEvent('portfolio_submitted', 'portfolio', 'dashboard'), TUTOR), + ).toEqual({ + kind: 'route', + audience: 'staff', + commands: ['/units', UNIT, 'students', 'portfolios', PROJECT], + }); + }); + + it('sends an unknown staff event with no task to the student list', () => { + expect(notificationTarget(projectEvent('future_event', 'general', 'dashboard'), TUTOR)).toEqual( + {kind: 'route', audience: 'staff', commands: ['/units', UNIT, 'students']}, + ); + }); +}); + +describe('notificationTarget when there is nothing to open', () => { + it('reports a deleted task as unavailable', () => { + const deleted = taskEvent('task_comment_created', 'feedback', true, { + taskDefinitionId: null, + taskDefinitionAbbr: null, + taskId: null, + }); + + expect(notificationTarget(deleted, STUDENT)).toEqual({kind: 'unavailable'}); + expect(notificationTarget(deleted, TUTOR)).toEqual({kind: 'unavailable'}); + }); + + it('reports a deleted project as unavailable', () => { + const deleted = notification({ + event: 'group_membership_changed', + notificationType: 'general', + link: `/projects/${PROJECT}/groups`, + unitId: null, + projectId: null, + studentId: null, + taskDefinitionAbbr: null, + }); + + expect(notificationTarget(deleted, STUDENT)).toEqual({kind: 'unavailable'}); + }); + + it('has nowhere to go for a notification that never had a page', () => { + const general = notification({event: 'general_event', notificationType: 'general', link: null}); + general.projectId = null; + + expect(notificationTarget(general, STUDENT)).toEqual({kind: 'none'}); + }); + + it('follows the link from an older api that sends no ids', () => { + const legacy = notification({ + event: 'task_comment_created', + notificationType: 'feedback', + link: `/projects/${PROJECT}/dashboard/${ABBR}/feedback`, + }); + + expect(notificationTarget(legacy, TUTOR)).toEqual({ + kind: 'link', + link: `/projects/${PROJECT}/dashboard/${ABBR}/feedback`, + }); + expect(notificationTarget(notification({event: 'x', link: null}), TUTOR)).toEqual({ + kind: 'none', + }); + }); +}); + +describe('notificationTarget for Unit Hub updates', () => { + function hubEvent(event: string, extra: Partial<Notification> = {}): Notification { + return notification({ + event, + notificationType: 'unit_hub', + link: '/unit-hub?unit=3&announcement=12', + unitId: UNIT, + projectId: null, + studentId: null, + taskDefinitionAbbr: null, + announcementId: null, + sessionId: null, + ...extra, + }); + } + + it.each(['unit_announcement_published', 'unit_announcement_updated'])( + '%s opens the hub on that announcement for students and staff', + (event) => { + const expected: NotificationTarget = { + kind: 'route', + audience: 'member', + commands: ['/unit-hub'], + queryParams: {unit: UNIT, announcement: 12}, + }; + + expect(notificationTarget(hubEvent(event, {announcementId: 12}), STUDENT)).toEqual(expected); + expect(notificationTarget(hubEvent(event, {announcementId: 12}), TUTOR)).toEqual(expected); + }, + ); + + it.each(['unit_session_changed', 'unit_session_starting_soon'])( + '%s opens the hub on that session', + (event) => { + expect(notificationTarget(hubEvent(event, {sessionId: 40}), STUDENT)).toEqual({ + kind: 'route', + audience: 'member', + commands: ['/unit-hub'], + queryParams: {unit: UNIT, session: 40}, + }); + }, + ); + + it('reports a deleted announcement or session as unavailable', () => { + expect( + notificationTarget(hubEvent('unit_announcement_published', {unitId: null}), STUDENT), + ).toEqual({kind: 'unavailable'}); + expect(notificationTarget(hubEvent('unit_session_changed', {unitId: null}), STUDENT)).toEqual({ + kind: 'unavailable', + }); + }); + + it('opens the hub on the unit for a Unit Hub event it does not know yet', () => { + expect(notificationTarget(hubEvent('unit_hub_future_event'), STUDENT)).toEqual({ + kind: 'route', + audience: 'member', + commands: ['/unit-hub'], + queryParams: {unit: UNIT}, + }); + }); + + it('follows the link from an api that sends no Unit Hub ids', () => { + const legacy = notification({ + event: 'unit_announcement_published', + notificationType: 'unit_hub', + link: '/unit-hub?unit=3&announcement=12', + }); + + expect(notificationTarget(legacy, STUDENT)).toEqual({ + kind: 'link', + link: '/unit-hub?unit=3&announcement=12', + }); + }); +}); diff --git a/src/app/common/notifications/notification-target.ts b/src/app/common/notifications/notification-target.ts new file mode 100644 index 0000000000..2909ea880d --- /dev/null +++ b/src/app/common/notifications/notification-target.ts @@ -0,0 +1,196 @@ +import {Params} from '@angular/router'; +import {Notification} from 'src/app/api/models/notification'; + +/** + * Where opening a notification should take the person reading it. + * + * - `route`: go to `commands` with `queryParams`. `audience` says whose page it + * is, so the caller can check a staff member still teaches that unit first. + * `member` is a page for anyone in the unit, such as the Unit Hub, which + * checks access itself. + * - `link`: the api is too old to send ids, so follow its link as before. + * - `unavailable`: the thing it was about is gone. Say so and stay put. + * - `none`: nothing to open, for example a general announcement. + */ +export type NotificationTarget = + | { + kind: 'route'; + audience: 'student' | 'staff' | 'member'; + commands: (string | number)[]; + queryParams?: Params; + } + | {kind: 'link'; link: string} + | {kind: 'unavailable'} + | {kind: 'none'}; + +export interface NotificationViewer { + id: number; +} + +// Opens on the comments pane on a student's dashboard. +const STUDENT_COMMENT_EVENTS: ReadonlySet<string> = new Set([ + 'task_comment_created', + 'discussion_request_created', + 'extension_assessed', +]); + +const PORTFOLIO_EVENTS: ReadonlySet<string> = new Set([ + 'portfolio_received', + 'portfolio_submitted', +]); + +const UNIT_HUB_ANNOUNCEMENT_EVENTS: ReadonlySet<string> = new Set([ + 'unit_announcement_published', + 'unit_announcement_updated', +]); + +const UNIT_HUB_SESSION_EVENTS: ReadonlySet<string> = new Set([ + 'unit_session_changed', + 'unit_session_starting_soon', +]); + +/** + * One place that decides where every notification goes, for either role. + * + * The role is read from the notification, not from the user's account. A + * notification about the viewer's own project is a student one, and one about + * somebody else's project can only have reached a member of staff. That keeps a + * tutor who is also enrolled in another unit on the right side of both. + */ +export function notificationTarget( + notification: Notification, + viewer: NotificationViewer, +): NotificationTarget { + if (notification.notificationType === 'unit_hub') { + return unitHubTarget(notification); + } + + const hasIds = notification.projectId !== undefined; + + if (!hasIds) { + return notification.link ? {kind: 'link', link: notification.link} : {kind: 'none'}; + } + + if (notification.projectId == null) { + // A link with no project behind it any more is a deleted project. No link + // at all is a notification that was never about a page. + return notification.link ? {kind: 'unavailable'} : {kind: 'none'}; + } + + const event = notification.event; + const projectId = notification.projectId; + const abbreviation = notification.taskDefinitionAbbr; + const linkNamesTask = /\/dashboard\/[^/]+/.test(notification.link ?? ''); + + // The link named a task and the api could not find it, so it was deleted. + if (linkNamesTask && !abbreviation) { + return {kind: 'unavailable'}; + } + + if (notification.studentId === viewer.id) { + return studentTarget(event, notification.notificationType, projectId, abbreviation); + } + + if (notification.unitId == null || notification.studentId == null) { + return {kind: 'unavailable'}; + } + + return staffTarget( + event, + notification.notificationType, + notification.unitId, + projectId, + notification.studentId, + abbreviation, + ); +} + +/** + * A Unit Hub announcement or session opens the hub on its unit, naming the one + * it is about in the query so the hub can open its details. + */ +function unitHubTarget(notification: Notification): NotificationTarget { + const event = notification.event; + const isSession = UNIT_HUB_SESSION_EVENTS.has(event); + const isAnnouncement = UNIT_HUB_ANNOUNCEMENT_EVENTS.has(event); + + // An api that sends no ids at all sends neither of these. + if (notification.announcementId === undefined && notification.sessionId === undefined) { + return notification.link ? {kind: 'link', link: notification.link} : {kind: 'none'}; + } + + const id = isSession ? notification.sessionId : notification.announcementId; + if (notification.unitId == null || ((isSession || isAnnouncement) && id == null)) { + return {kind: 'unavailable'}; + } + + const queryParams: Params = {unit: notification.unitId}; + if (isSession) { + queryParams.session = id; + } else if (isAnnouncement) { + queryParams.announcement = id; + } + + return {kind: 'route', audience: 'member', commands: ['/unit-hub'], queryParams}; +} + +function studentTarget( + event: string, + type: string, + projectId: number, + abbreviation: string | null | undefined, +): NotificationTarget { + const project = ['/projects', projectId]; + const route = (...rest: (string | number)[]): NotificationTarget => ({ + kind: 'route', + audience: 'student', + commands: [...project, ...rest], + }); + + if (event === 'group_membership_changed') { + return route('groups'); + } + if (event === 'tutorial_changed') { + return route('tutorials'); + } + if (PORTFOLIO_EVENTS.has(event) || (type === 'portfolio' && !abbreviation)) { + return route('portfolio'); + } + if (abbreviation) { + return STUDENT_COMMENT_EVENTS.has(event) + ? route('dashboard', abbreviation, 'feedback') + : route('dashboard', abbreviation); + } + return route('dashboard'); +} + +function staffTarget( + event: string, + type: string, + unitId: number, + projectId: number, + studentId: number, + abbreviation: string | null | undefined, +): NotificationTarget { + if (PORTFOLIO_EVENTS.has(event) || (type === 'portfolio' && !abbreviation)) { + return { + kind: 'route', + audience: 'staff', + commands: ['/units', unitId, 'students', 'portfolios', projectId], + }; + } + + if (abbreviation) { + // The inbox route that opens one student's task with its comments beside + // it. 'all' so a task the unit's convenor hears about, but does not tutor, + // is not filtered out of their list before it can be selected. + return { + kind: 'route', + audience: 'staff', + commands: ['/units', unitId, 'tasks', 'inbox', studentId, abbreviation], + queryParams: {students: 'all'}, + }; + } + + return {kind: 'route', audience: 'staff', commands: ['/units', unitId, 'students']}; +} diff --git a/src/app/sessions/service-worker-updater/check-for-update.service.spec.ts b/src/app/sessions/service-worker-updater/check-for-update.service.spec.ts new file mode 100644 index 0000000000..c46e0589c7 --- /dev/null +++ b/src/app/sessions/service-worker-updater/check-for-update.service.spec.ts @@ -0,0 +1,162 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {ApplicationRef} from '@angular/core'; +import {MatSnackBar} from '@angular/material/snack-bar'; +import {SwUpdate, UnrecoverableStateEvent, VersionEvent} from '@angular/service-worker'; +import {Subject} from 'rxjs'; +import {CheckForUpdateService} from './check-for-update.service'; + +const APP_STABILITY_DELAY_MS = 10 * 1000; +const UPDATE_CHECK_INTERVAL_MS = 4 * 60 * 60 * 1000; + +describe('CheckForUpdateService', () => { + let service: CheckForUpdateService; + let appIsStable: Subject<boolean>; + let versionUpdates: Subject<VersionEvent>; + let unrecoverable: Subject<UnrecoverableStateEvent>; + let updateActions: Subject<void>[]; + let reload: ReturnType<typeof vi.fn>; + let updates: { + isEnabled: boolean; + versionUpdates: Subject<VersionEvent>; + unrecoverable: Subject<UnrecoverableStateEvent>; + checkForUpdate: ReturnType<typeof vi.fn>; + activateUpdate: ReturnType<typeof vi.fn>; + }; + let snackBar: {open: ReturnType<typeof vi.fn>}; + + beforeEach(() => { + vi.useFakeTimers(); + appIsStable = new Subject<boolean>(); + versionUpdates = new Subject<VersionEvent>(); + unrecoverable = new Subject<UnrecoverableStateEvent>(); + updateActions = []; + reload = vi.fn(); + updates = { + isEnabled: true, + versionUpdates, + unrecoverable, + checkForUpdate: vi.fn().mockResolvedValue(false), + activateUpdate: vi.fn().mockResolvedValue(true), + }; + snackBar = { + open: vi.fn(() => { + const action: Subject<void> = new Subject(); + updateActions.push(action); + return {onAction: () => action}; + }), + }; + + service = new CheckForUpdateService( + {isStable: appIsStable} as unknown as ApplicationRef, + updates as unknown as SwUpdate, + snackBar as unknown as MatSnackBar, + {location: {reload}} as unknown as Document, + ); + }); + + afterEach(() => { + service.ngOnDestroy(); + vi.useRealTimers(); + }); + + it('checks after the app is stable for ten seconds and then every four hours', async () => { + appIsStable.next(false); + await vi.advanceTimersByTimeAsync(APP_STABILITY_DELAY_MS); + expect(updates.checkForUpdate).not.toHaveBeenCalled(); + + appIsStable.next(true); + await vi.advanceTimersByTimeAsync(APP_STABILITY_DELAY_MS - 1); + expect(updates.checkForUpdate).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + expect(updates.checkForUpdate).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(UPDATE_CHECK_INTERVAL_MS); + expect(updates.checkForUpdate).toHaveBeenCalledTimes(2); + }); + + it('continues polling after an offline update check fails', async () => { + updates.checkForUpdate.mockRejectedValueOnce(new Error('Network unavailable')); + appIsStable.next(true); + await vi.advanceTimersByTimeAsync(APP_STABILITY_DELAY_MS); + expect(updates.checkForUpdate).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(UPDATE_CHECK_INTERVAL_MS); + expect(updates.checkForUpdate).toHaveBeenCalledTimes(2); + expect(reload).not.toHaveBeenCalled(); + }); + + it('does not poll after the service is destroyed', async () => { + appIsStable.next(true); + await vi.advanceTimersByTimeAsync(APP_STABILITY_DELAY_MS); + expect(updates.checkForUpdate).toHaveBeenCalledTimes(1); + + service.ngOnDestroy(); + await vi.advanceTimersByTimeAsync(UPDATE_CHECK_INTERVAL_MS * 2); + + expect(updates.checkForUpdate).toHaveBeenCalledTimes(1); + }); + + it('does not check for updates when the service worker is disabled', async () => { + updates.isEnabled = false; + appIsStable.next(true); + + await vi.advanceTimersByTimeAsync(APP_STABILITY_DELAY_MS + UPDATE_CHECK_INTERVAL_MS); + + expect(updates.checkForUpdate).not.toHaveBeenCalled(); + }); + + it('offers a reload for a ready version and reloads only after activation', async () => { + let finishActivation: (activated: boolean) => void; + updates.activateUpdate.mockReturnValue( + new Promise<boolean>((resolve) => { + finishActivation = resolve; + }), + ); + + versionUpdates.next({ + type: 'VERSION_READY', + currentVersion: {hash: 'old'}, + latestVersion: {hash: 'new'}, + }); + + expect(snackBar.open).toHaveBeenCalledWith( + 'A new version of OnTrack is ready. Reload to update now.', + 'Reload', + ); + + updateActions[0].next(); + expect(updates.activateUpdate).toHaveBeenCalledTimes(1); + expect(reload).not.toHaveBeenCalled(); + + finishActivation(true); + await Promise.resolve(); + + expect(reload).toHaveBeenCalledTimes(1); + }); + + it('ignores version events that are not ready to activate', () => { + versionUpdates.next({type: 'VERSION_DETECTED', version: {hash: 'new'}}); + versionUpdates.next({type: 'NO_NEW_VERSION_DETECTED', version: {hash: 'current'}}); + versionUpdates.next({ + type: 'VERSION_INSTALLATION_FAILED', + version: {hash: 'broken'}, + error: 'Download failed', + }); + + expect(snackBar.open).not.toHaveBeenCalled(); + }); + + it('offers a direct reload when the current version is unrecoverable', () => { + unrecoverable.next({type: 'UNRECOVERABLE_STATE', reason: 'Cache is corrupt'}); + + expect(snackBar.open).toHaveBeenCalledWith( + 'This version of OnTrack can no longer run safely. Reload to recover.', + 'Reload', + ); + + updateActions[0].next(); + + expect(updates.activateUpdate).not.toHaveBeenCalled(); + expect(reload).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/app/sessions/service-worker-updater/check-for-update.service.ts b/src/app/sessions/service-worker-updater/check-for-update.service.ts index 302af93ef1..a8fa251959 100644 --- a/src/app/sessions/service-worker-updater/check-for-update.service.ts +++ b/src/app/sessions/service-worker-updater/check-for-update.service.ts @@ -1,44 +1,73 @@ -import {ApplicationRef, Injectable} from '@angular/core'; +import {DOCUMENT} from '@angular/common'; +import {ApplicationRef, Inject, Injectable, OnDestroy} from '@angular/core'; import {MatSnackBar} from '@angular/material/snack-bar'; import {SwUpdate} from '@angular/service-worker'; +import {Subject, concat, delay, filter, from, interval, of, switchMap, take, takeUntil} from 'rxjs'; + +const APP_STABILITY_DELAY_MS = 10 * 1000; +const UPDATE_CHECK_INTERVAL_MS = 4 * 60 * 60 * 1000; @Injectable() -export class CheckForUpdateService { +export class CheckForUpdateService implements OnDestroy { + private readonly destroy$: Subject<void> = new Subject(); + constructor( appRef: ApplicationRef, private updates: SwUpdate, - private _snackBar: MatSnackBar, + private snackBar: MatSnackBar, + @Inject(DOCUMENT) private document: Document, ) { - // Allow the app to stabilize first, before starting polling for updates with `interval()`. - // const appIsStable$ = appRef.isStable.pipe(delay(10000)); - - // Checks every 4 hours - // const updateInterval$ = interval(4 * 60 * 60 * 1000); - // const updateIntervalOnceAppIsStable$ = concat(appIsStable$, updateInterval$); - - // updateIntervalOnceAppIsStable$.subscribe((() => { - // this.checkForUpdate(); - // }).bind(this)); - this.updates.versionUpdates.subscribe((updateEvent) => { - if (updateEvent.type === 'VERSION_READY') { - const snackBarRef = _snackBar.open( - 'An update to the app has been found, would you like to refresh now?', - 'refresh', - ); - snackBarRef.onAction().subscribe((_result) => { - updates.activateUpdate().then(() => document.location.reload()); - }); + appRef.isStable + .pipe( + filter((isStable) => isStable), + take(1), + delay(APP_STABILITY_DELAY_MS), + switchMap(() => concat(of(0), interval(UPDATE_CHECK_INTERVAL_MS))), + takeUntil(this.destroy$), + ) + .subscribe(() => this.checkForUpdate()); + + this.updates.versionUpdates.pipe(takeUntil(this.destroy$)).subscribe((updateEvent) => { + if (updateEvent.type !== 'VERSION_READY') { + return; } + + const snackBarRef = this.snackBar.open( + 'A new version of OnTrack is ready. Reload to update now.', + 'Reload', + ); + snackBarRef + .onAction() + .pipe( + take(1), + switchMap(() => from(this.updates.activateUpdate())), + takeUntil(this.destroy$), + ) + .subscribe(() => this.document.location.reload()); }); - this.updates.unrecoverable.subscribe((_event) => { - _snackBar.open('An error occurred during update, please refresh the page'); + this.updates.unrecoverable.pipe(takeUntil(this.destroy$)).subscribe(() => { + const snackBarRef = this.snackBar.open( + 'This version of OnTrack can no longer run safely. Reload to recover.', + 'Reload', + ); + snackBarRef + .onAction() + .pipe(take(1), takeUntil(this.destroy$)) + .subscribe(() => this.document.location.reload()); }); } - public checkForUpdate() { + public ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + } + + public checkForUpdate(): void { if (this.updates.isEnabled) { - this.updates.checkForUpdate(); + // Installed apps can stay open while offline. A failed background check + // must not surface as an unhandled rejection or stop future polling. + void this.updates.checkForUpdate().catch(() => undefined); } } } diff --git a/src/assets/icons/android-chrome-maskable-512x512.png b/src/assets/icons/android-chrome-maskable-512x512.png new file mode 100644 index 0000000000..ca17d722a5 Binary files /dev/null and b/src/assets/icons/android-chrome-maskable-512x512.png differ diff --git a/src/manifest.webmanifest b/src/manifest.webmanifest index 0ddf9e42d5..03488ca143 100644 --- a/src/manifest.webmanifest +++ b/src/manifest.webmanifest @@ -1,6 +1,7 @@ { - "name": "formatif learning and feedback", - "short_name": "formatif", + "id": "/index.html", + "name": "OnTrack learning and feedback", + "short_name": "OnTrack", "icons": [ { "src": "/assets/icons/android-chrome-192x192.png", @@ -11,11 +12,40 @@ "src": "/assets/icons/android-chrome-512x512.png", "sizes": "512x512", "type": "image/png" + }, + { + "src": "/assets/icons/android-chrome-maskable-512x512.png", + "sizes": "512x512", + "type": "image/png", + "purpose": "maskable" } ], "theme_color": "#3939ff", - "background_color": "#ffffff", + "background_color": "#3939ff", "display": "standalone", "scope": "/", - "start_url": "/index.html" + "start_url": "/index.html", + "description": "Access your units, tasks, feedback and notifications in the OnTrack app.", + "categories": ["education", "productivity"], + "prefer_related_applications": false, + "shortcuts": [ + { + "name": "Home", + "short_name": "Home", + "description": "Open your OnTrack units and projects", + "url": "/home" + }, + { + "name": "Notifications", + "short_name": "Notifications", + "description": "Read your OnTrack notifications", + "url": "/notifications" + }, + { + "name": "Unit Hub", + "short_name": "Unit Hub", + "description": "View announcements and classes", + "url": "/unit-hub" + } + ] }