search

nebulab / solidus

Solidus, Rails eCommerce System
http://solidus.io/
Other
7 stars 3 forks source link

Do not destroy orders when merging #37

Closed spaghetticode closed 2 years ago

spaghetticode commented 4 years ago

See issue https://github.com/solidusio/solidus/issues/1449

The implementation we want is the one outlined in this comment: https://github.com/solidusio/solidus/issues/1449#issuecomment-565799493

spaghetticode commented 4 years ago

One main concern is the fact that we cannot have the usual state machine methods, if we use the merged state, as Spree::Order already defines the merge!(*args) method. But I think this is not blocking. We still can use the merge method that does not save the state change, and let the existing merge! method do what did before and also change the merged order state.

Other options looked less straightforward to me, and they generally implied some deprecation of the existingmerge! method with params which is not necessary with the current solution and more code rework.

spaghetticode commented 4 years ago

Since we don't want merged orders to show in reports, it may make sense to remove them from https://github.com/solidusio-contrib/solidus_reports once this feature is merged in solidus.

spaghetticode commented 4 years ago

PR opened: https://github.com/solidusio/solidus/pull/3486 There are a few tests failing, they seem to be unrelated to the changes to me 🤔

spaghetticode commented 4 years ago

PR is green. We discussed it during the last Solidus core meeting, Gregor proposed a rather drastic approach to the issue that this PR try to address. As the order merge functionality is so problematic when the order has line items, we should simply not merge significant orders (ie orders with line items, but what makes an order "significant" may differ from store to store).

While that solution has merits, I think it's not backward compatible, as it would change the current behavior for all existing stores, so I'm not sure that we should simply not provide the fix from the above-mentioned PR at all.

I'd like to discuss this topic with the rest of the Nebulab core team (@kennyadsl, @aldesantis) and the other members of the solidus-core squad.