Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Progress: fix script selector bugs #22248

Merged
merged 2 commits into from May 10, 2018
Merged

Progress: fix script selector bugs #22248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2144e05
show first script if a course is assigned, set default if no validScr…
Erin007 May 8, 2018
1a3a1a2
update sectionProgressReduxTest
Erin007 May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 18 additions & 23 deletions apps/src/sites/code.org/pages/public/teacher-dashboard/index.js
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@ import { Provider } from 'react-redux';
import { registerReducers, getStore } from '@cdo/apps/redux';
import SectionProjectsList from '@cdo/apps/templates/projects/SectionProjectsList';
import SectionProgress from '@cdo/apps/templates/sectionProgress/SectionProgress';
import experiments, { COURSE_VERSIONS } from '@cdo/apps/util/experiments';
import experiments from '@cdo/apps/util/experiments';
import firehoseClient from '@cdo/apps/lib/util/firehose';
import {
renderSyncOauthSectionControl,
Expand Down Expand Up @@ -55,29 +55,24 @@ function renderSectionProgress(section, validScripts) {
const store = getStore();
store.dispatch(setSection(section));

if (experiments.isEnabled(COURSE_VERSIONS)) {
const promises = [
$.ajax({
method: 'GET',
url: `/dashboardapi/sections/${section.id}/student_script_ids`,
dataType: 'json'
}),
$.ajax({
method: 'GET',
url: `/dashboardapi/courses?allVersions=1`,
dataType: 'json'
})
];
Promise.all(promises).then(data => {
let [studentScriptsData, validCourses] = data;
const { studentScriptIds } = studentScriptsData;
store.dispatch(setValidScripts(validScripts, studentScriptIds, validCourses, section.course_id));
renderSectionProgressReact(store);
});
} else {
store.dispatch(setValidScripts(validScripts));
const promises = [
$.ajax({
method: 'GET',
url: `/dashboardapi/sections/${section.id}/student_script_ids`,
dataType: 'json'
}),
$.ajax({
method: 'GET',
url: `/dashboardapi/courses?allVersions=1`,
dataType: 'json'
})
];
Promise.all(promises).then(data => {
let [studentScriptsData, validCourses] = data;
const { studentScriptIds } = studentScriptsData;
store.dispatch(setValidScripts(validScripts, studentScriptIds, validCourses, section.course_id));
renderSectionProgressReact(store);
}
});
}

function renderSectionProgressReact(store) {
Expand Down
33 changes: 27 additions & 6 deletions apps/src/templates/sectionProgress/sectionProgressRedux.js
View file
Open in desktop
Expand Up @@ -90,6 +90,8 @@ export const ViewType = {
DETAIL: "detail",
};

export const ACCELERATED_SCRIPT_ID = 1;

/**
* Shape for the section
* The section we get directly from angular right now. This gives us a
Expand Down Expand Up @@ -200,15 +202,13 @@ export default function sectionProgress(state=initialState, action) {
};
}
if (action.type === SET_VALID_SCRIPTS) {
// If no scriptId is assigned, use the first valid script.
const defaultScriptId = state.scriptId || action.validScripts[0].id;

let validScripts = action.validScripts;
if (action.studentScriptIds && action.validCourses) {

// Include the id for the Accelerated Course so that there will always be // at least one validScript and we don't end up with an empty dropdown.
const idMap = {ACCELERATED_SCRIPT_ID: true};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this will be popular from a product perspective. In the next PR, let's find a way to only add the accelerated course if the list would otherwise be empty.

// First, construct an id map consisting only of script ids which a
// student has participated in.
const idMap = {};
action.studentScriptIds.forEach(id => idMap[id] = true);

// If the student has participated in a script which is a unit in a
Expand All @@ -222,14 +222,35 @@ export default function sectionProgress(state=initialState, action) {
course.script_ids.forEach(id => idMap[id] = true);
}
});

validScripts = validScripts.filter(script => idMap[script.id]);

var scriptId;
switch (true) {
// When there is a scriptId already in state.
case !!state.scriptId:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I had not seen the switch (true) pattern before but it looks legit.

scriptId = state.scriptId;
break;
// When there is an assigned course, set scriptId to the first script in the assigned course.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an assigned script, it will already have been set in state.scriptId, because the setSection action is currently called before setValidScripts. It seems confusing to have this logic spread out between setSection and setValidScripts.

I would recommend passing in the whole section here, then using both section.course_id and section.script.id to determine which section to assign.

If you don't have time to do this now, please add a comment indicating that (1) the logic is split between these two redux actions, and (2) it is required that setSection is called before setValidScripts in order for the right thing to happen in the case where a section has both a course and a script assigned.

case !!action.assignedCourseId:
action.validCourses.forEach(course => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessary to do right now, but you can return early from the forEach loop once scriptId has been assigned a value

if (course.id === action.assignedCourseId) {
scriptId = course.script_ids[0];
}
});
break;
// If there are validScripts, set scriptId to the first valid script.
case validScripts.length > 0:
scriptId = validScripts[0].id;
break;
default:
scriptId = ACCELERATED_SCRIPT_ID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be safe to remove this default case, because validScripts will never be empty (always contains the accelerated course).

}
}

return {
...state,
validScripts,
scriptId: defaultScriptId,
scriptId: scriptId,
};
}
if (action.type === ADD_SCRIPT_DATA) {
Expand Down
22 changes: 13 additions & 9 deletions apps/test/unit/templates/sectionProgress/sectionProgressReduxTest.js
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import sectionProgress, {
setLessonOfInterest,
startLoadingProgress,
finishLoadingProgress,
ACCELERATED_SCRIPT_ID,
} from '@cdo/apps/templates/sectionProgress/sectionProgressRedux';

const fakeSectionData = {
Expand Down Expand Up @@ -172,23 +173,26 @@ describe('sectionProgressRedux', () => {
});

describe('setValidScripts', () => {
it('sets the script data and defaults scriptId', () => {
const action = setValidScripts(fakeValidScripts);
const nextState = sectionProgress(initialState, action);
assert.deepEqual(nextState.validScripts, fakeValidScripts);
assert.deepEqual(nextState.scriptId, fakeValidScripts[0].id);
});

it('sets the script data and does not override already assigned scriptId', () => {
const action = setValidScripts(fakeValidScripts);
it('does not override already assigned scriptId', () => {
const studentScriptIds = [];
const validCourses = [];
const action = setValidScripts(fakeValidScripts, studentScriptIds, validCourses);
const nextState = sectionProgress({
...initialState,
scriptId: 100
}, action);
assert.deepEqual(nextState.validScripts, fakeValidScripts);
assert.deepEqual(nextState.scriptId, 100);
});

it('filtered validScripts includes Accelerated script id by default', () => {
const studentScriptIds = [];
const validCourses = [];
const action = setValidScripts(fakeValidScripts, studentScriptIds, validCourses);
const nextState = sectionProgress(initialState, action);
assert.deepEqual(nextState.validScripts, fakeValidScripts.filter(script => script.id === ACCELERATED_SCRIPT_ID));
});

it('filters validScripts to those included in studentScriptIds', () => {
const studentScriptIds = [456];
const validCourses = [];
Expand Down