Add House, Grade, and Counselor Filters to Discipline Dashboard #2186

edavidsonsawyer · 2018-10-16T00:23:11Z

Who is this PR for?

Discipline dashboard users

What problem does this PR fix?

To be useful in high schools, educators need to be able to look at discipline incidents by house, grade, and counselor.

What does this PR do?

Adds a top level house filter for schools that support it. cf #1661

Screenshot (if adding a client-side feature)

Checklists

Which features or pages does this PR touch?

Discipline

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

Included specs for changes
Improved specs for existing code in need of better test coverage
Manual testing made more sense here

kevinrobinson · 2018-10-16T12:50:03Z

@edavidsonsawyer The tests are failing here, and the checklists in the pull request description aren't finished, so I didn't review this, but let me know when I should!

re: design, in #1661 (comment) the issue is to add filters for grade, house and counselor. If you want to ship those incrementally, that's great! If shipping incrementally, then the order matters so we don't reorder the filters on folks with each change we ship :)

You can use the absences dashboard as a model here, with the specific bits (eg, unexcused / incident type) coming first, and then the same other filters coming after in the same order:

edavidsonsawyer · 2018-10-16T13:05:29Z

@kevinrobinson yeah, this and the other PR are still WIP, I'll ping you when they're all set. I hadn't thought about reordering the filters - I was trying to get this quickly useful for high school educators, but it shouldn't be a problem to add all filters to this PR.

edavidsonsawyer · 2018-10-18T13:18:38Z

app/lib/dashboard_queries.rb

@@ -96,7 +96,9 @@ def shared_student_fields(student)
      :first_name,
      :last_name,
      :grade,
-      :id
+      :id,


@kevinrobinson I wanted to call this out since it also means sending that info to the tardies dashboard. My thinking is eventually we'll want to add these filters there as well, and it doesn't do any harm, but if we want to keep this compartmentalized then this can be done specifically for the data sent in the discipline api.

@kevinrobinson ignore this for now, though, still some work to be done on the tests before this all gets reviewed

edavidsonsawyer · 2018-10-24T13:50:47Z

@kevinrobinson All right, I've tested out these filters with and without large incident counts. They all look like they work and appear when needed based on the school. When you have the chance, I'd appreciate a code review.

kevinrobinson

@edavidsonsawyer This looks great! I reviewed this based on your comment, but please also update the pull request description to match too. :) And sorry on the delay here, I was out a bit this week and probably should have sent you a note about that.

This seems good, most of the comments here are minor style suggestions. The most meaningful bit for me is about how the "grade filter" looks like it's currently stored in two separate bits of state, depending on whether the user used the select box, or clicked on a chart. I wonder if there's an opportunity to simplify that to just one bit of state?

kevinrobinson · 2018-10-26T16:49:12Z

...cripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.test.js

  ));
+}
+
+it('displays house for SHS', () => {


nice tests! 👍

kevinrobinson · 2018-10-26T16:52:49Z

...cripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.test.js


+jest.mock('react-virtualized'); // doesn't work in test without a real DOM


FWIW if you want to use snapshot tests on something that uses react-virtualized you can do that using a similar approach to the one in #2196. It's fine to not do that here, just sharing about a new way to approach this that I was trying.

kevinrobinson · 2018-10-26T16:54:04Z

...javascripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.js

+      if (house !== ALL && student.house !== house) return false;
+      if (counselor !== ALL && student.counselor !== counselor) return false;
+      if (selectedCategory && shouldFilterSelectedCategory) { //used by the student list when a user selects a category within a chart
+        if (selectedChart === 'grade' && student.grade !== selectedCategory) return false;


This seems like there are two separate bits of state related to the grade filtering. One is for if the user uses the select filter, and the other is if they click on a grade within a chart.

Could these be a single bit of state? In other words, when the user clicks on a grade within a chart, would it simplify to call this.setState({grade}) or to call this.setsState({homeroom})?

We could certainly do that. It might be awkward to treat selecting a grade within a chart different from selecting other categories within a chart. We would still have need state for selectedCategory, but have an exception when that category is a grade. Since there are two ways you might filter a grade and the application behavior is different depending on which is selected, it seemed to make sense to hold that as two different bits of state.

I imagine moving forward that we might not need the grade/homeroom charts with the top level filters, in which case this problem should go away.

Thanks, this is super helpful! 👍 Sounds good to just keep them distinct then and 🚢 this.

kevinrobinson · 2018-10-26T16:54:07Z

...javascripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.js

@@ -187,7 +204,7 @@ export default class SchoolDisciplineDashboard extends React.Component {
  }

  studentTableRows(students) {
-    const filteredStudents = this.filterStudents(students);
+    const filteredStudents = this.filterStudents(students, true);


If you revise the argument to be options = {} this can help make it more obvious what's happening at the call site. Right now, reading this line of code, there's no indication of what true means.

Using the options = {} argument let's you write code like this at the call site:

this.filterStudents(students, { shouldFilterSelectedCategory: true })

and express explicitly what you want that function to be doing. I'd recommend this as a good rule of thumb in general.

Thanks for this! I was sure there was a better syntax I was missing.

kevinrobinson · 2018-10-26T16:54:09Z

...javascripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.js

+  allIncidentTypes() {
+    const {dashboardStudents} = this.props;
+    const incidents = _.uniqBy(_.compact(_.flatten(dashboardStudents.map(student => student.discipline_incidents))), 'incident_code');
+    return incidents.map(incident => incident.incident_code).sort();


As a small style suggestions, if this is just trying to get all the codes, maybe flat map then uniq then sort (like you do in the other methods)? It doesn't seem like the whole object of incidents is used.

This was an attempt to make it more readable, since this one has to iterate through students and then incidents. Are you saying it would be preferable to have it all on one line?

Yeah trying to make things more readable is great! My comment is a tiny one, but more about what's happening here logically. This is what I see in the code:

first, it grabs the incidents from each student and flattens that into an array

then it runs compact, which I think won't do anything (it shouldn't be possible for nil values to be here)

then it uniques by incident_code to get the list of incidents

then it maps the incidents down to just incident_code and sorts

So it looks to me like compact isn't doing anything here, and that if the map / uniq were swapped, then that would allow removing the repetition of doing something by incident_code twice in two lines. Concretely, it's sort of the difference between these two lines:

_.uniq(_.map(incidents, 'incident_code')) _.map(_.uniqBy(incidents, 'incident_code'), 'incident_code')

Also this is a super tiny thing, please just go with what you think is most readable and best, we don't have to keep 🚲 shedding it, and also it's fine if you disagree :)

kevinrobinson · 2018-10-26T16:55:31Z

...cripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.test.js

+  const props = {school: testSchool};
+  const el = testEl(props);
+  const dash = mount(elWrappedInContext(el, context));
+  expect(dash.find('SelectTimeRange').length > 0).toEqual(true);


Given that this code now works differently per district, it would be good to add at least smoke tests for that. Here's how this works in the absences page: https://github.com/studentinsights/studentinsights/blob/master/app/assets/javascripts/school_absences/SchoolAbsencesDashboard.test.js#L211

@kevinrobinson I've got most of your suggestions implemented, but the CI build is hassling me about these snapshots. Is there something I should be sending in this branch to be able to write snapshots?

@edavidsonsawyer Yeah! So with snapshots, you need to update this locally and push up the changes.

Conceptually, snapshots are the same as any other test - your PR has to include how the expectations for the tests are different. The only wrinkle is that Jest makes the snapshot expectations for you. It should prompt you to do this when you run yarn test locally, something like this:

Here's a direct link to the Jest docs about this: https://jestjs.io/docs/en/snapshot-testing.html#updating-snapshots or if you our setup isn't working quite right or is confusing, let me know and I can take a look with you so we can make this work nicely and remove any friction.

@kevinrobinson got it now, sorry that took so long. I forgot to merge master into this branch and it was messing up the snapshots here, but not locally.

@edavidsonsawyer no worries! Is this ready to change from WIP and to deploy now? I can do that later today if this is ready to 🚢 👍

@kevinrobinson sure is! thanks again for the review!

no problemo!

kevinrobinson · 2018-10-26T16:57:07Z

app/assets/javascripts/school_administrator_dashboard/DashboardTestData.js

+export const testSchool = {
+  local_id: 'HEA',
+  name: 'Arthur D. Healey'
+};


Can you say more about changing these from functions to shared global objects? One benefit of functions is that there's no possibility of a test polluting the value, since you're creating a new test object each time. If it's a global object, it opens up a class of bugs where test code or real code could update the object and break completely unrelated tests.

Ah, you're right. I was thinking of this as static fixture data, but there's no reason to switch while it's still a shared component.

edavidsonsawyer added 2 commits October 14, 2018 22:44

WIP

5a1009f

adding house filter

7cba33c

edavidsonsawyer mentioned this pull request Oct 16, 2018

Discipline: Remove selection labels from table #2173

Closed

edavidsonsawyer added 2 commits October 15, 2018 21:32

unused props

f823b0d

update spec

0e2fb4f

edavidsonsawyer changed the title ~~Add house filter to discipline dashboard~~ WIP Add house filter to discipline dashboard Oct 16, 2018

edavidsonsawyer added 3 commits October 17, 2018 09:10

add grade and counselor

dd9adf3

adding counselor to dashStudent attributes

799c1b8

adopting narrowselect style to fit

e34ce03

edavidsonsawyer changed the title ~~WIP Add house filter to discipline dashboard~~ Add House, Grade, and Counselor Filters to Discipline Dashboard Oct 18, 2018

edavidsonsawyer commented Oct 18, 2018

View reviewed changes

update spec to catch new dashboard query fields

3308d3a

edavidsonsawyer changed the title ~~Add House, Grade, and Counselor Filters to Discipline Dashboard~~ WIP Add House, Grade, and Counselor Filters to Discipline Dashboard Oct 18, 2018

edavidsonsawyer added 3 commits October 24, 2018 00:35

updating test cases and improving test fixtures

905e394

unused shallow now

fcc92ec

spellcheck

86ac8c5

kevinrobinson reviewed Oct 26, 2018

View reviewed changes

edavidsonsawyer added 5 commits October 28, 2018 12:20

better function readability

5146c8a

improving dashboard tests

a4e9114

snapshots

cfb50c9

Merge branch 'master' into discipline_add_house

a159ffd

merge master and update snaps

6bfa3db

edavidsonsawyer changed the title ~~WIP Add House, Grade, and Counselor Filters to Discipline Dashboard~~ Add House, Grade, and Counselor Filters to Discipline Dashboard Oct 29, 2018

kevinrobinson merged commit e2b739b into studentinsights:master Oct 29, 2018

kevinrobinson mentioned this pull request May 31, 2019

Absences and discipline: Add dropdown to filter by Grade, House and Counselor #1661

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add House, Grade, and Counselor Filters to Discipline Dashboard #2186

Add House, Grade, and Counselor Filters to Discipline Dashboard #2186

edavidsonsawyer commented Oct 16, 2018 •

edited

Loading

kevinrobinson commented Oct 16, 2018 •

edited

Loading

edavidsonsawyer commented Oct 16, 2018

edavidsonsawyer Oct 18, 2018

edavidsonsawyer Oct 18, 2018

edavidsonsawyer commented Oct 24, 2018

kevinrobinson left a comment

kevinrobinson Oct 26, 2018

kevinrobinson Oct 26, 2018

kevinrobinson Oct 26, 2018

edavidsonsawyer Oct 26, 2018

kevinrobinson Oct 26, 2018

kevinrobinson Oct 26, 2018

edavidsonsawyer Oct 26, 2018

kevinrobinson Oct 26, 2018

edavidsonsawyer Oct 29, 2018

kevinrobinson Oct 29, 2018

kevinrobinson Oct 26, 2018

edavidsonsawyer Oct 29, 2018

kevinrobinson Oct 29, 2018

edavidsonsawyer Oct 29, 2018

kevinrobinson Oct 29, 2018

edavidsonsawyer Oct 29, 2018

kevinrobinson Oct 29, 2018

kevinrobinson Oct 26, 2018

edavidsonsawyer Oct 29, 2018


		jest.mock('react-virtualized'); // doesn't work in test without a real DOM

Add House, Grade, and Counselor Filters to Discipline Dashboard #2186

Add House, Grade, and Counselor Filters to Discipline Dashboard #2186

Conversation

edavidsonsawyer commented Oct 16, 2018 • edited Loading

Who is this PR for?

What problem does this PR fix?

What does this PR do?

Screenshot (if adding a client-side feature)

Checklists

kevinrobinson commented Oct 16, 2018 • edited Loading

edavidsonsawyer commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edavidsonsawyer commented Oct 24, 2018

kevinrobinson 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

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

edavidsonsawyer commented Oct 16, 2018 •

edited

Loading

kevinrobinson commented Oct 16, 2018 •

edited

Loading