From 99166255c49ab9294a356dee89c22c746afd47f8 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Mon, 21 Oct 2024 17:09:05 -0400 Subject: [PATCH 1/8] feat: add functionality to see unit draft preview --- src/constants.ts | 1 + src/courseware/CoursewareContainer.jsx | 166 ++++++++++++------ src/courseware/course/Course.jsx | 8 + src/courseware/course/sequence/Sequence.jsx | 1 + .../course/sequence/SequenceContent.jsx | 10 +- src/courseware/course/sequence/Unit/index.jsx | 7 +- .../course/sequence/Unit/index.test.jsx | 4 +- src/courseware/course/sequence/Unit/urls.js | 2 + .../course/sequence/Unit/urls.test.js | 22 ++- .../SequenceNavigation.jsx | 64 +++---- .../sequence-navigation/UnitButton.jsx | 7 +- .../sequence-navigation/UnitNavigation.jsx | 60 +++---- .../generic/NextButton.jsx | 56 ++++++ .../generic/PreviousButton.jsx | 44 +++++ src/courseware/utils.jsx | 6 +- 15 files changed, 317 insertions(+), 141 deletions(-) create mode 100644 src/courseware/course/sequence/sequence-navigation/generic/NextButton.jsx create mode 100644 src/courseware/course/sequence/sequence-navigation/generic/PreviousButton.jsx diff --git a/src/constants.ts b/src/constants.ts index ac8ec85151..544e042ad0 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -13,6 +13,7 @@ export const DECODE_ROUTES = { '/course/:courseId/:sequenceId/:unitId', '/course/:courseId/:sequenceId', '/course/:courseId', + '/preview/course/:courseId/:sequenceId/:unitId', ], REDIRECT_HOME: 'home/:courseId', REDIRECT_SURVEY: 'survey/:courseId', diff --git a/src/courseware/CoursewareContainer.jsx b/src/courseware/CoursewareContainer.jsx index 8929419075..fe96652263 100644 --- a/src/courseware/CoursewareContainer.jsx +++ b/src/courseware/CoursewareContainer.jsx @@ -19,13 +19,15 @@ import { handleNextSectionCelebration } from './course/celebration'; import withParamsAndNavigation from './utils'; // Look at where this is called in componentDidUpdate for more info about its usage -const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSequenceId, navigate) => { +const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSequenceId, navigate, isPreview) => { if (courseStatus === 'loaded' && !sequenceId) { // Note that getResumeBlock is just an API call, not a redux thunk. getResumeBlock(courseId).then((data) => { // This is a replace because we don't want this change saved in the browser's history. if (data.sectionId && data.unitId) { - navigate(`/course/${courseId}/${data.sectionId}/${data.unitId}`, { replace: true }); + const baseUrl = `/course/${courseId}/${data.sectionId}`; + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + navigate(`${sequenceUrl}/${data.unitId}`, { replace: true }); } else if (firstSequenceId) { navigate(`/course/${courseId}/${firstSequenceId}`, { replace: true }); } @@ -34,9 +36,19 @@ const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSe }); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSectionUnitToUnitRedirect = memoize((courseStatus, courseId, sequenceStatus, section, unitId, navigate) => { +const checkSectionUnitToUnitRedirect = memoize(( + courseStatus, + courseId, + sequenceStatus, + section, + unitId, + navigate, + isPreview, +) => { if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && unitId) { - navigate(`/course/${courseId}/${unitId}`, { replace: true }); + const baseUrl = `/course/${courseId}`; + const courseUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + navigate(`${courseUrl}/${unitId}`, { replace: true }); } }); @@ -54,68 +66,84 @@ const checkSectionToSequenceRedirect = memoize((courseStatus, courseId, sequence }); // Look at where this is called in componentDidUpdate for more info about its usage -const checkUnitToSequenceUnitRedirect = memoize( - (courseStatus, courseId, sequenceStatus, sequenceMightBeUnit, sequenceId, section, routeUnitId, navigate) => { - if (courseStatus === 'loaded' && sequenceStatus === 'failed' && !section && !routeUnitId) { - if (sequenceMightBeUnit) { - // If the sequence failed to load as a sequence, but it is marked as a possible unit, then - // we need to look up the correct parent sequence for it, and redirect there. - const unitId = sequenceId; // just for clarity during the rest of this method - getSequenceForUnitDeprecated(courseId, unitId).then( - parentId => { - if (parentId) { - navigate(`/course/${courseId}/${parentId}/${unitId}`, { replace: true }); - } else { - navigate(`/course/${courseId}`, { replace: true }); - } - }, - () => { // error case +const checkUnitToSequenceUnitRedirect = memoize(( + courseStatus, + courseId, + sequenceStatus, + sequenceMightBeUnit, + sequenceId, + section, + routeUnitId, + navigate, + isPreview, +) => { + if (courseStatus === 'loaded' && sequenceStatus === 'failed' && !section && !routeUnitId) { + if (sequenceMightBeUnit) { + // If the sequence failed to load as a sequence, but it is marked as a possible unit, then + // we need to look up the correct parent sequence for it, and redirect there. + const unitId = sequenceId; // just for clarity during the rest of this method + getSequenceForUnitDeprecated(courseId, unitId).then( + parentId => { + if (parentId) { + const baseUrl = `/course/${courseId}/${parentId}`; + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + navigate(`${sequenceUrl}/${unitId}`, { replace: true }); + } else { navigate(`/course/${courseId}`, { replace: true }); - }, - ); - } else { - // Invalid sequence that isn't a unit either. Redirect up to main course. - navigate(`/course/${courseId}`, { replace: true }); - } + } + }, + () => { // error case + navigate(`/course/${courseId}`, { replace: true }); + }, + ); + } else { + // Invalid sequence that isn't a unit either. Redirect up to main course. + navigate(`/course/${courseId}`, { replace: true }); } - }, -); + } +}); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSequenceToSequenceUnitRedirect = memoize((courseId, sequenceStatus, sequence, unitId, navigate) => { - if (sequenceStatus === 'loaded' && sequence.id && !unitId) { - if (sequence.unitIds !== undefined && sequence.unitIds.length > 0) { - const nextUnitId = sequence.unitIds[sequence.activeUnitIndex]; - // This is a replace because we don't want this change saved in the browser's history. - navigate(`/course/${courseId}/${sequence.id}/${nextUnitId}`, { replace: true }); +const checkSequenceToSequenceUnitRedirect = memoize( + (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { + if (sequenceStatus === 'loaded' && sequence.id && !unitId) { + if (sequence.unitIds !== undefined && sequence.unitIds.length > 0) { + const baseUrl = `/course/${courseId}/${sequence.id}`; + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + const nextUnitId = sequence.unitIds[sequence.activeUnitIndex]; + // This is a replace because we don't want this change saved in the browser's history. + navigate(`${sequenceUrl}/${nextUnitId}`, { replace: true }); + } } - } -}); + }, +); // Look at where this is called in componentDidUpdate for more info about its usage const checkSequenceUnitMarkerToSequenceUnitRedirect = memoize( - (courseId, sequenceStatus, sequence, unitId, navigate) => { + (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { if (sequenceStatus !== 'loaded' || !sequence.id) { return; } + const baseUrl = `/course/${courseId}/${sequence.id}`; + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; const hasUnits = sequence.unitIds?.length > 0; if (unitId === 'first') { if (hasUnits) { const firstUnitId = sequence.unitIds[0]; - navigate(`/course/${courseId}/${sequence.id}/${firstUnitId}`, { replace: true }); + navigate(`${sequenceUrl}/${firstUnitId}`, { replace: true }); } else { // No units... go to general sequence page - navigate(`/course/${courseId}/${sequence.id}`, { replace: true }); + navigate(baseUrl, { replace: true }); } } else if (unitId === 'last') { if (hasUnits) { const lastUnitId = sequence.unitIds[sequence.unitIds.length - 1]; - navigate(`/course/${courseId}/${sequence.id}/${lastUnitId}`, { replace: true }); + navigate(`${sequenceUrl}/${lastUnitId}`, { replace: true }); } else { // No units... go to general sequence page - navigate(`/course/${courseId}/${sequence.id}`, { replace: true }); + navigate(baseUrl, { replace: true }); } } }, @@ -169,6 +197,7 @@ class CoursewareContainer extends Component { routeSequenceId, routeUnitId, navigate, + isPreview, } = this.props; // Load data whenever the course or sequence ID changes. @@ -197,7 +226,7 @@ class CoursewareContainer extends Component { // Check resume redirect: // /course/:courseId -> /course/:courseId/:sequenceId/:unitId // based on sequence/unit where user was last active. - checkResumeRedirect(courseStatus, courseId, sequenceId, firstSequenceId, navigate); + checkResumeRedirect(courseStatus, courseId, sequenceId, firstSequenceId, navigate, isPreview); // Check section-unit to unit redirect: // /course/:courseId/:sectionId/:unitId -> /course/:courseId/:unitId @@ -210,33 +239,69 @@ class CoursewareContainer extends Component { // otherwise, we could get stuck in a redirect loop, since a sequence that failed to load // would endlessly redirect to itself through `checkSectionUnitToUnitRedirect` // and `checkUnitToSequenceUnitRedirect`. - checkSectionUnitToUnitRedirect(courseStatus, courseId, sequenceStatus, sectionViaSequenceId, routeUnitId, navigate); + checkSectionUnitToUnitRedirect( + courseStatus, + courseId, + sequenceStatus, + sectionViaSequenceId, + routeUnitId, + navigate, + isPreview, + ); // Check section to sequence redirect: // /course/:courseId/:sectionId -> /course/:courseId/:sequenceId // by redirecting to the first sequence within the section. - checkSectionToSequenceRedirect(courseStatus, courseId, sequenceStatus, sectionViaSequenceId, routeUnitId, navigate); + checkSectionToSequenceRedirect( + courseStatus, + courseId, + sequenceStatus, + sectionViaSequenceId, + routeUnitId, + navigate, + ); // Check unit to sequence-unit redirect: // /course/:courseId/:unitId -> /course/:courseId/:sequenceId/:unitId // by filling in the ID of the parent sequence of :unitId. - checkUnitToSequenceUnitRedirect(( - courseStatus, courseId, sequenceStatus, sequenceMightBeUnit, - sequenceId, sectionViaSequenceId, routeUnitId, navigate - )); + checkUnitToSequenceUnitRedirect( + courseStatus, + courseId, + sequenceStatus, + sequenceMightBeUnit, + sequenceId, + sectionViaSequenceId, + routeUnitId, + navigate, + isPreview, + ); // Check sequence to sequence-unit redirect: // /course/:courseId/:sequenceId -> /course/:courseId/:sequenceId/:unitId // by filling in the ID the most-recently-active unit in the sequence, OR // the ID of the first unit the sequence if none is active. - checkSequenceToSequenceUnitRedirect(courseId, sequenceStatus, sequence, routeUnitId, navigate); + checkSequenceToSequenceUnitRedirect( + courseId, + sequenceStatus, + sequence, + routeUnitId, + navigate, + isPreview, + ); // Check sequence-unit marker to sequence-unit redirect: // /course/:courseId/:sequenceId/first -> /course/:courseId/:sequenceId/:unitId // /course/:courseId/:sequenceId/last -> /course/:courseId/:sequenceId/:unitId // by filling in the ID the first or last unit in the sequence. // "Sequence unit marker" is an invented term used only in this component. - checkSequenceUnitMarkerToSequenceUnitRedirect(courseId, sequenceStatus, sequence, routeUnitId, navigate); + checkSequenceUnitMarkerToSequenceUnitRedirect( + courseId, + sequenceStatus, + sequence, + routeUnitId, + navigate, + isPreview, + ); } handleUnitNavigationClick = () => { @@ -334,6 +399,7 @@ CoursewareContainer.propTypes = { fetchCourse: PropTypes.func.isRequired, fetchSequence: PropTypes.func.isRequired, navigate: PropTypes.func.isRequired, + isPreview: PropTypes.bool.isRequired, }; CoursewareContainer.defaultProps = { diff --git a/src/courseware/course/Course.jsx b/src/courseware/course/Course.jsx index 131db82e09..3bfd91eca5 100644 --- a/src/courseware/course/Course.jsx +++ b/src/courseware/course/Course.jsx @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import { Helmet } from 'react-helmet'; import { useDispatch, useSelector } from 'react-redux'; import { getConfig } from '@edx/frontend-platform'; +import { useLocation, useNavigate } from 'react-router-dom'; import { breakpoints, useWindowSize } from '@openedx/paragon'; import { AlertList } from '@src/generic/user-messages'; @@ -38,6 +39,13 @@ const Course = ({ const section = useModel('sections', sequence ? sequence.sectionId : null); const { enableNavigationSidebar } = useSelector(getCoursewareOutlineSidebarSettings); const navigationDisabled = enableNavigationSidebar || (sequence?.navigationDisabled ?? false); + const navigate = useNavigate(); + const { pathname } = useLocation(); + + if (!isStaff && pathname.startsWith('/preview')) { + const courseUrl = pathname.replace('/preview', ''); + navigate(courseUrl, { replace: true }); + } const pageTitleBreadCrumbs = [ sequence, diff --git a/src/courseware/course/sequence/Sequence.jsx b/src/courseware/course/sequence/Sequence.jsx index 1a2a8f2d33..dce4561bb2 100644 --- a/src/courseware/course/sequence/Sequence.jsx +++ b/src/courseware/course/sequence/Sequence.jsx @@ -204,6 +204,7 @@ const Sequence = ({ sequenceId={sequenceId} unitId={unitId} unitLoadedHandler={handleUnitLoaded} + isStaff={isStaff} /> {unitHasLoaded && renderUnitNavigation(false)} diff --git a/src/courseware/course/sequence/SequenceContent.jsx b/src/courseware/course/sequence/SequenceContent.jsx index 35a9a27f9d..6c40c5bfd2 100644 --- a/src/courseware/course/sequence/SequenceContent.jsx +++ b/src/courseware/course/sequence/SequenceContent.jsx @@ -1,6 +1,6 @@ import React, { Suspense, useEffect } from 'react'; import PropTypes from 'prop-types'; -import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; +import { useIntl } from '@edx/frontend-platform/i18n'; import PageLoading from '../../../generic/PageLoading'; import { useModel } from '../../../generic/model-store'; @@ -11,12 +11,13 @@ const ContentLock = React.lazy(() => import('./content-lock')); const SequenceContent = ({ gated, - intl, courseId, sequenceId, unitId, unitLoadedHandler, + isStaff, }) => { + const intl = useIntl(); const sequence = useModel('sequences', sequenceId); // Go back to the top of the page whenever the unit or sequence changes. @@ -59,6 +60,7 @@ const SequenceContent = ({ key={unitId} id={unitId} onLoaded={unitLoadedHandler} + isStaff={isStaff} /> ); }; @@ -69,11 +71,11 @@ SequenceContent.propTypes = { sequenceId: PropTypes.string.isRequired, unitId: PropTypes.string, unitLoadedHandler: PropTypes.func.isRequired, - intl: intlShape.isRequired, + isStaff: PropTypes.bool.isRequired, }; SequenceContent.defaultProps = { unitId: null, }; -export default injectIntl(SequenceContent); +export default SequenceContent; diff --git a/src/courseware/course/sequence/Unit/index.jsx b/src/courseware/course/sequence/Unit/index.jsx index a3081c7e85..71663cadc9 100644 --- a/src/courseware/course/sequence/Unit/index.jsx +++ b/src/courseware/course/sequence/Unit/index.jsx @@ -1,6 +1,6 @@ import PropTypes from 'prop-types'; import React from 'react'; -import { useSearchParams } from 'react-router-dom'; +import { useSearchParams, useLocation } from 'react-router-dom'; import { AppContext } from '@edx/frontend-platform/react'; import { useIntl } from '@edx/frontend-platform/i18n'; @@ -22,15 +22,18 @@ const Unit = ({ format, onLoaded, id, + isStaff, }) => { const { formatMessage } = useIntl(); const [searchParams] = useSearchParams(); + const { pathname } = useLocation(); const { authenticatedUser } = React.useContext(AppContext); const examAccess = useExamAccess({ id }); const shouldDisplayHonorCode = useShouldDisplayHonorCode({ courseId, id }); const unit = useModel(modelKeys.units, id); const isProcessing = unit.bookmarkedUpdateState === 'loading'; const view = authenticatedUser ? views.student : views.public; + const shouldDisplayUnitPreview = pathname.startsWith('/preview') && isStaff; const getUrl = usePluginsCallback('getIFrameUrl', () => getIFrameUrl({ id, @@ -38,6 +41,7 @@ const Unit = ({ format, examAccess, jumpToId: searchParams.get('jumpToId'), + preview: shouldDisplayUnitPreview ? '1' : '0', })); const iframeUrl = getUrl(); @@ -74,6 +78,7 @@ Unit.propTypes = { format: PropTypes.string, id: PropTypes.string.isRequired, onLoaded: PropTypes.func, + isStaff: PropTypes.bool.isRequired, }; Unit.defaultProps = { diff --git a/src/courseware/course/sequence/Unit/index.test.jsx b/src/courseware/course/sequence/Unit/index.test.jsx index bdaf2743d0..4f9a221640 100644 --- a/src/courseware/course/sequence/Unit/index.test.jsx +++ b/src/courseware/course/sequence/Unit/index.test.jsx @@ -1,7 +1,7 @@ import React from 'react'; import { when } from 'jest-when'; import { formatMessage, shallow } from '@edx/react-unit-test-utils/dist'; -import { useSearchParams } from 'react-router-dom'; +import { useSearchParams, useLocation } from 'react-router-dom'; import { useModel } from '@src/generic/model-store'; @@ -55,6 +55,7 @@ const props = { format: 'test-format', onLoaded: jest.fn().mockName('props.onLoaded'), id: 'test-props-id', + isStaff: false, }; const context = { authenticatedUser: { test: 'user' } }; @@ -89,6 +90,7 @@ describe('Unit component', () => { beforeEach(() => { useSearchParams.mockImplementation(() => [searchParams, setSearchParams]); + useLocation.mockImplementation(() => ({ pathname: `/course/${props.courseId}` })); jest.clearAllMocks(); el = shallow(); }); diff --git a/src/courseware/course/sequence/Unit/urls.js b/src/courseware/course/sequence/Unit/urls.js index 7e9fa3c94e..d13fe6e39e 100644 --- a/src/courseware/course/sequence/Unit/urls.js +++ b/src/courseware/course/sequence/Unit/urls.js @@ -13,6 +13,7 @@ export const getIFrameUrl = ({ format, examAccess, jumpToId, + preview, }) => { const xblockUrl = `${getConfig().LMS_BASE_URL}/xblock/${id}`; return stringifyUrl({ @@ -20,6 +21,7 @@ export const getIFrameUrl = ({ query: { ...iframeParams, view, + preview, ...(format && { format }), ...(!examAccess.blockAccess && { exam_access: examAccess.accessToken }), jumpToId, // Pass jumpToId as query param as fragmentIdentifier is not passed to server. diff --git a/src/courseware/course/sequence/Unit/urls.test.js b/src/courseware/course/sequence/Unit/urls.test.js index 9de154e2d1..9540f19e01 100644 --- a/src/courseware/course/sequence/Unit/urls.test.js +++ b/src/courseware/course/sequence/Unit/urls.test.js @@ -17,6 +17,7 @@ const props = { view: 'test-view', format: 'test-format', examAccess: { blockAccess: false, accessToken: 'test-access-token' }, + preview: false, }; describe('urls module getIFrameUrl', () => { @@ -28,6 +29,7 @@ describe('urls module getIFrameUrl', () => { view: props.view, format: props.format, exam_access: props.examAccess.accessToken, + preview: props.preview, }, }); expect(getIFrameUrl(props)).toEqual(url); @@ -35,11 +37,12 @@ describe('urls module getIFrameUrl', () => { test('no format provided, exam access blocked', () => { const url = stringifyUrl({ url: `${config.LMS_BASE_URL}/xblock/${props.id}`, - query: { ...iframeParams, view: props.view }, + query: { ...iframeParams, view: props.view, preview: props.preview }, }); expect(getIFrameUrl({ id: props.id, view: props.view, + preview: props.preview, examAccess: { blockAccess: true }, })).toEqual(url); }); @@ -50,6 +53,7 @@ describe('urls module getIFrameUrl', () => { ...iframeParams, view: props.view, format: props.format, + preview: props.preview, exam_access: props.examAccess.accessToken, jumpToId: 'some-xblock-id', }, @@ -60,4 +64,20 @@ describe('urls module getIFrameUrl', () => { jumpToId: 'some-xblock-id', })).toEqual(url); }); + test('preview is true and url param equals 1', () => { + const url = stringifyUrl({ + url: `${config.LMS_BASE_URL}/xblock/${props.id}`, + query: { + ...iframeParams, + view: props.view, + format: props.format, + preview: true, + exam_access: props.examAccess.accessToken, + }, + }); + expect(getIFrameUrl({ + ...props, + preview: true, + })).toEqual(url); + }); }); diff --git a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx index 38fd16bbc3..918f6c3ef3 100644 --- a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx +++ b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx @@ -1,15 +1,8 @@ import React from 'react'; -import { Link } from 'react-router-dom'; import PropTypes from 'prop-types'; -import { breakpoints, Button, useWindowSize } from '@openedx/paragon'; -import { ChevronLeft, ChevronRight } from '@openedx/paragon/icons'; +import { breakpoints, useWindowSize } from '@openedx/paragon'; import classNames from 'classnames'; -import { - injectIntl, - intlShape, - isRtl, - getLocale, -} from '@edx/frontend-platform/i18n'; +import { useIntl } from '@edx/frontend-platform/i18n'; import { PluginSlot } from '@openedx/frontend-plugin-framework'; import { useSelector } from 'react-redux'; @@ -21,9 +14,10 @@ import { useSequenceNavigationMetadata } from './hooks'; import { useModel } from '../../../../generic/model-store'; import messages from './messages'; +import PreviousButton from './generic/PreviousButton'; +import NextButton from './generic/NextButton'; const SequenceNavigation = ({ - intl, unitId, sequenceId, className, @@ -36,6 +30,7 @@ const SequenceNavigation = ({ open, close, }) => { + const intl = useIntl(); const sequence = useModel('sequences', sequenceId); const { isFirstUnit, @@ -76,29 +71,21 @@ const SequenceNavigation = ({ ); }; - const renderPreviousButton = () => { - const disabled = isFirstUnit; - const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft; - return navigationDisabledPrevSequence || ( - - ); - }; + const renderPreviousButton = () => navigationDisabledPrevSequence || ( + + ); const renderNextButton = () => { const { exitActive, exitText } = GetCourseExitNavigation(courseId, intl); const buttonText = (isLastUnit && exitText) ? exitText : intl.formatMessage(messages.nextButton); const disabled = isLastUnit && !exitActive; - const nextArrow = isRtl(getLocale()) ? ChevronLeft : ChevronRight; return navigationDisabledNextSequence || ( - + buttonLabel={shouldDisplayNotificationTriggerInSequence ? null : buttonText} + /> ); }; @@ -145,7 +126,6 @@ const SequenceNavigation = ({ }; SequenceNavigation.propTypes = { - intl: intlShape.isRequired, sequenceId: PropTypes.string.isRequired, unitId: PropTypes.string, className: PropTypes.string, @@ -169,4 +149,4 @@ SequenceNavigation.defaultProps = { nextSequenceHandler: null, }; -export default injectIntl(SequenceNavigation); +export default SequenceNavigation; diff --git a/src/courseware/course/sequence/sequence-navigation/UnitButton.jsx b/src/courseware/course/sequence/sequence-navigation/UnitButton.jsx index 48d236b861..3c99918963 100644 --- a/src/courseware/course/sequence/sequence-navigation/UnitButton.jsx +++ b/src/courseware/course/sequence/sequence-navigation/UnitButton.jsx @@ -1,5 +1,5 @@ import React, { useCallback } from 'react'; -import { Link } from 'react-router-dom'; +import { Link, useLocation } from 'react-router-dom'; import PropTypes from 'prop-types'; import { connect, useSelector } from 'react-redux'; import classNames from 'classnames'; @@ -22,6 +22,9 @@ const UnitButton = ({ showTitle, }) => { const { courseId, sequenceId } = useSelector(state => state.courseware); + const { pathname } = useLocation(); + const basePath = `/course/${courseId}/${sequenceId}/${unitId}`; + const unitPath = pathname.startsWith('/preview') ? `/preview${basePath}` : basePath; const handleClick = useCallback(() => { onClick(unitId); @@ -37,7 +40,7 @@ const UnitButton = ({ onClick={handleClick} title={title} as={Link} - to={`/course/${courseId}/${sequenceId}/${unitId}`} + to={unitPath} > {showTitle && {title}} diff --git a/src/courseware/course/sequence/sequence-navigation/UnitNavigation.jsx b/src/courseware/course/sequence/sequence-navigation/UnitNavigation.jsx index 9a338d58f0..d27e0acb6f 100644 --- a/src/courseware/course/sequence/sequence-navigation/UnitNavigation.jsx +++ b/src/courseware/course/sequence/sequence-navigation/UnitNavigation.jsx @@ -1,70 +1,53 @@ import classNames from 'classnames'; -import { Link } from 'react-router-dom'; import PropTypes from 'prop-types'; -import { Button } from '@openedx/paragon'; -import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; -import { faChevronLeft, faChevronRight } from '@fortawesome/free-solid-svg-icons'; -import { - injectIntl, intlShape, isRtl, getLocale, -} from '@edx/frontend-platform/i18n'; +import { useIntl } from '@edx/frontend-platform/i18n'; import { useSelector } from 'react-redux'; import { GetCourseExitNavigation } from '../../course-exit'; -import UnitNavigationEffortEstimate from './UnitNavigationEffortEstimate'; import { useSequenceNavigationMetadata } from './hooks'; import messages from './messages'; +import PreviousButton from './generic/PreviousButton'; +import NextButton from './generic/NextButton'; const UnitNavigation = ({ - intl, sequenceId, unitId, onClickPrevious, onClickNext, isAtTop, }) => { + const intl = useIntl(); const { isFirstUnit, isLastUnit, nextLink, previousLink, } = useSequenceNavigationMetadata(sequenceId, unitId); const { courseId } = useSelector(state => state.courseware); - const renderPreviousButton = () => { - const disabled = isFirstUnit; - const prevArrow = isRtl(getLocale()) ? faChevronRight : faChevronLeft; - return ( - - ); - }; + const renderPreviousButton = () => ( + + ); const renderNextButton = () => { const { exitActive, exitText } = GetCourseExitNavigation(courseId, intl); const buttonText = (isLastUnit && exitText) ? exitText : intl.formatMessage(messages.nextButton); const disabled = isLastUnit && !exitActive; - const nextArrow = isRtl(getLocale()) ? faChevronLeft : faChevronRight; return ( - + buttonLabel={buttonText} + nextLink={nextLink} + hasEffortEstimate + /> ); }; @@ -77,7 +60,6 @@ const UnitNavigation = ({ }; UnitNavigation.propTypes = { - intl: intlShape.isRequired, sequenceId: PropTypes.string.isRequired, unitId: PropTypes.string, onClickPrevious: PropTypes.func.isRequired, @@ -90,4 +72,4 @@ UnitNavigation.defaultProps = { isAtTop: false, }; -export default injectIntl(UnitNavigation); +export default UnitNavigation; diff --git a/src/courseware/course/sequence/sequence-navigation/generic/NextButton.jsx b/src/courseware/course/sequence/sequence-navigation/generic/NextButton.jsx new file mode 100644 index 0000000000..f0051df077 --- /dev/null +++ b/src/courseware/course/sequence/sequence-navigation/generic/NextButton.jsx @@ -0,0 +1,56 @@ +import PropTypes from 'prop-types'; +import { Link, useLocation } from 'react-router-dom'; +import { Button } from '@openedx/paragon'; +import { ChevronLeft, ChevronRight } from '@openedx/paragon/icons'; +import { isRtl, getLocale } from '@edx/frontend-platform/i18n'; + +import UnitNavigationEffortEstimate from '../UnitNavigationEffortEstimate'; + +const NextButton = ({ + onClick, + buttonLabel, + nextLink, + variant, + buttonStyle, + disabled, + hasEffortEstimate, +}) => { + const nextArrow = isRtl(getLocale()) ? ChevronLeft : ChevronRight; + const { pathname } = useLocation(); + const navLink = pathname.startsWith('/preview') ? `/preview${nextLink}` : nextLink; + const buttonContent = hasEffortEstimate ? ( + + {buttonLabel} + + ) : buttonLabel; + + return ( + + ); +}; + +NextButton.defaultProps = { + hasEffortEstimate: false, +}; + +NextButton.propTypes = { + onClick: PropTypes.func.isRequired, + buttonLabel: PropTypes.string.isRequired, + nextLink: PropTypes.string.isRequired, + variant: PropTypes.string.isRequired, + buttonStyle: PropTypes.string.isRequired, + disabled: PropTypes.bool.isRequired, + hasEffortEstimate: PropTypes.bool, +}; + +export default NextButton; diff --git a/src/courseware/course/sequence/sequence-navigation/generic/PreviousButton.jsx b/src/courseware/course/sequence/sequence-navigation/generic/PreviousButton.jsx new file mode 100644 index 0000000000..c79669c95d --- /dev/null +++ b/src/courseware/course/sequence/sequence-navigation/generic/PreviousButton.jsx @@ -0,0 +1,44 @@ +import PropTypes from 'prop-types'; +import { Link, useLocation } from 'react-router-dom'; +import { Button } from '@openedx/paragon'; +import { ChevronLeft, ChevronRight } from '@openedx/paragon/icons'; +import { isRtl, getLocale } from '@edx/frontend-platform/i18n'; + +const PreviousButton = ({ + onClick, + buttonLabel, + previousLink, + variant, + buttonStyle, + isFirstUnit, +}) => { + const disabled = isFirstUnit; + const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft; + const { pathname } = useLocation(); + const navLink = pathname.startsWith('/preview') ? `/preview${previousLink}` : previousLink; + + return ( + + ); +}; + +PreviousButton.propTypes = { + onClick: PropTypes.func.isRequired, + buttonLabel: PropTypes.string.isRequired, + previousLink: PropTypes.string.isRequired, + variant: PropTypes.string.isRequired, + buttonStyle: PropTypes.string.isRequired, + isFirstUnit: PropTypes.bool.isRequired, +}; + +export default PreviousButton; diff --git a/src/courseware/utils.jsx b/src/courseware/utils.jsx index 2c4c337996..df91c4e01b 100644 --- a/src/courseware/utils.jsx +++ b/src/courseware/utils.jsx @@ -1,17 +1,21 @@ import React from 'react'; -import { useNavigate, useParams } from 'react-router-dom'; +import { useLocation, useNavigate, useParams } from 'react-router-dom'; const withParamsAndNavigation = WrappedComponent => { const WithParamsNavigationComponent = props => { const { courseId, sequenceId, unitId } = useParams(); const navigate = useNavigate(); + const { pathname } = useLocation(); + const isPreview = pathname.startsWith('/preview'); + return ( ); From 501da9dd5e87efbdd6d822e17d7e86dbe8eec0e5 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Thu, 24 Oct 2024 14:51:31 -0400 Subject: [PATCH 2/8] feat: add tests for course link redirects --- src/constants.ts | 1 + src/courseware/CoursewareContainer.jsx | 81 +- src/courseware/CoursewareContainer.test.jsx | 810 +++++++++++++++++++- src/courseware/data/api.js | 6 +- 4 files changed, 855 insertions(+), 43 deletions(-) diff --git a/src/constants.ts b/src/constants.ts index 544e042ad0..484d84530d 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -14,6 +14,7 @@ export const DECODE_ROUTES = { '/course/:courseId/:sequenceId', '/course/:courseId', '/preview/course/:courseId/:sequenceId/:unitId', + '/preview/course/:courseId/:sequenceId', ], REDIRECT_HOME: 'home/:courseId', REDIRECT_SURVEY: 'survey/:courseId', diff --git a/src/courseware/CoursewareContainer.jsx b/src/courseware/CoursewareContainer.jsx index fe96652263..309b72daf8 100644 --- a/src/courseware/CoursewareContainer.jsx +++ b/src/courseware/CoursewareContainer.jsx @@ -19,24 +19,26 @@ import { handleNextSectionCelebration } from './course/celebration'; import withParamsAndNavigation from './utils'; // Look at where this is called in componentDidUpdate for more info about its usage -const checkResumeRedirect = memoize((courseStatus, courseId, sequenceId, firstSequenceId, navigate, isPreview) => { - if (courseStatus === 'loaded' && !sequenceId) { - // Note that getResumeBlock is just an API call, not a redux thunk. - getResumeBlock(courseId).then((data) => { - // This is a replace because we don't want this change saved in the browser's history. - if (data.sectionId && data.unitId) { - const baseUrl = `/course/${courseId}/${data.sectionId}`; - const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; - navigate(`${sequenceUrl}/${data.unitId}`, { replace: true }); - } else if (firstSequenceId) { - navigate(`/course/${courseId}/${firstSequenceId}`, { replace: true }); - } - }); - } -}); +export const checkResumeRedirect = memoize( + (courseStatus, courseId, sequenceId, firstSequenceId, navigate, isPreview) => { + if (courseStatus === 'loaded' && !sequenceId) { + // Note that getResumeBlock is just an API call, not a redux thunk. + getResumeBlock(courseId).then((data) => { + // This is a replace because we don't want this change saved in the browser's history. + if (data.sectionId && data.unitId) { + const baseUrl = `/course/${courseId}/${data.sectionId}`; + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + navigate(`${sequenceUrl}/${data.unitId}`, { replace: true }); + } else if (firstSequenceId) { + navigate(`/course/${courseId}/${firstSequenceId}`, { replace: true }); + } + }, () => {}); + } + }, +); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSectionUnitToUnitRedirect = memoize(( +export const checkSectionUnitToUnitRedirect = memoize(( courseStatus, courseId, sequenceStatus, @@ -53,20 +55,22 @@ const checkSectionUnitToUnitRedirect = memoize(( }); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSectionToSequenceRedirect = memoize((courseStatus, courseId, sequenceStatus, section, unitId, navigate) => { - if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && !unitId) { - // If the section is non-empty, redirect to its first sequence. - if (section.sequenceIds && section.sequenceIds[0]) { - navigate(`/course/${courseId}/${section.sequenceIds[0]}`, { replace: true }); - // Otherwise, just go to the course root, letting the resume redirect take care of things. - } else { - navigate(`/course/${courseId}`, { replace: true }); +export const checkSectionToSequenceRedirect = memoize( + (courseStatus, courseId, sequenceStatus, section, unitId, navigate) => { + if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && !unitId) { + // If the section is non-empty, redirect to its first sequence. + if (section.sequenceIds && section.sequenceIds[0]) { + navigate(`/course/${courseId}/${section.sequenceIds[0]}`, { replace: true }); + // Otherwise, just go to the course root, letting the resume redirect take care of things. + } else { + navigate(`/course/${courseId}`, { replace: true }); + } } - } -}); + }, +); // Look at where this is called in componentDidUpdate for more info about its usage -const checkUnitToSequenceUnitRedirect = memoize(( +export const checkUnitToSequenceUnitRedirect = memoize(( courseStatus, courseId, sequenceStatus, @@ -104,7 +108,7 @@ const checkUnitToSequenceUnitRedirect = memoize(( }); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSequenceToSequenceUnitRedirect = memoize( +export const checkSequenceToSequenceUnitRedirect = memoize( (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { if (sequenceStatus === 'loaded' && sequence.id && !unitId) { if (sequence.unitIds !== undefined && sequence.unitIds.length > 0) { @@ -119,32 +123,27 @@ const checkSequenceToSequenceUnitRedirect = memoize( ); // Look at where this is called in componentDidUpdate for more info about its usage -const checkSequenceUnitMarkerToSequenceUnitRedirect = memoize( +export const checkSequenceUnitMarkerToSequenceUnitRedirect = memoize( (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { if (sequenceStatus !== 'loaded' || !sequence.id) { return; } const baseUrl = `/course/${courseId}/${sequence.id}`; - const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; const hasUnits = sequence.unitIds?.length > 0; - if (unitId === 'first') { - if (hasUnits) { + if (hasUnits) { + const sequenceUrl = isPreview ? `/preview${baseUrl}` : baseUrl; + if (unitId === 'first') { const firstUnitId = sequence.unitIds[0]; navigate(`${sequenceUrl}/${firstUnitId}`, { replace: true }); - } else { - // No units... go to general sequence page - navigate(baseUrl, { replace: true }); - } - } else if (unitId === 'last') { - if (hasUnits) { + } else if (unitId === 'last') { const lastUnitId = sequence.unitIds[sequence.unitIds.length - 1]; navigate(`${sequenceUrl}/${lastUnitId}`, { replace: true }); - } else { - // No units... go to general sequence page - navigate(baseUrl, { replace: true }); } + } else { + // No units... go to general sequence page + navigate(baseUrl, { replace: true }); } }, ); diff --git a/src/courseware/CoursewareContainer.test.jsx b/src/courseware/CoursewareContainer.test.jsx index 1db3514d82..a48b4d052d 100644 --- a/src/courseware/CoursewareContainer.test.jsx +++ b/src/courseware/CoursewareContainer.test.jsx @@ -16,11 +16,19 @@ import tabMessages from '../tab-page/messages'; import { initializeMockApp, waitFor } from '../setupTest'; import { DECODE_ROUTES } from '../constants'; -import CoursewareContainer from './CoursewareContainer'; +import CoursewareContainer, { + checkResumeRedirect, + checkSectionToSequenceRedirect, + checkSectionUnitToUnitRedirect, + checkSequenceToSequenceUnitRedirect, + checkSequenceUnitMarkerToSequenceUnitRedirect, + checkUnitToSequenceUnitRedirect, +} from './CoursewareContainer'; import { buildSimpleCourseBlocks, buildBinaryCourseBlocks } from '../shared/data/__factories__/courseBlocks.factory'; import initializeStore from '../store'; import { appendBrowserTimezoneToUrl } from '../utils'; import { buildOutlineFromBlocks } from './data/__factories__/learningSequencesOutline.factory'; +import { getSequenceForUnitDeprecatedUrl } from './data/api'; // NOTE: Because the unit creates an iframe, we choose to mock it out as its rendering isn't // pertinent to this test. Instead, we render a simple div that displays the properties we expect @@ -525,3 +533,803 @@ describe('CoursewareContainer', () => { }); }); }); + +describe('Course redirect functions', () => { + let navigate; + let axiosMock; + + beforeEach(() => { + navigate = jest.fn(); + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + }); + + describe('isPreview equals true', () => { + describe('checkSequenceUnitMarkerToSequenceUnitRedirect', () => { + it('return when sequence is not loaded', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loading', + { id: 'sequence_1', unitIds: ['unit_1'] }, + 'first', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('return when sequence id is null', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: null, unitIds: ['unit_1'] }, + 'first', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('calls navigate with first unit id', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'] }, + 'first', + navigate, + true, + ); + const expectedUrl = '/preview/course/courseId/sequence_1/unit_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('calls navigate with last unit id', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'] }, + 'last', + navigate, + true, + ); + const expectedUrl = '/preview/course/courseId/sequence_1/unit_2'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + describe('checkSequenceToSequenceUnitRedirect', () => { + it('calls navigate with next unit id', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'], activeUnitIndex: 0 }, + null, + navigate, + true, + ); + const expectedUrl = '/preview/course/courseId/sequence_1/unit_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('returns when sequence status is loading', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loading', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'], activeUnitIndex: 0 }, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequence id is null', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loading', + { unitIds: ['unit_1', 'unit_2'], activeUnitIndex: 0 }, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when unit id is defined', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'], activeUnitIndex: 0 }, + 'unit_2', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when unit ids are undefiend', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', activeUnitIndex: 0 }, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe('checkUnitToSequenceUnitRedirect', () => { + const apiUrl = getSequenceForUnitDeprecatedUrl('courseId'); + + it('calls navigate with parentId and sequenceId', () => { + axiosMock.onGet(apiUrl).reply(200, { + parent: { id: 'sequence_1' }, + }); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate to course page when getSequenceForUnitDeprecated errors', () => { + const getSequenceForUnitDeprecated = jest.fn(); + axiosMock.onGet(apiUrl).reply(404); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId'; + + waitFor(() => { + expect(getSequenceForUnitDeprecated).toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate to course page when no parent id is returned', () => { + const getSequenceForUnitDeprecated = jest.fn(); + axiosMock.onGet(apiUrl).reply(200, { + parent: { children: ['block_1'] }, + }); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId'; + + waitFor(() => { + expect(getSequenceForUnitDeprecated).toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('returns when course status is loading', () => { + checkUnitToSequenceUnitRedirect( + 'loading', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequence status is not failed', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when section is defined', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + true, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when routeUnitId is defined', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + false, + 'unit_1', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe('checkSectionUnitToUnitRedirect', () => { + it('calls navigate with unitId', () => { + checkSectionUnitToUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_2', + navigate, + true, + ); + const expectedUrl = '/preview/course/courseId/unit_2'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('returns when course status is loading', () => { + checkSectionUnitToUnitRedirect( + 'loading', + 'courseId', + 'loaded', + true, + 'unit_2', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequence status is loading', () => { + checkSectionUnitToUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_2', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when section is null', () => { + checkSectionUnitToUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + null, + 'unit_2', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when unitId is null', () => { + checkSectionUnitToUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe('checkResumeRedirect', () => { + it('calls navigate with unitId', () => { + axiosMock.onGet( + `${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`, + ).reply(200, { + section_id: 'section_1', + unitId: 'unit_1', + }); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + true, + ); + const expectedUrl = '/preview/course/courseId/section_1/unit_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate with firstSequenceId', () => { + axiosMock.onGet( + `${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`, + ).reply(200, { + section_id: 'section_1', + first_sequence_id: 'sequence_1', + }); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + true, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('returns after calling getResumeBlock', () => { + const getResumeBlock = jest.fn(); + axiosMock.onGet( + `${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`, + ).reply(200, { + course_id: 'courseId', + }); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + true, + ); + + waitFor(() => { + expect(getResumeBlock).toHaveBeenCalled(); + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + it('returns when course status is loading', () => { + checkResumeRedirect( + 'loading', + 'courseId', + null, + 'sequence_1', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequenceId is defined', () => { + checkResumeRedirect( + 'loaded', + 'courseId', + 'sequence_3', + 'sequence_1', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when getResumeBlock throws error', () => { + const getResumeBlock = jest.fn(); + axiosMock.onGet(`${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`).reply(404); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + true, + ); + + waitFor(() => { + expect(getResumeBlock).toHaveBeenCalled(); + expect(navigate).not.toHaveBeenCalled(); + }); + }); + }); + }); + + describe('isPreview equals false', () => { + describe('checkSectionToSequenceRedirect', () => { + it('calls navigate with section based sequence id', () => { + checkSectionToSequenceRedirect( + 'loaded', + 'courseId', + 'failed', + { sequenceIds: ['sequence_1'] }, + null, + navigate, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('calls navigate with course id only', () => { + checkSectionToSequenceRedirect( + 'loaded', + 'courseId', + 'failed', + { sequenceIds: [] }, + null, + navigate, + ); + const expectedUrl = '/course/courseId'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('returns when course status is loading', () => { + checkSectionToSequenceRedirect( + 'loading', + 'courseId', + 'failed', + { sequenceIds: [] }, + null, + navigate, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequence status is not failed', () => { + checkSectionToSequenceRedirect( + 'loaded', + 'courseId', + 'loading', + { sequenceIds: [] }, + null, + navigate, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when section is not defined', () => { + checkSectionToSequenceRedirect( + 'loaded', + 'courseId', + 'failed', + null, + null, + navigate, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when unitId is defined', () => { + checkSectionToSequenceRedirect( + 'loaded', + 'courseId', + 'failed', + null, + 'unit_1', + navigate, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe('checkResumeRedirect', () => { + it('calls navigate with unitId', () => { + axiosMock.onGet( + `${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`, + ).reply(200, { + section_id: 'section_1', + unitId: 'unit_1', + }); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + false, + ); + const expectedUrl = '/preview/course/courseId/section_1/unit_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate with firstSequenceId', () => { + axiosMock.onGet( + `${getConfig().LMS_BASE_URL}/api/courseware/resume/courseId`, + ).reply(200, { + section_id: 'section_1', + first_sequence_id: 'sequence_1', + }); + checkResumeRedirect( + 'loaded', + 'courseId', + null, + 'sequence_1', + navigate, + false, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + }); + + describe('checkSequenceUnitMarkerToSequenceUnitRedirect', () => { + it('calls navigate with first unit id', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'] }, + 'first', + navigate, + false, + ); + const expectedUrl = '/course/courseId/sequence_1/unit_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('calls navigate with base url when no unit id', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: [] }, + 'first', + navigate, + true, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + + it('calls navigate with last unit id', () => { + checkSequenceUnitMarkerToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'] }, + 'last', + navigate, + false, + ); + const expectedUrl = '/course/courseId/sequence_1/unit_2'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + describe('checkSequenceToSequenceUnitRedirect', () => { + it('calls navigate with next unit id', () => { + checkSequenceToSequenceUnitRedirect( + 'courseId', + 'loaded', + { id: 'sequence_1', unitIds: ['unit_1', 'unit_2'], activeUnitIndex: 0 }, + null, + navigate, + false, + ); + const expectedUrl = '/course/courseId/sequence_1/unit_1'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + describe('checkUnitToSequenceUnitRedirect', () => { + const apiUrl = getSequenceForUnitDeprecatedUrl('courseId'); + + it('calls navigate with parentId and sequenceId', () => { + axiosMock.onGet(apiUrl).reply(200, { + parent: { id: 'sequence_1' }, + }); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId/sequence_1'; + + waitFor(() => { + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate to course page when getSequenceForUnitDeprecated errors', () => { + const getSequenceForUnitDeprecated = jest.fn(); + axiosMock.onGet(apiUrl).reply(404); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId'; + + waitFor(() => { + expect(getSequenceForUnitDeprecated).toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('calls navigate to course page when no parent id is returned', () => { + const getSequenceForUnitDeprecated = jest.fn(); + axiosMock.onGet(apiUrl).reply(200, { + parent: { children: ['block_1'] }, + }); + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId'; + + waitFor(() => { + expect(getSequenceForUnitDeprecated).toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + + it('returns when course status is loading', () => { + checkUnitToSequenceUnitRedirect( + 'loading', + 'courseId', + 'failed', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when sequence status is not failed', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + false, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when section is defined', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + true, + null, + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + + it('returns when routeUnitId is defined', () => { + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'loaded', + true, + 'unit_1', + false, + 'unit_1', + navigate, + true, + ); + + expect(navigate).not.toHaveBeenCalled(); + }); + }); + + describe('checkSectionUnitToUnitRedirect', () => { + it('calls navigate with unitId', () => { + checkSectionUnitToUnitRedirect( + 'loaded', + 'courseId', + 'failed', + true, + 'unit_2', + navigate, + false, + ); + const expectedUrl = '/course/courseId/unit_2'; + + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + }); +}); diff --git a/src/courseware/data/api.js b/src/courseware/data/api.js index 1b1492973a..b3b58baa76 100644 --- a/src/courseware/data/api.js +++ b/src/courseware/data/api.js @@ -6,7 +6,7 @@ import { } from './utils'; // Do not add further calls to this API - we don't like making use of the modulestore if we can help it -export async function getSequenceForUnitDeprecated(courseId, unitId) { +export const getSequenceForUnitDeprecatedUrl = (courseId) => { const authenticatedUser = getAuthenticatedUser(); const url = new URL(`${getConfig().LMS_BASE_URL}/api/courses/v2/blocks/`); url.searchParams.append('course_id', courseId); @@ -14,6 +14,10 @@ export async function getSequenceForUnitDeprecated(courseId, unitId) { url.searchParams.append('depth', 3); url.searchParams.append('requested_fields', 'children,discussions_url'); + return url; +}; +export async function getSequenceForUnitDeprecated(courseId, unitId) { + const url = getSequenceForUnitDeprecatedUrl(courseId); const { data } = await getAuthenticatedHttpClient().get(url.href, {}); const parent = Object.values(data.blocks).find(block => block.type === 'sequential' && block.children.includes(unitId)); return parent?.id; From a4b20c7696fdca3ce473da641c559f1e874a154a Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Fri, 25 Oct 2024 09:22:40 -0400 Subject: [PATCH 3/8] fix: course redirect unit to sequnce unit redirect --- src/courseware/CoursewareContainer.test.jsx | 33 +++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/courseware/CoursewareContainer.test.jsx b/src/courseware/CoursewareContainer.test.jsx index a48b4d052d..4ae40acf1d 100644 --- a/src/courseware/CoursewareContainer.test.jsx +++ b/src/courseware/CoursewareContainer.test.jsx @@ -673,8 +673,13 @@ describe('Course redirect functions', () => { it('calls navigate with parentId and sequenceId', () => { axiosMock.onGet(apiUrl).reply(200, { - parent: { id: 'sequence_1' }, + blocks: { + id: 'sequence_1', + type: 'seuenctial', + children: ['unit_1'], + }, }); + checkUnitToSequenceUnitRedirect( 'loaded', 'courseId', @@ -696,6 +701,7 @@ describe('Course redirect functions', () => { it('calls navigate to course page when getSequenceForUnitDeprecated errors', () => { const getSequenceForUnitDeprecated = jest.fn(); axiosMock.onGet(apiUrl).reply(404); + checkUnitToSequenceUnitRedirect( 'loaded', 'courseId', @@ -718,8 +724,9 @@ describe('Course redirect functions', () => { it('calls navigate to course page when no parent id is returned', () => { const getSequenceForUnitDeprecated = jest.fn(); axiosMock.onGet(apiUrl).reply(200, { - parent: { children: ['block_1'] }, + block: { type: 'sequential', children: ['block_1'] }, }); + checkUnitToSequenceUnitRedirect( 'loaded', 'courseId', @@ -739,6 +746,28 @@ describe('Course redirect functions', () => { }); }); + it('calls navigate to course page when sequnce is not unit', () => { + const getSequenceForUnitDeprecated = jest.fn(); + + checkUnitToSequenceUnitRedirect( + 'loaded', + 'courseId', + 'failed', + false, + 'unit_1', + false, + null, + navigate, + true, + ); + const expectedUrl = '/course/courseId'; + + waitFor(() => { + expect(getSequenceForUnitDeprecated).not.toHaveBeenCalled(); + expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); + }); + }); + it('returns when course status is loading', () => { checkUnitToSequenceUnitRedirect( 'loading', From 81590b7bc30ccad3e6529fc16b33991ccca4e0ba Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Mon, 28 Oct 2024 09:14:49 -0400 Subject: [PATCH 4/8] fix: test coverage --- src/courseware/CoursewareContainer.test.jsx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/courseware/CoursewareContainer.test.jsx b/src/courseware/CoursewareContainer.test.jsx index 4ae40acf1d..b1012746d1 100644 --- a/src/courseware/CoursewareContainer.test.jsx +++ b/src/courseware/CoursewareContainer.test.jsx @@ -669,15 +669,16 @@ describe('Course redirect functions', () => { }); describe('checkUnitToSequenceUnitRedirect', () => { - const apiUrl = getSequenceForUnitDeprecatedUrl('courseId'); + const { href: apiUrl } = getSequenceForUnitDeprecatedUrl('courseId'); it('calls navigate with parentId and sequenceId', () => { + const getSequenceForUnitDeprecated = jest.fn(); axiosMock.onGet(apiUrl).reply(200, { - blocks: { + blocks: [{ id: 'sequence_1', - type: 'seuenctial', + type: 'sequential', children: ['unit_1'], - }, + }], }); checkUnitToSequenceUnitRedirect( @@ -694,6 +695,7 @@ describe('Course redirect functions', () => { const expectedUrl = '/course/courseId/sequence_1'; waitFor(() => { + expect(getSequenceForUnitDeprecated).toHaveBeenCalled(); expect(navigate).toHaveBeenCalledWith(expectedUrl, { replace: true }); }); }); @@ -724,7 +726,11 @@ describe('Course redirect functions', () => { it('calls navigate to course page when no parent id is returned', () => { const getSequenceForUnitDeprecated = jest.fn(); axiosMock.onGet(apiUrl).reply(200, { - block: { type: 'sequential', children: ['block_1'] }, + blocks: [{ + id: 'sequence_1', + type: 'sequential', + children: ['block_1'], + }], }); checkUnitToSequenceUnitRedirect( From e2f78e48aa02442bcac8602cae2dc8657c9625b3 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 30 Oct 2024 10:09:31 -0400 Subject: [PATCH 5/8] fix: not showing preview when masquerading --- src/courseware/course/Course.jsx | 3 ++- src/courseware/course/sequence/Sequence.jsx | 2 +- src/courseware/course/sequence/SequenceContent.jsx | 6 +++--- src/courseware/course/sequence/Unit/index.jsx | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/courseware/course/Course.jsx b/src/courseware/course/Course.jsx index 3bfd91eca5..a5468fb3f4 100644 --- a/src/courseware/course/Course.jsx +++ b/src/courseware/course/Course.jsx @@ -34,6 +34,7 @@ const Course = ({ celebrations, isStaff, isNewDiscussionSidebarViewEnabled, + originalUserIsStaff, } = useModel('courseHomeMeta', courseId); const sequence = useModel('sequences', sequenceId); const section = useModel('sections', sequence ? sequence.sectionId : null); @@ -42,7 +43,7 @@ const Course = ({ const navigate = useNavigate(); const { pathname } = useLocation(); - if (!isStaff && pathname.startsWith('/preview')) { + if (!originalUserIsStaff && pathname.startsWith('/preview')) { const courseUrl = pathname.replace('/preview', ''); navigate(courseUrl, { replace: true }); } diff --git a/src/courseware/course/sequence/Sequence.jsx b/src/courseware/course/sequence/Sequence.jsx index dce4561bb2..69444eeb53 100644 --- a/src/courseware/course/sequence/Sequence.jsx +++ b/src/courseware/course/sequence/Sequence.jsx @@ -204,7 +204,7 @@ const Sequence = ({ sequenceId={sequenceId} unitId={unitId} unitLoadedHandler={handleUnitLoaded} - isStaff={isStaff} + isOriginalUserStaff={originalUserIsStaff} /> {unitHasLoaded && renderUnitNavigation(false)} diff --git a/src/courseware/course/sequence/SequenceContent.jsx b/src/courseware/course/sequence/SequenceContent.jsx index 6c40c5bfd2..567883f5fe 100644 --- a/src/courseware/course/sequence/SequenceContent.jsx +++ b/src/courseware/course/sequence/SequenceContent.jsx @@ -15,7 +15,7 @@ const SequenceContent = ({ sequenceId, unitId, unitLoadedHandler, - isStaff, + isOriginalUserStaff, }) => { const intl = useIntl(); const sequence = useModel('sequences', sequenceId); @@ -60,7 +60,7 @@ const SequenceContent = ({ key={unitId} id={unitId} onLoaded={unitLoadedHandler} - isStaff={isStaff} + isOriginalUserStaff={isOriginalUserStaff} /> ); }; @@ -71,7 +71,7 @@ SequenceContent.propTypes = { sequenceId: PropTypes.string.isRequired, unitId: PropTypes.string, unitLoadedHandler: PropTypes.func.isRequired, - isStaff: PropTypes.bool.isRequired, + isOriginalUserStaff: PropTypes.bool.isRequired, }; SequenceContent.defaultProps = { diff --git a/src/courseware/course/sequence/Unit/index.jsx b/src/courseware/course/sequence/Unit/index.jsx index 71663cadc9..4933dedf99 100644 --- a/src/courseware/course/sequence/Unit/index.jsx +++ b/src/courseware/course/sequence/Unit/index.jsx @@ -22,7 +22,7 @@ const Unit = ({ format, onLoaded, id, - isStaff, + isOriginalUserStaff, }) => { const { formatMessage } = useIntl(); const [searchParams] = useSearchParams(); @@ -33,7 +33,7 @@ const Unit = ({ const unit = useModel(modelKeys.units, id); const isProcessing = unit.bookmarkedUpdateState === 'loading'; const view = authenticatedUser ? views.student : views.public; - const shouldDisplayUnitPreview = pathname.startsWith('/preview') && isStaff; + const shouldDisplayUnitPreview = pathname.startsWith('/preview') && isOriginalUserStaff; const getUrl = usePluginsCallback('getIFrameUrl', () => getIFrameUrl({ id, @@ -78,7 +78,7 @@ Unit.propTypes = { format: PropTypes.string, id: PropTypes.string.isRequired, onLoaded: PropTypes.func, - isStaff: PropTypes.bool.isRequired, + isOriginalUserStaff: PropTypes.bool.isRequired, }; Unit.defaultProps = { From 2192c87cbbe1362065b6505ec27b71c8ac239417 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 30 Oct 2024 10:10:20 -0400 Subject: [PATCH 6/8] feat: in preview fetch draft branch of sequence metadata --- src/courseware/CoursewareContainer.jsx | 2 +- src/courseware/data/api.js | 4 ++-- src/courseware/data/thunks.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/courseware/CoursewareContainer.jsx b/src/courseware/CoursewareContainer.jsx index 309b72daf8..41aa01f227 100644 --- a/src/courseware/CoursewareContainer.jsx +++ b/src/courseware/CoursewareContainer.jsx @@ -168,7 +168,7 @@ class CoursewareContainer extends Component { checkFetchSequence = memoize((sequenceId) => { if (sequenceId) { - this.props.fetchSequence(sequenceId); + this.props.fetchSequence(sequenceId, this.props.isPreview); } }); diff --git a/src/courseware/data/api.js b/src/courseware/data/api.js index b3b58baa76..199aa37cbc 100644 --- a/src/courseware/data/api.js +++ b/src/courseware/data/api.js @@ -36,9 +36,9 @@ export async function getCourseMetadata(courseId) { return normalizeMetadata(metadata); } -export async function getSequenceMetadata(sequenceId) { +export async function getSequenceMetadata(sequenceId, params) { const { data } = await getAuthenticatedHttpClient() - .get(`${getConfig().LMS_BASE_URL}/api/courseware/sequence/${sequenceId}`, {}); + .get(`${getConfig().LMS_BASE_URL}/api/courseware/sequence/${sequenceId}`, { params }); return normalizeSequenceMetadata(data); } diff --git a/src/courseware/data/thunks.js b/src/courseware/data/thunks.js index 7222d357bf..3f84fbf221 100644 --- a/src/courseware/data/thunks.js +++ b/src/courseware/data/thunks.js @@ -133,11 +133,11 @@ export function fetchCourse(courseId) { }; } -export function fetchSequence(sequenceId) { +export function fetchSequence(sequenceId, isPreview) { return async (dispatch) => { dispatch(fetchSequenceRequest({ sequenceId })); try { - const { sequence, units } = await getSequenceMetadata(sequenceId); + const { sequence, units } = await getSequenceMetadata(sequenceId, { preview: isPreview ? '1' : '0' }); if (sequence.blockType !== 'sequential') { // Some other block types (particularly 'chapter') can be returned // by this API. We want to error in that case, since downstream From 8a5a7fcc7313b1502912b043409a0a0c66d3aa26 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 31 Oct 2024 15:48:24 +0530 Subject: [PATCH 7/8] feat: option to show/hide ungraded assignment in progress page (#1380) * feat: option to show/hide ungraded assignment in progress page test: add tests for show ungraded toggle feat: update score label in progress page based on grading refactor: update score label text and add tooltip refactor: move label tooltip near header as normal text refactor: update problem score label Graded scores * refactor: move config check to utils --- .env | 1 + .env.development | 1 + .env.test | 1 + .../progress-tab/ProgressTab.test.jsx | 117 +++++++++++++++++- .../grades/detailed-grades/DetailedGrades.jsx | 27 +++- .../detailed-grades/DetailedGradesTable.jsx | 8 +- .../detailed-grades/ProblemScoreDrawer.jsx | 10 +- .../progress-tab/grades/messages.ts | 16 ++- src/course-home/progress-tab/utils.ts | 7 ++ src/index.jsx | 1 + 10 files changed, 176 insertions(+), 13 deletions(-) create mode 100644 src/course-home/progress-tab/utils.ts diff --git a/.env b/.env index 697910497c..917bdea634 100644 --- a/.env +++ b/.env @@ -48,3 +48,4 @@ TWITTER_HASHTAG='' TWITTER_URL='' USER_INFO_COOKIE_NAME='' OPTIMIZELY_FULL_STACK_SDK_KEY='' +SHOW_UNGRADED_ASSIGNMENT_PROGRESS='' diff --git a/.env.development b/.env.development index a4bceccdbf..56541aa1d4 100644 --- a/.env.development +++ b/.env.development @@ -50,3 +50,4 @@ SESSION_COOKIE_DOMAIN='localhost' CHAT_RESPONSE_URL='http://localhost:18000/api/learning_assistant/v1/course_id' PRIVACY_POLICY_URL='http://localhost:18000/privacy' OPTIMIZELY_FULL_STACK_SDK_KEY='' +SHOW_UNGRADED_ASSIGNMENT_PROGRESS='' diff --git a/.env.test b/.env.test index cb4d0e0fa2..faa4b4ffbe 100644 --- a/.env.test +++ b/.env.test @@ -47,3 +47,4 @@ TWITTER_HASHTAG='myedxjourney' TWITTER_URL='https://twitter.com/edXOnline' USER_INFO_COOKIE_NAME='edx-user-info' PRIVACY_POLICY_URL='http://localhost:18000/privacy' +SHOW_UNGRADED_ASSIGNMENT_PROGRESS='' diff --git a/src/course-home/progress-tab/ProgressTab.test.jsx b/src/course-home/progress-tab/ProgressTab.test.jsx index a69426c3bd..1a9b15408d 100644 --- a/src/course-home/progress-tab/ProgressTab.test.jsx +++ b/src/course-home/progress-tab/ProgressTab.test.jsx @@ -1,6 +1,6 @@ import React from 'react'; import { Factory } from 'rosie'; -import { getConfig } from '@edx/frontend-platform'; +import { getConfig, setConfig } from '@edx/frontend-platform'; import { sendTrackEvent } from '@edx/frontend-platform/analytics'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; import { breakpoints } from '@openedx/paragon'; @@ -545,6 +545,111 @@ describe('Progress Tab', () => { await fetchAndRender(); expect(screen.getByText('Grades & Credit')).toBeInTheDocument(); }); + + it('does not render ungraded subsections when SHOW_UNGRADED_ASSIGNMENT_PROGRESS is false', async () => { + // The second assignment has has_graded_assignment set to false, so it should not be shown. + setTabData({ + section_scores: [ + { + display_name: 'First section', + subsections: [ + { + assignment_type: 'Homework', + block_key: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@12345', + display_name: 'First subsection', + learner_has_access: true, + has_graded_assignment: true, + num_points_earned: 1, + num_points_possible: 2, + percent_graded: 1.0, + show_correctness: 'always', + show_grades: true, + url: 'http://learning.edx.org/course/course-v1:edX+Test+run/first_subsection', + }, + ], + }, + { + display_name: 'Second section', + subsections: [ + { + assignment_type: 'Homework', + display_name: 'Second subsection', + learner_has_access: true, + has_graded_assignment: false, + num_points_earned: 1, + num_points_possible: 1, + percent_graded: 1.0, + show_correctness: 'always', + show_grades: true, + url: 'http://learning.edx.org/course/course-v1:edX+Test+run/second_subsection', + }, + ], + }, + ], + }); + + await fetchAndRender(); + expect(screen.getByText('First subsection')).toBeInTheDocument(); + expect(screen.queryByText('Second subsection')).not.toBeInTheDocument(); + }); + + it('renders both graded and ungraded subsections when SHOW_UNGRADED_ASSIGNMENT_PROGRESS is true', async () => { + // The second assignment has has_graded_assignment set to false. + setConfig({ + ...getConfig(), + SHOW_UNGRADED_ASSIGNMENT_PROGRESS: true, + }); + + setTabData({ + section_scores: [ + { + display_name: 'First section', + subsections: [ + { + assignment_type: 'Homework', + block_key: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@12345', + display_name: 'First subsection', + learner_has_access: true, + has_graded_assignment: true, + num_points_earned: 1, + num_points_possible: 2, + percent_graded: 1.0, + show_correctness: 'always', + show_grades: true, + url: 'http://learning.edx.org/course/course-v1:edX+Test+run/first_subsection', + }, + ], + }, + { + display_name: 'Second section', + subsections: [ + { + assignment_type: 'Homework', + display_name: 'Second subsection', + learner_has_access: true, + has_graded_assignment: false, + num_points_earned: 1, + num_points_possible: 1, + percent_graded: 1.0, + show_correctness: 'always', + show_grades: true, + url: 'http://learning.edx.org/course/course-v1:edX+Test+run/second_subsection', + }, + ], + }, + ], + }); + + await fetchAndRender(); + expect(screen.getByText('First subsection')).toBeInTheDocument(); + expect(screen.getByText('Second subsection')).toBeInTheDocument(); + + // reset config for other tests + setConfig({ + ...getConfig(), + SHOW_UNGRADED_ASSIGNMENT_PROGRESS: false, + }); + }); }); describe('Grade Summary', () => { @@ -809,7 +914,7 @@ describe('Progress Tab', () => { // Open the problem score drawer fireEvent.click(problemScoreDrawerToggle); - expect(screen.getByText('Problem Scores:')).toBeInTheDocument(); + expect(screen.getAllByText('Graded Scores:').length).toBeGreaterThan(1); expect(screen.getAllByText('0/1')).toHaveLength(3); }); @@ -821,6 +926,14 @@ describe('Progress Tab', () => { expect(screen.getByText('Detailed grades')).toBeInTheDocument(); expect(screen.getByText('You currently have no graded problem scores.')).toBeInTheDocument(); }); + + it('renders Detailed Grades table when section scores are populated', async () => { + await fetchAndRender(); + expect(screen.getByText('Detailed grades')).toBeInTheDocument(); + + expect(screen.getByText('First subsection')); + expect(screen.getByText('Second subsection')); + }); }); describe('Certificate Status', () => { diff --git a/src/course-home/progress-tab/grades/detailed-grades/DetailedGrades.jsx b/src/course-home/progress-tab/grades/detailed-grades/DetailedGrades.jsx index 40fb569c51..529859c52b 100644 --- a/src/course-home/progress-tab/grades/detailed-grades/DetailedGrades.jsx +++ b/src/course-home/progress-tab/grades/detailed-grades/DetailedGrades.jsx @@ -7,6 +7,7 @@ import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/ import { Blocked } from '@openedx/paragon/icons'; import { Icon, Hyperlink } from '@openedx/paragon'; import { useModel } from '../../../../generic/model-store'; +import { showUngradedAssignments } from '../../utils'; import DetailedGradesTable from './DetailedGradesTable'; @@ -28,6 +29,8 @@ const DetailedGrades = ({ intl }) => { } = useModel('progress', courseId); const hasSectionScores = sectionScores.length > 0; + const emptyTableMsg = showUngradedAssignments() + ? messages.detailedGradesEmpty : messages.detailedGradesEmptyOnlyGraded; const logOutlineLinkClick = () => { sendTrackEvent('edx.ui.lms.course_progress.detailed_grades.course_outline_link.clicked', { @@ -54,7 +57,25 @@ const DetailedGrades = ({ intl }) => { return (
-

{intl.formatMessage(messages.detailedGrades)}

+

{intl.formatMessage(messages.detailedGrades)}

+
    +
  • + {intl.formatMessage(messages.practiceScoreLabel)} + +
    • +
  • + {intl.formatMessage(messages.gradedScoreLabel)} + +
    • +
{gradesFeatureIsPartiallyLocked && (
@@ -65,9 +86,9 @@ const DetailedGrades = ({ intl }) => { )} {!hasSectionScores && ( -

{intl.formatMessage(messages.detailedGradesEmpty)}

+

{intl.formatMessage(emptyTableMsg)}

)} - {overviewTabUrl && ( + {overviewTabUrl && !showUngradedAssignments() && (

{ const { @@ -24,9 +25,10 @@ const DetailedGradesTable = ({ intl }) => { sectionScores.map((chapter) => { const subsectionScores = chapter.subsections.filter( (subsection) => !!( - subsection.hasGradedAssignment - && subsection.showGrades - && (subsection.numPointsPossible > 0 || subsection.numPointsEarned > 0)), + (showUngradedAssignments() || subsection.hasGradedAssignment) + && subsection.showGrades + && (subsection.numPointsPossible > 0 || subsection.numPointsEarned > 0) + ), ); if (subsectionScores.length === 0) { diff --git a/src/course-home/progress-tab/grades/detailed-grades/ProblemScoreDrawer.jsx b/src/course-home/progress-tab/grades/detailed-grades/ProblemScoreDrawer.jsx index 7970821aaf..9088c39f03 100644 --- a/src/course-home/progress-tab/grades/detailed-grades/ProblemScoreDrawer.jsx +++ b/src/course-home/progress-tab/grades/detailed-grades/ProblemScoreDrawer.jsx @@ -10,9 +10,12 @@ import messages from '../messages'; const ProblemScoreDrawer = ({ intl, problemScores, subsection }) => { const isLocaleRtl = isRtl(getLocale()); + + const scoreLabel = subsection.hasGradedAssignment ? messages.gradedScoreLabel : messages.practiceScoreLabel; + return ( - {intl.formatMessage(messages.problemScoreLabel)} + {intl.formatMessage(scoreLabel)}

    {problemScores.map((problemScore, i) => ( @@ -31,7 +34,10 @@ ProblemScoreDrawer.propTypes = { earned: PropTypes.number.isRequired, possible: PropTypes.number.isRequired, })).isRequired, - subsection: PropTypes.shape({ learnerHasAccess: PropTypes.bool }).isRequired, + subsection: PropTypes.shape({ + learnerHasAccess: PropTypes.bool, + hasGradedAssignment: PropTypes.bool, + }).isRequired, }; export default injectIntl(ProblemScoreDrawer); diff --git a/src/course-home/progress-tab/grades/messages.ts b/src/course-home/progress-tab/grades/messages.ts index 72a6624b2b..fb57809852 100644 --- a/src/course-home/progress-tab/grades/messages.ts +++ b/src/course-home/progress-tab/grades/messages.ts @@ -91,9 +91,14 @@ const messages = defineMessages({ defaultMessage: 'Detailed grades', description: 'Headline for the (detailed grade) section in the progress tab', }, - detailedGradesEmpty: { + detailedGradesEmptyOnlyGraded: { id: 'progress.detailedGrades.emptyTable', defaultMessage: 'You currently have no graded problem scores.', + description: 'It indicate that there are no graded problem or assignments to be scored', + }, + detailedGradesEmpty: { + id: 'progress.detailedGrades.including-ungraded.emptyTable', + defaultMessage: 'You currently have no graded or ungraded problem scores.', description: 'It indicate that there are no problem or assignments to be scored', }, footnotesTitle: { @@ -158,11 +163,16 @@ const messages = defineMessages({ defaultMessage: 'Passing grade', description: 'Label for mark on the (grade bar) chart which indicate the poisition of passing grade on the bar', }, - problemScoreLabel: { + gradedScoreLabel: { id: 'progress.detailedGrades.problemScore.label', - defaultMessage: 'Problem Scores:', + defaultMessage: 'Graded Scores:', description: 'Label text which precedes detailed view of all scores per assignment', }, + practiceScoreLabel: { + id: 'progress.detailedGrades.practice.problemScore.label', + defaultMessage: 'Practice Scores:', + description: 'Label text which precedes detailed view of all ungraded problem scores per assignment', + }, problemScoreToggleAltText: { id: 'progress.detailedGrades.problemScore.toggleButton', defaultMessage: 'Toggle individual problem scores for {subsectionTitle}', diff --git a/src/course-home/progress-tab/utils.ts b/src/course-home/progress-tab/utils.ts new file mode 100644 index 0000000000..29dd42de85 --- /dev/null +++ b/src/course-home/progress-tab/utils.ts @@ -0,0 +1,7 @@ +import { getConfig } from '@edx/frontend-platform'; + +/* eslint-disable import/prefer-default-export */ +export const showUngradedAssignments = () => ( + getConfig().SHOW_UNGRADED_ASSIGNMENT_PROGRESS === 'true' + || getConfig().SHOW_UNGRADED_ASSIGNMENT_PROGRESS === true +); diff --git a/src/index.jsx b/src/index.jsx index 9680c134d4..ec4f4dc284 100755 --- a/src/index.jsx +++ b/src/index.jsx @@ -173,6 +173,7 @@ initialize({ PROCTORED_EXAM_RULES_URL: process.env.PROCTORED_EXAM_RULES_URL || null, CHAT_RESPONSE_URL: process.env.CHAT_RESPONSE_URL || null, PRIVACY_POLICY_URL: process.env.PRIVACY_POLICY_URL || null, + SHOW_UNGRADED_ASSIGNMENT_PROGRESS: process.env.SHOW_UNGRADED_ASSIGNMENT_PROGRESS || false, }, 'LearnerAppConfig'); }, }, From 4c9143b2b4825ebd167ff05c067f128447ee0394 Mon Sep 17 00:00:00 2001 From: Bilal Qamar <59555732+BilalQamar95@users.noreply.github.com> Date: Fri, 1 Nov 2024 00:35:17 +0500 Subject: [PATCH 8/8] test: Remove support for Node 18 (#1454) * test: Remove support for Node 18 * chore: update code coverage artifact naming --- .github/workflows/validate.yml | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 5feb86ad88..8634743f6d 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -9,21 +9,16 @@ on: jobs: tests: runs-on: ubuntu-latest - strategy: - matrix: - node: [18, 20] steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: ${{ matrix.node }} + node-version-file: '.nvmrc' - run: make validate.ci - name: Archive code coverage results uses: actions/upload-artifact@v4 with: - name: code-coverage-report-${{ matrix.node }} - # When we're only using Node 20, replace the line above with the following: - # name: code-coverage-report + name: code-coverage-report path: coverage/*.* coverage: runs-on: ubuntu-latest @@ -33,9 +28,7 @@ jobs: - name: Download code coverage results uses: actions/download-artifact@v4 with: - name: code-coverage-report-20 - # When we're only using Node 20, replace the line above with the following: - # name: code-coverage-report + name: code-coverage-report - name: Upload coverage uses: codecov/codecov-action@v4 with: