From 4752db740b4a833ec0f4196a3c64bb13889e9bc2 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Fri, 13 Dec 2024 03:08:36 +0530 Subject: [PATCH 1/2] feat: Adding human readable 403 error access restricted Signed-off-by: Farhaan Bukhsh --- src/course-outline/constants.js | 1 + src/course-outline/data/thunk.js | 21 +--- src/course-outline/page-alerts/PageAlerts.jsx | 33 ++++++- .../page-alerts/PageAlerts.test.jsx | 99 ++++++++++++------- src/course-outline/page-alerts/messages.js | 15 +++ src/course-outline/utils/getErrorDetails.js | 24 +++++ .../utils/getErrorDetails.test.js | 36 +++++++ 7 files changed, 169 insertions(+), 60 deletions(-) create mode 100644 src/course-outline/utils/getErrorDetails.js create mode 100644 src/course-outline/utils/getErrorDetails.test.js diff --git a/src/course-outline/constants.js b/src/course-outline/constants.js index 4430694a78..eda0522a03 100644 --- a/src/course-outline/constants.js +++ b/src/course-outline/constants.js @@ -87,4 +87,5 @@ export const API_ERROR_TYPES = /** @type {const} */ ({ networkError: 'networkError', serverError: 'serverError', unknown: 'unknown', + forbidden: 'forbidden', }); diff --git a/src/course-outline/data/thunk.js b/src/course-outline/data/thunk.js index c555fcb978..c81ed44f72 100644 --- a/src/course-outline/data/thunk.js +++ b/src/course-outline/data/thunk.js @@ -1,7 +1,7 @@ import { RequestStatus } from '../../data/constants'; import { updateClipboardData } from '../../generic/data/slice'; import { NOTIFICATION_MESSAGES } from '../../constants'; -import { API_ERROR_TYPES, COURSE_BLOCK_NAMES } from '../constants'; +import { COURSE_BLOCK_NAMES } from '../constants'; import { hideProcessingNotification, showProcessingNotification, @@ -10,6 +10,7 @@ import { getCourseBestPracticesChecklist, getCourseLaunchChecklist, } from '../utils/getChecklistForStatusBar'; +import { getErrorDetails } from '../utils/getErrorDetails'; import { addNewCourseItem, deleteCourseItem, @@ -54,24 +55,6 @@ import { updateCourseLaunchQueryStatus, } from './slice'; -const getErrorDetails = (error, dismissible = true) => { - const errorInfo = { dismissible }; - if (error.response?.data) { - const { data } = error.response; - if ((typeof data === 'string' && !data.includes('')) || typeof data === 'object') { - errorInfo.data = JSON.stringify(data); - } - errorInfo.status = error.response.status; - errorInfo.type = API_ERROR_TYPES.serverError; - } else if (error.request) { - errorInfo.type = API_ERROR_TYPES.networkError; - } else { - errorInfo.type = API_ERROR_TYPES.unknown; - errorInfo.data = error.message; - } - return errorInfo; -}; - export function fetchCourseOutlineIndexQuery(courseId) { return async (dispatch) => { dispatch(updateOutlineIndexLoadingStatus({ status: RequestStatus.IN_PROGRESS })); diff --git a/src/course-outline/page-alerts/PageAlerts.jsx b/src/course-outline/page-alerts/PageAlerts.jsx index 4b12996395..9d6eefb211 100644 --- a/src/course-outline/page-alerts/PageAlerts.jsx +++ b/src/course-outline/page-alerts/PageAlerts.jsx @@ -343,13 +343,38 @@ const PageAlerts = ({ const renderApiErrors = () => { let errorList = Object.entries(errors).filter(obj => obj[1] !== null).map(([k, v]) => { switch (v.type) { - case API_ERROR_TYPES.serverError: + case API_ERROR_TYPES.forbidden: { + const description = intl.formatMessage(messages.forbiddenAlertBody, { + LMS: ( + + {intl.formatMessage(messages.forbiddenAlertLmsUrl)} + + ), + }); return { key: k, - desc: v.data || intl.formatMessage(messages.serverErrorAlertBody), + desc: description, + title: intl.formatMessage(messages.forbiddenAlert), + dismissible: v.dismissible, + }; + } + case API_ERROR_TYPES.serverError: { + const description = ( + + {v.data || intl.formatMessage(messages.serverErrorAlertBody)} + + ); + return { + key: k, + desc: description, title: intl.formatMessage(messages.serverErrorAlert), dismissible: v.dismissible, }; + } case API_ERROR_TYPES.networkError: return { key: k, @@ -378,7 +403,7 @@ const PageAlerts = ({ dismissError={() => dispatch(dismissError(msgObj.key))} > {msgObj.title} - {msgObj.desc && {msgObj.desc}} + {msgObj.desc} ) : ( {msgObj.title} - {msgObj.desc && {msgObj.desc}} + {msgObj.desc} ) )) diff --git a/src/course-outline/page-alerts/PageAlerts.test.jsx b/src/course-outline/page-alerts/PageAlerts.test.jsx index 6d646f7cb4..487a7e7ac7 100644 --- a/src/course-outline/page-alerts/PageAlerts.test.jsx +++ b/src/course-outline/page-alerts/PageAlerts.test.jsx @@ -71,19 +71,19 @@ describe('', () => { }); it('renders null when no alerts are present', () => { - const { queryByTestId } = renderComponent(); - expect(queryByTestId('browser-router')).toBeEmptyDOMElement(); + renderComponent(); + expect(screen.queryByTestId('browser-router')).toBeEmptyDOMElement(); }); it('renders configuration alerts', async () => { - const { queryByText } = renderComponent({ + renderComponent({ ...pageAlertsData, notificationDismissUrl: 'some-url', handleDismissNotification, }); - expect(queryByText(messages.configurationErrorTitle.defaultMessage)).toBeInTheDocument(); - const dismissBtn = queryByText('Dismiss'); + expect(screen.queryByText(messages.configurationErrorTitle.defaultMessage)).toBeInTheDocument(); + const dismissBtn = screen.queryByText('Dismiss'); await act(async () => fireEvent.click(dismissBtn)); expect(handleDismissNotification).toBeCalled(); @@ -117,7 +117,7 @@ describe('', () => { }); it('renders deprecation warning alerts', async () => { - const { queryByText } = renderComponent({ + renderComponent({ ...pageAlertsData, deprecatedBlocksInfo: { blocks: [['url1', 'block1'], ['url2']], @@ -126,20 +126,20 @@ describe('', () => { }, }); - expect(queryByText(messages.deprecationWarningTitle.defaultMessage)).toBeInTheDocument(); - expect(queryByText(messages.deprecationWarningBlocksText.defaultMessage)).toBeInTheDocument(); - expect(queryByText('block1')).toHaveAttribute('href', 'url1'); - expect(queryByText(messages.deprecatedComponentName.defaultMessage)).toHaveAttribute('href', 'url2'); + expect(screen.queryByText(messages.deprecationWarningTitle.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.deprecationWarningBlocksText.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText('block1')).toHaveAttribute('href', 'url1'); + expect(screen.queryByText(messages.deprecatedComponentName.defaultMessage)).toHaveAttribute('href', 'url2'); - const feedbackLink = queryByText(messages.advancedSettingLinkText.defaultMessage); + const feedbackLink = screen.queryByText(messages.advancedSettingLinkText.defaultMessage); expect(feedbackLink).toBeInTheDocument(); expect(feedbackLink).toHaveAttribute('href', `${getConfig().STUDIO_BASE_URL}/some-url`); - expect(queryByText('lti')).toBeInTheDocument(); - expect(queryByText('video')).toBeInTheDocument(); + expect(screen.queryByText('lti')).toBeInTheDocument(); + expect(screen.queryByText('video')).toBeInTheDocument(); }); it('renders proctoring alerts with mfe settings link', async () => { - const { queryByText } = renderComponent({ + renderComponent({ ...pageAlertsData, mfeProctoredExamSettingsUrl: 'mfe-url', proctoringErrors: [ @@ -148,15 +148,15 @@ describe('', () => { ], }); - expect(queryByText('error 1')).toBeInTheDocument(); - expect(queryByText('error 2')).toBeInTheDocument(); - expect(queryByText('message 1')).toBeInTheDocument(); - expect(queryByText('message 2')).toBeInTheDocument(); - expect(queryByText(messages.proctoredSettingsLinkText.defaultMessage)).toHaveAttribute('href', 'mfe-url'); + expect(screen.queryByText('error 1')).toBeInTheDocument(); + expect(screen.queryByText('error 2')).toBeInTheDocument(); + expect(screen.queryByText('message 1')).toBeInTheDocument(); + expect(screen.queryByText('message 2')).toBeInTheDocument(); + expect(screen.queryByText(messages.proctoredSettingsLinkText.defaultMessage)).toHaveAttribute('href', 'mfe-url'); }); it('renders proctoring alerts without mfe settings link', async () => { - const { queryByText } = renderComponent({ + renderComponent({ ...pageAlertsData, advanceSettingsUrl: '/some-url', proctoringErrors: [ @@ -165,11 +165,11 @@ describe('', () => { ], }); - expect(queryByText('error 1')).toBeInTheDocument(); - expect(queryByText('error 2')).toBeInTheDocument(); - expect(queryByText('message 1')).toBeInTheDocument(); - expect(queryByText('message 2')).toBeInTheDocument(); - expect(queryByText(messages.advancedSettingLinkText.defaultMessage)).toHaveAttribute( + expect(screen.queryByText('error 1')).toBeInTheDocument(); + expect(screen.queryByText('error 2')).toBeInTheDocument(); + expect(screen.queryByText('message 1')).toBeInTheDocument(); + expect(screen.queryByText('message 2')).toBeInTheDocument(); + expect(screen.queryByText(messages.advancedSettingLinkText.defaultMessage)).toHaveAttribute( 'href', `${getConfig().STUDIO_BASE_URL}/some-url`, ); @@ -181,10 +181,10 @@ describe('', () => { conflictingFiles: [], errorFiles: ['error.css'], }); - const { queryByText } = renderComponent(); - expect(queryByText(messages.newFileAlertTitle.defaultMessage)).toBeInTheDocument(); - expect(queryByText(messages.errorFileAlertTitle.defaultMessage)).toBeInTheDocument(); - expect(queryByText(messages.newFileAlertAction.defaultMessage)).toHaveAttribute( + renderComponent(); + expect(screen.queryByText(messages.newFileAlertTitle.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.errorFileAlertTitle.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.newFileAlertAction.defaultMessage)).toHaveAttribute( 'href', `${getConfig().STUDIO_BASE_URL}/assets/course-id`, ); @@ -196,16 +196,16 @@ describe('', () => { conflictingFiles: ['some.css', 'some.js'], errorFiles: [], }); - const { queryByText } = renderComponent(); - expect(queryByText(messages.conflictingFileAlertTitle.defaultMessage)).toBeInTheDocument(); - expect(queryByText(messages.newFileAlertAction.defaultMessage)).toHaveAttribute( + renderComponent(); + expect(screen.queryByText(messages.conflictingFileAlertTitle.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.newFileAlertAction.defaultMessage)).toHaveAttribute( 'href', `${getConfig().STUDIO_BASE_URL}/assets/course-id`, ); }); it('renders api error alerts', async () => { - const { queryByText } = renderComponent({ + renderComponent({ ...pageAlertsData, errors: { outlineIndexApi: { data: 'some error', status: 400, type: API_ERROR_TYPES.serverError }, @@ -213,9 +213,34 @@ describe('', () => { reindexApi: { type: API_ERROR_TYPES.unknown, data: 'some unknown error' }, }, }); - expect(queryByText(messages.networkErrorAlert.defaultMessage)).toBeInTheDocument(); - expect(queryByText(messages.serverErrorAlert.defaultMessage)).toBeInTheDocument(); - expect(queryByText('some error')).toBeInTheDocument(); - expect(queryByText('some unknown error')).toBeInTheDocument(); + expect(screen.queryByText(messages.networkErrorAlert.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.serverErrorAlert.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText('some error')).toBeInTheDocument(); + expect(screen.queryByText('some unknown error')).toBeInTheDocument(); + }); + + it('renders forbidden api error alerts', async () => { + renderComponent({ + ...pageAlertsData, + errors: { + outlineIndexApi: { + data: 'some error', status: 403, type: API_ERROR_TYPES.forbidden, dismissable: false, + }, + }, + }); + expect(screen.queryByText(messages.forbiddenAlert.defaultMessage)).toBeInTheDocument(); + expect(screen.queryByText(messages.forbiddenAlertBody.defaultMessage)).toBeInTheDocument(); + }); + + it('renders api error alerts when status is not 403', async () => { + renderComponent({ + ...pageAlertsData, + errors: { + outlineIndexApi: { + data: 'some error', status: 500, type: API_ERROR_TYPES.serverError, dismissable: true, + }, + }, + }); + expect(screen.queryByText('some error')).toBeInTheDocument(); }); }); diff --git a/src/course-outline/page-alerts/messages.js b/src/course-outline/page-alerts/messages.js index f9638398d8..9aa6756a78 100644 --- a/src/course-outline/page-alerts/messages.js +++ b/src/course-outline/page-alerts/messages.js @@ -121,6 +121,21 @@ const messages = defineMessages({ defaultMessage: 'Network error', description: 'Generic network error alert.', }, + forbiddenAlert: { + id: 'course-authoring.course-outline.page-alert.forbidden.title', + defaultMessage: 'Access Restricted', + description: 'Forbidden(403) alert title', + }, + forbiddenAlertBody: { + id: 'course-authoring.course-outline.page-alert.forbidden.body', + defaultMessage: 'It looks like you’re trying to access a page you don’t have permission to view. Contact your admin if you think this is a mistake, or head back to the {LMS}.', + description: 'Forbidden(403) alert body', + }, + forbiddenAlertLmsUrl: { + id: 'course-authoring.course-outline.page-alert.lms', + defaultMessage: 'LMS', + description: 'LMS base redirection url', + }, }); export default messages; diff --git a/src/course-outline/utils/getErrorDetails.js b/src/course-outline/utils/getErrorDetails.js new file mode 100644 index 0000000000..fa7e11145b --- /dev/null +++ b/src/course-outline/utils/getErrorDetails.js @@ -0,0 +1,24 @@ +import { API_ERROR_TYPES } from '../constants'; + +export const getErrorDetails = (error, dismissible = true) => { + const errorInfo = { dismissible }; + if (error.response?.status === 403) { + // For 403 status the error shouldn't be dismissible + errorInfo.dismissible = false; + errorInfo.type = API_ERROR_TYPES.forbidden; + errorInfo.status = error.response.status; + } else if (error.response?.data) { + const { data } = error.response; + if ((typeof data === 'string' && !data.includes('')) || typeof data === 'object') { + errorInfo.data = JSON.stringify(data); + } + errorInfo.status = error.response.status; + errorInfo.type = API_ERROR_TYPES.serverError; + } else if (error.request) { + errorInfo.type = API_ERROR_TYPES.networkError; + } else { + errorInfo.type = API_ERROR_TYPES.unknown; + errorInfo.data = error.message; + } + return errorInfo; +}; diff --git a/src/course-outline/utils/getErrorDetails.test.js b/src/course-outline/utils/getErrorDetails.test.js new file mode 100644 index 0000000000..5794859333 --- /dev/null +++ b/src/course-outline/utils/getErrorDetails.test.js @@ -0,0 +1,36 @@ +import { getErrorDetails } from './getErrorDetails'; +import { API_ERROR_TYPES } from '../constants'; + +describe('getErrorDetails', () => { + it('should handle 403 status error', () => { + const error = { response: { data: 'some data', status: 403 } }; + const result = getErrorDetails(error); + expect(result).toEqual({ dismissible: false, status: 403, type: API_ERROR_TYPES.forbidden }); + }); + + it('should handle response with data', () => { + const error = { response: { data: 'some data', status: 500 } }; + const result = getErrorDetails(error); + expect(result).toEqual({ + dismissible: true, data: '"some data"', status: 500, type: API_ERROR_TYPES.serverError, + }); + }); + + it('should handle response with HTML data', () => { + const error = { response: { data: 'error', status: 500 } }; + const result = getErrorDetails(error); + expect(result).toEqual({ dismissible: true, status: 500, type: API_ERROR_TYPES.serverError }); + }); + + it('should handle request error', () => { + const error = { request: {} }; + const result = getErrorDetails(error); + expect(result).toEqual({ dismissible: true, type: API_ERROR_TYPES.networkError }); + }); + + it('should handle unknown error', () => { + const error = { message: 'Unknown error' }; + const result = getErrorDetails(error); + expect(result).toEqual({ dismissible: true, type: API_ERROR_TYPES.unknown, data: 'Unknown error' }); + }); +}); From 4ccb21c582d3c161eeead280ea31c5a27c1c8dfe Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Fri, 10 Jan 2025 11:26:54 +0530 Subject: [PATCH 2/2] fix: Fixes typo in self paced Signed-off-by: Farhaan Bukhsh --- src/course-outline/data/slice.js | 4 ++-- src/course-outline/data/thunk.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/course-outline/data/slice.js b/src/course-outline/data/slice.js index 0bd146f95d..0275555262 100644 --- a/src/course-outline/data/slice.js +++ b/src/course-outline/data/slice.js @@ -104,7 +104,7 @@ const slice = createSlice({ ...payload, }; }, - fetchStatusBarSelPacedSuccess: (state, { payload }) => { + fetchStatusBarSelfPacedSuccess: (state, { payload }) => { state.statusBarData.isSelfPaced = payload.isSelfPaced; }, updateSavingStatus: (state, { payload }) => { @@ -206,7 +206,7 @@ export const { updateStatusBar, updateCourseActions, fetchStatusBarChecklistSuccess, - fetchStatusBarSelPacedSuccess, + fetchStatusBarSelfPacedSuccess, updateFetchSectionLoadingStatus, updateCourseLaunchQueryStatus, updateSavingStatus, diff --git a/src/course-outline/data/thunk.js b/src/course-outline/data/thunk.js index c81ed44f72..457d039114 100644 --- a/src/course-outline/data/thunk.js +++ b/src/course-outline/data/thunk.js @@ -42,7 +42,7 @@ import { updateStatusBar, updateCourseActions, fetchStatusBarChecklistSuccess, - fetchStatusBarSelPacedSuccess, + fetchStatusBarSelfPacedSuccess, updateSavingStatus, updateSectionList, updateFetchSectionLoadingStatus, @@ -108,7 +108,7 @@ export function fetchCourseLaunchQuery({ const data = await getCourseLaunch({ courseId, gradedOnly, validateOras, all, }); - dispatch(fetchStatusBarSelPacedSuccess({ isSelfPaced: data.isSelfPaced })); + dispatch(fetchStatusBarSelfPacedSuccess({ isSelfPaced: data.isSelfPaced })); dispatch(fetchStatusBarChecklistSuccess(getCourseLaunchChecklist(data))); dispatch(updateCourseLaunchQueryStatus({ status: RequestStatus.SUCCESSFUL }));