From 904c5d41453d07f34a24c863a406e411dc6c5b02 Mon Sep 17 00:00:00 2001 From: David Joy Date: Fri, 23 Apr 2021 14:23:33 -0400 Subject: [PATCH] feat: add network connectivity and permission denied alerts (#97) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: reusable connection error and permission denied alerts This commit pulls the connection error and permission denied alerts out of ProctoredExamSettings and also makes them Open edX friendly by removing references to “edX” and using the SUPPORT_URL environment variable to supply the support link. This is in preparation for using these alerts in the Discussions UI. * refactor: saveAppConfig now responsible for redirect I’ve moved the redirect to the pages and resources path into the thunk for saveAppConfig. This is because we only want to do it if the thunk is successful, and it’s easier to do it here than to have `then` and `catch` handlers in the component. In particular, this is because we can’t stop the `then` from happening unless we throw an error from the thunk, but the component has nothing to do on a thrown error. This avoids the awkward code in the component and just handles it all here. * feat: handle access denied by setting DENIED statuses This takes us one step closer to user messaging for permission denied errors by setting an explicit DENIED status and saveStatus when a request was denied because the user didn’t have permissions. Note that this is different than a 401, which is unauthorized, meaning the user is logged out. This doesn’t handle 401s. Following this, we can then use the DENIED status to give the user feedback on what’s going on. * feat: adding error alerts This commit adds friendly error alerts for connection and permission denied errors in the discussions app. If the initial fetch apps request errors out or is denied, then the entire contents of the modal is replaced with an error message. If the save app config request errors out, then a message is displayed at the top of the form. If the sae app config request is denied, the entire contents of the modal is replaced with a permission denied error message, as in the first case. --- .env | 9 +- .env.development | 10 +- .env.test | 11 +- src/generic/ConnectionErrorAlert.jsx | 30 ++ src/generic/PermissionDeniedAlert.jsx | 16 + src/generic/SaveFormConnectionErrorAlert.jsx | 30 ++ src/index.jsx | 9 +- src/messages.js | 15 + .../discussions/DiscussionsSettings.jsx | 32 +- .../discussions/DiscussionsSettings.test.jsx | 390 ++++++++++++------ .../app-config-form/AppConfigForm.jsx | 21 +- .../discussions/data/redux.test.js | 105 ++++- .../discussions/data/slice.js | 1 + .../discussions/data/thunks.js | 26 +- .../ProctoredExamSettings.jsx | 35 +- .../ProctoredExamSettings.messages.jsx | 2 +- .../ProctoredExamSettings.test.jsx | 8 +- 17 files changed, 551 insertions(+), 199 deletions(-) create mode 100644 src/generic/ConnectionErrorAlert.jsx create mode 100644 src/generic/PermissionDeniedAlert.jsx create mode 100644 src/generic/SaveFormConnectionErrorAlert.jsx create mode 100644 src/messages.js diff --git a/.env b/.env index 035b99d22..410a9f750 100644 --- a/.env +++ b/.env @@ -3,15 +3,22 @@ ACCESS_TOKEN_COOKIE_NAME=null BASE_URL=null CREDENTIALS_BASE_URL=null CSRF_TOKEN_API_PATH=null +DISCOVERY_API_BASE_URL= ECOMMERCE_BASE_URL=null +FAVICON_URL='' +FAVICON_URL=null LANGUAGE_PREFERENCE_COOKIE_NAME=null LMS_BASE_URL=null LOGIN_URL=null +LOGO_TRADEMARK_URL='' +LOGO_URL='' +LOGO_WHITE_URL='' LOGOUT_URL=null -FAVICON_URL=null MARKETING_SITE_BASE_URL=null ORDER_HISTORY_URL=null +PUBLISHER_BASE_URL= REFRESH_ACCESS_TOKEN_ENDPOINT=null SEGMENT_KEY=null SITE_NAME=null +SUPPORT_URL=null USER_INFO_COOKIE_NAME=null diff --git a/.env.development b/.env.development index 3634948f0..c2e8d56b2 100644 --- a/.env.development +++ b/.env.development @@ -3,17 +3,23 @@ ACCESS_TOKEN_COOKIE_NAME='edx-jwt-cookie-header-payload' BASE_URL='localhost:2001' CREDENTIALS_BASE_URL='http://localhost:18150' CSRF_TOKEN_API_PATH='/csrf/api/v1/token' +DISCOVERY_API_BASE_URL= ECOMMERCE_BASE_URL='http://localhost:18130' +FAVICON_URL='https://edx-cdn.org/v3/default/favicon.ico' LANGUAGE_PREFERENCE_COOKIE_NAME='openedx-language-preference' LMS_BASE_URL='http://localhost:18000' LOGIN_URL='http://localhost:18000/login' +LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg +LOGO_URL=https://edx-cdn.org/v3/default/logo.svg +LOGO_WHITE_URL=https://edx-cdn.org/v3/default/logo-white.svg LOGOUT_URL='http://localhost:18000/logout' -FAVICON_URL='https://edx-cdn.org/v3/default/favicon.ico' MARKETING_SITE_BASE_URL='http://localhost:18000' -STUDIO_BASE_URL='http://localhost:18010' ORDER_HISTORY_URL='localhost:1996/orders' PORT=2001 +PUBLISHER_BASE_URL= REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh' SEGMENT_KEY=null SITE_NAME='edX' +STUDIO_BASE_URL='http://localhost:18010' +SUPPORT_URL='https://support.edx.org' USER_INFO_COOKIE_NAME='edx-user-info' diff --git a/.env.test b/.env.test index 1a50e5b85..0e50cf2d7 100644 --- a/.env.test +++ b/.env.test @@ -2,17 +2,24 @@ ACCESS_TOKEN_COOKIE_NAME='edx-jwt-cookie-header-payload' BASE_URL='localhost:2001' CREDENTIALS_BASE_URL='http://localhost:18150' CSRF_TOKEN_API_PATH='/csrf/api/v1/token' +DISCOVERY_API_BASE_URL='http://localhost:18381' ECOMMERCE_BASE_URL='http://localhost:18130' +FAVICON_URL='https://edx-cdn.org/v3/default/favicon.ico' +FAVICON_URL=https://edx-cdn.org/v3/default/favicon.ico LANGUAGE_PREFERENCE_COOKIE_NAME='openedx-language-preference' LMS_BASE_URL='http://localhost:18000' LOGIN_URL='http://localhost:18000/login' +LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg +LOGO_URL=https://edx-cdn.org/v3/default/logo.svg +LOGO_WHITE_URL=https://edx-cdn.org/v3/default/logo-white.svg LOGOUT_URL='http://localhost:18000/logout' -FAVICON_URL='https://edx-cdn.org/v3/default/favicon.ico' -STUDIO_BASE_URL='http://localhost:18010' MARKETING_SITE_BASE_URL='http://localhost:18000' ORDER_HISTORY_URL='localhost:1996/orders' PORT=2001 +PUBLISHER_BASE_URL= REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh' SEGMENT_KEY=null SITE_NAME='edX' +STUDIO_BASE_URL='http://localhost:18010' +SUPPORT_URL='https://support.edx.org' USER_INFO_COOKIE_NAME='edx-user-info' diff --git a/src/generic/ConnectionErrorAlert.jsx b/src/generic/ConnectionErrorAlert.jsx new file mode 100644 index 000000000..ee2daf5fc --- /dev/null +++ b/src/generic/ConnectionErrorAlert.jsx @@ -0,0 +1,30 @@ +import React from 'react'; +import { injectIntl, intlShape, FormattedMessage } from '@edx/frontend-platform/i18n'; +import { Alert } from '@edx/paragon'; +import { getConfig } from '@edx/frontend-platform'; + +import messages from '../messages'; + +function ConnectionErrorAlert({ intl }) { + return ( + + + {intl.formatMessage(messages.supportText)} + + ), + }} + /> + + ); +} + +ConnectionErrorAlert.propTypes = { + intl: intlShape.isRequired, +}; + +export default injectIntl(ConnectionErrorAlert); diff --git a/src/generic/PermissionDeniedAlert.jsx b/src/generic/PermissionDeniedAlert.jsx new file mode 100644 index 000000000..3bef2d068 --- /dev/null +++ b/src/generic/PermissionDeniedAlert.jsx @@ -0,0 +1,16 @@ +import React from 'react'; +import { FormattedMessage } from '@edx/frontend-platform/i18n'; +import { Alert } from '@edx/paragon'; + +function PermissionDeniedAlert() { + return ( + + + + ); +} + +export default PermissionDeniedAlert; diff --git a/src/generic/SaveFormConnectionErrorAlert.jsx b/src/generic/SaveFormConnectionErrorAlert.jsx new file mode 100644 index 000000000..67a0564d5 --- /dev/null +++ b/src/generic/SaveFormConnectionErrorAlert.jsx @@ -0,0 +1,30 @@ +import React from 'react'; +import { injectIntl, intlShape, FormattedMessage } from '@edx/frontend-platform/i18n'; +import { Alert } from '@edx/paragon'; +import { getConfig } from '@edx/frontend-platform'; + +import messages from '../messages'; + +function SaveFormConnectionErrorAlert({ intl }) { + return ( + + + {intl.formatMessage(messages.supportText)} + + ), + }} + /> + + ); +} + +SaveFormConnectionErrorAlert.propTypes = { + intl: intlShape.isRequired, +}; + +export default injectIntl(SaveFormConnectionErrorAlert); diff --git a/src/index.jsx b/src/index.jsx index 241dc6902..2a3f91a05 100755 --- a/src/index.jsx +++ b/src/index.jsx @@ -2,7 +2,7 @@ import 'core-js/stable'; import 'regenerator-runtime/runtime'; import { - APP_INIT_ERROR, APP_READY, subscribe, initialize, + APP_INIT_ERROR, APP_READY, subscribe, initialize, mergeConfig, } from '@edx/frontend-platform'; import { AppProvider, ErrorPage } from '@edx/frontend-platform/react'; import React from 'react'; @@ -53,6 +53,13 @@ subscribe(APP_INIT_ERROR, (error) => { }); initialize({ + handlers: { + config: () => { + mergeConfig({ + SUPPORT_URL: process.env.SUPPORT_URL || null, + }, 'CourseAuthoringConfig'); + }, + }, messages: [ appMessages, footerMessages, diff --git a/src/messages.js b/src/messages.js new file mode 100644 index 000000000..1620914b5 --- /dev/null +++ b/src/messages.js @@ -0,0 +1,15 @@ +import { defineMessages } from '@edx/frontend-platform/i18n'; + +const messages = defineMessages({ + connectionError: { + id: 'authoring.alert.error.connection', + defaultMessage: 'We encountered a technical error when loading this page. This might be a temporary issue, so please try again in a few minutes. If the problem persists, please go to the {supportLink} for help.', + description: 'Error message shown to users when there is a connectivity issue with the server.', + }, + supportText: { + id: 'authoring.alert.support.text', + defaultMessage: 'Support Page', + }, +}); + +export default messages; diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.jsx index dbc170498..b8e2385ed 100644 --- a/src/pages-and-resources/discussions/DiscussionsSettings.jsx +++ b/src/pages-and-resources/discussions/DiscussionsSettings.jsx @@ -5,7 +5,7 @@ import PropTypes from 'prop-types'; import { useRouteMatch, } from 'react-router'; -import { useDispatch } from 'react-redux'; +import { useDispatch, useSelector } from 'react-redux'; import { history } from '@edx/frontend-platform'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; @@ -20,6 +20,9 @@ import DiscussionsProvider from './DiscussionsProvider'; import { fetchApps } from './data/thunks'; import AppList from './app-list'; import AppConfigForm from './app-config-form'; +import { DENIED, FAILED } from './data/slice'; +import ConnectionErrorAlert from '../../generic/ConnectionErrorAlert'; +import PermissionDeniedAlert from '../../generic/PermissionDeniedAlert'; const SELECTION_STEP = 'selection'; const SETTINGS_STEP = 'settings'; @@ -27,6 +30,7 @@ const SETTINGS_STEP = 'settings'; function DiscussionsSettings({ courseId, intl }) { const dispatch = useDispatch(); const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext); + const { status } = useSelector(state => state.discussions); useEffect(() => { dispatch(fetchApps(courseId)); @@ -50,6 +54,32 @@ function DiscussionsSettings({ courseId, intl }) { history.push(discussionsPath); }, [discussionsPath]); + if (status === FAILED) { + return ( + + + + ); + } + + if (status === DENIED) { + return ( + + + + ); + } + return ( diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx index 2ae175767..6a9707a24 100644 --- a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx +++ b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx @@ -1,14 +1,23 @@ import React from 'react'; import { + act, queryByLabelText, - queryByTestId, queryByText, render, screen, waitForElementToBeRemoved, act, + queryByTestId, + queryByText, + render, + screen, + waitForElementToBeRemoved, + queryByRole, + waitFor, } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { AppProvider, PageRoute } from '@edx/frontend-platform/react'; import { Switch } from 'react-router'; -import { history, initializeMockApp } from '@edx/frontend-platform'; +import { + getConfig, history, initializeMockApp, setConfig, +} from '@edx/frontend-platform'; import MockAdapter from 'axios-mock-adapter'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import DiscussionsSettings from './DiscussionsSettings'; @@ -23,6 +32,26 @@ let axiosMock; let store; let container; +function renderComponent() { + const wrapper = render( + + + + + + + + + , + ); + container = wrapper.container; +} + describe('DiscussionsSettings', () => { beforeEach(() => { initializeMockApp({ @@ -37,141 +66,242 @@ describe('DiscussionsSettings', () => { store = initializeStore(); axiosMock = new MockAdapter(getAuthenticatedHttpClient()); - axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); - // Leave the DiscussionsSettings route after the test. history.push(`/course/${courseId}/pages-and-resources`); - - const wrapper = render( - - - - - - - - - , - ); - container = wrapper.container; }); - afterEach(() => { + describe('with successful network connections', () => { + beforeEach(() => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); - }); - - test('sets selection step from routes', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - expect(queryByTestId(container, 'appList')).toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); - }); - - test('sets settings step from routes', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); - }); - - test('successfully advances to settings step for lti', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - userEvent.click(queryByLabelText(container, 'Select Piazza')); - userEvent.click(queryByText(container, 'Next')); - - expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); - expect(queryByTestId(container, 'ltiConfigForm')).toBeInTheDocument(); - expect(queryByTestId(container, 'legacyConfigForm')).not.toBeInTheDocument(); - }); - - test('successfully advances to settings step for legacy', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - userEvent.click(queryByLabelText(container, 'Select edX Discussions')); - userEvent.click(queryByText(container, 'Next')); - - expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); - expect(queryByTestId(container, 'ltiConfigForm')).not.toBeInTheDocument(); - expect(queryByTestId(container, 'legacyConfigForm')).toBeInTheDocument(); - }); - - test('successfully goes back to first step', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); - - userEvent.click(queryByText(container, 'Back')); - - expect(queryByTestId(container, 'appList')).toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); - }); - - test('successfully closes the modal', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions`); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - expect(queryByTestId(container, 'appList')).toBeInTheDocument(); - - userEvent.click(queryByLabelText(container, 'Close')); - - expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); - expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); - - expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources`); - }); - - test('successfully submit the modal', async () => { - history.push(`/course/${courseId}/pages-and-resources/discussions`); - - axiosMock.onPost(getAppsUrl(courseId)).reply(200, piazzaApiResponse); - - // This is an important line that ensures the spinner has been removed - and thus our main - // content has been loaded - prior to proceeding with our expectations. - await waitForElementToBeRemoved(screen.getByRole('status')); - - userEvent.click(queryByLabelText(container, 'Select Piazza')); - userEvent.click(queryByText(container, 'Next')); - // Apply causes an async action to take place - act(() => { - userEvent.click(queryByText(container, 'Apply')); + renderComponent(); }); - // This is an important line that ensures the Close button has been removed, which implies that - // the full screen modal has been closed following our click of Apply. Once this has happened, - // then it's safe to proceed with our expectations. - await waitForElementToBeRemoved(screen.queryByLabelText('Close')); + test('sets selection step from routes', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); - expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources`); + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + expect(queryByTestId(container, 'appList')).toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); + }); + + test('sets settings step from routes', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); + }); + + test('successfully advances to settings step for lti', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + userEvent.click(queryByLabelText(container, 'Select Piazza')); + userEvent.click(queryByText(container, 'Next')); + + expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); + expect(queryByTestId(container, 'ltiConfigForm')).toBeInTheDocument(); + expect(queryByTestId(container, 'legacyConfigForm')).not.toBeInTheDocument(); + }); + + test('successfully advances to settings step for legacy', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + userEvent.click(queryByLabelText(container, 'Select edX Discussions')); + userEvent.click(queryByText(container, 'Next')); + + expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); + expect(queryByTestId(container, 'ltiConfigForm')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'legacyConfigForm')).toBeInTheDocument(); + }); + + test('successfully goes back to first step', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); + + userEvent.click(queryByText(container, 'Back')); + + expect(queryByTestId(container, 'appList')).toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); + }); + + test('successfully closes the modal', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + expect(queryByTestId(container, 'appList')).toBeInTheDocument(); + + userEvent.click(queryByLabelText(container, 'Close')); + + expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); + + expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources`); + }); + + test('successfully submit the modal', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + axiosMock.onPost(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + userEvent.click(queryByLabelText(container, 'Select Piazza')); + userEvent.click(queryByText(container, 'Next')); + // Apply causes an async action to take place + act(() => { + userEvent.click(queryByText(container, 'Apply')); + }); + + // This is an important line that ensures the Close button has been removed, which implies that + // the full screen modal has been closed following our click of Apply. Once this has happened, + // then it's safe to proceed with our expectations. + await waitForElementToBeRemoved(screen.queryByLabelText('Close')); + + expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources`); + }); + }); + + describe('with network error fetchApps API requests', () => { + beforeEach(() => { + // Expedient way of getting SUPPORT_URL into config. + setConfig({ + ...getConfig(), + SUPPORT_URL: 'http://support.edx.org', + }); + + axiosMock.onGet(getAppsUrl(courseId)).networkError(); + + renderComponent(); + }); + + test('shows connection error alert', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + const alert = queryByRole(container, 'alert'); + expect(alert).toBeInTheDocument(); + expect(alert.textContent).toEqual(expect.stringContaining('We encountered a technical error when loading this page.')); + expect(alert.innerHTML).toEqual(expect.stringContaining(getConfig().SUPPORT_URL)); + }); + }); + + describe('with network error postAppConfig API requests', () => { + beforeEach(() => { + // Expedient way of getting SUPPORT_URL into config. + setConfig({ + ...getConfig(), + SUPPORT_URL: 'http://support.edx.org', + }); + + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + axiosMock.onPost(getAppsUrl(courseId)).networkError(); + renderComponent(); + }); + + test('shows connection error alert at top of form', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + // Apply causes an async action to take place + act(() => { + userEvent.click(queryByText(container, 'Apply')); + }); + + await waitFor(() => expect(axiosMock.history.post.length).toBe(1)); + + expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument(); + + const alert = queryByRole(container, 'alert'); + expect(alert).toBeInTheDocument(); + expect(alert.textContent).toEqual(expect.stringContaining('We encountered a technical error when applying changes.')); + expect(alert.innerHTML).toEqual(expect.stringContaining(getConfig().SUPPORT_URL)); + }); + }); + + describe('with permission denied error for fetchApps API requests', () => { + beforeEach(() => { + axiosMock.onGet(getAppsUrl(courseId)).reply(403); + + renderComponent(); + }); + + test('shows permission denied alert', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + const alert = queryByRole(container, 'alert'); + expect(alert).toBeInTheDocument(); + expect(alert.textContent).toEqual(expect.stringContaining('You are not authorized to view this page.')); + }); + }); + + describe('with permission denied error for postAppConfig API requests', () => { + beforeEach(() => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + axiosMock.onPost(getAppsUrl(courseId)).reply(403); + + renderComponent(); + }); + + test('shows permission denied alert at top of form', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + // This is an important line that ensures the spinner has been removed - and thus our main + // content has been loaded - prior to proceeding with our expectations. + await waitForElementToBeRemoved(screen.getByRole('status')); + + // Apply causes an async action to take place + act(() => { + userEvent.click(queryByText(container, 'Apply')); + }); + + await waitFor(() => expect(axiosMock.history.post.length).toBe(1)); + + expect(queryByTestId(container, 'appList')).not.toBeInTheDocument(); + expect(queryByTestId(container, 'appConfigForm')).not.toBeInTheDocument(); + + // We don't technically leave the route in this case, though the modal is hidden. + expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + const alert = queryByRole(container, 'alert'); + expect(alert).toBeInTheDocument(); + expect(alert.textContent).toEqual(expect.stringContaining('You are not authorized to view this page.')); + }); }); }); diff --git a/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx b/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx index 1bdce34ab..fcba84aea 100644 --- a/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx +++ b/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx @@ -5,12 +5,13 @@ import PropTypes from 'prop-types'; import { useDispatch, useSelector } from 'react-redux'; import { useRouteMatch } from 'react-router'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; -import { history } from '@edx/frontend-platform'; import { Container } from '@edx/paragon'; import { useModel } from '../../../generic/model-store'; import { PagesAndResourcesContext } from '../../PagesAndResourcesProvider'; -import { LOADED, LOADING, selectApp } from '../data/slice'; +import { + FAILED, LOADED, LOADING, selectApp, +} from '../data/slice'; import { saveAppConfig } from '../data/thunks'; import messages from './messages'; @@ -19,6 +20,7 @@ import AppConfigFormApplyButton from './AppConfigFormApplyButton'; import LegacyConfigForm from './apps/legacy'; import LtiConfigForm from './apps/lti'; import Loading from '../../../generic/Loading'; +import SaveFormConnectionErrorAlert from '../../../generic/SaveFormConnectionErrorAlert'; function AppConfigForm({ courseId, intl, @@ -27,7 +29,7 @@ function AppConfigForm({ const { formRef } = useContext(AppConfigFormContext); const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext); const { params: { appId: routeAppId } } = useRouteMatch(); - const { selectedAppId, status } = useSelector(state => state.discussions); + const { selectedAppId, status, saveStatus } = useSelector(state => state.discussions); const app = useModel('apps', selectedAppId); // appConfigs have no ID of their own, so we use the active app ID to reference them. // This appConfig may come back as null if the selectedAppId is not the activeAppId, i.e., @@ -44,9 +46,8 @@ function AppConfigForm({ // This is a callback that gets called after the form has been submitted successfully. const handleSubmit = useCallback((values) => { - dispatch(saveAppConfig(courseId, selectedAppId, values)).then(() => { - history.push(pagesAndResourcesPath); - }); + // Note that when this action succeeds, we redirect to pagesAndResurcesPath in the thunk. + dispatch(saveAppConfig(courseId, selectedAppId, values, pagesAndResourcesPath)); }, [courseId, selectedAppId, courseId]); if (!selectedAppId || status === LOADING) { @@ -55,6 +56,13 @@ function AppConfigForm({ ); } + let alert = null; + if (saveStatus === FAILED) { + alert = ( + + ); + } + let form = null; if (app.id === 'legacy') { form = ( @@ -78,6 +86,7 @@ function AppConfigForm({ } return ( + {alert} {form} ); diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js index 5ab146d18..4fd624d9d 100644 --- a/src/pages-and-resources/discussions/data/redux.test.js +++ b/src/pages-and-resources/discussions/data/redux.test.js @@ -1,9 +1,12 @@ import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import MockAdapter from 'axios-mock-adapter'; import { initializeMockApp } from '@edx/frontend-platform/testing'; +import { history } from '@edx/frontend-platform'; import initializeStore from '../../../store'; import { getAppsUrl } from './api'; -import { FAILED, SAVED, selectApp } from './slice'; +import { + FAILED, SAVED, DENIED, selectApp, +} from './slice'; import { fetchApps, saveAppConfig } from './thunks'; import { LOADED } from '../../../data/slice'; import { legacyApiResponse, piazzaApiResponse } from '../factories/mockApiResponses'; @@ -16,7 +19,7 @@ const executeThunk = async (thunk, dispatch, getState) => { }; const courseId = 'course-v1:edX+TestX+Test_Course'; - +const pagesAndResourcesPath = `/course/${courseId}/pages-and-resources`; const featuresState = { 'discussion-page': { id: 'discussion-page', @@ -103,6 +106,23 @@ describe('Data layer integration tests', () => { ); }); + test('permission denied error', async () => { + axiosMock.onGet(getAppsUrl(courseId)).reply(403); + + await executeThunk(fetchApps(courseId), store.dispatch); + + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: [], + featureIds: [], + activeAppId: null, + selectedAppId: null, + status: DENIED, + saveStatus: SAVED, + }), + ); + }); + test('successfully loads an LTI configuration', async () => { axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); @@ -170,14 +190,18 @@ describe('Data layer integration tests', () => { describe('saveAppConfig', () => { test('network error', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); axiosMock.onPost(getAppsUrl(courseId)).networkError(); // We call fetchApps and selectApp here too just to get us into a real state. await executeThunk(fetchApps(courseId), store.dispatch); store.dispatch(selectApp({ appId: 'piazza' })); - await executeThunk(saveAppConfig(courseId, 'piazza', {}), store.dispatch); + await executeThunk(saveAppConfig(courseId, 'piazza', {}, pagesAndResourcesPath), store.dispatch); + // Assert we're still on the form. + expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); expect(store.getState().discussions).toEqual( expect.objectContaining({ appIds: ['legacy', 'piazza'], @@ -190,7 +214,34 @@ describe('Data layer integration tests', () => { ); }); + test('permission denied error', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + axiosMock.onPost(getAppsUrl(courseId)).reply(403); + + // We call fetchApps and selectApp here too just to get us into a real state. + await executeThunk(fetchApps(courseId), store.dispatch); + store.dispatch(selectApp({ appId: 'piazza' })); + await executeThunk(saveAppConfig(courseId, 'piazza', {}, pagesAndResourcesPath), store.dispatch); + + // Assert we're still on the form. + expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'piazza', + selectedAppId: 'piazza', + status: DENIED, // We set BOTH statuses to DENIED for saveAppConfig - this removes the UI. + saveStatus: DENIED, + }), + ); + }); + test('successfully saves an LTI configuration', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); axiosMock.onPost(getAppsUrl(courseId), { context_key: courseId, @@ -217,12 +268,18 @@ describe('Data layer integration tests', () => { // We call fetchApps and selectApp here too just to get us into a real state. await executeThunk(fetchApps(courseId), store.dispatch); store.dispatch(selectApp({ appId: 'piazza' })); - await executeThunk(saveAppConfig(courseId, 'piazza', { - consumerKey: 'new_consumer_key', - consumerSecret: 'new_consumer_secret', - launchUrl: 'http://localhost/new_launch_url', - }), store.dispatch); + await executeThunk(saveAppConfig( + courseId, + 'piazza', + { + consumerKey: 'new_consumer_key', + consumerSecret: 'new_consumer_secret', + launchUrl: 'http://localhost/new_launch_url', + }, + pagesAndResourcesPath, + ), store.dispatch); + expect(window.location.pathname).toEqual(pagesAndResourcesPath); expect(store.getState().discussions).toEqual( expect.objectContaining({ appIds: ['legacy', 'piazza'], @@ -242,6 +299,8 @@ describe('Data layer integration tests', () => { }); test('successfully saves a Legacy configuration', async () => { + history.push(`/course/${courseId}/pages-and-resources/discussions/configure/legacy`); + axiosMock.onGet(getAppsUrl(courseId)).reply(200, legacyApiResponse); axiosMock.onPost(getAppsUrl(courseId), { context_key: courseId, @@ -266,19 +325,25 @@ describe('Data layer integration tests', () => { // We call fetchApps and selectApp here too just to get us into a real state. await executeThunk(fetchApps(courseId), store.dispatch); store.dispatch(selectApp({ appId: 'legacy' })); - await executeThunk(saveAppConfig(courseId, 'legacy', { - allowAnonymousPosts: true, - allowAnonymousPostsPeers: true, - blackoutDates: '[["2015-09-15","2015-09-21"],["2015-10-01","2015-10-08"]]', - // TODO: Note! As of this writing, all the data below this line is NOT returned in the API - // but we technically send it to the thunk, so here it is. - divideByCohorts: true, - allowDivisionByUnit: true, - divideCourseWideTopics: true, - divideGeneralTopic: true, - divideQuestionsForTAsTopic: true, - }), store.dispatch); + await executeThunk(saveAppConfig( + courseId, + 'legacy', + { + allowAnonymousPosts: true, + allowAnonymousPostsPeers: true, + blackoutDates: '[["2015-09-15","2015-09-21"],["2015-10-01","2015-10-08"]]', + // TODO: Note! As of this writing, all the data below this line is NOT returned in the API + // but we technically send it to the thunk, so here it is. + divideByCohorts: true, + allowDivisionByUnit: true, + divideCourseWideTopics: true, + divideGeneralTopic: true, + divideQuestionsForTAsTopic: true, + }, + pagesAndResourcesPath, + ), store.dispatch); + expect(window.location.pathname).toEqual(pagesAndResourcesPath); expect(store.getState().discussions).toEqual( expect.objectContaining({ appIds: ['legacy', 'piazza'], diff --git a/src/pages-and-resources/discussions/data/slice.js b/src/pages-and-resources/discussions/data/slice.js index 112644921..c10b4af0d 100644 --- a/src/pages-and-resources/discussions/data/slice.js +++ b/src/pages-and-resources/discussions/data/slice.js @@ -4,6 +4,7 @@ import { createSlice } from '@reduxjs/toolkit'; export const LOADING = 'LOADING'; export const LOADED = 'LOADED'; export const FAILED = 'FAILED'; +export const DENIED = 'DENIED'; export const SAVING = 'SAVING'; export const SAVED = 'SAVED'; diff --git a/src/pages-and-resources/discussions/data/thunks.js b/src/pages-and-resources/discussions/data/thunks.js index 0c26dc766..5f4248ecc 100644 --- a/src/pages-and-resources/discussions/data/thunks.js +++ b/src/pages-and-resources/discussions/data/thunks.js @@ -1,3 +1,4 @@ +import { history } from '@edx/frontend-platform'; import { addModel, addModels } from '../../../generic/model-store'; import { getApps, postAppConfig } from './api'; @@ -9,6 +10,7 @@ import { SAVING, updateStatus, updateSaveStatus, + DENIED, } from './slice'; export function fetchApps(courseId) { @@ -31,15 +33,16 @@ export function fetchApps(courseId) { featureIds: features.map(feature => feature.id), })); } catch (error) { - // TODO: We need generic error handling in the app for when a request just fails... in other - // parts of the app (proctored exam settings) we show a nice message and ask the user to - // reload/try again later. - dispatch(updateStatus({ status: FAILED })); + if (error.response && error.response.status === 403) { + dispatch(updateStatus({ status: DENIED })); + } else { + dispatch(updateStatus({ status: FAILED })); + } } }; } -export function saveAppConfig(courseId, appId, drafts) { +export function saveAppConfig(courseId, appId, drafts, successPath) { return async (dispatch) => { dispatch(updateSaveStatus({ status: SAVING })); @@ -60,11 +63,16 @@ export function saveAppConfig(courseId, appId, drafts) { featureIds: features.map(feature => feature.id), })); dispatch(updateSaveStatus({ status: SAVED })); + // Note that we redirect here to avoid having to work with the promise over in AppConfigForm. + history.push(successPath); } catch (error) { - // TODO: We need generic error handling in the app for when a request just fails... in other - // parts of the app (proctored exam settings) we show a nice message and ask the user to - // reload/try again later. - dispatch(updateSaveStatus({ status: FAILED })); + if (error.response && error.response.status === 403) { + dispatch(updateSaveStatus({ status: DENIED })); + // This second one will remove the interface as well and hide it from the user. + dispatch(updateStatus({ status: DENIED })); + } else { + dispatch(updateSaveStatus({ status: FAILED })); + } } }; } diff --git a/src/proctored-exam-settings/ProctoredExamSettings.jsx b/src/proctored-exam-settings/ProctoredExamSettings.jsx index 5725ff018..6340adf94 100644 --- a/src/proctored-exam-settings/ProctoredExamSettings.jsx +++ b/src/proctored-exam-settings/ProctoredExamSettings.jsx @@ -12,9 +12,12 @@ import { FormattedMessage, } from '@edx/frontend-platform/i18n'; +import { getConfig } from '@edx/frontend-platform'; import messages from './ProctoredExamSettings.messages'; import StudioApiService from '../data/services/StudioApiService'; import Loading from '../generic/Loading'; +import ConnectionErrorAlert from '../generic/ConnectionErrorAlert'; +import PermissionDeniedAlert from '../generic/PermissionDeniedAlert'; function ProctoredExamSettings({ courseId, intl }) { const [loading, setLoading] = useState(true); @@ -394,31 +397,13 @@ function ProctoredExamSettings({ courseId, intl }) { function renderConnectionError() { return ( - - {intl.formatMessage(messages['authoring.examsettings.support.text'])} }} - description="" - /> - + ); } function renderPermissionError() { return ( - - - + ); } @@ -461,9 +446,15 @@ function ProctoredExamSettings({ courseId, intl }) { We encountered a technical error while trying to save proctored exam settings. This might be a temporary issue, so please try again in a few minutes. If the problem persists, - please go to {support_link} for help. + please go to the {support_link} for help. `} - values={{ support_link: {intl.formatMessage(messages['authoring.examsettings.support.text'])} }} + values={{ + support_link: ( + + {intl.formatMessage(messages['authoring.examsettings.support.text'])} + + ), + }} /> ); diff --git a/src/proctored-exam-settings/ProctoredExamSettings.messages.jsx b/src/proctored-exam-settings/ProctoredExamSettings.messages.jsx index 86a7dbd7c..aa5491954 100644 --- a/src/proctored-exam-settings/ProctoredExamSettings.messages.jsx +++ b/src/proctored-exam-settings/ProctoredExamSettings.messages.jsx @@ -23,7 +23,7 @@ const messages = defineMessages({ }, 'authoring.examsettings.support.text': { id: 'authoring.examsettings.support.text', - defaultMessage: 'edX Support Page', + defaultMessage: 'Support Page', description: 'Text linking to the support page.', }, 'authoring.examsettings.enableproctoredexams.label': { diff --git a/src/proctored-exam-settings/ProctoredExamSettings.test.jsx b/src/proctored-exam-settings/ProctoredExamSettings.test.jsx index 3a5f5c62b..58b2ed6f1 100644 --- a/src/proctored-exam-settings/ProctoredExamSettings.test.jsx +++ b/src/proctored-exam-settings/ProctoredExamSettings.test.jsx @@ -491,9 +491,9 @@ describe('ProctoredExamSettings', () => { ).reply(500); await act(async () => render(intlWrapper())); - const connectionError = screen.getByTestId('connectionError'); + const connectionError = screen.getByTestId('connectionErrorAlert'); expect(connectionError.textContent).toEqual( - expect.stringContaining('We encountered a technical error'), + expect.stringContaining('We encountered a technical error when loading this page.'), ); }); @@ -503,8 +503,8 @@ describe('ProctoredExamSettings', () => { ).reply(403); await act(async () => render(intlWrapper())); - const connectionError = screen.getByTestId('permissionError'); - expect(connectionError.textContent).toEqual( + const permissionError = screen.getByTestId('permissionDeniedAlert'); + expect(permissionError.textContent).toEqual( expect.stringContaining('You are not authorized to view this page'), ); });