Use in-memory adjustments in OrderUpdater

[jhawthorn]

From @mtomov's suggestion in #1389

This currently fails because this line in ItemAdjustments causes the order's adjustments to become stale.

[jrochkind]

That's an confusing comment at the line @jhawthorn mentions "This is done intentionally to avoid loading the association." But it doesn't avoid loading the association at all, it does load the association... then caches it.

Since it says its' working around a bug elsewhere in spree, and when that bug is remedied it should be removed.... I wonder if that mysterious bug elsewhere in spree is covered by tests, so if the tests still pass it means that line isn't needed?

[cbrunsdon]

I think this is a good change ( 👍 ) and would be down with killing that memoization

[jrochkind]

Rails caches/memoizes associations itself of course -- it's unclear why ordinary AR caching isn't sufficient here.

[jhawthorn]

Running assoc.all.to_a is different than assoc.to_a in that it does not load and cache the adjustments association.

> item = Spree::LineItem.last
> item.adjustments.loaded?
=> false
> item.adjustments.all.to_a
Spree::Adjustment Load (0.6ms)  SELECT "spree_adjustments".* FROM ...
[#<Spree::Adjustment:...
> item.adjustments.loaded?
=> false
> item.adjustments.all.to_a
Spree::Adjustment Load (0.6ms)  SELECT "spree_adjustments".* FROM ...
[#<Spree::Adjustment:...

> item.adjustments.to_a
Spree::Adjustment Load (0.6ms)  SELECT "spree_adjustments".* FROM ...
[#<Spree::Adjustment:...
> item.adjustments.loaded?
=> true
> item.adjustments.to_a # (cached. no query)
[#<Spree::Adjustment:...

There's a few dozen spec failures when using the association directly.

The reason this is done is that there are a number of things that operate on adjustments without going through the association.. A simple example is this line in create_item_adjustments.rb.

We'll have to ensure everything that operates on adjustments does so through the items association and keeps it fresh.

[jrochkind]

Ah, I see, I missed that all and it's meaning. Hmm.

Would it be at all useful for some places that want to operate directly without using the association to call adjustments.reset after doing so, so next time the association is loaded it will be fresh?

But that still requires individual discovery and attention to every place that does this, and might not be a good idea anyway.

[cbrunsdon]

Looks good to me 👍

[mtomov]

To me as well. On the second line change you might as well add a question mark after eligible - select(&:eligible?) - to make it consistent

[jhawthorn]

To me as well. On the second line change you might as well add a question mark after eligible - select(&:eligible?) - to make it consistent

Good catch. I made this change.