Order totals stale after add variant #1595

Closed
wants to merge 2 commits into
from

3 participants

@mscottford

Calling Order#add_variant should update order totals. While the values are getting correctly updated in the database, the in-memory instance of the order is not getting updated. One solution is to call reload from within the Order#add_variant method.

@radar radar added a commit that referenced this pull request May 24, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Merges #1595
f69175d
@radar
Spree Commerce member

Merged into both master and 1-1-stable. Thanks!

@radar radar added a commit that referenced this pull request May 24, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Merges #1595
7868902
@radar radar closed this May 24, 2012
@radar radar added a commit that referenced this pull request May 24, 2012
@radar radar Revert "Fixes issue that causes order totals to be stale after callin…
…g Order#add_variant."

This reverts commit f69175d.

Re-opens #1595
23ba4cf
@radar radar reopened this May 24, 2012
@radar radar added a commit that referenced this pull request May 24, 2012
@radar radar Revert "Fixes issue that causes order totals to be stale after callin…
…g Order#add_variant."

This reverts commit 7868902.

Re-opens #1595
a1d3a4d
@radar
Spree Commerce member

This commit causes the api build to break for 1-1-stable and master. Can you look into it to see why?

@mscottford

Sorry about that. I was seeing a failure that I could not explain, because it was happening with and without the change to Spree::Order. I thought it was an environment issue on my end. I'll dig deeper.

@mscottford mscottford Changes order api test to use actual Spree::Variant objects instead o…
…f stubs, because of the interactions between Spree::LineItem and Spree::Order.
7c09662
@mscottford

Found the problem. I did not run the api test suite before I committed. One of the tests in there was using a stub for Spree::Variant which was not playing nice with Spree::Order#add_variant calling #reload internally. I changed the test to use an actual Spree::Variant object instead.

@radar
Spree Commerce member

Excellent, I'll merge this in now. Thanks!

@radar radar added a commit that referenced this pull request May 26, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Also, Changes order api test to use actual Spree::Variant objects instead of stubs, because of the interactions between Spree::LineItem and Spree::Order.

Merges #1595
d08be0f
@radar radar added a commit that referenced this pull request May 26, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Also, Changes order api test to use actual Spree::Variant objects instead of stubs, because of the interactions between Spree::LineItem and Spree::Order.

Merges #1595
7e06598
@radar radar closed this May 26, 2012
@radar radar added a commit that referenced this pull request May 29, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Merges #1595
0a995c0
@radar radar added a commit that referenced this pull request May 29, 2012
@radar radar Revert "Fixes issue that causes order totals to be stale after callin…
…g Order#add_variant."

This reverts commit 7868902.

Re-opens #1595
163cdde
@radar radar added a commit that referenced this pull request May 29, 2012
@mscottford mscottford Fixes issue that causes order totals to be stale after calling Order#…
…add_variant.

Also, Changes order api test to use actual Spree::Variant objects instead of stubs, because of the interactions between Spree::LineItem and Spree::Order.

Merges #1595
a864a6a
@maximkulkin

This fix is not necessary. First, it still doesn't handle cases when line items are updated. Second, IMO, reloading object from inside that object is a bad practice and can lead to unexpected side effects (when suddenly some changes to order disappear while updating line items).
In fact, totals are perfectly updated if ActiveRecord's Identity Map feature is enabled. Just put following line in you config/application.rb and you are good to go: config.active_record.identity_map = true. This, of course, requires Rails 3.1 or higher.

@mscottford

Hi @maximkulkin, I'd encourage you to try to craft another test case that illustrates the issue you seem to have found when line items are updated. This fix addresses the issue that I found. It might not be good enough to fix the issue you found, though. It sounds like you should create a new issue either way.

I found that reloading the object was needed because there are side effects that make updates to the database without the order object getting notified about them.

I don't know enough about ActiveRecord's Identity Map to have an opinion either way, but I do remember lots of discussion about situations where it can be dangerous during a session at RailsConf 2012. Does the entire test suite pass when it's turned on? If you'd like to explore that further, I suggest that you create a different issue for it.

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