WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.46k stars 4.18k forks source link

New authors dropdown breaks author selection for editors #26476

Closed TimothyBJacobs closed 3 years ago

TimothyBJacobs commented 3 years ago

Describe the bug

23237 stopped using getAuthors and instead uses getUsers. getUsers automatically applies a context=edit which requires that the current user have the list_users capability. This capability only exists for administrators.

To reproduce

  1. Create a new user with the "Editor" role.
  2. Create a new post.
  3. Observe the authors dropdown is missing.

Expected behavior To be able to see the list of users.

Screenshots image

Editor version (please complete the following information):

Desktop (please complete the following information):


This dropdown really should only be loading users in the embed context if all it needs are their names and IDs.

Cc: @adamsilverstein, @youknowriad.

adamsilverstein commented 3 years ago

I'm not sure what the best approach to fixing this is.

We can change back to getAuthors - in that case we would also need to extend it to get a single author by id and search authors - both features are not currently part of getAuthors. or...

We can adjust our resolvers (getEntityRecords and getEntityRecord) so they use only add context: 'edit' for endpoints that requires it?

What do you think @youknowriad?

TimothyBJacobs commented 3 years ago

IMO getEntityRecord(s) should allow for passing your own context value, right now it forcibly overwrites it. The data layer should store which context the entity was requested in. If someone then requests a larger context, then the data layer would refetch.

In other words...

// Triggers a query.
getEntityRecords( 'postType', 'post', { context: 'view' } );

// Uses cached values.
getEntityRecords( 'postType', 'post', { context: 'embed' } );

// Makes a new query
getEntityRecords( 'postType', 'post', { context: 'edit' } ); // Or getEntityRecords( 'postType', 'post' );
youknowriad commented 3 years ago

I think I shared this several times already, passing "context" like suggested above is not a good approach and it will mess up the cache of the data layer because the format is different when you change the context so you'll receive the wrong value.

In my ideal world, we need a "all" context where all fields available to the user are returned. I know this is controvertial for you so the second alternative is to fetch the schema for endpoints and depending on the request "fields" switch the context dynamically. (This is not that easy to implement)

Now, to fix this issue quickly we should just restore getAuthors for the moment and potentially have an __unstableGetAuthor too while we figure out the best approach above.

adamsilverstein commented 3 years ago

@youknowriad I will work on changing back to getAuthors and adding the support to search and get a single author.

adamsilverstein commented 3 years ago

@youknowriad - I started work on this in https://github.com/WordPress/gutenberg/pull/26554. one thing I noticed when creating __unstableGetAuthor is that the REST API seems to require the edit context to get a single user? is this correct @TimothyBJacobs? eg. the route wp-json/wp/v2/users/[ID] requires the edit context. So this bit doesn't actually work for editors as far as I can tell.

TimothyBJacobs commented 3 years ago

It should accept a view context as long as the user has authored posts in a REST API viewable post type. Is it returning an error for all users?

adamsilverstein commented 3 years ago

It is returning an error for my test editor user. This user is brand new and has no posts. Why is "as long as the user has authored posts in a REST API viewable post type" a prerequisite?

TimothyBJacobs commented 3 years ago

We don't want to expose users who haven't authored posts because their information wouldn't otherwise have been public pre-REST API.

adamsilverstein commented 3 years ago

We don't want to expose users who haven't authored posts because their information wouldn't otherwise have been public pre-REST API.

What about if they have a drafted post, not published?

In this case the user is an editor and is selecting that author and about to publish a post for them.

I'll think about how we can work around this.

TimothyBJacobs commented 3 years ago

In this case the user is an editor and is selecting that author and about to publish a post for them.

I don't think that was possible in Gutenberg previously, so we don't need to solve it in this issue IMO.

adamsilverstein commented 3 years ago

I don't think that was possible in Gutenberg previously, so we don't need to solve it in this issue IMO.

The problem is the current approach relied on this working. When editing a post where the author is not among the initial set of authors returned from the API, we make a secondary request for that author specifically to get the name for display. We can possibly move that logic to the PHP side for now?

TimothyBJacobs commented 3 years ago

Sorry, I'm probably missing something obvious, but I'm struggling to understand. If the author is selectable, why wouldn't they be included?

adamsilverstein commented 3 years ago

Let's say your site has 5,000 users. The post you are editing is authored by user with ID 500 and the dropdown should show that user name in the author selector - the current author.

The post REST response includes the author id (not their name). The initial REST API response for the "Authors" returns the first 100 of these users. The user with ID 500 is not among these users. So the problem is - how do we know this users name? The current approach makes a secondary request for this user to ensure we have their name. This breaks when the user is an editor.

I have explored an alternate approach in this diff. Using a filter, we attach the author name to the REST API response, then use that in the component instead of making the secondary request.

In my testing this resolves the issue for Editors. This may not be the best long term solution, thinking more as a temporary measure until we work out the underlying context issues.

What do you think? cc: @youknowriad

adamsilverstein commented 3 years ago

Alternate draft PR with these additional changes in case that is easier to test: https://github.com/WordPress/gutenberg/pull/26584

adamsilverstein commented 3 years ago

@youknowriad & @TimothyBJacobs - https://github.com/WordPress/gutenberg/issues/26476 is ready for review. To test, please try as an editor with 100's of users, and make sure you select a user > 100, save and refresh.

The final fix as @TimothyBJacobs suggested was to adjust the single user query to use /users?who=authors&include=<id>.

skorasaurus commented 3 years ago

For what it's worth, I have an additional use case that led me to this issue:

We have a plugin at CPL that creates a custom post type (with custom content preloaded) and the users who publish and edit those posts do not have the capability (neither edit_users nor list_users) to change the post's author.

We want to minimize any confusion or work by automatically assigning the author to them (the post author) and not displaying the post author component. We successfully did that in the past by omitting 'author' from the add_post_type_support function.

But with the introduction of https://github.com/WordPress/gutenberg/pull/23237 (was merged into Gutenberg 9.2); the editor failed to load when a user without the list_users capability (and no error appeared in the console).

With this PR #26554, a 403 error appears in the console - XHRGEThttps://local.wordpress.test/wp-json/wp/v2/users/?who=authors&per_page=100&_locale=user [HTTP/2 403 Forbidden 184ms] ; but the editor now successfully loads and the user can edit the post.