Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Redirect to order_path instead of cart_path when adding item, updating an order #1301

Closed
cwise opened this Issue Mar 21, 2012 · 8 comments

Comments

Projects
None yet
4 participants
Contributor

cwise commented Mar 21, 2012

I've just made the jump to Spree 1.1beta and am getting really odd behaviour. When adding an item, I am no longer going to the cart_path (orders#edit) but am instead redirected to the order summary page (orders#show). It's really confusing as it doesn't appear that the OrdersController has changed much from 0.70.

When calling the standard "Add to Cart" functionality (order#populate) on the product#show page or any action that should result in a redirect_to cart_path (order#update etc) ends up redirecting to order#show instead.

I added some logging to the ApplicationController to try to track down who might be calling redirect_to:

...
Redirected by /Volumes/smyth2_users/Users/cwise/.rvm/gems/ruby-1.9.2-p290/gems/actionpack-3.2.2/lib/action_controller/metal/responder.rb:134:in `redirect_to'
Redirected to http://demo.local:3000/orders/R443613505

The routes all appear to be correct as far as I can tell. Going to /cart renders order#edit as expected. It's really perplexing. I'm going to try to dig deeper to find where the redirect is coming from.

In OrdersController (update) when:

respond_with(@order) { |format| format.html { redirect_to cart_path } }

I went and created a new app using Rails 3.2.2 and Spree edge - no issues. I then updated our app to Spree edge (hadn't for a few days - bundle update spree) and I still get the problem with the faulty redirect. There are no responder overrides in our application and the Orders controller hasn't been extended or anything like that.

Contributor

cwise commented Mar 21, 2012

I've located some of the source of the issue. We do in fact have a custom responder on the Admin::OrdersController which is not directly related to this controller. It's presence does however cause a problem.

In ActionController::Base respond_with there is the following line:

    if defined_response = collector.response and Spree::BaseController.spree_responders.empty?

Basically if you have ANY respond_overrides, you'll fail the second condition and defined_response.call will never be called.

I commented out the respond_override and started the app again. It works correctly!

So the issue is that there's still an issue and this method might possibly need a little love.

Member

radar commented Mar 22, 2012

What happens if you change Spree::BaseController to self.class? Does that make it stop doing that?

I could be going down the wrong path here... Do you have a way (in steps form) that I could reproduce this issue on my machine?

Contributor

cwise commented Mar 22, 2012

Unfortunately not. Going through the debugger I see it's the same hash w/ the unrelated overrides:

(rdb:2) self.class.spree_responders

{:"Spree::Admin::OrdersController"=>{:show=>{:pdf=>{:success=>#<Proc:0x000001034d7598@/Volumes/smyth2_users/Users/cwise/RubyApps/nd_spree/app/controllers/admin/orders_controller_decorator.rb:5 (lambda)>}}}, :"Spree::Admin::PaymentMethodsController"=>{:edit=>{:html=>{:success=>#<Proc:0x00000102f123d8@/Volumes/smyth2_users/Users/cwise/RubyApps/nd_spree/app/controllers/admin/payment_methods_controller_decorator.rb:2 (lambda)>}}}}

(rdb:2) self.class
Spree::OrdersController

Contributor

cwise commented Mar 23, 2012

This seems to work for both cases (on controllers with overrides and those without).

    if defined_response = collector.response and !Spree::BaseController.spree_responders.include?(:self.class.to_s)

I apologize that I don't have tests to go along with that as I am not even 100% sure how that mechanism is supposed to work with respond_with overall.

Member

iloveitaly commented May 7, 2012

I'm having this exact issue.

To reproduce the issue install the spree_volume_pricing extension. I'm pretty sure this file's respond_override is causing the issue: https://github.com/spree/spree_volume_pricing/blob/master/app/controllers/spree/admin/variants_controller_decorator.rb

This is what is causing #1506 as well. Removing spree_volume_pricing fixes the routing errors.

Member

radar commented May 7, 2012

Does the respond_override do anything useful at all or can we just get rid of it?

Member

iloveitaly commented May 7, 2012

I'm not sure... I've only made a couple tweaks to the spree_volume_pricing extension; no idea what respond_override is doing.

Member

JDutil commented May 8, 2012

@radar the respond_override is being used as a way for extensions to modify a controllers behavior. I had the same thought when it caused bugs upgrading to Rails 3.2. There are some Specs for the behavior here: https://github.com/spree/spree/blob/master/core/spec/controllers/spree/orders_controller_extension_spec.rb

If @cwise solution works with those specs still passing that's great, but a regression spec would definitely be best.

@JDutil JDutil added a commit to JDutil/spree that referenced this issue May 8, 2012

@JDutil @JDutil JDutil + JDutil Only use Spree::Responder if needed for the current controller.
Stops preventing defined_response.call when any spree_responders are
defined. [fixes #1301]
f80facc

@radar radar closed this in 3a6e8c3 May 8, 2012

@tvdeyen tvdeyen pushed a commit to magiclabs/spree that referenced this issue Jun 1, 2012

@JDutil @radar JDutil + radar Only use Spree::Responder if needed for the current controller.
Stops preventing defined_response.call when any spree_responders are
defined. [fixes #1301]

Merges #1515
2db1929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment