Allow saving and publishing posts #594

nylen · 2017-05-01T20:43:25Z

This PR adds the capability to publish new posts and update existing posts via the WP REST API. Closes #526.

This PR is the first time we actually need to store all of the data for a post object in the Redux state. It looks like our combineUndoableReducers utility expects to have all undoable actions reside within a single key in the state tree, and the post properties should be part of the undo history. Accordingly, I've renamed state.blocks to state.editor:

state.blocks.byUid → state.editor.blocksByUid
state.blocks.order → state.editor.blockOrder
newly introduced state.editor.post

Even though we're not using Backbone in this project, I'm using the wp-api.js Backbone client to save posts because the library is already included in WP core and is the simplest way to make an API call. The alternative would be to use jQuery or another AJAX library, which would involve some error-prone bits of setup that are already handled for us (like building the API URLs and making sure they work with all the different permalink structures).

Currently the state of the publish/update operation is reflected in the main action button text. I expect we'll want to change this later, but there are 8 possible messages currently:

Publish (editing a new post; no action has been taken)
Publishing... (in the process of publishing a new post; the button is disabled)
Published! (just published a new post)
Publish failed (an error occurred)
Update (editing an existing post; no action has been taken)
Updating... (in the process of updating an existing post; the button is disabled)
Updated! (just updated an existing post)
Update failed (an error occurred)

Until the plugin is more stable, I wonder if we should update the default action to be creating a new draft instead of immediately publishing a post?

joyously · 2017-05-01T22:05:49Z

but there are 8 possible messages currently

Does this include what the Contributor role would see? That role can only save as draft, not publish. Don't the messages come from the server?

nylen · 2017-05-01T22:26:23Z

That's a good point about some users not being able to publish. I think this is another reason to limit the plugin to create new drafts or update existing posts for now - keep in mind that we haven't built the controls to change post status yet.

youknowriad · 2017-05-02T07:52:18Z

editor/header/tools/publish-button.js

+		buttonText = wp.i18n.__( 'Update' );
+	} else {
+		buttonText = wp.i18n.__( 'Publish' );
+	}


I know some people don't like it but I prefer to use switch (true) in these cases.

const buttonEnabled = ! isRequesting; switch( true ) { case isRequesting && requestIsNewPost: buttonText = wp.i18n.__( 'Publishing...' ); break; case isRequesting && !requestIsNewPost: buttonText = wp.i18n.__( 'Publishing...' ); break; // ... }

IMO it's more readable.

I'd probably change this to a single if chain without nesting instead, like if ( isRequesting && requestIsNewPost ). I'll try it

youknowriad · 2017-05-02T07:54:01Z

editor/header/tools/publish-button.js

+			post,
+			isNew,
+		} );
+	} );


Is it time to start separating the action creators into their own actions.js file. This would ease testing these actions. I'm also thinking about testing the mergeWithPrevious action this way.

Yes, this definitely doesn't belong here.

youknowriad · 2017-05-02T07:57:42Z

editor/state.js

+ * @param  {Object} action Dispatched action
+ * @return {string}        Updated state
+ */
+export function api( state = {}, action ) {


Should this be renamed saving or something like that. Thinking we may have other unrelated requests in the future.

youknowriad

Great work @nylen, This is testing well!

Should we drop the "Published" or "Updated" message in favor of "Update" when the post is made dirty again?

nylen · 2017-05-02T14:38:28Z

Should we drop the "Published" or "Updated" message in favor of "Update" when the post is made dirty again?

Yes - do we have the mechanism in place yet to detect dirty content?

youknowriad · 2017-05-02T15:02:50Z

Yes - do we have the mechanism in place yet to detect dirty content?

We don't for now, we could do these actions: 'INSERT_BLOCK', 'UPDATE_BLOCK', 'SWITCH_BLOCK_TYPE', 'MOVE_BLOCK_DOWN', 'MOVE_BLOCK_UP' and 'REMOVE_BLOCK'. Granted this is bot very convenient.

nylen · 2017-05-02T16:36:15Z

Ok - this should also include typing (content changes), so I'd prefer to wait until this is implemented fully and we have an isDirty state we can easily hook into. So I think what's there is ok for this PR. I still want to make the following changes today:

Save drafts instead of publishing posts
Clean up the messages
Split savePost into a new actions.js file and add a couple of tests
Rename api state to saving for now; maybe we'll need a better / more flexible mechanism to store request state later on

nylen · 2017-05-02T22:49:13Z

I think this is ready for another round of review. The latest commits address all feedback except the "dirty state" which I've started to explore in #610.

youknowriad · 2017-05-03T07:45:46Z

editor/header/tools/publish-button.js

+	( dispatch ) => ( {
+		onUpdate( post, blocks ) {
+			post.content.raw = wp.blocks.serialize( blocks );
+			savePost( dispatch, post );


I wonder if we should introduce, redux-thunk or any alternative async middleware to simplify this. Could be done later.

Discussed further at #594 (comment). As noted there, I'd prefer to explore this separately.

youknowriad

Thanks @nylen. This is a great addition towards allowing people to test the plugin with real posts. 👍

mtias · 2017-05-03T09:05:16Z

@jasmussen mind giving a quick look at the design implications for the different states?

jasmussen · 2017-05-03T10:11:37Z

Really nice work. This is exciting!

UI wise we should try some changes, so the publish button doesn't do it all. Though I don't think these changes specifically have to happen in this PR — it works pretty well and is so exciting we shouldn't let a few UI tweaks prevent it from being merged.

Here are some blueprint mockups. First, a publish dropdown that does not need to happen immediately, if at all. I will open a separate ticket for it, but wanted to show it here regardless:

Publish flow:

Drafting flow:

If you agree the above makes sense, I'll open a separate ticket for splitting drafting/saving behavior out into a separate ticket.

There are also some thoughts in #486.

nylen · 2017-05-03T19:21:33Z

Let's get this one merged as soon as possible and continue design improvements in separate PRs, it's renaming one of the main state keys so there are already a lot of conflicts.

I'll clean those up later today and plan to merge unless anyone has further feedback, in particular @aduth I don't think you've taken a look through here yet.

aduth · 2017-05-03T23:23:24Z

editor/actions.js

+export function savePost( dispatch, post ) {
+	const isNew = ! post.id;
+	dispatch( {
+		type: 'POST_UPDATE_REQUEST',


Thus far we've taken the convention of prefixing the action verb, not suffixing, i.e. REQUEST_POST_UPDATE vs. POST_UPDATE_REQUEST. I don't feel strongly one way or the other, but noting the inconsistency.

aduth · 2017-05-03T23:35:08Z

editor/state.js

+export function saving( state = {}, action ) {
+	switch ( action.type ) {
+		case 'POST_UPDATE_REQUEST':
+			return {


When a reducer returns an object of consistent keys, I wonder if it may be better to use combineReducers which has the advantages of not updating state references when the simple object values don't change (reference) and allowing you to ignore keys not relevant to a specific action type (maybe error in the case of POST_UPDATE_REQUEST_SUCCESS).

Something like:

export const saving = combineReducers( { requesting: ( state = false, action ) => { /* ... */ }, successful: ( state = false, action ) => { /* ... */ }, error: ( state = null, action ) => { /* ... */ }, isNew: ( state = false, action ) => { /* ... */ } } );

Might end up being more sprawling, and benefit might be minimal here, so not something I'd push hard for.

I tried this out, and it ended up being longer and (IMO) harder to read:

export const saving = combineReducers( { requesting: ( state = false, action ) => { switch ( action.type ) { case 'REQUEST_POST_UPDATE': return true; case 'REQUEST_POST_UPDATE_SUCCESS': case 'REQUEST_POST_UPDATE_FAILURE': return false; } return state; }, successful: ( state = false, action ) => { switch ( action.type ) { case 'REQUEST_POST_UPDATE_SUCCESS': return true; case 'REQUEST_POST_UPDATE_FAILURE': return false; } return state; }, error: ( state = false, action ) => { switch ( action.type ) { case 'REQUEST_POST_UPDATE_SUCCESS': return false; case 'REQUEST_POST_UPDATE_FAILURE': return true; } return state; }, isNew: ( state = false, action ) => { switch ( action.type ) { case 'REQUEST_POST_UPDATE': case 'REQUEST_POST_UPDATE_SUCCESS': case 'REQUEST_POST_UPDATE_FAILURE': return action.isNew; } return state; }, } );

In this case, at least, I prefer the explicitness of "here are 3 action types and here is how each property changes in response". I think updating state references and possible re-renders when a network request is fired or completed is OK.

aduth · 2017-05-03T23:42:04Z

editor/state.js

 		switch ( action.type ) {
-			case 'REPLACE_BLOCKS':
+			case 'EDIT_POST':


Won't this fall out of sync with state.editor.post when receiving the saved post via POST_UPDATE_REQUEST_SUCCESS if the content was manipulated on the server? Not clear to me whether we'll need the entire post object in state, or pieces like postId (all we're using at the moment).

This is a good point (and the above regarding undoable), this is probably not stored correctly. I think we'll end up needing to maintain something like editor.post.edits (containing any changed keys, the next one other than content will likely be status).

Then, separately, do we need to store the full post object outside of the undoable reducer to represent what we think is the current state of the post on the server, and update it when we receive better information? Maybe we don't need this functionality yet.

Update - this PR is currently using the latest known version of the post object to determine if the currently edited post is new (has an ID) or not. Also lots of interesting uses for this later: with regular updates from the server, and conflict resolution where appropriate (a huge task, to be fair), we basically have collaborative editing.

aduth · 2017-05-03T23:44:44Z

editor/state.js

+				return action.post || state;
+
+			case 'POST_UPDATE_REQUEST_SUCCESS':
+				return action.post;


By including this in the undoable reducer, seems like we'll be able to revert state back to convincing the UI that the post is a new draft, even if we know it's been saved. Will we need to include POST_UPDATE_REQUEST_SUCCESS in reset types?

Good point. Probably the entire post object shouldn't be included in the undoable state, rather just the list of edits like we do in Calypso. Seems closely related to the thread below as well (#594 (comment)).

aduth · 2017-05-03T23:46:47Z

editor/header/tools/publish-button.js

+		},
+
+		onSaveDraft( post, blocks ) {
+			post.content.raw = wp.blocks.serialize( blocks );


post here is a reference to state.editor.post, so we're mutating state directly.

http://redux.js.org/docs/introduction/ThreePrinciples.html#state-is-read-only

Might be enough to do:

savePost( dispatch, { ...post, status: 'draft', content: { ...post.content, raw: wp.blocks.serialize( blocks ) } } );

aduth · 2017-05-03T23:50:30Z

editor/header/tools/publish-button.js

+	onUpdate,
+	onSaveDraft,
+} ) {
+	let buttonEnabled = true;


Minor: We could calculate this at the variable declaration without much trouble:

const isButtonEnabled = ! isRequesting;

aduth · 2017-05-03T23:53:55Z

editor/header/tools/publish-button.js

+	if ( isRequesting ) {
+		buttonEnabled = false;
+		buttonText = requestIsNewPost
+			? wp.i18n.__( 'Saving...' )


I don't know that it's documented as a core i18n convention, but in other projects we've enforced that ellipsis be represented with the ellipsis character for semantic meaning, i.e. … instead of ....

https://github.com/Automattic/eslint-plugin-wpcalypso/blob/master/docs/rules/i18n-ellipsis.md

aduth · 2017-05-04T00:16:48Z

editor/actions.js

+ */
+import { get } from 'lodash';
+
+export function savePost( dispatch, post ) {


Already noted by @youknowriad that we'll need to consider adopting a pattern for asynchronous data flow. This file is perhaps a bit misleading since at a glance I'd expect it to include a set of action creators, but this isn't a true action creator in that it accepts dispatch and doesn't return an action object.

type ActionCreator = (...args: any) => Action | AsyncAction

http://redux.js.org/docs/Glossary.html#action-creator

I'm fine to flesh this out separately; redux-thunk is a popular option, but I've become less a fan of it over time, since thunks are hard to test, less predictable, and with the default implementation, encourage access to state via the second getState argument (thus breaking the unidirectional data flow).

If I were to suggest an alternative, I think it shouldn't be much trouble to devise a pattern to encourage treating actions with asynchronous effects as expressing intent. Using middleware(s), those intents could be mapped to side effects which themselves may incur additional actions.

Roughly something like: https://gist.github.com/aduth/b36ac70f0d3cd3f61faa0b2b81b98a2a

Not set in stone, specifically:

Couldn't savePost just return REQUEST ? Maybe we don't need a separate effects mapping. But sometimes the save intent isn't always just about firing a request, but affecting reducer values (e.g. SAVE_POST provides semantic meaning that of just REQUEST which should cause the UI to update to reflect in-progress save)

Instead of an effect's success property, we could enforce that the request completion itself should be an action expressing intent REQUEST_COMPLETED that has its own effect(s) to cause receivePost to be dispatched

Maybe we forgo the REQUEST middleware and treat effects as mini-middlewares which have access to dispatch and could just use fetch or wp-api.js in their implementations

Anyways, this has been a bit of a brain dump that'll get lost in pull request history, but jotting down now that it's becoming relevant since this has been on my mind lately.

A few additional resources from side projects where I've been applying these ideas:

https://github.com/aduth/dones/blob/master/src/state/requests/middleware.js

https://github.com/aduth/dones/blob/master/src/state/dones/effects.js

https://github.com/aduth/refx

I would prefer to address this separately as well. Unlike other parts of this PR (how we actually store the post + its edits in state), it should be pretty easy to refactor this later without touching lots of other parts of the code.

aduth · 2017-05-04T00:32:21Z

editor/actions.js

+			window.history.replaceState(
+				{},
+				'Post ' + newPost.id,
+				window.location.href.replace( /&post_id=[^&]*$/, '' )


Don't know how much we care to address this here, but these are very specific assumptions about URLs where the editor would be loaded. Also breaks down on some valid URLs:

'/admin.php?page=gutenberg&post_id=5'.replace( /&post_id=[^&]*$/, '' ) // Correct: "/admin.php?page=gutenberg" '/admin.php?post_id=5&page=gutenberg'.replace( /&post_id=[^&]*$/, '' ) // Incorrect: "/admin.php?post_id=5&page=gutenberg" '/admin.php?page=gutenberg&post_id=5&foo=1'.replace( /&post_id=[^&]*$/, '' ) // Incorrect: "/admin.php?page=gutenberg&post_id=5&foo=1"

In a final solution we'll likely want to consider a querystring / URL parser & serializer, such as:

https://nodejs.org/api/querystring.html

https://nodejs.org/api/url.html

https://www.npmjs.com/package/querystringify

This seems easy enough to address here.

#594 (comment)

Addresses these review threads: #594 (comment) #594 (comment) #594 (comment)

#594 (comment)

nylen · 2017-05-05T20:49:21Z

I've addressed or at least responded to all review feedback. @aduth is there anything else you think we should do in this PR?

aduth

I'm going to create a handful of follow-up task issues regarding some of the points we've discussed here, but this should serve as a good base for moving forward 👍

nylen requested review from aduth and youknowriad May 1, 2017 20:43

youknowriad reviewed May 2, 2017

View reviewed changes

mtias added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 2, 2017

nylen force-pushed the add/save-and-publish-posts branch from 82c2efd to 53accc0 Compare May 2, 2017 22:25

nylen mentioned this pull request May 2, 2017

Add basic 'dirty state' handling #610

Merged

youknowriad reviewed May 3, 2017

View reviewed changes

youknowriad approved these changes May 3, 2017

View reviewed changes

mtias added General Interface Parts of the UI which don't fall neatly under other labels. Design labels May 3, 2017

mtias added this to the May Week 1 milestone May 3, 2017

aduth reviewed May 4, 2017

View reviewed changes

nylen force-pushed the add/save-and-publish-posts branch from 2479f1e to f9a6320 Compare May 4, 2017 03:32

nylen added 17 commits May 5, 2017 11:48

Update page URL after creating a new post

7a994ea

Make eslint happy again

fe02d3f

Make the test suite happy again

77c37c9

Make message determination more readable

b9ac89d

Save drafts instead of publishing posts

ad29d20

Move savePost to a new actions.js file

9954198

Rename state.api to state.saving

8cce4eb

Change POST_UPDATE_REQUEST to REQUEST_POST_UPDATE

12252a0

#594 (comment)

Parse the query string rather than using a regex replace

53fb259

#594 (comment)

Store post edits in undoable state and full post object in another key

a2fcfea

Addresses these review threads: #594 (comment) #594 (comment) #594 (comment)

Fix a typo in the saving reducer

5b2c081

Simplify assignment of buttonEnabled flag

dbcb85b

#594 (comment)

Use ellipsis character instead of three dots

72e7aa8

#594 (comment)

Clarify window.location splitting logic using array destructuring

7872b75

Add tests for editor.edits reducer

5106309

Add tests for currentPost reducer

0702d66

Add tests for saving reducer

e938d3e

nylen force-pushed the add/save-and-publish-posts branch from 1ae4810 to e938d3e Compare May 5, 2017 20:37

Update dirty state key

d83ea67

aduth approved these changes May 5, 2017

View reviewed changes

This was referenced May 5, 2017

Decide whether we need formal asynchronous state patterns #691

Closed

Consider state middleware injection as extensibility mechanism #692

Closed

nylen added this to the May Week 1 milestone May 5, 2017

nylen merged commit fa237f9 into master May 5, 2017

nylen deleted the add/save-and-publish-posts branch May 5, 2017 22:52

This was referenced May 5, 2017

Consider adding Redux state selectors #693

Closed

Integrate dirty state, API requests, and publish button #700

Merged

jasmussen mentioned this pull request May 8, 2017

UI: Separate out drafting, saving, publishing actions #707

Closed

Verdinjoshua26 mentioned this pull request Nov 30, 2023

[Snyk] Security upgrade eslint-plugin-jest from 23.20.0 to 24.0.0 Verdinjoshua26/gutenberg#99

Open

Allow saving and publishing posts #594

Allow saving and publishing posts #594

Conversation

nylen commented May 1, 2017

joyously commented May 1, 2017

nylen commented May 1, 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

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

nylen commented May 2, 2017

youknowriad commented May 2, 2017

nylen commented May 2, 2017

nylen commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

mtias commented May 3, 2017

jasmussen commented May 3, 2017 • edited Loading

nylen commented May 3, 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

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

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

Choose a reason for hiding this comment

nylen commented May 5, 2017

aduth left a comment

Choose a reason for hiding this comment

jasmussen commented May 3, 2017 •

edited

Loading