Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hide scatterplot for large schools #2362

Merged
merged 2 commits into from Jan 22, 2019
Merged

Hide scatterplot for large schools #2362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7b01b7
removing scatter option for large schools
edavidsonsawyer Jan 21, 2019
deb15b5
updating specs
edavidsonsawyer Jan 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/assets/javascripts/school_administrator_dashboard/DashboardTestData.js
View file
Open in desktop
Expand Up @@ -68,6 +68,15 @@ export function createStudents(nowMoment) {
id: 1,
absences: [testEvents.oneMonthAgo, testEvents.twoMonthsAgo, testEvents.threeMonthsAgo],
tardies: [testEvents.oneMonthAgo, testEvents.twoMonthsAgo, testEvents.threeMonthsAgo],
discipline_incidents: [{
id:23,
incident_code:"Assault",
created_at: nowMoment.clone().subtract(30, 'days').format(),
incident_location:"Playground",
incident_description:"Description",
occurred_at: nowMoment.clone().subtract(30, 'days').format(),
has_exact_time:true,
student_id:1}],
events: 3,
latest_note: {
event_note_type_id: 300,
Expand Down
12 changes: 10 additions & 2 deletions ...ascripts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.js
View file
Open in desktop
Expand Up @@ -49,6 +49,13 @@ export default class SchoolDisciplineDashboard extends React.Component {
this.memoize = memoizer();
}

//Remove scatter plot option when there are too many incidents to show patterns
componentDidMount() {
if (this.getIncidentsFromStudents(this.props.dashboardStudents).length > 500) {
this.setState({selectedChart: 'incident_location', scatterPlotAvailable: false});
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say more about why this is here, rather than computed in render? I don't follow why it's using the lifecycle hook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's here because the state has to change (from default scatter to some alternative), which can't be done in render. I could probably set the initial state conditionally in the constructor, but it seemed cleaner to just use getIncidentsFromStudents in the mounted component. It shouldn't be much of a performance issue since we cache the result of that function with memoizer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, thanks! Got it, it's because this is set in initialState in the constructor, that's super helpful. Using the lifecycle hook implies that it's because of something related to the DOM or outside of React, and then leads to double-rendering on load.

Yeah, the better way to do this is describe this explicitly in the constructor, and then let initialState decide what selectedChart should be based on those props. Here that would involve factoring out the incident and filtering functions to be plain functions (rather than relying on this.state and this.props) and that seems a larger change than this is worth.

So if you tested this and it works without a visible jump, let me know and let's 🚢 as is. And thanks for explaining and helping me learn! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I have tested this locally with schools above and below the 500 student threshold, and in the former case the scatter plot is cleanly removed. I would like to double check in production just to be sure the threshold is appropriate, but that can probably come from feedback from users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edavidsonsawyer awesome, thanks! 🚢 ing now and I'll verify as well when it's out.

filterIncidents(disciplineIncidents, options = {}) {
return this.memoize(['filteredIncidents', this.state, arguments], () => {
if (!disciplineIncidents) return [];
Expand Down Expand Up @@ -315,10 +322,10 @@ export default class SchoolDisciplineDashboard extends React.Component {

render() {
const {districtKey} = this.context;
const {timeRangeKey, selectedChart, grade, house, counselor} = this.state;
const {scatterPlotAvailable, timeRangeKey, selectedChart, grade, house, counselor} = this.state;
const {school, dashboardStudents} = this.props;
const chartOptions = [
{value: 'scatter', label: 'Day & Time'},
... scatterPlotAvailable ? [{value: 'scatter', label: 'Day & Time'}] : [],
{value: 'incident_location', label: 'Location'},
{value: 'time', label: 'Time'},
{value: 'homeroom_label', label: 'Classroom'},
Expand Down Expand Up @@ -481,6 +488,7 @@ const styles = {
function initialState() {
return {
timeRangeKey: TIME_RANGE_45_DAYS_AGO,
scatterPlotAvailable: true,
selectedChart: 'scatter',
selectedIncidentCode: ALL,
selectedCategory: null,
Expand Down
70 changes: 52 additions & 18 deletions ...pts/school_administrator_dashboard/discipline_dashboard/SchoolDisciplineDashboard.test.js
View file
Open in desktop
@@ -1,6 +1,5 @@
import React from 'react';
import {mount, shallow} from 'enzyme';
import moment from 'moment';
import renderer from 'react-test-renderer';
import {toMomentFromTimestamp} from '../../helpers/toMoment';
import {withNowMoment, withNowContext} from '../../testing/NowContainer';
Expand All @@ -15,20 +14,20 @@ import SchoolDisciplineDashboard from './SchoolDisciplineDashboard';

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

function setDate() {
return toMomentFromTimestamp('2018-09-22T17:03:06.123Z');
}

function testContext(context = {}) {
const nowMoment = toMomentFromTimestamp('2018-09-22T17:03:06.123Z');
const nowMoment = setDate();
return {
districtKey: 'somerville',
nowFn() { return nowMoment; },
...context
};
}

function testEl(moreProps = {}) {
const props = {
dashboardStudents: createStudents(moment.utc()),
...moreProps
};
function testEl(props = {}) {
return <SchoolDisciplineDashboard {...props} />;
}

Expand All @@ -48,80 +47,115 @@ function renderShallow(props = {}) {
return shallow(el, {context});
}

function create501Students() {
const now = setDate();
let students = [];
for (let i=0; i < 501; i++) {
const student = {
first_name: 'Pierrot',
last_name: 'Zanni',
homeroom_label: 'Test 1',
grade: '4',
id: 1,
discipline_incidents: [{
id:23,
incident_code:"Assault",
created_at: now.clone().subtract(30, 'days').format(),
incident_location:"Playground",
incident_description:"Description",
occurred_at: now.clone().subtract(30, 'days').format(),
has_exact_time:true,
student_id:1}],
events: 3
};
students.push(student);
}
return students;
}

it('displays house for SHS', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testHighSchool()};
const props = {school: testHighSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectHouse').length > 0).toEqual(true);
});

it('does not display house for Healey', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectHouse').length > 0).toEqual(false);
});

it('displays counselor for SHS', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testHighSchool()};
const props = {school: testHighSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectCounselor').length > 0).toEqual(true);
});

it('does not display counselor for Healey', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectCounselor').length > 0).toEqual(false);
});

it('displays grade for SHS', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testHighSchool()};
const props = {school: testHighSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectGrade').length > 0).toEqual(true);
});

it('displays grade for Healey', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectGrade').length > 0).toEqual(true);
});

it('renders a scatter plot', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('DisciplineScatterPlot').length > 0).toEqual(true);
});

it('does not render a scatter plot when there are more than 500 incidents', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool(), dashboardStudents: create501Students(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('DisciplineScatterPlot').length === 0).toEqual(true);
expect(dash.find('DashboardBarChart').length > 0).toEqual(true);
});

it('renders a student list', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('StudentsTable').length > 0).toEqual(true);
});

it('renders a date range selector', () => {
const context = testContext({districtKey: 'somerville'});
const props = {school: testSchool()};
const props = {school: testSchool(), dashboardStudents: createStudents(setDate())};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('SelectTimeRange').length > 0).toEqual(true);
});

it('can render a bar chart', () => {
const dash = renderShallow({school: testSchool()});
const dash = renderShallow({school: testSchool(), dashboardStudents: createStudents(setDate())});
dash.setState({selectedChart: 'grade'});
expect(dash.find('DashboardBarChart').length > 0).toEqual(true);
});
Expand All @@ -130,7 +164,7 @@ function snapshotJson(districtKey) {
return renderer.create(
withNowContext('2018-09-22T17:03:06.123Z',
<PerDistrictContainer districtKey={districtKey}>
{pageSizeFrame(testEl({school: testSchool()}))}
{pageSizeFrame(testEl({school: testSchool(), dashboardStudents: createStudents(setDate())}))}
</PerDistrictContainer>
));
}
Expand Down