diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 221f7c75c5..dfebf6283e 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -20,7 +20,6 @@ class JSConfig: class Mode(str, Enum): OAUTH2_REDIRECT_ERROR = "oauth2-redirect-error" BASIC_LTI_LAUNCH = "basic-lti-launch" - EMAIL_NOTIFICATIONS = "email-notifications" FILE_PICKER = "content-item-selection" ERROR_DIALOG = "error-dialog" diff --git a/lms/static/scripts/frontend_apps/components/AppRoot.tsx b/lms/static/scripts/frontend_apps/components/AppRoot.tsx index a8fc7c5fa0..c4290c40e3 100644 --- a/lms/static/scripts/frontend_apps/components/AppRoot.tsx +++ b/lms/static/scripts/frontend_apps/components/AppRoot.tsx @@ -40,7 +40,7 @@ export default function AppRoot({ initialConfig, services }: AppRootProps) { - + diff --git a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx index 4f1cacfb84..963ef91201 100644 --- a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx +++ b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx @@ -55,7 +55,7 @@ export default function BasicLTILaunchApp() { // Content URL to show in the iframe. viaUrl: viaURL, canvas, - } = useConfig(['hypothesisClient']); + } = useConfig(['api', 'hypothesisClient']); const clientRPC = useService(ClientRPC); diff --git a/lms/static/scripts/frontend_apps/components/ContentSelector.tsx b/lms/static/scripts/frontend_apps/components/ContentSelector.tsx index 71fab89c68..55ca683f2f 100644 --- a/lms/static/scripts/frontend_apps/components/ContentSelector.tsx +++ b/lms/static/scripts/frontend_apps/components/ContentSelector.tsx @@ -91,7 +91,7 @@ export default function ContentSelector({ }, youtube: { enabled: youtubeEnabled }, }, - } = useConfig(['filePicker']); + } = useConfig(['api', 'filePicker']); // Map the existing content selection to a dialog type and value. We don't // open the corresponding dialog immediately, but do pre-fill the dialog diff --git a/lms/static/scripts/frontend_apps/components/EmailNotificationsApp.tsx b/lms/static/scripts/frontend_apps/components/EmailNotificationsApp.tsx index 6d7e4356fc..5fcd8114d9 100644 --- a/lms/static/scripts/frontend_apps/components/EmailNotificationsApp.tsx +++ b/lms/static/scripts/frontend_apps/components/EmailNotificationsApp.tsx @@ -1,4 +1,4 @@ -import { useState } from 'preact/hooks'; +import { useCallback, useState } from 'preact/hooks'; import { useConfig } from '../config'; import EmailNotificationsPreferences from './EmailNotificationsPreferences'; @@ -6,6 +6,8 @@ import EmailNotificationsPreferences from './EmailNotificationsPreferences'; export default function EmailNotificationsApp() { const { emailNotifications } = useConfig(['emailNotifications']); const [selectedDays, setSelectedDays] = useState(emailNotifications); + const [saving, setSaving] = useState(false); + const onSave = useCallback(() => setSaving(true), []); return (
@@ -15,6 +17,8 @@ export default function EmailNotificationsApp() { updateSelectedDays={newSelectedDays => setSelectedDays(prev => ({ ...prev, ...newSelectedDays })) } + onSave={onSave} + saving={saving} />
diff --git a/lms/static/scripts/frontend_apps/components/EmailNotificationsPreferences.tsx b/lms/static/scripts/frontend_apps/components/EmailNotificationsPreferences.tsx index e60489a380..7d25992163 100644 --- a/lms/static/scripts/frontend_apps/components/EmailNotificationsPreferences.tsx +++ b/lms/static/scripts/frontend_apps/components/EmailNotificationsPreferences.tsx @@ -1,43 +1,66 @@ import type { PanelProps } from '@hypothesis/frontend-shared'; -import { Button, Checkbox, Panel } from '@hypothesis/frontend-shared'; +import { Button, Callout, Checkbox, Panel } from '@hypothesis/frontend-shared'; import classnames from 'classnames'; import { useCallback } from 'preact/hooks'; import type { EmailNotificationsPreferences, WeekDay } from '../config'; const dayNames: [WeekDay, string][] = [ - ['instructor_email_digests.days.sun', 'Sunday'], - ['instructor_email_digests.days.mon', 'Monday'], - ['instructor_email_digests.days.tue', 'Tuesday'], - ['instructor_email_digests.days.wed', 'Wednesday'], - ['instructor_email_digests.days.thu', 'Thursday'], - ['instructor_email_digests.days.fri', 'Friday'], - ['instructor_email_digests.days.sat', 'Saturday'], + ['sun', 'Sunday'], + ['mon', 'Monday'], + ['tue', 'Tuesday'], + ['wed', 'Wednesday'], + ['thu', 'Thursday'], + ['fri', 'Friday'], + ['sat', 'Saturday'], ]; export type EmailNotificationsPreferencesProps = { + /** Currently selected days */ selectedDays: EmailNotificationsPreferences; + /** Callback to fully or partially update currently selected days, without saving */ updateSelectedDays: ( newSelectedDays: Partial ) => void; + + /** Callback invoked when saving currently selected days */ + onSave: (submitEvent: Event) => void; + /** Indicates if a save operation is in progress */ + saving?: boolean; + /** + * Represents the result of saving preferences, which can be error or success, + * and includes a message to display. + */ + result?: { + status: 'success' | 'error'; + message: string; + }; + + /** + * Callback used to handle closing the panel. + * If not provided, then the panel won't be considered closable. + */ onClose?: PanelProps['onClose']; }; export default function EmailNotificationsPreferences({ - onClose, selectedDays, updateSelectedDays, + onSave, + saving = false, + result, + onClose, }: EmailNotificationsPreferencesProps) { const setAllTo = useCallback( (enabled: boolean) => updateSelectedDays({ - 'instructor_email_digests.days.sun': enabled, - 'instructor_email_digests.days.mon': enabled, - 'instructor_email_digests.days.tue': enabled, - 'instructor_email_digests.days.wed': enabled, - 'instructor_email_digests.days.thu': enabled, - 'instructor_email_digests.days.fri': enabled, - 'instructor_email_digests.days.sat': enabled, + sun: enabled, + mon: enabled, + tue: enabled, + wed: enabled, + thu: enabled, + fri: enabled, + sat: enabled, }), [updateSelectedDays] ); @@ -45,62 +68,77 @@ export default function EmailNotificationsPreferences({ const selectNone = useCallback(() => setAllTo(false), [setAllTo]); return ( - Save} - > -

- Receive email notifications when your students annotate. -

-

Select the days you{"'"}d like your emails:

- -
-
- {dayNames.map(([day, name]) => ( - - - updateSelectedDays({ [day]: !selectedDays[day] }) - } - data-testid={`${day}-checkbox`} - > - - {name} - - - - ))} -
-
+
+ - Select all - - + } + > +

+ Receive email notifications when your students annotate. +

+

Select the days you{"'"}d like your emails:

+ +
+
+ {dayNames.map(([day, name]) => ( + + + updateSelectedDays({ [day]: !selectedDays[day] }) + } + data-testid={`${day}-checkbox`} + > + + {name} + + + + ))} +
+
+ + +
-
- + {result && {result.message}} + + ); } diff --git a/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx b/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx index 09da619b79..3f8b55c107 100644 --- a/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx +++ b/lms/static/scripts/frontend_apps/components/FilePickerApp.tsx @@ -105,7 +105,7 @@ export async function loadFilePickerConfig( throw new Error('Assignment editing config missing'); } - const authToken = config.api.authToken; + const authToken = config.api!.authToken; const { path, data } = config.editing.getConfig; const { assignment, filePicker } = await apiCall>({ authToken, @@ -166,7 +166,7 @@ export default function FilePickerApp({ onSubmit }: FilePickerAppProps) { }, assignment, filePicker: { deepLinkingAPI, formAction, formFields, promptForTitle }, - } = useConfig(['filePicker']); + } = useConfig(['api', 'filePicker']); // Currently selected content for assignment. const [content, setContent] = useState( diff --git a/lms/static/scripts/frontend_apps/components/GradingControls.tsx b/lms/static/scripts/frontend_apps/components/GradingControls.tsx index 3bcf9c9c4d..7215bf967f 100644 --- a/lms/static/scripts/frontend_apps/components/GradingControls.tsx +++ b/lms/static/scripts/frontend_apps/components/GradingControls.tsx @@ -39,7 +39,7 @@ export default function GradingControls({ }: GradingControlsProps) { const { api: { authToken, sync: syncAPICallInfo }, - } = useConfig(); + } = useConfig(['api']); const clientRPC = useService(ClientRPC); const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false); diff --git a/lms/static/scripts/frontend_apps/components/GroupConfigSelector.tsx b/lms/static/scripts/frontend_apps/components/GroupConfigSelector.tsx index ae48f9103e..e95d2931de 100644 --- a/lms/static/scripts/frontend_apps/components/GroupConfigSelector.tsx +++ b/lms/static/scripts/frontend_apps/components/GroupConfigSelector.tsx @@ -153,7 +153,7 @@ export default function GroupConfigSelector({ product: { api: { listGroupSets: listGroupSetsAPI }, }, - } = useConfig(); + } = useConfig(['api']); const useGroupSet = groupConfig.useGroupSet; const groupSet = useGroupSet ? groupConfig.groupSet : null; diff --git a/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js b/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js index aa29264d02..181749f785 100644 --- a/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js +++ b/lms/static/scripts/frontend_apps/components/test/AppRoot-test.js @@ -70,6 +70,7 @@ describe('AppRoot', () => { { config: { mode: 'email-notifications' }, appComponent: 'EmailNotificationsApp', + route: '/email/preferences', }, { config: { mode: 'error-dialog' }, @@ -79,9 +80,9 @@ describe('AppRoot', () => { config: { mode: 'oauth2-redirect-error' }, appComponent: 'OAuth2RedirectErrorApp', }, - ].forEach(({ config, appComponent }) => { + ].forEach(({ config, appComponent, route }) => { it('launches correct app for "mode" config', () => { - navigateTo(`/app/${config.mode}`); + navigateTo(route ?? `/app/${config.mode}`); const wrapper = renderAppRoot({ config, services: new Map() }); assert.isTrue(wrapper.exists(appComponent)); }); diff --git a/lms/static/scripts/frontend_apps/components/test/EmailNotificationsApp-test.js b/lms/static/scripts/frontend_apps/components/test/EmailNotificationsApp-test.js index 6277bacdfc..4945d749bc 100644 --- a/lms/static/scripts/frontend_apps/components/test/EmailNotificationsApp-test.js +++ b/lms/static/scripts/frontend_apps/components/test/EmailNotificationsApp-test.js @@ -1,19 +1,28 @@ +import { mockImportedComponents } from '@hypothesis/frontend-testing'; import { mount } from 'enzyme'; import { Config } from '../../config'; -import EmailNotificationsApp from '../EmailNotificationsApp'; +import EmailNotificationsApp, { $imports } from '../EmailNotificationsApp'; describe('EmailNotificationsApp', () => { const emailNotificationsConfig = { - 'instructor_email_digests.days.mon': true, - 'instructor_email_digests.days.tue': true, - 'instructor_email_digests.days.wed': false, - 'instructor_email_digests.days.thu': false, - 'instructor_email_digests.days.fri': true, - 'instructor_email_digests.days.sat': false, - 'instructor_email_digests.days.sun': true, + mon: true, + tue: true, + wed: false, + thu: false, + fri: true, + sat: false, + sun: true, }; + beforeEach(() => { + $imports.$mock(mockImportedComponents()); + }); + + afterEach(() => { + $imports.$restore(); + }); + function createComponent() { return mount( @@ -36,8 +45,8 @@ describe('EmailNotificationsApp', () => { it('allows selected days to be updated', () => { const wrapper = createComponent(); const newSelectedDays = { - 'instructor_email_digests.days.mon': false, - 'instructor_email_digests.days.wed': true, + mon: false, + wed: true, }; wrapper @@ -54,4 +63,13 @@ describe('EmailNotificationsApp', () => { } ); }); + + it('when preferences are saved it sets saving to true', () => { + const wrapper = createComponent(); + + wrapper.find('EmailNotificationsPreferences').props().onSave(); + wrapper.update(); + + assert.isTrue(wrapper.find('EmailNotificationsPreferences').prop('saving')); + }); }); diff --git a/lms/static/scripts/frontend_apps/components/test/EmailNotificationsPreferences-test.js b/lms/static/scripts/frontend_apps/components/test/EmailNotificationsPreferences-test.js index 99c6e8cefe..28b5864a23 100644 --- a/lms/static/scripts/frontend_apps/components/test/EmailNotificationsPreferences-test.js +++ b/lms/static/scripts/frontend_apps/components/test/EmailNotificationsPreferences-test.js @@ -5,32 +5,31 @@ import EmailNotificationsPreferences from '../EmailNotificationsPreferences'; describe('EmailNotificationsPreferences', () => { let fakeUpdateSelectedDays; const initialSelectedDays = { - 'instructor_email_digests.days.sun': true, - 'instructor_email_digests.days.mon': true, - 'instructor_email_digests.days.tue': false, - 'instructor_email_digests.days.wed': true, - 'instructor_email_digests.days.thu': false, - 'instructor_email_digests.days.fri': false, - 'instructor_email_digests.days.sat': true, + sun: true, + mon: true, + tue: false, + wed: true, + thu: false, + fri: false, + sat: true, }; beforeEach(() => { fakeUpdateSelectedDays = sinon.stub(); }); - function createComponent() { + function createComponent(props = {}) { return mount( ); } function getCheckbox(wrapper, day) { - return wrapper.find( - `Checkbox[data-testid="instructor_email_digests.days.${day}-checkbox"]` - ); + return wrapper.find(`Checkbox[data-testid="${day}-checkbox"]`); } it('initially selects appropriate checkboxes', () => { @@ -51,13 +50,13 @@ describe('EmailNotificationsPreferences', () => { wrapper.find('button[data-testid="select-all-button"]').simulate('click'); assert.calledWith(fakeUpdateSelectedDays, { - 'instructor_email_digests.days.sun': true, - 'instructor_email_digests.days.mon': true, - 'instructor_email_digests.days.tue': true, - 'instructor_email_digests.days.wed': true, - 'instructor_email_digests.days.thu': true, - 'instructor_email_digests.days.fri': true, - 'instructor_email_digests.days.sat': true, + sun: true, + mon: true, + tue: true, + wed: true, + thu: true, + fri: true, + sat: true, }); }); @@ -67,26 +66,54 @@ describe('EmailNotificationsPreferences', () => { wrapper.find('button[data-testid="select-none-button"]').simulate('click'); assert.calledWith(fakeUpdateSelectedDays, { - 'instructor_email_digests.days.sun': false, - 'instructor_email_digests.days.mon': false, - 'instructor_email_digests.days.tue': false, - 'instructor_email_digests.days.wed': false, - 'instructor_email_digests.days.thu': false, - 'instructor_email_digests.days.fri': false, - 'instructor_email_digests.days.sat': false, + sun: false, + mon: false, + tue: false, + wed: false, + thu: false, + fri: false, + sat: false, }); }); ['sun', 'mon', 'thu'].forEach(day => { it('lets individual days be changed', () => { const wrapper = createComponent(); - const dayKey = `instructor_email_digests.days.${day}`; getCheckbox(wrapper, day).props().onChange(); assert.calledWith(fakeUpdateSelectedDays, { - [dayKey]: !initialSelectedDays[dayKey], + [day]: !initialSelectedDays[day], }); }); }); + + [ + { message: 'Error', status: 'error' }, + { message: 'Success', status: 'success' }, + undefined, + ].forEach(result => { + it('displays error message if provided', () => { + const wrapper = createComponent({ result }); + assert.equal(wrapper.exists('Callout'), !!result); + }); + }); + + [true, false].forEach(saving => { + it('disables save button while saving', () => { + const wrapper = createComponent({ saving }); + const button = wrapper.find('Button[data-testid="save-button"]'); + + assert.equal(button.prop('disabled'), saving); + }); + }); + + it('saves preferences', () => { + const onSave = sinon.stub(); + const wrapper = createComponent({ onSave }); + + wrapper.find('form').simulate('submit'); + + assert.called(onSave); + }); }); diff --git a/lms/static/scripts/frontend_apps/config.ts b/lms/static/scripts/frontend_apps/config.ts index 6169648bbe..656b45a4e8 100644 --- a/lms/static/scripts/frontend_apps/config.ts +++ b/lms/static/scripts/frontend_apps/config.ts @@ -221,14 +221,7 @@ export type Product = { api: ProductAPI; }; -export type WeekDay = - | 'instructor_email_digests.days.mon' - | 'instructor_email_digests.days.tue' - | 'instructor_email_digests.days.wed' - | 'instructor_email_digests.days.thu' - | 'instructor_email_digests.days.fri' - | 'instructor_email_digests.days.sat' - | 'instructor_email_digests.days.sun'; +export type WeekDay = 'mon' | 'tue' | 'wed' | 'thu' | 'fri' | 'sat' | 'sun'; export type EmailNotificationsPreferences = Record; @@ -243,7 +236,9 @@ export type EmailNotificationsPreferences = Record; export type ConfigObject = { // Configuration present in all modes. mode: AppMode; - api: { + + // Present in most modes. + api?: { authToken: string; // Only present in "basic-lti-launch" mode. diff --git a/lms/static/scripts/frontend_apps/index.tsx b/lms/static/scripts/frontend_apps/index.tsx index deb71f8be2..5079faa38b 100644 --- a/lms/static/scripts/frontend_apps/index.tsx +++ b/lms/static/scripts/frontend_apps/index.tsx @@ -4,6 +4,7 @@ import { render } from 'preact'; import 'preact/debug'; import AppRoot from './components/AppRoot'; +import type { AppMode } from './config'; import { readConfig } from './config'; import { ClientRPC, @@ -13,6 +14,24 @@ import { } from './services'; import type { ServiceMap } from './services'; +/** + * Converts an app mode into its corresponding URL. + * + * For historical reasons, some resolve the mode itself prefixed with `/app/`, + * but that is not ideal, and should be avoided, because the BE is not aware of + * those routes and reloading the page results in a 404. + */ +function routeForAppMode(mode: AppMode): string { + if (mode === 'email-notifications') { + // For the email-notifications mode, since this app is not designed to be + // opened in an iframe, but as the main window frame, we want to use a route + // that matches the server-side one. + return '/email/preferences'; + } + + return `/app/${mode}`; +} + export function init() { // Read configuration embedded into page by backend. const config = readConfig(); @@ -39,12 +58,14 @@ export function init() { // Construct services used by the file picker app. We need this both for // direct launches into this app, and for transitions from viewing to // editing assignments. - services.set( - VitalSourceService, - new VitalSourceService({ authToken: config.api.authToken }) - ); + if (config.api?.authToken) { + services.set( + VitalSourceService, + new VitalSourceService({ authToken: config.api.authToken }) + ); + } - if (config.rpcServer && config.hypothesisClient) { + if (config.api && config.rpcServer && config.hypothesisClient) { const { authToken } = config.api; const clientRPC = new ClientRPC({ authToken, @@ -68,7 +89,7 @@ export function init() { } // Set route based on app mode. - const routePath = `/app/${config.mode}`; + const routePath = routeForAppMode(config.mode); if (location.pathname !== routePath) { history.replaceState({}, 'unused', routePath); } diff --git a/lms/static/scripts/frontend_apps/test/index-test.js b/lms/static/scripts/frontend_apps/test/index-test.js index 8985e1d915..9fd4883866 100644 --- a/lms/static/scripts/frontend_apps/test/index-test.js +++ b/lms/static/scripts/frontend_apps/test/index-test.js @@ -10,6 +10,7 @@ const minimalConfig = { allowedOrigins: ['https://example.com'], }, mode: 'basic-lti-launch', + emailNotifications: {}, }; describe('LMS frontend entry', () => { @@ -50,16 +51,20 @@ describe('LMS frontend entry', () => { $imports.$restore(); }); - it('renders root component', () => { - init(); + ['basic-lti-launch', 'email-notifications'].forEach(mode => { + it('renders root component', () => { + fakeReadConfig.returns({ ...minimalConfig, mode }); - const container = document.querySelector('#app'); - assert.ok(container.querySelector('[data-testid=app-root]')); + init(); + + const container = document.querySelector('#app'); + assert.ok(container.querySelector('[data-testid=app-root]')); - assert.called(AppRoot); - const props = AppRoot.args[0][0]; - assert.equal(props.initialConfig, fakeReadConfig()); - assert.instanceOf(props.services, Map); + assert.called(AppRoot); + const props = AppRoot.args[0][0]; + assert.equal(props.initialConfig, fakeReadConfig()); + assert.instanceOf(props.services, Map); + }); }); it('console logs debug values', () => { diff --git a/lms/static/scripts/frontend_apps/utils/api.ts b/lms/static/scripts/frontend_apps/utils/api.ts index abf2d24d0b..980bc78f18 100644 --- a/lms/static/scripts/frontend_apps/utils/api.ts +++ b/lms/static/scripts/frontend_apps/utils/api.ts @@ -173,7 +173,7 @@ export function useAPIFetch( ): FetchResult { const { api: { authToken }, - } = useConfig(); + } = useConfig(['api']); const fetcher: Fetcher | undefined = path ? signal => diff --git a/lms/templates/email/preferences.html.jinja2 b/lms/templates/email/preferences.html.jinja2 index ec13864135..64a269a622 100644 --- a/lms/templates/email/preferences.html.jinja2 +++ b/lms/templates/email/preferences.html.jinja2 @@ -1,23 +1,20 @@ -{% extends 'templates/base.plain.html.jinja2' %} +{% extends "lms:templates/base.html.jinja2" %} {% block content %} -{% for message in request.session.pop_flash() %} -

{{ message }}

-{% endfor %} - -
-
- Receive email notifications when your students annotate. Select - the days you'd like your emails: +
+{% endblock %} -
    - {% for key, value in preferences.items() %} -
  • -
  • - {% endfor %} -
-
+{% block styles %} + {% for url in asset_urls("frontend_apps_css") %} + + {% endfor %} +{% endblock %} - -
+{% block scripts %} + {# Not calling super() here, as the parent template tries to build the js-config using the JSConfig class, which is + not available here. #} + + {% for url in asset_urls("frontend_apps_js") %} + + {% endfor %} {% endblock %} diff --git a/lms/views/email.py b/lms/views/email.py index 821a918ad9..e319a0c6db 100644 --- a/lms/views/email.py +++ b/lms/views/email.py @@ -77,9 +77,12 @@ def preferences_redirect(self): ) def preferences(self): return { - "preferences": self.email_preferences_service.get_preferences( - self.request.authenticated_userid - ).days(), + "jsConfig": { + "mode": "email-notifications", + "emailNotifications": self.email_preferences_service.get_preferences( + self.request.authenticated_userid + ).days(), + } } @view_config( diff --git a/tests/unit/lms/views/email_test.py b/tests/unit/lms/views/email_test.py index 0ae4ebfc1c..af9962114a 100644 --- a/tests/unit/lms/views/email_test.py +++ b/tests/unit/lms/views/email_test.py @@ -55,7 +55,10 @@ def test_preferences(self, views, email_preferences_service, pyramid_request): pyramid_request.authenticated_userid ) assert result == { - "preferences": email_preferences_service.get_preferences.return_value.days.return_value + "jsConfig": { + "mode": "email-notifications", + "emailNotifications": email_preferences_service.get_preferences.return_value.days.return_value, + } } def test_set_preferences(self, views, email_preferences_service, pyramid_request):