From 0cd0b1992534de63743aab45c675309f5e68520f Mon Sep 17 00:00:00 2001 From: Jesse Stewart Date: Mon, 14 Jul 2025 16:42:20 -0400 Subject: [PATCH 1/5] fix: sidebar unitid undefined --- .../sidebar/sidebars/course-outline/hooks.js | 14 ++++++++++++-- src/courseware/data/thunks.js | 4 ++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/courseware/course/sidebar/sidebars/course-outline/hooks.js b/src/courseware/course/sidebar/sidebars/course-outline/hooks.js index 71ed4bff63..1bdb115e0d 100644 --- a/src/courseware/course/sidebar/sidebars/course-outline/hooks.js +++ b/src/courseware/course/sidebar/sidebars/course-outline/hooks.js @@ -74,9 +74,17 @@ export const useCourseOutlineSidebar = () => { const handleUnitClick = ({ sequenceId, activeUnitId, id }) => { const logEvent = (eventName, widgetPlacement) => { - const findSequenceByUnitId = () => Object.values(sequences).find(seq => seq.unitIds.includes(activeUnitId)); + const findSequenceByUnitId = (searchUnitId) => { + if (!searchUnitId) { + return null; + } + return Object.values(sequences).find(seq => seq.unitIds.includes(searchUnitId)); + }; const activeSequence = findSequenceByUnitId(activeUnitId); const targetSequence = findSequenceByUnitId(id); + if (!activeSequence || !targetSequence) { + return; + } const payload = { id: activeUnitId, current_tab: activeSequence.unitIds.indexOf(activeUnitId) + 1, @@ -95,7 +103,9 @@ export const useCourseOutlineSidebar = () => { }; logEvent('edx.ui.lms.sequence.tab_selected', 'left'); - dispatch(checkBlockCompletion(courseId, sequenceId, activeUnitId)); + if (activeUnitId) { + dispatch(checkBlockCompletion(courseId, sequenceId, activeUnitId)); + } // Hide the sidebar after selecting a unit on a mobile device. if (shouldDisplayFullScreen) { diff --git a/src/courseware/data/thunks.js b/src/courseware/data/thunks.js index 15d36f7251..8703a99b20 100644 --- a/src/courseware/data/thunks.js +++ b/src/courseware/data/thunks.js @@ -176,6 +176,10 @@ export function fetchSequence(sequenceId, isPreview) { export function checkBlockCompletion(courseId, sequenceId, unitId) { return async (dispatch, getState) => { + if (!unitId) { + return {}; + } + const { models } = getState(); if (models.units[unitId]?.complete) { return {}; // do nothing. Things don't get uncompleted after they are completed. From 9ce622f393c7a7fad7cca1f65e4db327d59c425f Mon Sep 17 00:00:00 2001 From: Jesse Stewart Date: Tue, 15 Jul 2025 10:18:43 -0400 Subject: [PATCH 2/5] fix: remove trailing space --- src/courseware/data/thunks.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/courseware/data/thunks.js b/src/courseware/data/thunks.js index 8703a99b20..d275a8232b 100644 --- a/src/courseware/data/thunks.js +++ b/src/courseware/data/thunks.js @@ -179,7 +179,6 @@ export function checkBlockCompletion(courseId, sequenceId, unitId) { if (!unitId) { return {}; } - const { models } = getState(); if (models.units[unitId]?.complete) { return {}; // do nothing. Things don't get uncompleted after they are completed. From 02ade79ac9b10cec789819bbc1219a57af9f684d Mon Sep 17 00:00:00 2001 From: Jesse Stewart Date: Tue, 15 Jul 2025 10:19:17 -0400 Subject: [PATCH 3/5] fix: remove trailing space --- src/courseware/data/thunks.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/courseware/data/thunks.js b/src/courseware/data/thunks.js index d275a8232b..2bb76e4a9e 100644 --- a/src/courseware/data/thunks.js +++ b/src/courseware/data/thunks.js @@ -179,6 +179,7 @@ export function checkBlockCompletion(courseId, sequenceId, unitId) { if (!unitId) { return {}; } + const { models } = getState(); if (models.units[unitId]?.complete) { return {}; // do nothing. Things don't get uncompleted after they are completed. From 86c2b64d3179d7fcb1eba357aef7fe4841bbafe3 Mon Sep 17 00:00:00 2001 From: Jesse Stewart Date: Tue, 15 Jul 2025 13:29:15 -0400 Subject: [PATCH 4/5] feat: update test coverage --- .../components/SidebarUnit.test.jsx | 77 +++++++++++++++++++ src/courseware/data/redux.test.js | 51 ++++++++++++ 2 files changed, 128 insertions(+) diff --git a/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx b/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx index 496c4a9cf1..ec22422e0f 100644 --- a/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx +++ b/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx @@ -98,6 +98,83 @@ describe('', () => { expect(screen.getByText(unit.title)).toBeInTheDocument(); }); + describe('handleUnitClick coverage tests', () => { + it('should handle click when activeUnitId is null/undefined (covers findSequenceByUnitId return null)', async () => { + const user = userEvent.setup(); + await initTestStore(); + + // Render with activeUnitId as null to trigger the findSequenceByUnitId return null + renderWithProvider({ + activeUnitId: null, + unit: { ...unit } + }); + + // Click should not cause errors and should not send tracking events + await user.click(screen.getByText(unit.title)); + + // Since activeSequence will be null, tracking events should not be sent + expect(sendTrackEvent).not.toHaveBeenCalled(); + expect(sendTrackingLogEvent).not.toHaveBeenCalled(); + }); + + it('should handle click when activeUnitId is undefined (covers findSequenceByUnitId return null)', async () => { + const user = userEvent.setup(); + await initTestStore(); + + // Render with activeUnitId as undefined to trigger the findSequenceByUnitId return null + renderWithProvider({ + activeUnitId: undefined, + unit: { ...unit } + }); + + await user.click(screen.getByText(unit.title)); + + // Since activeSequence will be null, tracking events should not be sent + expect(sendTrackEvent).not.toHaveBeenCalled(); + expect(sendTrackingLogEvent).not.toHaveBeenCalled(); + }); + + it('should handle click when unit id is not found in any sequence (covers early return)', async () => { + const user = userEvent.setup(); + await initTestStore(); + + // Use a unit ID that doesn't exist in any sequence + const nonExistentUnitId = 'non-existent-unit-id'; + + renderWithProvider({ + unit: { ...unit, id: nonExistentUnitId }, + id: nonExistentUnitId, + activeUnitId: unit.id // valid activeUnitId but invalid target unit + }); + + await user.click(screen.getByText(unit.title)); + + // Since targetSequence will be null, tracking events should not be sent + expect(sendTrackEvent).not.toHaveBeenCalled(); + expect(sendTrackingLogEvent).not.toHaveBeenCalled(); + }); + + it('should handle click when both activeUnitId and target unit id are invalid', async () => { + const user = userEvent.setup(); + await initTestStore(); + + const nonExistentUnitId = 'non-existent-unit-id'; + const anotherNonExistentUnitId = 'another-non-existent-unit-id'; + + renderWithProvider({ + unit: { ...unit, id: nonExistentUnitId }, + id: nonExistentUnitId, + activeUnitId: anotherNonExistentUnitId + }); + + await user.click(screen.getByText(unit.title)); + + // Both sequences will be null, so tracking events should not be sent + expect(sendTrackEvent).not.toHaveBeenCalled(); + expect(sendTrackingLogEvent).not.toHaveBeenCalled(); + }); + }); + describe('When a unit is clicked', () => { it('sends log event correctly', async () => { const user = userEvent.setup(); diff --git a/src/courseware/data/redux.test.js b/src/courseware/data/redux.test.js index 91b6e210e7..4a7e10cbe6 100644 --- a/src/courseware/data/redux.test.js +++ b/src/courseware/data/redux.test.js @@ -13,6 +13,7 @@ import { buildSimpleCourseBlocks } from '../../shared/data/__factories__/courseB import { buildOutlineFromBlocks } from './__factories__/learningSequencesOutline.factory'; import { initializeMockApp } from '../../setupTest'; import initializeStore from '../../store'; +import { updateModel } from '../../generic/model-store'; const { loggingService } = initializeMockApp(); @@ -351,6 +352,56 @@ describe('Data layer integration tests', () => { expect(store.getState().courseware.courseOutline.sequences[sequence.id].complete).not.toBeTruthy(); expect(store.getState().courseware.courseOutline.sections[section.id].complete).not.toBeTruthy(); }); + + it('Should return empty object when unitId is not provided', async () => { + // Test the first return {} - when unitId is falsy + const thunk = thunks.checkBlockCompletion(courseId, sequenceId, null); + const result = await thunk(store.dispatch, store.getState); + + expect(result).toEqual({}); + }); + + it('Should return empty object when unitId is undefined', async () => { + // Test the first return {} - when unitId is undefined + const thunk = thunks.checkBlockCompletion(courseId, sequenceId, undefined); + const result = await thunk(store.dispatch, store.getState); + + expect(result).toEqual({}); + }); + + it('Should return empty object when unitId is empty string', async () => { + // Test the first return {} - when unitId is empty string + const thunk = thunks.checkBlockCompletion(courseId, sequenceId, ''); + const result = await thunk(store.dispatch, store.getState); + + expect(result).toEqual({}); + }); + + it('Should return empty object when unit is already complete', async () => { + // First, set up the state so that the unit is already marked as complete + const testUnitId = 'test-unit-id'; + + // Dispatch an action to mark the unit as complete in the store + store.dispatch(updateModel({ + modelType: 'units', + model: { + id: testUnitId, + complete: true, + }, + })); + + // Record the current number of API calls before our test + const initialRequestCount = axiosMock.history.post.length; + + // Now call checkBlockCompletion - it should return {} without making API calls + const thunk = thunks.checkBlockCompletion(courseId, sequenceId, testUnitId); + const result = await thunk(store.dispatch, store.getState); + + expect(result).toEqual({}); + + // Verify that no new API calls were made (since it should return early) + expect(axiosMock.history.post.length).toBe(initialRequestCount); + }); }); describe('Test saveSequencePosition', () => { From 5806489af6ce9a5a6dcaa83beee406b411505378 Mon Sep 17 00:00:00 2001 From: Jesse Stewart Date: Tue, 15 Jul 2025 13:33:32 -0400 Subject: [PATCH 5/5] chore: lint files --- .../components/SidebarUnit.test.jsx | 28 +++++++++---------- src/courseware/data/redux.test.js | 18 ++++++------ 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx b/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx index ec22422e0f..e443438e86 100644 --- a/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx +++ b/src/courseware/course/sidebar/sidebars/course-outline/components/SidebarUnit.test.jsx @@ -102,11 +102,11 @@ describe('', () => { it('should handle click when activeUnitId is null/undefined (covers findSequenceByUnitId return null)', async () => { const user = userEvent.setup(); await initTestStore(); - + // Render with activeUnitId as null to trigger the findSequenceByUnitId return null - renderWithProvider({ + renderWithProvider({ activeUnitId: null, - unit: { ...unit } + unit: { ...unit }, }); // Click should not cause errors and should not send tracking events @@ -120,11 +120,11 @@ describe('', () => { it('should handle click when activeUnitId is undefined (covers findSequenceByUnitId return null)', async () => { const user = userEvent.setup(); await initTestStore(); - + // Render with activeUnitId as undefined to trigger the findSequenceByUnitId return null - renderWithProvider({ + renderWithProvider({ activeUnitId: undefined, - unit: { ...unit } + unit: { ...unit }, }); await user.click(screen.getByText(unit.title)); @@ -137,14 +137,14 @@ describe('', () => { it('should handle click when unit id is not found in any sequence (covers early return)', async () => { const user = userEvent.setup(); await initTestStore(); - + // Use a unit ID that doesn't exist in any sequence const nonExistentUnitId = 'non-existent-unit-id'; - - renderWithProvider({ + + renderWithProvider({ unit: { ...unit, id: nonExistentUnitId }, id: nonExistentUnitId, - activeUnitId: unit.id // valid activeUnitId but invalid target unit + activeUnitId: unit.id, // valid activeUnitId but invalid target unit }); await user.click(screen.getByText(unit.title)); @@ -157,14 +157,14 @@ describe('', () => { it('should handle click when both activeUnitId and target unit id are invalid', async () => { const user = userEvent.setup(); await initTestStore(); - + const nonExistentUnitId = 'non-existent-unit-id'; const anotherNonExistentUnitId = 'another-non-existent-unit-id'; - - renderWithProvider({ + + renderWithProvider({ unit: { ...unit, id: nonExistentUnitId }, id: nonExistentUnitId, - activeUnitId: anotherNonExistentUnitId + activeUnitId: anotherNonExistentUnitId, }); await user.click(screen.getByText(unit.title)); diff --git a/src/courseware/data/redux.test.js b/src/courseware/data/redux.test.js index 4a7e10cbe6..720636d18a 100644 --- a/src/courseware/data/redux.test.js +++ b/src/courseware/data/redux.test.js @@ -357,15 +357,15 @@ describe('Data layer integration tests', () => { // Test the first return {} - when unitId is falsy const thunk = thunks.checkBlockCompletion(courseId, sequenceId, null); const result = await thunk(store.dispatch, store.getState); - + expect(result).toEqual({}); }); it('Should return empty object when unitId is undefined', async () => { - // Test the first return {} - when unitId is undefined + // Test the first return {} - when unitId is undefined const thunk = thunks.checkBlockCompletion(courseId, sequenceId, undefined); const result = await thunk(store.dispatch, store.getState); - + expect(result).toEqual({}); }); @@ -373,14 +373,14 @@ describe('Data layer integration tests', () => { // Test the first return {} - when unitId is empty string const thunk = thunks.checkBlockCompletion(courseId, sequenceId, ''); const result = await thunk(store.dispatch, store.getState); - + expect(result).toEqual({}); }); it('Should return empty object when unit is already complete', async () => { // First, set up the state so that the unit is already marked as complete const testUnitId = 'test-unit-id'; - + // Dispatch an action to mark the unit as complete in the store store.dispatch(updateModel({ modelType: 'units', @@ -389,16 +389,16 @@ describe('Data layer integration tests', () => { complete: true, }, })); - + // Record the current number of API calls before our test const initialRequestCount = axiosMock.history.post.length; - + // Now call checkBlockCompletion - it should return {} without making API calls const thunk = thunks.checkBlockCompletion(courseId, sequenceId, testUnitId); const result = await thunk(store.dispatch, store.getState); - + expect(result).toEqual({}); - + // Verify that no new API calls were made (since it should return early) expect(axiosMock.history.post.length).toBe(initialRequestCount); });