From b4bedfe3f018b88c7c93d30f42827ba80c27ff52 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Fri, 30 Jul 2021 14:20:42 -0400 Subject: [PATCH] fix: re-enable access error redirects for course home (#570) These redirects are already in place for the courseware, and will redirect to the outline page, or the dashboard, or wherever, based on the type of access error (unenrolled, access expired, survey needed, etc). This commit stops the course home tabs from paying attention to the 401 error messages coming from the backend - course_access in the metadata API handles that now. This commit also changes our useModel hook to more gracefully handle not being able to find the model - it is a valid case we want to allow (still will cause problems if you actually try to use the data, but will at least provide an object you can inspect). --- src/course-home/data/api.js | 12 +++-- src/course-home/data/thunks.js | 7 ++- src/course-home/dates-tab/DatesTab.test.jsx | 12 ++++- src/generic/model-store/hooks.js | 4 +- src/shared/access-denied-redirect/index.js | 3 -- .../AccessDeniedRedirect.jsx => access.js} | 44 ++++------------ src/tab-page/TabPage.jsx | 51 +++++++++++-------- 7 files changed, 62 insertions(+), 71 deletions(-) delete mode 100644 src/shared/access-denied-redirect/index.js rename src/shared/{access-denied-redirect/AccessDeniedRedirect.jsx => access.js} (62%) diff --git a/src/course-home/data/api.js b/src/course-home/data/api.js index e6fd22d5..9791171a 100644 --- a/src/course-home/data/api.js +++ b/src/course-home/data/api.js @@ -199,10 +199,12 @@ export async function getDatesTabData(courseId) { const { httpErrorStatus } = error && error.customAttributes; if (httpErrorStatus === 404) { global.location.replace(`${getConfig().LMS_BASE_URL}/courses/${courseId}/dates`); + return {}; } - // 401 can be returned for unauthenticated users or users who are not enrolled if (httpErrorStatus === 401) { - global.location.replace(`${getConfig().BASE_URL}/course/${courseId}/home`); + // The backend sends this for unenrolled and unauthenticated learners, but we handle those cases by examining + // courseAccess in the metadata call, so just ignore this status for now. + return {}; } throw error; } @@ -266,10 +268,12 @@ export async function getProgressTabData(courseId, targetUserId) { const { httpErrorStatus } = error && error.customAttributes; if (httpErrorStatus === 404) { global.location.replace(`${getConfig().LMS_BASE_URL}/courses/${courseId}/progress`); + return {}; } - // 401 can be returned for unauthenticated users or users who are not enrolled if (httpErrorStatus === 401) { - global.location.replace(`${getConfig().BASE_URL}/course/${courseId}/home`); + // The backend sends this for unenrolled and unauthenticated learners, but we handle those cases by examining + // courseAccess in the metadata call, so just ignore this status for now. + return {}; } throw error; } diff --git a/src/course-home/data/thunks.js b/src/course-home/data/thunks.js index 1002a7b0..99fdc6fd 100644 --- a/src/course-home/data/thunks.js +++ b/src/course-home/data/thunks.js @@ -17,7 +17,7 @@ import { } from '../../generic/model-store'; import { - // fetchTabDenied, + fetchTabDenied, fetchTabFailure, fetchTabRequest, fetchTabSuccess, @@ -63,10 +63,9 @@ export function fetchTab(courseId, tab, getTabData, targetUserId) { } // Disable the access-denied path for now - it caused a regression - /* if (fetchedCourseHomeCourseMetadata && !courseHomeCourseMetadataResult.value.courseAccess.hasAccess) { + if (fetchedCourseHomeCourseMetadata && !courseHomeCourseMetadataResult.value.courseAccess.hasAccess) { dispatch(fetchTabDenied({ courseId })); - } else */ - if (fetchedCourseHomeCourseMetadata && fetchedTabData) { + } else if (fetchedCourseHomeCourseMetadata && fetchedTabData) { dispatch(fetchTabSuccess({ courseId, targetUserId })); } else { dispatch(fetchTabFailure({ courseId })); diff --git a/src/course-home/dates-tab/DatesTab.test.jsx b/src/course-home/dates-tab/DatesTab.test.jsx index d0937772..62c9e195 100644 --- a/src/course-home/dates-tab/DatesTab.test.jsx +++ b/src/course-home/dates-tab/DatesTab.test.jsx @@ -299,7 +299,7 @@ describe('DatesTab', () => { }); }); - describe.skip('when receiving an access denied error', () => { + describe('when receiving an access denied error', () => { // These tests could go into any particular tab, as they all go through the same flow. But dates tab works. async function renderDenied(errorCode) { @@ -338,5 +338,15 @@ describe('DatesTab', () => { await renderDenied('course_not_started'); expect(global.location.href).toEqual('http://localhost/redirect/dashboard?notlive=2/5/2013'); // date from factory }); + + it('redirects to the home page when unauthenticated', async () => { + await renderDenied('authentication_required'); + expect(global.location.href).toEqual(`http://localhost/redirect/course-home/${courseMetadata.id}`); + }); + + it('redirects to the home page when unenrolled', async () => { + await renderDenied('enrollment_required'); + expect(global.location.href).toEqual(`http://localhost/redirect/course-home/${courseMetadata.id}`); + }); }); }); diff --git a/src/generic/model-store/hooks.js b/src/generic/model-store/hooks.js index 8ee8b41e..15294697 100644 --- a/src/generic/model-store/hooks.js +++ b/src/generic/model-store/hooks.js @@ -2,7 +2,7 @@ import { useSelector, shallowEqual } from 'react-redux'; export function useModel(type, id) { return useSelector( - state => (state.models[type] !== undefined ? state.models[type][id] : undefined), + state => (state.models[type] !== undefined ? state.models[type][id] : {}), shallowEqual, ); } @@ -10,7 +10,7 @@ export function useModel(type, id) { export function useModels(type, ids) { return useSelector( state => ids.map( - id => (state.models[type] !== undefined ? state.models[type][id] : undefined), + id => (state.models[type] !== undefined ? state.models[type][id] : {}), ), shallowEqual, ); diff --git a/src/shared/access-denied-redirect/index.js b/src/shared/access-denied-redirect/index.js deleted file mode 100644 index cc0d1e00..00000000 --- a/src/shared/access-denied-redirect/index.js +++ /dev/null @@ -1,3 +0,0 @@ -import AccessDeniedRedirect from './AccessDeniedRedirect'; - -export default AccessDeniedRedirect; diff --git a/src/shared/access-denied-redirect/AccessDeniedRedirect.jsx b/src/shared/access.js similarity index 62% rename from src/shared/access-denied-redirect/AccessDeniedRedirect.jsx rename to src/shared/access.js index 02e05d08..759475d0 100644 --- a/src/shared/access-denied-redirect/AccessDeniedRedirect.jsx +++ b/src/shared/access.js @@ -1,26 +1,11 @@ -import React from 'react'; -import PropTypes from 'prop-types'; +/* eslint-disable import/prefer-default-export */ import { getLocale } from '@edx/frontend-platform/i18n'; -import { Redirect } from 'react-router'; -import { useModel } from '../../generic/model-store'; -// This component inspects an access denied error and redirects to a /redirect/... path, which then renders a nice -// little message while the browser loads the next page. +// This function inspects an access denied error and provides a redirect url (looks like a /redirect/... path), +// which then renders a nice little message while the browser loads the next page. // This is basically a frontend version of check_course_access_with_redirect in the backend. - -function AccessDeniedRedirect(props) { - const { - courseId, - metadataModel, - unitId, - } = props; - - const { - courseAccess, - start, - } = useModel(metadataModel, courseId); - - let url = `/redirect/course-home/${courseId}`; +export function getAccessDeniedRedirectUrl(courseId, activeTabSlug, courseAccess, start, unitId) { + let url = null; switch (courseAccess.errorCode) { case 'audit_expired': url = `/redirect/dashboard?access_response_error=${courseAccess.additionalContextUserMessage}`; @@ -48,20 +33,9 @@ function AccessDeniedRedirect(props) { case 'authentication_required': case 'enrollment_required': default: + if (activeTabSlug !== 'outline') { + url = `/redirect/course-home/${courseId}`; + } } - return ( - - ); + return url; } - -AccessDeniedRedirect.defaultProps = { - unitId: null, -}; - -AccessDeniedRedirect.propTypes = { - courseId: PropTypes.string.isRequired, - metadataModel: PropTypes.string.isRequired, - unitId: PropTypes.string, -}; - -export default AccessDeniedRedirect; diff --git a/src/tab-page/TabPage.jsx b/src/tab-page/TabPage.jsx index 87438dd1..a84ad3ac 100644 --- a/src/tab-page/TabPage.jsx +++ b/src/tab-page/TabPage.jsx @@ -2,31 +2,37 @@ import React from 'react'; import PropTypes from 'prop-types'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import { useDispatch, useSelector } from 'react-redux'; +import { Redirect } from 'react-router'; import { Toast } from '@edx/paragon'; import { Header } from '../course-header'; -import AccessDeniedRedirect from '../shared/access-denied-redirect'; +import { getAccessDeniedRedirectUrl } from '../shared/access'; import PageLoading from '../generic/PageLoading'; +import { useModel } from '../generic/model-store'; import genericMessages from '../generic/messages'; import messages from './messages'; import LoadedTabPage from './LoadedTabPage'; import { setCallToActionToast } from '../course-home/data/slice'; -function TabPage({ - intl, - courseId, - courseStatus, - metadataModel, - unitId, - ...passthroughProps -}) { +function TabPage({ intl, ...props }) { + const { + activeTabSlug, + courseId, + courseStatus, + metadataModel, + unitId, + } = props; const { toastBodyLink, toastBodyText, toastHeader, } = useSelector(state => state.courseHome); const dispatch = useDispatch(); + const { + courseAccess, + start, + } = useModel(metadataModel, courseId); if (courseStatus === 'loading') { return ( @@ -39,7 +45,16 @@ function TabPage({ ); } - if (courseStatus === 'loaded') { + if (courseStatus === 'denied') { + const redirectUrl = getAccessDeniedRedirectUrl(courseId, activeTabSlug, courseAccess, start, unitId); + if (redirectUrl) { + return (); + } + } + + // Either a success state or a denied state that wasn't redirected above (some tabs handle denied states themselves, + // like the outline tab handling unenrolled learners) + if (courseStatus === 'loaded' || courseStatus === 'denied') { return ( <> {toastHeader} - + ); } - if (courseStatus === 'denied') { - return ( - - ); - } - // courseStatus 'failed' and any other unexpected course status. return ( <> @@ -80,12 +85,14 @@ function TabPage({ } TabPage.defaultProps = { + courseId: null, unitId: null, }; TabPage.propTypes = { + activeTabSlug: PropTypes.string.isRequired, intl: intlShape.isRequired, - courseId: PropTypes.string.isRequired, + courseId: PropTypes.string, courseStatus: PropTypes.string.isRequired, metadataModel: PropTypes.string.isRequired, unitId: PropTypes.string,