Skip to content
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

Enable larger video uploads for Jetpack customers with standalone VideoPress subs #16322

Merged

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Apr 13, 2022

Fixes #16077

This PR addresses an issue where Jetpack-connected users with standalone VideoPress subscriptions were blocked from uploading videos via the app.

The reason for the block was that, as per #15719, users with free plans were recently prevented from uploading videos longer than five minutes in length via the app. If a Jetpack user has a standalone subscription, they're technically counted as being on a "free plan", even though they're paying for VideoPress.

Testing

Pre-requisites:

  • A Jetpack-connected site with a standalone VideoPress subscription.
  • A video that is longer than five minutes in length uploaded to your Android device.

To test:

  • Navigate to the post editor for your Jetpack-connected site.
  • Add a new video block and select the "choose from device" option.
  • Select the larger video (it should be longer than five minutes for our purposes) and wait for it to upload.
  • Verify that the video uploads with no error.

Code changes

isActiveModuleEnabled("videopress") has been added alongside existing checks for hasFreePlan. This new check will always return false for WordPress.com sites and only return true in cases where a Jetpack site has the VideoPress module enabled, which can only be the case if there is an active VideoPress subscription.

Note, the IsVideoPressSupported check was not used as it always returns as false if there is no paid plan, even if there is a standalone VideoPress subscription. The isActiveModuleEnabled("videopress") check was the only method I could find to fix our specific use case, where we need to determine if a site has a standalone VideoPress subscription.

Regression Notes

  1. Potential unintended areas of impact

This PR shouldn't impact or block the way other users are able to upload videos. Those with free WordPress.com plans should still not be able to upload videos while those with a self-hosted site should be able to.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

I went through the manual testing steps on the following site types:

  • Simple WordPress.com site with a free plan.
  • Simple WordPress.com site with a paid plan.
  • Atomic WordPress.com site.
  • Jetpack-connected site with a VideoPress subscription.
  • Jetpack-connected site without a VideoPress subscription.
  • Self-hosted site.
  1. What automated tests I added (or what prevented me from doing so)

No automated tests were added as this is a small change that I didn't feel warranted such tests.

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

With this commit, video uploads are enabled for all VideoPress subs via an explicit check for whether VideoPress is enabled. Previously, there was only a check for whether a user was on a free plan, which excluded those with standalone VideoPress subs.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 13, 2022

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 13, 2022

You can test the changes on this Pull Request by downloading the APKs:

@SiobhyB SiobhyB changed the title Enable video uploads for all VideoPress subs Enable all video uploads for Jetpack customers with standalone VideoPress subs Apr 14, 2022
@SiobhyB SiobhyB changed the title Enable all video uploads for Jetpack customers with standalone VideoPress subs Enable larger video uploads for Jetpack customers with standalone VideoPress subs Apr 14, 2022
@SiobhyB SiobhyB marked this pull request as ready for review April 14, 2022 01:03
@SiobhyB SiobhyB requested review from ravishanker and mkevins April 14, 2022 01:03
Siobhan added 2 commits April 19, 2022 14:03
This is to resolve the buildkite error being caused by the previously complex logic.
@SiobhyB SiobhyB added this to the 19.8 milestone Apr 19, 2022
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job tracking this down and finding a solution 👍 I tested this via the described steps on a Pixel 3a, physical device (and also reproduced the original issue in trunk), and confirmed that this PR fixes the issue. I also manually tested a Business site as well as a free site for regressions, and everything is still working as expected for those scenarios as well.

The code changes look good too.

While looking at the code, I saw an opportunity to refactor things a bit to reduce some unnecessary complexity (not introduced in your PR). I've opened a PR to address that here: #16375, please feel free to merge this into your PR, or merge this PR as is (the target branch on that PR should auto-update in that case). 😄

…loads-for-all-videopress-subscribers--refactor-suggestion

Refactor video duration check to simplify the nested ifs
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Apr 21, 2022

Thank you for the review @mkevins! 🙇‍♀️

I've gone ahead to approve and merge your PR into this one via #16375 (review). :D I'll go ahead to merge this one now, too.

@SiobhyB SiobhyB enabled auto-merge April 21, 2022 06:20
@SiobhyB SiobhyB merged commit 67edc28 into trunk Apr 21, 2022
@SiobhyB SiobhyB deleted the issue/16077-enable-uploads-for-all-videopress-subscribers branch April 21, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Video Upload Errors Despite VideoPress Subscription
2 participants