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.

Sign up for GitHub

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

Editor: Reset Post Edits when switching posts in the post editor #10177

Merged
merged 5 commits into from
Jan 3, 2017
Merged

Editor: Reset Post Edits when switching posts in the post editor #10177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b641420
Editor: Reset Post Edits when switching posts in the post editor
youknowriad Dec 20, 2016
d029c25
Changes per review
youknowriad Dec 20, 2016
31c9c58
Adding editor start action
youknowriad Dec 20, 2016
de3074e
Trying a new approach
youknowriad Dec 22, 2016
f8f19dd
Changes per review
youknowriad Jan 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions client/post-editor/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ var actions = require( 'lib/posts/actions' ),
userUtils = require( 'lib/user/utils' ),
analytics = require( 'lib/analytics' );
import PostEditor from './post-editor';
import { setEditorPostId } from 'state/ui/editor/actions';
import { startEditingPost, stopEditingPost } from 'state/ui/editor/actions';
import { getSelectedSiteId } from 'state/ui/selectors';
import { getEditorPostId, getEditorPath } from 'state/ui/editor/selectors';
import { editPost } from 'state/posts/actions';

function getPostID( context ) {
if ( ! context.params.post || 'new' === context.params.post ) {
Expand Down Expand Up @@ -114,8 +113,7 @@ module.exports = {
const postID = getPostID( context );

function startEditing( siteId ) {
context.store.dispatch( setEditorPostId( postID ) );
context.store.dispatch( editPost( siteId, postID, { type: postType } ) );
context.store.dispatch( startEditingPost( siteId, postID, postType ) );

if ( maybeRedirect( context ) ) {
return;
Expand Down Expand Up @@ -177,6 +175,15 @@ module.exports = {
renderEditor( context, postType );
},

exitPost: function( context, next ) {
const postId = getPostID( context );
const siteId = getSelectedSiteId( context.store.getState() );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing a few errors here on the Calypso errors dashboard:

Cannot read property 'getState' of undefined

It's not clear to me when or why context.store would be ever be undefined.

Copy link
Contributor Author

@youknowriad youknowriad Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it's because https://github.com/Automattic/wp-calypso/blob/master/client/state/initial-state.js#L90 is async. It seems that the context is initialized on the callback of this function https://github.com/Automattic/wp-calypso/blob/master/client/boot/index.js#L260

Is this the only place we have this kind of errors? seems surprising to me

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad Following the logic from boot, I don't think we render anything until the initial state has finished loading. It may have something to do with it being a page.exit handler, which we don't use in very many (any?) other parts of Calypso.

if ( siteId ) {
context.store.dispatch( stopEditingPost( siteId, postId ) );
}
next();
},

pressThis: function( context, next ) {
context.getSiteSelectionHeaderText = function() {
return i18n.translate( 'Select a site to start writing' );
Expand Down
3 changes: 3 additions & 0 deletions client/post-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@ 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.exit( '/post/:site?/:post?', controller.exitPost );

page( '/page', sitesController.siteSelection, sitesController.sites );
page( '/page/new', () => page.redirect( '/page' ) ); // redirect from beep-beep-boop
page( '/page/:site?/:post?', sitesController.siteSelection, sitesController.fetchJetpackSettings, controller.post );
page.exit( '/page/:site?/:post?', controller.exitPost );

if ( config.isEnabled( 'manage/custom-post-types' ) ) {
page( '/edit/:type', sitesController.siteSelection, sitesController.sites );
page( '/edit/:type/new', ( context ) => page.redirect( `/edit/${ context.params.type }` ) );
page( '/edit/:type/:site?/:post?', sitesController.siteSelection, sitesController.fetchJetpackSettings, controller.post );
page.exit( '/edit/:type/:site?/:post?', controller.exitPost );
}
};
21 changes: 5 additions & 16 deletions client/post-editor/post-editor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ const actions = require( 'lib/posts/actions' ),
import { getSelectedSiteId } from 'state/ui/selectors';
import { setEditorLastDraft, resetEditorLastDraft } from 'state/ui/editor/last-draft/actions';
import { isEditorDraftsVisible, getEditorPostId, getEditorPath } from 'state/ui/editor/selectors';
import { toggleEditorDraftsVisible, setEditorPostId } from 'state/ui/editor/actions';
import { receivePost, resetPostEdits } from 'state/posts/actions';
import { toggleEditorDraftsVisible } from 'state/ui/editor/actions';
import { receivePost, savePostSuccess } from 'state/posts/actions';
import { getPostEdits, isEditedPostDirty } from 'state/posts/selectors';
import EditorDocumentHead from 'post-editor/editor-document-head';
import EditorPostTypeUnsupported from 'post-editor/editor-post-type-unsupported';
Expand Down Expand Up @@ -138,9 +138,6 @@ export const PostEditor = React.createClass( {
componentWillUnmount: function() {
PostEditStore.removeListener( 'change', this.onEditedPostChange );

// Reset post edits after leaving editor
this.props.resetPostEdits( this.props.siteId, this.props.postId );

// TODO: REDUX - remove flux actions when whole post-editor is reduxified
actions.stopEditing();

Expand Down Expand Up @@ -690,19 +687,12 @@ export const PostEditor = React.createClass( {
this.props.resetEditorLastDraft();
}

// Assign editor post ID to saved value (especially important when
// transitioning from an unsaved post to a saved one)
if ( post.ID !== this.props.postId ) {
this.props.setEditorPostId( post.ID );
}
// Remove this when the editor is completely reduxified ( When using Redux actions for all post saving requests )
this.props.savePostSuccess( post.site_ID, this.props.postId, post, {} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Only by a bit of luck does this not cause a notice to be shown (which would be non-ideal, at least for now, given that it's redundant with the editor's own notice). See:

wp-calypso/client/state/notices/middleware.js

Lines 78 to 93 in 795cd4d

export function onPostSaveSuccess( dispatch, action ) {
let text;
switch ( action.post.status ) {
case 'trash':
text = translate( 'Post successfully moved to trash' );
break;
case 'publish':
text = translate( 'Post successfully published' );
break;
}
if ( text ) {
dispatch( successNotice( text ) );
}
}
(it would show a notice if {} instead included a status property).

Arguably this is an argument against the lack of discoverability of middlewares 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I noticed these notices as well :). I'm starting to think, we should avoid using the middleware to automatically trigger notices on request success. I had to revert my own save settings global notices here #10278 to avoid double notices. I think we should rely on componentWillReceiveProps to trigger these notices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we could create separate extended action creators for a more specific variations if we only want a notice to be shown in some cases, e.g. adding a meta flag that the middleware checks the presence of.


// Receive updated post into state
this.props.receivePost( post );

// Reset previous edits, preserving type
this.props.resetPostEdits( this.props.siteId );
this.props.resetPostEdits( post.site_ID, post.ID );

const nextState = {
isSaving: false,
isPublishing: false
Expand Down Expand Up @@ -785,8 +775,7 @@ export default connect(
setEditorLastDraft,
resetEditorLastDraft,
receivePost,
resetPostEdits,
setEditorPostId,
savePostSuccess,
setEditorModePreference: savePreference.bind( null, 'editor-mode' ),
setLayoutFocus,
}, dispatch );
Expand Down
4 changes: 2 additions & 2 deletions client/state/action-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,9 @@ export const EDITOR_CONTACT_FORM_LOAD = 'EDITOR_CONTACT_FORM_LOAD';
export const EDITOR_CONTACT_FORM_SETTINGS_UPDATE = 'EDITOR_CONTACT_FORM_SETTINGS_UPDATE';
export const EDITOR_LAST_DRAFT_SET = 'EDITOR_LAST_DRAFT_SET';
export const EDITOR_MEDIA_EDIT_ITEM_SET = 'EDITOR_MEDIA_EDIT_ITEM_SET';
export const EDITOR_POST_ID_SET = 'EDITOR_POST_ID_SET';
export const EDITOR_SHOW_DRAFTS_TOGGLE = 'EDITOR_SHOW_DRAFTS_TOGGLE';
export const EDITOR_START = 'EDITOR_START';
export const EDITOR_STOP = 'EDITOR_STOP';
export const EXPORT_ADVANCED_SETTINGS_FETCH = 'EXPORT_ADVANCED_SETTINGS_FETCH';
export const EXPORT_ADVANCED_SETTINGS_FETCH_FAIL = 'EXPORT_ADVANCED_SETTINGS_FETCH_FAIL';
export const EXPORT_ADVANCED_SETTINGS_RECEIVE = 'EXPORT_ADVANCED_SETTINGS_RECEIVE';
Expand Down Expand Up @@ -292,7 +293,6 @@ export const POST_DELETE = 'POST_DELETE';
export const POST_DELETE_FAILURE = 'POST_DELETE_FAILURE';
export const POST_DELETE_SUCCESS = 'POST_DELETE_SUCCESS';
export const POST_EDIT = 'POST_EDIT';
export const POST_EDITS_RESET = 'POST_EDITS_RESET';
export const POST_FORMATS_RECEIVE = 'POST_FORMATS_RECEIVE';
export const POST_FORMATS_REQUEST = 'POST_FORMATS_REQUEST';
export const POST_FORMATS_REQUEST_FAILURE = 'POST_FORMATS_REQUEST_FAILURE';
Expand Down
28 changes: 12 additions & 16 deletions client/state/posts/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
POST_DELETE_SUCCESS,
POST_DELETE_FAILURE,
POST_EDIT,
POST_EDITS_RESET,
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
Expand Down Expand Up @@ -164,18 +163,21 @@ export function editPost( siteId, postId = null, post ) {
}

/**
* Returns an action object to be used in signalling that any post edits for
* the specified post should be discarded.
* Returns an action object to be used in signalling that a post has been saved
*
* @param {Number} siteId Site ID
* @param {Number} postId Post ID
* @return {Object} Action object
* @param {Number} siteId Site ID
* @param {Number} postId Post ID
* @param {Object} savedPost Updated post
* @param {Object} post Post attributes
* @return {Object} Action thunk
*/
export function resetPostEdits( siteId, postId ) {
export function savePostSuccess( siteId, postId = null, savedPost, post ) {
return {
type: POST_EDITS_RESET,
type: POST_SAVE_SUCCESS,
siteId,
postId
postId,
savedPost,
post
};
}

Expand All @@ -201,13 +203,7 @@ export function savePost( siteId, postId = null, post ) {
const normalizedPost = normalizePostForApi( post );
postHandle = postHandle[ postId ? 'update' : 'add' ].bind( postHandle );
return postHandle( { apiVersion: '1.2' }, normalizedPost ).then( ( savedPost ) => {
dispatch( {
type: POST_SAVE_SUCCESS,
siteId,
postId,
savedPost,
post
} );
dispatch( savePostSuccess( siteId, postId, savedPost, post ) );
dispatch( receivePost( savedPost ) );
} ).catch( ( error ) => {
dispatch( {
Expand Down
15 changes: 13 additions & 2 deletions client/state/posts/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,19 @@ import { get, set, omit, omitBy, isEqual, reduce, merge, findKey, mapValues } fr
*/
import PostQueryManager from 'lib/query-manager/post';
import {
EDITOR_START,
EDITOR_STOP,
POST_DELETE,
POST_DELETE_SUCCESS,
POST_DELETE_FAILURE,
POST_EDIT,
POST_EDITS_RESET,
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
POST_RESTORE,
POST_RESTORE_FAILURE,
POST_SAVE,
POST_SAVE_SUCCESS,
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_SUCCESS,
Expand Down Expand Up @@ -261,7 +263,16 @@ export function edits( state = {}, action ) {
}
} );

case POST_EDITS_RESET:
case EDITOR_START:
return Object.assign( {}, state, {
[ action.siteId ]: {
...state[ action.siteId ],
[ action.postId || '' ]: { type: action.postType }
}
} );

case EDITOR_STOP:
case POST_SAVE_SUCCESS:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered if clearing edits on success would have a negative impact where we currently depend on type to be always present, but realized that this should gracefully fall back to the saved post's type property. Nice 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About that, I think we should move the type out of the edits state. I don't think it should be stored there since it's not something editable. I would store this on the ui subtree, the same way we store the current edited ID. Maybe another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an edit in that it needs to be sent with the initial payload for a new draft of non-post types. Though you're right it's not really editable for saved items and posts of type "post".

if ( ! state.hasOwnProperty( action.siteId ) ) {
break;
}
Expand Down
31 changes: 17 additions & 14 deletions client/state/posts/test/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
POST_DELETE_SUCCESS,
POST_DELETE_FAILURE,
POST_EDIT,
POST_EDITS_RESET,
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
Expand All @@ -35,8 +34,8 @@ import {
requestSitePost,
requestPosts,
editPost,
resetPostEdits,
savePost,
savePostSuccess,
trashPost,
deletePost,
restorePost,
Expand Down Expand Up @@ -75,6 +74,22 @@ describe( 'actions', () => {
} );
} );

describe( 'savePostSuccess()', () => {
it( 'should return an action object', () => {
const savedPost = { ID: 841, title: 'Hello World' };
const attributes = { status: 'draft' };
const action = savePostSuccess( 10, 841, savedPost, attributes );

expect( action ).to.eql( {
type: POST_SAVE_SUCCESS,
siteId: 10,
postId: 841,
savedPost: savedPost,
post: attributes,
} );
} );
} );

describe( '#requestSitePosts()', () => {
useNock( ( nock ) => {
nock( 'https://public-api.wordpress.com:443' )
Expand Down Expand Up @@ -274,18 +289,6 @@ describe( 'actions', () => {
} );
} );

describe( '#resetPostEdits()', () => {
it( 'should return an action object', () => {
const action = resetPostEdits( 2916284 );

expect( action ).to.eql( {
type: POST_EDITS_RESET,
siteId: 2916284,
postId: undefined
} );
} );
} );

describe( 'savePost()', () => {
useNock( ( nock ) => {
nock( 'https://public-api.wordpress.com:443' )
Expand Down
75 changes: 71 additions & 4 deletions client/state/posts/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,19 @@ import deepFreeze from 'deep-freeze';
*/
import { useSandbox } from 'test/helpers/use-sinon';
import {
EDITOR_START,
EDITOR_STOP,
POST_DELETE,
POST_DELETE_SUCCESS,
POST_DELETE_FAILURE,
POST_EDIT,
POST_EDITS_RESET,
POST_REQUEST,
POST_REQUEST_SUCCESS,
POST_REQUEST_FAILURE,
POST_RESTORE,
POST_RESTORE_FAILURE,
POST_SAVE,
POST_SAVE_SUCCESS,
POSTS_RECEIVE,
POSTS_REQUEST,
POSTS_REQUEST_FAILURE,
Expand Down Expand Up @@ -892,15 +894,15 @@ describe( 'reducer', () => {
it( 'should ignore reset edits action when discarded site doesn\'t exist', () => {
const original = deepFreeze( {} );
const state = edits( original, {
type: POST_EDITS_RESET,
type: POST_SAVE_SUCCESS,
siteId: 2916284,
postId: 841
} );

expect( state ).to.equal( original );
} );

it( 'should discard edits when reset edits action dispatched', () => {
it( 'should discard edits when the post is saved', () => {
const state = edits( deepFreeze( {
2916284: {
841: {
Expand All @@ -911,7 +913,7 @@ describe( 'reducer', () => {
}
}
} ), {
type: POST_EDITS_RESET,
type: POST_SAVE_SUCCESS,
siteId: 2916284,
postId: 841
} );
Expand All @@ -925,6 +927,71 @@ describe( 'reducer', () => {
} );
} );

it( 'should ignore stop editor action when site doesn\'t exist', () => {
const original = deepFreeze( {} );
const state = edits( original, {
type: EDITOR_STOP,
siteId: 2916284,
postId: 841
} );

expect( state ).to.equal( original );
} );

it( 'should discard edits when we stop editing the post', () => {
const state = edits( deepFreeze( {
2916284: {
841: {
title: 'Hello World'
},
'': {
title: 'Ribs & Chicken'
}
}
} ), {
type: EDITOR_STOP,
siteId: 2916284,
postId: 841
} );

expect( state ).to.eql( {
2916284: {
'': {
title: 'Ribs & Chicken'
}
}
} );
} );

it( 'should reset edits when we start editing a post', () => {
const state = edits( deepFreeze( {
2916284: {
841: {
title: 'Hello World'
},
'': {
title: 'Ribs & Chicken'
}
}
} ), {
type: EDITOR_START,
siteId: 2916284,
postId: 841,
postType: 'jetpack-testimonial',
} );

expect( state ).to.eql( {
2916284: {
841: {
type: 'jetpack-testimonial'
},
'': {
title: 'Ribs & Chicken'
}
}
} );
} );

it( 'should not persist state', () => {
const state = edits( deepFreeze( {
2916284: {
Expand Down
Loading