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

refactor prioritize #244

Merged
merged 1 commit into from Apr 14, 2017

Conversation

Projects
None yet
2 participants
@EthanJStark
Collaborator

EthanJStark commented Apr 14, 2017

No description provided.

@EthanJStark EthanJStark changed the title from 240 refactor prioritize to WIP refactor prioritize Apr 14, 2017

@jrob8577

Love the file organization.

Didn't go through the tests too closely...

Show outdated Hide outdated public/frontend/prioritization/active.js
const active = ( requests, coachId ) =>
requests.map( request => Object.assign( {}, request, { active: isActive( request, coachId ) }))
//TODO throw error if not given 2nd arg coachId

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

I'm not a fan of throwing an error - the function should be invoked correctly.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

I'm not a fan of throwing an error - the function should be invoked correctly.

Show outdated Hide outdated tests/fixtures/requests/index.js
goal: { coach_id: coach_id || DEFAULT_COACH_ID }
})
const REQUESTS = [

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Do we still need these? If so, we should generate entries using the request function, for consistency.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Do we still need these? If so, we should generate entries using the request function, for consistency.

Show outdated Hide outdated tests/frontend/prioritization/active.js
describe( 'active', () => {
describe( 'when no requests are active', () => {
it( 'returns array with no active requests', () => {
const input = [{}].map( item => request( item ))

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

I'm regretting the use of a function like this now in order to generate a valid request object... it makes it unclear in the test that we are generating a request. Maybe we could refactor our request function to be a member of an object that makes it clear that we are generating test mocks:

const Factory = {
  request
}

Factory.request()
Factory.request({ created_at: `whatever` })
Factory.request({ coach_id: `1234-5678` })
@jrob8577

jrob8577 Apr 14, 2017

Contributor

I'm regretting the use of a function like this now in order to generate a valid request object... it makes it unclear in the test that we are generating a request. Maybe we could refactor our request function to be a member of an object that makes it clear that we are generating test mocks:

const Factory = {
  request
}

Factory.request()
Factory.request({ created_at: `whatever` })
Factory.request({ coach_id: `1234-5678` })
@EthanJStark

This comment has been minimized.

Show comment
Hide comment
@EthanJStark

EthanJStark Apr 14, 2017

Collaborator

For your review!

All tests are passing, but can't run them unless you move frontend to root directory and uncomment require statements.

Collaborator

EthanJStark commented Apr 14, 2017

For your review!

All tests are passing, but can't run them unless you move frontend to root directory and uncomment require statements.

const active = ( requests, coachId ) =>
requests.map( request => Object.assign( {}, request, { active: isActive( request, coachId ) }))
const isActive = ( request, coachId ) => request.events.some( ({ data }) => data.claimed_by === coachId )

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Can't active also mean escalated by me at some point?

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Can't active also mean escalated by me at some point?

This comment has been minimized.

@EthanJStark

EthanJStark Apr 14, 2017

Collaborator

It is the case that an escalated-by-me request has also been previously claimed-by-me, so isActive will catch all relevant requests.

@EthanJStark

EthanJStark Apr 14, 2017

Collaborator

It is the case that an escalated-by-me request has also been previously claimed-by-me, so isActive will catch all relevant requests.

Show outdated Hide outdated public/frontend/prioritization/prioritize.js
const prioritize = requests =>
requests.map( request =>
Object.assign( {}, request, { priority: calculatePriority( request ) }))

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting off... I suggest closing the map on a new line, and applying sort on that line, with proper alignment (sort is at the same level as requests.map)

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting off... I suggest closing the map on a new line, and applying sort on that line, with proper alignment (sort is at the same level as requests.map)

Show outdated Hide outdated public/javascripts/coach.js
const visibleRequests = decoratedSortedRequests.filter( request => request.visible )
const activeRequests = decoratedSortedRequests.filter( request => request.active )
.map( request => Object.assign( {}, request, { escalatable:
!request.events.some( ({ data }) => data.escalated_by === userId )} ))

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Isn't this notEscalatedByMe?

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Isn't this notEscalatedByMe?

Show outdated Hide outdated public/javascripts/coach.js
@@ -149,6 +126,7 @@ const escalationClick = event => {
const { request_id } = event.target.dataset
fetch( '/events', params( 'post', { request_id, name: 'escalate' }))
.then( _ => window.location.reload( true ) )

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This was intentionally removed - refresh should happen on event publication...

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This was intentionally removed - refresh should happen on event publication...

This comment has been minimized.

@EthanJStark

EthanJStark Apr 14, 2017

Collaborator

@jrob8577 It's not refreshing without this . . . Do you have any insights on where we could look to see why it's not?

@EthanJStark

EthanJStark Apr 14, 2017

Collaborator

@jrob8577 It's not refreshing without this . . . Do you have any insights on where we could look to see why it's not?

Show outdated Hide outdated public/javascripts/coach.js
@@ -157,6 +135,7 @@ const claimClick = event => {
const { request_id } = event.target.dataset
fetch( '/events', params( 'post', { request_id, name: 'claim' }))
.then( _ => window.location.reload( true ) )

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This was intentionally removed - refresh should happen on event publication...

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This was intentionally removed - refresh should happen on event publication...

Show outdated Hide outdated tests/frontend/prioritization/visible.js
describe( 'notEscalatedByMe', () => {
describe( 'when the request was escalated by me', () => {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

Show outdated Hide outdated tests/frontend/prioritization/visible.js
})
describe( 'when the request was escalated by another coach', () => {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

Show outdated Hide outdated tests/frontend/prioritization/visible.js
})
describe( 'when the request has not been escalated', () => {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first it

Show outdated Hide outdated tests/frontend/prioritization/prioritize.js
describe( 'calculatePriority', () => {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first describe

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Remove extraneous whitespace between describe and first describe

Show outdated Hide outdated tests/frontend/prioritization/prioritize.js
describe( 'comparing one older and one newer request', () => {
const newerRequest = Factory.request( {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Move test cases into it

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Move test cases into it

Show outdated Hide outdated public/javascripts/coach/templates.js
@@ -33,7 +33,7 @@ const queueTemplate = ({ id, goal, created_at, events, players, claimable, escal
}[ events[ events.length - 1 ].name ]
return `
<div class="panel panel-${type}" data-created-at="${created_at}">
<div class="panel panel-${type}" data-created_at="${created_at}">

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This should be created-at...

@jrob8577

jrob8577 Apr 14, 2017

Contributor

This should be created-at...

Show outdated Hide outdated public/javascripts/coach.js
@@ -126,7 +99,7 @@ load()
const buttons = className => {
const elements = document.querySelectorAll( className )
if( elements !== null ) {
if ( elements !== null ) {

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

There should not be a space here.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

There should not be a space here.

Show outdated Hide outdated public/javascripts/coach.js
const visibleRequests = decoratedSortedRequests.filter( request => request.visible )
const activeRequests = decoratedSortedRequests.filter( request => request.active )
.map( request => Object.assign( {}, request, { escalatable:
!request.events.some( ({ data }) => data.escalated_by === userId )} ))

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting is hard to understand. Close map ) should align with map

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting is hard to understand. Close map ) should align with map

goal: { coach_id: coach_id || DEFAULT_COACH_ID }
})
}

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

remove extra space

@jrob8577

jrob8577 Apr 14, 2017

Contributor

remove extra space

Show outdated Hide outdated tests/frontend/prioritization/prioritize.js
const sortedRequests = prioritize(testRequests)
expect( sortedRequests.map( request => request.events[ request.events.length - 1 ].created_at ))
.to.deep.equal([ priorityOneEvent.events[ priorityOneEvent.events.length - 1 ].created_at,

This comment has been minimized.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting hard to read.

@jrob8577

jrob8577 Apr 14, 2017

Contributor

Formatting hard to read.

[Fixes #240] Claimable bug, refactor prioritize()
* Complete refactor of how we sort, filter, and decorate requests
* Wrote and tested small helper functions that serve active, visible, and prioritize

@EthanJStark EthanJStark changed the title from WIP refactor prioritize to refactor prioritize Apr 14, 2017

@EthanJStark EthanJStark merged commit cdfc2c7 into GuildCrafts:master Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment