Add video block size validation in the Media Library by kienstra · Pull Request #2755 · ampproject/amp-wp

kienstra · 2019-07-08T05:52:01Z

In the Media Library, on selecting a video with the wrong size, this displays a notice
Applies to the Media Library for the Video block and 'Background Media'
Doesn't disable the 'Select' button
Branched off Enforce the file type of the featured image, background media, and video block #2737, as that refactored the Media Library logic
If/when that PR is merged, I'll probably rebase this PR to develop and target this PR to develop

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.

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.

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

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.

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

These functions will be useful in displaying the notice for video size, and that logic is in commmon/. So move all of this to common/.

This applies to the Core Video block and the AMP Stories 'Background Media' component. Also, add unit tests for the helper methods.

The heading mentioned the file size, where it should have been type.

When there were 2 notices, like for the wrong video type and file size, the notices overlapped some of the media. So add a class to the media library if there are 2 notices. And add a style rule for that class.

kienstra · 2019-07-09T06:26:50Z

Steps To Test

Create a new story
For the 'Background Media' of a page, upload or select from the 'Media Library' tab a video with more than 1 MB per second, like this
Expected: the 'Select' button is not disabled, but there is a notice like:

A video size of less than 1 MB per second is recommended. The selected video is 10 MB per second.

Here's the notice:

Repeat steps 2-3, but for the Video block (using the 'Media Library' button)
Edge case: for the Video block, upload via the Media Library a .mov file with more than 1 MB per second, like this
Expected: there are two notices: one for the wrong file type and one for the wrong file size:

kienstra · 2019-07-09T06:48:07Z

This isn't quite ready for review, as it's still branched off the fix/upload-file-type branch. If and when that branch is merged into develop, I'll probably rebase this to develop and target that branch.

googlebot · 2019-07-11T20:55:34Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-11T20:57:10Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

swissspidy · 2019-07-16T09:29:22Z

Added a warning now when pasting a video URL:

miina · 2019-07-16T10:07:18Z

I'm seeing 0 at the place of the notice if there is no error (see below "Replace Image")

miina · 2019-07-16T10:16:55Z

Likely not the most common use case but looks like displaying the warning based on URL stops showing when I do the following:

Add a video from URL that exceeds the limits.
Click "Edit Video" icon.
Choose "Insert from URL".
Don't change anything, leave the same URL, submit.

miina · 2019-07-16T10:20:31Z

I guess the only place where the warning is missing then is when a previous video is changed via "Edit Video" and "Upload".

Actually, there is no need to change a previous video, just using "Upload" directly and choosing a video exceeding the limits won't show the warning since the Media Library doesn't open then.

swissspidy · 2019-07-16T11:56:08Z

Likely not the most common use case but looks like displaying the warning based on URL stops showing when I do the following

Cannot reproduce this.

I'm seeing 0 at the place of the notice if there is no error (see below "Replace Image")

Cannot reproduce either.

Edit: now I can see it 🤷‍♂

Since 4 is used for every test, maybe for readability we could use a general descriptive variable for the seconds

Made the tests much more readable now in 3ce5e76.

swissspidy · 2019-07-16T11:57:47Z

Actually, there is no need to change a previous video, just using "Upload" directly and choosing a video exceeding the limits won't show the warning since the Media Library doesn't open then.

I guess we can then just show the warning in the sidebar.

swissspidy · 2019-07-16T12:07:54Z

@miina Mind checking again?

miina · 2019-07-16T12:13:33Z

Awesome, the issues mentioned before are gone now!

One more oddity that I just saw, the same video shows different values in the Media Library vs in the sidebar.
Sidebar (8MB):

Media Library (10MB):

Thoughts?

assets/src/stories-editor/components/with-enforced-video-upload-type.js

swissspidy · 2019-07-16T12:50:37Z

One more oddity that I just saw, the same video shows different values in the Media Library vs in the sidebar.

Interesting. Will try to reproduce.

miina · 2019-07-16T12:53:03Z

Interesting. Will try to reproduce.

Note that I was using this video.

swissspidy · 2019-07-16T13:02:41Z

Looks like it was a rounding issue.

miina

🎉

kienstra added 18 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.

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.

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

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.

In a comment, change a . to a ,

a7d957e

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

Move helper functions to common, add unit tests

8a6b1ee

These functions will be useful in displaying the notice for video size, and that logic is in commmon/. So move all of this to common/.

Add a notice of the video file size in the Media Library

f35f560

This applies to the Core Video block and the AMP Stories 'Background Media' component. Also, add unit tests for the helper methods.

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

kienstra added 2 commits July 8, 2019 19:43

Remove extra empty line, improve DocBlock heading

e2133c7

The heading mentioned the file size, where it should have been type.

If there are 2 notices in the Media Library, prevent styling overlap

e2e81c0

When there were 2 notices, like for the wrong video type and file size, the notices overlapped some of the media. So add a class to the media library if there are 2 notices. And add a style rule for that class.

westonruter changed the base branch from fix/upload-file-type to develop July 11, 2019 20:55

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Jul 11, 2019

westonruter changed the base branch from develop to fix/upload-file-type July 11, 2019 20:55

westonruter force-pushed the add/video-block-size-validation branch from cf596ab to e2e81c0 Compare July 11, 2019 20:57

Add size warning when using video from URL

52e69cd

swissspidy added 2 commits July 16, 2019 13:54

Use parameterized test for isVideoSizeExcessive

3ce5e76

Simplify condition for notice

5acbcab

swissspidy added 2 commits July 16, 2019 14:07

Fix proptype

1dd9146

Simplify logic

e92419e

miina reviewed Jul 16, 2019

View reviewed changes

assets/src/stories-editor/components/with-enforced-video-upload-type.js Outdated Show resolved Hide resolved

Remove errant space

3327054

Merge branch 'develop' into add/video-block-size-validation

3127b7d

swissspidy added 4 commits July 16, 2019 15:01

Cast content-length to number

a56f28a

Round video duration for consistency

15c00a5

Clear state after changes

cba1587

Re-fetch content-length when src changes

d6e7d9d

swissspidy requested a review from miina July 16, 2019 13:04

miina approved these changes Jul 16, 2019

View reviewed changes

swissspidy merged commit 715ecb2 into develop Jul 16, 2019

swissspidy deleted the add/video-block-size-validation branch July 16, 2019 14:10

kienstra mentioned this pull request Jul 16, 2019

Display a notice if a video in the video block is over 1 MB per second #2560

Closed

swissspidy added this to the v1.2.1 milestone Jul 18, 2019

kienstra mentioned this pull request Aug 1, 2019

Add e2e tests for media size notices and featured image #2927

Merged

Conversation

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

googlebot commented Jul 11, 2019

Uh oh!

googlebot commented Jul 11, 2019

Uh oh!

swissspidy commented Jul 16, 2019

Uh oh!

miina commented Jul 16, 2019

Uh oh!

miina commented Jul 16, 2019

Uh oh!

miina commented Jul 16, 2019

Uh oh!

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

Uh oh!

Uh oh!

swissspidy commented Jul 16, 2019

Uh oh!

swissspidy commented Jul 16, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

swissspidy commented Jul 16, 2019

Uh oh!

miina commented Jul 16, 2019

Uh oh!

swissspidy commented Jul 16, 2019

Uh oh!

miina left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kienstra commented Jul 8, 2019 •

edited

Loading

kienstra commented Jul 9, 2019 •

edited

Loading

kienstra commented Jul 9, 2019 •

edited

Loading

swissspidy commented Jul 16, 2019 •

edited

Loading

miina commented Jul 16, 2019 •

edited

Loading