From 3b60b636520a7fd735f10b1f24055832ec20141a Mon Sep 17 00:00:00 2001 From: Ahtisahm Shahid Date: Fri, 18 Sep 2026 08:13:36 +0500 Subject: [PATCH 1/2] feat(settings): add course notification preferences to dashboard settings Learners could manage MITx Online course notifications only on MITx Online's own account settings page. This brings that section to MIT Learn's dashboard settings page, reading and writing the same Open edX state through the MITx Online API that the gateway already proxies. Each row toggles on-site and email delivery for one notification type, with an email cadence select that stays visible but disabled while email delivery is off. Channels the LMS marks non-editable render disabled, and the section renders a notice in place of the controls when preferences are unavailable, keeping the #notifications anchor resolvable either way. --- .../hooks/notificationPreferences/index.ts | 65 ++++ .../hooks/notificationPreferences/queries.ts | 80 ++++ .../api/src/mitxonline/test-utils/urls.ts | 6 + .../NotificationPreferences.test.tsx | 283 ++++++++++++++ .../DashboardPage/NotificationPreferences.tsx | 354 ++++++++++++++++++ .../DashboardPage/SettingsContent.test.tsx | 9 + .../DashboardPage/SettingsContent.tsx | 2 + .../components/SimpleSelect/SimpleSelect.tsx | 2 + 8 files changed, 801 insertions(+) create mode 100644 frontends/api/src/mitxonline/hooks/notificationPreferences/index.ts create mode 100644 frontends/api/src/mitxonline/hooks/notificationPreferences/queries.ts create mode 100644 frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx create mode 100644 frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx diff --git a/frontends/api/src/mitxonline/hooks/notificationPreferences/index.ts b/frontends/api/src/mitxonline/hooks/notificationPreferences/index.ts new file mode 100644 index 0000000000..d4a0adab61 --- /dev/null +++ b/frontends/api/src/mitxonline/hooks/notificationPreferences/index.ts @@ -0,0 +1,65 @@ +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query" + +import type { MutationHookOptions } from "../../../mutations/mutationMeta" +import mitxAxios from "../../axios" +import { + NOTIFICATION_PREFERENCES_URL, + notificationPreferencesKeys, + notificationPreferencesQueries, +} from "./queries" +import type { + NotificationPreferences, + NotificationPreferenceUpdate, + PreferenceConfig, + PreferenceGroup, +} from "./queries" + +type UseNotificationPreferencesOptions = { + enabled?: boolean +} + +const useNotificationPreferences = ({ + enabled = true, +}: UseNotificationPreferencesOptions = {}) => { + return useQuery({ + ...notificationPreferencesQueries.detail(), + enabled, + }) +} + +/** + * Open edX updates one channel per request, so each toggle or cadence change is + * its own mutation. + * + * The response body is not enough to render from — the LMS fans a change to a + * grouped type out to several types — so we re-read on success rather than + * patching the cache. + */ +const useUpdateNotificationPreference = ({ + meta, +}: MutationHookOptions = {}) => { + const queryClient = useQueryClient() + return useMutation({ + mutationFn: (update: NotificationPreferenceUpdate) => + mitxAxios.put(NOTIFICATION_PREFERENCES_URL, update), + onSuccess: () => { + queryClient.invalidateQueries({ + queryKey: notificationPreferencesKeys.detail(), + }) + }, + meta, + }) +} + +export { + useNotificationPreferences, + useUpdateNotificationPreference, + notificationPreferencesQueries, + notificationPreferencesKeys, +} +export type { + NotificationPreferences, + NotificationPreferenceUpdate, + PreferenceConfig, + PreferenceGroup, +} diff --git a/frontends/api/src/mitxonline/hooks/notificationPreferences/queries.ts b/frontends/api/src/mitxonline/hooks/notificationPreferences/queries.ts new file mode 100644 index 0000000000..a2bd833dab --- /dev/null +++ b/frontends/api/src/mitxonline/hooks/notificationPreferences/queries.ts @@ -0,0 +1,80 @@ +import { queryOptions } from "@tanstack/react-query" + +import mitxAxios from "../../axios" + +/** + * MITx Online proxies these straight through from Open edX, which owns the + * state. The endpoint is excluded from MITx Online's OpenAPI schema, so there + * is no generated client for it and we call it through the configured axios + * instance instead. + */ +const NOTIFICATION_PREFERENCES_URL = "/api/notification-preferences/" + +type NotificationChannel = "web" | "email" | "email_cadence" + +type PreferenceConfig = { + web: boolean + push: boolean + email: boolean + email_cadence: string + info: string +} + +type PreferenceGroup = { + enabled: boolean + /** + * Keyed by notification type -> the channels that type locks, e.g. + * `{ new_discussion_post: ["push"] }`. Not a flat list. + */ + non_editable: Record | string[] | null + notification_types: Record +} + +type NotificationPreferences = { + data?: Record | null + /** The LMS gates the whole feature with this. */ + show_preferences?: boolean + show_email_preferences?: boolean +} + +type NotificationPreferenceUpdate = { + notification_app: string + notification_type: string + notification_channel: NotificationChannel +} & ({ value: boolean } | { email_cadence: string }) + +const notificationPreferencesKeys = { + root: ["mitxonline", "notificationPreferences"], + detail: () => [...notificationPreferencesKeys.root, "detail"], +} + +const notificationPreferencesQueries = { + detail: () => + queryOptions({ + queryKey: notificationPreferencesKeys.detail(), + queryFn: async (): Promise => { + return mitxAxios + .get(NOTIFICATION_PREFERENCES_URL) + .then((res) => res.data) + }, + /** + * A 409 means the learner has no Open edX account yet, and a 4xx will not + * start working by asking again. Retrying only delays the notice the + * section shows in its place. + */ + retry: false, + }), +} + +export { + notificationPreferencesQueries, + notificationPreferencesKeys, + NOTIFICATION_PREFERENCES_URL, +} +export type { + NotificationChannel, + NotificationPreferences, + NotificationPreferenceUpdate, + PreferenceConfig, + PreferenceGroup, +} diff --git a/frontends/api/src/mitxonline/test-utils/urls.ts b/frontends/api/src/mitxonline/test-utils/urls.ts index 953d1b3073..c533dfb8a8 100644 --- a/frontends/api/src/mitxonline/test-utils/urls.ts +++ b/frontends/api/src/mitxonline/test-utils/urls.ts @@ -166,6 +166,11 @@ const verifiedProgramEnrollments = { `${getApiBaseUrl()}/api/v2/verified_program_enrollments/${encodeURIComponent(courserunId)}/`, } +const notificationPreferences = { + get: () => `${getApiBaseUrl()}/api/notification-preferences/`, + put: () => `${getApiBaseUrl()}/api/notification-preferences/`, +} + export { b2b, b2bAttach, @@ -184,4 +189,5 @@ export { baskets, orders, verifiedProgramEnrollments, + notificationPreferences, } diff --git a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx new file mode 100644 index 0000000000..495322a628 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx @@ -0,0 +1,283 @@ +import React from "react" +import NotificationPreferences from "./NotificationPreferences" +import { renderWithProviders, screen, within, user } from "@/test-utils" +import { setMockResponse, makeRequest } from "api/test-utils" +import { urls as mitxonlineUrls } from "api/mitxonline-test-utils" + +/** + * Mirrors a real GET /api/notification-preferences/ response: `non_editable` is + * an object keyed by notification type, not a flat list of channels. + */ +const makePreferences = (overrides = {}) => ({ + show_preferences: true, + show_email_preferences: true, + data: { + discussion: { + enabled: true, + non_editable: { new_discussion_post: ["web"] }, + notification_types: { + new_discussion_post: { + web: true, + push: false, + email: false, + email_cadence: "Daily", + info: "", + }, + grouped_notification: { + web: true, + push: false, + email: true, + email_cadence: "Weekly", + info: "Covers several activity types", + }, + }, + }, + grading: { + enabled: false, + non_editable: {}, + notification_types: { + ora_grade_assigned: { + web: true, + push: false, + email: false, + email_cadence: "Daily", + info: "", + }, + }, + }, + }, + ...overrides, +}) + +const setupApi = ( + responseBody: unknown = makePreferences(), + { code = 200 }: { code?: number } = {}, +) => { + setMockResponse.get( + mitxonlineUrls.notificationPreferences.get(), + responseBody, + { + code, + }, + ) + setMockResponse.put(mitxonlineUrls.notificationPreferences.put(), {}) +} + +const rowFor = async (notificationType: string) => + await screen.findByTestId(`notification-row-${notificationType}`) + +describe("NotificationPreferences", () => { + test("renders a group heading using the display label, not the API key", async () => { + setupApi() + renderWithProviders() + + expect( + await screen.findByRole("heading", { name: "Discussions" }), + ).toBeInTheDocument() + }) + + test("renders one row per notification type, with our description copy", async () => { + setupApi() + renderWithProviders() + + const row = await rowFor("new_discussion_post") + expect(within(row).getByText("New discussion posts")).toBeInTheDocument() + expect( + within(row).getByText( + "When someone starts a new discussion in your courses.", + ), + ).toBeInTheDocument() + }) + + test("falls back to the API's info for a type we do not know about", async () => { + setupApi( + makePreferences({ + data: { + discussion: { + enabled: true, + non_editable: {}, + notification_types: { + brand_new_type: { + web: true, + push: false, + email: false, + email_cadence: "Daily", + info: "Straight from the LMS", + }, + }, + }, + }, + }), + ) + renderWithProviders() + + const row = await rowFor("brand_new_type") + expect(within(row).getByText("Straight from the LMS")).toBeInTheDocument() + }) + + test("skips groups the API reports as disabled", async () => { + setupApi() + renderWithProviders() + + await screen.findByRole("heading", { name: "Discussions" }) + expect( + screen.queryByRole("heading", { name: "Grading" }), + ).not.toBeInTheDocument() + }) + + test("disables a channel the API marks non-editable for that type", async () => { + setupApi() + renderWithProviders() + + const locked = await rowFor("new_discussion_post") + expect(within(locked).getByLabelText("On site")).toBeDisabled() + + // non_editable is keyed by type, so the other row stays editable. + const unlocked = await rowFor("grouped_notification") + expect(within(unlocked).getByLabelText("On site")).toBeEnabled() + }) + + test("toggling a channel PUTs that single channel", async () => { + setupApi() + renderWithProviders() + + const row = await rowFor("grouped_notification") + await user.click(within(row).getByLabelText("On site")) + + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "put", + url: mitxonlineUrls.notificationPreferences.put(), + body: { + notification_app: "discussion", + notification_type: "grouped_notification", + notification_channel: "web", + value: false, + }, + }), + ) + }) + + test("keeps the cadence control in place, disabled, while email is off", async () => { + setupApi() + renderWithProviders() + + const emailOn = await rowFor("grouped_notification") + expect(within(emailOn).getByRole("combobox")).toBeInTheDocument() + expect(within(emailOn).getByRole("combobox")).not.toHaveAttribute( + "aria-disabled", + "true", + ) + + // Still rendered, showing the stored cadence, but not operable. + const emailOff = await rowFor("new_discussion_post") + const disabled = within(emailOff).getByRole("combobox") + expect(disabled).toBeInTheDocument() + expect(disabled).toHaveTextContent("Daily") + expect(disabled).toHaveAttribute("aria-disabled", "true") + }) + + test("disables the cadence control when email is locked by the API", async () => { + setupApi( + makePreferences({ + data: { + discussion: { + enabled: true, + non_editable: { locked_type: ["email"] }, + notification_types: { + locked_type: { + web: true, + push: false, + email: true, + email_cadence: "Weekly", + info: "", + }, + }, + }, + }, + }), + ) + renderWithProviders() + + const row = await rowFor("locked_type") + expect(within(row).getByLabelText("Email")).toBeDisabled() + expect(within(row).getByRole("combobox")).toHaveAttribute( + "aria-disabled", + "true", + ) + }) + + test("changing the cadence PUTs email_cadence rather than a value", async () => { + setupApi() + renderWithProviders() + + const row = await rowFor("grouped_notification") + await user.click(within(row).getByRole("combobox")) + await user.click(await screen.findByRole("option", { name: "Immediately" })) + + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "put", + url: mitxonlineUrls.notificationPreferences.put(), + body: { + notification_app: "discussion", + notification_type: "grouped_notification", + notification_channel: "email_cadence", + email_cadence: "Immediately", + }, + }), + ) + }) + + test("hides email controls when the API turns email preferences off", async () => { + setupApi(makePreferences({ show_email_preferences: false })) + renderWithProviders() + + const row = await rowFor("grouped_notification") + expect(within(row).getByLabelText("On site")).toBeInTheDocument() + expect(within(row).queryByLabelText("Email")).not.toBeInTheDocument() + expect(within(row).queryByRole("combobox")).not.toBeInTheDocument() + }) + + test.each([ + { + description: "the LMS has the feature switched off", + response: makePreferences({ show_preferences: false }), + code: 200, + notice: "Notifications are not enabled for your courses.", + }, + { + description: "the learner has no course account yet", + response: { detail: "no edx auth" }, + code: 409, + notice: + "Your course account is still being set up. Please check back shortly.", + }, + { + description: "the read fails", + response: { detail: "boom" }, + code: 503, + notice: + "We could not load your notification settings. Please try again later.", + }, + { + description: "there is nothing to manage", + response: makePreferences({ data: {} }), + code: 200, + notice: "You have no notification settings to manage yet.", + }, + ])( + "renders the section with a notice when $description", + async ({ response, code, notice }) => { + setupApi(response, { code }) + renderWithProviders() + + expect(await screen.findByText(notice)).toBeInTheDocument() + // The anchor must resolve even when there are no controls to show. + expect( + await screen.findByRole("heading", { name: "Notifications" }), + ).toBeInTheDocument() + expect(screen.queryByLabelText("On site")).not.toBeInTheDocument() + }, + ) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx new file mode 100644 index 0000000000..9ab43df6b3 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx @@ -0,0 +1,354 @@ +"use client" + +import React from "react" +import { SimpleSelect, Typography, styled } from "ol-components" +import { Checkbox } from "@mitodl/smoot-design" +import type { AxiosError } from "axios" +import { + useNotificationPreferences, + useUpdateNotificationPreference, +} from "api/mitxonline-hooks/notificationPreferences" +import type { + NotificationPreferenceUpdate, + PreferenceConfig, + PreferenceGroup, +} from "api/mitxonline-hooks/notificationPreferences" + +/** + * Course notification settings, ported from MITx Online's account settings + * page. MITx Online proxies Open edX, which owns this state, so the section + * renders whatever the LMS reports rather than anything stored in Learn. + */ + +const APP_LABELS: Record = { + discussion: "Discussions", + grading: "Grading", + updates: "Updates", +} + +const TYPE_LABELS: Record = { + grouped_notification: "Activity notifications", + new_discussion_post: "New discussion posts", + new_question_post: "New question posts", + new_instructor_all_learners_post: "New posts from instructors", + new_comment_on_response: "Comments on your responses", + new_comment: "Comments on your posts", + new_response: "Responses to your posts", + response_on_followed_post: "Responses on posts you follow", + comment_on_followed_post: "Comments on posts you follow", + response_endorsed_on_thread: "Endorsements on your posts", + response_endorsed: "Endorsements of your responses", + content_reported: "Reported content", + course_updates: "Course updates", + ora_grade_assigned: "Essay assignment grade received", + ora_reminder: "Essay assignment reminders", + ora_staff_notifications: "Essay assignments awaiting grading", +} + +/** + * The API only returns an `info` string for a couple of types, so descriptions + * live here to keep every row consistent. Rows still fall back to the API's + * `info` for any type added upstream that we do not know about yet. + */ +const TYPE_DESCRIPTIONS: Record = { + grouped_notification: + "Responses, comments and endorsements on your posts and on posts you follow.", + new_discussion_post: "When someone starts a new discussion in your courses.", + new_question_post: "When someone posts a new question in your courses.", + new_instructor_all_learners_post: + "When the course team posts an update to everyone.", + new_comment_on_response: "When someone comments on one of your responses.", + new_comment: "When someone comments on one of your posts.", + new_response: "When someone responds to one of your posts.", + response_on_followed_post: "When someone responds to a post you follow.", + comment_on_followed_post: "When someone comments on a post you follow.", + response_endorsed_on_thread: + "When the course team endorses a response on your post.", + response_endorsed: "When the course team endorses one of your responses.", + content_reported: "When a learner reports a post for review.", + course_updates: "Announcements and updates from the course team.", + ora_grade_assigned: + "When a peer or the course team grades your essay assignment.", + ora_reminder: "When you still have peer or self reviews to complete.", + ora_staff_notifications: + "When an essay assignment is waiting for your review.", +} + +const EMAIL_CADENCES = ["Daily", "Weekly", "Immediately"] + +const CADENCE_OPTIONS = EMAIL_CADENCES.map((cadence) => ({ + value: cadence, + label: cadence, +})) + +const SectionTitle = styled(Typography)(({ theme }) => ({ + marginTop: "16px", + marginBottom: "8px", + color: theme.custom.colors.darkGray2, + ...theme.typography.h5, +})) as typeof Typography + +const Intro = styled(Typography)(({ theme }) => ({ + marginBottom: "16px", + color: theme.custom.colors.darkGray2, + ...theme.typography.body2, +})) + +const GroupTitle = styled(Typography)(({ theme }) => ({ + marginTop: "16px", + marginBottom: "8px", + color: theme.custom.colors.darkGray2, + ...theme.typography.subtitle1, +})) as typeof Typography + +const Row = styled.div(({ theme }) => ({ + display: "flex", + gap: "16px", + alignItems: "center", + padding: "12px 0", + borderBottom: `1px solid ${theme.custom.colors.lightGray2}`, + ":last-of-type": { + borderBottom: "none", + }, + [theme.breakpoints.down("sm")]: { + alignItems: "flex-start", + flexDirection: "column", + gap: "8px", + }, +})) + +const RowText = styled.div({ + display: "flex", + flexDirection: "column", + gap: "4px", + flex: "1 0 0", +}) + +const RowLabel = styled.span(({ theme }) => ({ + ...theme.typography.subtitle2, + color: theme.custom.colors.darkGray2, +})) + +const RowDescription = styled.span(({ theme }) => ({ + ...theme.typography.body3, + color: theme.custom.colors.silverGrayDark, +})) + +const Controls = styled.div({ + display: "flex", + alignItems: "center", + gap: "16px", +}) + +const labelForApp = (app: string) => APP_LABELS[app] || app + +const labelForType = (type: string) => TYPE_LABELS[type] || type + +/** Our own copy first, then whatever the API happened to send. */ +const descriptionForType = (type: string, info: string) => + TYPE_DESCRIPTIONS[type] || info || "" + +/** + * The API returns `non_editable` keyed by notification type. Older releases + * returned a flat list for the whole app, so tolerate both. + */ +const lockedChannelsFor = ( + group: PreferenceGroup, + notificationType: string, +): string[] => { + const nonEditable = group.non_editable + if (!nonEditable) return [] + if (Array.isArray(nonEditable)) return nonEditable + return nonEditable[notificationType] || [] +} + +type PreferenceRowProps = { + notificationApp: string + notificationType: string + config: PreferenceConfig + nonEditable: string[] + showEmail: boolean + onChange: (update: NotificationPreferenceUpdate) => void +} + +const PreferenceRow: React.FC = ({ + notificationApp, + notificationType, + config, + nonEditable, + showEmail, + onChange, +}) => { + const label = labelForType(notificationType) + const description = descriptionForType(notificationType, config.info) + const webLocked = nonEditable.includes("web") + const emailLocked = nonEditable.includes("email") + + return ( + + + {label} + {description ? {description} : null} + + + + onChange({ + notification_app: notificationApp, + notification_type: notificationType, + notification_channel: "web", + value: !config.web, + }) + } + /> + {showEmail ? ( + <> + + onChange({ + notification_app: notificationApp, + notification_type: notificationType, + notification_channel: "email", + value: !config.email, + }) + } + /> + {/* + The cadence only means something while email delivery is on, but + it stays in place disabled rather than appearing and disappearing: + a control that pops into the row on click shifts the other rows, + and the stored cadence is worth seeing even when email is off. + */} + `${value}`} + onChange={(event) => + onChange({ + notification_app: notificationApp, + notification_type: notificationType, + notification_channel: "email_cadence", + email_cadence: `${event.target.value}`, + }) + } + /> + + ) : null} + + + ) +} + +const Section: React.FC<{ children: React.ReactNode }> = ({ children }) => ( + /* + * The section always renders — including when there is nothing to show — so + * that /dashboard/settings#notifications, the target Open edX's notifications + * gear links to, still resolves. + */ +
+ Notifications + {children} +
+) + +/** Why the controls cannot be shown, or null to show them. */ +const noticeFor = ({ + isPending, + error, + showPreferences, +}: { + isPending: boolean + error: unknown + showPreferences?: boolean +}): string | null => { + if (isPending) return "Loading your notification settings..." + if (error) { + const status = (error as AxiosError)?.response?.status + return status === 409 + ? "Your course account is still being set up. Please check back shortly." + : "We could not load your notification settings. Please try again later." + } + // The LMS gates the whole feature with show_preferences. + if (showPreferences === false) { + return "Notifications are not enabled for your courses." + } + return null +} + +const NotificationPreferences: React.FC = () => { + const preferences = useNotificationPreferences() + const updatePreference = useUpdateNotificationPreference({ + meta: { + getErrorMessage: (error) => + (error as AxiosError)?.response?.status === 429 + ? "Too many changes at once. Please wait a moment and try again." + : "We could not save that notification setting. Please try again.", + }, + }) + + const notice = noticeFor({ + isPending: preferences.isPending, + error: preferences.error, + showPreferences: preferences.data?.show_preferences, + }) + + if (notice) { + return ( +
+ {notice} +
+ ) + } + + const byApp = preferences.data?.data ?? {} + const apps = Object.keys(byApp).filter((app) => byApp[app].enabled) + const showEmail = preferences.data?.show_email_preferences !== false + + if (apps.length === 0) { + return ( +
+ You have no notification settings to manage yet. +
+ ) + } + + return ( +
+ Choose how you hear about activity in your courses. + {apps.map((app) => { + const group = byApp[app] + const types = group.notification_types || {} + return ( +
+ {labelForApp(app)} + {Object.keys(types).map((type) => ( + updatePreference.mutate(update)} + /> + ))} +
+ ) + })} +
+ ) +} + +export default NotificationPreferences +export { descriptionForType, lockedChannelsFor, PreferenceRow } diff --git a/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx index 7ef414543f..ef49b20389 100644 --- a/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx @@ -2,6 +2,7 @@ import React from "react" import { SettingsContent } from "./SettingsContent" import { renderWithProviders, screen, within, user } from "@/test-utils" import { urls, setMockResponse, factories, makeRequest } from "api/test-utils" +import { urls as mitxonlineUrls } from "api/mitxonline-test-utils" import type { LearningResourcesUserSubscriptionApiLearningResourcesUserSubscriptionCheckListRequest as CheckSubscriptionRequest } from "api" import { useFeatureFlagEnabled } from "posthog-js/react" import { FeatureFlags } from "@/common/feature_flags" @@ -44,6 +45,14 @@ const setupApis = ({ setMockResponse.get(urls.profileMe.get(), { email_optin: emailOptin }) setMockResponse.patch(urls.profileMe.patch(), {}) + // Notification preferences live in MITx Online; the section renders a notice + // when they are unavailable, which is the default here. + setMockResponse.get( + mitxonlineUrls.notificationPreferences.get(), + { detail: "No Open edX account" }, + { code: 409 }, + ) + const subscribeResponse = isSubscribed ? factories.percolateQueries.percolateQueryList({ count: 5 }).results : factories.percolateQueries.percolateQueryList({ count: 0 }).results diff --git a/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx b/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx index 7741d6093b..8f0a604862 100644 --- a/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx +++ b/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx @@ -22,6 +22,7 @@ import * as NiceModal from "@ebay/nice-modal-react" import { FeatureFlags } from "@/common/feature_flags" import { AccountAction, SETTINGS, accountAction } from "@/common/urls" import AccountActionAlert from "./AccountActionAlert" +import NotificationPreferences from "./NotificationPreferences" import { TitleText } from "./HomeContent" const SOURCE_LABEL_DISPLAY = { topic: "Topic", @@ -275,6 +276,7 @@ const SettingsContent: React.FC = () => { checked={profile?.email_optin ?? true} onChange={(e) => updateProfile({ email_optin: e.target.checked })} /> + {user.is_authenticated ? : null} Following diff --git a/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx b/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx index d21066e3a2..f87415aa59 100644 --- a/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx +++ b/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx @@ -12,6 +12,7 @@ type SimpleSelectProps = Pick< | "renderValue" | "className" | "name" + | "disabled" > & { /** * The options for the dropdown @@ -60,6 +61,7 @@ type SimpleSelectFieldProps = Pick< | "name" | "className" | "renderValue" + | "disabled" > & { /** * The options for the dropdown From 0df9b61d395a78423ac3af51430e8ed7100b9a61 Mon Sep 17 00:00:00 2001 From: Ahtisahm Shahid Date: Mon, 21 Sep 2026 17:57:10 +0500 Subject: [PATCH 2/2] fix(settings): address review on notification preferences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from review: - The cadence select announced no accessible name: `name` does not label MUI's combobox, and the row group's label does not reach each control. It now carries "Email frequency for ", which needed `inputProps` on ol-components' SimpleSelect (already forwarded at runtime, only the prop type omitted it). - Clicking a checkbox twice quickly sent the same inverted value twice, because both handlers read the last fetched state. A row's controls are now disabled while that row's write is in flight — keyed on the mutation's variables, so the rest of the section stays usable. - The row test id was keyed only by notification type, so a type that appears under two apps produced duplicate ids. It now includes the app. - A throttled read fell through to the generic failure message; 429 gets its own, matching what the write path already does. --- .../NotificationPreferences.test.tsx | 86 ++++++++++++++++++- .../DashboardPage/NotificationPreferences.tsx | 40 +++++++-- .../components/SimpleSelect/SimpleSelect.tsx | 1 + 3 files changed, 116 insertions(+), 11 deletions(-) diff --git a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx index 495322a628..3c644a6232 100644 --- a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx @@ -1,6 +1,12 @@ import React from "react" import NotificationPreferences from "./NotificationPreferences" -import { renderWithProviders, screen, within, user } from "@/test-utils" +import { + renderWithProviders, + screen, + within, + user, + waitFor, +} from "@/test-utils" import { setMockResponse, makeRequest } from "api/test-utils" import { urls as mitxonlineUrls } from "api/mitxonline-test-utils" @@ -63,8 +69,8 @@ const setupApi = ( setMockResponse.put(mitxonlineUrls.notificationPreferences.put(), {}) } -const rowFor = async (notificationType: string) => - await screen.findByTestId(`notification-row-${notificationType}`) +const rowFor = async (notificationType: string, app = "discussion") => + await screen.findByTestId(`notification-row-${app}-${notificationType}`) describe("NotificationPreferences", () => { test("renders a group heading using the display label, not the API key", async () => { @@ -163,7 +169,11 @@ describe("NotificationPreferences", () => { renderWithProviders() const emailOn = await rowFor("grouped_notification") - expect(within(emailOn).getByRole("combobox")).toBeInTheDocument() + expect( + within(emailOn).getByRole("combobox", { + name: "Email frequency for Activity notifications", + }), + ).toBeInTheDocument() expect(within(emailOn).getByRole("combobox")).not.toHaveAttribute( "aria-disabled", "true", @@ -239,6 +249,68 @@ describe("NotificationPreferences", () => { expect(within(row).queryByRole("combobox")).not.toBeInTheDocument() }) + test("a type repeated across apps gets its own row, not a duplicate id", async () => { + setupApi( + makePreferences({ + data: { + discussion: { + enabled: true, + non_editable: {}, + notification_types: { + grouped_notification: { + web: true, + push: false, + email: false, + email_cadence: "Daily", + info: "", + }, + }, + }, + updates: { + enabled: true, + non_editable: {}, + notification_types: { + grouped_notification: { + web: false, + push: false, + email: false, + email_cadence: "Weekly", + info: "", + }, + }, + }, + }, + }), + ) + renderWithProviders() + + const discussion = await rowFor("grouped_notification", "discussion") + const updates = await rowFor("grouped_notification", "updates") + expect(within(discussion).getByLabelText("On site")).toBeChecked() + expect(within(updates).getByLabelText("On site")).not.toBeChecked() + }) + + test("a row's controls are inert while its own write is in flight", async () => { + setupApi() + // Never resolves: the mutation stays pending for the assertion. + setMockResponse.put( + mitxonlineUrls.notificationPreferences.put(), + new Promise(() => {}), + ) + renderWithProviders() + + const row = await rowFor("grouped_notification") + await user.click(within(row).getByLabelText("On site")) + + await waitFor(() => + expect(within(row).getByLabelText("On site")).toBeDisabled(), + ) + expect(within(row).getByLabelText("Email")).toBeDisabled() + // A different row is unaffected. + const other = await rowFor("new_discussion_post") + expect(within(other).getByLabelText("Email")).toBeEnabled() + }) + test.each([ { description: "the LMS has the feature switched off", @@ -253,6 +325,12 @@ describe("NotificationPreferences", () => { notice: "Your course account is still being set up. Please check back shortly.", }, + { + description: "the read is throttled", + response: { detail: "slow down" }, + code: 429, + notice: "Too many requests at once. Please wait a moment and reload.", + }, { description: "the read fails", response: { detail: "boom" }, diff --git a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx index 9ab43df6b3..2f1ae00158 100644 --- a/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx +++ b/frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx @@ -168,6 +168,12 @@ type PreferenceRowProps = { config: PreferenceConfig nonEditable: string[] showEmail: boolean + /** + * A write for THIS row is in flight. The controls still show the last known + * server state, so a second click would recompute the same inverted value + * and cancel nothing — disable them until the refetch lands. + */ + pending: boolean onChange: (update: NotificationPreferenceUpdate) => void } @@ -177,6 +183,7 @@ const PreferenceRow: React.FC = ({ config, nonEditable, showEmail, + pending, onChange, }) => { const label = labelForType(notificationType) @@ -185,7 +192,9 @@ const PreferenceRow: React.FC = ({ const emailLocked = nonEditable.includes("email") return ( - + {label} {description ? {description} : null} @@ -195,7 +204,7 @@ const PreferenceRow: React.FC = ({ name={`web-${notificationApp}-${notificationType}`} label="On site" checked={config.web} - disabled={webLocked} + disabled={webLocked || pending} onChange={() => onChange({ notification_app: notificationApp, @@ -211,7 +220,7 @@ const PreferenceRow: React.FC = ({ name={`email-${notificationApp}-${notificationType}`} label="Email" checked={config.email} - disabled={emailLocked} + disabled={emailLocked || pending} onChange={() => onChange({ notification_app: notificationApp, @@ -232,7 +241,8 @@ const PreferenceRow: React.FC = ({ name={`cadence-${notificationApp}-${notificationType}`} options={CADENCE_OPTIONS} value={config.email_cadence} - disabled={!config.email || emailLocked} + disabled={!config.email || emailLocked || pending} + inputProps={{ "aria-label": `Email frequency for ${label}` }} renderValue={(value) => `${value}`} onChange={(event) => onChange({ @@ -275,9 +285,13 @@ const noticeFor = ({ if (isPending) return "Loading your notification settings..." if (error) { const status = (error as AxiosError)?.response?.status - return status === 409 - ? "Your course account is still being set up. Please check back shortly." - : "We could not load your notification settings. Please try again later." + if (status === 409) { + return "Your course account is still being set up. Please check back shortly." + } + if (status === 429) { + return "Too many requests at once. Please wait a moment and reload." + } + return "We could not load your notification settings. Please try again later." } // The LMS gates the whole feature with show_preferences. if (showPreferences === false) { @@ -297,6 +311,14 @@ const NotificationPreferences: React.FC = () => { }, }) + /** + * One mutation serves every row, so `isPending` alone would freeze the whole + * section. `variables` names the row actually being written. + */ + const inFlight = updatePreference.isPending + ? updatePreference.variables + : undefined + const notice = noticeFor({ isPending: preferences.isPending, error: preferences.error, @@ -340,6 +362,10 @@ const NotificationPreferences: React.FC = () => { config={types[type]} nonEditable={lockedChannelsFor(group, type)} showEmail={showEmail} + pending={ + inFlight?.notification_app === app && + inFlight?.notification_type === type + } onChange={(update) => updatePreference.mutate(update)} /> ))} diff --git a/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx b/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx index f87415aa59..dad8ea0d14 100644 --- a/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx +++ b/frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx @@ -13,6 +13,7 @@ type SimpleSelectProps = Pick< | "className" | "name" | "disabled" + | "inputProps" > & { /** * The options for the dropdown