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

Posts: Add posts Redux state handlers #2248

merged 6 commits into from
Jan 12, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jan 8, 2016

Related: #708, #2167
Prerequisite of: #303

This pull request seeks to add new basic posts querying handlers to the global Redux state. This will be used in the post editor in enabling a user to search for existing content to link.

Implementation notes:

A few choices worth scrutinized review:

  • I chose to index posts by global_ID, so that the items map could be a simple flattened object. Since post IDs are unique to a specific site, global_ID enables us to keep all known posts more readily accessible.
  • "Request" as a chosen verb instead of "Fetch". There was some previous discussion in which a concern was raised over using fetch in action names, as network requests aren't always fetch-related.
  • Multiple queries can be tracked simultaneously, and are referenced by its JSON serialized query (where a query is the object passed to wpcom.js postsList method). Aside from allowing multiple post queries to be performed, this also prevents unnecessary re-requests when switching back to a previously known query result.

Testing instructions:

Confirm that Mocha tests pass by running make run in the project root directory, or directly from client/state/posts.

The included actions are not currently used anywhere in the application, though you should confirm that the inclusion of the posts reducer does not cause any errors to occur when loading any Calypso page.

@aduth aduth added Posts [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 8, 2016
@aduth aduth self-assigned this Jan 8, 2016
} ).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.

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 12, 2016
aduth added a commit that referenced this pull request Jan 12, 2016
Posts: Add posts Redux state handlers
@aduth aduth merged commit 0c62a64 into master Jan 12, 2016
@aduth aduth deleted the add/posts-redux branch January 12, 2016 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. Posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants