Enforce the file type of the featured image, background media, and video block by kienstra · Pull Request #2737 · ampproject/amp-wp

kienstra · 2019-07-03T02:05:17Z

If the selected featured image, like a .mov file, is the wrong type, it displays a notice and disables the 'Select' button.
Applies to the featured image in the normal post block editor and the AMP Story editor. Though maybe it should only apply to the AMP Story editor.
Uses the allowedTypes property of the MediaUpload component.

Here's the notice:

This doesn't disable the 'Select' button now, though it shows a notice both in the AMP Story editor and the normal post or page block editor.

Before, this simply displayed a notice. Now, it actually disables the button.

…tion I didn't write this originally, but I used this same logic for the file type notice. So instead of simply copy and pasting it, this extracts it into a separate function.

westonruter · 2019-07-03T03:32:49Z

Does this also apply to the video block? Users should be disallowed from selecting or uploading anything but MP4 videos.

kienstra · 2019-07-03T03:35:34Z

Does this also apply to the video block? Users should be disallowed from selecting or uploading anything but MP4 videos.

Hi @westonruter, no, it doesn't apply to the video block or to the 'Background Media' control. I still need to work on that.

Refactor much of the existing logic, which was for featured images. Rename a file and many functions.

There was duplicated logic, so use that function in place of the copy-pasted code.

It looks like that wasn't preventing .mov files from appearing. Also, the new logic uses ALLOWED_MEDIA_TYPES as passed in allowedTypes. It checks that the attachment.get( 'type' ) matches that, and the attachment.get( 'type' ) is never 'video/mp4'. That is only the attachment.get( 'mime' ).

There was a file that should have been moved, but I forgot to delete it.

There an error: fatal: Path 'amp.php' exists on disk, but not in 'eb9bb06b77d811ba80ca70ec60fbe551aea4ca73'. Pascal had suggested an empty commit for this.

It looks like this needed a 'select' handler. So add one here.

assets/src/common/helpers/index.js

kienstra · 2019-07-04T07:52:17Z

Mainly Working
I Still Need To Test More

This is mainly working for the 'Background Media,' Core Video block, and the featured image.

If any of them have the wrong mime type, like 'video/quicktime' for a .mov file, it displays a notice and disables the 'Select' button:

The same applies for uploading other incorrect types, like .txt files.

I still need to test this more, and I'm working on a unit test for enforceFileType().

Add a mockClasses.js file to store some mocks. And use this for the tests in isFileTypeAllowed.js.

assets/src/common/components/select-media-frame.js

assets/src/common/components/with-enforced-file-type.js

kienstra · 2019-07-06T01:15:41Z

The first 3 scenarios are new in this PR, the last 2 are to test regressions.

Scenario	Expected behavior
1. AMP Story and normal post editors: upload as featured image a `.txt` or `.mov` file	'Select' button disabled, notice appears: "The selected file mime type ... is not allowed."
2. AMP Story Background Media: upload a `.txt` or `.mov` file	Same as above
3. Core Video block only in AMP Story editor (not normal post editor): upload via the Media Library or 'Upload' button on block anything other than an `.mp4` video, like an `.mov`	Same as above
4. AMP Story editor: select or upload as featured image an image with dimensions smaller than`1200 x 1600`	Shouldn't change in this PR: should display the notice "The selected image is too small..."
5. AMP Story editor: select or upload as featured image an image with dimensions larger than `1200 x 1600`, like 1900 x 1900	Shouldn't change in this PR: this should still display the cropper

That seems to have caused .mov files to appear in the Background Media. So revert the change to that, and instead change isFileTypeAllowed() to return true whene a mime type matches a value in the allowedTypes array.

This PR already blocked selecting uploading them. But if they were uploaded before, they would still display in the 'Media Library' tab of the Media Library. So this blocks displaying them in that tab if they were uploaded before.

This caused a failed Travis build, as it was imported but not used.

Before, some of these were joined using a comma. Instead, keep to the convention in the rest of this PR.

kienstra · 2019-07-06T02:54:09Z

Request For Review

Hi @swissspidy and @westonruter,
Could you please review this when you can?

In the first 3 scenarios above, this displays a notice in the Media Library and disables the 'Select' button:

Thanks, and have a great weekend!

There shouldn't have been a period in the middle of the sentence.

westonruter

Attempting to select a MOV video to upload for page background successfully results in the Select button being disabled and a warning message displayed:

Same thing happened successfully when attempting to upload a video via the Media Library button in the video block:

Nevertheless, it does not successfully restrict the video type when selecting the Upload button from the video block:

assets/src/common/components/with-enforced-file-type.js

westonruter · 2019-07-08T03:54:39Z

Is applying the restriction to the Video block's Upload button too difficult to include here?

kienstra · 2019-07-08T05:33:42Z

Video Block Upload Button

Hi @westonruter,

Is applying the restriction to the Video block's Upload button too difficult to include here?

Good question. I'll look at that tomorrow if that's alright.

It looks like if the accept prop were video/mp4 instead of video/*, that might restrict uploading to mp4 files.

Maybe there's a way to change that with the editor.BlockEdit filter.

…mp4' files There was already logic to prevent uploading in the Media Library. But this applies to the 'Upload' button in the block's Edit component, not the Media Library.

kienstra · 2019-07-09T00:27:13Z

Video Upload Button

Hi @westonruter,

Is applying the restriction to the Video block's Upload button too difficult to include here?

Sure, a72b3e5 restricts the upload type of the Video block to `video/mp4':

Sorry for the delay.

westonruter

It works! I'll let @swissspidy do a final review of the JS prior to merging.

kienstra · 2019-07-09T22:08:20Z

Thanks, @westonruter!

kienstra · 2019-07-12T03:40:09Z

Request For Review

Hi @swissspidy,
Could you please review this?

Here's the expected behavior.

Thanks, and hope you have a great weekend.

kienstra added 3 commits July 2, 2019 19:51

If the wrong file type is uploaded for a featured image, display notice

22bdba4

This doesn't disable the 'Select' button now, though it shows a notice both in the AMP Story editor and the normal post or page block editor.

Disable the 'Select' button if the wrong file size is selected

0750718

Before, this simply displayed a notice. Now, it actually disables the button.

Extract the existing logic for creating a notice template into a func…

befaae0

…tion I didn't write this originally, but I used this same logic for the file type notice. So instead of simply copy and pasting it, this extracts it into a separate function.

googlebot added the cla: yes Signed the Google CLA label Jul 3, 2019

kienstra changed the title ~~Enforce the file type of the featured image~~ [WIP] Enforce the file type of the featured image Jul 3, 2019

kienstra added 6 commits July 3, 2019 19:12

Also display a notice for the Background Media and Video blocks

56d17ea

Refactor much of the existing logic, which was for featured images. Rename a file and many functions.

Move enforceFileType() into helpers, as it'll be used in 2 places

8e88e65

There was duplicated logic, so use that function in place of the copy-pasted code.

Remove duplicate file, add export keyword

bfdb184

There was a file that should have been moved, but I forgot to delete it.

Empty commit to addres failed Travis build

3b261f4

There an error: fatal: Path 'amp.php' exists on disk, but not in 'eb9bb06b77d811ba80ca70ec60fbe551aea4ca73'. Pascal had suggested an empty commit for this.

Fix an issue where you couldn't select an image as the Background Media

5e82181

It looks like this needed a 'select' handler. So add one here.

kienstra commented Jul 4, 2019

View reviewed changes

assets/src/common/helpers/index.js Show resolved Hide resolved

kienstra added 2 commits July 5, 2019 18:37

Add unit tests for enforceFileType()

cabf888

Add a mockClasses.js file to store some mocks. And use this for the tests in isFileTypeAllowed.js.

Merge branch 'develop' into fix/upload-file-type

477bd5c

kienstra commented Jul 6, 2019

View reviewed changes

assets/src/common/components/select-media-frame.js Show resolved Hide resolved

kienstra commented Jul 6, 2019

View reviewed changes

assets/src/common/components/with-enforced-file-type.js Show resolved Hide resolved

This comment has been minimized.

Sign in to view

kienstra added 4 commits July 5, 2019 21:01

Revert change to ALLOWED_MEDIA_TYPES, as that caused a regression

5390f66

That seems to have caused .mov files to appear in the Background Media. So revert the change to that, and instead change isFileTypeAllowed() to return true whene a mime type matches a value in the allowedTypes array.

Remove unused withEnforcedFileType

8ca6465

This caused a failed Travis build, as it was imported but not used.

Use const at the beginning of each const declaration

0d37e84

Before, some of these were joined using a comma. Instead, keep to the convention in the rest of this PR.

kienstra requested review from swissspidy and westonruter July 6, 2019 02:54

kienstra changed the title ~~[WIP] Enforce the file type of the featured image~~ Enforce the file type of the featured image Jul 6, 2019

kienstra changed the title ~~Enforce the file type of the featured image~~ Enforce the file type of the featured image, background media, and video block Jul 6, 2019

In a comment, change a . to a ,

a7d957e

There shouldn't have been a period in the middle of the sentence.

westonruter reviewed Jul 8, 2019

View reviewed changes

assets/src/common/components/with-enforced-file-type.js Show resolved Hide resolved

kienstra mentioned this pull request Jul 8, 2019

Add video block size validation in the Media Library #2755

Merged

kienstra added 2 commits July 8, 2019 19:12

For the Video block in the Story editor, only allow uploading 'video/…

a72b3e5

…mp4' files There was already logic to prevent uploading in the Media Library. But this applies to the 'Upload' button in the block's Edit component, not the Media Library.

Merge branch 'develop' into fix/upload-file-type

e821db1

westonruter added this to the v1.2.1 milestone Jul 9, 2019

westonruter approved these changes Jul 9, 2019

View reviewed changes

Merge branch 'develop' into fix/upload-file-type

96bacc7

kienstra assigned swissspidy Jul 12, 2019

swissspidy approved these changes Jul 13, 2019

View reviewed changes

swissspidy merged commit 8720098 into develop Jul 13, 2019

swissspidy deleted the fix/upload-file-type branch July 13, 2019 14:00

kienstra mentioned this pull request Jul 13, 2019

Video upload does not work #2727

Closed

westonruter added the AMP Stories label Jul 18, 2019

Conversation

kienstra commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Jul 3, 2019

Uh oh!

kienstra commented Jul 3, 2019

Uh oh!

Uh oh!

kienstra commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kienstra commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

kienstra commented Jul 6, 2019

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

westonruter commented Jul 8, 2019

Uh oh!

kienstra commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Jul 9, 2019

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

kienstra commented Jul 9, 2019

Uh oh!

kienstra commented Jul 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kienstra commented Jul 3, 2019 •

edited

Loading

kienstra commented Jul 4, 2019 •

edited

Loading

kienstra commented Jul 6, 2019 •

edited

Loading

kienstra commented Jul 8, 2019 •

edited

Loading