ManageStudentsTable: save all functional #20954

caleybrock · 2018-03-01T07:27:34Z

Behind manageStudents experiment and does not currently effect current user changes.

"Save all" button now works: adds `RowType.NEW_STUDENT` and saves `RowType.STUDENT` that are currently being edited. Does not save the add row.

Technically: expand addStudent and saveStudent to handle a set of students, and add a save all function.

Redux tests updated appropriately

Erin007

A few tiny little edits, but LGTM! Congrats on being so close to finishing the overhaul of this table!

Erin007 · 2018-03-02T00:24:12Z

apps/i18n/common/en_us.json

-  "manageStudentsNotificationCannotAdd": "You could not add student to your section. Please try again or refresh the page.",
-  "manageStudentsNotificationAddSuccess": "You added 1 student to your section.",
+  "manageStudentsNotificationCannotAdd": "You could not add {numStudents} students to your section. Please try again or refresh the page.",
+  "manageStudentsNotificationAddSuccess": "You added {numStudents} students to your section.",
  "manageStudentsNotificationSuccess": "Success!",


tiny nit: Do we want this string to have "student(s)" for cases when numStudents is 1?

Erin007 · 2018-03-02T00:27:22Z

apps/src/templates/manageStudents/manageStudentsRedux.js

@@ -51,11 +51,16 @@ const blankNewStudentRow = {
 };

 // Initial state for the manageStudents redux store.
+
+// addStatus should be an object with the follow keys
+// status - AddStatus enum descripting the result of the add


tiny nit: "following keys" and "describing"?

Erin007 · 2018-03-02T00:30:49Z

apps/src/templates/manageStudents/manageStudentsRedux.js

        }
      },
-      addStatus: AddStatus.SUCCESS,
+      addStatus: {status: AddStatus.SUCCESS, numStudents: action.numStudents}
    };


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

Apparently they are both okay: https://github.com/code-dot-org/code-dot-org/blob/staging/STYLEGUIDE.md#js-object-curly-spacing

Erin007 · 2018-03-02T00:32:05Z

apps/test/unit/templates/manageStudents/manageStudentsReduxTest.js

@@ -338,7 +338,7 @@ describe('manageStudentsRedux', () => {
      assert.deepEqual(nextState.editingData[0], {...expectedBlankRow, loginType: 'picture'});
    });

-    it('addStudentSuccess updates studentData,removes editingData, and adds new blank row', () => {
+    it('addStudentsSuccess updates studentData,removes editingData, and adds new blank row', () => {


tiny nit: space before "removes"

islemaster

Very elegant solution, changing addStudent to addStudents throughout and wrapping the single-student cases as needed. Looks like there are some test failures to deal with?

islemaster · 2018-03-02T19:08:08Z

apps/i18n/common/en_us.json

-  "manageStudentsNotificationCannotAdd": "You could not add student to your section. Please try again or refresh the page.",
-  "manageStudentsNotificationAddSuccess": "You added 1 student to your section.",
+  "manageStudentsNotificationCannotAdd": "You could not add {numStudents, plural, one {1 student} other {# students}} to your section. Please try again or refresh the page.",
+  "manageStudentsNotificationAddSuccess": "You added {numStudents, plural, one {1 student} other {# students}} to your section.",


islemaster · 2018-03-02T19:11:56Z

apps/src/templates/manageStudents/ManageStudentsTable.jsx

+}), dispatch => ({
+  saveAllStudents() {
+    dispatch(saveAllStudents());
+  },
 }))(ManageStudentsTable);


This is a good place for the propsFromDispatch shorthand:

export default connect(state => ({ loginType: state.manageStudents.loginType, // Other propsFromState stuff... }), { // If you just pass an object of action creators as the second argument to connect, // react-redux assumes they're all simple pass-throughs like this: // saveAllStudents: (...args) => dispatch(saveAllStudents(...args)), saveAllStudents, })(ManageStudentsTable);

I'm going to leave this one as it. I find it confusing to understand what's going without the explicit call.

islemaster · 2018-03-02T19:12:54Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+
+// 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


Why doesn't it have these keys in its initial state?

This is a thing Immutable.js is really good at 😁 but not sure it's worth pivoting right now.

islemaster · 2018-03-02T19:15:12Z

apps/src/templates/manageStudents/manageStudentsRedux.js

-    dispatch(startSavingStudent(studentId));
-    addStudentOnServer(state.editingData[studentId], state.sectionId, (error, data) => {
+
+    // Currently, every update is an individual call to the server.


Do we plan to change this later?

Maybe for V2. I think it could be useful to think about changing the API once we create the "edit all" feature.

islemaster · 2018-03-02T19:18:07Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+    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));


Suggestion: My personal preference for straightforward iterations like this is

studentsToSave.forEach(student => dispatch(saveStudent(student.id)));

islemaster · 2018-03-02T19:19:47Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+    // 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);


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

islemaster · 2018-03-02T19:21:58Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+      age: updatedStudentsInfo[i].age,
+      gender: updatedStudentsInfo[i].gender,
+    };
+  }


This looks like a map operation.

Oops missed this one - but will follow up.

Save all functional

748f523

caleybrock added the star-labs label Mar 1, 2018

Update tests

0fada6c

caleybrock force-pushed the save-all-rows branch from cd34b91 to 0fada6c Compare March 1, 2018 23:22

caleybrock added 2 commits March 1, 2018 15:54

Add multiple students test

752aece

Failure add test for multiple students

882d5a4

caleybrock force-pushed the save-all-rows branch from 83328be to 882d5a4 Compare March 2, 2018 00:08

caleybrock requested review from islemaster and Erin007 March 2, 2018 00:08

Erin007 approved these changes Mar 2, 2018

View reviewed changes

caleybrock added 2 commits March 2, 2018 10:12

Code review feedback (mostly typos)

fce9965

Merge remote-tracking branch 'origin/staging' into save-all-rows

29f6fee

islemaster reviewed Mar 2, 2018

View reviewed changes

caleybrock added 2 commits March 2, 2018 12:05

Code review feedback

89fa254

Fix storybook

7520107

caleybrock merged commit a3bac41 into staging Mar 2, 2018

caleybrock deleted the save-all-rows branch March 2, 2018 23:41

caleybrock added teacher-dashboard and removed star-labs labels May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ManageStudentsTable: save all functional #20954

ManageStudentsTable: save all functional #20954

caleybrock commented Mar 1, 2018 •

edited

Erin007 left a comment

Erin007 Mar 2, 2018

Erin007 Mar 2, 2018

Erin007 Mar 2, 2018

caleybrock Mar 2, 2018

Erin007 Mar 2, 2018

islemaster left a comment

islemaster Mar 2, 2018

islemaster Mar 2, 2018

caleybrock Mar 2, 2018

islemaster Mar 2, 2018

islemaster Mar 2, 2018

caleybrock Mar 2, 2018

islemaster Mar 2, 2018

islemaster Mar 2, 2018

islemaster Mar 2, 2018

caleybrock Mar 2, 2018

ManageStudentsTable: save all functional #20954

ManageStudentsTable: save all functional #20954

Conversation

caleybrock commented Mar 1, 2018 • edited

Erin007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caleybrock commented Mar 1, 2018 •

edited