draper+rspec does not honor default_url_options #313

Closed
lidaobing opened this Issue Oct 18, 2012 · 20 comments

Comments

Projects
None yet
6 participants
@lidaobing

how to reproduce:

  1. add default_url_options to ApplicationController
  def self.default_url_options(options = nil)
    {:locale => (I18n.locale =~ /^en/ ? 'en' : nil) }
  end
  1. add locale part to routes.rb
  scope "(:locale)", :locale => /en|zh/ do
    #...
  end
  1. use helper in your decorator
class PostDecorator < Draper::Base
  def as_json(opts={})
    {:post_path => h.post_path(self.model)}
  end
end
  1. add a rspec test
  context '#as_json' do
    it 'should works' do
      post = create :post
      res = PostDecorator.decorate(post).as_json
      res[:post_path].should be_a(String)
    end
  end
  1. run this rspec

it failed with something like:

    ActionController::RoutingError:
       No route matches {:action=>"show", :controller=>"posts", :locale=>#<Post id=1>}
@nashby

This comment has been minimized.

Show comment
Hide comment
@nashby

nashby Oct 18, 2012

Member

@lidaobing does it work without Draper?

Member

nashby commented Oct 18, 2012

@lidaobing does it work without Draper?

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Oct 18, 2012

yes, it works without Draper, like in a view test, maybe I need setup a project to reproduce this bug

yes, it works without Draper, like in a view test, maybe I need setup a project to reproduce this bug

@nashby

This comment has been minimized.

Show comment
Hide comment
@nashby

nashby Oct 18, 2012

Member

that would be super awesome!

Member

nashby commented Oct 18, 2012

that would be super awesome!

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Oct 18, 2012

ok, new way to reproduce this bug:

git clone git://github.com/lidaobing/draper-issue-313.git
cd draper-issue-313/
bundle 
RAILS_ENV=test rake db:migrate
rspec spec/decorators/post_decorator_spec.rb 

if you want to test whether this works, run rails s, and create a new post with "http://localhost:3000/posts/new", then visit "http://localhost:3000/posts/1.json" and "http://localhost:3000/posts/1", both work and have called PostDecorator#as_json

ok, new way to reproduce this bug:

git clone git://github.com/lidaobing/draper-issue-313.git
cd draper-issue-313/
bundle 
RAILS_ENV=test rake db:migrate
rspec spec/decorators/post_decorator_spec.rb 

if you want to test whether this works, run rails s, and create a new post with "http://localhost:3000/posts/new", then visit "http://localhost:3000/posts/1.json" and "http://localhost:3000/posts/1", both work and have called PostDecorator#as_json

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Oct 18, 2012

I add the following code to make the rspec view test works: https://github.com/lidaobing/draper-issue-313/blob/master/spec/spec_helper.rb#L14

you can try the view testcase by rspec spec/views/posts/show.html.erb_spec.rb

module ActionView::Helpers::UrlHelper
  def url_options
    locale = I18n.locale.to_s.split('-')[0]
    { :locale => locale == 'en' ? 'en' : nil }.merge(super)
  end
end

but I don't know any workaround to make draper works

I add the following code to make the rspec view test works: https://github.com/lidaobing/draper-issue-313/blob/master/spec/spec_helper.rb#L14

you can try the view testcase by rspec spec/views/posts/show.html.erb_spec.rb

module ActionView::Helpers::UrlHelper
  def url_options
    locale = I18n.locale.to_s.split('-')[0]
    { :locale => locale == 'en' ? 'en' : nil }.merge(super)
  end
end

but I don't know any workaround to make draper works

@nashby

This comment has been minimized.

Show comment
Hide comment
@nashby

nashby Oct 18, 2012

Member

@steveklabnik looks like we still have an issue with view context in specs...
@lidaobing for now you can add

config.before :type => :decorator do
  ApplicationController.new.view_context
end

to your RSpec.configure block.

Member

nashby commented Oct 18, 2012

@steveklabnik looks like we still have an issue with view context in specs...
@lidaobing for now you can add

config.before :type => :decorator do
  ApplicationController.new.view_context
end

to your RSpec.configure block.

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Oct 18, 2012

@nashby cool, it works for me, thanks

@nashby cool, it works for me, thanks

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Oct 18, 2012

Member

Ugh. yeah, leaving this open; it needs fixed.

Member

steveklabnik commented Oct 18, 2012

Ugh. yeah, leaving this open; it needs fixed.

@nashby

This comment has been minimized.

Show comment
Hide comment
@nashby

nashby Oct 18, 2012

Member

@steveklabnik why it was removed here 75a8204 anyway?

Member

nashby commented Oct 18, 2012

@steveklabnik why it was removed here 75a8204 anyway?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Oct 18, 2012

Member

Because when we started doing https://github.com/drapergem/draper/blob/master/lib/draper/view_context.rb#L34-38 it didn't seem neccesary anymore.

Member

steveklabnik commented Oct 18, 2012

Because when we started doing https://github.com/drapergem/draper/blob/master/lib/draper/view_context.rb#L34-38 it didn't seem neccesary anymore.

@nashby

This comment has been minimized.

Show comment
Hide comment
@nashby

nashby Oct 18, 2012

Member

@steveklabnik we need integration tests badly.

Member

nashby commented Oct 18, 2012

@steveklabnik we need integration tests badly.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Oct 18, 2012

Member

Agreed. The branch is set up, I just haven't had time to write any.
Probably early next week.

On Thursday, October 18, 2012, Vasiliy Ermolovich wrote:

@steveklabnik https://github.com/steveklabnik we need integration tests
badly.


Reply to this email directly or view it on GitHubhttps://github.com/drapergem/draper/issues/313#issuecomment-9556349.

Member

steveklabnik commented Oct 18, 2012

Agreed. The branch is set up, I just haven't had time to write any.
Probably early next week.

On Thursday, October 18, 2012, Vasiliy Ermolovich wrote:

@steveklabnik https://github.com/steveklabnik we need integration tests
badly.


Reply to this email directly or view it on GitHubhttps://github.com/drapergem/draper/issues/313#issuecomment-9556349.

@lidaobing

This comment has been minimized.

Show comment
Hide comment
@lidaobing

lidaobing Oct 23, 2012

@nashby your workaround has a problem, the h.post_url no longer works

how to reproduce

git clone git://github.com/lidaobing/draper-issue-313.git
cd draper-issue-313/
bundle 
RAILS_ENV=test rake db:migrate

# without your workaround h.post_url works:
rspec spec/decorators/post_decorator_spec.rb:49

# with your workaround, h.post_url no longer works
rspec spec/decorators/post_decorator_spec.rb:23

@nashby your workaround has a problem, the h.post_url no longer works

how to reproduce

git clone git://github.com/lidaobing/draper-issue-313.git
cd draper-issue-313/
bundle 
RAILS_ENV=test rake db:migrate

# without your workaround h.post_url works:
rspec spec/decorators/post_decorator_spec.rb:49

# with your workaround, h.post_url no longer works
rspec spec/decorators/post_decorator_spec.rb:23

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 8, 2012

Member

Ugh. Spent some more time looking at this, still don't know why it doesn't respect default_url_options.

Member

steveklabnik commented Nov 8, 2012

Ugh. Spent some more time looking at this, still don't know why it doesn't respect default_url_options.

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Nov 9, 2012

Contributor

It's because of this:

context.instance_eval do
  def url_options
    ActionMailer::Base.default_url_options
  end
end unless context.request

If you take it out it works fine. I'm going to look into it further to work out why we need to do this. I think part of the problem might be this:

def self.setup_action_mailer(component)
  include Draper::ViewContext
end

which should be

def self.setup_action_mailer(component)
  component.class_eval do
    include Draper::ViewContext
  end
end

but I will investigate further.

Contributor

haines commented Nov 9, 2012

It's because of this:

context.instance_eval do
  def url_options
    ActionMailer::Base.default_url_options
  end
end unless context.request

If you take it out it works fine. I'm going to look into it further to work out why we need to do this. I think part of the problem might be this:

def self.setup_action_mailer(component)
  include Draper::ViewContext
end

which should be

def self.setup_action_mailer(component)
  component.class_eval do
    include Draper::ViewContext
  end
end

but I will investigate further.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 9, 2012

Member

So, I had a hunch that this was it, but when I took it out, it DIDNT work in the test app.

The git history should show this pretty cleanly... I forget exactly why this was added, but I think it was to help with this kind of thing. If the tests all pass, please submit a pull request. :D

Member

steveklabnik commented Nov 9, 2012

So, I had a hunch that this was it, but when I took it out, it DIDNT work in the test app.

The git history should show this pretty cleanly... I forget exactly why this was added, but I think it was to help with this kind of thing. If the tests all pass, please submit a pull request. :D

@haines

This comment has been minimized.

Show comment
Hide comment
@haines

haines Nov 9, 2012

Contributor

Will do! I'll put some integration tests together first so I don't break anything. This view context stuff is tricky!!

Contributor

haines commented Nov 9, 2012

Will do! I'll put some integration tests together first so I don't break anything. This view context stuff is tricky!!

@kangguru

This comment has been minimized.

Show comment
Hide comment
@kangguru

kangguru Jul 30, 2014

I'm having a similar problem when helper-specs are executed before the request-specs, i guess because the view_context gets cached in the helper specs and therefore looses the default_url_options set in the ApplicationController when running the request specs

config.before type: :request do
  Draper::ViewContext.clear!
end

is fixing the issue, but i'd be cool if draper could take care of it on its own :)

I'm having a similar problem when helper-specs are executed before the request-specs, i guess because the view_context gets cached in the helper specs and therefore looses the default_url_options set in the ApplicationController when running the request specs

config.before type: :request do
  Draper::ViewContext.clear!
end

is fixing the issue, but i'd be cool if draper could take care of it on its own :)

@jfragoulis

This comment has been minimized.

Show comment
Hide comment
@jfragoulis

jfragoulis Jun 15, 2016

The problem still exists. I am running on 2.1 and getting the wrong host from decorated models.

The problem still exists. I am running on 2.1 and getting the wrong host from decorated models.

@jfragoulis

This comment has been minimized.

Show comment
Hide comment
@jfragoulis

jfragoulis Jun 16, 2016

Found the solution. Override the controller setup (and the test request which overrides the default_url_options) by using the :fast build strategy for tests. Add this in your spec helper:

Draper::ViewContext.test_strategy :fast do
  include Rails.application.routes.url_helpers
end

Found the solution. Override the controller setup (and the test request which overrides the default_url_options) by using the :fast build strategy for tests. Add this in your spec helper:

Draper::ViewContext.test_strategy :fast do
  include Rails.application.routes.url_helpers
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment