Skip to content

Conversation

@mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 18, 2020

Fixes #12711

The problem described in the aforementioned issue was due to having the target site enable VideoPress, but we had no
support for VideoPress as of yet.

This PR fixes that by simply building up the VideoPress URL correctly for the case when the video's mime type is video/videopress.

Note: this other branch here explores what I had in mind, but it turns out the SiteModel being passed around is not needed (the value for mIsVideoPressSupported is false for our whitelisted test site anyway), and we'd also need to do more work than what can be seen on that branch to actually support the original file URL in FluxC. I think it may not be a great idea to do all of that for VideoPress URLs given the content subdomain seems to have been fixed for a long time and has been kept as is.
If / when the case happens that different sites may have different URLs for VideoPress videos, we can go ahead and implement support for this on FLuxC to correctly populate a new field with this information.
Added a TODO comment there to remind our future selves when/if this is needed.

To test:

  1. switch site to a whitelisted story site create a story
  2. cretae a new story
  3. add a video slide
  4. tap NEXT and then add some title to the post
  5. upload
  6. verify the url is OK and the video plays correctly

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@mzorz mzorz added this to the 15.6 milestone Aug 18, 2020
@mzorz mzorz requested a review from aforcier August 18, 2020 20:27
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 18, 2020

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 18, 2020

You can test the changes on this Pull Request by downloading the APK here.

@aforcier
Copy link
Contributor

I was curious what happens when uploading a video from Gutenberg, it looks like the video block knows how to interpret that kind of URL:

<!-- wp:video {"autoplay":false,"guid":"NhaXbXse","id":305,"loop":false,"muted":false,"playsInline":false,"src":"https://site.me/wp_story1597806821800_0-mp4/"} -->
<figure class="wp-block-video wp-block-embed is-type-video is-provider-videopress"><div class="wp-block-embed__wrapper">
https://videopress.com/v/NhaXbXse?preloadContent=metadata
</div></figure>
<!-- /wp:video -->

So perhaps the ideal fix is changing the story block to support videopress URLs like that, and including the videopress guid on our end? cc @Tug

I'll proceed with reviewing this anyway, but the above seems like a better long-term fix.

Copy link
Contributor

@aforcier aforcier left a comment

Choose a reason for hiding this comment

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

Working well @mzorz - :shipit:!

@aforcier aforcier merged commit cd67fe1 into develop Aug 19, 2020
@aforcier aforcier deleted the issue/12711-fix-video-press-url-take2 branch August 19, 2020 05:25
@aforcier
Copy link
Contributor

Just noting that my comment above came up in conversation with Tug and he mentioned it might be possible to have the story block rely on the video id alone, and remove the need for us to assemble the URL 'by hand' on mobile. He'll look into it but it's agreed that it's not very high priority and we're okay with the solution in this PR for now.

@mzorz
Copy link
Contributor Author

mzorz commented Aug 19, 2020

Agreed, the best path forward is having the block make the right interpretation - we'll dig into that when we get to Automattic/stories-android#504 (just realized it was being tracked as a note in Automattic/stories-android#249 but it wasn't being tracked as a separate issue that I could find); I had a few notes on the side for when the time comes, but given this came up here as well leaving them here for completeness:

see wordpress-mobile/gutenberg-mobile#854 (comment)

To do this on Mobile, we might have to import the JetPack block, right?

I think we just need to read the VideoPress hash, query the native side (there is an API call that from a VP hash does return video info) and replace it with the video URL when the call returns.

Related Automattic/jetpack#12358

@aforcier
Copy link
Contributor

Quick follow-up, I believe with Automattic/jetpack@d81748c the story block now just needs the media library ID to render the video (and will resolve the appropriate URL on its own). We were already sending that, so I think this PR can be reverted and we can avoid relying on a hardcoded videopress URL. Seems to be working from my own tests with a build before this PR. @Tug might have some more specifics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WP Stories: videos URL not welformed

3 participants