Assign course/script to newly created section #17056
Conversation
| */ | ||
| export function getQueryString_() { | ||
| return window.location.search; | ||
| } |
There was a problem hiding this comment.
Don't know if there's a better way to be able to stub this, but this seems to work (semi-copied from what we do in experiments.js)
| defaultScriptId = parseInt(query.scriptId, 10); | ||
| } | ||
| store.dispatch(setDefaultAssignment()); | ||
| console.log('dispatched'); |
| if (experiments.isEnabled(SECTION_FLOW_2017)) { | ||
| hrefNewSection = '/home?' + queryString.stringify({courseId, scriptId}); | ||
| } else { | ||
| hrefNewSection = `${window.dashboard.CODE_ORG_URL}/teacher-dashboard?` + |
There was a problem hiding this comment.
Could use the pegasus() helper here.
| ...state, | ||
| defaultCourseId: courseId, | ||
| defaultScriptId: scriptId | ||
| }; |
There was a problem hiding this comment.
I'm confused - don't we want to launch straight into the new section flow when we pass these queryparams? Why wouldn't we dispatch that and set the course and script ids on the sectionBeingEdited instead of introducing some new and independent state?
There was a problem hiding this comment.
Hmm, that might make more sense. Let me give that a try.
| */ | ||
| export function getQueryString_() { | ||
| return window.location.search; | ||
| } |
|
Your approach makes a lot more sense. I've updated to do that. As part of this, I moved all of the query param stuff out of redux (where it didn't really feel right). Though it's semi-duplicated between the two entry points, one of these will go away once we don't have the experiment. |
| @@ -40,6 +38,8 @@ class OwnedSections extends React.Component { | |||
| beginEditingNewSection: PropTypes.func.isRequired, | |||
| beginEditingSection: PropTypes.func.isRequired, | |||
| beginImportRosterFlow: PropTypes.func.isRequired, | |||
| defaultCourseId: PropTypes.number, | |||
| defaultScriptId: PropTypes.number, | |||
There was a problem hiding this comment.
TODO: remove these props
| @@ -1,6 +1,7 @@ | |||
| import sinon from 'sinon'; | |||
| import { assert, expect } from '../../../util/configuredChai'; | |||
| import {stubRedux, restoreRedux, registerReducers, getStore} from '@cdo/apps/redux'; | |||
| // Use wildcard import so that we can stub | |||
|
@islemaster If you could take another look at this PR and sign off if things look good, that would be great :) |
| if (experiments.isEnabled(SECTION_FLOW_2017)) { | ||
| this.props.beginEditingNewSection(defaultCourseId, defaultScriptId); | ||
| this.props.beginEditingNewSection(); | ||
| } else { | ||
| // This is the only usage of the newSection action, and can be removed once | ||
| // SECTION_FLOW_2017 is finished |
There was a problem hiding this comment.
Note: This comment is not longer accurate :)
| } else { | ||
| // This is the only usage of the newSection action, and can be removed once | ||
| // SECTION_FLOW_2017 is finished | ||
| return this.props.newSection(defaultCourseId); | ||
| return this.props.newSection(); |
There was a problem hiding this comment.
Not blocking this PR (this is really my bad) but note to self: We should have a unit test for OwnedSections.jsx that clicks this button both with the experiment on and off.
This PR makes it so that when we click on New Section in the assign course dropdown, we load the page with the section edit experience started. This should be true whether our experiment is on or off.
It does so by going to the page with courseId/scriptId query params. We have a redux action that will parse these query params, and put them in the store. Not sure if it would be better to keep the query param parse logic outside of redux?