From b13bd6906e08616b0fe793b96af8b60d3dc5ad5c Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 6 Aug 2021 11:09:33 +0530 Subject: [PATCH] feat: restrict editing certain providers to global staff (#176) This change allows certain LTI-based providers to be disabled for editing by regular course admins/staff. In some cases, there are special configuration needs that require that the provider be configured by a global staff user. --- .env | 1 + .env.development | 1 + .env.test | 1 + src/index.jsx | 1 + .../discussions/DiscussionsSettings.test.jsx | 75 ++++++++-- .../app-config-form/AppConfigForm.jsx | 2 +- .../apps/lti/LtiConfigForm.jsx | 131 ++++++++++-------- .../app-config-form/apps/lti/messages.js | 6 +- .../apps/shared/AppExternalLinks.jsx | 6 +- .../discussion-topics/TopicItem.test.jsx | 2 +- .../discussions/app-list/AppCard.test.jsx | 2 +- .../discussions/data/api.js | 1 + .../discussions/data/redux.test.js | 2 + .../discussions/factories/mockApiResponses.js | 7 +- 14 files changed, 160 insertions(+), 78 deletions(-) diff --git a/.env b/.env index 9332343bb..d78446d67 100644 --- a/.env +++ b/.env @@ -19,5 +19,6 @@ PUBLISHER_BASE_URL= REFRESH_ACCESS_TOKEN_ENDPOINT=null SEGMENT_KEY=null SITE_NAME=null +SUPPORT_EMAIL=null SUPPORT_URL=null USER_INFO_COOKIE_NAME=null diff --git a/.env.development b/.env.development index c2e8d56b2..89165d505 100644 --- a/.env.development +++ b/.env.development @@ -21,5 +21,6 @@ REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh' SEGMENT_KEY=null SITE_NAME='edX' STUDIO_BASE_URL='http://localhost:18010' +SUPPORT_EMAIL='support@example.com' SUPPORT_URL='https://support.edx.org' USER_INFO_COOKIE_NAME='edx-user-info' diff --git a/.env.test b/.env.test index b2e446dfa..cc3e9abd2 100644 --- a/.env.test +++ b/.env.test @@ -20,5 +20,6 @@ REFRESH_ACCESS_TOKEN_ENDPOINT='http://localhost:18000/login_refresh' SEGMENT_KEY=null SITE_NAME='edX' STUDIO_BASE_URL='http://localhost:18010' +SUPPORT_EMAIL='support@example.com' SUPPORT_URL='https://support.edx.org' USER_INFO_COOKIE_NAME='edx-user-info' diff --git a/src/index.jsx b/src/index.jsx index 574bb9be8..ef1a3e297 100755 --- a/src/index.jsx +++ b/src/index.jsx @@ -45,6 +45,7 @@ initialize({ config: () => { mergeConfig({ SUPPORT_URL: process.env.SUPPORT_URL || null, + SUPPORT_EMAIL: process.env.SUPPORT_EMAIL || null, CALCULATOR_HELP_URL: process.env.CALCULATOR_HELP_URL || null, }, 'CourseAuthoringConfig'); }, diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx index 6dffbfb54..0c137d02f 100644 --- a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx +++ b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx @@ -1,32 +1,31 @@ -import React from 'react'; - +import { + getConfig, history, initializeMockApp, setConfig, +} from '@edx/frontend-platform'; +import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; +import { AppProvider, PageRoute } from '@edx/frontend-platform/react'; import { act, queryByLabelText, + queryByRole, queryByTestId, queryByText, render, screen, - waitForElementToBeRemoved, - queryByRole, waitFor, + waitForElementToBeRemoved, } 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 { - 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'; -import PagesAndResourcesProvider from '../PagesAndResourcesProvider'; +import React from 'react'; +import { Switch } from 'react-router'; import initializeStore from '../../store'; -import { getAppsUrl } from './data/api'; -import { piazzaApiResponse, legacyApiResponse } from './factories/mockApiResponses'; +import PagesAndResourcesProvider from '../PagesAndResourcesProvider'; import appMessages from './app-config-form/messages'; import appListMessages from './app-list/messages'; +import ltiMessages from './app-config-form/apps/lti/messages'; +import { getAppsUrl } from './data/api'; +import DiscussionsSettings from './DiscussionsSettings'; +import { generatePiazzaApiResponse, legacyApiResponse, piazzaApiResponse } from './factories/mockApiResponses'; const courseId = 'course-v1:edX+TestX+Test_Course'; let axiosMock; @@ -307,3 +306,49 @@ describe('DiscussionsSettings', () => { }); }); }); + +describe.each([ + { isAdmin: false, isAdminOnlyConfig: false }, + { isAdmin: false, isAdminOnlyConfig: true }, + { isAdmin: true, isAdminOnlyConfig: false }, + { isAdmin: true, isAdminOnlyConfig: true }, +])('LTI Admin only config test', ({ isAdmin, isAdminOnlyConfig }) => { + beforeEach(() => { + initializeMockApp({ + authenticatedUser: { + userId: 3, + username: 'abc123', + administrator: isAdmin, + roles: [], + }, + }); + + store = initializeStore(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + + // Leave the DiscussionsSettings route after the test. + history.push(`/course/${courseId}/pages-and-resources`); + axiosMock.onGet(getAppsUrl(courseId)).reply(200, generatePiazzaApiResponse(isAdminOnlyConfig)); + renderComponent(); + }); + + test(`successfully advances to settings step for lti when adminOnlyConfig=${isAdminOnlyConfig} and user ${isAdmin ? 'is' : 'is not'} admin`, async () => { + const showLTIConfig = isAdmin || !isAdminOnlyConfig; + history.push(`/course/${courseId}/pages-and-resources/discussion`); + + // 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, appListMessages.nextButton.defaultMessage)); + + if (showLTIConfig) { + expect(queryByText(container, ltiMessages.formInstructions.defaultMessage)).toBeInTheDocument(); + expect(queryByTestId(container, 'ltiConfigFields')).toBeInTheDocument(); + } else { + expect(queryByText(container, ltiMessages.formInstructions.defaultMessage)).not.toBeInTheDocument(); + expect(queryByTestId(container, 'ltiConfigFields')).not.toBeInTheDocument(); + } + }); +}); 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 bd1333d6e..fb51c03e1 100644 --- a/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx +++ b/src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx @@ -84,7 +84,7 @@ function AppConfigForm({ app={app} appConfig={appConfig} onSubmit={handleSubmit} - title={intl.formatMessage(messages[`appName-${app.id}`])} + providerName={intl.formatMessage(messages[`appName-${app.id}`])} /> ); } diff --git a/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx b/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx index 0334b5a54..c8849036a 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx @@ -1,23 +1,26 @@ import React, { useEffect } from 'react'; import PropTypes from 'prop-types'; -import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; -import { - Card, Form, -} from '@edx/paragon'; +import { ensureConfig, getConfig } from '@edx/frontend-platform'; +import { getAuthenticatedUser } from '@edx/frontend-platform/auth'; +import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n'; +import { Card, Form, MailtoLink } from '@edx/paragon'; import { useFormik } from 'formik'; -import * as Yup from 'yup'; import { useDispatch } from 'react-redux'; +import * as Yup from 'yup'; -import { - updateValidationStatus, -} from '../../../data/slice'; +import { updateValidationStatus } from '../../../data/slice'; import AppExternalLinks from '../shared/AppExternalLinks'; import messages from './messages'; +ensureConfig([ + 'SITE_NAME', + 'SUPPORT_EMAIL', +], 'LTI Config Form'); + function LtiConfigForm({ - appConfig, app, onSubmit, intl, formRef, title, + appConfig, app, onSubmit, intl, formRef, providerName, }) { const ltiAppConfig = { consumerKey: appConfig.consumerKey || '', @@ -26,7 +29,7 @@ function LtiConfigForm({ piiShareUsername: appConfig.piiShareUsername, piiShareEmail: appConfig.piiShareEmail, }; - + const user = getAuthenticatedUser(); const dispatch = useDispatch(); const { externalLinks } = app; const { @@ -47,10 +50,11 @@ function LtiConfigForm({ }), onSubmit, }); - const isInvalidConsumerKey = Boolean(touched.consumerKey && errors.consumerKey); const isInvalidConsumerSecret = Boolean(touched.consumerSecret && errors.consumerSecret); const isInvalidLaunchUrl = Boolean(touched.launchUrl && errors.launchUrl); + const supportEmail = getConfig().SUPPORT_EMAIL; + const showLTIConfig = user.administrator || !app.adminOnlyConfig; useEffect(() => { dispatch(updateValidationStatus({ hasError: Object.keys(errors).length > 0 })); @@ -59,50 +63,68 @@ function LtiConfigForm({ return (
-

{title}

-

{intl.formatMessage(messages.formInstructions)}

+

{providerName}

+

{showLTIConfig + ? intl.formatMessage(messages.formInstructions) + : ( + {supportEmail} + : 'support', + }} + /> + )} +

{app.messages && app.messages.map(msg => (

{msg}

))} - - - {isInvalidConsumerKey && ( - - {errors.consumerKey} - - )} - - - - {isInvalidConsumerSecret && ( - - {errors.consumerSecret} - - )} - - - - {isInvalidLaunchUrl && ( - - {errors.launchUrl} - - )} - + {showLTIConfig && ( + <> + + + {isInvalidConsumerKey && ( + + {errors.consumerKey} + + )} + + + + {isInvalidConsumerSecret && ( + + {errors.consumerSecret} + + )} + + + + {isInvalidLaunchUrl && ( + + {errors.launchUrl} + + )} + + + )} {appConfig.piiSharing && ( <> @@ -129,7 +151,7 @@ function LtiConfigForm({ )} - +
); } @@ -137,6 +159,7 @@ function LtiConfigForm({ LtiConfigForm.propTypes = { app: PropTypes.shape({ id: PropTypes.string.isRequired, + adminOnlyConfig: PropTypes.bool, externalLinks: PropTypes.shape({ learnMore: PropTypes.string, configuration: PropTypes.string, @@ -158,7 +181,7 @@ LtiConfigForm.propTypes = { onSubmit: PropTypes.func.isRequired, // eslint-disable-next-line react/forbid-prop-types formRef: PropTypes.object.isRequired, - title: PropTypes.string.isRequired, + providerName: PropTypes.string.isRequired, }; LtiConfigForm.defaultProps = { diff --git a/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js b/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js index a48fc18f5..a977f287a 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js +++ b/src/pages-and-resources/discussions/app-config-form/apps/lti/messages.js @@ -39,6 +39,10 @@ const messages = defineMessages({ defaultMessage: 'Launch URL is a required field', description: 'Tells the user that the Launch URL field is required and must have a value.', }, + adminOnlyConfig: { + id: 'authoring.discussions.adminOnlyConfig', + defaultMessage: '{providerName} can only be configured by {platformName} administrators. Please contact {supportEmail} to enable this feature.', + }, piiSharing: { id: 'authoring.discussions.piiSharing', defaultMessage: 'Optionally share a user\'s username and/or email with the LTI provider:', @@ -75,7 +79,7 @@ const messages = defineMessages({ }, learnMore: { id: 'authoring.discussions.appDocInstructions.learnMoreLink', - defaultMessage: 'Learn more about {title}', + defaultMessage: 'Learn more about {providerName}', description: 'Application Document Instructions message for learn more links', }, linkTextHeading: { diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/AppExternalLinks.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/AppExternalLinks.jsx index 5af5784e9..2074a8420 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/AppExternalLinks.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/AppExternalLinks.jsx @@ -12,7 +12,7 @@ import messages from '../lti/messages'; function AppExternalLinks({ externalLinks, intl, - title, + providerName, }) { const { contactEmail, ...links } = externalLinks; const linkTypes = Object.keys(links).filter(key => links[key]); @@ -31,7 +31,7 @@ function AppExternalLinks({ rel="noopener noreferrer" showLaunchIcon={false} > - { intl.formatMessage(messages[type], { title }) } + { intl.formatMessage(messages[type], { providerName }) } ))} @@ -67,7 +67,7 @@ AppExternalLinks.propTypes = { accessibility: PropTypes.string, contactEmail: PropTypes.string, }).isRequired, - title: PropTypes.string.isRequired, + providerName: PropTypes.string.isRequired, intl: intlShape.isRequired, }; diff --git a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx index ba943c8c6..87101d220 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/shared/discussion-topics/TopicItem.test.jsx @@ -136,7 +136,7 @@ describe('TopicItem', () => { expect(topicCard.querySelector('input')).toBeInTheDocument(); }); - test('renders delete topic popup with title, label, helping text, a delete and a cancel button', async () => { + test('renders delete topic popup with providerName, label, helping text, a delete and a cancel button', async () => { await mockStore(legacyApiResponse); createComponent(additionalTopic); diff --git a/src/pages-and-resources/discussions/app-list/AppCard.test.jsx b/src/pages-and-resources/discussions/app-list/AppCard.test.jsx index 4f6f7c796..b807634ff 100644 --- a/src/pages-and-resources/discussions/app-list/AppCard.test.jsx +++ b/src/pages-and-resources/discussions/app-list/AppCard.test.jsx @@ -42,7 +42,7 @@ describe('AppCard', () => { test.each([ [true], [false], - ])('title and text from the app are displayed with full support %s', (hasFullSupport) => { + ])('providerName and text from the app are displayed with full support %s', (hasFullSupport) => { const appWithCustomSupport = { ...app, hasFullSupport }; const title = messages[`appName-${appWithCustomSupport.id}`].defaultMessage; const text = messages[`appDescription-${appWithCustomSupport.id}`].defaultMessage; diff --git a/src/pages-and-resources/discussions/data/api.js b/src/pages-and-resources/discussions/data/api.js index feb20e6c3..751cae126 100644 --- a/src/pages-and-resources/discussions/data/api.js +++ b/src/pages-and-resources/discussions/data/api.js @@ -65,6 +65,7 @@ function normalizeApps(data) { contactEmail: app.external_links.contact_email, }, hasFullSupport: app.has_full_support, + adminOnlyConfig: !!app.admin_only_config, })); return { courseId: data.context_key, diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js index 960d470d8..3185af22b 100644 --- a/src/pages-and-resources/discussions/data/redux.test.js +++ b/src/pages-and-resources/discussions/data/redux.test.js @@ -52,10 +52,12 @@ const legacyApp = { }, hasFullSupport: true, messages: [], + adminOnlyConfig: false, }; const piazzaApp = { id: 'piazza', + adminOnlyConfig: false, featureIds: [ 'discussion-page', 'embedded-course-sections', diff --git a/src/pages-and-resources/discussions/factories/mockApiResponses.js b/src/pages-and-resources/discussions/factories/mockApiResponses.js index 0bd3a9c09..ac65adda4 100644 --- a/src/pages-and-resources/discussions/factories/mockApiResponses.js +++ b/src/pages-and-resources/discussions/factories/mockApiResponses.js @@ -1,4 +1,4 @@ -export const piazzaApiResponse = { +export const generatePiazzaApiResponse = (piazzaAdminOnlyConfig = false) => ({ context_key: 'course-v1:edX+DemoX+Demo_Course', enabled: true, provider_type: 'piazza', @@ -51,10 +51,11 @@ export const piazzaApiResponse = { }, messages: [], has_full_support: false, + admin_only_config: piazzaAdminOnlyConfig, }, }, }, -}; +}); export const legacyApiResponse = { context_key: 'course-v1:edX+DemoX+Demo_Course', @@ -140,3 +141,5 @@ export const emptyAppApiResponse = { }, }, }; + +export const piazzaApiResponse = generatePiazzaApiResponse(false);