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

Use the MediaUploadCheck component before each Upload component #11924

Merged
merged 2 commits into from Nov 17, 2018

Conversation

Projects
None yet
2 participants
@imath
Contributor

imath commented Nov 15, 2018

This PR should fix #11910

Description

  1. It adds a MediaUploadCheck component before the forgotten MediaUpload components used directly in some blocks :
  • within the edit toolbar buttons of the cover, file and image blocks,
  • within the Poster image setting of the video block.
  1. It also adds a MediaUploadCheck component before rendering the Block Drop Zone.

How has this been tested?

Using WordPress trunk & Gutenberg Master I've visually checked the bugs listed into the #11910 issue were fixed for the contributor role (user without the upload_files capability). I also ran successfully the unit tests suite.

Types of changes

Improves #4155 by making sure the forgotten Upload components are used after the MediaUploadCheck one.

Checklist:

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

@gziolo gziolo self-requested a review Nov 15, 2018

@gziolo gziolo added the [Type] Bug label Nov 15, 2018

@gziolo gziolo added this to the 4.5 milestone Nov 15, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 16, 2018

It tests well. Great work on fixing all the remaining issues.

We discussed it on Slack, but I wanted to double check if you still think it is an option to include MediaUploadCheck inside MediaUpload component to ensure that it is always disabled when a user doesn't have proper capabilities? I really like how it is done for DropZone and MediaPlaceholder.

@gziolo

gziolo approved these changes Nov 16, 2018

imath added some commits Nov 15, 2018

Use the MediaUploadCheck component before each Upload component
This PR should fix #11910

1. It adds a MediaUploadCheck component before the forgotten MediaUpload components used directly in some blocks :
- within the edit toolbar buttons of the cover, file and image blocks,
- within the Poster image setting of the video block.
2. It also adds a MediaUploadCheck component before rendering the Block Drop Zone.
Edit the MediaUpload documentation about Upload Permissions
Include some informations about how to make sure the current user has upload permissions (by wrapping the MediaUpload component into the MediaUploadCheck one)

@imath imath force-pushed the imath:improve/upload_files-capability-checks branch from 01674f4 to 423e9b3 Nov 17, 2018

@imath

This comment has been minimized.

Contributor

imath commented Nov 17, 2018

@gziolo I've been looking into it and came to the conclusion that it's fine the way it is actually 😃 . Into the MediaPlaceHolder we have the MediaUpload component but we also have the DropZone and the FormFileUpload components. So I've tried to do the same UploadPermissions check into these components but never succeeded : it looks like the upload permissions are not set yet.

So I thought, as we need to wrap the DropZone, FormFileUpload and the MediaUpload components into a MediaUploadCheck one anyway, doing an extra check into MediaUpload was weird.

@gziolo gziolo merged commit 88b5033 into WordPress:master Nov 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gziolo

This comment has been minimized.

Member

gziolo commented Nov 17, 2018

Let's go with the current approach and see how it work. There are so many different use cases where the check for upload permissions is necessary that it's hard to find a perfect approach. We can always iterate on it. Thanks for fixing all remaining bugs I could find.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Use the MediaUploadCheck component before each Upload component (Word…
…Press#11924)

* Use the MediaUploadCheck component before each Upload component

This PR should fix WordPress#11910

1. It adds a MediaUploadCheck component before the forgotten MediaUpload components used directly in some blocks :
- within the edit toolbar buttons of the cover, file and image blocks,
- within the Poster image setting of the video block.
2. It also adds a MediaUploadCheck component before rendering the Block Drop Zone.

* Edit the MediaUpload documentation about Upload Permissions

Include some informations about how to make sure the current user has upload permissions (by wrapping the MediaUpload component into the MediaUploadCheck one)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment