Skip to content

Commit 5df4cd9

Browse files
diana-villalvazo-wgudiana-villalvazo-wgu
authored
fix: refactor best practices checklist logic (openedx#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
1 parent 7274316 commit 5df4cd9

12 files changed

Lines changed: 16 additions & 103 deletions

‎src/course-checklist/ChecklistSection/messages.js‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,6 @@ const messages = defineMessages({
7171
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.',
7272
description: 'Description for a section that prompts a user to follow best practices for video length',
7373
},
74-
mobileFriendlyVideoShortDescription: {
75-
id: 'mobileFriendlyVideoShortDescription',
76-
defaultMessage: 'Create mobile-friendly video',
77-
description: 'Label for a section that describes mobile friendly videos',
78-
},
79-
mobileFriendlyVideoLongDescription: {
80-
id: 'mobileFriendlyVideoLongDescription',
81-
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.',
82-
description: 'Description for a section that prompts a user to follow best practices for mobile friendly videos',
83-
},
8474
diverseSequencesShortDescription: {
8575
id: 'diverseSequencesShortDescription',
8676
defaultMessage: 'Build diverse learning sequences',

‎src/course-checklist/ChecklistSection/utils/courseChecklistData.jsx‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,6 @@ export const checklistItems = {
3636
id: 'videoDuration',
3737
pacingTypeFilter: filters.ALL,
3838
},
39-
{
40-
id: 'mobileFriendlyVideo',
41-
pacingTypeFilter: filters.ALL,
42-
},
4339
{
4440
id: 'diverseSequences',
4541
pacingTypeFilter: filters.ALL,

‎src/course-checklist/ChecklistSection/utils/courseChecklistValidators.js‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,8 @@ export const hasAssignmentDeadlines = (assignments, dates) => {
3535

3636
export const hasShortVideoDuration = (videos) => {
3737
if (videos.totalNumber === 0) {
38-
return true;
39-
} if (videos.totalNumber > 0 && videos.durations.median <= 600) {
40-
return true;
41-
}
42-
43-
return false;
44-
};
45-
46-
export const hasMobileFriendlyVideos = (videos) => {
47-
if (videos.totalNumber === 0) {
48-
return true;
49-
} if (videos.totalNumber > 0 && (videos.numMobileEncoded / videos.totalNumber) >= 0.9) {
38+
return false;
39+
} if (videos.totalNumber > 0 && videos.durations.median !== null && videos.durations.median <= 600) {
5040
return true;
5141
}
5242

@@ -57,7 +47,7 @@ export const hasDiverseSequences = (subsections) => {
5747
if (subsections.totalVisible === 0) {
5848
return false;
5949
} if (subsections.totalVisible > 0) {
60-
return ((subsections.numWithOneBlockType / subsections.totalVisible) < 0.2);
50+
return ((subsections.numWithOneBlockType / subsections.totalVisible) <= 0.2);
6151
}
6252

6353
return false;
@@ -68,7 +58,7 @@ export const hasWeeklyHighlights = sections => (
6858
);
6959

7060
export const hasShortUnitDepth = units => (
71-
units.numBlocks.median <= 3
61+
units.numBlocks.median <= 3 && units.totalVisible > 0
7262
);
7363

7464
export const hasProctoringEscalationEmail = proctoring => (

‎src/course-checklist/ChecklistSection/utils/courseChecklistValidators.test.js‎

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,8 @@ describe('courseCheckValidators utility functions', () => {
189189
);
190190

191191
describe('hasShortVideoDuration', () => {
192-
it('returns true if course run has no videos', () => {
193-
expect(validators.hasShortVideoDuration({ totalNumber: 0 })).toEqual(true);
192+
it('returns false if course run has no videos', () => {
193+
expect(validators.hasShortVideoDuration({ totalNumber: 0 })).toEqual(false);
194194
});
195195

196196
it('returns true if course run videos have a median duration <= to 600', () => {
@@ -204,22 +204,6 @@ describe('courseCheckValidators utility functions', () => {
204204
});
205205
});
206206

207-
describe('hasMobileFriendlyVideos', () => {
208-
it('returns true if course run has no videos', () => {
209-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 0 })).toEqual(true);
210-
});
211-
212-
it('returns true if course run videos are >= 90% mobile friendly', () => {
213-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 9 }))
214-
.toEqual(true);
215-
});
216-
217-
it('returns true if course run videos are < 90% mobile friendly', () => {
218-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 8 }))
219-
.toEqual(false);
220-
});
221-
});
222-
223207
describe('hasDiverseSequences', () => {
224208
it('returns true if < 20% of visible subsections have more than one block type', () => {
225209
expect(validators.hasDiverseSequences({ totalVisible: 10, numWithOneBlockType: 1 }))
@@ -264,6 +248,7 @@ describe('courseCheckValidators utility functions', () => {
264248
describe('hasShortUnitDepth', () => {
265249
it('returns true when course run has median number of blocks <= 3', () => {
266250
const units = {
251+
totalVisible: 2,
267252
numBlocks: {
268253
median: 3,
269254
},
@@ -274,6 +259,7 @@ describe('courseCheckValidators utility functions', () => {
274259

275260
it('returns false when course run has median number of blocks > 3', () => {
276261
const units = {
262+
totalVisible: 2,
277263
numBlocks: {
278264
median: 4,
279265
},

‎src/course-checklist/ChecklistSection/utils/getValidatedValue.js‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@ const getValidatedValue = (data, id) => {
1414
return healthValidators.hasAssignmentDeadlines(data.assignments, data.dates);
1515
case 'videoDuration':
1616
return healthValidators.hasShortVideoDuration(data.videos);
17-
case 'mobileFriendlyVideo':
18-
return healthValidators.hasMobileFriendlyVideos(data.videos);
1917
case 'diverseSequences':
2018
return healthValidators.hasDiverseSequences(data.subsections);
2119
case 'weeklyHighlights':

‎src/course-checklist/ChecklistSection/utils/getValidatedValue.test.js‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,20 +88,6 @@ describe('getValidatedValue utility function', () => {
8888
expect(spy).toHaveBeenCalledTimes(1);
8989
});
9090

91-
it('mobile friendly video', () => {
92-
const spy = jest.fn();
93-
localValidators.hasMobileFriendlyVideos = spy;
94-
95-
const props = {
96-
data: {
97-
videos: {},
98-
},
99-
};
100-
101-
getValidatedValue(props, 'mobileFriendlyVideo');
102-
expect(spy).toHaveBeenCalledTimes(1);
103-
});
104-
10591
it('diverse sequences', () => {
10692
const spy = jest.fn();
10793
localValidators.hasDiverseSequences = spy;

‎src/course-outline/CourseOutline.test.jsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,7 @@ describe('<CourseOutline />', () => {
481481
courseId, excludeGraded: true, all: true,
482482
}), store.dispatch);
483483

484-
expect(getByText('4/9 completed')).toBeInTheDocument();
484+
expect(getByText('3/8 completed')).toBeInTheDocument();
485485
});
486486

487487
it('render alerts if checklist api fails', async () => {

‎src/course-outline/constants.js‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,6 @@ export const BEST_PRACTICES_CHECKLIST = /** @type {const} */ ({
5858
id: 'videoDuration',
5959
pacingTypeFilter: CHECKLIST_FILTERS.ALL,
6060
},
61-
{
62-
id: 'mobileFriendlyVideo',
63-
pacingTypeFilter: CHECKLIST_FILTERS.ALL,
64-
},
6561
{
6662
id: 'diverseSequences',
6763
pacingTypeFilter: CHECKLIST_FILTERS.ALL,

‎src/course-outline/utils/courseChecklistValidators.js‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -62,20 +62,7 @@ export const hasShortVideoDuration = (videos) => {
6262
if (totalNumber === 0) {
6363
return true;
6464
}
65-
if (totalNumber > 0 && durations.median <= 600) {
66-
return true;
67-
}
68-
69-
return false;
70-
};
71-
72-
export const hasMobileFriendlyVideos = (videos) => {
73-
const { totalNumber, numMobileEncoded } = videos;
74-
75-
if (totalNumber === 0) {
76-
return true;
77-
}
78-
if (totalNumber > 0 && (numMobileEncoded / totalNumber) >= 0.9) {
65+
if (totalNumber > 0 && durations.median !== null && durations.median <= 600) {
7966
return true;
8067
}
8168

@@ -89,7 +76,7 @@ export const hasDiverseSequences = (subsections) => {
8976
return false;
9077
}
9178
if (totalVisible > 0) {
92-
return ((numWithOneBlockType / totalVisible) < 0.2);
79+
return ((numWithOneBlockType / totalVisible) <= 0.2);
9380
}
9481

9582
return false;
@@ -101,6 +88,6 @@ export const hasWeeklyHighlights = (sections) => {
10188
return highlightsActiveForCourse && highlightsEnabled;
10289
};
10390

104-
export const hasShortUnitDepth = (units) => units.numBlocks.median <= 3;
91+
export const hasShortUnitDepth = (units) => units.numBlocks.median <= 3 && units.totalVisible > 0;
10592

10693
export const hasProctoringEscalationEmail = (proctoring) => proctoring.hasProctoringEscalationEmail;

‎src/course-outline/utils/courseChecklistValidators.test.js‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -204,22 +204,6 @@ describe('courseCheckValidators utility functions', () => {
204204
});
205205
});
206206

207-
describe('hasMobileFriendlyVideos', () => {
208-
it('returns true if course run has no videos', () => {
209-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 0 })).toEqual(true);
210-
});
211-
212-
it('returns true if course run videos are >= 90% mobile friendly', () => {
213-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 9 }))
214-
.toEqual(true);
215-
});
216-
217-
it('returns true if course run videos are < 90% mobile friendly', () => {
218-
expect(validators.hasMobileFriendlyVideos({ totalNumber: 10, numMobileEncoded: 8 }))
219-
.toEqual(false);
220-
});
221-
});
222-
223207
describe('hasDiverseSequences', () => {
224208
it('returns true if < 20% of visible subsections have more than one block type', () => {
225209
expect(validators.hasDiverseSequences({ totalVisible: 10, numWithOneBlockType: 1 }))
@@ -264,6 +248,7 @@ describe('courseCheckValidators utility functions', () => {
264248
describe('hasShortUnitDepth', () => {
265249
it('returns true when course run has median number of blocks <= 3', () => {
266250
const units = {
251+
totalVisible: 2,
267252
numBlocks: {
268253
median: 3,
269254
},
@@ -274,6 +259,7 @@ describe('courseCheckValidators utility functions', () => {
274259

275260
it('returns false when course run has median number of blocks > 3', () => {
276261
const units = {
262+
totalVisible: 2,
277263
numBlocks: {
278264
median: 4,
279265
},

0 commit comments

Comments
 (0)