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

Post Creation #134

Merged
merged 44 commits into from May 25, 2016

Conversation

Projects
None yet
2 participants
@valendesigns
Copy link
Member

commented May 11, 2016

Fixes #48, fixes #50

  • Create a new post/page and reload the preview while giving focus to that section
  • Ensure any newly created post/page persist in the panel
  • Add a customize-draft status which is assigned to an auto-draft post once it has been saved as part of a snapshot. This will prevent it from being garbage-collected.
  • Add link in a post section to allow navigating back to the post/page in the preview after having navigated away.
  • Display Ajax error messages in the console
  • Add unit tests
  • Rework the dependency on refreshing the preview to see new posts in the panel

@valendesigns valendesigns added this to the 0.6 milestone May 11, 2016

valendesigns and others added some commits May 11, 2016

@westonruter westonruter changed the title [WIP] Post Authoring [WIP] Post Creation May 11, 2016

@westonruter

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

@valendesigns what were the todos we listed out for this PR? I added one in the description. I realize we can continue to use auto-draft for posts added in the Customizer; we only need to introduce the customize-draft to prevent posts from being garbage-collected after they have become part of a snapshot. We'll need to think this through further with transactions, however.

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

I'll update this later tonight I just got back from the airport. 😀

valendesigns and others added some commits May 17, 2016

panel: panel
} ) );

$( '.add-new-' + panel.postType ).on( 'click', function( event ) {

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

I believe this should be scoped, for example:

panel.container.find( '.add-new.button-secondary' ).on( … )

In this case, there wouldn't need to be a type-specific class name because the button would be scoped to the container.

@@ -64,6 +66,62 @@
},

/**
* Add new post UI & listen for click events.
*/
addNewPost: function() {

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Let's rename this to something like setupPostAddition. A method like addNewPost would seem like a method we'd want to use to actually add a new post, like the result of clicking the button.

add_action( 'customize_controls_print_footer_scripts', array( $this, 'render_templates' ) );
add_action( 'init', array( $this, 'register_customize_draft' ) );
add_filter( 'customize_snapshot_save', array( $this, 'transition_customize_draft' ) );
add_action( 'pre_get_posts', array( $this, 'exclude_non_previewed_drafts' ) );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

I think this action should be added in WP_Customize_Post_Setting::preview() if not has_action() already. Similar to https://github.com/xwp/wordpress-develop/blob/8345fc60c7498b914d618b1b43a281fe269a0757/src/wp-includes/customize/class-wp-customize-nav-menu-item-setting.php#L396-L399

if ( $post instanceof WP_Post ) {
$data = array(
'sectionId' => WP_Customize_Post_Setting::get_post_setting_id( $post ),
'url' => get_preview_post_link( $post->ID ),

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Should this be using Edit_Post_Preview::get_preview_post_link() instead?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 18, 2016

Author Member

We sure can.

wp_send_json_error( 'missing_post_type' );
}
$post_type_obj = get_post_type_object( $_POST['post_type'] );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

wp_unslash() for good measure.

wp_send_json_error( 'insufficient_post_permissions' );
}
$post = $this->add_new_post( $_POST['post_type'] );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Use $post_type_object->name instead of accessing input var again.

add_filter( 'wp_insert_post_empty_content', '__return_false' );
$args = array(
'post_status' => 'auto-draft',
'post_type' => $post_type,

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Lacking tabs for indenting the array args.


request.fail( function() {

// @todo Display errors in the Customize Settings Validation area.

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

For now we can just console.error() it out. The notification area depends on https://core.trac.wordpress.org/ticket/35210

@@ -172,6 +184,7 @@ public function enqueue_customize_scripts() {
*/
public function make_auto_draft_status_previewable() {
global $wp_post_statuses;
$wp_post_statuses['auto-draft']->public = true;
$wp_post_statuses['auto-draft']->protected = true;

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Maybe this should also be added to WP_Customize_Post_Setting::preview() since customize_preview_init does not trigger in a frontend request?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 18, 2016

Author Member

We don't need auto-draft on the frontend because they are transitioned to customize-draft in snapshots.

@@ -77,6 +84,13 @@ public function __construct( WP_Customize_Manager $manager ) {
add_filter( 'customize_dynamic_setting_args', array( $this, 'filter_customize_dynamic_setting_args' ), 10, 2 );
add_filter( 'customize_dynamic_setting_class', array( $this, 'filter_customize_dynamic_setting_class' ), 5, 3 );
add_filter( 'customize_save_response', array( $this, 'filter_customize_save_response_for_conflicts' ), 10, 2 );
add_action( 'customize_controls_print_footer_scripts', array( $this, 'render_templates' ) );
add_action( 'init', array( $this, 'register_customize_draft' ) );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Should this also be added to WP_Customize_Post_Setting::preview()? Could there be an scenario where a non-publish post could get leaked accidentally otherwise? Or rather, maybe the preview method could grab the post status object and set the public property to true?

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

My comment is no longer valid.

}
return $data;
}

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Nice work on transition_customize_draft.

public function exclude_non_previewed_drafts( $query ) {
if ( $query->is_main_query() && ! is_admin() && ! $query->is_single() && ! empty( $_REQUEST['customized'] ) ) {
$post_ids = array();
$settings = json_decode( wp_unslash( $_REQUEST['customized'] ), true );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

Replace this (and $_REQUEST['customized'] above) with $settings = $this->manager->unsanitized_post_values().

) );
$excluded = array_diff( $draft_query->posts, $post_ids );
if ( ! empty( $excluded ) ) {
$query->set( 'post__not_in', $excluded );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

This should amend to any pre-existing post__not_in.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 18, 2016

Author Member

Makes sense!

'posts_per_page' => -1,
'post_status' => self::$draft_status,
'fields' => 'ids',
) );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

@valendesigns I wonder if there couldn't be a way to include as opposed to exclude posts?

This query for getting all posts could hit a lot of posts.

Let me confirm my understanding for how Customizer draft posts are appearing in the query: currently it is setting the post status for auto-draft and customize-draft to being public so that they will be included in the call to get_post_stati() to obtain the list of post statuses that should be used in a query: https://github.com/xwp/wordpress-develop/blob/8345fc60c7498b914d618b1b43a281fe269a0757/src/wp-includes/query.php#L3093-L3116

But what if the current WP_Query did include a specified post_status? Even with 'post_status' => 'publish', which is the default arg supplied when calling get_posts(), it would result in these Customizer posts not being included. I suppose that's in part why you have the $query->is_main_query() check?

Here's an idea for an alternate approach for how we could perhaps go about being inclusive rather than exclusive:

  1. Let our statuses not have the public post status.
  2. Leave the original pre_get_posts alone to do its normal thing.
  3. Add a the_posts filter which takes the current query vars for the current $query and then creates a new WP_Query that supplies the query vars merged with posts__in => $post_ids, to obtain all of the posts in the Customizer dirty state that align with the query vars in the existing query. (Naturally there'd need to be a mechanism to prevent infinite the_posts filter recursion here.)
  4. The the_posts filter for previewing posts data should have overridden each of the posts at this point to use the actual post_status from the current Customizer state.
  5. The list of $posts in the subquery can now be iterated over and any can be rejected that have a previewed post_status that does not align with the post_status from the parent query.
  6. The remaining posts can be amended to the post list that is returned by the_posts, ensuring that no duplicates are introduced, that the number returned does not exceed the posts_per_page, and that the posts are re-sorted according to order and orderby.

The result here should be that the query is more efficient, but also (more importantly) it will ensure that you will be take an existing draft or private post, change its status to publish in the Customizer, and be able to see such posts in queries for publish status posts in the preview. At the moment, the logic here is only accounting for posts that have never been saved.

Does this make sense?

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 20, 2016

Author Member

@westonruter I've figured out a way to be inclusive by filtering posts_where, and it's working with CPT's; which is pretty magical. I'm just writing the unit tests and I'll push this up. I think it makes for a very big improvement over the previously exclusive code and should be much more performant. As well, we no longer will be setting public to true for these draft statuses, which had some very strange side effects. Also, pagination is working without any special sorting.

This comment has been minimized.

Copy link
@westonruter

westonruter May 20, 2016

Contributor

Looking forward to seeing the magic 🌟

} );

request.done( function( response ) {
wp.customize.previewer.previewUrl( response.url );

This comment has been minimized.

Copy link
@westonruter

westonruter May 18, 2016

Contributor

@valendesigns in thinking about adding new Nav Menu Items, I'm second guessing delegating to the preview to create the new post section once the preview loads. I think that we should include the post setting data in the Ajax response and pass it along to receivePreviewData (but extracting the logic from there to be useful in scenarios where the preview refreshes or if the post is added without intending to navigate to that post in the preview).

This will also be key for adding new posts that are not public but rather are embedded within other posts.

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 25, 2016

@westonruter This PR is ready for a final review.

public function include_previewed_drafts( $where, $query ) {
global $wpdb;
if ( ! $query->is_admin() && ! $query->is_singular() ) {

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

PhpStorm is complaining here that is_admin() is not a method found on WP_Query. I can't find a WP_Query::is_admin() method either, but also it is not throwing an error for me which is also unexpected. I suppose it is because there is a magic getter defined.

So that leads me to think that $query->is_admin() is actually always returning null and isn't returning the bool that you expect if accessing $query->is_admin.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 25, 2016

Author Member

is_admin() will work.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 25, 2016

Author Member

We can change it to if ( ! is_admin() && ! $query->is_singular() ) {

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

For some reason $wp_query->is_admin() is returning a bool. However note:

$query = new WP_Query();
$query->is_admin = true;
$this->assertTrue( $query->is_admin() );

This test fails. All of this to say, I think that $query->is_admin should be used instead of $query->is_admin()

@valendesigns

This comment has been minimized.

Copy link
Member Author

commented May 25, 2016

@westonruter Looks good!

@valendesigns valendesigns changed the title [WIP] Post Creation Post Creation May 25, 2016

westonruter added some commits May 25, 2016

Restore navigating to the newly-inserted post if publicly-queryable
* Harden logic for focusing on first control in section
* Reject a promise with the error message provided.
* Export publicly_queryable among the post type args.
Only show the post nav link if the current previewUrl is not this pos…
…t's URL

Skip showing navigation link if post type is not publicly_queryable
_.defer( function() {
firstControl.focus( {
completeCallback: function() {
firstControl.container.find( 'input:first' ).select();

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

Haha. I really did do some egregious callback nesting here didn't I 😆

* @param string $post_type Main query post type.
* @param string $setting_post_type Current setting post type.
*/
$main_query_post_type = apply_filters( 'customize_posts_main_query_post_type', 'post', $matches['post_type'] );

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

I'm not totally sure about the need for this filter.

This comment has been minimized.

Copy link
@valendesigns

valendesigns May 25, 2016

Author Member

The idea was that if the main query is being filtered manually you could add those post types if needed, but probably don't need it.

This comment has been minimized.

Copy link
@westonruter

westonruter May 25, 2016

Contributor

OK, done in 9cdcb7c

westonruter added some commits May 25, 2016

Simplify get_previewed_posts_for_query by replacing customize_posts_m…
…ain_query_post_type filter with any post type lookup
Rename add_new_post to insert_auto_draft_post
Always return WP_Post or WP_Error, pass back to client in error message

westonruter added a commit to xwp/wordpress-develop that referenced this pull request May 25, 2016

valendesigns and others added some commits May 25, 2016

Update wp-plugin-dev-lib 046c3c5...4c79f77: Ensure symlink for node_m…
…odules gets created to fix ESLint checks

xwp/wp-dev-lib@046c3c5...4c79f77

d14fb0f Fix activation of plugins via WP_TEST_ACTIVATED_PLUGINS config
9a6eaba Merge pull request xwp/wp-dev-lib#176 from branch bugfix/test-activated-plugins
e34026e Ignore `node_modules` symlinks when installing `npm`.
6242f95 Merge pull request xwp/wp-dev-lib#177 from branch bugfix/ignore-node-modules-symlink
c05837c Define Paths * Define paths to the current WP install instead of using the wordpress-develop paths as per the tests bootstrap
ad82ec3 Define WP_CONTENT_DIR and WP_PLUGIN_DIR
4c79f77 Ensure symlink for node_modules gets created to fix ESLint checks

@westonruter westonruter merged commit 32e268d into develop May 25, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-1.8%) to 95.83%
Details

@westonruter westonruter deleted the feature/authoring branch May 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.