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

Posts: Add posts Redux state handlers #2248

Merged
merged 6 commits into from
Jan 12, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions client/state/action-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ export const FETCH_SITE_PLANS = 'FETCH_SITE_PLANS';
export const FETCH_SITE_PLANS_COMPLETED = 'FETCH_SITE_PLANS_COMPLETED';
export const FETCH_PUBLICIZE_CONNECTIONS = 'FETCH_PUBLICIZE_CONNECTIONS';
export const NEW_NOTICE = 'NEW_NOTICE';
export const POSTS_RECEIVE = 'POSTS_RECEIVE';
export const POSTS_REQUEST = 'POSTS_REQUEST';
export const POSTS_REQUEST_FAILURE = 'POSTS_REQUEST_FAILURE';
export const POSTS_REQUEST_SUCCESS = 'POSTS_REQUEST_SUCCESS';
export const RECEIVE_PUBLICIZE_CONNECTIONS = 'RECEIVE_PUBLICIZE_CONNECTIONS';
export const RECEIVE_SITE = 'RECEIVE_SITE';
export const REMOVE_NOTICE = 'REMOVE_NOTICE';
Expand Down
2 changes: 2 additions & 0 deletions client/state/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { createStore, applyMiddleware, combineReducers, compose } from 'redux';
*/
import { analyticsMiddleware } from 'lib/themes/middlewares.js';
import notices from './notices/reducer';
import posts from './posts/reducer';
import sharing from './sharing/reducer';
import sites from './sites/reducer';
import siteSettings from './site-settings/reducer'
Expand All @@ -21,6 +22,7 @@ import ui from './ui/reducer';
*/
const reducer = combineReducers( {
notices,
posts,
sharing,
sites,
siteSettings,
Expand Down
10 changes: 10 additions & 0 deletions client/state/posts/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
REPORTER ?= spec
NODE_BIN := $(shell npm bin)
MOCHA ?= $(NODE_BIN)/mocha
BASE_DIR := $(NODE_BIN)/../..
NODE_PATH := $(BASE_DIR)/client:$(BASE_DIR)/shared

test:
@NODE_ENV=test NODE_PATH=$(NODE_PATH) $(MOCHA) --compilers jsx:babel/register,js:babel/register --reporter $(REPORTER)

.PHONY: test
69 changes: 69 additions & 0 deletions client/state/posts/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* Internal dependencies
*/
import wpcom from 'lib/wp';
import {
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
POSTS_REQUEST_FAILURE
} from 'state/action-types';

/**
* Returns an action object to be used in signalling that a post object has
* been received.
*
* @param {Object} post Post received
* @return {Object} Action object
*/
export function receivePost( post ) {
return receivePosts( [ post ] );
}

/**
* Returns an action object to be used in signalling that post objects have
* been received.
*
* @param {Array} posts Posts received
* @return {Object} Action object
*/
export function receivePosts( posts ) {
return {
type: POSTS_RECEIVE,
posts
};
}

/**
* Triggers a network request to fetch posts for the specified site and query.
*
* @param {Number} siteId Site ID
* @param {String} query Post query
* @return {Function} Action thunk
*/
export function requestSitePosts( siteId, query = {} ) {
return ( dispatch ) => {
dispatch( {
type: POSTS_REQUEST,
siteId,
query
} );

return wpcom.site( siteId ).postsList( { query } ).then( ( { posts } ) => {
dispatch( receivePosts( posts ) );
dispatch( {
type: POSTS_REQUEST_SUCCESS,
siteId,
query,
posts
} );
} ).catch( ( error ) => {
dispatch( {
type: POSTS_REQUEST_FAILURE,
siteId,
query,
error
} );
} );
};
}
101 changes: 101 additions & 0 deletions client/state/posts/reducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/**
* External dependencies
*/
import { combineReducers } from 'redux';
import indexBy from 'lodash/collection/indexBy';

/**
* Internal dependencies
*/
import {
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
POSTS_REQUEST_FAILURE
} from 'state/action-types';

/**
* Tracks all known post objects, indexed by post global ID.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function items( state = {}, action ) {
switch ( action.type ) {
case POSTS_RECEIVE:
state = Object.assign( {}, state, indexBy( action.posts, 'global_ID' ) );
break;
Copy link
Member

Choose a reason for hiding this comment

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

As in my comment above, we have two code paths here for essentially the same action. Could we not simply always use the list case, even guarding it with a simple concat if we were worried about getting a single post instead of a list of posts?

Then I think it would be clear exactly what the intended action is and also it would be clear that the result of getting one post is really no different than that of when getting ten posts or one hundred posts etc…

}

return state;
}

/**
* Returns the updated site posts state after an action has been dispatched.
* The state reflects a mapping of site ID, post ID pairing to global post ID.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function sitePosts( state = {}, action ) {
switch ( action.type ) {
case POSTS_RECEIVE:
state = Object.assign( {}, state );
action.posts.forEach( ( post ) => {
if ( ! state[ post.site_ID ] ) {
state[ post.site_ID ] = {};
}

state[ post.site_ID ][ post.ID ] = post.global_ID;
} );
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we'll end up making lots of clones here and I wonder if you didn't intend on cloning state outside of the forEach loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if you didn't intend on cloning state outside of the forEach loop.

I think I did, or at least I should have. Will look to change. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

State cloned only once in a71c6f5.

break;
}

return state;
}

/**
* Returns the updated post query state after an action has been dispatched.
* The state reflects a mapping of site ID to active queries.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function siteQueries( state = {}, action ) {
switch ( action.type ) {
case POSTS_REQUEST:
case POSTS_REQUEST_SUCCESS:
case POSTS_REQUEST_FAILURE:
const { type, siteId, posts } = action;
const query = JSON.stringify( action.query );

// Clone state and ensure that site is tracked
state = Object.assign( {}, state );
if ( ! state[ siteId ] ) {
state[ siteId ] = {};
}

// Only the initial request should be tracked as fetching. Success
// or failure types imply that fetching has completed.
state[ siteId ][ query ] = {
fetching: POSTS_REQUEST === type
};

// When a request succeeds, map the received posts to state.
if ( POSTS_REQUEST_SUCCESS === type ) {
state[ siteId ][ query ].posts = posts.map( ( post ) => post.global_ID );
}
break;
}

return state;
}

export default combineReducers( {
items,
sitePosts,
siteQueries
} );
27 changes: 27 additions & 0 deletions client/state/posts/selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Returns a post object by its global ID.
*
* @param {Object} state Global state tree
* @param {String} globalId Post global ID
* @return {Object} Post object
*/
export function getPost( state, globalId ) {
return state.posts.items[ globalId ];
}

/**
* Returns a post object by site ID, post ID pair.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {String} postId Post ID
* @return {?Object} Post object
*/
export function getSitePost( state, siteId, postId ) {
const { sitePosts } = state.posts;
if ( ! sitePosts[ siteId ] || ! sitePosts[ siteId ][ postId ] ) {
return null;
}

return getPost( state, sitePosts[ siteId ][ postId ] );
}
137 changes: 137 additions & 0 deletions client/state/posts/test/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/**
* External dependencies
*/
import nock from 'nock';
import sinon from 'sinon';
import sinonChai from 'sinon-chai';
import Chai, { expect } from 'chai';

/**
* Internal dependencies
*/
import {
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
POSTS_REQUEST_FAILURE
} from 'state/action-types';
import {
receivePost,
receivePosts,
requestSitePosts
} from '../actions';

describe( 'actions', () => {
describe( '#receivePost()', () => {
it( 'should return an action object', () => {
const post = { ID: 841, title: 'Hello World' };
const action = receivePost( post );

expect( action ).to.eql( {
type: POSTS_RECEIVE,
posts: [ post ]
} );
} );
} );

describe( '#receivePosts()', () => {
it( 'should return an action object', () => {
const posts = [ { ID: 841, title: 'Hello World' } ];
const action = receivePosts( posts );

expect( action ).to.eql( {
type: POSTS_RECEIVE,
posts
} );
} );
} );

describe( '#requestSitePosts()', () => {
const spy = sinon.spy();

before( () => {
Chai.use( sinonChai );

nock( 'https://public-api.wordpress.com:443' )
.persist()
.get( '/rest/v1.1/sites/2916284/posts' )
.reply( 200, {
posts: [
{ ID: 841, title: 'Hello World' },
{ ID: 413, title: 'Ribs & Chicken' }
]
} )
.get( '/rest/v1.1/sites/2916284/posts' )
.query( { search: 'Hello' } )
.reply( 200, {
posts: [ { ID: 841, title: 'Hello World' } ]
} )
.get( '/rest/v1.1/sites/77203074/posts' )
.reply( 403, {
error: 'authorization_required',
message: 'User cannot access this private blog.'
} );
} );

beforeEach( () => {
spy.reset();
} );

after( () => {
nock.restore();
} );

it( 'should dispatch fetch action when thunk triggered', () => {
requestSitePosts( 2916284 )( spy );

expect( spy ).to.have.been.calledWith( {
type: POSTS_REQUEST,
siteId: 2916284,
query: {}
} );
} );

it( 'should dispatch posts receive action when request completes', ( done ) => {
requestSitePosts( 2916284 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POSTS_RECEIVE,
posts: [
{ ID: 841, title: 'Hello World' },
{ ID: 413, title: 'Ribs & Chicken' }
]
} );

done();
} ).catch( done );
} );

it( 'should dispatch posts posts request success action when request completes', ( done ) => {
requestSitePosts( 2916284 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POSTS_REQUEST_SUCCESS,
siteId: 2916284,
query: {},
posts: [
{ ID: 841, title: 'Hello World' },
{ ID: 413, title: 'Ribs & Chicken' }
]
} );

done();
} ).catch( done );
} );

it( 'should dispatch fail action when request fails', ( done ) => {
requestSitePosts( 77203074 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POSTS_REQUEST_FAILURE,
siteId: 77203074,
query: {},
error: sinon.match( { message: 'User cannot access this private blog.' } )
} );

done();
} ).catch( done );
} );
} );
} );
Copy link
Member

Choose a reason for hiding this comment

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

this is a great example test file and is worth publishing in a weekly digest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a great example test file and is worth publishing in a weekly digest

Thanks @dmsnell !

Related, I had struggled for a while to find a way to test the dispatch callback without relying on the return value being a promise. I think this is reflected by the requested Sinon feature at sinonjs/sinon#118, as it could allow me to write something like:

// Before:

requestSitePosts( 77203074 )( spy ).then( () => {
    expect( spy ).to.have.been.calledWith( {
        type: POSTS_REQUEST_FAILURE,
        siteId: 77203074,
        query: {},
        error: sinon.match( { message: 'User cannot access this private blog.' } )
    } );
    done();
} ).catch( done );

// After:

const stub = sinon.stub().calledWith( {
    type: POSTS_REQUEST_FAILURE,
    siteId: 77203074,
    query: {},
    error: sinon.match( { message: 'User cannot access this private blog.' } )
} ).calls( done );

requestSitePosts( 77203074 )( stub );

Curious if anyone has encountered a similar need.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just getting started in my journey to learn what sinon offers, but I'm really impressed with it. We could probably all benefit by spending some focussed time reading through their (pretty incredible) documentation.

as far as your response goes - I haven't seen many people besides you actually stubbing up calls like these, so I'm not familiar with other attempts at it.

Loading