added rottentomatoes.com onebox (PR #783)

Hey All,

I’ve added a onebox for rottentomatoes.com.

Screens

Screen Shot 2013-04-28 at 4 26 44 PM

Comments/concerns:

  • I had to override the user agent for this onebox because rottentomatoes has an advertisement for their mobile app if you visit the site on a mobile device.
  • Any recommendations about the placement of the CSS?
  • I get some warnings, and I’m not sure how to resolve them (the first two appear on master as well):
/vagrant/lib/oneboxer.rb:13: warning: already initialized constant Oneboxer::Result
/vagrant/lib/oneboxer.rb:13: warning: previous definition of Result was here
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:6: warning: already initialized constant Oneboxer::RottentomatoesOnebox::SYNOPSIS_MAX_TEXT
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:6: warning: previous definition of SYNOPSIS_MAX_TEXT was here
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:7: warning: already initialized constant Oneboxer::RottentomatoesOnebox::ROTTEN_IMG
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:7: warning: previous definition of ROTTEN_IMG was here
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:8: warning: already initialized constant Oneboxer::RottentomatoesOnebox::FRESH_IMG
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:8: warning: previous definition of FRESH_IMG was here
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:9: warning: already initialized constant Oneboxer::RottentomatoesOnebox::POPCORN_IMG
/vagrant/lib/oneboxer/rottentomatoes_onebox.rb:9: warning: previous definition of POPCORN_IMG was here
  • There are 3 failing tests, however, they fail on master as well.

Thanks!

GitHub

You’ve signed the CLA, bolandrm. Thank you! This pull request is ready for review.

:heart:

the warnings are an indicator the class is being reloaded I can have a look at fixing, not tests failing here, which ones are failing for you? what version of ruby?

I think we should centralize all css required by oneboxes in the onebox.scss file for now, don’t like the idea of embedding.

Great job, love how it looks.

Using ruby 2.0.0p0.

Failures:

  1) IncomingLink add is able to look up user_id and log it from the GET params
     Failure/Error: IncomingLink.add(req('http://somesite.com?u=bob'))
     ActiveRecord::UnknownAttributeError:
       unknown attribute: current_user_id
     # ./app/models/incoming_link.rb:28:in `add'
     # ./spec/models/incoming_link_spec.rb:72:in `block (3 levels) in <top (required)>'

  2) Stores incoming links doesn't raise an error on a very long link
     Failure/Error: lambda { get topic.relative_url, nil, {'HTTP_REFERER' => "http://#{'a' * 2000}.com"} }.should_not raise_error
       expected no Exception, got #<ActiveRecord::UnknownAttributeError: unknown attribute: current_user_id> with backtrace:
         # ./app/models/incoming_link.rb:28:in `add'
         # ./app/controllers/application_controller.rb:243:in `store_incoming_links'
         # ./config/initializers/quiet_logger.rb:10:in `call_with_quiet_assets'
         # ./config/initializers/silence_logger.rb:19:in `call'
         # ./spec/requests/store_incoming_spec.rb:19:in `block (3 levels) in <top (required)>'
         # ./spec/requests/store_incoming_spec.rb:19:in `block (2 levels) in <top (required)>'
     # ./spec/requests/store_incoming_spec.rb:19:in `block (2 levels) in <top (required)>'

  3) Stores incoming links stores an incoming link when there is an off-site referer
     Failure/Error: get topic.relative_url, nil, {'HTTP_REFERER' => "http://google.com/search"}
     ActiveRecord::UnknownAttributeError:
       unknown attribute: current_user_id
     # ./app/models/incoming_link.rb:28:in `add'
     # ./app/controllers/application_controller.rb:243:in `store_incoming_links'
     # ./config/initializers/quiet_logger.rb:10:in `call_with_quiet_assets'
     # ./config/initializers/silence_logger.rb:19:in `call'
     # ./spec/requests/store_incoming_spec.rb:24:in `block (3 levels) in <top (required)>'
     # ./spec/requests/store_incoming_spec.rb:23:in `block (2 levels) in <top (required)>'

Finished in 2 minutes 13.68 seconds
2108 examples, 3 failures, 1 pending

Failed examples:

rspec ./spec/models/incoming_link_spec.rb:70 # IncomingLink add is able to look up user_id and log it from the GET params
rspec ./spec/requests/store_incoming_spec.rb:18 # Stores incoming links doesn't raise an error on a very long link
rspec ./spec/requests/store_incoming_spec.rb:22 # Stores incoming links stores an incoming link when there is an off-site referer

aha … looks to me like you did not migrate your test db :slight_smile:

On Mon, Apr 29, 2013 at 12:51 PM, Ryan Boland notifications@github.comwrote:

Using ruby 2.0.0p0.

Failures:

  1. IncomingLink add is able to look up user_id and log it from the GET params Failure/Error: IncomingLink.add(req(‘http://somesite.com?u=bob’)) ActiveRecord::UnknownAttributeError: unknown attribute: current_user_id

    ./app/models/incoming_link.rb:28:in `add’

    ./spec/models/incoming_link_spec.rb:72:in`block (3 levels) in <top (required)>’

  2. Stores incoming links doesn’t raise an error on a very long link Failure/Error: lambda { get topic.relative_url, nil, {‘HTTP_REFERER’ => “http://#{‘a’ * 2000}.com”} }.should_not raise_error expected no Exception, got #<ActiveRecord::UnknownAttributeError: unknown attribute: current_user_id> with backtrace: # ./app/models/incoming_link.rb:28:in add' # ./app/controllers/application_controller.rb:243:instore_incoming_links’ # ./config/initializers/quiet_logger.rb:10:in call_with_quiet_assets' # ./config/initializers/silence_logger.rb:19:incall’ # ./spec/requests/store_incoming_spec.rb:19:in block (3 levels) in <top (required)>' # ./spec/requests/store_incoming_spec.rb:19:inblock (2 levels) in <top (required)>’

    ./spec/requests/store_incoming_spec.rb:19:in `block (2 levels) in <top (required)>’

  3. Stores incoming links stores an incoming link when there is an off-site referer Failure/Error: get topic.relative_url, nil, {‘HTTP_REFERER’ => “http://google.com/search”} ActiveRecord::UnknownAttributeError: unknown attribute: current_user_id

    ./app/models/incoming_link.rb:28:in `add’

    ./app/controllers/application_controller.rb:243:in`store_incoming_links’

    ./config/initializers/quiet_logger.rb:10:in `call_with_quiet_assets’

    ./config/initializers/silence_logger.rb:19:in`call’

    ./spec/requests/store_incoming_spec.rb:24:in `block (3 levels) in <top (required)>’

    ./spec/requests/store_incoming_spec.rb:23:in`block (2 levels) in <top (required)>’

Finished in 2 minutes 13.68 seconds 2108 examples, 3 failures, 1 pending

Failed examples:

rspec ./spec/models/incoming_link_spec.rb:70 # IncomingLink add is able to look up user_id and log it from the GET params rspec ./spec/requests/store_incoming_spec.rb:18 # Stores incoming links doesn’t raise an error on a very long link rspec ./spec/requests/store_incoming_spec.rb:22 # Stores incoming links stores an incoming link when there is an off-site referer

— Reply to this email directly or view it on GitHubhttps://github.com/discourse/discourse/pull/783#issuecomment-17148205 .

haha good call. I should read them next time instead of just glancing long enough to be sure that I didn’t cause them =P

Thanks !

btw did you check if there is any way we can use: http://developer.rottentomatoes.com/ to avoid all this shenanigans ? is this a violation of the rotten tomatoes TOS … can you discuss with them on their dev forum

Looks like we can use the API, sorry about that. I’ll look into it later on.

Is there some configuration that might allow someone setting up a discourse forum to provide their own API key?

On Apr 28, 2013, at 11:49 PM, Sam notifications@github.com wrote:

btw did you check if there is any way we can use: http://developer.rottentomatoes.com/ to avoid all this shenanigans ? is this a violation of the rotten tomatoes TOS … can you discuss with them on their dev forum

— Reply to this email directly or view it on GitHub.

I guess you can create a site setting that forum owners can use to provide their API key.

its possible, I’m concerned that requiring API keys will heavily diminish the usefulness of the API, is there a way we can just make our api key public, I did this in the past with thetvdb and themoviedb and they were fine with that.

Can you raise it http://developer.rottentomatoes.com/forum/topics/100322

On Mon, Apr 29, 2013 at 9:15 PM, Régis Hanol notifications@github.comwrote:

I guess you can create a site setting that forum owners can use to provide their API key.

— Reply to this email directly or view it on GitHubhttps://github.com/discourse/discourse/pull/783#issuecomment-17161070 .

Yeah this is a problem, if we require 20 different “keys” for oneboxing to work… that’s unsustainable. If the solution @sam proposed, to get one key as “discourse forum” and check it into the code for everyone to use… that is workable.