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

Editor: CPT: Manage non-hierarchical term edits using Redux state #6548

Merged
merged 7 commits into from
Jul 17, 2016

Conversation

nylen
Copy link
Contributor

@nylen nylen commented Jul 6, 2016

Similar in approach to #5445.

To test

Verify no functional changes when editing non-hierarchical CPT terms:

Edit a post of a custom type that has one or more non-hierarchical taxonomies. Ensure that existing taxonomies are loaded correctly in the TokenField:

  • values (terms assigned to the saved post)
  • suggestions (all terms from the site)

Ensure that terms can be updated and saved as expected.

Test live: https://calypso.live/?branch=update/editor/cpt-term-token-field-redux

@nylen nylen force-pushed the update/editor/cpt-term-token-field-redux branch 2 times, most recently from 118501b to d586f70 Compare July 16, 2016 17:53
@nylen nylen added [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 Jul 16, 2016
@@ -54,7 +54,7 @@ EditorDrawerTaxonomies.propTypes = {
siteId: PropTypes.number,
postType: PropTypes.string,
postTerms: PropTypes.object,
taxonomies: PropTypes.array
taxonomies: PropTypes.array,
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing commas. so hot right now.

@nylen
Copy link
Contributor Author

nylen commented Jul 16, 2016

Found a bug with this logic: if you add a tag then you cannot remove it until saving the draft with the tag added.

@nylen nylen added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 16, 2016
* @param {Number} postId Post ID
* @return {Boolean} Whether content exists
*/
export function isEditedPostContentEmpty( state, siteId, postId ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, we should remove this selector, tests and usage from this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 1e58d39.

@nylen
Copy link
Contributor Author

nylen commented Jul 16, 2016

The bug is related to the use of _.merge to manipulate post edits: they are accumulated in the reducer logic for EDIT_POST and resolved in the getEditedPost selector.

This is correct and useful for some cases like nested properties, but not for array-like properties where items can be added and removed.

I'll address this by moving from merge to mergeWith using a custom function that does not attempt to merge arrays.


On a related note, the Redux state for terms attached to a post is a bit inconsistent because we receive an object like this from the API:

{
    terms: {
        taxonomyName: {
            term1: { ID: 1, ... }
        }
    }
}

and upon editing a (non-hierarchical) taxonomy, we end up with an object like this:

{
    terms: {
        taxonomyName: [ 'termName1', ... ]
    }
}

There are a couple of issues with this:

  • Duplicate information in the state tree: the full information for each term is stored in duplicate in the site terms state and in the post state.
  • The shape of the terms state on the post object changes based on whether or not it has been edited.

I think the ideal solution for this is to normalize the post terms state in the reducer logic for the POST_RECEIVE action. To accommodate hierarchical terms that can have different parents but the same name, the state for a post should look like this:

{
    terms: {
        taxonomyName: [ {
            ID: 1,
            name: 'termName1',
        }, {
            ...
        } ]
    }
}

I'm inclined to address this in a separate PR, though, since we're waiting for the editor redux code in this PR for a couple of other tasks.

@timmyc
Copy link
Contributor

timmyc commented Jul 16, 2016

Related discovery and notes about the discrepancies from the API in #6468

@nylen
Copy link
Contributor Author

nylen commented Jul 16, 2016

Bug fixed in 6e8f380; marking as Needs Review again.

@nylen nylen added [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 Jul 16, 2016
@timmyc
Copy link
Contributor

timmyc commented Jul 16, 2016

Confirmed the bug is fixed with the mergeWith approach. Looking good and testing out in general for me as well. @aduth do you want to give this one a spin?

@@ -569,7 +573,10 @@ const PostEditor = React.createClass( {

this.saveRawContent();
// TODO: REDUX - remove flux actions when whole post-editor is reduxified
actions.edit( { content: this.refs.editor.getContent() } );
const edits = merge( {}, this.props.edits, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple thoughts here:

Could we get away with a shallow clone? That opens up possibility of mutating the original reference on nested properties (terms, discussion), but I feel that's unlikely to happen.

const edits = {
    ...this.props.edits,
    content: this.refs.editor.getContent()
};

Otherwise, we could make this ever so slightly more efficient by merging the prop edits into the constructed object directly.

const edits = merge( {
    content: this.refs.editor.getContent()
}, this.props.edits );

(Same goes for below where we're created a new merged object with status)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fine - mutating individual elements of the state might cause subtle issues, but no one should be doing that anyway. Done in ae1fb72.

@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 Jul 17, 2016
@nylen nylen force-pushed the update/editor/cpt-term-token-field-redux branch from 83251c8 to 052bfc9 Compare July 17, 2016 18:29
aduth and others added 6 commits July 17, 2016 13:33
Adds a `mergeIgnoringArrays` utility method that works correctly when
editing arrays of terms.

Also, use shallow clone instead of lodash merge to resolve post edits.
Two cases where we’d thought it’d be needed:

(a) New post has type assigned, so immediately dirty. This is okay
because because post has no content, it can’t be saved. With content,
it’s dirty anyways.

(b) Existing posts. Test previously was flawed because preexisting post
object would have post assigned, and dirty checking would know that it
hasn’t changed despite being in edits object.
@nylen nylen force-pushed the update/editor/cpt-term-token-field-redux branch from 052bfc9 to 0556ad0 Compare July 17, 2016 18:33
@nylen nylen merged commit a6d1224 into master Jul 17, 2016
@nylen nylen deleted the update/editor/cpt-term-token-field-redux branch July 17, 2016 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Custom Post Types (CPT) [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants