Skip to content

fix: sidebar unitid undefined #1762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,83 @@ describe('<SidebarUnit />', () => {
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();
Expand Down
14 changes: 12 additions & 2 deletions src/courseware/course/sidebar/sidebars/course-outline/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
51 changes: 51 additions & 0 deletions src/courseware/data/redux.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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', () => {
Expand Down
4 changes: 4 additions & 0 deletions src/courseware/data/thunks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down