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.

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

Use estimated data for trend charts #707

Merged
merged 15 commits into from
Apr 26, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"lodash.flatten": "^4.4.0",
"lodash.lowercase": "^4.3.0",
"lodash.range": "^3.2.0",
"lodash.snakecase": "^4.1.1",
"lodash.startcase": "^4.4.0",
"lodash.throttle": "^4.1.1",
"lodash.upperfirst": "^4.3.1",
Expand Down
16 changes: 12 additions & 4 deletions src/components/Explorer.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,13 @@ class Explorer extends React.Component {

componentDidMount() {
const { appState, dispatch, router } = this.props
const { placeType, since, until } = appState.filters
const { query } = router.location
const { since, until } = appState.filters
const { place } = getPlaceInfo(appState.filters)
let { place } = appState.filters

if (!place && !placeType) {
place = nationalKey
Copy link
Contributor

@brendansudol brendansudol Apr 25, 2017

Choose a reason for hiding this comment

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

why did u replace the getPlaceInfo method in componentDidMount and render? i think that helps consolidate this place assignment logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I think that just got removed when I was fixing rebase conflicts. I'll look for it and take a second look

}

const clean = (val, alt) => {
const yr = +val
Expand Down Expand Up @@ -64,7 +68,12 @@ class Explorer extends React.Component {
render() {
const { appState, dispatch, params, router } = this.props
const { crime } = params
const { place, placeType } = getPlaceInfo(params)
let { place, placeType } = params

if (!params.place && !params.placeType) {
place = nationalKey
placeType = 'national'
}

// show not found page if crime or place unfamiliar
if (!offenses.includes(crime) || !lookup(place, placeType)) {
Expand Down Expand Up @@ -123,7 +132,6 @@ class Explorer extends React.Component {
placeType={placeType}
since={filters.since}
summaries={summaries}
ucr={ucr}
until={filters.until}
/>
{showNibrs && (<NibrsContainer
Expand Down
2 changes: 1 addition & 1 deletion src/components/TimePeriodFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import range from 'lodash.range'
import React from 'react'


const MIN_YEAR = 1960
const MIN_YEAR = 1995
const MAX_YEAR = 2014
const YEAR_RANGE = range(MIN_YEAR, MAX_YEAR + 1)

Expand Down
2 changes: 1 addition & 1 deletion src/components/TrendChart.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ class TrendChart extends React.Component {
</div>
<div className='my1 fs-10 sm-fs-12 monospace center'>
<div className='bold'>{startCase(crime)} rate per 100,000 people</div>
<div className='italic'>(Does not include estimates)</div>
<div className='italic'>(Crime counts include FBI estimates)</div>
</div>
<DownloadDataBtn data={download} />
</div>
Expand Down
47 changes: 13 additions & 34 deletions src/components/TrendContainer.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import snakeCase from 'lodash.snakecase'
import startCase from 'lodash.startcase'
import PropTypes from 'prop-types'
import React from 'react'
Expand All @@ -6,51 +7,33 @@ import Loading from './Loading'
import NoData from './NoData'
import TrendChart from './TrendChart'
import TrendSourceText from './TrendSourceText'
import mungeSummaryData from '../util/summary'


const dataByYear = data => (
Object.assign(
...Object.keys(data).map(k => ({
[k]: Object.assign(...data[k].map(d => ({ [d.year]: d }))),
})),
)
)

const mungeSummaryData = (summaries, ucr, place) => {
if (!summaries || !summaries[place]) return false

const keys = Object.keys(summaries)
const summaryByYear = dataByYear(summaries)
const ucrByYear = dataByYear(ucr)

return summaries[place].map(d => (
Object.assign(
{ date: d.year },
...keys.map(k => {
const count = summaryByYear[k][d.year].actual
const pop = ucrByYear[k][d.year].total_population
return { [k]: { count, pop, rate: (count / pop) * 100000 } }
}),
)
))
const filterDataWithinYears = ({ data, since, until }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if this function should be in '../util/summary' too and consolidated within the mungeSummaryData call to avoid multiples passes through the data

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this could do the trick:

const mungeSummaryData = (crime, summaries, place, since, until) => {
  if (!summaries || !summaries[place]) return false

  const keys = Object.keys(summaries)
  return summaries[place]
    .filter(d => d.year >= since && d.year <= until)
    .map(year => {
      const data = { date: year.year }
      keys.forEach(key => {
        const source = key !== place
          ? summaries[key].find(d => d.year === data.date)
          : year
        data[key] = {
          pop: source.population,
          count: source[crime],
          rate: source[crime] / source.population * 100000,
        }
      })
      return data
    })
}

note the filter call before the map (and new since and until params)

const places = Object.keys(data)
const yearData = places.map(place => ({
[place]: data[place].filter(d => d.year <= until && d.year >= since)
}))
return Object.assign(...yearData)
}

const TrendContainer = ({
crime,
dispatch,
place,
placeType,
dispatch,
since,
summaries,
ucr,
until,
}) => {
const loading = summaries.loading || ucr.loading
const loading = summaries.loading

let content = null
if (loading) content = <Loading />
else {
const data = mungeSummaryData(summaries.data, ucr.data, place)
const dataWithinYears = filterDataWithinYears({ data: summaries.data, since, until })
const data = mungeSummaryData(snakeCase(crime), dataWithinYears, place)
if (!data || data.length === 0) content = <NoData />
else {
content = (
Expand Down Expand Up @@ -100,11 +83,7 @@ TrendContainer.propTypes = {
data: PropTypes.object,
loading: PropTypes.boolean,
}).isRequired,
ucr: PropTypes.shape({
data: PropTypes.object,
loading: PropTypes.boolean,
}).isRequired,
until: PropTypes.number.isRequired,
until: React.PropTypes.number.isRequired,
}

export default TrendContainer
37 changes: 11 additions & 26 deletions src/util/api.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import upperFirst from 'lodash.upperfirst'

import { get } from './http'
import { mapToApiOffense, mapToApiOffenseParam } from './offenses'
import { mapToApiOffense } from './offenses'
import lookupUsa, { nationalKey } from './usa'


Expand Down Expand Up @@ -50,46 +50,31 @@ const getNibrsRequests = params => {
return slices.map(s => getNibrs({ ...s, crime, place }))
}

const buildSummaryQueryString = params => {
const { crime, place, since, until } = params
const offense = mapToApiOffense(crime)
const offenseParam = mapToApiOffenseParam(crime)

const qs = [
`${offenseParam}=${offense}`,
`per_page=${(until - since) + 1}`,
`year>=${since}`,
`year<=${until}`,
]

if (place && place !== nationalKey) {
qs.push(`state=${lookupUsa(params.place)}`)
}

return qs.join('&')
}

const getSummary = params => {
const { place } = params
const endpoint = `${API}/counts`
const qs = buildSummaryQueryString(params)
let endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

would use ternary here to save a few lines:

const endpoint = (place === nationalKey)
  ? `${API}/estimates/national`
  : `${API}/estimates/states/${lookupUsa(place).toUpperCase()}`

if (place === nationalKey) {
endpoint = `${API}/estimates/national`
} else {
endpoint = `${API}/estimates/states/${lookupUsa(place).toUpperCase()}`
}

return get(`${endpoint}?${qs}`).then(response => ({
return get(`${endpoint}?per_page=50`).then(response => ({
place,
results: response.results,
}))
}

const getSummaryRequests = params => {
const { crime, place, since, until } = params
const { place, since, until } = params

const requests = [
getSummary({ crime, place, since, until }),
getSummary({ place, since, until }),
]

// add national summary request (unless you already did)
if (place !== nationalKey) {
requests.push(getSummary({ crime, place: nationalKey, since, until }))
requests.push(getSummary({ place: nationalKey, since, until }))
}

return requests
Expand Down
17 changes: 1 addition & 16 deletions src/util/offenses.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,6 @@ const propertyCrime = [
]

const crimeTypes = { violentCrime, propertyCrime }

const offenseParams = {
'aggravated-assault': 'explorer_offense',
arson: 'explorer_offense',
burglary: 'explorer_offense',
'larceny-theft': 'explorer_offense',
'motor-vehicle-theft': 'explorer_offense',
homicide: 'explorer_offense',
'property-crime': 'classification',
rape: 'explorer_offense',
robbery: 'explorer_offense',
'violent-crime': 'classification',
}

const mapToApiOffense = o => offenses[o] || slugify(o)
const mapToApiOffenseParam = o => offenseParams[o]

export { ids as default, mapToApiOffense, mapToApiOffenseParam, crimeTypes }
export { ids as default, mapToApiOffense, crimeTypes }
19 changes: 19 additions & 0 deletions src/util/summary.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const mungeSummaryData = (crime, summaries, place) => {
if (!summaries || !summaries[place]) return false

const keys = Object.keys(summaries)
return summaries[place].map(year => {
const data = { date: year.year }
keys.forEach(key => {
const source = key !== place ? summaries[key].find(d => d.year === data.date) : year
data[key] = {
pop: source.population,
count: source[crime],
rate: (source[crime] / source.population) * 100000,
}
})
return data
})
}

export default mungeSummaryData
17 changes: 13 additions & 4 deletions test/util/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import sinon from 'sinon'

import api from '../../src/util/api'
import * as http from '../../src/util/http'
import { nationalKey } from '../../src/util/usa'


const createPromise = (res, err) => {
Expand Down Expand Up @@ -72,12 +73,20 @@ describe('api utility', () => {
})

describe('getSummary()', () => {
it('should call the /count endpoint', done => {
it('should request /estimates/national for national', done => {
const spy = sandbox.stub(http, 'get', () => createPromise(success))
api.getSummary(params).then(() => {
api.getSummary({ place: nationalKey }).then(() => {
const url = spy.args[0].pop()
expect(url.includes('/count')).toEqual(true)
expect(url.includes('?explorer_offense=homicide')).toEqual(true)
expect(url.includes('/estimates/national')).toEqual(true)
done()
})
})

it('should request /estimates/states/:state if place is a state', done => {
const spy = sandbox.stub(http, 'get', () => createPromise(success))
api.getSummary({ place: 'california' }).then(() => {
const url = spy.args[0].pop()
expect(url.includes('/estimates/states/CA')).toEqual(true)
done()
})
})
Expand Down
1 change: 1 addition & 0 deletions test/util/history.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ describe('history utility', () => {
location: { query: {} },
params: { crime: 'murder', place: 'oregon' }
}

it('should change the place value in pathname if it is a state', () => {
const change = { place: 'california' }
const router = Object.assign({}, mockRouter)
Expand Down
14 changes: 1 addition & 13 deletions test/util/offenses.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint no-undef: 0 */

import ids, { mapToApiOffense, mapToApiOffenseParam } from '../../src/util/offenses'
import ids, { mapToApiOffense } from '../../src/util/offenses'


describe('offenses utility', () => {
Expand All @@ -14,16 +14,4 @@ describe('offenses utility', () => {
expect(actual).toEqual('aggravated-assault')
})
})

describe('mapToApiOffenseParam', () => {
it('should return "classification" for violent crime', () => {
const actual = mapToApiOffenseParam('violent-crime')
expect(actual).toEqual('classification')
})

it('should return "explorer_offense" for homicide', () => {
const actual = mapToApiOffenseParam('homicide')
expect(actual).toEqual('explorer_offense')
})
})
})
34 changes: 34 additions & 0 deletions test/util/summary.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/* eslint no-undef: 0 */

import mungeSummaryData from '../../src/util/summary'


describe('summary data munging utility', () => {
it('should return false if summaries is not supplied', () => {
const crime = 'violent-crime'
expect(mungeSummaryData(null, crime)).toEqual(false)
})

it('should return false if summaries[place] is not supplied', () => {
const crime = 'violent-crime'
const summaries = {}
const place = 'california'
expect(mungeSummaryData(crime, summaries, place)).toEqual(false)
})

it('should reshape the data', () => {
const crime = 'violent-crime'
const summaries = {
california: [{ year: 2014, 'violent-crime': 10, population: 100 }],
'united-states': [{ year: 2014, 'violent-crime': 10, population: 100 }],
}
const place = 'california'
expect(mungeSummaryData(crime, summaries, place)).toEqual([
{
date: 2014,
california: { count: 10, pop: 100, rate: 10000 },
'united-states': { count: 10, pop: 100, rate: 10000 },
}
])
})
})