Skip to content

Commit 06902d8

Browse files
ihor-romaniukihor-romaniuk
authored
feat: remove waffle flags for managing course outline sidebar (#1713)
* feat: remove waffle flags for managing course outline sidebar * fix: flag and tests * fix: product-tours tests * fix: remove default content for SequenceNavigationSlot and update tests * fix: remove default content for CourseBreadcrumbsSlot * fix: update plugin-slots version and documentation * revert: update plugin-slots version * fix: update tests
1 parent e413464 commit 06902d8

31 files changed

+193
-245
lines changed

‎src/courseware/CoursewareContainer.test.jsx

Lines changed: 18 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { getConfig, history } from '@edx/frontend-platform';
22
import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
33
import { AppProvider } from '@edx/frontend-platform/react';
4-
import { waitForElementToBeRemoved, fireEvent } from '@testing-library/dom';
4+
import { waitForElementToBeRemoved } from '@testing-library/dom';
55
import '@testing-library/jest-dom';
66
import { render, screen } from '@testing-library/react';
77
import React from 'react';
@@ -193,15 +193,13 @@ describe('CoursewareContainer', () => {
193193
expect(courseHeader.querySelector('.course-title')).toHaveTextContent(courseHomeMetadata.title);
194194
}
195195

196-
function assertSequenceNavigation(container, expectedUnitCount = 3) {
197-
// Ensure we had appropriate sequence navigation buttons. We should only have one unit.
196+
function assertNoSequenceNavigation(container) {
198197
const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation a, nav.sequence-navigation button');
199-
expect(sequenceNavButtons).toHaveLength(expectedUnitCount + 2);
198+
expect(sequenceNavButtons).toHaveLength(0);
200199

201-
expect(sequenceNavButtons[0]).toHaveTextContent('Previous');
202-
// Prove this button is rendering an SVG tasks icon, meaning it's a unit/vertical.
203-
expect(sequenceNavButtons[1].querySelector('svg')).toHaveClass('fa-tasks');
204-
expect(sequenceNavButtons[sequenceNavButtons.length - 1]).toHaveTextContent('Next');
200+
expect(container.querySelector('button, a')).not.toHaveTextContent('Previous');
201+
expect(container.querySelector('svg.fa-tasks')).toBeNull();
202+
expect(container.querySelector('button, a')).not.toHaveTextContent('Next');
205203
}
206204

207205
beforeEach(async () => {
@@ -224,7 +222,7 @@ describe('CoursewareContainer', () => {
224222
const container = await loadContainer();
225223

226224
assertLoadedHeader(container);
227-
assertSequenceNavigation(container);
225+
assertNoSequenceNavigation(container);
228226

229227
expect(container.querySelector('.fake-unit')).toHaveTextContent('Unit Contents');
230228
expect(container.querySelector('.fake-unit')).toHaveTextContent(courseId);
@@ -247,7 +245,7 @@ describe('CoursewareContainer', () => {
247245
const container = await loadContainer();
248246

249247
assertLoadedHeader(container);
250-
assertSequenceNavigation(container);
248+
assertNoSequenceNavigation(container);
251249

252250
expect(container.querySelector('.fake-unit')).toHaveTextContent('Unit Contents');
253251
expect(container.querySelector('.fake-unit')).toHaveTextContent(courseId);
@@ -274,29 +272,12 @@ describe('CoursewareContainer', () => {
274272
setUpMockRequests({ courseBlocks });
275273
});
276274

277-
// describe('when the URL contains a unit ID', () => {
278-
// it('should ignore the section ID and redirect based on the unit ID', async () => {
279-
// const urlUnit = unitTree[1][1][1];
280-
// setUrl(sectionTree[1].id, urlUnit.id);
281-
// const container = await loadContainer();
282-
// assertLoadedHeader(container);
283-
// assertSequenceNavigation(container, 2);
284-
// assertLocation(container, sequenceTree[1][1].id, urlUnit.id);
285-
// });
286-
287-
// it('should ignore invalid unit IDs and redirect to the course root', async () => {
288-
// setUrl(sectionTree[1].id, 'foobar');
289-
// await loadContainer();
290-
// expect(global.location.href).toEqual(`http://localhost/course/${courseId}`);
291-
// });
292-
// });
293-
294275
describe('when the URL does not contain a unit ID', () => {
295276
it('should choose a unit within the section\'s first sequence', async () => {
296277
setUrl(sectionTree[1].id);
297278
const container = await loadContainer();
298279
assertLoadedHeader(container);
299-
assertSequenceNavigation(container, 2);
280+
assertNoSequenceNavigation(container);
300281
assertLocation(container, sequenceTree[1][0].id, unitTree[1][0][0].id);
301282
});
302283
});
@@ -342,27 +323,6 @@ describe('CoursewareContainer', () => {
342323
});
343324
});
344325

345-
// describe('when the URL only contains a unit ID', () => {
346-
// const { courseBlocks, unitTree, sequenceTree } = buildBinaryCourseBlocks(courseId, courseMetadata.name);
347-
348-
// beforeEach(async () => {
349-
// setUpMockRequests({ courseBlocks });
350-
// });
351-
352-
// it('should insert the sequence ID into the URL', async () => {
353-
// const unit = unitTree[1][0][1];
354-
// history.push(`/course/${courseId}/${unit.id}`);
355-
// const container = await loadContainer();
356-
357-
// assertLoadedHeader(container);
358-
// assertSequenceNavigation(container, 2);
359-
// const expectedSequenceId = sequenceTree[1][0].id;
360-
// const expectedUrl = `http://localhost/course/${courseId}/${expectedSequenceId}/${unit.id}`;
361-
// expect(global.location.href).toEqual(expectedUrl);
362-
// expect(container.querySelector('.fake-unit')).toHaveTextContent(unit.id);
363-
// });
364-
// });
365-
366326
describe('when the URL contains a course ID and sequence ID', () => {
367327
const sequenceBlock = defaultSequenceBlock;
368328
const unitBlocks = defaultUnitBlocks;
@@ -372,7 +332,7 @@ describe('CoursewareContainer', () => {
372332
const container = await loadContainer();
373333

374334
assertLoadedHeader(container);
375-
assertSequenceNavigation(container);
335+
assertNoSequenceNavigation(container);
376336

377337
expect(container.querySelector('.fake-unit')).toHaveTextContent('Unit Contents');
378338
expect(container.querySelector('.fake-unit')).toHaveTextContent(courseId);
@@ -391,7 +351,7 @@ describe('CoursewareContainer', () => {
391351
const container = await loadContainer();
392352

393353
assertLoadedHeader(container);
394-
assertSequenceNavigation(container);
354+
assertNoSequenceNavigation(container);
395355

396356
expect(container.querySelector('.fake-unit')).toHaveTextContent('Unit Contents');
397357
expect(container.querySelector('.fake-unit')).toHaveTextContent(courseId);
@@ -408,44 +368,24 @@ describe('CoursewareContainer', () => {
408368
const container = await loadContainer();
409369

410370
assertLoadedHeader(container);
411-
assertSequenceNavigation(container);
371+
assertNoSequenceNavigation(container);
412372

413373
expect(container.querySelector('.fake-unit')).toHaveTextContent('Unit Contents');
414374
expect(container.querySelector('.fake-unit')).toHaveTextContent(courseId);
415375
expect(container.querySelector('.fake-unit')).toHaveTextContent(unitBlocks[2].id);
416376
});
417377

418-
it('should navigate between units and check block completion', async () => {
419-
axiosMock.onPost(`${courseId}/xblock/${sequenceBlock.id}/handler/get_completion`).reply(200, {
420-
complete: true,
421-
});
378+
it('should render the sequence_navigation plugin slot correctly', async () => {
379+
axiosMock
380+
.onPost(`${courseId}/xblock/${sequenceBlock.id}/handler/get_completion`)
381+
.reply(200, { complete: true });
422382

423383
history.push(`/course/${courseId}/${sequenceBlock.id}/${unitBlocks[0].id}`);
424-
const container = await loadContainer();
425-
426-
const sequenceNavButtons = container.querySelectorAll('nav.sequence-navigation a, nav.sequence-navigation button');
427-
const sequenceNextButton = sequenceNavButtons[4];
428-
expect(sequenceNextButton).toHaveTextContent('Next');
429-
fireEvent.click(sequenceNextButton);
384+
await loadContainer();
430385

431-
expect(global.location.href).toEqual(`http://localhost/course/${courseId}/${sequenceBlock.id}/${unitBlocks[1].id}`);
386+
expect(screen.getByTestId('org.openedx.frontend.learning.sequence_navigation.v1')).toBeInTheDocument();
432387
});
433388
});
434-
435-
// describe('when the current sequence is an exam', () => {
436-
// const { location } = window;
437-
438-
// beforeEach(() => {
439-
// delete window.location;
440-
// window.location = {
441-
// assign: jest.fn(),
442-
// };
443-
// });
444-
445-
// afterEach(() => {
446-
// window.location = location;
447-
// });
448-
// });
449389
});
450390

451391
describe('when receiving a course_access error_code', () => {

‎src/courseware/course/Course.jsx

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,13 @@
11
import { useEffect, useState } from 'react';
22
import PropTypes from 'prop-types';
33
import { Helmet } from 'react-helmet';
4-
import { useDispatch, useSelector } from 'react-redux';
4+
import { useDispatch } from 'react-redux';
55
import { getConfig } from '@edx/frontend-platform';
66
import { useLocation, useNavigate } from 'react-router-dom';
77
import { breakpoints, useWindowSize } from '@openedx/paragon';
88

99
import { AlertList } from '@src/generic/user-messages';
1010
import { useModel } from '@src/generic/model-store';
11-
import { getCoursewareOutlineSidebarSettings } from '../data/selectors';
1211
import Chat from './chat/Chat';
1312
import SidebarProvider from './sidebar/SidebarContextProvider';
1413
import NewSidebarProvider from './new-sidebar/SidebarContextProvider';
@@ -37,8 +36,6 @@ const Course = ({
3736
} = useModel('courseHomeMeta', courseId);
3837
const sequence = useModel('sequences', sequenceId);
3938
const section = useModel('sections', sequence ? sequence.sectionId : null);
40-
const { enableNavigationSidebar } = useSelector(getCoursewareOutlineSidebarSettings);
41-
const navigationDisabled = enableNavigationSidebar || (sequence?.navigationDisabled ?? false);
4239
const navigate = useNavigate();
4340
const { pathname } = useLocation();
4441

@@ -84,17 +81,13 @@ const Course = ({
8481
<title>{`${pageTitleBreadCrumbs.join(' | ')} | ${getConfig().SITE_NAME}`}</title>
8582
</Helmet>
8683
<div className="position-relative d-flex align-items-xl-center mb-4 mt-1 flex-column flex-xl-row">
87-
{navigationDisabled || (
88-
<>
89-
<CourseBreadcrumbsSlot
90-
courseId={courseId}
91-
sectionId={section ? section.id : null}
92-
sequenceId={sequenceId}
93-
isStaff={isStaff}
94-
unitId={unitId}
95-
/>
96-
</>
97-
)}
84+
<CourseBreadcrumbsSlot
85+
courseId={courseId}
86+
sectionId={section ? section.id : null}
87+
sequenceId={sequenceId}
88+
isStaff={isStaff}
89+
unitId={unitId}
90+
/>
9891
{shouldDisplayChat && (
9992
<>
10093
<Chat

‎src/courseware/course/Course.test.jsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -202,15 +202,15 @@ describe('Course', () => {
202202
});
203203
});
204204

205-
it('renders course breadcrumbs as expected', async () => {
205+
it('doesn\'t renders course breadcrumbs by default', async () => {
206206
const courseMetadata = Factory.build('courseMetadata');
207207
const unitBlocks = Array.from({ length: 3 }).map(() => Factory.build(
208208
'block',
209209
{ type: 'vertical' },
210210
{ courseId: courseMetadata.id },
211211
));
212212
const testStore = await initializeTestStore({
213-
courseMetadata, unitBlocks, enableNavigationSidebar: { enable_navigation_sidebar: false },
213+
courseMetadata, unitBlocks,
214214
}, false);
215215
const { courseware, models } = testStore.getState();
216216
const { courseId, sequenceId } = courseware;
@@ -226,10 +226,10 @@ describe('Course', () => {
226226
await waitFor(() => {
227227
expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument();
228228
});
229-
// expect the section and sequence "titles" to be loaded in as breadcrumb labels.
230-
waitFor(() => {
231-
expect(screen.findByText(Object.values(models.sections)[0].title)).toBeInTheDocument();
232-
expect(screen.findByText(Object.values(models.sequences)[0].title)).toBeInTheDocument();
229+
// expect the section and sequence "titles" not to be loaded in as breadcrumb labels.
230+
await waitFor(() => {
231+
expect(screen.queryByText(Object.values(models.sections)[0].title)).not.toBeInTheDocument();
232+
expect(screen.queryByText(Object.values(models.sequences)[0].title)).not.toBeInTheDocument();
233233
});
234234
});
235235

‎src/courseware/course/sequence/Sequence.jsx

Lines changed: 44 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import { CourseOutlineSidebarTriggerSlot } from '@src/plugin-slots/CourseOutline
1919
import { NotificationsDiscussionsSidebarSlot } from '@src/plugin-slots/NotificationsDiscussionsSidebarSlot';
2020
import SequenceNavigationSlot from '@src/plugin-slots/SequenceNavigationSlot';
2121

22-
import { getCoursewareOutlineSidebarSettings } from '../../data/selectors';
2322
import CourseLicense from '../course-license';
2423
import messages from './messages';
2524
import HiddenAfterDue from './hidden-after-due';
@@ -48,7 +47,7 @@ const Sequence = ({
4847
const unit = useModel('units', unitId);
4948
const sequenceStatus = useSelector(state => state.courseware.sequenceStatus);
5049
const sequenceMightBeUnit = useSelector(state => state.courseware.sequenceMightBeUnit);
51-
const { enableNavigationSidebar: isEnabledOutlineSidebar } = useSelector(getCoursewareOutlineSidebarSettings);
50+
5251
const handleNext = () => {
5352
const nextIndex = sequence.unitIds.indexOf(unitId) + 1;
5453
const newUnitId = sequence.unitIds[nextIndex];
@@ -91,6 +90,30 @@ const Sequence = ({
9190
sendTrackingLogEvent(eventName, payload);
9291
};
9392

93+
/* istanbul ignore next */
94+
const nextHandler = () => {
95+
logEvent('edx.ui.lms.sequence.next_selected', 'top');
96+
handleNext();
97+
};
98+
99+
/* istanbul ignore next */
100+
const previousHandler = () => {
101+
logEvent('edx.ui.lms.sequence.previous_selected', 'top');
102+
handlePrevious();
103+
};
104+
105+
/* istanbul ignore next */
106+
const onNavigate = (destinationUnitId) => {
107+
logEvent('edx.ui.lms.sequence.tab_selected', 'top', destinationUnitId);
108+
handleNavigate(destinationUnitId);
109+
};
110+
111+
const sequenceNavProps = {
112+
nextHandler,
113+
previousHandler,
114+
onNavigate,
115+
};
116+
94117
useSequenceBannerTextAlert(sequenceId);
95118
useSequenceEntranceExamAlert(courseId, sequenceId, intl);
96119

@@ -171,30 +194,25 @@ const Sequence = ({
171194
/>
172195
<CourseOutlineSidebarSlot />
173196
<div className="sequence w-100">
174-
{!isEnabledOutlineSidebar && (
175-
<div className="sequence-navigation-container">
176-
<SequenceNavigationSlot
177-
sequenceId={sequenceId}
178-
unitId={unitId}
179-
nextHandler={() => {
180-
logEvent('edx.ui.lms.sequence.next_selected', 'top');
181-
handleNext();
182-
}}
183-
onNavigate={(destinationUnitId) => {
184-
logEvent('edx.ui.lms.sequence.tab_selected', 'top', destinationUnitId);
185-
handleNavigate(destinationUnitId);
186-
}}
187-
previousHandler={() => {
188-
logEvent('edx.ui.lms.sequence.previous_selected', 'top');
189-
handlePrevious();
190-
}}
191-
{...{
192-
nextSequenceHandler,
193-
handleNavigate,
194-
}}
195-
/>
196-
</div>
197-
)}
197+
<div className="sequence-navigation-container">
198+
{/**
199+
SequenceNavigationSlot renders nothing by default.
200+
However, we still pass nextHandler, previousHandler, and onNavigate,
201+
because, as per the slot's contract, if this slot is replaced
202+
with the default SequenceNavigation component, these props are required.
203+
These handlers are excluded from test coverage via istanbul ignore,
204+
since they are not used unless the slot is overridden.
205+
*/}
206+
<SequenceNavigationSlot
207+
sequenceId={sequenceId}
208+
unitId={unitId}
209+
{...{
210+
...sequenceNavProps,
211+
nextSequenceHandler,
212+
handleNavigate,
213+
}}
214+
/>
215+
</div>
198216

199217
<div className="unit-container flex-grow-1 pt-4">
200218
<SequenceContent
@@ -204,7 +222,6 @@ const Sequence = ({
204222
unitId={unitId}
205223
unitLoadedHandler={handleUnitLoaded}
206224
isOriginalUserStaff={originalUserIsStaff}
207-
isEnabledOutlineSidebar={isEnabledOutlineSidebar}
208225
renderUnitNavigation={renderUnitNavigation}
209226
/>
210227
{unitHasLoaded && renderUnitNavigation(false)}

0 commit comments

Comments
 (0)