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

VideoPress: Replace video uploads in Gutenberg #13682

Merged
merged 7 commits into from
Oct 14, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Oct 7, 2019

Fixes #11194

Changes proposed in this Pull Request:

Load an API fetch middleware in the block editor that overrides the video uploads in Gutenberg so they are uploaded against the WP.com API media endpoint and thus transcoded by VideoPress.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Improve VideoPress (paYKcK-f0-p2, p9dueE-xm-p2).

Testing instructions:

  • Apply D33658-code and sandbox the API.
  • Apply this branch to your JP dev environment.
  • Start from a Jetpack site with an active Professional upgrade.
  • Go to Jetpack > Settings in your dashboard, and enable the VideoPress feature.
  • Load the block editor.
  • Insert a video block.
  • Upload a video using the "Upload" button (not the "Media Library" one).
  • Check the VideoPress player is rendered on the editor.
  • Publish the post.
  • Check the VideoPress player is rendered on the published view.
  • Deactivate VideoPress.
  • Load the block editor again.
  • Make sure the gutenberg-video-upload.js script is not enqueued.
  • Sandbox a WP.com site and load the block editor.
  • Make sure the gutenberg-video-upload.js script is not enqueued on the WP.com site.

Proposed changelog entry for your changes:

VideoPress: Replace video uploads in Gutenberg

@mmtr mmtr added [Feature] VideoPress A feature to help you upload and insert videos on your site. [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Oct 7, 2019
@mmtr mmtr requested review from a team October 7, 2019 14:32
@mmtr mmtr self-assigned this Oct 7, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mmtr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33658-code before merging this PR. Thank you!

@mmtr
Copy link
Member Author

mmtr commented Oct 7, 2019

This is not working currently due to a CORS error when making the request to the WP.com media endpoint:

Access to fetch at 'https://public-api.wordpress.com/rest/v1.1/sites/<SITE_ID>/media/new?_locale=user' from origin '<JP_SITE>' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is 'false' which must be 'true' when the request's credentials mode is 'include'.

@jetpackbot
Copy link

jetpackbot commented Oct 7, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 813a173

@mmtr
Copy link
Member Author

mmtr commented Oct 7, 2019

This is not working currently due to a CORS error when making the request to the WP.com media endpoint:

Access to fetch at 'https://public-api.wordpress.com/rest/v1.1/sites/<SITE_ID>/media/new?_locale=user' from origin '<JP_SITE>' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: The value of the 'Access-Control-Allow-Credentials' header in the response is 'false' which must be 'true' when the request's credentials mode is 'include'.

Solved with fe79d63 but now we have another CORS error caused by the X-WP-Nonce header:

Access to fetch at 'https://public-api.wordpress.com/rest/v1.1/sites/<SITE_ID>/media/new?_locale=user' from origin '<JP_SITE>' has been blocked by CORS policy: Request header field x-wp-nonce is not allowed by Access-Control-Allow-Headers in preflight response.

@mmtr
Copy link
Member Author

mmtr commented Oct 7, 2019

Access to fetch at 'https://public-api.wordpress.com/rest/v1.1/sites/<SITE_ID>/media/new?_locale=user' from origin '<JP_SITE>' has been blocked by CORS policy: Request header field x-wp-nonce is not allowed by Access-Control-Allow-Headers in preflight response.

This has been solved in D33658-code by allowing the X-WP-Nonce header in the CORS requests.

@mmtr
Copy link
Member Author

mmtr commented Oct 7, 2019

The request is returning now a 400 error because the endpoint expects the data in a different format, so the middleware should change how the file is included in the payload.
Gutenberg also expects the returned data by the endpoint in a different format so the middleware should parse and format the response.

@kwight
Copy link
Contributor

kwight commented Oct 8, 2019

The request is returning now a 400 error because the endpoint expects the data in a different format, so the middleware should change how the file is included in the payload.

FWIW, yep, this is what I get (with all of the expected pieces already in place) 👍

@matticbot
Copy link
Contributor

mmtr, Your synced wpcom patch D33658-code has been updated.

@matticbot
Copy link
Contributor

mmtr, Your synced wpcom patch D33658-code has been updated.

@mmtr mmtr changed the title [WIP] VideoPress: Replace video uploads in Gutenberg VideoPress: Replace video uploads in Gutenberg Oct 9, 2019
@mmtr mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Oct 9, 2019
@matticbot
Copy link
Contributor

mmtr, Your synced wpcom patch D33658-code has been updated.

@mmtr
Copy link
Member Author

mmtr commented Oct 9, 2019

The request is returning now a 400 error because the endpoint expects the data in a different format, so the middleware should change how the file is included in the payload.
Gutenberg also expects the returned data by the endpoint in a different format so the middleware should parse and format the response.

Solved both problems in 7682948 and ff0194b, so this is now ready to review.

@mmtr mmtr requested a review from a team October 9, 2019 13:21
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Edit: my bad, I forgot to apply the related diff. I'll try again, but I'll read the instructions this time, sorry!

I get this error when I try to upload a video:

Access to fetch at 'https://public-api.wordpress.com/rest/v1.1/sites/164776955/media/new?_locale=user' from origin 'http://my.localsite' has been blocked by CORS policy: Request header field x-wp-nonce is not allowed by Access-Control-Allow-Headers in preflight response.
Uncaught (in promise) TypeError: Cannot read property 'code' of undefined
    at http://my.localsite/wp-content/plugins/gutenberg/build/api-fetch/index.js?ver=e20b1c8bba67f2198e1cfae53eac80c5:formatted:365

I don't get any feedback in the editor though; everything happens in the console and the editor seems to be stuck on uploading:
image

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works a lot better when I follow instructions! :)

I would have a remark to load a minified file when possible.

modules/videopress/class.videopress-gutenberg.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 9, 2019
@matticbot
Copy link
Contributor

mmtr, Your synced wpcom patch D33658-code has been updated.

@mmtr mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 10, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me, it tests well. That's improving the VideoPress experience so much, too ❤️

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 10, 2019
@jeherve jeherve added this to the 7.9 milestone Oct 10, 2019
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Nice work @mmtr! This tests well with both the upload button and drag-and-drop uploads. I also verified that the script was not loaded when VideoPress was disabled in settings.

Oct-10-2019 15-55-37

@mmtr mmtr merged commit a211b9e into master Oct 14, 2019
@mmtr mmtr deleted the update/videopress-gutenberg-upload-video branch October 14, 2019 10:49
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 14, 2019
@jeherve jeherve added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Oct 14, 2019
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VideoPress: allow users to upload videos from Gutenberg [2]
6 participants