Patterns: fix capabilities settings for pattern categories #55379
Conversation
glendaviesnz
added
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.4/class-gutenberg-rest-blocks-controller-6-4.php ❔ lib/compat/wordpress-6.4/class-gutenberg-rest-pattern-categories-controller.php ❔ lib/compat/wordpress-6.4/block-patterns.php ❔ lib/compat/wordpress-6.4/blocks.php ❔ lib/load.php |
|
One question with this is whether Authors should be allowed to add new categories to their patterns, which is the case with the changes in this PR, or should we be making it that Authors can only select from existing categories? |
|
Size Change: +3.16 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2311222. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6584404650
|
|
This PR is working well for me. As an admin I can still manage all categories, but as an author I can only add/manage my own terms.
Good question. I can envision a world where site owners might want to restrict category creation. Still, authors can already create patterns in the post editor so it's not a great leap to also grant the capability to manage the terms of those patterns. However I see that adding new post categories is restricted for authors:
Tags are allowed. So maybe the question is: do we view pattern taxonomy as "categories", with hierarchies and so on, or rather "tags"? Out of interest, it looks like these might be the default taxonomy capabilities. If we wanted to restrict pattern taxonomy, would they suffice? https://github.com/WordPress/wordpress-develop/blob/991fa6bff23590fe71e9f2807cfcdb218d661c95/src/wp-includes/class-wp-taxonomy.php#L430 Or the same caps as post categories: 'capabilities' => array(
'manage_terms' => 'manage_categories',
'edit_terms' => 'edit_categories',
'delete_terms' => 'delete_categories',
'assign_terms' => 'assign_categories',
),I'd say, if we wanted to prevent authors and contributors from managing pattern taxonomy then we might also want to update the form logic so they only search and select. 
|
I'd lean towards matching the post categories capabilities as well. Which then would require updating the category selector in the create pattern modal as suggested.
That sounds like the best starting point to me. We can extend capabilities later. It would also make it easier to handle the wrinkle where the "core categories" are merged as suggestions for the category selector. We can simply omit merging core categories into the suggestions if the user doesn't have the required capabilities. |
|
The PR has now been updated to restrict adding of new categories to users with This currently means that if site editors/admins want Author users to be able to assign core pattern categories to their patterns they will need to manually add the core pattern categories to their site's |
There was a problem hiding this comment.
Nice work wrangling the fix for this @glendaviesnz 👍
It tests well as per the test instructions although it looks like we still need to update other places where authors could assign categories e.g. when editing a pattern via wp-admin wp-admin/edit.php?post_type=wp_block
In addition, it would be good if we could remove the Add new tag label there so that it matches the control when editing a pattern in the site editor for admin users.
We might also need some design feedback on the multi-select control in the create pattern modal. To me, it looks a bit rough but given the potential number of pattern categories I'm not sure if there is a better option.
lib/compat/wordpress-6.4/class-gutenberg-rest-pattern-categories-controller.php
Show resolved
Hide resolved
|
This is testing well according to the instructions, thanks! Using an author account in trunk I managed, it seems, to in fact create a pattern category which, when switching to this branch persisted: 
In the editor it looks like creating a new category as an author doesn't work, but it saves anyway. So when switching to this branch they're there. Not a huge deal I suppose since the editor doesn't give any expectation to the user that the category is "created" (even though it is). While testing I also found something else that we might like to address in a follow up - despite having no permissions in the backend, authors can edit patterns created by, for example, an admin in the editor. This leads to a case where saving is blocked in the editor with no advice on how to get around it. The "fix" is to revert all changes, or refresh the page. This might lead to content loss however. 2023-10-18.12.37.29.mp4 |
This is an outstanding issue and has been there since reusable block days - an issue for it here |
Yep that is correct, on trunk authors are able to add categories because pattern categories is a flat taxonomy and they have permission to add those, which this PR hopefully fixes. |
glendaviesnz
force-pushed
the
fix/pattern-categories-capabilities
branch
from
4281c9e to
c433e5c
Compare
Ah, thanks for pointing that out, sorry for the noise.
👍🏻 All good. I think the transition, even if there are folks out there relying on it, won't be so jarring. Any categories created by an author will be preserved should the site ever update. |
glendaviesnz
changed the title
|
I think this is closer now. It now shows a list of checkboxes instead of multiselect, and this component is reused by the post editor taxonomy panel just for wp_pattern_category when user does not have create capability. |
…tegories to their patterns. Also set API call to `view` context so contributors can see user pattern categories
…permission requirements
…t show current available categories
|
I would like to tidy up some of the language used, eg. |
There was a problem hiding this comment.
✅ Log in as a Contributer user and make sure the new categories appear in the editor inserter Patterns tab
✅ (just got a log in screen, but that's fine) Check that going to /wp-admin/edit-tags.php?taxonomy=wp_pattern_category gives an insufficient permissions error.
✅ Log in as an Author and make sure the new categories appear in the editor inserter Patterns tab
✅ Add a new pattern and make sure you can only select from the custom categories added above and that you can't add any new categories, and check pattern saves correctly and is available in the assigned categories
✅ Go to wp-admin/edit.php?post_type=wp_block and choose a pattern to edit, and make sure you can only select and not create categories in the right panel
✅ (just got a log in screen, but that's fine) Check that going to /wp-admin/edit-tags.php?taxonomy=wp_pattern_category gives an insufficient permissions error
✅ Login as Author and add a new pattern and check that the category option is not shown because there are now no categories to select from
✅ Login as an Editor and add a new pattern and make sure you can assign new categories to a pattern as well as select existing ones
All worked exactly as expected with all the different user roles.
There was a problem hiding this comment.
Working as described for each of the roles.
Thanks for sticking with this one @glendaviesnz
| // Patterns categories are a flat hierarchy (like tags), but work more like post categories in terms of permissions. | ||
| if ( ! current_user_can( $taxonomy_obj->cap->edit_terms ) ) { | ||
| return new WP_Error( | ||
| 'rest_cannot_create', |
There was a problem hiding this comment.
Not for this PR, but I think when syncing with Core it'd be good have some unit tests, e.g., something like
There was a problem hiding this comment.
for sure, I added one on the backport PR
| const unescapeString = ( arg ) => { | ||
| return decodeEntities( arg ); | ||
| }; |
There was a problem hiding this comment.
| const unescapeString = ( arg ) => { | |
| return decodeEntities( arg ); | |
| }; | |
| const unescapeString = ( arg ) => decodeEntities( arg ); |
Totally optional. Just a style thing
There was a problem hiding this comment.
ah, yeh, thanks, I think that was a copy pasta that wasn't tidied up, done now.
|
With cherry picking of this for the 6.4 rc2 release there will be a minor conflict caused by the inclusion of |
|
@SiobhyB - just hold of cherry picking this one until I get back to you - there are some questions about whether the approach taken here should be going into core. |
Co-authored-by: Daniel Richards <[email protected]>
|
@glendaviesnz, ah, whoops, I cherry-picked this change in 6f83c92, at around the same time as your message came in. 🙈 I'd like to wait for the tests to run to verify they all pass with the changes, but will revert straight after that. I'll follow along for when it's possible to cherry-pick again! |
|
I confirmed all the tests passed with the cherry-picked changes and went ahead to revert the cherry-pick in a783961. Let me know when it's ready for inclusion again! |
* Focus submenu button when clicked (#55198) * Focus element manually when open submenu on click * Try using `tabindex="-1"` * Use `tabindex="-1"` also in body when a submenu is opened * Replace tabindex with event listener * Explain the tabindex on <li> * Don't store the element on hover to restore the focus later * Improve explanations * Add tests to cover webkit frontend menu interactions Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari. * Focus clicked button on Safari Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu. * Added the document.addEventListener body click back in Authored by Luis Herranz <[email protected]>. I'm just re-applying the change. * Remove tab keypresses from webkit menu interaction tests Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress. * Use body click instead for consistency across environments --------- Co-authored-by: Luis Herranz <[email protected]> Co-authored-by: Jerry Jones <[email protected]> * Make layout support compatible with enhanced pagination (#55416) * Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name * Update default fullscreen icon for lightbox trigger (#55463) * Make duotone compatible with enhanced pagination (#55415) * Patterns: fix capabilities settings for pattern categories (#55379) Co-authored-by: Daniel Richards <[email protected]> * Revert "Patterns: fix capabilities settings for pattern categories (#55379)" This reverts commit 6f83c92. * Image block: wrap images with hrefs in an A tag (#55470) * This commit sees what happens when we wrap the image element in an A tag in the editor. This is to ensure any inherited styles from the link element, such as border color, apply to the image. * Removing duplicate <a href /> wrapper Adding disabled onClick and aria attribute * Image: Improve focus management in lightbox (#55428) * Improve focus management This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes. * Add delay before focusing when closing lightbox * Put focus back on close button when opening lightbox It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox. * Move focus to the dialog when opening the lightbox. * Fix SVG markup. * Consistent indentation with spaces. * Remove unnecessary tabindex --------- Co-authored-by: Andrea Fercia <[email protected]> * Fix: - Update the title when using enhanced pagination --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Luis Herranz <[email protected]> Co-authored-by: Jerry Jones <[email protected]> Co-authored-by: Artemio Morales <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Andrea Fercia <[email protected]>
glendaviesnz
removed
the
Backport to WP 6.7 Beta/RC
|
@SiobhyB A decision has been made to revert this change in Gutenberg so it will not be included in 6.4 either so you can cross it off your cherry pick list. Thanks. |
|
Thanks for keeping me in the loop @glendaviesnz 🙌 |
What?
Fixes the capabilities settings for pattern categories so Authors and below can view existing user categories in the editor inserter patterns tab. Also prevents Authors from adding new categories, but still provides the ability for them to assign existing categories to their own patterns.
Why?
Currently Authors and contributors get API errors when trying to view pattern categories, and Authors are able to add as well as assign pattern categories
How?
viewso contributors can see the user categories listwp_pattern_categoryTesting Instructions
wp-admin/edit-tags.php?taxonomy=wp_pattern_categoryand remove any categories/wp-admin/edit-tags.php?taxonomy=wp_pattern_categorygives an insufficient permissions error.wp-admin/edit.php?post_type=wp_blockand choose a pattern to edit, and make sure you can only select and not create categories in the right panel/wp-admin/edit-tags.php?taxonomy=wp_pattern_categorygives an insufficient permissions errorScreenshots or screencast
Before:
author-pattern-caps-before.mp4
After:
pattern-caps-after-2.mp4