From dbad0f6aac54805c97cf33db3e1b14e6861e6f7f Mon Sep 17 00:00:00 2001 From: David Joy Date: Thu, 22 Apr 2021 07:49:08 -0400 Subject: [PATCH] feat: hookup discussions config to POST APIs (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: PageCard should use the pages and resources path from context This prevents some odd behaviors around changing the URL based on the current path. * fix: use the correct formRef for AppConfigForm children Straight up refactoring bug. This should be using the formRef from the context, but was still defining its own and passing it down. This ultimately meant that the submit button wasn’t properly hooked up to the form, since the form was given a different ref than the submit button. * fix: Only validate fields that have been touched * fix: use the title instead of app.name app.name doesn’t exist - but the title is the string we’re looking for. Use it. * refactor: default appConfig appropriately for both config forms In subsequent PRs we’re going to start passing null appConfigs to forms sometimes - this uses prop types to set default values for the forms if the component isn’t passed a meaningful appConfig. * refactor: replace and flesh out discussions data layer Since we only have one GET endpoint for all the data around discussions, we don’t need two separate slices/thunks/api files. The API needs to be hit once when the discussions settings load. This means the app-list and app-config-form share far more state than originally thought. The AppList and AppConfigForm are no longer responsible for data loading - we’ve moved that back up into DiscussionsSettings. Now they just read from the redux state and load what they need. The main change is moving app-list/data/slice.js up to discussions/data/slice.js, renaming the reducer, and adding a saveStatus to it - turns out this is all we need. The original two slices go away. The discussions/data/api file gets a proper implementation of postAppConfig which mostly works with the server as of this writing - we have some data shape issues to figure out. AppConfigForm needed a little help. It now checks whether our data is loaded and selects an app based on its route. This allows us to deep link in and keep the selectedAppId correct. * Adding a loading spinner to AppList This is only tangentally related to the current PR, admittedly. * test: adding some title tests to LegacyConfigForm.test.jsx Needed to add the title prop, then decided I’d test it. * test: adding a test suite for discussions/data files This test exercises the thunks, slice, and api parts of the data layer all at once in ‘real’ scenarios with mocked API requests. --- src/generic/Loading.jsx | 25 ++ .../discussions/DiscussionsSettings.jsx | 21 +- .../app-config-form/AppConfigForm.jsx | 51 ++- .../AppConfigFormApplyButton.jsx | 6 +- .../apps/legacy/LegacyConfigForm.jsx | 15 +- .../apps/legacy/LegacyConfigForm.test.jsx | 17 + .../apps/lti/LtiConfigForm.jsx | 31 +- .../discussions/app-config-form/data/api.js | 143 ------- .../discussions/app-config-form/data/slice.js | 39 -- .../app-config-form/data/thunks.js | 60 --- .../discussions/app-config-form/index.js | 1 - .../discussions/app-list/AppList.jsx | 29 +- .../app-list/AppListNextButton.jsx | 2 +- .../discussions/app-list/data/api.js | 34 -- .../discussions/app-list/data/thunks.js | 29 -- .../discussions/app-list/index.js | 1 - .../discussions/data/api.js | 130 ++++++ .../discussions/data/reducer.js | 11 - .../discussions/data/redux.test.js | 380 ++++++++++++++++++ .../discussions/{app-list => }/data/slice.js | 17 +- .../discussions/data/thunks.js | 70 ++++ src/pages-and-resources/discussions/index.js | 2 +- src/pages-and-resources/pages/PageCard.jsx | 8 +- .../ProctoredExamSettings.jsx | 19 +- 24 files changed, 738 insertions(+), 403 deletions(-) create mode 100644 src/generic/Loading.jsx delete mode 100644 src/pages-and-resources/discussions/app-config-form/data/api.js delete mode 100644 src/pages-and-resources/discussions/app-config-form/data/slice.js delete mode 100644 src/pages-and-resources/discussions/app-config-form/data/thunks.js delete mode 100644 src/pages-and-resources/discussions/app-list/data/api.js delete mode 100644 src/pages-and-resources/discussions/app-list/data/thunks.js create mode 100644 src/pages-and-resources/discussions/data/api.js delete mode 100644 src/pages-and-resources/discussions/data/reducer.js create mode 100644 src/pages-and-resources/discussions/data/redux.test.js rename src/pages-and-resources/discussions/{app-list => }/data/slice.js (75%) create mode 100644 src/pages-and-resources/discussions/data/thunks.js diff --git a/src/generic/Loading.jsx b/src/generic/Loading.jsx new file mode 100644 index 000000000..2e12d2b18 --- /dev/null +++ b/src/generic/Loading.jsx @@ -0,0 +1,25 @@ +import React from 'react'; +import { Spinner } from '@edx/paragon'; +import { FormattedMessage } from '@edx/frontend-platform/i18n'; + +export default function Loading() { + return ( +
+ + + + + +
+ ); +} diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.jsx index e2c11c9a6..aa51f5d09 100644 --- a/src/pages-and-resources/discussions/DiscussionsSettings.jsx +++ b/src/pages-and-resources/discussions/DiscussionsSettings.jsx @@ -1,5 +1,5 @@ import React, { - useCallback, useContext, + useCallback, useContext, useEffect, } from 'react'; import PropTypes from 'prop-types'; import { @@ -7,6 +7,7 @@ import { useLocation, useRouteMatch, } from 'react-router'; +import { useDispatch } from 'react-redux'; import { history } from '@edx/frontend-platform'; import { PageRoute } from '@edx/frontend-platform/react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; @@ -16,18 +17,24 @@ import { Check } from '@edx/paragon/icons'; import FullScreenModal from '../../generic/full-screen-modal'; import Stepper from '../../generic/stepper'; import { PagesAndResourcesContext } from '../PagesAndResourcesProvider'; -import AppList from './app-list'; -import AppConfigForm from './app-config-form'; + import messages from './messages'; import DiscussionsProvider from './DiscussionsProvider'; +import { fetchApps } from './data/thunks'; +import AppList from './app-list'; +import AppConfigForm from './app-config-form'; function DiscussionsSettings({ courseId, intl }) { + const dispatch = useDispatch(); const { path } = useRouteMatch(); const { pathname } = useLocation(); - const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext); - const discussionsPath = `${pagesAndResourcesPath}/discussions`; + useEffect(() => { + dispatch(fetchApps(courseId)); + }, [courseId]); + + const discussionsPath = `${pagesAndResourcesPath}/discussions`; const isFirstStep = pathname === discussionsPath; const steps = [{ @@ -65,9 +72,7 @@ function DiscussionsSettings({ courseId, intl }) { - + { - dispatch(fetchAppConfig(courseId, routeAppId)); - }, [courseId]); + const { formRef } = useContext(AppConfigFormContext); + const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext); + const { params: { appId: routeAppId } } = useRouteMatch(); + const { selectedAppId, status } = 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., + // if we're configuring a new app. + const appConfig = useModel('appConfigs', selectedAppId); - const { appId, appConfigId } = useSelector(state => state.discussions.appConfigForm); + useEffect(() => { + if (status === LOADED) { + if (routeAppId !== selectedAppId) { + dispatch(selectApp({ appId: routeAppId })); + } + } + }, [selectedAppId, routeAppId, status]); // This is a callback that gets called after the form has been submitted successfully. const handleSubmit = useCallback((values) => { - dispatch(saveAppConfig(courseId, appId, values)).then(() => { + dispatch(saveAppConfig(courseId, selectedAppId, values)).then(() => { history.push(pagesAndResourcesPath); }); - }, [courseId, appId, courseId]); + }, [courseId, selectedAppId, courseId]); - const app = useModel('apps', appId); - const appConfig = useModel('appConfigs', appConfigId); - - if (!appConfig || !app) { + if (!selectedAppId || status !== LOADED) { return null; } let form = null; - if (app.id === 'legacy') { form = ( state.discussions.appConfigForm.status); + const saveStatus = useSelector(state => state.discussions.saveStatus); const { formRef } = useContext(AppConfigFormContext); - const submitButtonState = status === SAVING ? 'pending' : 'default'; + const submitButtonState = saveStatus === SAVING ? 'pending' : 'default'; // This causes the form to be submitted from a button outside the form. const handleApply = useCallback(() => { diff --git a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx index c9911cf93..b75df9a44 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.jsx @@ -70,7 +70,7 @@ LegacyConfigForm.propTypes = { allowAnonymousPosts: PropTypes.bool.isRequired, allowAnonymousPostsPeers: PropTypes.bool.isRequired, blackoutDates: PropTypes.string.isRequired, - }).isRequired, + }), onSubmit: PropTypes.func.isRequired, // eslint-disable-next-line react/forbid-prop-types formRef: PropTypes.object.isRequired, @@ -78,4 +78,17 @@ LegacyConfigForm.propTypes = { title: PropTypes.string.isRequired, }; +LegacyConfigForm.defaultProps = { + appConfig: { + divideByCohorts: false, + allowDivisionByUnit: false, + divideCourseWideTopics: false, + divideGeneralTopic: false, + divideQuestionsForTAs: false, + allowAnonymousPosts: false, + allowAnonymousPostsPeers: false, + blackoutDates: '[]', + }, +}; + export default injectIntl(LegacyConfigForm); diff --git a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx index f0fea3a8b..79d61bcc8 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx @@ -16,6 +16,20 @@ const defaultAppConfig = { }; describe('LegacyConfigForm', () => { + test('title rendering', () => { + const { container } = render( + + + , + ); + + expect(container.querySelector('h3')).toHaveTextContent('Test Legacy edX Discussions'); + }); test('calls onSubmit when the formRef is submitted', async () => { const formRef = createRef(); const handleSubmit = jest.fn(); @@ -23,6 +37,7 @@ describe('LegacyConfigForm', () => { render( { const { container } = render( { const { container } = render(
@@ -44,47 +49,46 @@ function LtiConfigForm({ target="_blank" rel="noopener noreferrer" > - {intl.formatMessage(messages.documentationPage, { name: app.name })} + {intl.formatMessage(messages.documentationPage, { name: title })} ), - name: app.name, }} />

- + {intl.formatMessage(messages.consumerKey)} - {errors.consumerKey && ( + {isInvalidConsumerKey && ( {errors.consumerKey} )} - + {intl.formatMessage(messages.consumerSecret)} - {errors.consumerSecret && ( + {isInvalidConsumerSecret && ( {errors.consumerSecret} )} - + {intl.formatMessage(messages.launchUrl)} - {errors.launchUrl && ( + {isInvalidLaunchUrl && ( {errors.launchUrl} @@ -98,14 +102,13 @@ function LtiConfigForm({ LtiConfigForm.propTypes = { app: PropTypes.shape({ id: PropTypes.string.isRequired, - name: PropTypes.string.isRequired, documentationUrl: PropTypes.string.isRequired, }).isRequired, appConfig: PropTypes.shape({ consumerKey: PropTypes.string.isRequired, consumerSecret: PropTypes.string.isRequired, launchUrl: PropTypes.string.isRequired, - }).isRequired, + }), intl: intlShape.isRequired, onSubmit: PropTypes.func.isRequired, // eslint-disable-next-line react/forbid-prop-types @@ -113,4 +116,12 @@ LtiConfigForm.propTypes = { title: PropTypes.string.isRequired, }; +LtiConfigForm.defaultProps = { + appConfig: { + consumerKey: '', + consumerSecret: '', + launchUrl: '', + }, +}; + export default injectIntl(LtiConfigForm); diff --git a/src/pages-and-resources/discussions/app-config-form/data/api.js b/src/pages-and-resources/discussions/app-config-form/data/api.js deleted file mode 100644 index 37c42c297..000000000 --- a/src/pages-and-resources/discussions/app-config-form/data/api.js +++ /dev/null @@ -1,143 +0,0 @@ -const legacyEdXDiscussions = { - id: 'legacy', - hasFullSupport: false, - featureIds: [ - 'discussion-page', - 'embedded-course-sections', - 'wcag-2.1', - ], -}; - -const piazzaApp = { - id: 'piazza', - hasFullSupport: false, - featureIds: [ - 'lti', - 'discussion-page', - 'embedded-course-sections', - 'wcag-2.1', - ], -}; - -const yellowdigApp = { - id: 'yellowdig', - hasFullSupport: false, - featureIds: [ - 'lti', - 'discussion-page', - 'embedded-course-sections', - 'wcag-2.1', - ], -}; - -export function getAppConfig(courseId, appConfigId, appId) { - let app = null; - switch (appId) { - case 'piazza': - app = piazzaApp; - break; - case 'yellowdig': - app = yellowdigApp; - break; - default: - app = legacyEdXDiscussions; - } - - let appConfig = { - id: 'appConfig1', - consumerSecret: 'its-a-secret-to-everybody', - consumerKey: 'abc123', - launchUrl: 'https://localhost/launch', - }; - - if (appId === 'legacy') { - appConfig = { - id: 'legacy', - divideByCohorts: false, - allowDivisionByUnit: false, - divideCourseWideTopics: false, - divideGeneralTopic: false, - divideQuestionsForTAs: false, - inContextDiscussion: false, - gradedUnitPages: false, - groupInContextSubsection: false, - allowUnitLevelVisibility: false, - allowAnonymousPosts: false, - allowAnonymousPostsPeers: false, - blackoutDates: '[]', - }; - } - - return Promise.resolve({ - app, - appConfig, - features: [ - { - id: 'lti', - }, - { - id: 'discussion-page', - }, - { - id: 'embedded-course-sections', - }, - { - id: 'embedded-course-units', - }, - { - id: 'wcag-2.1', - }, - ], - }); -} - -export function postAppConfig(courseId, appId, drafts) { - let app = null; - switch (appId) { - case 'piazza': - app = piazzaApp; - break; - case 'yellowdig': - app = yellowdigApp; - break; - default: - app = legacyEdXDiscussions; - } - return new Promise((resolve) => { - setTimeout(() => { - resolve({ - app, - appConfig: { - id: 'appConfig1', - consumerSecret: 'its-a-secret-to-everybody', - consumerKey: 'abc123', - launchUrl: 'https://localhost/launch', - documentationUrl: 'https://localhost/docs', - ...drafts, - }, - features: [ - { - id: 'lti', - name: 'LTI Integration', - }, - { - id: 'discussion-page', - name: 'Discussion Page', - }, - { - id: 'embedded-course-sections', - name: 'Embedded Course Sections', - }, - { - id: 'embedded-course-units', - name: 'Embedded Course Units', - }, - { - id: 'wcag-2.1', - name: 'WCAG 2.1 Support', - }, - ], - }); - }, 1000); - }); -} diff --git a/src/pages-and-resources/discussions/app-config-form/data/slice.js b/src/pages-and-resources/discussions/app-config-form/data/slice.js deleted file mode 100644 index b8871afd7..000000000 --- a/src/pages-and-resources/discussions/app-config-form/data/slice.js +++ /dev/null @@ -1,39 +0,0 @@ -/* eslint-disable no-param-reassign */ -import { createSlice } from '@reduxjs/toolkit'; - -export const LOADING = 'LOADING'; -export const LOADED = 'LOADED'; -export const FAILED = 'FAILED'; -export const SAVING = 'SAVING'; -export const SAVED = 'SAVED'; -export const DIRTY = 'DIRTY'; - -const slice = createSlice({ - name: 'appConfigForm', - initialState: { - // app config state - appId: null, - appConfigId: null, - appConfigStatus: LOADING, - }, - reducers: { - loadAppConfig: (state, { payload }) => { - state.appId = payload.appId; - state.appConfigId = payload.appConfigId; - state.featureIds = payload.featureIds; - state.status = LOADED; - }, - updateStatus: (state, { status }) => { - state.status = status; - }, - }, -}); - -export const { - loadAppConfig, - updateStatus, -} = slice.actions; - -export const { - reducer, -} = slice; diff --git a/src/pages-and-resources/discussions/app-config-form/data/thunks.js b/src/pages-and-resources/discussions/app-config-form/data/thunks.js deleted file mode 100644 index ac317de8a..000000000 --- a/src/pages-and-resources/discussions/app-config-form/data/thunks.js +++ /dev/null @@ -1,60 +0,0 @@ -import { addModel, addModels } from '../../../../generic/model-store'; -import { - getAppConfig, - postAppConfig, -} from './api'; -import { - FAILED, - LOADING, - updateStatus, - SAVING, - SAVED, - loadAppConfig, -} from './slice'; - -export function fetchAppConfig(courseId, appId, appConfigId) { - return async (dispatch) => { - dispatch(updateStatus({ status: LOADING })); - try { - const { app, appConfig, features } = await getAppConfig(courseId, appConfigId, appId); - - dispatch(addModel({ modelType: 'apps', model: app })); - dispatch(addModels({ modelType: 'features', models: features })); - dispatch(addModel({ modelType: 'appConfigs', model: appConfig })); - - dispatch(loadAppConfig({ - appId: app.id, - appConfigId: appConfig.id, - 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 })); - } - }; -} - -export function saveAppConfig(courseId, appId, drafts) { - return async (dispatch) => { - dispatch(updateStatus({ status: SAVING })); - - try { - const { app, appConfig, features } = await postAppConfig(courseId, appId, drafts); - - dispatch(addModel({ modelType: 'apps', model: app })); - dispatch(addModels({ modelType: 'features', models: features })); - dispatch(addModel({ modelType: 'appConfigs', model: appConfig })); - - dispatch(loadAppConfig({ - activeAppId: app.id, - activeAppConfigId: appConfig.id, - featureIds: features.map(feature => feature.id), - })); - dispatch(updateStatus({ status: SAVED })); - } catch (error) { - dispatch(updateStatus({ status: FAILED })); - } - }; -} diff --git a/src/pages-and-resources/discussions/app-config-form/index.js b/src/pages-and-resources/discussions/app-config-form/index.js index de8b7964e..971895f30 100644 --- a/src/pages-and-resources/discussions/app-config-form/index.js +++ b/src/pages-and-resources/discussions/app-config-form/index.js @@ -1,2 +1 @@ export { default } from './AppConfigForm'; -export { reducer } from './data/slice'; diff --git a/src/pages-and-resources/discussions/app-list/AppList.jsx b/src/pages-and-resources/discussions/app-list/AppList.jsx index a55e6a1f3..18471b5c1 100644 --- a/src/pages-and-resources/discussions/app-list/AppList.jsx +++ b/src/pages-and-resources/discussions/app-list/AppList.jsx @@ -1,28 +1,23 @@ -import React, { useCallback, useEffect } from 'react'; -import PropTypes from 'prop-types'; +import React, { useCallback } from 'react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { CardGrid, Container } from '@edx/paragon'; import { useDispatch, useSelector } from 'react-redux'; import { useModels } from '../../../generic/model-store'; +import { selectApp, LOADED, LOADING } from '../data/slice'; import AppCard from './AppCard'; import messages from './messages'; import FeaturesTable from './FeaturesTable'; import AppListNextButton from './AppListNextButton'; +import Loading from '../../../generic/Loading'; -import { fetchApps } from './data/thunks'; -import { selectApp, LOADED } from './data/slice'; - -function AppList({ courseId, intl }) { +function AppList({ intl }) { const dispatch = useDispatch(); - useEffect(() => { - dispatch(fetchApps(courseId)); - }, [courseId]); const { - appIds, featureIds, status, selectedAppId, - } = useSelector(state => state.discussions.appList); + appIds, featureIds, status, activeAppId, selectedAppId, + } = useSelector(state => state.discussions); const apps = useModels('apps', appIds); const features = useModels('features', featureIds); @@ -30,6 +25,15 @@ function AppList({ courseId, intl }) { dispatch(selectApp({ appId })); }, [selectedAppId]); + // If selectedAppId is not set, use activeAppId + const finalSelectedAppId = selectedAppId || activeAppId; + + if (status === LOADING) { + return ( + + ); + } + if (status === LOADED && apps.length === 0) { return ( @@ -54,7 +58,7 @@ function AppList({ courseId, intl }) { ))} @@ -73,7 +77,6 @@ function AppList({ courseId, intl }) { } AppList.propTypes = { - courseId: PropTypes.string.isRequired, intl: intlShape.isRequired, }; diff --git a/src/pages-and-resources/discussions/app-list/AppListNextButton.jsx b/src/pages-and-resources/discussions/app-list/AppListNextButton.jsx index 0a98e1b69..c10434839 100644 --- a/src/pages-and-resources/discussions/app-list/AppListNextButton.jsx +++ b/src/pages-and-resources/discussions/app-list/AppListNextButton.jsx @@ -9,7 +9,7 @@ import { DiscussionsContext } from '../DiscussionsProvider'; import messages from './messages'; function AppListNextButton({ intl }) { - const { selectedAppId } = useSelector(state => state.discussions.appList); + const { selectedAppId } = useSelector(state => state.discussions); const { path: discussionsPath } = useContext(DiscussionsContext); const handleStartConfig = useCallback(() => { diff --git a/src/pages-and-resources/discussions/app-list/data/api.js b/src/pages-and-resources/discussions/app-list/data/api.js deleted file mode 100644 index 435fcbd69..000000000 --- a/src/pages-and-resources/discussions/app-list/data/api.js +++ /dev/null @@ -1,34 +0,0 @@ -import { getConfig } from '@edx/frontend-platform'; -import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; - -function normalizeApps(data) { - const apps = Object.entries(data.providers.available).map(([key, app]) => ({ - id: key, - featureIds: app.features, - hasFullSupport: app.features.length >= data.features.length, - })); - return { - courseId: data.context_key, - enabled: data.enabled, - features: data.features.map(id => ({ - id, - })), - appConfig: data.plugin_configuration, - // TODO: Defaulting to "legacy" should come from the backend. For now this makes sense as we - // don't create a DiscussionsConfiguration object in the backend for legacy discussions, which - // is configured by default on all courses. The endpoint doesn't know this and returns a null - // for `active`, which we can then interpret as "the user hasn't configured something else" and - // choose to fall back to legacy. In the long run, we should be able to trust `active` to give - // us 'legacy' if the legacy experience is being used. - activeAppId: data.providers.active || 'legacy', - apps, - }; -} - -// eslint-disable-next-line import/prefer-default-export -export async function getApps(courseId) { - const { data } = await getAuthenticatedHttpClient() - .get(`${getConfig().LMS_BASE_URL}/discussions/api/v0/${courseId}`); - - return normalizeApps(data); -} diff --git a/src/pages-and-resources/discussions/app-list/data/thunks.js b/src/pages-and-resources/discussions/app-list/data/thunks.js deleted file mode 100644 index a55958a1c..000000000 --- a/src/pages-and-resources/discussions/app-list/data/thunks.js +++ /dev/null @@ -1,29 +0,0 @@ -import { addModels } from '../../../../generic/model-store'; - -import { getApps } from './api'; -import { - updateStatus, LOADING, FAILED, loadApps, -} from './slice'; - -/* eslint-disable import/prefer-default-export */ -export function fetchApps(courseId) { - return async (dispatch) => { - dispatch(updateStatus({ status: LOADING })); - try { - const { apps, features, activeAppId } = await getApps(courseId); - - dispatch(addModels({ modelType: 'apps', models: apps })); - dispatch(addModels({ modelType: 'features', models: features })); - dispatch(loadApps({ - activeAppId, - appIds: apps.map(app => app.id), - 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 })); - } - }; -} diff --git a/src/pages-and-resources/discussions/app-list/index.js b/src/pages-and-resources/discussions/app-list/index.js index 1038107ed..a303ce01c 100644 --- a/src/pages-and-resources/discussions/app-list/index.js +++ b/src/pages-and-resources/discussions/app-list/index.js @@ -1,2 +1 @@ export { default } from './AppList'; -export { reducer } from './data/slice'; diff --git a/src/pages-and-resources/discussions/data/api.js b/src/pages-and-resources/discussions/data/api.js new file mode 100644 index 000000000..27fa255e8 --- /dev/null +++ b/src/pages-and-resources/discussions/data/api.js @@ -0,0 +1,130 @@ +import { getConfig } from '@edx/frontend-platform'; +import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; + +function normalizeLtiConfig(data) { + if (!data || Object.keys(data).length < 1) { + return {}; + } + + return { + consumerKey: data.lti_1p1_client_key, + consumerSecret: data.lti_1p1_client_secret, + launchUrl: data.lti_1p1_launch_url, + }; +} + +function normalizePluginConfig(data) { + if (!data || Object.keys(data).length < 1) { + return {}; + } + + return { + allowAnonymousPosts: data.allow_anonymous, + allowAnonymousPostsPeers: data.allow_anonymous_to_peers, + blackoutDates: JSON.stringify(data.discussion_blackouts), + // TODO: We need all these added to the API. ... default them for now + divideByCohorts: false, + allowDivisionByUnit: false, + divideCourseWideTopics: false, + // TODO: Note that these last two are in the `discussion_topics` data, but we haven't been able + // to add them properly here yet. I'm not sure the data is in a usable state as is, since it + // only seems to include the topics for which these would be "true". Assuming its only these + // two, I think we could check for the existence of "General" for the first, but I'm not sure + // what the topic title is for the second. "Questions for TAs" maybe? + divideGeneralTopic: false, + divideQuestionsForTAs: false, + }; +} + +function normalizeApps(data) { + const apps = Object.entries(data.providers.available).map(([key, app]) => ({ + id: key, + featureIds: app.features, + // TODO: Fix this and get it from the backend! + documentationUrl: 'http://example.com', + hasFullSupport: app.features.length >= data.features.length, + })); + return { + courseId: data.context_key, + enabled: data.enabled, + features: data.features.map(id => ({ + id, + })), + appConfig: { + id: data.providers.active, + ...normalizePluginConfig(data.plugin_configuration), + ...normalizeLtiConfig(data.lti_configuration), + }, + activeAppId: data.providers.active, + apps, + }; +} + +function denormalizeData(courseId, appId, data) { + /* + TODO: What about these? Some are from the instructor dashboard, presumably... but I don't think they all are. + + divideByCohorts + allowDivisionByUnit + divideCourseWideTopics + divideGeneralTopic + divideQuestionsForTAs + */ + const pluginConfiguration = {}; + + if (data.allowAnonymousPosts) { + pluginConfiguration.allow_anonymous = data.allowAnonymousPosts; + } + if (data.allowAnonymousPostsPeers) { + pluginConfiguration.allow_anonymous_to_peers = data.allowAnonymousPostsPeers; + } + if (data.blackoutDates) { + pluginConfiguration.discussion_blackouts = JSON.parse(data.blackoutDates); + } + + const ltiConfiguration = {}; + + if (data.consumerKey) { + ltiConfiguration.lti_1p1_client_key = data.consumerKey; + } + if (data.consumerSecret) { + ltiConfiguration.lti_1p1_client_secret = data.consumerSecret; + } + if (data.launchUrl) { + ltiConfiguration.lti_1p1_launch_url = data.launchUrl; + } + + if (Object.keys(ltiConfiguration).length > 0) { + // Only add this in if we're sending LTI fields. + // TODO: Eventually support LTI v1.3 here. + ltiConfiguration.version = 'lti_1p1'; + } + + return { + context_key: courseId, + enabled: true, + lti_configuration: ltiConfiguration, + plugin_configuration: pluginConfiguration, + provider_type: appId, + }; +} + +export function getAppsUrl(courseId) { + return `${getConfig().LMS_BASE_URL}/discussions/api/v0/${courseId}`; +} + +export async function getApps(courseId) { + const { data } = await getAuthenticatedHttpClient() + .get(getAppsUrl(courseId)); + + return normalizeApps(data); +} + +export async function postAppConfig(courseId, appId, values) { + const { data } = await getAuthenticatedHttpClient().post( + getAppsUrl(courseId), + denormalizeData(courseId, appId, values), + ); + + return normalizeApps(data); +} diff --git a/src/pages-and-resources/discussions/data/reducer.js b/src/pages-and-resources/discussions/data/reducer.js deleted file mode 100644 index 64ae7b9c3..000000000 --- a/src/pages-and-resources/discussions/data/reducer.js +++ /dev/null @@ -1,11 +0,0 @@ -import { combineReducers } from 'redux'; - -import { reducer as appListReducer } from '../app-list'; -import { reducer as appConfigFormReducer } from '../app-config-form'; - -const reducer = combineReducers({ - appList: appListReducer, - appConfigForm: appConfigFormReducer, -}); - -export default reducer; diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js new file mode 100644 index 000000000..83b95034c --- /dev/null +++ b/src/pages-and-resources/discussions/data/redux.test.js @@ -0,0 +1,380 @@ +import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; +import MockAdapter from 'axios-mock-adapter'; +import { initializeMockApp } from '@edx/frontend-platform/testing'; +import initializeStore from '../../../store'; +import { getAppsUrl } from './api'; +import { FAILED, SAVED, selectApp } from './slice'; +import { fetchApps, saveAppConfig } from './thunks'; +import { LOADED } from '../../../data/slice'; + +let axiosMock; +let store; + +// Helper, that is used to forcibly finalize all promises +// in thunk before running matcher against state. +const executeThunk = async (thunk, dispatch, getState) => { + await thunk(dispatch, getState); + await new Promise(setImmediate); +}; + +const courseId = 'course-v1:edX+TestX+Test_Course'; + +const piazzaApiResponse = { + context_key: 'course-v1:edX+DemoX+Demo_Course', + enabled: true, + provider_type: 'piazza', + features: [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + 'lti', + ], + lti_configuration: { + lti_1p1_client_key: 'client_key_123', + lti_1p1_client_secret: 'client_secret_123', + lti_1p1_launch_url: 'https://localhost/example', + version: 'lti_1p1', + }, + plugin_configuration: {}, + providers: { + active: 'piazza', + available: { + legacy: { + features: [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + ], + }, + piazza: { + features: [ + 'discussion-page', + 'lti', + ], + }, + }, + }, +}; + +const legacyApiResponse = { + context_key: 'course-v1:edX+DemoX+Demo_Course', + enabled: true, + provider_type: 'legacy', + features: [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + 'lti', + ], + lti_configuration: {}, + plugin_configuration: { + allow_anonymous: false, + allow_anonymous_to_peers: false, + // Note, this gets stringified when normalized into the app, but the API returns it as an + // actual array. Argh. + discussion_blackouts: [], + }, + providers: { + active: 'legacy', + available: { + legacy: { + features: [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + ], + }, + piazza: { + features: [ + 'discussion-page', + 'lti', + ], + }, + }, + }, +}; + +const featuresState = { + 'discussion-page': { + id: 'discussion-page', + }, + 'embedded-course-sections': { + id: 'embedded-course-sections', + }, + 'wcag-2.1': { + id: 'wcag-2.1', + }, + lti: { + id: 'lti', + }, +}; + +const featureIds = [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + 'lti', +]; + +const legacyApp = { + id: 'legacy', + featureIds: [ + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + ], + documentationUrl: 'http://example.com', + hasFullSupport: false, +}; + +const piazzaApp = { + id: 'piazza', + featureIds: [ + 'discussion-page', + 'lti', + ], + documentationUrl: 'http://example.com', + hasFullSupport: false, +}; + +describe('Data layer integration tests', () => { + beforeEach(() => { + initializeMockApp({ + authenticatedUser: { + userId: 3, + username: 'abc123', + administrator: true, + roles: [], + }, + }); + + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + + store = initializeStore(); + }); + + afterEach(() => { + axiosMock.reset(); + }); + + describe('fetchApps', () => { + test('network error', async () => { + axiosMock.onGet(getAppsUrl(courseId)).networkError(); + + await executeThunk(fetchApps(courseId), store.dispatch); + + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: [], + featureIds: [], + activeAppId: null, + selectedAppId: null, + status: FAILED, + saveStatus: SAVED, + }), + ); + }); + + test('successfully loads an LTI configuration', async () => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + + await executeThunk(fetchApps(courseId), store.dispatch); + + expect(store.getState().discussions).toEqual({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'piazza', + selectedAppId: null, + status: LOADED, + saveStatus: SAVED, + }); + expect(store.getState().models.apps.legacy).toEqual(legacyApp); + expect(store.getState().models.apps.piazza).toEqual(piazzaApp); + expect(store.getState().models.features).toEqual(featuresState); + expect(store.getState().models.appConfigs.piazza).toEqual({ + id: 'piazza', + consumerKey: 'client_key_123', + consumerSecret: 'client_secret_123', + launchUrl: 'https://localhost/example', + }); + }); + + test('successfully loads a Legacy configuration', async () => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, legacyApiResponse); + + await executeThunk(fetchApps(courseId), store.dispatch); + + expect(store.getState().discussions).toEqual({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'legacy', + selectedAppId: null, + status: LOADED, + saveStatus: SAVED, + }); + expect(store.getState().models.apps.legacy).toEqual(legacyApp); + expect(store.getState().models.apps.piazza).toEqual(piazzaApp); + expect(store.getState().models.features).toEqual(featuresState); + expect(store.getState().models.appConfigs.legacy).toEqual({ + id: 'legacy', + allowAnonymousPosts: false, + allowAnonymousPostsPeers: false, + blackoutDates: '[]', + // TODO: Note! As of this writing, all the data below this line is NOT returned in the API + // but we add it in during normalization. + divideByCohorts: false, + allowDivisionByUnit: false, + divideCourseWideTopics: false, + divideGeneralTopic: false, + divideQuestionsForTAs: false, + }); + }); + }); + + describe('selectApp', () => { + test('sets selectedAppId', () => { + const appId = 'piazza'; + store.dispatch(selectApp({ appId })); + + expect(store.getState().discussions.selectedAppId).toEqual(appId); + }); + }); + + describe('saveAppConfig', () => { + test('network error', async () => { + 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); + + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'piazza', + selectedAppId: 'piazza', + status: LOADED, + saveStatus: FAILED, + }), + ); + }); + + test('successfully saves an LTI configuration', async () => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse); + axiosMock.onPost(getAppsUrl(courseId), { + context_key: courseId, + enabled: true, + lti_configuration: { + lti_1p1_client_key: 'new_consumer_key', + lti_1p1_client_secret: 'new_consumer_secret', + lti_1p1_launch_url: 'http://localhost/new_launch_url', + version: 'lti_1p1', + }, + plugin_configuration: {}, + provider_type: 'piazza', + }).reply(200, { + ...piazzaApiResponse, // This uses the existing configuration but with a replacement + // lti_configuration that matches what we tried to save. + lti_configuration: { + lti_1p1_client_key: 'new_consumer_key', + lti_1p1_client_secret: 'new_consumer_secret', + lti_1p1_launch_url: 'https://localhost/new_launch_url', + version: 'lti_1p1', + }, + }); + + // 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); + + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'piazza', + selectedAppId: 'piazza', + status: LOADED, + saveStatus: SAVED, + }), + ); + expect(store.getState().models.appConfigs.piazza).toEqual({ + id: 'piazza', + consumerKey: 'new_consumer_key', + consumerSecret: 'new_consumer_secret', + launchUrl: 'https://localhost/new_launch_url', + }); + }); + + test('successfully saves a Legacy configuration', async () => { + axiosMock.onGet(getAppsUrl(courseId)).reply(200, legacyApiResponse); + axiosMock.onPost(getAppsUrl(courseId), { + context_key: courseId, + enabled: true, + lti_configuration: {}, + plugin_configuration: { + allow_anonymous: true, + allow_anonymous_to_peers: true, + discussion_blackouts: [['2015-09-15', '2015-09-21'], ['2015-10-01', '2015-10-08']], + }, + provider_type: 'legacy', + }).reply(200, { + ...legacyApiResponse, // This uses the existing configuration but with a replacement + // plugin_configuration that matches what we tried to save. + plugin_configuration: { + allow_anonymous: true, + allow_anonymous_to_peers: true, + discussion_blackouts: [['2015-09-15', '2015-09-21'], ['2015-10-01', '2015-10-08']], + }, + }); + + // 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, + divideQuestionsForTAs: true, + }), store.dispatch); + + expect(store.getState().discussions).toEqual( + expect.objectContaining({ + appIds: ['legacy', 'piazza'], + featureIds, + activeAppId: 'legacy', + selectedAppId: 'legacy', + status: LOADED, + saveStatus: SAVED, + }), + ); + expect(store.getState().models.appConfigs.legacy).toEqual({ + id: 'legacy', + // These three fields should be updated. + allowAnonymousPosts: true, + allowAnonymousPostsPeers: true, + blackoutDates: '[["2015-09-15","2015-09-21"],["2015-10-01","2015-10-08"]]', + // TODO: Note! The values we tried to save were ignored, this test reflects what currently + // happens, but NOT what we want to have happen! + divideByCohorts: false, + allowDivisionByUnit: false, + divideCourseWideTopics: false, + divideGeneralTopic: false, + divideQuestionsForTAs: false, + }); + }); + }); +}); diff --git a/src/pages-and-resources/discussions/app-list/data/slice.js b/src/pages-and-resources/discussions/data/slice.js similarity index 75% rename from src/pages-and-resources/discussions/app-list/data/slice.js rename to src/pages-and-resources/discussions/data/slice.js index f0c588911..112644921 100644 --- a/src/pages-and-resources/discussions/app-list/data/slice.js +++ b/src/pages-and-resources/discussions/data/slice.js @@ -4,9 +4,11 @@ import { createSlice } from '@reduxjs/toolkit'; export const LOADING = 'LOADING'; export const LOADED = 'LOADED'; export const FAILED = 'FAILED'; +export const SAVING = 'SAVING'; +export const SAVED = 'SAVED'; const slice = createSlice({ - name: 'appList', + name: 'discussions', initialState: { appIds: [], featureIds: [], @@ -17,24 +19,28 @@ const slice = createSlice({ // instead. selectedAppId: null, status: LOADING, + saveStatus: SAVED, }, reducers: { loadApps: (state, { payload }) => { state.activeAppId = payload.activeAppId; - // When the UI loads, we want to set the selectedAppId to the activeAppId. This ensures the - // active one will be checked. - state.selectedAppId = payload.activeAppId; state.appIds = payload.appIds; state.featureIds = payload.featureIds; state.status = LOADED; + state.saveStatus = SAVED; }, selectApp: (state, { payload }) => { const { appId } = payload; state.selectedAppId = appId; }, - updateStatus: (state, { status }) => { + updateStatus: (state, { payload }) => { + const { status } = payload; state.status = status; }, + updateSaveStatus: (state, { payload }) => { + const { status } = payload; + state.saveStatus = status; + }, }, }); @@ -42,6 +48,7 @@ export const { loadApps, selectApp, updateStatus, + updateSaveStatus, } = slice.actions; export const { diff --git a/src/pages-and-resources/discussions/data/thunks.js b/src/pages-and-resources/discussions/data/thunks.js new file mode 100644 index 000000000..0c26dc766 --- /dev/null +++ b/src/pages-and-resources/discussions/data/thunks.js @@ -0,0 +1,70 @@ +import { addModel, addModels } from '../../../generic/model-store'; + +import { getApps, postAppConfig } from './api'; +import { + FAILED, + loadApps, + LOADING, + SAVED, + SAVING, + updateStatus, + updateSaveStatus, +} from './slice'; + +export function fetchApps(courseId) { + return async (dispatch) => { + dispatch(updateStatus({ status: LOADING })); + try { + const { + apps, + features, + activeAppId, + appConfig, + } = await getApps(courseId); + + dispatch(addModels({ modelType: 'apps', models: apps })); + dispatch(addModels({ modelType: 'features', models: features })); + dispatch(addModel({ modelType: 'appConfigs', model: appConfig })); + dispatch(loadApps({ + activeAppId, + appIds: apps.map(app => app.id), + 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 })); + } + }; +} + +export function saveAppConfig(courseId, appId, drafts) { + return async (dispatch) => { + dispatch(updateSaveStatus({ status: SAVING })); + + try { + const { + apps, + features, + activeAppId, + appConfig, + } = await postAppConfig(courseId, appId, drafts); + + dispatch(addModels({ modelType: 'apps', models: apps })); + dispatch(addModels({ modelType: 'features', models: features })); + dispatch(addModel({ modelType: 'appConfigs', model: appConfig })); + dispatch(loadApps({ + activeAppId, + appIds: apps.map(app => app.id), + featureIds: features.map(feature => feature.id), + })); + dispatch(updateSaveStatus({ status: SAVED })); + } 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 })); + } + }; +} diff --git a/src/pages-and-resources/discussions/index.js b/src/pages-and-resources/discussions/index.js index 58b407589..aac60e6db 100644 --- a/src/pages-and-resources/discussions/index.js +++ b/src/pages-and-resources/discussions/index.js @@ -1,2 +1,2 @@ export { default } from './DiscussionsSettings'; -export { default as reducer } from './data/reducer'; +export { reducer } from './data/slice'; diff --git a/src/pages-and-resources/pages/PageCard.jsx b/src/pages-and-resources/pages/PageCard.jsx index 68f8525ea..4c5120d35 100644 --- a/src/pages-and-resources/pages/PageCard.jsx +++ b/src/pages-and-resources/pages/PageCard.jsx @@ -1,14 +1,14 @@ import PropTypes from 'prop-types'; -import React from 'react'; +import React, { useContext } from 'react'; import classNames from 'classnames'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { Button } from '@edx/paragon'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; import { faCog } from '@fortawesome/free-solid-svg-icons'; -import { useLocation } from 'react-router'; import { history } from '@edx/frontend-platform'; import messages from '../messages'; +import { PagesAndResourcesContext } from '../PagesAndResourcesProvider'; const CoursePageShape = PropTypes.shape({ id: PropTypes.string.isRequired, @@ -23,7 +23,7 @@ const CoursePageShape = PropTypes.shape({ export { CoursePageShape }; function PageCard({ intl, page }) { - const { pathname } = useLocation(); + const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext); const pageStatusMsgId = page.isEnabled ? 'pageStatus.enabled' : 'pageStatus.disabled'; const componentClasses = classNames( @@ -33,7 +33,7 @@ function PageCard({ intl, page }) { ); const handleClick = () => { - history.push(`${pathname}/${page.id}`); + history.push(`${pagesAndResourcesPath}/${page.id}`); }; return ( diff --git a/src/proctored-exam-settings/ProctoredExamSettings.jsx b/src/proctored-exam-settings/ProctoredExamSettings.jsx index b60750587..c4ed093ee 100644 --- a/src/proctored-exam-settings/ProctoredExamSettings.jsx +++ b/src/proctored-exam-settings/ProctoredExamSettings.jsx @@ -14,6 +14,7 @@ import { import messages from './ProctoredExamSettings.messages'; import StudioApiService from '../data/services/StudioApiService'; +import Loading from '../generic/Loading'; function ProctoredExamSettings({ courseId, intl }) { const [loading, setLoading] = useState(true); @@ -387,23 +388,7 @@ function ProctoredExamSettings({ courseId, intl }) { function renderLoading() { return ( -
- - - - - -
+ ); }