Write 'move students' functionality in React

[maddiedierker]

This is the first of what will be a few PRs to re-write the 'move students' functionality under the 'manage students' tab in React. Here is the ticket in Axosoft and a quicklink to the spec in Google Docs.

Currently, 'move students' is written in Angular and looks like this:

For this PR, I have set up the basic dialog in React and populated it with student names for that section in a sortable table:

The MoveStudents component is not hooked up in the Manage Students tab yet, but there is a storybook included to demonstrate functionality and styles.

This is my first PR with actual code in it, so please nit!

[islemaster]

Awesome work Maddie! I'm hard-pressed to find anything to criticize here. You've done good work and picked up on our code style really quick.

The one behavior I noticed that I would change is that we've got a custom onSort that removes the unsorted state from the rotation, but that's still the initial state of the table. It would be nice if the table was initially sorted by name, A-Z.

Great test coverage: The only holes seem to be the formatter functions and the onSort function (which I've recommended extracting anyway). This is pretty natural since you're shallow-rendering in the unit tests and we might decide it's fine because we'll add a UI test later that actually depends on all these parts working. Although, if you haven't already you might explore using enzyme's mount() instead of shallow() here so that the formatters get covered.

Another thing I have mixed feelings about is all the unit tests against the helper methods in your component (I suspect this is also because of shallow-rendering?). In my opinion something closer to black-box testing for your component would be better: Tests that use enzyme's event simulation and verify rendered output, instead of calling helpers and checking their return value or the final component state object. As is, none of your tests actually check the results of the component render method. Tests over internal methods can be brittle in the long run, too.

All that said, I'm am thoroughly impressed and fine with a merge as-is and working on these concerns in a future iteration. Good work!

[islemaster]

Some weird indentation here, and below. Maybe:

return [
  {
    property: 'selected',
    header: {

[islemaster]

We use this in eleven places now. Let's extract it somewhere! Maybe into wrapped_sortable.js (which should be renamed to camelCase).

[islemaster]

I'd specify exactly what you need in the shape for studentData:

PropTypes.arrayOf(
  React.PropTypes.shape({
    id: PropTypes.number.isRequired,
    name: PropTypes.string.isRequired,
  })
).isRequired

[islemaster]

I don't think it needs a label - can remove this TODO.

[islemaster]

😆 Here you've tested that the dialog stays closed when it's already closed and you call closeDialog(). That is an important behavior, but maybe not the one you meant to cover?

This is also interesting because in your implementation those two cases are the same code path - but I could imagine a world where some future developer refactors this component and breaks one case but not the other.

[maddiedierker]

😱 copy-pasting tests wins again

[islemaster]

Also, I tend to be overly verbose and make unneeded suggestions in reviews. Feel free to push back on anything you disagree with. We're all about that.

[maddiedierker]

@islemaster this is great, thank you for such a thorough review! I'm going to push some of the smaller and/or more pressing changes you mentioned to this branch, and have made note of everything else for my next iteration on this feature (specifically unit tests since those will be changing quite a bit with my next PR and I can use that as a chance to refactor them more thoroughly).

[davidsbailey]

Great work Maddie! This is an awesome amount of code to crank out for your first PR, and the test coverage looks great too. It looks like Brad has given you some very thorough feedback -- I don't have anything specific to add.