Use save instead of update_columns in the OrderUpdater

[jhawthorn]

This uses a plan-old #save(validate: false) instead of using update_columns. A few changes were needed to make this work:

• Removing homogenize_line_item_currencies, which could cause an order update on an order save.
• Tweaking the Importer::Order so that order.contents.add wasn't called with an unsaved shipment on the order.

I don't think this would be considered a change to our public API, but it has some potential to cause issues in other stores. However, I think it's also a significant improvement, and would allow after_save and similar AR hooks to actually work as expected.

Looking for thoughts on:

• What is the point of homogenize_line_item_currencies? It was unit tested, but I don't understand what sort of store would need this behaviour.
• Can we make this change to the Updater.

[Senjai]

I'm 👍, having recently made changes so that attribute changes are present for the order updater hooks:

• FIx a stale data issue when using Spree::OrderCapturing. #407
• Use the order updater when shipping shipments, or dealing with payments. #389

Ideally, I wouldn't want to register an update hook with order here, but seeing as though the states I was interested in were only changing with update_columns, there was no other way I could add behavior around situations where those states change.

Changing this to save would allow me to use Active Record hooks instead, which is considered the "proper" approach for the specific problem I was attempting to solve.

I also have no idea what homogenize_line_item_currencies is really for.

[gmacdougall]

This is a great step in the right direction. 👍

[magnusvk]

Well, we're not multi-currency, so we don't care about homogenize_line_item_currencies. From reading the spec, it seems that method is intended to support switching currencies on an order. So if you flip an order from USD to EUR, say, it would also flip all the line items to EUR if there's a EUR price available or raise an error otherwise. That sounds like sensible behavior to me, but it's certainly not something we rely on.

Anyhow, I'm super excited about eliminating updates without callbacks, so 👍 on this approach. Still a little nervous about the potential impact of this. Perhaps we should put this on a dev box and see how it works for us?

[Senjai]

I would expect that the most significant issues would come with:

• Callbacks now being triggered when they weren't before on orders.
• If anyone uses update! in a scenario when the order is saved, a stack level too deep error could occur where it wouldn't before.
• Mystery currency stuff Magnus talked about.

[jhawthorn]

Another potential for issues: unpersisted associated records will now be saved on an update!

[mtomov]

To help a bit with the CurrencyUpdater mystery. At woolandthegang, we were dealing with 3 currencies. There was a Spree extension for switching currency, which was effectively changing the session[:currency] variable.

The session[:currency] variable is returned from the #current_currency method and used below through the #current_order_params to search for a previous order by token.

So, no change of order currency is happening at the initial current_order lookup. Now, the other place I was thinking where this one can be called from is the #merge! method on Order. However, you will see that this is only merging line items with the same currency.

So, to my experience, there is no code or interface in Spree to actually change the currency of an order.

Furthermore that concern is so fundamentally broken:

• business logic class is persisting data (no single responsibility .. )
• after_update with an if condition on Order.rb is missing tests
• the after_update "catch all" is clearly hard to reason about. This class should be explicitly called from where the actual currency change is happening.

So, either delete it, or change it to only assign changes to the Order without persistence. I'm favouring deletion.

[gmacdougall]

As the person who wrote most of the initial multi-currency code, I should probably weigh in here.

The intent when I was designing the multi-currency system was to explicitly not allow people to switch the currency of an order. If you switch currencies, you get a new order. This avoids all of the problems that homogenize_line_item_currencies tries to solve, but doesn't do a particularly good job of.

I am in favour of removing it, with a note in the release notes to explain why it has gone.

[magnusvk]

Does this still look like something we'd like to ship? It does to me!

[mtomov]

I don't see why not. Maybe @jhawthorn is a bit out of time.

[jhawthorn]

Rebased. This is still good to go by me.

[jhawthorn]

Rebased again

[cbrunsdon]

I am pretty scared by the change of the persist_totals call.

I'm very pro this change overall, but aren't comfortable merging it in just before we get 1.2 ready, so @jhawthorn has agreed to merge it very early in 1.3.

Stores that heavily abuse callbacks could get affected by the extra ones running on the .save, and if there were problems caused by that change they might take a while to figure out something wrong is even happening, with potential large impact to a running store.

In short: 👍 for 1.3

[jhawthorn]

Merging. We want this to get as much testing and exposure as possible before our 1.3 release.