From 5df4cd941d176949a813146908dfcc3d6df2f915 Mon Sep 17 00:00:00 2001 From: diana-villalvazo-wgu Date: Wed, 4 Jun 2025 13:46:01 -0600 Subject: [PATCH] fix: refactor best practices checklist logic (#2038) The Best Practices Checklist behavior was wrong for some cases: * Video: if duration is null it shouldn't be marked as completed * Video: if there are no videos it shouldn't be marked as completed * Unit depth: if course doesn't have units it shouldn't be marked as completed * Diverse learning sequence: description mentions that 80% should contain multiple content types, so if it is exactly 80% it should be marked as completed --- .../ChecklistSection/messages.js | 10 --------- .../utils/courseChecklistData.jsx | 4 ---- .../utils/courseChecklistValidators.js | 18 ++++----------- .../utils/courseChecklistValidators.test.js | 22 ++++--------------- .../utils/getValidatedValue.js | 2 -- .../utils/getValidatedValue.test.js | 14 ------------ src/course-outline/CourseOutline.test.jsx | 2 +- src/course-outline/constants.js | 4 ---- .../utils/courseChecklistValidators.js | 19 +++------------- .../utils/courseChecklistValidators.test.js | 18 ++------------- .../utils/getChecklistForStatusBar.test.js | 4 ++-- .../utils/getChecklistValues.js | 2 -- 12 files changed, 16 insertions(+), 103 deletions(-) diff --git a/src/course-checklist/ChecklistSection/messages.js b/src/course-checklist/ChecklistSection/messages.js index 362f568d1..106e0bf0b 100644 --- a/src/course-checklist/ChecklistSection/messages.js +++ b/src/course-checklist/ChecklistSection/messages.js @@ -71,16 +71,6 @@ const messages = defineMessages({ defaultMessage: 'Learners engage best with short videos followed by opportunities to practice. Ensure that 80% or more of course videos are less than 10 minutes long.', description: 'Description for a section that prompts a user to follow best practices for video length', }, - mobileFriendlyVideoShortDescription: { - id: 'mobileFriendlyVideoShortDescription', - defaultMessage: 'Create mobile-friendly video', - description: 'Label for a section that describes mobile friendly videos', - }, - mobileFriendlyVideoLongDescription: { - id: 'mobileFriendlyVideoLongDescription', - defaultMessage: 'Mobile-friendly videos can be viewed across all supported devices. Ensure that at least 90% of course videos are mobile friendly by uploading course videos to the edX video pipeline.', - description: 'Description for a section that prompts a user to follow best practices for mobile friendly videos', - }, diverseSequencesShortDescription: { id: 'diverseSequencesShortDescription', defaultMessage: 'Build diverse learning sequences', diff --git a/src/course-checklist/ChecklistSection/utils/courseChecklistData.jsx b/src/course-checklist/ChecklistSection/utils/courseChecklistData.jsx index c59d1f83b..d7e9178e4 100644 --- a/src/course-checklist/ChecklistSection/utils/courseChecklistData.jsx +++ b/src/course-checklist/ChecklistSection/utils/courseChecklistData.jsx @@ -36,10 +36,6 @@ export const checklistItems = { id: 'videoDuration', pacingTypeFilter: filters.ALL, }, - { - id: 'mobileFriendlyVideo', - pacingTypeFilter: filters.ALL, - }, { id: 'diverseSequences', pacingTypeFilter: filters.ALL, diff --git a/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.js b/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.js index 583ca4bac..9cb8f84fc 100644 --- a/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.js +++ b/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.js @@ -35,18 +35,8 @@ export const hasAssignmentDeadlines = (assignments, dates) => { export const hasShortVideoDuration = (videos) => { if (videos.totalNumber === 0) { - return true; - } if (videos.totalNumber > 0 && videos.durations.median <= 600) { - return true; - } - - return false; -}; - -export const hasMobileFriendlyVideos = (videos) => { - if (videos.totalNumber === 0) { - return true; - } if (videos.totalNumber > 0 && (videos.numMobileEncoded / videos.totalNumber) >= 0.9) { + return false; + } if (videos.totalNumber > 0 && videos.durations.median !== null && videos.durations.median <= 600) { return true; } @@ -57,7 +47,7 @@ export const hasDiverseSequences = (subsections) => { if (subsections.totalVisible === 0) { return false; } if (subsections.totalVisible > 0) { - return ((subsections.numWithOneBlockType / subsections.totalVisible) < 0.2); + return ((subsections.numWithOneBlockType / subsections.totalVisible) <= 0.2); } return false; @@ -68,7 +58,7 @@ export const hasWeeklyHighlights = sections => ( ); export const hasShortUnitDepth = units => ( - units.numBlocks.median <= 3 + units.numBlocks.median <= 3 && units.totalVisible > 0 ); export const hasProctoringEscalationEmail = proctoring => ( diff --git a/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.test.js b/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.test.js index 401475bc2..32414efc8 100644 --- a/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.test.js +++ b/src/course-checklist/ChecklistSection/utils/courseChecklistValidators.test.js @@ -189,8 +189,8 @@ describe('courseCheckValidators utility functions', () => { ); describe('hasShortVideoDuration', () => { - it('returns true if course run has no videos', () => { - expect(validators.hasShortVideoDuration({ totalNumber: 0 })).toEqual(true); + it('returns false if course run has no videos', () => { + expect(validators.hasShortVideoDuration({ totalNumber: 0 })).toEqual(false); }); it('returns true if course run videos have a median duration <= to 600', () => { @@ -204,22 +204,6 @@ describe('courseCheckValidators utility functions', () => { }); }); - describe('hasMobileFriendlyVideos', () => { - it('returns true if course run has no videos', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 0 })).toEqual(true); - }); - - it('returns true if course run videos are >= 90% mobile friendly', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 9 })) - .toEqual(true); - }); - - it('returns true if course run videos are < 90% mobile friendly', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 8 })) - .toEqual(false); - }); - }); - describe('hasDiverseSequences', () => { it('returns true if < 20% of visible subsections have more than one block type', () => { expect(validators.hasDiverseSequences({ totalVisible: 10, numWithOneBlockType: 1 })) @@ -264,6 +248,7 @@ describe('courseCheckValidators utility functions', () => { describe('hasShortUnitDepth', () => { it('returns true when course run has median number of blocks <= 3', () => { const units = { + totalVisible: 2, numBlocks: { median: 3, }, @@ -274,6 +259,7 @@ describe('courseCheckValidators utility functions', () => { it('returns false when course run has median number of blocks > 3', () => { const units = { + totalVisible: 2, numBlocks: { median: 4, }, diff --git a/src/course-checklist/ChecklistSection/utils/getValidatedValue.js b/src/course-checklist/ChecklistSection/utils/getValidatedValue.js index f6b745e1e..ff8c3a94e 100644 --- a/src/course-checklist/ChecklistSection/utils/getValidatedValue.js +++ b/src/course-checklist/ChecklistSection/utils/getValidatedValue.js @@ -14,8 +14,6 @@ const getValidatedValue = (data, id) => { return healthValidators.hasAssignmentDeadlines(data.assignments, data.dates); case 'videoDuration': return healthValidators.hasShortVideoDuration(data.videos); - case 'mobileFriendlyVideo': - return healthValidators.hasMobileFriendlyVideos(data.videos); case 'diverseSequences': return healthValidators.hasDiverseSequences(data.subsections); case 'weeklyHighlights': diff --git a/src/course-checklist/ChecklistSection/utils/getValidatedValue.test.js b/src/course-checklist/ChecklistSection/utils/getValidatedValue.test.js index bed8f76ef..08a201470 100644 --- a/src/course-checklist/ChecklistSection/utils/getValidatedValue.test.js +++ b/src/course-checklist/ChecklistSection/utils/getValidatedValue.test.js @@ -88,20 +88,6 @@ describe('getValidatedValue utility function', () => { expect(spy).toHaveBeenCalledTimes(1); }); - it('mobile friendly video', () => { - const spy = jest.fn(); - localValidators.hasMobileFriendlyVideos = spy; - - const props = { - data: { - videos: {}, - }, - }; - - getValidatedValue(props, 'mobileFriendlyVideo'); - expect(spy).toHaveBeenCalledTimes(1); - }); - it('diverse sequences', () => { const spy = jest.fn(); localValidators.hasDiverseSequences = spy; diff --git a/src/course-outline/CourseOutline.test.jsx b/src/course-outline/CourseOutline.test.jsx index 9f8176f42..3ff6690d2 100644 --- a/src/course-outline/CourseOutline.test.jsx +++ b/src/course-outline/CourseOutline.test.jsx @@ -481,7 +481,7 @@ describe('', () => { courseId, excludeGraded: true, all: true, }), store.dispatch); - expect(getByText('4/9 completed')).toBeInTheDocument(); + expect(getByText('3/8 completed')).toBeInTheDocument(); }); it('render alerts if checklist api fails', async () => { diff --git a/src/course-outline/constants.js b/src/course-outline/constants.js index eda0522a0..74cc51dad 100644 --- a/src/course-outline/constants.js +++ b/src/course-outline/constants.js @@ -58,10 +58,6 @@ export const BEST_PRACTICES_CHECKLIST = /** @type {const} */ ({ id: 'videoDuration', pacingTypeFilter: CHECKLIST_FILTERS.ALL, }, - { - id: 'mobileFriendlyVideo', - pacingTypeFilter: CHECKLIST_FILTERS.ALL, - }, { id: 'diverseSequences', pacingTypeFilter: CHECKLIST_FILTERS.ALL, diff --git a/src/course-outline/utils/courseChecklistValidators.js b/src/course-outline/utils/courseChecklistValidators.js index a304e50b7..d02f7a088 100644 --- a/src/course-outline/utils/courseChecklistValidators.js +++ b/src/course-outline/utils/courseChecklistValidators.js @@ -62,20 +62,7 @@ export const hasShortVideoDuration = (videos) => { if (totalNumber === 0) { return true; } - if (totalNumber > 0 && durations.median <= 600) { - return true; - } - - return false; -}; - -export const hasMobileFriendlyVideos = (videos) => { - const { totalNumber, numMobileEncoded } = videos; - - if (totalNumber === 0) { - return true; - } - if (totalNumber > 0 && (numMobileEncoded / totalNumber) >= 0.9) { + if (totalNumber > 0 && durations.median !== null && durations.median <= 600) { return true; } @@ -89,7 +76,7 @@ export const hasDiverseSequences = (subsections) => { return false; } if (totalVisible > 0) { - return ((numWithOneBlockType / totalVisible) < 0.2); + return ((numWithOneBlockType / totalVisible) <= 0.2); } return false; @@ -101,6 +88,6 @@ export const hasWeeklyHighlights = (sections) => { return highlightsActiveForCourse && highlightsEnabled; }; -export const hasShortUnitDepth = (units) => units.numBlocks.median <= 3; +export const hasShortUnitDepth = (units) => units.numBlocks.median <= 3 && units.totalVisible > 0; export const hasProctoringEscalationEmail = (proctoring) => proctoring.hasProctoringEscalationEmail; diff --git a/src/course-outline/utils/courseChecklistValidators.test.js b/src/course-outline/utils/courseChecklistValidators.test.js index 401475bc2..bc81eab9f 100644 --- a/src/course-outline/utils/courseChecklistValidators.test.js +++ b/src/course-outline/utils/courseChecklistValidators.test.js @@ -204,22 +204,6 @@ describe('courseCheckValidators utility functions', () => { }); }); - describe('hasMobileFriendlyVideos', () => { - it('returns true if course run has no videos', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 0 })).toEqual(true); - }); - - it('returns true if course run videos are >= 90% mobile friendly', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 9 })) - .toEqual(true); - }); - - it('returns true if course run videos are < 90% mobile friendly', () => { - expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 8 })) - .toEqual(false); - }); - }); - describe('hasDiverseSequences', () => { it('returns true if < 20% of visible subsections have more than one block type', () => { expect(validators.hasDiverseSequences({ totalVisible: 10, numWithOneBlockType: 1 })) @@ -264,6 +248,7 @@ describe('courseCheckValidators utility functions', () => { describe('hasShortUnitDepth', () => { it('returns true when course run has median number of blocks <= 3', () => { const units = { + totalVisible: 2, numBlocks: { median: 3, }, @@ -274,6 +259,7 @@ describe('courseCheckValidators utility functions', () => { it('returns false when course run has median number of blocks > 3', () => { const units = { + totalVisible: 2, numBlocks: { median: 4, }, diff --git a/src/course-outline/utils/getChecklistForStatusBar.test.js b/src/course-outline/utils/getChecklistForStatusBar.test.js index b7bc1cd14..9c91d00af 100644 --- a/src/course-outline/utils/getChecklistForStatusBar.test.js +++ b/src/course-outline/utils/getChecklistForStatusBar.test.js @@ -89,8 +89,8 @@ describe('getChecklistForStatusBar util functions', () => { }; expect(getCourseBestPracticesChecklist(data)).toEqual({ - totalCourseBestPracticesChecks: 4, - completedCourseBestPracticesChecks: 2, + totalCourseBestPracticesChecks: 3, + completedCourseBestPracticesChecks: 1, }); }); }); diff --git a/src/course-outline/utils/getChecklistValues.js b/src/course-outline/utils/getChecklistValues.js index 5b53ca734..ad9a49faa 100644 --- a/src/course-outline/utils/getChecklistValues.js +++ b/src/course-outline/utils/getChecklistValues.js @@ -32,8 +32,6 @@ const getChecklistValidatedValue = (data, id) => { return healthValidators.hasAssignmentDeadlines(assignments, dates); case 'videoDuration': return healthValidators.hasShortVideoDuration(videos); - case 'mobileFriendlyVideo': - return healthValidators.hasMobileFriendlyVideos(videos); case 'diverseSequences': return healthValidators.hasDiverseSequences(subsections); case 'weeklyHighlights':