Skip to content

Commit 88674a1

Browse files
ihor-romaniuknsprenkle
ihor-romaniuk
authored and
nsprenkle
committed
feat: remove waffle flags for managing course outline sidebar (openedx#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 11a0a8e commit 88674a1

30 files changed

Lines changed: 185 additions & 230 deletions

‎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.test.jsx‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -210,15 +210,15 @@ describe('Course', () => {
210210
});
211211
});
212212

213-
it('renders course breadcrumbs as expected', async () => {
213+
it('doesn\'t renders course breadcrumbs by default', async () => {
214214
const courseMetadata = Factory.build('courseMetadata');
215215
const unitBlocks = Array.from({ length: 3 }).map(() => Factory.build(
216216
'block',
217217
{ type: 'vertical' },
218218
{ courseId: courseMetadata.id },
219219
));
220220
const testStore = await initializeTestStore({
221-
courseMetadata, unitBlocks, enableNavigationSidebar: { enable_navigation_sidebar: false },
221+
courseMetadata, unitBlocks,
222222
}, false);
223223
const { courseware, models } = testStore.getState();
224224
const { courseId, sequenceId } = courseware;
@@ -234,10 +234,10 @@ describe('Course', () => {
234234
await waitFor(() => {
235235
expect(screen.queryByText('Loading learning sequence...')).not.toBeInTheDocument();
236236
});
237-
// expect the section and sequence "titles" to be loaded in as breadcrumb labels.
238-
waitFor(() => {
239-
expect(screen.findByText(Object.values(models.sections)[0].title)).toBeInTheDocument();
240-
expect(screen.findByText(Object.values(models.sequences)[0].title)).toBeInTheDocument();
237+
// expect the section and sequence "titles" not to be loaded in as breadcrumb labels.
238+
await waitFor(() => {
239+
expect(screen.queryByText(Object.values(models.sections)[0].title)).not.toBeInTheDocument();
240+
expect(screen.queryByText(Object.values(models.sequences)[0].title)).not.toBeInTheDocument();
241241
});
242242
});
243243

‎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)}

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ describe('Sequence', () => {
2424
{ type: 'vertical' },
2525
{ courseId: courseMetadata.id },
2626
));
27-
const enableNavigationSidebar = { enable_navigation_sidebar: false };
2827

2928
beforeAll(async () => {
3029
const store = await initializeTestStore({ courseMetadata, unitBlocks });
@@ -96,7 +95,6 @@ describe('Sequence', () => {
9695
unitBlocks,
9796
sequenceBlocks,
9897
sequenceMetadata,
99-
enableNavigationSidebar: { enable_navigation_sidebar: true },
10098
}, false);
10199
const { container } = render(
102100
<SidebarWrapper overrideData={{ sequenceId: sequenceBlocks[0].id }} />,
@@ -131,7 +129,7 @@ describe('Sequence', () => {
131129
{ courseId: courseMetadata.id, unitBlocks, sequenceBlock: sequenceBlocks[0] },
132130
)];
133131
const testStore = await initializeTestStore({
134-
courseMetadata, unitBlocks, sequenceBlocks, sequenceMetadata, enableNavigationSidebar,
132+
courseMetadata, unitBlocks, sequenceBlocks, sequenceMetadata,
135133
}, false);
136134
render(
137135
<Sequence {...mockData} {...{ sequenceId: sequenceBlocks[0].id }} />,
@@ -190,7 +188,7 @@ describe('Sequence', () => {
190188

191189
beforeAll(async () => {
192190
testStore = await initializeTestStore({
193-
courseMetadata, unitBlocks, sequenceBlocks, enableNavigationSidebar,
191+
courseMetadata, unitBlocks, sequenceBlocks,
194192
}, false);
195193
});
196194

@@ -366,7 +364,6 @@ describe('Sequence', () => {
366364
unitBlocks,
367365
sequenceBlocks: testSequenceBlocks,
368366
sequenceMetadata: testSequenceMetadata,
369-
enableNavigationSidebar,
370367
}, false);
371368
const testData = {
372369
...mockData,

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ const SequenceContent = ({
1616
unitId,
1717
unitLoadedHandler,
1818
isOriginalUserStaff,
19-
isEnabledOutlineSidebar,
2019
renderUnitNavigation,
2120
}) => {
2221
const intl = useIntl();
@@ -63,7 +62,6 @@ const SequenceContent = ({
6362
id={unitId}
6463
onLoaded={unitLoadedHandler}
6564
isOriginalUserStaff={isOriginalUserStaff}
66-
isEnabledOutlineSidebar={isEnabledOutlineSidebar}
6765
renderUnitNavigation={renderUnitNavigation}
6866
/>
6967
);
@@ -76,7 +74,6 @@ SequenceContent.propTypes = {
7674
unitId: PropTypes.string,
7775
unitLoadedHandler: PropTypes.func.isRequired,
7876
isOriginalUserStaff: PropTypes.bool.isRequired,
79-
isEnabledOutlineSidebar: PropTypes.bool.isRequired,
8077
renderUnitNavigation: PropTypes.func.isRequired,
8178
};
8279

‎src/courseware/course/sequence/SequenceContent.test.jsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ describe('Sequence Content', () => {
1515
sequenceId: courseware.sequenceId,
1616
unitId: models.sequences[courseware.sequenceId].unitIds[0],
1717
unitLoadedHandler: () => { },
18+
renderUnitNavigation: () => { },
1819
};
1920
});
2021

@@ -38,7 +39,7 @@ describe('Sequence Content', () => {
3839
});
3940

4041
it('displays message for no content', () => {
41-
render(<SequenceContent {...mockData} unitId={null} />, { wrapWithRouter: true });
42+
render(<SequenceContent {...mockData} unitId="" />, { wrapWithRouter: true });
4243
expect(screen.getByText('There is no content here.')).toBeInTheDocument();
4344
});
4445
});

0 commit comments

Comments
 (0)