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

Posts: Add single site post requesting behavior to Redux state #3108

Merged
merged 3 commits into from
Feb 5, 2016
Merged

Posts: Add single site post requesting behavior to Redux state #3108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
615773a
Posts: Add single site requesting behavior to Redux state
aduth Feb 5, 2016
fbb511f
Posts: Return promise object in Mocha tests
aduth Feb 5, 2016
52ad4f8
Posts: Use deepFreeze in tests, fixing broken tests
aduth Feb 5, 2016
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
3 changes: 3 additions & 0 deletions client/state/action-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ export const FETCH_SITE_PLANS = 'FETCH_SITE_PLANS';
export const FETCH_SITE_PLANS_COMPLETED = 'FETCH_SITE_PLANS_COMPLETED';
export const FETCH_WPORG_PLUGIN_DATA = 'FETCH_WPORG_PLUGIN_DATA';
export const NEW_NOTICE = 'NEW_NOTICE';
export const POST_REQUEST = 'POST_REQUEST';
export const POST_REQUEST_SUCCESS = 'POST_REQUEST_SUCCESS';
export const POST_REQUEST_FAILURE = 'POST_REQUEST_FAILURE';
Copy link
Contributor

Choose a reason for hiding this comment

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

(note: not really for this PR)

I wonder if we should flip all of these to keymirror... It turns out the const doesn't transfer through the export, so an importer can still reassign the imported variable. I feel like the const is giving a false sense of security.

But maybe it helps with code completion in some editors?

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 we should flip all of these to keymirror

What advantage would there be in using keyMirror?

For me at least, this is less about feeling of security with const and rather in opting to use simple language constructs than an external library that, in my opinion, doesn't provide much value.

There's some discussion in the keyMirror repository about minifiers crushing keys, though I don't think it affects our situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

just less duplication. it's really easy to fat finger one side of the const construction and not notice.

MY_REALLY_LONG_KEY = "MY_REALY_LONG_KEY"

vs

keyMirror( { MY_REALLY_LONG_KEY: t } )

I'm not worried about the minifier advantages really. Though that could speed up string compares I guess. Likely meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's really easy to fat finger one side of the const construction and not notice

Maybe some custom lint rules could help here?

Related: #3015, https://github.com/Automattic/eslint-plugin-wpcalypso

export const POSTS_RECEIVE = 'POSTS_RECEIVE';
export const POSTS_REQUEST = 'POSTS_REQUEST';
export const POSTS_REQUEST_FAILURE = 'POSTS_REQUEST_FAILURE';
Expand Down
36 changes: 36 additions & 0 deletions client/state/posts/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
*/
import wpcom from 'lib/wp';
import {
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
Expand Down Expand Up @@ -68,3 +71,36 @@ export function requestSitePosts( siteId, query = {} ) {
} );
};
}

/**
* Triggers a network request to fetch a specific post from a site.
*
* @param {Number} siteId Site ID
* @param {Number} postId Post ID
* @return {Function} Action thunk
*/
export function requestSitePost( siteId, postId ) {
return ( dispatch ) => {
dispatch( {
type: POST_REQUEST,
siteId,
postId
} );

return wpcom.site( siteId ).post( postId ).get().then( ( post ) => {
dispatch( receivePost( post ) );
dispatch( {
type: POST_REQUEST_SUCCESS,
siteId,
postId
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably return the post in case someone consuming this wants to chain onto the Promise and use it. See http://jsbin.com/jufayefego/edit?js,console

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 should probably return the post in case someone consuming this wants to chain onto the Promise and use it. See http://jsbin.com/jufayefego/edit?js,console

Arguably, we shouldn't want to allow this, as the only consumption of data should be directly from the Redux state tree, not from an action thunk resolution. The only reason I opted to return the Promise in the first place is in facilitating test's awareness of the requests completing (as nock mocks aren't as immediate as you might expect). It may also come in handy for server-side rendering in knowing whether the response is ready to serve (based on the resolution of all initial actions), but again, not in specifically consuming any data result.

Copy link
Contributor

Choose a reason for hiding this comment

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

as nock mocks aren't as immediate as you might expect

Promises always fulfill or reject on the next (well, a future) turn of the event loop, unless you mean something else? See http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony for most of the reasoning. Zalgo is bad.

Arguably, we shouldn't want to allow this, as the only consumption of data should be directly from the Redux state tree, not from an action thunk resolution.

I can get behind that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Promises always fulfill or reject on the next (well, a future) turn of the event loop, unless you mean something else?

Yeah, I mean something else. Naive attempts by me in testing nock would look more like:

it( 'should finish request on next tick', ( done ) => {
    someRequestMockedByNock();

    setTimeout( () => {
        expect( myAsyncSpy ).to.have.been.calledOnce;
        done();
    }, 0 );
} );

But for whatever reason, the nock mocks aren't resolved in any predictable next tick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting! Good to know.

} ).catch( ( error ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

(this is curiosity talking, not a criticism) Why use catch instead of passing the error handler as the second arg to then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why use catch instead of passing the error handler as the second arg to then?

Don't feel strongly either way, but I suppose an argument could be made that it lends more clarity to what the code is effectively accomplishing, as catch is more meaningful than happening to be passed as the second argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a real subtle diff between the two cases. By using catch after the then, you're also catching any errors that the first then handler throws. If you use the then( yay, boo ) construct, boo will never see errors that yay throws. So it really depends on what you're trying to capture and who's going to consume the Promise.

see http://www.html5rocks.com/en/tutorials/es6/promises/#toc-error-handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a real subtle diff between the two cases.

Subtle indeed, though still unsure which makes the most sense. Would we want to allow the yay case to throw uncaptured, or is it preferable to have the catch block capture errors (dispatching failure action) in both the request and yay handler? Obviously we don't want errors in our handlers in any situation, so perhaps not important to dwell too much over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, don't dwell. Usage here is fine. If you want to flip to returning the promise from the test, just drop the catch.

dispatch( {
type: POST_REQUEST_FAILURE,
siteId,
postId,
error
} );
} );
};
}
62 changes: 46 additions & 16 deletions client/state/posts/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import indexBy from 'lodash/collection/indexBy';
* Internal dependencies
*/
import {
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
Expand Down Expand Up @@ -67,6 +70,34 @@ export function sitePosts( state = {}, action ) {
return state;
}

/**
* Returns the updated site post requests state after an action has been
* dispatched. The state reflects a mapping of site ID, post ID pairing to a
* boolean reflecting whether a request for the post is in progress.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function siteRequests( state = {}, action ) {
switch ( action.type ) {
case POST_REQUEST:
case POST_REQUEST_SUCCESS:
case POST_REQUEST_FAILURE:
return Object.assign( {}, state, {
[ action.siteId ]: Object.assign( {}, state[ action.siteId ], {
[ action.postId ]: POST_REQUEST === action.type
} )
} );

case SERIALIZE:
case DESERIALIZE:
return {};
}

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.
Expand All @@ -83,25 +114,23 @@ export function siteQueries( state = {}, action ) {
const { type, siteId, posts } = action;
const query = getSerializedPostsQuery( action.query );

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

if ( ! state[ siteId ][ query ] ) {
state[ siteId ][ query ] = {};
}
state = Object.assign( {}, state, {
[ siteId ]: Object.assign( {}, 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 );
state[ siteId ][ query ] = Object.assign( {}, state[ siteId ][ query ], {
// Only the initial request should be tracked as fetching.
// Success or failure types imply that fetching has completed.
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 );
}

return state;

case SERIALIZE:
case DESERIALIZE:
return {};
Expand All @@ -123,15 +152,15 @@ export function siteQueriesLastPage( state = {}, action ) {
case POSTS_REQUEST_SUCCESS:
const { siteId, found } = action;

state = Object.assign( {}, state );
if ( ! state[ siteId ] ) {
state[ siteId ] = {};
}
state = Object.assign( {}, state, {
[ siteId ]: Object.assign( {}, state[ siteId ] )
} );

const serializedQuery = getSerializedPostsQueryWithoutPage( action.query );
const lastPage = Math.ceil( found / ( action.query.number || DEFAULT_POST_QUERY.number ) );
state[ siteId ][ serializedQuery ] = Math.max( lastPage, 1 );
return state;

case SERIALIZE:
case DESERIALIZE:
return {};
Expand All @@ -142,6 +171,7 @@ export function siteQueriesLastPage( state = {}, action ) {
export default combineReducers( {
items,
sitePosts,
siteRequests,
siteQueries,
siteQueriesLastPage
} );
17 changes: 17 additions & 0 deletions client/state/posts/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,20 @@ export function getSitePostsForQueryIgnoringPage( state, siteId, query ) {
return memo.concat( getSitePostsForQuery( state, siteId, pageQuery ) || [] );
}, [] );
}

/**
* Returns true if a request is in progress for the specified site post, or
* false otherwise.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {Number} postId Post ID
* @return {Boolean} Whether request is in progress
*/
export function isRequestingSitePost( state, siteId, postId ) {
if ( ! state.posts.siteRequests[ siteId ] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are you guaranteed that state.posts.siteRequests exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you guaranteed that state.posts.siteRequests exists?

Yep, because the Redux store will be initialized with the default return value of the reducer. Under the hood, the initial state is constructed during the Redux internal @@INIT action type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, because the Redux store will be initialized with the default return value of the reducer.

Are you guaranteed a caller won't do run isRequestingSitePost before redux initializes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you guaranteed a caller won't do run isRequestingSitePost before redux initializes?

Yes, or at least, if there are callers, that's a larger issue in itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good good

return false;
}

return !! state.posts.siteRequests[ siteId ][ postId ];
}
120 changes: 91 additions & 29 deletions client/state/posts/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import Chai, { expect } from 'chai';
* Internal dependencies
*/
import {
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
Expand All @@ -18,10 +21,25 @@ import {
import {
receivePost,
receivePosts,
requestSitePosts
requestSitePosts,
requestSitePost
} from '../actions';

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

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

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

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

describe( '#receivePost()', () => {
it( 'should return an action object', () => {
const post = { ID: 841, title: 'Hello World' };
Expand All @@ -47,11 +65,7 @@ describe( 'actions', () => {
} );

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' )
Expand All @@ -75,12 +89,8 @@ describe( 'actions', () => {
} );
} );

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

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

it( 'should dispatch fetch action when thunk triggered', () => {
Expand All @@ -93,22 +103,20 @@ describe( 'actions', () => {
} );
} );

it( 'should dispatch posts receive action when request completes', ( done ) => {
requestSitePosts( 2916284 )( spy ).then( () => {
it( 'should dispatch posts receive action when request completes', () => {
return 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( () => {
it( 'should dispatch posts posts request success action when request completes', () => {
return requestSitePosts( 2916284 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POSTS_REQUEST_SUCCESS,
siteId: 2916284,
Expand All @@ -119,13 +127,11 @@ describe( 'actions', () => {
{ ID: 413, title: 'Ribs & Chicken' }
]
} );

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

it( 'should dispatch posts request success action with query results', ( done ) => {
requestSitePosts( 2916284, { search: 'Hello' } )( spy ).then( () => {
it( 'should dispatch posts request success action with query results', () => {
return requestSitePosts( 2916284, { search: 'Hello' } )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POSTS_REQUEST_SUCCESS,
siteId: 2916284,
Expand All @@ -135,22 +141,78 @@ describe( 'actions', () => {
{ ID: 841, title: 'Hello World' }
]
} );

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

it( 'should dispatch fail action when request fails', ( done ) => {
requestSitePosts( 77203074 )( spy ).then( () => {
it( 'should dispatch fail action when request fails', () => {
return 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 );
describe( '#requestSitePost()', () => {
before( () => {
nock( 'https://public-api.wordpress.com:443' )
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, but if it's too painful to mock wpcom.js, maybe we should look at fixing wpcom.js to make it easier to test through. Having to bake all this info (host / port / protocol / path) into tests feels fragile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should look at fixing wpcom.js to make it easier to test through.

Yeah, I've used and seen a few different approaches to mocking WPCOM.js requests, though I'm mostly content with this one (see also: mocking WPCOM.js methods gets hairy too).

Maybe some abstractions or constants to reduce the chance of changes breaking tests? I dunno, seems a bit premature, and this doesn't feel particularly troubling to me.

.persist()
.get( '/rest/v1.1/sites/2916284/posts/413' )
.reply( 200, { ID: 413, title: 'Ribs & Chicken' } )
.get( '/rest/v1.1/sites/2916284/posts/420' )
.reply( 404, {
error: 'unknown_post',
message: 'Unknown post'
} );
} );

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

it( 'should dispatch request action when thunk triggered', () => {
requestSitePost( 2916284, 413 )( spy );

expect( spy ).to.have.been.calledWith( {
type: POST_REQUEST,
siteId: 2916284,
postId: 413
} );
} );

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

it( 'should dispatch posts posts request success action when request completes', () => {
return requestSitePost( 2916284, 413 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
type: POST_REQUEST_SUCCESS,
siteId: 2916284,
postId: 413
} );
} );
} );

it( 'should dispatch fail action when request fails', () => {
return requestSitePost( 2916284, 420 )( spy ).then( () => {
expect( spy ).to.have.been.calledWith( {
Copy link
Contributor

Choose a reason for hiding this comment

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

i wouldn't expect the success callback to be called at all?

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 wouldn't expect the success callback to be called at all?

Similar points as above in #3108 (comment) and #3108 (comment). The promise really only exists to mark progress of the request, but is not intended to be used in any effective way (either through consumption of the resolved object, or in throwing errors that occurred during the request). These are better handled by the Redux action handlers.

type: POST_REQUEST_FAILURE,
siteId: 2916284,
postId: 420,
error: sinon.match( { message: 'Unknown post' } )
} );
} );
} );
} );
} );
Loading