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

[BBT-123]: Add functionality to allow editors to manage multiple style variations #51

Merged
merged 12 commits into from
Feb 2, 2024

Conversation

Joe-Rouse
Copy link

@Joe-Rouse Joe-Rouse commented Jan 29, 2024

This PR branches off the work completed in #50 so that PR should be reviewed and merged before reviewing this PR.

Description

Completes BBT-123 - This PR adds the ability for an editor to manage multiple styles at once, and to select which one of those styles is 'active'. The UI is very basic here and will change in the future, but this PR just focuses on adding the basic functionality.

I've reused the endpoint from BBT-122, but changed it up slightly so it handles both GET & POST requests - a GET request allows us to retrieve all the style variations for the current theme, and a POST request sets the active style variation for the current theme.

Rather than grabbing the globalStylesId using the core __experimentalGetCurrentGlobalStylesId() function, I'm now making a request to our endpoint and finding the post that is published as there should only ever be 1 for the current theme.

Change Log

  • Renamed endpoint now it has more functionality
  • Added GET/POST request handling on the endpoint, with the GET containing the existing functionality and the POST handling setting the active style variation
  • Added a <select> control in the top bar which allows us to switch between variations
  • Added a new Activate button which sets the variation that's currently being edited as the active variation

Steps to test

Screenshots/Videos

Screen.Recording.2024-01-29.at.13.37.29.mov

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@Joe-Rouse Joe-Rouse self-assigned this Jan 29, 2024
@Joe-Rouse Joe-Rouse added the enhancement New feature or request label Jan 29, 2024
Comment on lines 194 to 205
foreach ( $posts as $post ) {
$post_status = 'draft';
if ( $post->ID === $global_styles_id ) {
$post_status = 'publish';
}
wp_update_post(
array(
'ID' => $post->ID,
'post_status' => $post_status,
)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Just flagging this as potentially slow, it will perform wp_update_post on all posts regardless of whether they are already draft and there could be many.

We expect (but can't assume) that there's only going to be 1 published post at a time we need to revert.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense - I've added an extra check at the beginning of the loop so any posts that aren't already published and aren't going to be published can be ignored.

if ( 'publish' !== $post->post_status && $post->ID !== $global_styles_id ) {
	continue;
}

Comment on lines 157 to 176
$posts = get_posts(
array(
'post_type' => 'wp_global_styles',
'post_status' => array( 'publish', 'draft' ),
'orderby' => 'date',
'order' => 'desc',
'numberposts' => -1,
'ignore_sticky_posts' => true,
'no_found_rows' => true,
'update_post_meta_cache' => false,
'update_post_term_cache' => false,
'tax_query' => array( // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query -- This could be a slow query, but it's necessary.
array(
'taxonomy' => 'wp_theme',
'field' => 'name',
'terms' => $theme,
),
),
)
);
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a potentially slow query we may want to avoid running it until necessary.

During POST requests we already have the post ID, so can check it exists and run appropriate checks first. It may also be better to split this function up to handle POST and GET logic separately.

Copy link
Author

Choose a reason for hiding this comment

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

Made a couple of changes related to your comment:

  1. I've split this function up so each request method calls a separate method in the class.
  2. I've moved the get_posts segment into its own utility function, so it can be called when needed by each method, and will likely come in handy in the future.
  3. I'm now checking if the $global_styles_id is a valid post before grabbing all the posts.

@Joe-Rouse Joe-Rouse changed the base branch from release/1.0.0 to main January 31, 2024 17:19
@Joe-Rouse Joe-Rouse changed the base branch from main to release/1.0.0 January 31, 2024 17:19
Comment on lines 169 to 171
if ( ! $global_styles_id || ! get_post_status( $global_styles_id ) ) {
return new WP_Error( 'invalid_global_styles_id', __( 'Invalid global styles ID', 'themer' ) );
}
Copy link
Member

Choose a reason for hiding this comment

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

We may need to do some additional checks here. Imagine you are accessing the REST API directly and can therefore pass any value to the global_styles_id. We should:

  • Check the post type is correct
  • Check the post is related to the active theme via taxonomy term

Copy link
Author

Choose a reason for hiding this comment

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

I've refactored this further now to include the checks you've mentioned. I've moved this logic into a validation_callback in the register_rest_route call, so this will all be checked before we even run the main endpoint logic.

Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

looks great, thanks @Joe-Rouse

@Joe-Rouse Joe-Rouse merged commit 61554b5 into release/1.0.0 Feb 2, 2024
1 check passed
@g-elwell g-elwell mentioned this pull request Jul 26, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants