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

Add OpenGraph video support #7043

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@svbergerem
Copy link
Member

svbergerem commented Sep 1, 2016

Fixes #4325.

If the OpenGraphReader finds a og_video:secure_url on the page it will be saved in the database. If we consider this URL secure (currently the bandcamp player is enabled) we will add a play button to the open graph image and on click we will load the video in an iframe.

opengraph bandcamp

this.statusMessage = factory.statusMessage({
"open_graph_cache": open_graph_cache
context("without a video_url", function() {
beforeEach(function() {

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Sep 1, 2016

Extra space after key 'model'.

this.openGraphCache = {
"url": "http://example.com/articles/123",
"title": "Example title",
"description": "Test description",

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Sep 1, 2016

Missing space before opening brace.

});

describe("rendering", function(){
it("shows the preview based on the opengraph data", function(){

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Sep 1, 2016

Extra space after key 'model'.

this.view.render();
var html = this.view.$el.html();
context("with a video_url", function() {
beforeEach(function() {

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Sep 1, 2016

Missing space before opening brace.

@svbergerem svbergerem force-pushed the svbergerem:add-opengraph-video branch from c77672f to 82450cb Sep 1, 2016

Steffen van Bergerem

@svbergerem svbergerem force-pushed the svbergerem:add-opengraph-video branch from 82450cb to 29d0820 Sep 1, 2016

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Sep 1, 2016

I changed the behavior a bit: now we will only save the video_url if we consider it secure. (Before the check was done before displaying the video_url) Currently Bandcamp is the only secure provider so we can assume that the given URL should be embedded in an iframe.

In the future we might decide that we want to add more secure providers and in this case we might have to check og_video:type (text/html in case of Bandcamp) and save it in the database. Since we don't need it right now I didn't add it in this PR.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Sep 1, 2016

Did bandcamp switch to an HTML5 player or is it still using a flash one?

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Sep 1, 2016

Since it's working on my laptop I'm pretty sure that they don't use a flash player.

@denschub

This comment has been minimized.

Copy link
Member

denschub commented Sep 4, 2016

This looks fine, but I'm unsure about the migration. Do we have some runtime estimations?

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Sep 4, 2016

== 20160901072443 AddVideoUrlToOpenGraphCache: migrated (16.9579s) ============

with my production DB.

@denschub

This comment has been minimized.

Copy link
Member

denschub commented Sep 4, 2016

r+, feel free to merge to next-minor.

@denschub denschub added this to the 0.6.1.0 milestone Sep 4, 2016

@SuperTux88 SuperTux88 closed this in 4d51c02 Sep 4, 2016

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Sep 4, 2016

Merged as 4d51c02

Thanks :)

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Sep 4, 2016

It was even faster on my real production DB :)

== 20160901072443 AddVideoUrlToOpenGraphCache: migrated (4.7082s) =============

@svbergerem svbergerem deleted the svbergerem:add-opengraph-video branch Sep 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.