OEmbed support for PreviewCard #2337

Gargron · 2017-04-23T00:21:08Z

Oh boy, this is quite a whammy!

I had to make an improved version of OEmbed::ProviderDiscovery because the original was using regex to get link tags from HTML and it broke if they were in the wrong order or there was a, say, rel="alternate" in the wrong place
Expand PreviewCard schema to save the variety of attributes you can get from OpenGraph and OEmbed
Use sanitize gem instead of Rails sanitize helper
Try to get OEmbed from a URL, if that's no good, try OpenGraph
Fix our own OEmbed API not working with new type of prettier URLs
<Card /> component can now display links, photos and videos:

expenses · 2017-04-23T02:29:22Z

Great! Does it Oembed linked toots?

ineffyble · 2017-04-23T16:40:12Z

Code Climate is not happy - do we care?

nightpool · 2017-04-23T17:12:04Z

app/lib/provider_discovery.rb

+        format ||= :json
+      end
+
+      if format.nil? || format == :xml


The first conditional here will never be true, since if format is nil, we always set it to :json on line 17.

I'm not sure this series of conditionals is doing what you expect it to do. Are you trying to do something like this?

def provider_endpoint(format) case format when :json html.at_xpath('//link[@type="application/json+oembed"]')['href'] when :xml html.at_xpath('//link[@type="application/xml+oembed"]')['href'] else provider_endpoint(:json) || provider_endpoint(:xml) end end

the current implementation uses JSON, or XML, or always json+oembed if no format is specified.

Gargron · 2017-04-23T19:01:55Z

For the time being, "rich" OEmbed types get ignored, because stuff like tweets tries to include <script> tags and looks generally shitty without them

don't display a link card anyway

Gargron · 2017-04-23T19:31:08Z

CodeClimate is not happy on this one but the formatting it wants me to do is unreadable so I say, ignore it.

mjankowski · 2017-04-25T00:12:22Z

app/controllers/api/oembed_controller.rb

@@ -14,8 +14,9 @@ def show
  def stream_entry_from_url(url)
    params = Rails.application.routes.recognize_path(url)

-    raise ActiveRecord::RecordNotFound unless params[:controller] == 'stream_entries' && params[:action] == 'show'
+    raise ActiveRecord::RecordNotFound unless %w(stream_entries statuses).include?(params[:controller]) && params[:action] == 'show'


Maybe extract that conditional into a method?

mjankowski · 2017-04-25T00:14:31Z

app/controllers/api/oembed_controller.rb


-    StreamEntry.find(params[:id])
+    return StreamEntry.find(params[:id]) if params[:controller] == 'stream_entries'


These two lines could also be a separate method...

def stream_entry if params[:controller] == 'stream_entries' StreamEntry.find(params[:id]) else Status.find(params[:id]).stream_entry end end

mjankowski · 2017-04-25T00:15:37Z

app/lib/provider_discovery.rb

+# frozen_string_literal: true
+
+class ProviderDiscovery < OEmbed::ProviderDiscovery
+  USER_AGENT = "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::VERSION}; +http://#{Rails.configuration.x.local_domain}/)"


Is this string already assembled somewhere else?

It would be great if we could have some sort of HTTP factory because headers and timeout settings have to be set from quite a few places, but I'm not sure how to do that idiomatically. HttpFactory doesn't sound like Ruby...

Fix #1681 - return existing access token when applicable instead of creating new

mjankowski · 2017-04-27T12:35:16Z

app/controllers/api/oembed_controller.rb

@@ -14,8 +14,20 @@ def show
  def stream_entry_from_url(url)
    params = Rails.application.routes.recognize_path(url)


I don't think this change needs to hold back merging this ... but it might be good to use a better local variable name here, since this is happening inside of a controller, and params is already a method (and used up in the show action...)

ghost · 2017-04-27T22:13:23Z

Is there a list of currently working websites? I tried e.g. Soundcloud and Twitter, but that didn't yet work. Just a normal link preview.

* OEmbed support for PreviewCard * Improve ProviderDiscovery code failure treatment * Do not crawl links if there is a content warning, since those don't display a link card anyway * Reset db schema * Fresh migrate * Fix rubocop style issues Fix mastodon#1681 - return existing access token when applicable instead of creating new * Fix test * Extract http client to helper * Improve oembed controller

Apparently, nokogumbo (pulled in by sanitize, added with `OEmbed Support for PreviewCard` (mastodon#2337) - 88725d6) tries to install before nokogiri, despite needing nokogiri available to build properly. Instruct it to use the same settings as nokogiri does when building nokogiri directly, instead of via bundler.

* Nanobox Support - Added support for running Mastodon using Nanobox, both for local development, and for deployment to production - Dev mode tested and is working properly - Deployment is undergoing test as of this writing. If it works, this line will be amended to state success; if not, one or more subsequent commits will provide fixes. * [nanobox] Resolve Deploy Issues Everything seems to work except routing to the streaming API. Will investigate with the Nanobox staff and make fix commits if needed. Changes made: - Also need `NODE_ENV` in production - Node runs on `:4000` - Use `envsubst` to commit `.env.production` values, since `dotEnv` packages don't always support referencing other variables - Can't precompile assets after `transform` hook, but do this locally so it only has to be done once. - Rails won't create `production.log` on its own, so we do this ourselves. - Some `start` commands run from `/data/` for some reason, so use absolute paths in command arguments * [nanobox] Update Ruby version * [nanobox] Fix db.rake Ruby code style issues * [nanobox] Minor Fixes Some minor adjustments to improve functionality: - Fixed routing to `web.stream` instances - Adjust `.env.nanobox` to properly generate a default `SMTP_FROM_ADDRESS` via `envsubst` - Update Nginx configs to properly support the needed HTTP version and headers for proper functionality (the streaming API doesn't work without some of these settings in place) * [nanobox] Move usage info to docs repo * [nanobox] Updates for 1.2.x - Need to leave out `pkg-config` since Nanobox deploys without Ruby's headers - create a gem group to exclude the gem during Nanobox installs, but allow it to remain part of the default set otherwise - Update cron jobs to cover new/updated Rake tasks - Update `.env.nanobox` to include latest defaults and additions * [nanobox] Fix for nokogumbo, added in 1.3.x Apparently, nokogumbo (pulled in by sanitize, added with `OEmbed Support for PreviewCard` (#2337) - 88725d6) tries to install before nokogiri, despite needing nokogiri available to build properly. Instruct it to use the same settings as nokogiri does when building nokogiri directly, instead of via bundler. * [nanobox] Set NODE_ENV during asset compile The switch to WebPack will rely on the local value of the NODE_ENV evar, so set it to production during asset compilation. * [nanobox] Rebase on master; update Nginx configs - `pkg-config` Gem no longer causes issues in Nanobox, so revert the Gemfile change which allowed excluding it - Update Nginx configuration files with latest recommendations from production documentation - Rebase on master to Get This Merged™ Everything should be golden!

OEmbed support for PreviewCard

1b3a47c

Gargron requested a review from mjankowski April 23, 2017 00:21

nightpool reviewed Apr 23, 2017

View reviewed changes

Improve ProviderDiscovery code failure treatment

5fc48ae

Gargron added 2 commits April 23, 2017 21:02

Merge branch 'master' into feature-oembed

89f7f81

Do not crawl links if there is a content warning, since those

f07bf48

don't display a link card anyway

mjankowski reviewed Apr 25, 2017

View reviewed changes

Gargron added 7 commits April 26, 2017 04:47

Reset db schema

f951483

Merge branch 'master' into feature-oembed

42c33ff

Fresh migrate

f4495b0

Fix rubocop style issues

95e743e

Fix #1681 - return existing access token when applicable instead of creating new

Fix test

64df1c1

Extract http client to helper

682da0f

Improve oembed controller

b74b597

mjankowski reviewed Apr 27, 2017

View reviewed changes

mjankowski approved these changes Apr 27, 2017

View reviewed changes

Gargron merged commit 88725d6 into master Apr 27, 2017

Gargron deleted the feature-oembed branch April 27, 2017 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEmbed support for PreviewCard #2337

OEmbed support for PreviewCard #2337

Gargron commented Apr 23, 2017

expenses commented Apr 23, 2017

ineffyble commented Apr 23, 2017

nightpool Apr 23, 2017 •

edited

Loading

nightpool Apr 23, 2017 •

edited

Loading

Gargron Apr 23, 2017

Gargron commented Apr 23, 2017

Gargron commented Apr 23, 2017

mjankowski Apr 25, 2017

mjankowski Apr 25, 2017

mjankowski Apr 25, 2017

Gargron Apr 25, 2017

mjankowski Apr 27, 2017

ghost commented Apr 27, 2017


		StreamEntry.find(params[:id])
		return StreamEntry.find(params[:id]) if params[:controller] == 'stream_entries'

		@@ -14,8 +14,20 @@ def show
		def stream_entry_from_url(url)
		params = Rails.application.routes.recognize_path(url)

OEmbed support for PreviewCard #2337

OEmbed support for PreviewCard #2337

Conversation

Gargron commented Apr 23, 2017

expenses commented Apr 23, 2017

ineffyble commented Apr 23, 2017

nightpool Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

nightpool Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

Gargron Apr 23, 2017

Choose a reason for hiding this comment

Gargron commented Apr 23, 2017

Gargron commented Apr 23, 2017

mjankowski Apr 25, 2017

Choose a reason for hiding this comment

mjankowski Apr 25, 2017

Choose a reason for hiding this comment

mjankowski Apr 25, 2017

Choose a reason for hiding this comment

Gargron Apr 25, 2017

Choose a reason for hiding this comment

mjankowski Apr 27, 2017

Choose a reason for hiding this comment

ghost commented Apr 27, 2017

nightpool Apr 23, 2017 •

edited

Loading

nightpool Apr 23, 2017 •

edited

Loading