Post revisions into redux #12733

bperson · 2017-04-03T14:43:29Z

First PR for my trial project described on the Team IO P2 blog: a UI for browsing a post's revisions.

While I continue to discuss the UI / UX for a post's history in the editor, this is a first step towards getting the revisions data for a post loaded in Redux. A placeholder page/route has been created and is available at: /post/revision/<siteId>/<postId>

QueryPostRevisions component
POST_REVISIONS_RECEIVE action and reducer handling
POST_REVISIONS_REQUEST_(SUCCESS|FAILURE) action and reducer handling
Proper state / display normalization of revisions coming from the REST 2.0 API
Tests actions / reducers
Add README.md for new components + JSDoc

Further down the line / might not be worth including in this specific PR:

Do not load all revisions for a post? (requires API update): https://core.trac.wordpress.org/ticket/40510
Support for revisions with various (siteId / postId) tuples in the same POST_REVISIONS_RECEIVE action, postponed / not needed for now

… `QueryPostRevisions`

a8ci18n · 2017-04-03T14:47:47Z

client/post-editor/post-revision.jsx

+				<QueryPostRevisions
+					siteId={ this.props.siteId }
+					postId={ this.props.postId } />
+				<h1>{ this.props.translate( 'Post History' ) }</h1>


Hi! I've found a possible matching string that has already been translated 63 times:
translate( 'History' ) ES Score: 9

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

This string will need to include the post type as a translation placeholder.

…state

…tate/posts`

…uests in Redux

…sions

nylen · 2017-04-13T21:35:44Z

client/post-editor/controller.js

+		const siteId = getSelectedSiteId( context.store.getState() );
+		const postId = getPostID( context );
+
+		renderRevision( context, siteId, postId );


Shouldn't these be revisions (current method) and renderRevisions (current line)?

it should :) That said, I removed that code (for now) to keep the PR focused on redux changes. Will keep it in mind for the next PR, thx :)

nylen · 2017-04-13T21:38:00Z

client/post-editor/index.js

@@ -14,6 +14,7 @@ module.exports = function() {
 	page( '/post', controller.pressThis, sitesController.siteSelection, sitesController.sites );
 	page( '/post/new', () => page.redirect( '/post' ) ); // redirect from beep-beep-boop
 	page( '/post/:site?/:post?', sitesController.siteSelection, sitesController.fetchJetpackSettings, controller.post );
+	page( '/post/revision/:site?/:post?', sitesController.siteSelection, sitesController.fetchJetpackSettings, controller.revision );


I'd prefer to see /post/:site/:post/revisions (note: revisions comes last, and there are no ? or optional parameters). I don't think we can ever reach this point before selecting a site and a post.

Also, it doesn't have to be done in this PR, but it's worth thinking about whether the approach we're using will handle revisions for post types other than post (page and custom post types).

/post/:site/:post/revisions

will do :)

Also, it doesn't have to be done in this PR, but it's worth thinking about whether the approach we're using will handle revisions for post types other than post (page and custom post types).

It seems for pages, there will be some changes needed when querying as the URI changes to use /pages. But - as you said - considering this one PR is already XL, I will probably tackle that in a subsequent PR.

will do :)

or maybe not ^^. It seems this breaks the expected "position" of the site ID in getSiteFragment: https://github.com/Automattic/wp-calypso/blob/0492a2f/client/lib/route/path.js#L23

So we should either stick to /post/revisions/:site/:post/ or maybe /revision/:site/:post. Last solution could be to attack the root cause and get getSiteFragment to use page.js's params first and then default to exploding the path. It seems most routes are using :site, I've found only 3 occurences of :siteId, there could be other weird usages out there so this feels like a risky move tbh.

From getSiteFragment code that you linked:

There are 2 URL positions where we should look for the site fragment: last (most sections) and second-to-last (post ID is last in editor)

Aren't we just adding a 3rd position here? So, the only changes to getSiteFragment needed to support /post/:site/:post/revisions should be as follows:

change i = 2 to i = 3

add pieces.length bounds checking, since otherwise we'll try to evaluate pieces[ -1 ]

update the comments and the tests accordingly

Last solution could be to attack the root cause and get getSiteFragment to use page.js's params first and then default to exploding the path ... there could be other weird usages out there so this feels like a risky move tbh.

yes, I also think this could easily break something, whether now or in the future.

Aren't we just adding a 3rd position here? So, the only changes to getSiteFragment needed to support /post/:site/:post/revisions should be as follows:

The more positions we accept, the harder it will be to maintain and the more chances we have of breaking something. After having read #9561, #9445 and #4614 it definitely feels like this is already quite fragile, hence why I'm kinda reluctant at touching it :)

I proposed the idea of using page.js params as that would free us from any positional guessing. The probability of having a slug-like (or number) at a position where we interpret it as a site identifier is considerably higher than naming a param :site or :siteId that wouldn't actually contain a site identifier. The (hopefully) net gain in maintainability makes the "risk" of touching it potentially worth it (but it's still a (very) risky move imho)

Things are less clear in the rest of Calypso because of (some) support for manipulating multiple sites at once, but in the post editor the URLs follow the convention of /verb/site/more-specific-things-here. Viewing revisions is a more specific action than viewing a particular post ID, so it should come after the post ID in the URL.

I think the only way adding an extra URL position in getSiteFragment would break anything is if we have a URL like /verb.with.dots/site.wordpress.com/numeric/numeric (something that would be newly and incorrectly recognized as a site ID in the length - 3 position). This seems extremely unlikely to me.

In the past when changing a function like getSiteFragment, I've successfully used a testing approach like this:

Implement both versions of the function side-by-side

Add debugging to print out return values from both versions of the code, especially if they differ

Navigate around the app, hitting as many sections as possible

Unfortunately for the issues you pointed to, this was missed in some of our longer and harder-to-reach routes (#9542 and #9550 for specific examples).

I agree that using page.js params would be a better design, but it's a much larger (and to me, much scarier) change - currently getSiteFragment is called using an actual parameterized URL rather than a page.js route, so this would require restructuring all of the code where it is used.

Finally I agree with @youknowriad's suggestion that we keep this PR to the Redux changes only. There are other things to discuss around URLs, like what the possible flows look like for getting to this feature (different post types, etc) and what that implies about the possible URL structures.

I think the only way adding an extra URL position in getSiteFragment would break anything is if we have a URL like /verb.with.dots/site.wordpress.com/numeric/numeric (something that would be newly and incorrectly recognized as a site ID in the length - 3 position). This seems extremely unlikely to me.

Yes it's probably a fair assessment of the risk if we add the third-to-last position only in the slug detection (which introduces asymmetry between the slug and numeric ID detections). Adding it to the numeric ID detection might be a bit riskier. (but looking at the current page calls I couldn't see anything ;) )

Anyway, @youknowriad is right: this isn't needed for this PR ;)

nylen · 2017-04-13T21:49:22Z

Tests actions / reducers

I agree this needs tests.

Do not load all revisions for a post? (requires API update)

It's really unfortunate that this endpoint doesn't support pagination. I wasn't aware of that, and it's something we need to get fixed in WP core soon. I would suggest building the feature without this capability yet, but leaving the door open to add it later.

Then, once we have a patch for the API endpoint, we can run it on WP.com for a while to make sure it works well, and get it committed to WP core and released as part of the 4.7.x series.

One thing you could do in the meantime is create a ticket at https://core.trac.wordpress.org/ and provide some details, including links to the current/previous issues with running out of memory when displaying revisions.

youknowriad

Great job here. I left some comments. I think the placeholder route is not necessary here, we could keep the PR only for adding redux stuff (minor).

Also, testing the selectors, reducers and action creators would be awesome.

youknowriad · 2017-04-17T09:03:37Z

client/components/data/query-post-revisions/index.jsx

+		this.request( this.props );
+	}
+
+	componentWillReceiveProps( nextProps ) {


Should we use componentDidUpdate instead to trigger side effects? Here's a gist on what should we do where in React Lifecycle methods https://gist.github.com/bvaughn/923dffb2cd9504ee440791fade8db5f9

TIL, great gist! PR updated :)

youknowriad · 2017-04-17T09:08:59Z

client/state/posts/revisions/selectors.js

+	( state, siteId, postId ) => {
+		return map(
+			get( state.posts.revisions.revisions, [ siteId, postId ], [] ),
+			normalizePostForDisplay );


Minor: I guess it's personal preference, but I like to keep closing/opening parens on their own line
when we have long params like here.

return map( get( state.posts.revisions.revisions, [ siteId, postId ], [] ), normalizePostForDisplay );

Same above with the if

if ( this.props.siteId === nextProps.siteId && this.props.postId === nextProps.postId ) {

np, I wonder if it's worth maybe updating the if example in the Javascript Guidelines?

Currently it's showing:

if ( firstCondition() && secondCondition() && thirdCondition() ) { doStuff(); }

…12733

…illReceiveProps` to dispatch the data request

bperson · 2017-04-21T08:23:24Z

Fixed comments + added tests for the reducers / selectors. I didn't see much value in creating tests for the action creators as they are currently "dumb" arrow functions with no logic in them.

As suggested by @nylen I also just created a ticket on trac to track the REST endpoint pagination enhancement request in WP Core: https://core.trac.wordpress.org/ticket/40510 . That said, while testing more edge cases / wordpress setups for revisions, I found out that Wordpress.com sites have a default limit of 25 revisions which means pagination wouldn't bring lots of value, but it could be useful for Jetpack sites?

youknowriad · 2017-04-21T08:40:50Z

client/state/posts/revisions/reducer.js

+					...state[ action.siteId ],
+					[ action.postId ]: action.type === POST_REVISIONS_REQUEST,
+				},
+			};


we can probably use lodash merge here to avoid the multiple ...

youknowriad · 2017-04-21T08:43:50Z

client/state/posts/revisions/reducer.js

+	revisions,
+} );
+
+function normalizeRevisionForState( revision ) {


I wonder if we should normalize the objects in the state or in the data middleware layer. I probably have a preference over the data layer (because we could imagine using different APIs with different shapes and reuse the same reducer)

Makes sense, a couple of other places in the data-layer are already normalizing stuff "fromAPI": https://github.com/Automattic/wp-calypso/blob/master/client/state/data-layer/wpcom/read/tags/utils.js

Updated in the latest version + a couple of "dumb" tests of the revisions data-layer.

youknowriad · 2017-04-21T13:59:49Z

client/state/posts/revisions/reducer.js

@@ -42,13 +38,11 @@ export function requesting( state = {}, action ) {
 export function revisions( state = {}, action ) {
 	if ( action.type === POST_REVISIONS_RECEIVE ) {
 		const { siteId, postId } = action;
-		return {
-			...state,
+		return merge( {}, state, {


Thanks for these updates, this is looking really good. Just one last thing, I think merge is not suited in this exact place because revision are objects that would be merged recursively here and since the reducer is not really aware of the shape of these objects. So, it's possible to end up with a meged revision object (old and new revision of the same Id) and the intent here is to replace the object completely.

Sorry, If I confused you with my previous comment, I like to use merge in reducer of scalar values (like above) but not when the reducers objects with unknown shapes.

nice catch!

Sorry, If I confused you with my previous comment, I like to use merge in reducer of scalar values (like above) but not when the reducers objects with unknown shapes.

hehe np, that's why code reviews are great :) that makes a lot of sense, will keep it in mind :)

I've added a "test" for it as well as reverting back to using spreads for that reducer.

…tations of discarding previous content

youknowriad

If you can just add a README for the query-post-revisions component.

This is looking really good. Great work 👍

youknowriad · 2017-04-24T07:55:18Z

client/components/data/query-post-revisions/README.md

+The site ID for which we request post revisions.
+
+
+### `siteId`


postId ? 😄

…h the original action once the request is completed.

nylen · 2017-04-27T18:53:49Z

I found out that Wordpress.com sites have a default limit of 25 revisions which means pagination wouldn't bring lots of value

What form does this limit take? I.e., when a post would go over 25 revisions, how is this handled?

bperson · 2017-04-27T19:05:26Z

What form does this limit take? I.e., when a post would go over 25 revisions, how is this handled?

it's dropping old revisions to make room for new ones (FIFO style) Not sure how it's done by the backend though.

Post: Add /revision route + early infrastructure to load revisions:…

e4fa86a

… `QueryPostRevisions`

matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels Apr 3, 2017

a8ci18n reviewed Apr 3, 2017

View reviewed changes

bperson added 4 commits April 3, 2017 17:04

Fix(tests): Add revisions to the expected shape of the posts redux …

59e3fb5

…state

Moving revisions action/reducer/selector into its own subdir under `s…

e5400ff

…tate/posts`

Post Revisions: Normalize revisions for state

071d119

Post Revisions: Add requesting to track the status of revisions req…

af903ce

…uests in Redux

bperson requested review from nylen and youknowriad April 4, 2017 15:55

lancewillett added the [Status] In Progress label Apr 6, 2017

Post Revisions: Use data-layer instead of redux-thunk to fetch revi…

40ec8e2

…sions

bperson mentioned this pull request Apr 12, 2017

Data Layer: Fix POST-ing formData with an apiVersion while introducing apiNamespace #13028

Closed

nylen reviewed Apr 13, 2017

View reviewed changes

youknowriad reviewed Apr 17, 2017

View reviewed changes

bperson added 4 commits April 18, 2017 13:32

Post Revisions: Remove all presentation code for a cleaner Redux PR #…

2999c43

…12733

Post Revisions: Tests for reducers

a6ca250

Post Revisions: Tests for selectors

ef55dd7

Query Post Revisions: Use componentDidUpdate instead of `componentW…

33d10ad

…illReceiveProps` to dispatch the data request

youknowriad reviewed Apr 21, 2017

View reviewed changes

bperson added 2 commits April 21, 2017 14:10

Post Revisions: Reducers: Use Lodash's merge over multiple spreads

f04b065

Post Revisions: Normalize Revisions API response in the data-layer

ce32197

youknowriad reviewed Apr 21, 2017

View reviewed changes

Post Revisions: Reducers: Revert using merge on reducers with expec…

696ef27

…tations of discarding previous content

youknowriad approved these changes Apr 21, 2017

View reviewed changes

Post Revisions: Add README for QueryPostRevisions component

d96500c

youknowriad reviewed Apr 24, 2017

View reviewed changes

youknowriad added [Status] Ready to Merge and removed [Status] In Progress labels Apr 24, 2017

bperson added 3 commits April 24, 2017 10:44

Query Post Revisions: Fix README typo: siteId -> postId

6498395

Post Revisions: Data Layer: Fix usage of http to make it re-dispatc…

8ea42a7

…h the original action once the request is completed.

Post Revisions: Fix tests broken by 8ea42a7

877d7cd

youknowriad changed the title ~~WIP - Post revisions into redux~~ Post revisions into redux Apr 24, 2017

youknowriad merged commit 63bfc14 into master Apr 24, 2017

youknowriad deleted the add/post-revisions branch April 24, 2017 16:37

matticbot removed the [Status] Ready to Merge label Apr 24, 2017

bperson mentioned this pull request May 18, 2017

Revisions #2: Revisions state update and diff-ing #14241

Merged

jblz mentioned this pull request Oct 25, 2017

Editor: Enable Post Revisions Feature in stage #19149

Merged

awaisab172 mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#174

Open

awaisab172 mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities awaisab172/wp-calypso#196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post revisions into redux #12733

Post revisions into redux #12733

bperson commented Apr 3, 2017 •

edited

Loading

a8ci18n Apr 3, 2017

nylen Apr 13, 2017

nylen Apr 13, 2017

bperson Apr 18, 2017

nylen Apr 13, 2017

bperson Apr 14, 2017 •

edited

Loading

bperson Apr 14, 2017 •

edited by nylen

Loading

nylen Apr 14, 2017

bperson Apr 14, 2017

nylen Apr 17, 2017

bperson Apr 18, 2017

nylen commented Apr 13, 2017

youknowriad left a comment

youknowriad Apr 17, 2017

bperson Apr 18, 2017

youknowriad Apr 17, 2017

bperson Apr 18, 2017

bperson commented Apr 21, 2017

youknowriad Apr 21, 2017

youknowriad Apr 21, 2017

bperson Apr 21, 2017

youknowriad Apr 21, 2017

bperson Apr 21, 2017 •

edited

Loading

youknowriad left a comment

youknowriad Apr 24, 2017

bperson Apr 24, 2017

nylen commented Apr 27, 2017

bperson commented Apr 27, 2017 •

edited

Loading

		The site ID for which we request post revisions.


		### `siteId`

Post revisions into redux #12733

Post revisions into redux #12733

Conversation

bperson commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bperson Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

bperson Apr 14, 2017 • edited by nylen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Apr 13, 2017

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bperson commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bperson Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Apr 27, 2017

bperson commented Apr 27, 2017 • edited Loading

bperson commented Apr 3, 2017 •

edited

Loading

bperson Apr 14, 2017 •

edited

Loading

bperson Apr 14, 2017 •

edited by nylen

Loading

bperson Apr 21, 2017 •

edited

Loading

bperson commented Apr 27, 2017 •

edited

Loading