feat(settings): add course notification preferences to dashboard settings - #3960
AhtishamShahid wants to merge 2 commits into
Conversation
…ings 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.
OpenAPI Changes1 changes: 0 error, 1 warning, 0 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
🟡 Changes recommended
Rapid checkbox interactions can lose user intent, and cadence selects lack accessible names.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Open edX course notification preferences to MIT Learn’s dashboard settings.
Changes:
- Adds notification delivery and cadence controls with unavailable-state notices.
- Adds MITx Online query and mutation hooks.
- Extends
SimpleSelectdisabled-state typing and adds tests.
File summaries
| File | Description |
|---|---|
frontends/ol-components/src/components/SimpleSelect/SimpleSelect.tsx |
Exposes the existing disabled prop. |
frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx |
Adds notification settings to the dashboard. |
frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx |
Mocks unavailable notification preferences. |
frontends/main/src/app-pages/DashboardPage/NotificationPreferences.tsx |
Implements notification controls and notices. |
frontends/main/src/app-pages/DashboardPage/NotificationPreferences.test.tsx |
Tests rendering, updates, and fallback states. |
frontends/api/src/mitxonline/test-utils/urls.ts |
Adds notification endpoint test URLs. |
frontends/api/src/mitxonline/hooks/notificationPreferences/queries.ts |
Defines preference types and query configuration. |
frontends/api/src/mitxonline/hooks/notificationPreferences/index.ts |
Adds query and update hooks. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <SimpleSelect | ||
| size="small" | ||
| name={`cadence-${notificationApp}-${notificationType}`} | ||
| options={CADENCE_OPTIONS} | ||
| value={config.email_cadence} | ||
| disabled={!config.email || emailLocked} |
There was a problem hiding this comment.
done in 0df9b61 - added aria-label via inputProps, had to expose inputProps on SimpleSelect in ol-components first.
| config={types[type]} | ||
| nonEditable={lockedChannelsFor(group, type)} | ||
| showEmail={showEmail} | ||
| onChange={(update) => updatePreference.mutate(update)} |
There was a problem hiding this comment.
done in 0df9b61 - a row's controls are disabled while that row's own write is in flight.
| const emailLocked = nonEditable.includes("email") | ||
|
|
||
| return ( | ||
| <Row data-testid={`notification-row-${notificationType}`}> |
There was a problem hiding this comment.
Minor point, but data-testid={notification-row-${notificationType}} is keyed only by notificationType, not by app, so if a type name like grouped_notification repeats across multiple notification apps (which can happen for edX's per-app digest type), two rows get the same test id. This wouldn't be an issue in prod, but getByTestId/findByTestId in future tests will throw on "multiple elements found." Might be worth writing it as notification-row-${notificationApp}-${notificationType}, which would also mean updating rowFor in NotificationPreferences.test.tsx:66 to take the app too, plus the 10 places it's called.
There was a problem hiding this comment.
good catch, fixed in 0df9b61 - id is notification-row-${app}-${type} now, rowFor takes the app too.
| 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." |
There was a problem hiding this comment.
Also minor: a 429 on this GET falls through to the generic "could not load" message. You could add a status === 429 branch here with its own message — the code that sends the PUT already does this for its own errors, so you'd just be matching that.
There was a problem hiding this comment.
done in 0df9b61 - 429 has its own message on the read now, same as the PUT already did.
alexfigtree
left a comment
alexfigtree
left a comment
There was a problem hiding this comment.
Quick flag before this goes further — this depends on a backend endpoint (GET/PUT /api/notification-preferences/ on MITx Online) that doesn't exist on mitxonline's main yet. The implementation lived in mitodl/mitxonline#3897, which was closed today "in favour of" this PR, but its backend files (openedx/api.py, urls.py, views.py, etc.) don't seem to have landed anywhere else. As merged, every GET here would 404.
Also, the description's backend verification claims ("verified end-to-end against a local LMS," "199 backend / 29 frontend tests pass") look copied from #3897 — worth clarifying they don't apply to this diff, which is frontend-only.
Is a follow-up mitxonline PR on the way, or should #3897 be reopened instead of closed?
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 <row>", 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.
|
you were right on both. 3897 is reopened and backend only now, the UI moved here, so this PR is blocked on it. description is rewritten too, those backend claims were copied over by mistake. |
What are the relevant tickets?
Relates to mitodl/hq#12930 — the Open edX notifications gear links to a settings anchor that had no notification settings behind it.
Depends on mitodl/mitxonline#3897, which adds the API this one calls and carries the
Fixesfor that issue. Merge it first; without it every request here 404s.Description (What does it do?)
Learners could only manage MITx Online course notifications on MITx Online's own account page. This adds a Notifications section to
/dashboard/settings, so they can do it where they already are.This PR is frontend only. Open edX owns the preference data and MITx Online proxies it, exactly as it does for enrollments and certificates; the section reads and writes through the existing
/mitxonline/*gateway route. Learn stores nothing.Each row toggles on-site and email delivery for one notification type, with an email cadence select that stays visible but disabled while email is off. Channels the LMS reports as non-editable render disabled. When preferences are unavailable — no courseware account yet, the LMS has the feature off, a throttle, or a failed read — the section shows a notice in place of the controls, so the
#notificationsanchor still resolves.SimpleSelectinol-componentsgainsdisabledandinputPropsin its prop type. Both were already forwarded to the underlying MUISelectat runtime; only thePickomitted them.How can this be tested?
/dashboard/settings.Validation run on this branch: 17 unit tests for the section, 836 across the DashboardPage,
apiandSimpleSelectsuites, plustypecheckandlint-check— all passing. Manually exercised against a local stack (Keycloak + APISIX + Tutor Open edX) with the MITx Online branch from #3897 applied. Full-suite CI has not been run yet, and nothing has been tested against a deployed environment, since the API is not deployed anywhere.Additional Context
Review feedback from the first pass is addressed in
0df9b61d: an accessible name on the cadence select, a row's controls disabled while its own write is in flight (a fast double-click previously re-sent the same value),data-testidkeyed by app as well as type so a type appearing under two apps cannot collide, and a distinct message for a throttled read.Not covered here: Learn has no notification state of its own, so the section is empty for anyone without a MITx Online courseware account. It says so rather than hiding itself, matching MITx Online's behaviour for the same learner.
A direct Learn → Open edX version was prototyped and dropped. It works only when the learner already holds an LMS session in that browser — signing in to Learn does not create one — so it would have needed either a redirect out to the LMS or a Learn-side credential store. Worth revisiting if Learn takes on more learner-level Open edX features.