Limit video uploads on free sites #17689

Gio2018 · 2021-12-20T06:42:40Z

This PR prevents videos longer than 5 minutes to be uploaded on free sites. This applies to captured or selected videos from:

My Site - Media
Editor (posts and pages)
Stories

Notes:

Video upload from "Other Apps" was already disabled, for videos of any duration, before this PR
On Stories, there was an existing video limit of 29 seconds: this has not been changed

Fixes #NA

To test:

Pre requisites:

make sure to have local files both on device and from other apps (e.g Files) videos that are longer than 5 minutes.
Tip: to use shorter videos (this is particularly useful when testing the capture), you can temporary tweak the following code (WPMediaAsset+VideoLimits.swift)
let maximumVideoDurationForFreeSites: CGFloat = 300
choosing a shorter value than 5 minutes (e.g. 5 seconds)

Go to My site and make sure to select a site with a free plan
Go to Media
Tap on the + (top right corner)
Tap on "Take Photo or Video"
Record a video longer that the maximum allowed duration
Tap on "Use video"
Make sure the "Video not uploaded" (see screenshot below) alert is shown, and no video is added to the media library
Record a video shorter than the maximum allowed duration, and make sure it can be uploaded instead.
Tap on + again
Tap on "Choose from My Device"
Make sure that videos longer than the maximum allowed duration are greyed out and cannot be selected. Also, make sure that the "Selection not allowed" alert appears (see screenshot below)
Optional (this was not changed in this PR): tap on "Other apps", and make sure that video files (of any duration) are still greyed out and cannot be uploaded.
Go back and tap on the blue Floating Action Button (FAB)
Select "Story Post"
Tap on the icon in the lower left corner and repeat step 11
Optional (this was not changed in this PR): record a story video and make sure the max duration of 29 seconds applies
Go back and either tap again on the FAB and then "Blog Post" to create a new post, or edit an existing post from the "Posts" section
Repeat steps 4 to 12 (you can either add multiple video blocks or tap "Add Video" on a video block that has no media yet, to display the selection options)
Go back and either create a new page from the FAB or edit an existing one from the "Pages" section
Repeat step 18
Repeat the steps 2 to 20 on a paid site, and make sure you can upload videos of any duration

Captured video too long	Selected video too long	Choose video
		!

Regression Notes

Potential unintended areas of impact
Limited impact: these changes impact the video upload sections in media, Gutenberg and Stories
What I did to test those areas of impact (or what existing automated tests I relied on)
Test all the impacted cases, both with free and paid plans
What automated tests I added (or what prevented me from doing so)
None

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…a does not exceed video limit duration, if it's a video

…a do not exceed video limit duration, if it's a video

…ss-iOS into issue/limit-video-uploads-on-free-sites

…o exceeds the allowed duration on free sites

…sers about video limit exceeded

…videos that cannot be selected in the media picker

…do not exceed allowed duration on free sites

… exceed allowed duration on free sites

…new video limits for free sites to WP Stories

… free sites to the editor

peril-wordpress-mobile · 2021-12-20T06:42:45Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-12-20T07:03:33Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

frosty

For the most part, this is working really well! I tested across the Media library, blog post editor, and stories editor. I'm seeing one issue where I think the logic is wrong where we're using a guard statement, which wouldn't let me upload a video to my paid blog.

My other main comment is that we're duplicating a snippet of logic across many places to check if we can upload an asset, so perhaps we should extract that somewhere central.

The newly added files also need adding to the Jetpack target, as it's currently failing to build.

frosty · 2021-12-20T11:11:20Z

WordPress/Classes/Utility/Media/VideoLimitsAlertPresenter.swift

+    /// - Parameters:
+    ///   - viewController: presenting UIViewController
+    ///   - title: title of the alert
+    private func persentVideoLimitsAlert(on viewController: UIViewController, title: String) {


Nitpick, typo: persent -> present

frosty · 2021-12-20T11:15:59Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

 extension GutenbergMediaPickerHelper: WPMediaPickerViewControllerDelegate {

    func mediaPickerController(_ picker: WPMediaPickerViewController, didFinishPicking assets: [WPMediaAsset]) {
-        invokeMediaPickerCallback(asset: assets)
-        picker.dismiss(animated: true, completion: nil)
+        if picker == self.cameraPicker,


Can omit all uses of self in this method.

frosty · 2021-12-20T11:18:23Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

+        asset.exceedsFreeSitesAllowance() &&
+        !post.blog.hasPaidPlan


We're repeating this bit of logic in three places in this class – perhaps we should create a smaller helper method?

func canUploadAsset(_ asset: WPMediaAsset, for blog: Blog) -> Bool { asset.exceeedsFreeSitesAllowance() && !blog.hasPaidPlan }

Or thinking about it, we're using this same bit of logic across a whole range of classes multiple times. Perhaps we should move it onto Blog? canUploadAsset(_ asset: WPMediaAsset)?

That makes perfect sense @frosty ! Only one minor change to the above snippet, I think the condition for a blog that can upload video assets should be (if we put it in Blog):

func canUploadAsset(_ asset: WPMediaAsset) -> Bool { hasPaidPlan || !asset.exceedsFreeSitesAllowance() }

does it sound right?

Yep, that makes sense to me!

frosty · 2021-12-20T11:22:29Z

WordPress/Classes/ViewRelated/Media/CameraCaptureCoordinator.swift

-                    return
+            guard let media = media as? PHAsset,
+                    !media.exceedsFreeSitesAllowance(),
+                    !blog.hasPaidPlan else {


I think this logic isn't correct. I'm seeing a case where media is exceeding the free sizes allowance, but the blog has a paid plan, but I'm not allowed to upload it. We only want to present the limit if both the media exceeds the limit and the blog doesn't have a paid plan.

Thanks for catching this @frosty ! This is one more case where your suggestion to extract the canUploadAsset method comes in handy.

… didFinishPicking assets: [WPMediaAsset]) in GutenbergMediaPickerHelper, removed unnecessary use of self

…if the blog can upload the given asset

… for Blog

Gio2018 · 2021-12-20T17:48:15Z

Thanks for reviewing @frosty ! PR is ready for another pass, whenever you can.

frosty

Great changes! Just one small typo, and one question but otherwise let's

frosty · 2021-12-20T22:17:52Z

WordPress/Classes/Utility/Media/Blog+VideoLimits.swift

@@ -0,0 +1,7 @@
+extension Blog {
+
+    /// returns true if the blog is allowed upload the given asset, true otherwise


Nitpick allowed _to_ upload
Also, false otherwise! :)

frosty · 2021-12-20T22:18:50Z

WordPress/Classes/ViewRelated/Gutenberg/GutenbergMediaPickerHelper.swift

-        picker !== cameraPicker &&
-        asset.exceedsFreeSitesAllowance() &&
-        !post.blog.hasPaidPlan
+        !post.blog.canUploadAsset(asset)


Do we still need to check if it's not the camera picker here?

Yes, thanks for catching this!!

roundhill · 2021-12-20T23:24:51Z

Does this just check for the video length, or does it also check that the file size is less than 100MB? Reason I'm asking is that it is much easier for us to check the file size at the api endpoint rather than the file length, so it'd be good to tell the user in the app that their files should be under 100MB.

Gio2018 · 2021-12-21T00:14:26Z

Does this just check for the video length, or does it also check that the file size is less than 100MB? Reason I'm asking is that it is much easier for us to check the file size at the api endpoint rather than the file length, so it'd be good to tell the user in the app that their files should be under 100MB.

Hey @roundhill , we are only checking duration.

The main reason is that users can control duration much more than size, especially with newer devices allowing 4K videos. Potentially there could be shorter videos that exceed the size and longer ones that do not. Also, the current UI of both the media picker and the camera capture do not provide info about the size. This could probably be changed, but we'd need to investigate and do a separate project for this.

…not the camera picker prior to show an overlay for disallowed videos

…ss-iOS into issue/limit-video-uploads-on-free-sites

Giorgio Ruscigno added 13 commits December 16, 2021 12:10

Refactor CameraCaptureCoordinator, added check that the uploaded medi…

6edf55c

…a does not exceed video limit duration, if it's a video

Refactor MediaLibraryViewController, add check that the selected medi…

8d5b57e

…a do not exceed video limit duration, if it's a video

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

774f90c

…ss-iOS into issue/limit-video-uploads-on-free-sites

Fix WordPressFlux package resolution error

1bd5fe8

Remove unused PHAsset extension

1f907a9

Add WPMediaAsset extension that contains a helper to detect if a vide…

b95c425

…o exceeds the allowed duration on free sites

Add VideoLimitsAlertPresenter, a type containing alerts that inform u…

240328e

…sers about video limit exceeded

Refactor MediaLibraryPicker, add a semi transparent grey overlay for …

e894ab5

…videos that cannot be selected in the media picker

Refactor CameraCaptureCoordinator, add checking that captured videos …

70d0737

…do not exceed allowed duration on free sites

Refactor MediaLibraryViewController, prevent selection of videos that…

c58e1ee

… exceed allowed duration on free sites

Refactor WPMediaPickerForKanvas and MediaPickerDelegate to apply the …

957c8d5

…new video limits for free sites to WP Stories

Refactor GutenbergMediaPickerHelper to apply the new video limits for…

c4b2831

… free sites to the editor

Updated release notes

2ce70d6

Gio2018 requested a review from frosty December 20, 2021 06:42

Gio2018 added [Type] Enhancement Media labels Dec 20, 2021

Gio2018 added this to the 19.0 milestone Dec 20, 2021

frosty requested changes Dec 20, 2021

View reviewed changes

Giorgio Ruscigno added 4 commits December 20, 2021 11:02

Fixed typo in VideoLimitsAlertPresenter

8bc5538

Refactor mediaPickerController(_ picker: WPMediaPickerViewController,…

67e09fa

… didFinishPicking assets: [WPMediaAsset]) in GutenbergMediaPickerHelper, removed unnecessary use of self

Add Blog extension that contains the canUploadAsset method, to check …

e712a8a

…if the blog can upload the given asset

Refactor Gutenberg and Media Library to use the canUploadAsset method…

63a7e80

… for Blog

Gio2018 requested a review from frosty December 20, 2021 17:47

Add jetpack target to files that were missing it

1149d73

frosty approved these changes Dec 20, 2021

View reviewed changes

Giorgio Ruscigno added 3 commits December 20, 2021 18:18

Update Blog+VideoLimits, fixed typo in comment

142843f

Update GutenbergMediaPickerHelper, restored check that the picker is …

f2c12cf

…not the camera picker prior to show an overlay for disallowed videos

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

c115c25

…ss-iOS into issue/limit-video-uploads-on-free-sites

Gio2018 merged commit 3f8ba8f into develop Dec 21, 2021

Gio2018 deleted the issue/limit-video-uploads-on-free-sites branch December 21, 2021 18:31

twstokes mentioned this pull request Apr 4, 2022

Video length restrictions should only apply to free .com sites #18277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit video uploads on free sites #17689

Limit video uploads on free sites #17689

Gio2018 commented Dec 20, 2021

peril-wordpress-mobile bot commented Dec 20, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Dec 20, 2021 •

edited

Loading

frosty left a comment •

edited

Loading

frosty Dec 20, 2021

frosty Dec 20, 2021

frosty Dec 20, 2021

frosty Dec 20, 2021

Gio2018 Dec 20, 2021

frosty Dec 20, 2021

frosty Dec 20, 2021

Gio2018 Dec 20, 2021

Gio2018 commented Dec 20, 2021

frosty left a comment

frosty Dec 20, 2021

frosty Dec 20, 2021

Gio2018 Dec 21, 2021

roundhill commented Dec 20, 2021

Gio2018 commented Dec 21, 2021

		@@ -0,0 +1,7 @@
		extension Blog {

		/// returns true if the blog is allowed upload the given asset, true otherwise

Limit video uploads on free sites #17689

Limit video uploads on free sites #17689

Conversation

Gio2018 commented Dec 20, 2021

Regression Notes

peril-wordpress-mobile bot commented Dec 20, 2021 • edited Loading

peril-wordpress-mobile bot commented Dec 20, 2021 • edited Loading

frosty left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gio2018 commented Dec 20, 2021

frosty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roundhill commented Dec 20, 2021

Gio2018 commented Dec 21, 2021

peril-wordpress-mobile bot commented Dec 20, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Dec 20, 2021 •

edited

Loading

frosty left a comment •

edited

Loading