From 2eaf8827342ed346bd815a65584ca0fe1eb1f99f Mon Sep 17 00:00:00 2001 From: ayesha waris <73840786+ayesha-waris@users.noreply.github.com> Date: Tue, 25 Apr 2023 15:00:35 +0500 Subject: [PATCH] fix: only global staff can see 2 edx discussion providers in settings (#477) * fix: only global staff can see 2 edx discussion providers in settings * test: adds and updated test cases for app list * refactor: memoized showoneedxprovider constant --------- Co-authored-by: ayesha waris <73840786+ayeshoali@users.noreply.github.com> --- .../discussions/DiscussionsSettings.test.jsx | 13 +- .../discussions/app-list/AppList.jsx | 30 ++-- .../discussions/app-list/AppList.test.jsx | 162 ++++++++++-------- .../discussions/data/redux.test.js | 14 +- .../discussions/factories/mockApiResponses.js | 18 ++ 5 files changed, 139 insertions(+), 98 deletions(-) diff --git a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx index 3941362fd..73af4b9e0 100644 --- a/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx +++ b/src/pages-and-resources/discussions/DiscussionsSettings.test.jsx @@ -5,17 +5,8 @@ import { import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import { AppProvider, PageRoute } from '@edx/frontend-platform/react'; import { - act, - findByRole, - getByRole, - queryByLabelText, - queryByRole, - queryByTestId, - queryByText, - render, - screen, - waitFor, - waitForElementToBeRemoved, + act, findByRole, getByRole, queryByLabelText, queryByRole, queryByTestId, queryByText, render, + screen, waitFor, waitForElementToBeRemoved, } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import MockAdapter from 'axios-mock-adapter'; diff --git a/src/pages-and-resources/discussions/app-list/AppList.jsx b/src/pages-and-resources/discussions/app-list/AppList.jsx index 3358c9120..fecda28c7 100644 --- a/src/pages-and-resources/discussions/app-list/AppList.jsx +++ b/src/pages-and-resources/discussions/app-list/AppList.jsx @@ -1,8 +1,9 @@ -import React, { useCallback, useEffect } from 'react'; +import React, { useCallback, useEffect, useMemo } from 'react'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { CardGrid, Container, breakpoints } from '@edx/paragon'; import { useDispatch, useSelector } from 'react-redux'; import Responsive from 'react-responsive'; +import { getAuthenticatedUser } from '@edx/frontend-platform/auth'; import { useModels } from '../../../generic/model-store'; import { selectApp, LOADED, LOADING, @@ -16,12 +17,17 @@ import Loading from '../../../generic/Loading'; const AppList = ({ intl }) => { const dispatch = useDispatch(); - const { appIds, featureIds, status, activeAppId, selectedAppId, } = useSelector(state => state.discussions); const apps = useModels('apps', appIds); const features = useModels('features', featureIds); + const isGlobalStaff = getAuthenticatedUser().administrator; + const ltiProvider = !['openedx', 'legacy'].includes(activeAppId); + + const showOneEdxProvider = useMemo(() => apps.filter(app => ( + activeAppId === 'openedx' ? app.id !== 'legacy' : app.id !== 'openedx' + )), [activeAppId]); // This could be a bit confusing. activeAppId is the ID of the app that is currently configured // according to the server. selectedAppId is the ID of the app that we _want_ to configure here @@ -54,6 +60,16 @@ const AppList = ({ intl }) => { ); } + const showAppCard = (filteredApps) => filteredApps.map(app => ( + + )); + return (

@@ -67,15 +83,7 @@ const AppList = ({ intl }) => { xl: 4, }} > - {apps.map(app => ( - - ))} + {(isGlobalStaff || ltiProvider) ? showAppCard(apps) : showAppCard(showOneEdxProvider)}

diff --git a/src/pages-and-resources/discussions/app-list/AppList.test.jsx b/src/pages-and-resources/discussions/app-list/AppList.test.jsx index bc5625235..f2ad10072 100644 --- a/src/pages-and-resources/discussions/app-list/AppList.test.jsx +++ b/src/pages-and-resources/discussions/app-list/AppList.test.jsx @@ -1,14 +1,13 @@ /* eslint-disable react/jsx-no-constructed-context-values */ import React from 'react'; - +import { + render, screen, within, queryAllByRole, +} from '@testing-library/react'; import { initializeMockApp } from '@edx/frontend-platform'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { AppProvider } from '@edx/frontend-platform/react'; import { breakpoints } from '@edx/paragon'; -import { - queryByText, render, queryAllByRole, queryByRole, getByRole, queryByLabelText, getByLabelText, queryAllByText, -} from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import MockAdapter from 'axios-mock-adapter'; import { Context as ResponsiveContext } from 'react-responsive'; @@ -20,84 +19,109 @@ import { fetchDiscussionSettings, fetchProviders } from '../data/thunks'; import { generateProvidersApiResponse, piazzaApiResponse, + legacyApiResponse, } from '../factories/mockApiResponses'; import AppList from './AppList'; import messages from './messages'; const courseId = 'course-v1:edX+TestX+Test_Course'; +let axiosMock; +let store; +let container; + +const mockStore = async (mockResponse, provider) => { + axiosMock.onGet(getDiscussionsProvidersUrl(courseId)) + .reply(200, generateProvidersApiResponse(false, provider)); + axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).reply(200, mockResponse); + await executeThunk(fetchProviders(courseId), store.dispatch); + await executeThunk(fetchDiscussionSettings(courseId), store.dispatch); +}; + +function renderComponent(screenWidth = breakpoints.extraLarge.minWidth) { + const wrapper = render( + + + + + + + , + ); + container = wrapper.container; +} describe('AppList', () => { - let axiosMock; - let store; - let container; + describe('AppList for Admin role', () => { + beforeEach(async () => { + initializeMockApp({ + authenticatedUser: { + userId: 3, + username: 'abc123', + administrator: true, + roles: [], + }, + }); - function createComponent(screenWidth = breakpoints.extraLarge.minWidth) { - return ( - - - - - - - - ); - } - - beforeEach(async () => { - initializeMockApp({ - authenticatedUser: { - userId: 3, - username: 'abc123', - administrator: true, - roles: [], - }, + store = await initializeStore(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + await mockStore(piazzaApiResponse); }); - store = await initializeStore(); - axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + test('display a card for each available app', async () => { + renderComponent(); + const appCount = store.getState().discussions.appIds.length; + expect(screen.queryAllByRole('radio')).toHaveLength(appCount); + }); + + test('displays the FeaturesTable at desktop sizes', async () => { + renderComponent(); + expect(screen.queryByRole('table')).toBeInTheDocument(); + }); + + test('hides the FeaturesTable at mobile sizes', async () => { + renderComponent(breakpoints.extraSmall.maxWidth); + expect(screen.queryByRole('table')).not.toBeInTheDocument(); + }); + + test('hides the FeaturesList at desktop sizes', async () => { + renderComponent(); + expect(screen.queryByText(messages['supportedFeatureList-mobile-show'].defaultMessage)).not.toBeInTheDocument(); + }); + + test('displays the FeaturesList at mobile sizes', async () => { + renderComponent(breakpoints.extraSmall.maxWidth); + const appCount = store.getState().discussions.appIds.length; + expect(screen.queryAllByText(messages['supportedFeatureList-mobile-show'].defaultMessage)).toHaveLength(appCount); + }); + + test('selectApp is called when an app is clicked', async () => { + renderComponent(); + userEvent.click(screen.getByLabelText('Select Piazza')); + const clickedCard = screen.getByRole('radio', { checked: true }); + expect(within(clickedCard).queryByLabelText('Select Piazza')).toBeInTheDocument(); + }); }); - const mockStore = async (mockResponse, screenWidth = breakpoints.extraLarge.minWidth) => { - axiosMock.onGet(getDiscussionsProvidersUrl(courseId)).reply(200, generateProvidersApiResponse()); - axiosMock.onGet(getDiscussionsSettingsUrl(courseId)).reply(200, mockResponse); - await executeThunk(fetchProviders(courseId), store.dispatch); - await executeThunk(fetchDiscussionSettings(courseId), store.dispatch); - const component = createComponent(screenWidth); - const wrapper = render(component); - container = wrapper.container; - }; + describe('AppList for Non Admin role', () => { + beforeEach(async () => { + initializeMockApp({ + authenticatedUser: { + userId: 3, + username: 'abc123', + administrator: false, + roles: [], + }, + }); - test('display a card for each available app', async () => { - await mockStore(piazzaApiResponse); - const appCount = store.getState().discussions.appIds.length; - expect(queryAllByRole(container, 'radio')).toHaveLength(appCount); - }); + store = await initializeStore(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + await mockStore(legacyApiResponse, 'legacy'); + }); - test('displays the FeaturesTable at desktop sizes', async () => { - await mockStore(piazzaApiResponse); - expect(queryByRole(container, 'table')).toBeInTheDocument(); - }); - - test('hides the FeaturesTable at mobile sizes', async () => { - await mockStore(piazzaApiResponse, breakpoints.extraSmall.maxWidth); - expect(queryByRole(container, 'table')).not.toBeInTheDocument(); - }); - - test('hides the FeaturesList at desktop sizes', async () => { - await mockStore(piazzaApiResponse); - expect(queryByText(container, messages['supportedFeatureList-mobile-show'].defaultMessage)).not.toBeInTheDocument(); - }); - - test('displays the FeaturesList at mobile sizes', async () => { - await mockStore(piazzaApiResponse, breakpoints.extraSmall.maxWidth); - const appCount = store.getState().discussions.appIds.length; - expect(queryAllByText(container, messages['supportedFeatureList-mobile-show'].defaultMessage)).toHaveLength(appCount); - }); - - test('selectApp is called when an app is clicked', async () => { - await mockStore(piazzaApiResponse); - userEvent.click(getByLabelText(container, 'Select Piazza')); - const clickedCard = getByRole(container, 'radio', { checked: true }); - expect(queryByLabelText(clickedCard, 'Select Piazza')).toBeInTheDocument(); + test('does not display two edx providers card for non admin role', async () => { + renderComponent(); + const appCount = store.getState().discussions.appIds.length; + expect(queryAllByRole(container, 'radio')).toHaveLength(appCount - 1); + }); }); }); diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js index c63c23c04..cc464db36 100644 --- a/src/pages-and-resources/discussions/data/redux.test.js +++ b/src/pages-and-resources/discussions/data/redux.test.js @@ -161,7 +161,7 @@ describe('Data layer integration tests', () => { await executeThunk(fetchDiscussionSettings(courseId), store.dispatch); expect(store.getState().discussions).toEqual(expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'piazza', selectedAppId: null, @@ -197,7 +197,7 @@ describe('Data layer integration tests', () => { await executeThunk(fetchDiscussionSettings(courseId), store.dispatch); expect(store.getState().discussions).toEqual(expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'piazza', selectedAppId: null, @@ -225,7 +225,7 @@ describe('Data layer integration tests', () => { await executeThunk(fetchDiscussionSettings(courseId), store.dispatch); expect(store.getState().discussions).toEqual(expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'legacy', selectedAppId: null, @@ -292,7 +292,7 @@ describe('Data layer integration tests', () => { expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); expect(store.getState().discussions).toEqual( expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'piazza', selectedAppId: 'piazza', @@ -319,7 +319,7 @@ describe('Data layer integration tests', () => { expect(window.location.pathname).toEqual(`/course/${courseId}/pages-and-resources/discussions/configure/piazza`); expect(store.getState().discussions).toEqual( expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'piazza', selectedAppId: 'piazza', @@ -374,7 +374,7 @@ describe('Data layer integration tests', () => { expect(window.location.pathname).toEqual(pagesAndResourcesPath); expect(store.getState().discussions).toEqual( expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'piazza', selectedAppId: 'piazza', @@ -468,7 +468,7 @@ describe('Data layer integration tests', () => { expect(window.location.pathname).toEqual(pagesAndResourcesPath); expect(store.getState().discussions).toEqual( expect.objectContaining({ - appIds: ['legacy', 'piazza', 'discourse'], + appIds: ['legacy', 'openedx', 'piazza', 'discourse'], featureIds, activeAppId: 'legacy', selectedAppId: 'legacy', diff --git a/src/pages-and-resources/discussions/factories/mockApiResponses.js b/src/pages-and-resources/discussions/factories/mockApiResponses.js index 0dfaf9b8f..46baab45a 100644 --- a/src/pages-and-resources/discussions/factories/mockApiResponses.js +++ b/src/pages-and-resources/discussions/factories/mockApiResponses.js @@ -42,6 +42,24 @@ export const generateProvidersApiResponse = (piazzaAdminOnlyConfig = false, acti has_full_support: true, admin_only_config: false, }, + openedx: { + features: [ + 'basic-configuration', + 'discussion-page', + 'embedded-course-sections', + 'wcag-2.1', + ], + external_links: { + learn_more: '', + configuration: '', + general: '', + accessibility: '', + contact_email: '', + }, + messages: [], + has_full_support: true, + admin_only_config: false, + }, piazza: { features: [ // We give piazza all features just so we can test our "full support" text.