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

Notifications Error #526

Closed
r3ap3r2004 opened this Issue Jul 24, 2011 · 9 comments

Comments

Projects
None yet
5 participants
Contributor

r3ap3r2004 commented Jul 24, 2011

There is a problem with the call to fire_event('spree.order.contents_changed') in orders_controller.rb.

The problem is that the order details that get sent to the observer are the order details prior to the change.

An @order.update! is required before the call to fire_event('spree.order.contents_changed') in order for the correct contents to be sent to the observers.

If this isn't done in the core it will require every observer to force reload the order since they won't be able to ensure that the values they have are correct or not.

This change needs to happen inside of:
orders_controller.rb => update
orders_controller.rb => populate
spree_promo.rb => update

NOTE: Simply passing {:order => @order} into the fire_event as extra_payload doesn't work because @order itself contains the old values.

Chris

Contributor

modology commented Aug 4, 2011

Pull request is welcome

Contributor

r3ap3r2004 commented Aug 5, 2011

The reason that I didn't make a pull request to begin with is that I'm not 100% positive that this will not cause any issues. I just started looking at the notifications and found the problem when I actually tried to use them. I'm happy to make the request, but the change is a single line of code in three files. It would take just about as many keystrokes to make the fix as it would to pull in a pull request. Unfortunately I only have a couple days a week that I can spend on Spree and today isn't one of them, so it will need to wait until next week.

I've actually never run the spree tests either (not exactly sure how), so I'm not sure if there is something that I'm missing that this would break. I'm pretty sure it wouldn't break anything, but I figured someone with a better knowledge of the notification system would be more qualified to make that determination.

Chris

Member

BDQ commented Aug 8, 2011

@r3ap3r2004 - To run spree's entire test suite, just type 'rake' from the root directory and sit back!

Contributor

r3ap3r2004 commented Aug 16, 2011

modology,
Have you actually tested your fix. I'm not somewhere that I can test right now (and to be honest I modified how I approached the problem so I am no longer using the notifications), but when I tried doing it the way you did it didn't work because you are still passing the outdated order info, hence my original 'NOTE' comment.

Using your fix the line items get updated, but the order totals, etc aren't updated yet. As a result every extension that is monitoring the notification is required to do an @order.update! to ensure that they are working with the proper values. This causes the method to be called multiple times instead of just once before the fire_event('spree.order.contents_changed') notification.

The test is to listen to the notification and then look at the order totals. They won't be correct (at least they weren't when I tested your method).

Member

radar commented Feb 2, 2012

Is this still an issue?

Contributor

gmile commented Mar 21, 2012

Yes I can confirm this as of 1.1.0beta

Member

radar commented Mar 21, 2012

Could you please provide some steps to reproduce it?

Member

radar commented May 3, 2012

I will be closing this issue in a week's time if there is no steps provided to reliably reproduce it.

@radar radar closed this May 3, 2012

@radar radar reopened this May 3, 2012

Contributor

gmile commented May 3, 2012

I actually had no luck reproducing the bug since then. Close or keep the issue opened at will.

@r3ap3r2004 r3ap3r2004 closed this May 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment