Skip to content

feat: Add support for the new Open edX discussion provider [BD-38] [TNL-8622] #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xitij2000 merged 1 commit into from
Feb 22, 2022
Merged

feat: Add support for the new Open edX discussion provider [BD-38] [TNL-8622] #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/pages-and-resources/discussions/DiscussionsSettings.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { PagesAndResourcesContext } from '../PagesAndResourcesProvider';

import messages from './messages';
import DiscussionsProvider from './DiscussionsProvider';
import { fetchApps } from './data/thunks';
import { fetchProviders } from './data/thunks';
import AppList from './app-list';
import AppConfigForm from './app-config-form';
import { DENIED, FAILED } from './data/slice';
Expand All @@ -36,7 +36,7 @@ function DiscussionsSettings({ courseId, intl }) {
const courseDetail = useModel('courseDetails', courseId);

useEffect(() => {
dispatch(fetchApps(courseId));
dispatch(fetchProviders(courseId));
}, [courseId]);

const discussionsPath = `${pagesAndResourcesPath}/discussion`;
Expand Down
73 changes: 49 additions & 24 deletions src/pages-and-resources/discussions/DiscussionsSettings.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
import { AppProvider, PageRoute } from '@edx/frontend-platform/react';
import {
act,
findByRole,
getByRole,
queryByLabelText,
queryByRole,
Expand All @@ -19,21 +20,22 @@ import userEvent from '@testing-library/user-event';
import MockAdapter from 'axios-mock-adapter';
import React from 'react';
import { Switch } from 'react-router';
import { fetchCourseDetail } from '../../data/thunks';
import initializeStore from '../../store';
import { executeThunk } from '../../utils';
import PagesAndResourcesProvider from '../PagesAndResourcesProvider';
import ltiMessages from './app-config-form/apps/lti/messages';
import appMessages from './app-config-form/messages';
import messages from './app-list/messages';
import ltiMessages from './app-config-form/apps/lti/messages';
import { getAppsUrl } from './data/api';
import { getDiscussionsProvidersUrl, getDiscussionsSettingsUrl } from './data/api';
import DiscussionsSettings from './DiscussionsSettings';
import {
courseDetailResponse,
generatePiazzaApiResponse,
generateProvidersApiResponse,
legacyApiResponse,
piazzaApiResponse,
courseDetailResponse,
} from './factories/mockApiResponses';
import { executeThunk } from '../../utils';
import { fetchCourseDetail } from '../../data/thunks';

const courseId = 'course-v1:edX+TestX+Test_Course';
let axiosMock;
Expand Down Expand Up @@ -88,7 +90,10 @@ describe('DiscussionsSettings', () => {

describe('with successful network connections', () => {
beforeEach(() => {
axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse);
axiosMock.onGet(getDiscussionsProvidersUrl(courseId))
.reply(200, generateProvidersApiResponse(false));
axiosMock.onGet(getDiscussionsSettingsUrl(courseId))
.reply(200, piazzaApiResponse);
renderComponent();
});

Expand Down Expand Up @@ -124,24 +129,29 @@ describe('DiscussionsSettings', () => {
userEvent.click(queryByLabelText(container, 'Select Piazza'));
userEvent.click(queryByText(container, messages.nextButton.defaultMessage));

await waitForElementToBeRemoved(screen.getByRole('status'));

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 () => {
axiosMock.onGet(getAppsUrl(courseId)).reply(200, legacyApiResponse);
axiosMock.onGet(getDiscussionsProvidersUrl(courseId)).reply(200, generateProvidersApiResponse(false, 'legacy'));
axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).reply(200, legacyApiResponse);
renderComponent();
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 edX'));
userEvent.click(queryByLabelText(container, 'Select edX (Legacy)'));
userEvent.click(queryByText(container, messages.nextButton.defaultMessage));

await waitForElementToBeRemoved(screen.getByRole('status'));

expect(queryByTestId(container, 'appList')).not.toBeInTheDocument();
expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument();
expect(queryByTestId(container, 'ltiConfigForm')).not.toBeInTheDocument();
Expand Down Expand Up @@ -183,7 +193,7 @@ describe('DiscussionsSettings', () => {
test('successfully submit the modal', async () => {
history.push(`/course/${courseId}/pages-and-resources/discussion`);

axiosMock.onPost(getAppsUrl(courseId)).reply(200, piazzaApiResponse);
axiosMock.onPost(getDiscussionsSettingsUrl(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.
Expand All @@ -193,7 +203,7 @@ describe('DiscussionsSettings', () => {

userEvent.click(getByRole(container, 'button', { name: 'Next' }));

userEvent.click(getByRole(container, 'button', { name: 'Save' }));
userEvent.click(await findByRole(container, 'button', { name: 'Save' }));

// 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,
Expand All @@ -215,6 +225,7 @@ describe('DiscussionsSettings', () => {
userEvent.click(getByRole(container, 'checkbox', { name: 'Select Discourse' }));
userEvent.click(getByRole(container, 'button', { name: 'Next' }));

await findByRole(container, 'button', { name: 'Save' });
userEvent.type(getByRole(container, 'textbox', { name: 'Consumer Key' }), 'key');
userEvent.type(getByRole(container, 'textbox', { name: 'Consumer Secret' }), 'secret');
userEvent.type(getByRole(container, 'textbox', { name: 'Launch URL' }), 'http://example.test');
Expand All @@ -237,6 +248,7 @@ describe('DiscussionsSettings', () => {
userEvent.click(discourseBox);

userEvent.click(getByRole(container, 'button', { name: 'Next' }));
await waitForElementToBeRemoved(screen.getByRole('status'));
expect(getByRole(container, 'heading', { name: 'Discourse' })).toBeInTheDocument();

userEvent.type(getByRole(container, 'textbox', { name: 'Consumer Key' }), 'a');
Expand All @@ -252,16 +264,16 @@ describe('DiscussionsSettings', () => {
});
});

describe('with network error fetchApps API requests', () => {
describe('with network error fetchProviders API requests', () => {
beforeEach(() => {
// Expedient way of getting SUPPORT_URL into config.
setConfig({
...getConfig(),
SUPPORT_URL: 'http://support.edx.org',
});

axiosMock.onGet(getAppsUrl(courseId)).networkError();

axiosMock.onGet(getDiscussionsProvidersUrl(courseId)).networkError();
axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).networkError();
renderComponent();
});

Expand All @@ -287,8 +299,11 @@ describe('DiscussionsSettings', () => {
SUPPORT_URL: 'http://support.edx.org',
});

axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse);
axiosMock.onPost(getAppsUrl(courseId)).networkError();
axiosMock.onGet(getDiscussionsProvidersUrl(courseId))
.reply(200, generateProvidersApiResponse());
axiosMock.onGet(getDiscussionsSettingsUrl(courseId))
.reply(200, piazzaApiResponse);
axiosMock.onPost(getDiscussionsSettingsUrl(courseId)).networkError();
renderComponent();
});

Expand All @@ -307,17 +322,17 @@ describe('DiscussionsSettings', () => {
await waitFor(() => expect(axiosMock.history.post.length).toBe(1));

expect(queryByTestId(container, 'appConfigForm')).toBeInTheDocument();

const alert = queryByRole(container, 'alert');
const alert = await findByRole(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', () => {
describe('with permission denied error for fetchProviders API requests', () => {
beforeEach(() => {
axiosMock.onGet(getAppsUrl(courseId)).reply(403);
axiosMock.onGet(getDiscussionsProvidersUrl(courseId)).reply(403);
axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).reply(403);

renderComponent();
});
Expand All @@ -337,8 +352,10 @@ describe('DiscussionsSettings', () => {

describe('with permission denied error for postAppConfig API requests', () => {
beforeEach(() => {
axiosMock.onGet(getAppsUrl(courseId)).reply(200, piazzaApiResponse);
axiosMock.onPost(getAppsUrl(courseId)).reply(403);
axiosMock.onGet(getDiscussionsProvidersUrl(courseId))
.reply(200, generateProvidersApiResponse());
axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).reply(200, piazzaApiResponse);
axiosMock.onPost(getDiscussionsSettingsUrl(courseId)).reply(403);

renderComponent();
});
Expand All @@ -360,7 +377,7 @@ describe('DiscussionsSettings', () => {
// 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/discussion/configure/piazza`);

const alert = queryByRole(container, 'alert');
const alert = await findByRole(container, 'alert');
expect(alert).toBeInTheDocument();
expect(alert.textContent).toEqual(expect.stringContaining('You are not authorized to view this page.'));
});
Expand Down Expand Up @@ -394,7 +411,10 @@ describe.each([

// Leave the DiscussionsSettings route after the test.
history.push(`/course/${courseId}/pages-and-resources`);
axiosMock.onGet(getAppsUrl(courseId)).reply(200, generatePiazzaApiResponse(isAdminOnlyConfig));
axiosMock.onGet(getDiscussionsProvidersUrl(courseId))
.reply(200, generateProvidersApiResponse(isAdminOnlyConfig));
axiosMock.onGet(getDiscussionsSettingsUrl(courseId))
.reply(200, generatePiazzaApiResponse());
renderComponent();
});

Expand All @@ -408,6 +428,7 @@ describe.each([

userEvent.click(queryByLabelText(container, 'Select Piazza'));
userEvent.click(queryByText(container, messages.nextButton.defaultMessage));
await waitForElementToBeRemoved(screen.getByRole('status'));

if (showLTIConfig) {
expect(queryByText(container, ltiMessages.formInstructions.defaultMessage)).toBeInTheDocument();
Expand Down Expand Up @@ -446,7 +467,10 @@ describe.each([

// Leave the DiscussionsSettings route after the test.
history.push(`/course/${courseId}/pages-and-resources`);
axiosMock.onGet(getAppsUrl(courseId)).reply(200, generatePiazzaApiResponse(false, piiSharingAllowed));
axiosMock.onGet(getDiscussionsProvidersUrl(courseId))
.reply(200, generateProvidersApiResponse(false));
axiosMock.onGet(getDiscussionsSettingsUrl(courseId))
.reply(200, generatePiazzaApiResponse(piiSharingAllowed));
renderComponent();
});

Expand All @@ -460,6 +484,7 @@ describe.each([

userEvent.click(queryByLabelText(container, 'Select Piazza'));
userEvent.click(queryByText(container, messages.nextButton.defaultMessage));
await waitForElementToBeRemoved(screen.getByRole('status'));
if (enablePIISharing) {
expect(queryByTestId(container, 'piiSharingFields')).toBeInTheDocument();
} else {
Expand Down
31 changes: 24 additions & 7 deletions src/pages-and-resources/discussions/app-config-form/AppConfigForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import {
DENIED,
FAILED, LOADED, LOADING, selectApp,
} from '../data/slice';
import { saveAppConfig } from '../data/thunks';
import LegacyConfigForm from './apps/legacy';
import { fetchDiscussionSettings, saveProviderConfig } from '../data/thunks';
import OpenedXConfigForm from './apps/openedx';
import LtiConfigForm from './apps/lti';
import AppConfigFormProvider, { AppConfigFormContext } from './AppConfigFormProvider';
import AppConfigFormSaveButton from './AppConfigFormSaveButton';
Expand All @@ -36,12 +36,20 @@ function AppConfigForm({
const { formRef } = useContext(AppConfigFormContext);
const { path: pagesAndResourcesPath } = useContext(PagesAndResourcesContext);
const { params: { appId: routeAppId } } = useRouteMatch();
const [isLoading, setLoading] = useState(true);
const {
activeAppId, selectedAppId, status, saveStatus,
} = useSelector(state => state.discussions);

const [confirmationDialogVisible, setConfirmationDialogVisible] = useState(false);

useEffect(() => {
(async () => {
await dispatch(fetchDiscussionSettings(courseId, selectedAppId));
setLoading(false);
})();
}, [courseId, selectedAppId]);

useEffect(() => {
if (status === LOADED) {
if (routeAppId !== selectedAppId) {
Expand All @@ -57,12 +65,12 @@ function AppConfigForm({
setConfirmationDialogVisible(true);
} else {
setConfirmationDialogVisible(false);
// Note that when this action succeeds, we redirect to pagesAndResurcesPath in the thunk.
dispatch(saveAppConfig(courseId, selectedAppId, values, pagesAndResourcesPath));
// Note that when this action succeeds, we redirect to pagesAndResourcesPath in the thunk.
dispatch(saveProviderConfig(courseId, selectedAppId, values, pagesAndResourcesPath));
}
}, [activeAppId, confirmationDialogVisible, courseId, selectedAppId]);

if (!selectedAppId || status === LOADING) {
if (!selectedAppId || status === LOADING || isLoading) {
return (
<Loading />
);
Expand All @@ -78,12 +86,21 @@ function AppConfigForm({
alert = <PermissionDeniedAlert />;
}

let form = null;
let form;
if (selectedAppId === 'legacy') {
form = (
<LegacyConfigForm
<OpenedXConfigForm
formRef={formRef}
onSubmit={handleSubmit}
legacy
/>
);
} else if (selectedAppId === 'openedx') {
form = (
<OpenedXConfigForm
formRef={formRef}
onSubmit={handleSubmit}
legacy={false}
/>
);
} else {
Expand Down
1 change: 0 additions & 1 deletion src/pages-and-resources/discussions/app-config-form/apps/legacy/index.js
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions src/pages-and-resources/discussions/app-config-form/apps/lti/LtiConfigForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ ensureConfig(['SITE_NAME', 'SUPPORT_EMAIL'], 'LTI Config Form');
function LtiConfigForm({ onSubmit, intl, formRef }) {
const dispatch = useDispatch();

const { selectedAppId } = useSelector((state) => state.discussions);

const piiConfig = useModel('appConfigs', 'pii');
const { selectedAppId, piiConfig } = useSelector((state) => state.discussions);
const appConfig = useModel('appConfigs', selectedAppId);
const app = useModel('apps', selectedAppId);
const providerName = intl.formatMessage(appMessages[`appName-${app?.id}`]);
Expand Down
Loading