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

Ensure contributors can create tags and manage categories #6761

Merged
merged 11 commits into from
May 17, 2018

Conversation

danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented May 15, 2018

Description

Ensures contributors and authors can create tags and manage categories. More specifically, this pull request:

  • Incorporates REST API capability changes identified in this Trac ticket. Doing so required a series of amazing hacks.
  • Migrates taxonomy data to core-data.
  • Uses wp:action-* targetSchema to expose whether or not the current user can create or assign terms for each taxonomy.

See #6361
Fixes #5879

How has this been tested?

When signed in as a contributor, you should be able to create tags and assign categories:

image

When signed in as an editor, you should be able to also create new categories:

image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Ports over capability changes mentioned in Trac ticket.
@danielbachhuber danielbachhuber added this to the 2.9 milestone May 15, 2018
@danielbachhuber danielbachhuber changed the title [WIP] Ensure contributors can create tags and manage categories Ensure contributors can create tags and manage categories May 15, 2018
@danielbachhuber danielbachhuber requested a review from a team May 15, 2018 23:59
@danielbachhuber danielbachhuber requested review from tofumatt and pento May 16, 2018 00:10
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

I can see how it was an interesting challenge to implement the PHP side of things: I left one comment, but I'm generally fine with it being super hacky in Gutenberg, as the actual core changes are fairly straightforward.

I'm not sure how I feel about all the additional processing happening in PostTaxonomies now. Maybe it'll look at little less like a wall of text once #6655 is fixed.

export function receiveTaxonomies( taxonomies ) {
return {
type: 'RECEIVE_TAXONOMIES',
taxonomies: taxonomies,
Copy link
Member

Choose a reason for hiding this comment

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

This could just be taxonomies, instead of taxonomies: taxonomies.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -174,6 +174,10 @@ class FlatTermSelector extends Component {
const termRemovedLabel = sprintf( _x( '%s removed', 'term' ), singularName );
const removeTermLabel = sprintf( _x( 'Remove %s', 'term' ), singularName );

if ( ! hasAssignAction ) {
Copy link
Member

Choose a reason for hiding this comment

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

It's a tiny micro-optimisation, but this check could happen immediately after hasAssignAction is declared, to avoid the processing needed to declare all the other local variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -242,10 +242,14 @@ class HierarchicalTermSelector extends Component {
const newTermSubmitLabel = newTermButtonLabel;
const inputId = `editor-post-taxonomies__hierarchical-terms-input-${ instanceId }`;

if ( ! hasAssignAction ) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as in flat-term-selector.js, this check could happen earlier.

Copy link
Member Author

Choose a reason for hiding this comment

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

lib/rest-api.php Outdated
&& is_array( $handler['permission_callback'] )
&& is_a( $handler['permission_callback'][0], 'WP_REST_Terms_Controller' ) ) {
// 'taxonomy' is a protected attribute, and PHP 5.2 doesn't support
// ReflectionProperty->setAccessible(), so we need to get really dirty.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not going to lie, I'm impressed with this hack.

But, can't you get the value of ::taxonomy from the data returned by ::get_item_schema()?

Copy link
Member Author

Choose a reason for hiding this comment

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

But, can't you get the value of ::taxonomy from the data returned by ::get_item_schema()?

You can, in fact. I'd forgotten about that. 0c9c329

}

return state;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a new reducer/selectors/actions, we could just leverage the new entities abstraction to automatically generate those. https://github.com/WordPress/gutenberg/blob/master/core-data/entities.js#L6

Currently, the entities only support fetching by id getEntityRecord but we should definitely introduce a findAll/findQuery as well getEntityRecords( state, kind, entity, query )

If you think it's too much work, it's fine to do it separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to handle separately (and also include more tests within core-data). I've filed an issue #6780

const visibleTaxonomies = filter( availableTaxonomies, ( taxonomy ) => taxonomy.visibility.show_ui );
return visibleTaxonomies.map( ( taxonomy ) => {
const TaxonomyComponent = taxonomy.hierarchical ? HierarchicalTermSelector : FlatTermSelector;
const restBase = taxonomy.rest_base;
const hasCreateAction = get( post, [ '_links', 'wp:action-create-' + restBase ], false );
const hasAssignAction = get( post, [ '_links', 'wp:action-assign-' + restBase ], false );
Copy link
Contributor

@youknowriad youknowriad May 16, 2018

Choose a reason for hiding this comment

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

Can we move these two variables inside withSelect instead?. Boolean change less often than a objects which avoid rerenders.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I guess it's per taxonomy, This is for me an indication that these checks should be moved to the TaxonomyComponent instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved in 04aa6e5

@youknowriad youknowriad modified the milestones: 2.9, 3.0 May 16, 2018
@danielbachhuber
Copy link
Member Author

@youknowriad @pento Good to go now.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM for the JS part of things. I'll defer to @pento for the endpoint updates.

@tofumatt
Copy link
Member

FYI I'm away on holiday for the next week or so; feel free to un-assign me for review and have someone else take a look. Otherwise I can have a look come May 28-ish...

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

Noice. Let's do it.

@pento pento modified the milestones: 3.0, 2.9 May 17, 2018
@pento pento merged commit 304302f into master May 17, 2018
@pento pento deleted the 5879-term-capabilities branch May 17, 2018 07:29
return {
hasCreateAction: get( getCurrentPost(), [ '_links', 'wp:action-create-' + ownProps.restBase ], false ),
Copy link
Member

Choose a reason for hiding this comment

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

Should this logic be its own selector? Something like canUserForPost( actionType, restBase ) ?

Minor: getCurrentPost as implemented is trivial, though nothing guarantees it is, in which case assigning a single variable of const currentPost = getCurrentPost() to use across the two merged props would be marginally more performant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this logic be its own selector? Something like canUserForPost( actionType, restBase ) ?

See #6655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants