Progress: fix script selector bugs #22248
Conversation
…ipts to accelerated course
Erin007
force-pushed
the
course-selection-bug
branch
from
8160da8
to
1a3a1a2
Compare
| case !!state.scriptId: | ||
| scriptId = state.scriptId; | ||
| break; | ||
| // When there is an assigned course, set scriptId to the first script in the assigned course. |
There was a problem hiding this comment.
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.
| var scriptId; | ||
| switch (true) { | ||
| // When there is a scriptId already in state. | ||
| case !!state.scriptId: |
There was a problem hiding this comment.
nice, I had not seen the switch (true) pattern before but it looks legit.
|
|
||
| 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}; |
There was a problem hiding this comment.
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.
| scriptId = validScripts[0].id; | ||
| break; | ||
| default: | ||
| scriptId = ACCELERATED_SCRIPT_ID; |
There was a problem hiding this comment.
it should be safe to remove this default case, because validScripts will never be empty (always contains the accelerated course).
|
Logic in description makes sense to me 👍 |
| break; | ||
| // When there is an assigned course, set scriptId to the first script in the assigned course. | ||
| case !!action.assignedCourseId: | ||
| action.validCourses.forEach(course => { |
There was a problem hiding this comment.
not necessary to do right now, but you can return early from the forEach loop once scriptId has been assigned a value
There was a problem hiding this comment.
great work! and i second @davidsbailey's comment. there are a couple of test failures around what /v2/sections/valid_scripts is returning (they seem unrelated, i think), but everything on your end looks good
This PR updates the logic for what should be shown in the
ScriptSelectordropdown on the progress tab. Currently, with the sectionProgressRedesign experiment on, there is a bug where if a course is assigned to a section, we do not correctly show that course in this dropdown menu. To fix this bug, I exposed some of @davidsbailey course versioning work from behind its flag to accessassignedCourseId. In doing so, I exposed a different bug, wherein if there are no assigned courses or scripts and no progress for the section's students, we were filtering everyvalidScriptout of the possible options to show in the dropdown. To fix this second bug, I always keep theAccelerated Course(scriptId=== 1) in the dropdown.If the section is assigned a course, we should show the first unit of that course by default in the dropdown.
If the section is assigned a script, we should show the that script in the dropdown.
If there is neither an assigned course nor an assigned script, but any of the section's students have made progress in scripts, we should show the first valid script associated with students in the section.
If none of the above, we show the Accelerated Course by default to avoid having an empty dropdown.
sectionProgressRedux tests have been updated accordingly.