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

New authors dropdown breaks author selection for editors #26476

Closed
TimothyBJacobs opened this issue Oct 26, 2020 · 16 comments · Fixed by #26554
Closed

New authors dropdown breaks author selection for editors #26476

TimothyBJacobs opened this issue Oct 26, 2020 · 16 comments · Fixed by #26554
Assignees
Labels
REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@TimothyBJacobs
Copy link
Member

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):

  • WordPress version: trunk
  • Does the website has Gutenberg plugin installed, or is it using the block editor that comes by default? plugin
  • If the Gutenberg plugin is installed, which version is it? 9.2.1

Desktop (please complete the following information):

  • OS: MacOS
  • Browser Safari
  • Version 14

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

Cc: @adamsilverstein, @youknowriad.

@TimothyBJacobs TimothyBJacobs added [Type] Bug An existing feature does not function as intended REST API Interaction Related to REST API [Type] Regression Related to a regression in the latest release labels Oct 26, 2020
@adamsilverstein adamsilverstein self-assigned this Oct 26, 2020
@adamsilverstein
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member

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

@adamsilverstein
Copy link
Member

@youknowriad - I started work on this in #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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

adamsilverstein commented Oct 29, 2020

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
Copy link
Member

Alternate draft PR with these additional changes in case that is easier to test: #26584

@adamsilverstein
Copy link
Member

adamsilverstein commented Oct 29, 2020

@youknowriad & @TimothyBJacobs - #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
Copy link
Member

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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants