From c7bef2f721b879e05a4239c72b7744c8d440a8c3 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Thu, 20 Aug 2026 01:33:56 -0400 Subject: [PATCH] refactor: de-class CoursewareContainer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert CoursewareContainer from a class + connect component to a functional component using hooks. Structural only — no data-layer or behavior change: it dispatches the same thunks and reads the same store. This unblocks the later courseware React Query peels, which need a function component to call query hooks a class can't. - class -> function; connect -> useSelector/useDispatch - inline the route hooks (useParams/useNavigate/useLocation) and delete the withParamsAndNavigation HOC (utils.jsx), its only consumer - keep the reselect memoize fetch/save guards (held in refs, reading a per-render latest ref) inside one no-dependency effect, matching the old componentDidMount + componentDidUpdate exactly - drop the dead previousSequence selector/prop (unused; keep the empty previousSequenceHandler that Course still requires) - guard useIFrameBehavior's activeSequence.unitIds read: removing connect lets the global getSequenceId briefly lead the rendered sequenceId prop, so the sequence model can transiently be the outline stub (no unitIds) - drop dead fetchCourseRecommendations{Request,Success,Failure} exports (orphaned from the recommendations RQ conversion in #1967) Co-Authored-By: Claude Opus 4.8 --- src/courseware/CoursewareContainer.jsx | 397 +++++++----------- src/courseware/CoursewareContainer.test.jsx | 59 ++- .../sequence/Unit/hooks/useIFrameBehavior.ts | 2 +- src/courseware/data/slice.js | 3 - src/courseware/utils.jsx | 26 -- 5 files changed, 201 insertions(+), 286 deletions(-) delete mode 100644 src/courseware/utils.jsx diff --git a/src/courseware/CoursewareContainer.jsx b/src/courseware/CoursewareContainer.jsx index a715a7b955..a0e351ef1f 100644 --- a/src/courseware/CoursewareContainer.jsx +++ b/src/courseware/CoursewareContainer.jsx @@ -1,6 +1,6 @@ -import React, { Component } from 'react'; -import PropTypes from 'prop-types'; -import { connect } from 'react-redux'; +import { useEffect, useRef } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { useLocation, useNavigate, useParams } from 'react-router-dom'; import { createSelector } from '@reduxjs/toolkit'; import { defaultMemoize as memoize } from 'reselect'; @@ -16,9 +16,8 @@ import { TabPage } from '../tab-page'; import Course from './course'; import { handleNextSectionCelebration } from './course/celebration'; -import withParamsAndNavigation from './utils'; -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkResumeRedirect = memoize( (courseStatus, courseId, sequenceId, firstSequenceId, navigate, isPreview) => { if (courseStatus === 'loaded' && !sequenceId) { @@ -37,7 +36,7 @@ export const checkResumeRedirect = memoize( }, ); -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkSectionUnitToUnitRedirect = memoize(( courseStatus, courseId, @@ -54,7 +53,7 @@ export const checkSectionUnitToUnitRedirect = memoize(( } }); -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkSectionToSequenceRedirect = memoize( (courseStatus, courseId, sequenceStatus, section, unitId, navigate) => { if (courseStatus === 'loaded' && sequenceStatus === 'failed' && section && !unitId) { @@ -69,7 +68,7 @@ export const checkSectionToSequenceRedirect = memoize( }, ); -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkUnitToSequenceUnitRedirect = memoize(( courseStatus, courseId, @@ -107,7 +106,7 @@ export const checkUnitToSequenceUnitRedirect = memoize(( } }); -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkSequenceToSequenceUnitRedirect = memoize( (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { if (sequenceStatus === 'loaded' && sequence.id && !unitId) { @@ -122,7 +121,7 @@ export const checkSequenceToSequenceUnitRedirect = memoize( }, ); -// Look at where this is called in componentDidUpdate for more info about its usage +// Look at where this is called in the render effect for more info about its usage export const checkSequenceUnitMarkerToSequenceUnitRedirect = memoize( (courseId, sequenceStatus, sequence, unitId, navigate, isPreview) => { if (sequenceStatus !== 'loaded' || !sequence.id) { @@ -148,63 +147,135 @@ export const checkSequenceUnitMarkerToSequenceUnitRedirect = memoize( }, ); -class CoursewareContainer extends Component { - checkSaveSequencePosition = memoize((unitId) => { - const { - courseId, - sequenceId, - sequenceStatus, - sequence, - } = this.props; - if (sequenceStatus === 'loaded' && sequence.saveUnitPosition && unitId) { - const activeUnitIndex = sequence.unitIds.indexOf(unitId); - this.props.saveSequencePosition(courseId, sequenceId, activeUnitIndex); +const currentCourseSelector = createSelector( + (state) => state.models.coursewareMeta || {}, + (state) => state.courseware.courseId, + (coursesById, courseId) => (coursesById[courseId] ? coursesById[courseId] : null), +); + +const currentSequenceSelector = createSelector( + (state) => state.models.sequences || {}, + (state) => state.courseware.sequenceId, + (sequencesById, sequenceId) => (sequencesById[sequenceId] ? sequencesById[sequenceId] : null), +); + +const sequenceIdsSelector = createSelector( + (state) => state.courseware.courseStatus, + currentCourseSelector, + (state) => state.models.sections, + (courseStatus, course, sectionsById) => { + if (courseStatus !== 'loaded') { + return []; } - }); + const { sectionIds = [] } = course; + return sectionIds.flatMap(sectionId => sectionsById[sectionId].sequenceIds); + }, +); - checkFetchCourse = memoize((courseId) => { - this.props.fetchCourse(courseId); - }); +const nextSequenceSelector = createSelector( + sequenceIdsSelector, + (state) => state.models.sequences || {}, + (state) => state.courseware.sequenceId, + (sequenceIds, sequencesById, sequenceId) => { + if (!sequenceId || sequenceIds.length === 0) { + return null; + } + const sequenceIndex = sequenceIds.indexOf(sequenceId); + const nextSequenceId = sequenceIndex < sequenceIds.length - 1 ? sequenceIds[sequenceIndex + 1] : null; + return nextSequenceId !== null ? sequencesById[nextSequenceId] : null; + }, +); - checkFetchSequence = memoize((sequenceId) => { - if (sequenceId) { - this.props.fetchSequence(sequenceId, this.props.isPreview); +const firstSequenceIdSelector = createSelector( + (state) => state.courseware.courseStatus, + currentCourseSelector, + (state) => state.models.sections || {}, + (courseStatus, course, sectionsById) => { + if (courseStatus !== 'loaded') { + return null; } - }); + const { sectionIds = [] } = course; - componentDidMount() { - const { - routeCourseId, - routeSequenceId, - } = this.props; - // Load data whenever the course or sequence ID changes. - this.checkFetchCourse(routeCourseId); - this.checkFetchSequence(routeSequenceId); + if (sectionIds.length === 0) { + return null; + } + + return sectionsById[sectionIds[0]].sequenceIds[0]; + }, +); + +const sectionViaSequenceIdSelector = createSelector( + (state) => state.models.sections || {}, + (state) => state.courseware.sequenceId, + (sectionsById, sequenceId) => (sectionsById[sequenceId] ? sectionsById[sequenceId] : null), +); + +const CoursewareContainer = () => { + const dispatch = useDispatch(); + const navigate = useNavigate(); + const { pathname } = useLocation(); + const { + courseId: routeCourseId, + sequenceId: routeSequenceId, + unitId: routeUnitId, + } = useParams(); + const isPreview = pathname.startsWith('/preview'); + + const courseId = useSelector((state) => state.courseware.courseId); + const sequenceId = useSelector((state) => state.courseware.sequenceId); + const courseStatus = useSelector((state) => state.courseware.courseStatus); + const sequenceStatus = useSelector((state) => state.courseware.sequenceStatus); + const sequenceMightBeUnit = useSelector((state) => state.courseware.sequenceMightBeUnit); + const course = useSelector(currentCourseSelector); + const sequence = useSelector(currentSequenceSelector); + const nextSequence = useSelector(nextSequenceSelector); + const firstSequenceId = useSelector(firstSequenceIdSelector); + const sectionViaSequenceId = useSelector(sectionViaSequenceIdSelector); + + const latest = useRef(); + + const guards = useRef(); + if (!guards.current) { + guards.current = { + checkFetchCourse: memoize((id) => { + dispatch(fetchCourse(id)); + }), + checkFetchSequence: memoize((id) => { + if (id) { + dispatch(fetchSequence(id, latest.current.isPreview)); + } + }), + checkSaveSequencePosition: memoize((unitId) => { + const { + courseId: cId, + sequenceId: sId, + sequenceStatus: sStatus, + sequence: seq, + } = latest.current; + if (sStatus === 'loaded' && seq.saveUnitPosition && unitId) { + const activeUnitIndex = seq.unitIds.indexOf(unitId); + dispatch(saveSequencePosition(cId, sId, activeUnitIndex)); + } + }), + }; } - componentDidUpdate() { - const { + useEffect(() => { + latest.current = { courseId, sequenceId, - courseStatus, sequenceStatus, - sequenceMightBeUnit, sequence, - firstSequenceId, - sectionViaSequenceId, - routeCourseId, - routeSequenceId, - routeUnitId, - navigate, isPreview, - } = this.props; + }; + const { checkFetchCourse, checkFetchSequence, checkSaveSequencePosition } = guards.current; // Load data whenever the course or sequence ID changes. - this.checkFetchCourse(routeCourseId); - this.checkFetchSequence(routeSequenceId); + checkFetchCourse(routeCourseId); + checkFetchSequence(routeSequenceId); // Check if we should save our sequence position. Only do this when the route unit ID changes. - this.checkSaveSequencePosition(routeUnitId); + checkSaveSequencePosition(routeUnitId); // Coerce the route ids into null here because they can be undefined, but the redux ids would be null instead. if (courseId !== (routeCourseId || null) || sequenceId !== (routeSequenceId || null)) { @@ -301,26 +372,13 @@ class CoursewareContainer extends Component { navigate, isPreview, ); - } - - handleUnitNavigationClick = () => { - const { - courseId, - sequenceId, - routeUnitId, - } = this.props; + }); - this.props.checkBlockCompletion(courseId, sequenceId, routeUnitId); + const handleUnitNavigationClick = () => { + dispatch(checkBlockCompletion(courseId, sequenceId, routeUnitId)); }; - handleNextSequenceClick = () => { - const { - course, - nextSequence, - sequence, - sequenceId, - } = this.props; - + const handleNextSequenceClick = () => { if (nextSequence !== null) { const celebrateFirstSection = course && course.celebrations && course.celebrations.firstSection; if (celebrateFirstSection && sequence.sectionId !== nextSequence.sectionId) { @@ -329,194 +387,25 @@ class CoursewareContainer extends Component { } }; - handlePreviousSequenceClick = () => {}; + const handlePreviousSequenceClick = () => {}; - render() { - const { - courseStatus, - courseId, - sequenceId, - routeUnitId, - } = this.props; - - return ( - + - - - ); - } -} - -const sequenceShape = PropTypes.shape({ - id: PropTypes.string.isRequired, - unitIds: PropTypes.arrayOf(PropTypes.string), - sectionId: PropTypes.string.isRequired, - saveUnitPosition: PropTypes.any, // eslint-disable-line -}); - -const sectionShape = PropTypes.shape({ - id: PropTypes.string.isRequired, - sequenceIds: PropTypes.arrayOf(PropTypes.string).isRequired, -}); - -const courseShape = PropTypes.shape({ - celebrations: PropTypes.shape({ - firstSection: PropTypes.bool, - }), -}); - -CoursewareContainer.propTypes = { - routeCourseId: PropTypes.string.isRequired, - routeSequenceId: PropTypes.string, - routeUnitId: PropTypes.string, - courseId: PropTypes.string, - sequenceId: PropTypes.string, - firstSequenceId: PropTypes.string, - courseStatus: PropTypes.oneOf(['loaded', 'loading', 'failed', 'denied']).isRequired, - sequenceStatus: PropTypes.oneOf(['loaded', 'loading', 'failed']).isRequired, - sequenceMightBeUnit: PropTypes.bool.isRequired, - nextSequence: sequenceShape, - previousSequence: sequenceShape, - sectionViaSequenceId: sectionShape, - course: courseShape, - sequence: sequenceShape, - saveSequencePosition: PropTypes.func.isRequired, - checkBlockCompletion: PropTypes.func.isRequired, - fetchCourse: PropTypes.func.isRequired, - fetchSequence: PropTypes.func.isRequired, - navigate: PropTypes.func.isRequired, - isPreview: PropTypes.bool.isRequired, -}; - -CoursewareContainer.defaultProps = { - courseId: null, - sequenceId: null, - routeSequenceId: null, - routeUnitId: null, - firstSequenceId: null, - nextSequence: null, - previousSequence: null, - sectionViaSequenceId: null, - course: null, - sequence: null, -}; - -const currentCourseSelector = createSelector( - (state) => state.models.coursewareMeta || {}, - (state) => state.courseware.courseId, - (coursesById, courseId) => (coursesById[courseId] ? coursesById[courseId] : null), -); - -const currentSequenceSelector = createSelector( - (state) => state.models.sequences || {}, - (state) => state.courseware.sequenceId, - (sequencesById, sequenceId) => (sequencesById[sequenceId] ? sequencesById[sequenceId] : null), -); - -const sequenceIdsSelector = createSelector( - (state) => state.courseware.courseStatus, - currentCourseSelector, - (state) => state.models.sections, - (courseStatus, course, sectionsById) => { - if (courseStatus !== 'loaded') { - return []; - } - const { sectionIds = [] } = course; - return sectionIds.flatMap(sectionId => sectionsById[sectionId].sequenceIds); - }, -); - -const previousSequenceSelector = createSelector( - sequenceIdsSelector, - (state) => state.models.sequences || {}, - (state) => state.courseware.sequenceId, - (sequenceIds, sequencesById, sequenceId) => { - if (!sequenceId || sequenceIds.length === 0) { - return null; - } - const sequenceIndex = sequenceIds.indexOf(sequenceId); - const previousSequenceId = sequenceIndex > 0 ? sequenceIds[sequenceIndex - 1] : null; - return previousSequenceId !== null ? sequencesById[previousSequenceId] : null; - }, -); - -const nextSequenceSelector = createSelector( - sequenceIdsSelector, - (state) => state.models.sequences || {}, - (state) => state.courseware.sequenceId, - (sequenceIds, sequencesById, sequenceId) => { - if (!sequenceId || sequenceIds.length === 0) { - return null; - } - const sequenceIndex = sequenceIds.indexOf(sequenceId); - const nextSequenceId = sequenceIndex < sequenceIds.length - 1 ? sequenceIds[sequenceIndex + 1] : null; - return nextSequenceId !== null ? sequencesById[nextSequenceId] : null; - }, -); - -const firstSequenceIdSelector = createSelector( - (state) => state.courseware.courseStatus, - currentCourseSelector, - (state) => state.models.sections || {}, - (courseStatus, course, sectionsById) => { - if (courseStatus !== 'loaded') { - return null; - } - const { sectionIds = [] } = course; - - if (sectionIds.length === 0) { - return null; - } - - return sectionsById[sectionIds[0]].sequenceIds[0]; - }, -); - -const sectionViaSequenceIdSelector = createSelector( - (state) => state.models.sections || {}, - (state) => state.courseware.sequenceId, - (sectionsById, sequenceId) => (sectionsById[sequenceId] ? sectionsById[sequenceId] : null), -); - -const mapStateToProps = (state) => { - const { - courseId, - sequenceId, - courseStatus, - sequenceStatus, - sequenceMightBeUnit, - } = state.courseware; - - return { - courseId, - sequenceId, - courseStatus, - sequenceStatus, - sequenceMightBeUnit, - course: currentCourseSelector(state), - sequence: currentSequenceSelector(state), - previousSequence: previousSequenceSelector(state), - nextSequence: nextSequenceSelector(state), - firstSequenceId: firstSequenceIdSelector(state), - sectionViaSequenceId: sectionViaSequenceIdSelector(state), - }; + nextSequenceHandler={handleNextSequenceClick} + previousSequenceHandler={handlePreviousSequenceClick} + unitNavigationHandler={handleUnitNavigationClick} + /> + + ); }; -export default connect(mapStateToProps, { - checkBlockCompletion, - saveSequencePosition, - fetchCourse, - fetchSequence, -})(withParamsAndNavigation(CoursewareContainer)); +export default CoursewareContainer; diff --git a/src/courseware/CoursewareContainer.test.jsx b/src/courseware/CoursewareContainer.test.jsx index 8e00d693c9..815ed898d9 100644 --- a/src/courseware/CoursewareContainer.test.jsx +++ b/src/courseware/CoursewareContainer.test.jsx @@ -5,6 +5,7 @@ import { QueryClientProvider } from '@tanstack/react-query'; import { waitForElementToBeRemoved } from '@testing-library/dom'; import '@testing-library/jest-dom'; import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import React from 'react'; import { BrowserRouter, MemoryRouter, Route, Routes, @@ -38,11 +39,17 @@ import { getSequenceForUnitDeprecatedUrl } from './data/api'; // proves that the component is rendered and receives the correct props. We probably COULD render // Unit.jsx and its iframe in this test, but it's already complex enough. +let mockRenderUnitNav = false; jest.mock( './course/sequence/Unit', // eslint-disable-next-line react/prop-types - () => function ({ courseId, id }) { - return
Unit Contents {courseId} {id}
; + () => function ({ courseId, id, renderUnitNavigation }) { + return ( +
+ Unit Contents {courseId} {id} + {mockRenderUnitNav && renderUnitNavigation ? renderUnitNavigation() : null} +
+ ); }, ); @@ -113,6 +120,10 @@ describe('CoursewareContainer', () => { ); }); + afterEach(() => { + mockRenderUnitNav = false; + }); + function setUpMockRequests(options = {}) { // If we weren't given course blocks or metadata, use the defaults. const courseBlocks = options.courseBlocks || defaultCourseBlocks; @@ -391,6 +402,50 @@ describe('CoursewareContainer', () => { expect(screen.getByTestId('org.openedx.frontend.learning.sequence_navigation.v1')).toBeInTheDocument(); }); + + it('marks the current unit complete when navigating to the next unit', async () => { + const completionUrl = `${getConfig().LMS_BASE_URL}/courses/${courseId}/xblock/${sequenceBlock.id}/handler/get_completion`; + axiosMock.onPost(completionUrl).reply(200, { complete: true }); + + mockRenderUnitNav = true; + history.push(`/course/${courseId}/${sequenceBlock.id}/${unitBlocks[0].id}`); + await loadContainer(); + + const user = userEvent.setup(); + await user.click(screen.getByRole('link', { name: /next/i })); + + await waitFor(() => { + expect(axiosMock.history.post.filter(request => request.url === completionUrl)).toHaveLength(1); + }); + }); + }); + }); + + describe('when the course has no sections', () => { + it('does not resume-redirect (there is no first sequence to pick)', async () => { + global.scrollTo = jest.fn(); + const courseId = defaultCourseId; + const emptyCourseBlocks = { + courseId, + title: defaultCourseHomeMetadata.title, + blocks: { + [courseId]: { + id: courseId, + type: 'course', + display_name: defaultCourseHomeMetadata.title, + children: [], + }, + }, + root: courseId, + }; + setUpMockRequests({ courseBlocks: emptyCourseBlocks }); + axiosMock.onGet(`${getConfig().LMS_BASE_URL}/api/courseware/resume/${courseId}`).reply(200, {}); + + history.push(`/course/${courseId}`); + const container = await loadContainer(); + + expect(global.location.href).toEqual(`http://localhost/course/${courseId}`); + expect(container.querySelector('.fake-unit')).toBeNull(); }); }); diff --git a/src/courseware/course/sequence/Unit/hooks/useIFrameBehavior.ts b/src/courseware/course/sequence/Unit/hooks/useIFrameBehavior.ts index 91c45c5a59..89b66810ac 100644 --- a/src/courseware/course/sequence/Unit/hooks/useIFrameBehavior.ts +++ b/src/courseware/course/sequence/Unit/hooks/useIFrameBehavior.ts @@ -39,7 +39,7 @@ const useIFrameBehavior = ({ const activeSequenceId = useSelector(getSequenceId); const navigate = useNavigate(); const activeSequence = useModel('sequences', activeSequenceId); - const activeUnitId = activeSequence.unitIds.length > 0 + const activeUnitId = activeSequence.unitIds?.length > 0 ? activeSequence.unitIds[activeSequence.activeUnitIndex] : null; const { isLastUnit, nextLink } = useSequenceNavigationMetadata(activeSequenceId, activeUnitId); diff --git a/src/courseware/data/slice.js b/src/courseware/data/slice.js index d1cb2031ef..90bd82654e 100644 --- a/src/courseware/data/slice.js +++ b/src/courseware/data/slice.js @@ -133,9 +133,6 @@ export const { fetchSequenceRequest, fetchSequenceSuccess, fetchSequenceFailure, - fetchCourseRecommendationsRequest, - fetchCourseRecommendationsSuccess, - fetchCourseRecommendationsFailure, fetchCourseOutlineRequest, fetchCourseOutlineSuccess, fetchCourseOutlineFailure, diff --git a/src/courseware/utils.jsx b/src/courseware/utils.jsx deleted file mode 100644 index df91c4e01b..0000000000 --- a/src/courseware/utils.jsx +++ /dev/null @@ -1,26 +0,0 @@ -import React from 'react'; - -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 ( - - ); - }; - return WithParamsNavigationComponent; -}; - -export default withParamsAndNavigation;