ManageStudentsTable: save all functional #20954
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,11 +51,16 @@ const blankNewStudentRow = { | |
| }; | ||
|
|
||
| // Initial state for the manageStudents redux store. | ||
|
|
||
| // addStatus should be an object with the following keys | ||
| // status - AddStatus enum describing the result of the add | ||
| // numStudents - number of students that were attempted to add | ||
|
There was a problem hiding this comment.
|
||
| const initialState = { | ||
| loginType: '', | ||
| studentData: {}, | ||
| editingData: {}, | ||
| sectionId: null, | ||
| addStatus: {}, | ||
| }; | ||
|
|
||
| const SET_LOGIN_TYPE = 'manageStudents/SET_LOGIN_TYPE'; | ||
|
|
@@ -84,8 +89,12 @@ export const setSecretWords = (studentId, words) => ({ type: SET_SECRET_WORDS, s | |
| export const editStudent = (studentId, studentData) => ({ type: EDIT_STUDENT, studentId, studentData }); | ||
| export const startSavingStudent = (studentId) => ({ type: START_SAVING_STUDENT, studentId }); | ||
| export const saveStudentSuccess = (studentId) => ({ type: SAVE_STUDENT_SUCCESS, studentId }); | ||
| export const addStudentsSuccess = (numStudents, rowIds, studentData) => ( | ||
| { type: ADD_STUDENT_SUCCESS, numStudents, rowIds, studentData } | ||
| ); | ||
| export const addStudentsFailure = (numStudents, error, studentIds) => ( | ||
| { type: ADD_STUDENT_FAILURE, numStudents, error, studentIds } | ||
| ); | ||
| export const addMultipleRows = (studentData) => ({ type: ADD_MULTIPLE_ROWS, studentData }); | ||
|
|
||
| export const saveStudent = (studentId) => { | ||
|
|
@@ -101,24 +110,57 @@ export const saveStudent = (studentId) => { | |
| }; | ||
| }; | ||
|
|
||
| // Saves all RowType.STUDENT currently being edited and adds all | ||
| // RowType.NEW_STUDENT currently being edited. | ||
| export const saveAllStudents = () => { | ||
| return (dispatch, getState) => { | ||
| const state = getState().manageStudents; | ||
|
|
||
| // Currently, every update is an individual call to the server. | ||
|
There was a problem hiding this comment. Do we plan to change this later? There was a problem hiding this comment. Maybe for V2. I think it could be useful to think about changing the API once we create the "edit all" feature. |
||
| const currentlyEditedData = convertStudentDataToArray(state.editingData); | ||
| let studentsToSave = currentlyEditedData.filter(student => student.rowType === RowType.STUDENT); | ||
| for (let i = 0; i<studentsToSave.length; i++) { | ||
| dispatch(saveStudent(studentsToSave[i].id)); | ||
|
There was a problem hiding this comment. Suggestion: My personal preference for straightforward iterations like this is studentsToSave.forEach(student => dispatch(saveStudent(student.id))); |
||
| } | ||
|
|
||
| // Adding students can be saved together. | ||
| const arrayOfEditedData = convertStudentDataToArray(state.editingData); | ||
| const newStudentsToAdd = arrayOfEditedData.filter(student => student.rowType === RowType.NEW_STUDENT).map( | ||
| student => student.id); | ||
|
There was a problem hiding this comment. Style nit: This is a long line, I'd break it across your chained method calls. const newStudentsToAdd = arrayOfEditedData
.filter(student => student.rowType === RowType.NEW_STUDENT)
.map(student => student.id);...also |
||
| if (newStudentsToAdd.length > 0) { | ||
| dispatch(addStudents(newStudentsToAdd)); | ||
| } | ||
| }; | ||
| }; | ||
|
|
||
| // Adds a student, with the given row id (studentIds), from RowType.ADD or | ||
| // RowType.NEW_STUDENT. | ||
| export const addStudents = (studentIds) => { | ||
| return (dispatch, getState) => { | ||
| const state = getState().manageStudents; | ||
| const numStudentsToAdd = studentIds.length; | ||
|
|
||
| // Update each row to saving in progress. | ||
| for (let i = 0; i<studentIds.length; i++) { | ||
| dispatch(startSavingStudent(studentIds[i])); | ||
| } | ||
|
|
||
| const arrayOfEditedData = convertStudentDataToArray(state.editingData); | ||
| const filteredData = arrayOfEditedData.filter(student => studentIds.includes(student.id)); | ||
| addStudentOnServer(filteredData, | ||
| state.sectionId, (error, data) => { | ||
| if (error) { | ||
| dispatch(addStudentsFailure(numStudentsToAdd, error, studentIds)); | ||
| console.error(error); | ||
| } else { | ||
| dispatch(addStudentsSuccess(numStudentsToAdd, studentIds, | ||
| convertStudentServerData(data, state.loginType, state.sectionId))); | ||
| } | ||
| }); | ||
| }; | ||
| }; | ||
|
|
||
| // Creates a new RowType.NEW_STUDENT for each name in the array. | ||
| export const addMultipleAddRows = (studentNames) => { | ||
| return (dispatch, getState) => { | ||
| let studentData = {}; | ||
|
|
@@ -240,43 +282,39 @@ export default function manageStudents(state=initialState, action) { | |
| }; | ||
| } | ||
| if (action.type === ADD_STUDENT_FAILURE) { | ||
| let newState = { | ||
| ...state, | ||
| addStatus: {status: AddStatus.FAIL, numStudents: action.numStudents} | ||
| }; | ||
| for (let i = 0; i<action.studentIds.length; i++) { | ||
| newState.studentData[action.studentIds[i]] = { | ||
| ...state.studentData[action.studentIds[i]], | ||
| isSaving: false, | ||
| }; | ||
| } | ||
| return newState; | ||
| } | ||
| if (action.type === ADD_STUDENT_SUCCESS) { | ||
| let newState = { | ||
| ...state, | ||
| studentData: { | ||
| ..._.omit(state.studentData, action.rowIds), | ||
| ...action.studentData, | ||
| [addRowId]: { | ||
| ...blankAddRow, | ||
| loginType: state.loginType, | ||
| } | ||
| }, | ||
| editingData: { | ||
| ..._.omit(state.editingData, action.rowIds), | ||
| [addRowId]: { | ||
| ...blankAddRow, | ||
| loginType: state.loginType, | ||
| } | ||
| }, | ||
| addStatus: {status: AddStatus.SUCCESS, numStudents: action.numStudents} | ||
| }; | ||
|
There was a problem hiding this comment. When should we have spaces in braces { like this } and when should we not {like this}? I see both in our codebase, and I don't know think the styleguide has a ruling. 🤷🏼♀️ just curious There was a problem hiding this comment. Apparently they are both okay: https://github.com/code-dot-org/code-dot-org/blob/staging/STYLEGUIDE.md#js-object-curly-spacing |
||
| return newState; | ||
| } | ||
| if (action.type === EDIT_STUDENT) { | ||
| return { | ||
|
|
@@ -353,39 +391,20 @@ export const convertStudentServerData = (studentData, loginType, sectionId) => { | |
| id: student.id, | ||
| name: student.name, | ||
| username: student.username, | ||
| age: student.age || '', | ||
| gender: student.gender || '', | ||
| secretWords: student.secret_words, | ||
| secretPicturePath: student.secret_picture_path, | ||
| loginType: loginType, | ||
| sectionId: sectionId, | ||
| isEditing: false, | ||
| isSaving: false, | ||
| rowType: RowType.STUDENT, | ||
| }; | ||
| } | ||
| return studentLookup; | ||
| }; | ||
|
|
||
| // Converts key/value id/student pairs to an array of student objects for the | ||
| // component to display | ||
| // TODO(caleybrock): memoize this - sections could be a few thousand students | ||
|
|
@@ -413,19 +432,22 @@ const updateStudentOnServer = (updatedStudentInfo, onComplete) => { | |
| }); | ||
| }; | ||
|
|
||
| // Make a post request to add students. | ||
| const addStudentOnServer = (updatedStudentsInfo, sectionId, onComplete) => { | ||
| const studentsToAdd = []; | ||
| for (let i = 0; i<updatedStudentsInfo.length; i++) { | ||
| studentsToAdd[i] = { | ||
| editing: true, | ||
| name: updatedStudentsInfo[i].name, | ||
| age: updatedStudentsInfo[i].age, | ||
| gender: updatedStudentsInfo[i].gender, | ||
| }; | ||
| } | ||
|
There was a problem hiding this comment. This looks like a There was a problem hiding this comment. Oops missed this one - but will follow up. |
||
| $.ajax({ | ||
| url: `/v2/sections/${sectionId}/students`, | ||
| method: 'POST', | ||
| contentType: 'application/json;charset=UTF-8', | ||
| data: JSON.stringify(studentsToAdd), | ||
| }).done((data) => { | ||
| onComplete(null, data); | ||
| }).fail((jqXhr, status) => { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.