Editor: Display trash button only if user has delete capability

[aduth]

Fixes #14828

This pull request seeks to resolve an issue where the "Move to Trash" button is shown even if the user does not have the correct capabilities to be able to trash the post.

Implementation notes:

I followed the suggested implementation from @talldan at #14828 (comment) . I could not encounter the same issued described from the comment there with respect to a 403 (Forbidden) response. @talldan could you check to see if the issue is present here, and if so, elaborate on the instructions to reproduce it?

Testing Instructions:

Repeat Steps to Reproduce from #14828, verifying that the Move to Trash button is only shown when the user has capabilities to delete the post.

[aduth]

There appear to be some legitimate failures in the end-to-end suite, specifically for the template tests, in encountering a 404 error from one of its requests.

["Failed to load resource: the server responded with a status of 404 (Not Found)"]

I'll plan to take a look.

[talldan]

I think the tests have caught a problem related to the postType, see the comment below.

@talldan could you check to see if the issue is present here, and if so, elaborate on the instructions to reproduce it?

It looks like this only happens for drafts. My steps were:

1. Create a draft as an admin and take note of the post id
2. Login as a contributor and open up the editor
3. In the console run wp.data.select( 'core' ).canUser( 'delete', 'posts', postId ); using the postId from step 1.
4. Observe that the request returns a 403.

Here's my console output:

8 is a draft and 6 is published.

In real world usage this isn't much of an issue, a contributor can't load up one of these posts in the editor and the selector still returns a falsey.

[talldan]

Should the postType be hard-coded here? I'm seeing a 404 from the request that the resolver makes when trying to edit a reusable block. That might also be the cause of the test failures.

IIRC, the rest_base from the postType is the value that should be used.

[aduth]

Should the postType be hard-coded here? I'm seeing a 404 from the request that the resolver makes when trying to edit a reusable block. That might also be the cause of the test failures.

IIRC, the rest_base from the postType is the value that should be used.

Yep, I think you're correct on both accounts (shouldn't be hard-coded, should leverage rest_base).

[aduth]

I forget why I had never circled back here. Apologies for allowing your review to linger without response @talldan.

I don't know if you recall what you were expecting, but are you thinking the problem is in how (a) a network error is logged (might be out of our control and default Chrome behavior) and (b) the selector returns undefined and not exclusively either true or false (a boolean value)?

[aduth]

Looking at the implementation, the conditional rendering probably doesn't need to happen in PostTrash. It should be up to the render usage (notably, @wordpress/edit-post's implementation) to decide if and how to use the PostTrashCheck. Otherwise, it seems there's no point in having PostTrashCheck to begin with.

[talldan]

@aduth Struggling to remember all the details about this one, sorry!

I supposed that the canUser resolver doesn't specifically check for a 403 and return a false might be what I was thinking. But it does return falsey so not so much of an issue.

[aduth]

I supposed that the canUser resolver doesn't specifically check for a 403 and return a false might be what I was thinking. But it does return falsey so not so much of an issue.

I think false would be the expected return value. Whether it's being handled? Hard to tell at the moment. I see it does a try / catch on the request itself:

gutenberg/packages/core-data/src/resolvers.js

Lines 159 to 173 in 56ff721

	try {
	response = yield apiFetch( {
	path,
	// Ideally this would always be an OPTIONS request, but unfortunately there's
	// a bug in the REST API which causes the Allow header to not be sent on
	// OPTIONS requests to /posts/:id routes.
	// https://core.trac.wordpress.org/ticket/45753
	method: id ? 'GET' : 'OPTIONS',
	parse: false,
	} );
	} catch ( error ) {
	// Do nothing if our OPTIONS request comes back with an API error (4xx or
	// 5xx). The previously determined isAllowed value will remain in the store.
	return;
	}

But I'd want to confirm:

• If apiFetch actually throws based on status codes. I know this is a pain point with window.fetch, that non-success HTTP status codes are still treated as success for the promise itself (i.e. an error is not thrown, and it's up to the developer to confirm the status code)
• Whether, even if the request does not throw, if the remaining logic would be sufficient to accommodate this, even for a failed request.

[talldan]

This has been resolved by #23174