Move students: Add redux, UI tweaks, refactoring#22138
Conversation
…o account for 'other teacher' option
caleybrock
left a comment
There was a problem hiding this comment.
Just a few little comments, but overall, this looks great! I particularly like the use of redux - I think it makes it clear how everything is working together.
| import BaseDialog from '../BaseDialog'; | ||
| import DialogFooter from "../teacherDashboard/DialogFooter"; | ||
| import {sectionsNameAndId} from '@cdo/apps/templates/teacherDashboard/teacherSectionsRedux'; | ||
| import {updateStudentTransfer, transferStudents, OTHER_TEACHER, COPY_STUDENTS, blankStudentTransfer} from './manageStudentsRedux'; |
There was a problem hiding this comment.
nit: This line is too long, break up into multiple.
| header: { | ||
| label: i18n.name(), | ||
| props: { | ||
| id: 'name-header', |
There was a problem hiding this comment.
nit: when these are for testing only, we typically prepend with uitest like this:
| export const transferStudents = () => { | ||
| return (dispatch, getState) => { | ||
| const state = getState(); | ||
| const currentSectionCode = sectionCode(state, state.manageStudents.sectionId); |
There was a problem hiding this comment.
Given our original concern, might be good to have a comment here pointing out we are pulling information from another part of the redux state tree.
…are not being moved to another teacher
|
all good points, @caleybrock! i've updated my PR with your feedback, and fixed the storybook for this component since it caused some tests in circleci to fail |
davidsbailey
left a comment
There was a problem hiding this comment.
great progress Maddie! A few misc comments below.
| // OTHER_TEACHER - value determines whether students will be moved to a section owned by a different teacher | ||
| // COPY_STUDENTS - value determines whether students will be copied to the new section or moved (and subsequently removed from current section) | ||
| export const OTHER_TEACHER = "otherTeacher"; | ||
| export const COPY_STUDENTS = "copy"; |
There was a problem hiding this comment.
nit: life might be easier in some future debugging session if this value is "copyStudents" rather than just "copy"
There was a problem hiding this comment.
Also, it would be more consistent with our style to write the comment closer to each constant, e.g.:
// Constants around moving students to another section.
// whether students will be moved to a section owned by a different teacher
export const OTHER_TEACHER = "otherTeacher";
// ...
export const COPY_STUDENTS = "copyStudents";
| export const COPY_STUDENTS = "copy"; | ||
|
|
||
| /** Initial state for manageStudents.transferData redux store. | ||
| * studentIds - student ids selected to be moved to another section |
There was a problem hiding this comment.
For multi-line comments we follow the style shown here, with comment starting on the next line, and aligning with the first *, and ending with */ like this:
/**
* Initial state...
* ...
*/
I know it's no fun to receive arbitrary style corrections, but we believe it'll make our code more readable if we follow our style guide closely.
| import {expect} from '../../../util/configuredChai'; | ||
| import {UnconnectedMoveStudents as MoveStudents, DEFAULT_STATE} from '@cdo/apps/templates/manageStudents/MoveStudents'; | ||
| import sinon from 'sinon'; | ||
| import {blankStudentTransfer} from '@cdo/apps/templates/manageStudents/manageStudentsRedux'; |
|
@davidsbailey thank you! i agree about having a cohesive style, so i don't mind stylistic nits at all 😃 |
| updateStudentTransfer(transferData) { | ||
| dispatch(updateStudentTransfer(transferData)); | ||
| }, | ||
| transferStudents(studentIds, currentSectionCode, newSectionCode, copyStudents) { |
There was a problem hiding this comment.
I think these params aren't needed.
caleybrock
left a comment
There was a problem hiding this comment.
Awesome test coverage!
This is the third in a series of PRs to rewrite 'move students' in React. Previous PR here, spec here, and Axisoft ticket here.
These changes include:
transferStudentsto reduxmanageStudentsReduxthat previously lived in theMoveStudentscomponent (includes some renaming for clarity in context of reducer rather than component)MoveStudentsunit tests to test event simulation rather than output of helper methods (these are basically totally rewritten, so I'm sorry that the diff is confusing)Conditions to enable 'Move students' button:
What's left?
/sections/transfersendpoint changes and UI changes once new 'move students' is ready