Editor: Add PostListStoreFactory #708

timmyc · 2015-11-25T00:51:12Z

Part of #303 - In order to have multiple components in a given view that need to interact with distinct query options of a post-list-store - the current post-list-store needs to be abstracted into a factory akin to the category-store.

Background
While building the "Parent Page Selector" in the editor - both the parent page selector and the post scheduler were modifying the post-list-store query options, and as such the query results. This lead to some odd behavior between the two, and ultimately was fixed by disabling the post scheduler when editing pages.

By moving to a factory setup, components like the post-list-fetcher can create their own instance of a post-list-store to ensure query options and results are not polluted by other components on the page.

This branch indeed got a bit larger then I typically like, but a large amount of the new lines are in the form of tests. Additionally, reading the diff of post-list-store might be a bit difficult and is probably best read outside of the diff viewer. I did not change any logic within the post-list-store, just restructured it to flow with the factory setup, and ES6-ified it.

To Test
Test coverage for the post-list-store has been added in this branch. The tests can be executed by performing cd client/lib/posts && make.

Additionally, the post-list-store acts as a data provider for the following components. All should be interacted with during testing to ensure no odd behavior is encountered

my-sites/drafts/draft-list.jsx Can be tested by using the "Drafts Drawer" in the editor
my-sites/menus/item-options/post-list.jsx Test via the menu builder by adding menu options for both a page and a post. Interact with the search box here as well and validate the search filter still operates as expected
my-sites/pages/page-list.jsx View a page list for a site. Interact with the filter bar by toggling between Drafts and Trash. Filter pages using search
my-sites/post-selector/index.jsx Open a page in the editor, interact with the parent page selector in the sidebar.
my-sites/posts/post-list.jsx Open a sites posts list. Interact with the filterbar and search.
post-editor/editor-ground-control/index.jsx Inside the post editor, expand the post scheduler. Ensure calendar items are circled as expected for dates where posts have been published.
Stats Insights Page - The "Latest Post Summary" on the insights tab of the stats page also uses the post-list-store. Validate that data shows up as expected inside this module

aduth · 2015-11-25T19:23:11Z

client/components/post-list-fetcher/index.jsx

@@ -14,11 +14,12 @@ var PostListStore = require( 'lib/posts/post-list-store' ),

 var PostListFetcher;

-function dispatchQueryActions( query ) {
-	actions.queryPosts( query );
+function dispatchQueryActions( postListStoreId, query ) {


Was it a conscious decision to not have postListStoreId be an optional second parameter, falling back to a default store ID as we did with the term actions?

Nevermind, I didn't see that this was the fetcher component and that you're specifying a default prop below.

timmyc · 2015-11-25T21:48:17Z

@aduth thanks for the review. In de35df8 I made changes per your feedback - specifically moved _nextId to a module level variable so it is incremented across all post-list-stores and added a test case around that.

aduth · 2015-12-07T14:06:54Z

client/lib/posts/post-list-store.js

-	last = require( 'lodash/array/last' ),
-	max = require( 'lodash/collection/max' );
+import debugFactory from 'debug';
+const debug = debugFactory( 'calypso:posts-list' );


Nitpicking now, but I'd still keep the dependencies blocks for imports only and move this assignment to Module variables, though I'm not sure whether there's any precedent here.

I agree. This matches precedent as well: there are 44 instances of import debug or import _debug in the code, and only 6 are immediately followed by the const debug statement.

Because I was curious:

String Count

import debugModule 25

import debugFactory 12

import _debug 4

import Debug 3

aduth · 2015-12-07T14:22:17Z

Not to throw a wrench in everything, and I think its implications require far more work than what we're working towards here, but we should start to think about how problems like these can be addressed and/or better solved in the global Redux state tree that we're moving towards. Posts will probably end up being one of our most complex state subtrees, as it involves querying, pagination, and hierarchy.

Some very rough and naive ideation of how this tree could look:

{
    posts: {
        byId: {
            100: {
                title: 'Hello World'
            }
        },
        queries: {
            'status=publish': {
                posts: [ 100 ],
                fetching: false,
                page: 1
            },
            'status=publish&search=mypost': {
                posts: null,
                fetching: true
            }
        }
    }
}

With selectors to pull out posts:

function getPosts( state, query ) {
    return state.posts.queries[ query ].posts.map( ( postId ) => {
        return state.posts.byId[ postId ];
    } );
}

We can certainly move forward with the approach in this pull request, but it behooved me to at the very least mention Redux in the context of this conversation.

nylen · 2015-12-07T18:25:07Z

client/lib/posts/post-list-store.js

+module.exports = function( id ) {
+	if ( ! id ) {
+		throw new Error( 'must supply a post-list-store id' );
+	}


Why not es6ify this as export default function( id )?

nylen · 2015-12-07T19:15:34Z

This branch indeed got a bit larger then I typically like, but a large amount of the new lines are in the form of tests.

At first I was like

😩

but then I was like

😎

This works well in my testing, and appears to me to be the shortest path between where we are and getting link search working, which is the # 1 user request for the editor (that we're tracking, at least): pKdGS-XV-p2

Left a few inline notes, which shouldn't block this from being merged. The added tests are nice and give us more confidence both now and in the future when we move this logic to Redux.

timmyc · 2015-12-07T21:26:38Z

Okay rebased the branch to pull in @gwwar's recent addition to the post-list-store of the hasRecentError() method. Added tests for that new method in 054f011, and took care of @nylen's feedback in 5cf488f

timmyc · 2015-12-09T20:49:02Z

ping @mtias and @aduth are you both okay with this moving ahead as a temporary solution until a redux approach can be adopted?

aduth · 2015-12-09T21:01:36Z

client/lib/posts/actions.js


 		if ( siteID ) {
 			debug( 'Fetching posts that have been updated for %s since %s %o', siteID, params.modified_after, params );
 			wpcom
 				.site( siteID )
-				.postsList( params, PostActions.receiveUpdated.bind( null, id ) );
+				.postsList( params, PostActions.receiveUpdated.bind( null, id, postListStoreId ) );


Does this need to pass postListStoreId? I'd think any instance of the PostListStore would be interested in receiving updated posts.

At first glance I think I would agree with you, but if I'm following the logic correctly, in the store if we allow all posts to be passed down regardless of postListStoreId, we would end up with post.IDs that could possibly not map to the store's active query #

if I'm following the logic correctly, in the store if we allow all posts to be passed down regardless of postListStoreId, we would end up with post.IDs that could possibly not map to the store's active query

Yeah, appears so, which is unfortunate, since receiving updated posts should be agnostic to the currently active list. It's not a big concern and more of an ideal, perhaps something we can aim to improve in future iterations.

aduth · 2015-12-09T21:06:32Z

Outside of my question related to whether we can allow all instances of post stores to receive updated posts, I have no objections with merging this. Tests well for me. 👍

timmyc · 2015-12-11T16:57:26Z

After discussing this in our team hangout again yesterday I'm going to give this a merge.

Editor: Add PostListStoreFactory

timmyc added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress labels Nov 25, 2015

timmyc self-assigned this Nov 25, 2015

timmyc added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 25, 2015

aduth reviewed Nov 25, 2015
View reviewed changes

aduth reviewed Dec 7, 2015
View reviewed changes

nylen reviewed Dec 7, 2015
View reviewed changes

timmyc added 4 commits December 7, 2015 13:06

PostListStore: Add factory for multiple store support and basic tests.

67924f4

Lint fixes in tests.

46753d4

PostListStore: Additional test coverage for factories.

0ee0e7c

Lint fixes.

a6bcd5b

timmyc added 2 commits December 7, 2015 13:07

Updates per feedback.

33c1eb1

Fixing test after rebase.

f7cb813

timmyc force-pushed the add/post-list-factory branch from de35df8 to f7cb813 Compare December 7, 2015 21:14

timmyc added 2 commits December 7, 2015 13:22

Add tests for hasRecentError

054f011

One last es6-ification and renaming and ordering of debug module

5cf488f

timmyc mentioned this pull request Dec 9, 2015

Editor: Allow parent page to be removed. #896

Merged

aduth reviewed Dec 9, 2015
View reviewed changes

timmyc added a commit that referenced this pull request Dec 11, 2015

Merge pull request #708 from Automattic/add/post-list-factory

7116d67

Editor: Add PostListStoreFactory

timmyc merged commit 7116d67 into master Dec 11, 2015

scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 11, 2015

mtias deleted the add/post-list-factory branch December 11, 2015 17:44

aduth mentioned this pull request Jan 8, 2016

Posts: Add posts Redux state handlers #2248

Merged

snyk-bot mentioned this pull request Jan 22, 2020

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Add PostListStoreFactory #708

Editor: Add PostListStoreFactory #708

timmyc commented Nov 25, 2015

aduth Nov 25, 2015

aduth Nov 25, 2015

timmyc commented Nov 25, 2015

aduth Dec 7, 2015

nylen Dec 7, 2015

aduth commented Dec 7, 2015

nylen Dec 7, 2015

nylen commented Dec 7, 2015

timmyc commented Dec 7, 2015

timmyc commented Dec 9, 2015

aduth Dec 9, 2015

timmyc Dec 9, 2015

aduth Dec 10, 2015

aduth commented Dec 9, 2015

timmyc commented Dec 11, 2015

String	Count
`import debugModule`	25
`import debugFactory`	12
`import _debug`	4
`import Debug`	3

Editor: Add PostListStoreFactory #708

Editor: Add PostListStoreFactory #708

Conversation

timmyc commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc commented Nov 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 7, 2015

Choose a reason for hiding this comment

nylen commented Dec 7, 2015

timmyc commented Dec 7, 2015

timmyc commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Dec 9, 2015

timmyc commented Dec 11, 2015