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: Course selection bug followup #22289

Merged
merged 8 commits into from
May 15, 2018
Merged

Progress: Course selection bug followup #22289

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6c152a7
Merge branch 'staging' into course-selection-bug
Erin007 May 9, 2018
67a22d9
Express course is default, update tests
Erin007 May 9, 2018
6aa7e1b
Merge branch 'staging' into course-selection-bug-followup
Erin007 May 10, 2018
b73c234
Merge branch 'staging' into course-selection-bug-followup
Erin007 May 10, 2018
75c6484
Merge branch 'staging' into course-selection-bug-followup
Erin007 May 14, 2018
e59f290
DEFAULT_SCRIPT_NAME constant, remove default empty string for dropdown
Erin007 May 14, 2018
955f680
group validScripts logic and scriptId logic, better commenting
Erin007 May 14, 2018
bc632a7
Merge branch 'staging' into course-selection-bug-followup
Erin007 May 15, 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
13 changes: 8 additions & 5 deletions apps/src/templates/sectionProgress/sectionProgressRedux.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ 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 @@ -204,9 +202,11 @@ export default function sectionProgress(state=initialState, action) {
if (action.type === SET_VALID_SCRIPTS) {

let validScripts = action.validScripts;
// Set defaultScript to Express Course to use if there are no validScripts
let defaultScript = action.validScripts.filter(script => script.name === "Express Course")[0];
Copy link
Member

Choose a reason for hiding this comment

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

style nits:

  • let validScripts --> const validScripts (on previous line)
  • .filter(...)[0] --> .find(...)
  • let defaultScript --> const defaultScript
  • action.validScripts --> validScripts
  • var --> let (line 227)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on dave's comment. we end up using "Express Course" in a few places now, so i would also pull this out into a constant called DEFAULT_SCRIPT_NAME (or something along those lines)


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};
const idMap = {};
// First, construct an id map consisting only of script ids which a
// student has participated in.
action.studentScriptIds.forEach(id => idMap[id] = true);
Expand Down Expand Up @@ -243,7 +243,10 @@ export default function sectionProgress(state=initialState, action) {
scriptId = validScripts[0].id;
break;
default:
scriptId = ACCELERATED_SCRIPT_ID;
// Use the default script (currently Express Course) so we don't have
// an empty dropdown.
scriptId = defaultScript.id;
validScripts.push(defaultScript);
Copy link
Member

Choose a reason for hiding this comment

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

this is starting to get a bit confusing, because now the part of the function that determines the scriptId is also modifying validScripts. As a first step, I would recommend moving the logic for if (validScripts.length === 0) {validScripts.push(defaultScript)} to line 226 and replacing the default and last case statement here with just

default:
  scriptId = validScripts[0].id

and also adding a comment explaining that one section computes validScripts and the other computes scriptId.

Ideally you would extract 1 or 2 self-documenting functions like this:

if (action.type === SET_VALID_SCRIPTS) {
  const validScripts = computeValidScripts(...)
  const scriptId = getDefaultScriptId(...)
}

but given the time constraints it seems ok not to do that now.

}
}

Expand Down
16 changes: 12 additions & 4 deletions apps/test/unit/templates/sectionProgress/sectionProgressReduxTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import sectionProgress, {
setLessonOfInterest,
startLoadingProgress,
finishLoadingProgress,
ACCELERATED_SCRIPT_ID,
} from '@cdo/apps/templates/sectionProgress/sectionProgressRedux';

const fakeSectionData = {
Expand Down Expand Up @@ -72,7 +71,16 @@ const fakeValidScripts = [
id: 301,
name: 'csp2',
position: 23
}
},
// Include Express Course to use as default if needed
{
category: "CS Fundamentals",
category_priority: 1,
id: 182,
name: "Express Course",
position: 6,
script_name: "express",
},
];

const fakeValidCourses = [
Expand Down Expand Up @@ -185,12 +193,12 @@ describe('sectionProgressRedux', () => {
assert.deepEqual(nextState.scriptId, 100);
});

it('filtered validScripts includes Accelerated script id by default', () => {
it('includes Express Course if nothing assigned and no progress', () => {
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));
assert.deepEqual(nextState.validScripts, fakeValidScripts.filter(script => script.name === "Express Course"));
});

it('filters validScripts to those included in studentScriptIds', () => {
Expand Down