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

State: Update counts state in response to post status changes #6040

Merged
merged 2 commits into from
Jun 20, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jun 14, 2016

Related: #5613

This pull request seeks to enhance the state.posts.counts state to account for changes in post status, transitioning counts from its previous status to the new status to reflect the change.

Implementation notes:

This is a difficult challenge to solve, since post counts are received and tracked in state independently of the posts themselves. This is desirable since when navigating to a post listing screen, the filter tabs should display the counts of each status, even before all posts of that status are requested. This poses a problem when posts change over time, since there's effectively a dependency between the two states. When deleting a post or receiving a new post, the previous implementation had no way of determining what the post's previous status was, since that was tracked elsewhere in state.

To address this, I've experimented with the idea of having the reducer track its own local state, monitoring actions that it considers relevant to count changes. Here, this includes trashing posts (POST_DELETE action) and changes to posts over time (POSTS_RECEIVE). Further, since counts are distinguished by "mine" and "all", the reducer must also track the current user ID as it changes.

While the implementation is quite different, this was very loosely inspired by an undoable higher-order reducer demo'd during this year's React Europe conference (video).

Testing instructions:

Ensure that Mocha tests pass:

npm run test-client client/state/posts/counts/test/reducer.js

Observe that when trashing a post from the CPT listing screen, the post counts in both the filter bar and sidebar draft button update accordingly. Specifically, note that posts and pages support "trash", while other post types do not, so "trash" count is only incremented for these types. Additionally, when trashing an already-trashed post, the post count for trashed should decrement.

You can also verify that publishing a draft from the editor updates these counts.

Caveats:

  • Not all posts are tracked in Redux state. Notably, the (non-CPT) post listing screen still uses legacy Flux stores, so the draft count in the sidebar will not update when trashing a post from this screen

/cc @mtias

Test live: https://calypso.live/?branch=update/cpt-increment-post-counts

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Custom Post Types (CPT) State labels Jun 14, 2016
@mtias
Copy link
Member

mtias commented Jun 15, 2016

This seems like a much more scalable approach — thanks for exploring it. It reduces the problem to 1) the full count that the server provided as the basis 2) any local changes to posts modify the totals.

* @return {String} Serialized key
*/
function getPostStatusKey( siteId, postId ) {
return [ siteId, postId ].join();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to join with a : or - here to prevent a possible edge case ( very very very unlikely ) where a duplicate key could be created ( i.e. siteId of 11 with a postId 1 conflicting with siteId 1 and postId 11 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to prevent a possible edge case ( very very very unlikely ) where a duplicate key could be created

Granted it's very unobvious, but the default separator for join is a comma, so there shouldn't be a worry of conflicts:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/join

@timmyc
Copy link
Contributor

timmyc commented Jun 18, 2016

One minor comment above, but otherwise looks great and tests out 👍 as well

@timmyc timmyc 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 Jun 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants