Change order state to `merged` instead of destroying it when merging orders #3486

spaghetticode · 2020-01-24T12:57:06Z

Description

This is an (opinionated, see later) attempt at #1449.

Destroying orders when merging them is not ideal, as we lose the ability to debug what happened, which may be very important in some edge cases.

This implementation adds the merged state to Spree::Order and updates the Spree::OrderMerger class accordingly. The merged order is associated with the resulting order via the #merged_to_order association.

The opinionated part is I kept the existing Spree::Order#merge! functionality and interface rather than switching to the one provided by default by the state machine when adding the merge event, see https://github.com/solidusio/solidus/compare/master...nebulab:spaghetticode/order-merged-state?expand=1#diff-2c3e70899d4f22d0569021b01b0e307fR62 (though the auto-generatedSpree::Order#merge method is retained and used in Spree::OrderMerge).
Doing things differently would have required more extensive code changes, deprecations, and other tradeoffs.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

core/db/migrate/20200117125833_add_merged_to_order_id.rb

core/app/models/spree/order_merger.rb

core/app/models/spree/order.rb

elia

👍 this is great, debugging merged orders is like trying to catch ghosts 👻

I left a couple suggestions/considerations.

elia · 2020-02-04T14:06:28Z

core/db/migrate/20200117125833_add_merged_to_order_id.rb

+class AddMergedToOrderId < ActiveRecord::Migration[5.2]
+  def change
+    add_column :spree_orders, :merged_to_order_id, :integer, limit: 4
+    add_foreign_key :spree_orders, :spree_orders, column: :merged_to_order_id


I would also consider adding a merged_at timestamp, especially when reconstructing weird edge cases that can be a precious information. In alternative the default order merger should at least add a note mentioning the date of the merge.

@elia I think that would be redundant. We have state_changes on orders, and since merged is a new state, a new record pops up after merging the order:

2.6.3 :001 > order2.state_changes => [#<Spree::StateChange:0x00007fea97cd4f90 id: 1, name: "order", previous_state: "cart", stateful_id: 2, user_id: 2, stateful_type: "Spree::Order", next_state: "merged", created_at: Fri, 07 Feb 2020 11:28:08 UTC +00:00, updated_at: Fri, 07 Feb 2020 11:28:08 UTC +00:00>]

elia · 2020-02-04T14:06:35Z

core/app/models/spree/order.rb

@@ -54,6 +54,9 @@ class CannotRebuildShipments < StandardError; end
    deprecate temporary_credit_card: :temporary_payment_source, deprecator: Spree::Deprecation
    deprecate :temporary_credit_card= => :temporary_payment_source=, deprecator: Spree::Deprecation

+    has_one :from_merged_order, foreign_key: :merged_to_order_id, class_name: 'Spree::Order', inverse_of: :merged_to_order


I recall that a single orders can be the result of merging multiple orders. Should this be a has_many?

solidus/core/lib/spree/core/controller_helpers/order.rb

Lines 60 to 66 in 9f99aa6

def set_current_order

if try_spree_current_user && current_order

try_spree_current_user.orders.by_store(current_store).incomplete.where('id != ?', current_order.id).each do |order|

current_order.merge!(order, try_spree_current_user)

end

end

end

@elia thank you for pointing this out, I did not realize this could happen as well.

When merging two orders together, one of them (the one passed as argument to `Spree::Order#merge!`) will now transition to the `merged` state. When an order is in `merged` state, it cannot transition to any other state. In order for an order to become merged, we need to set the new Active Record relation `#merged_to_order`. The association allows to keep a reference, for mostly for debugging purposes, to what order the merged order was merged to. The order that survives from the merge process has the corresponding inverse relation `#from_merged_order` that points to the merged order. One thing to keep in mind is that the order state machine should not define the `#merge!` method, as it's already defined for the `Spree::Order` class, see `Spree::Order#merge!(order)`. This is not actually an issue, as the existing `#merge!` method manages the state transition by leveraging the non-persisting method `#merge` that is implicitly defined by the state machine. This little inconsistency allows to keep the existing interface, without needing to deprecate the existing `Spree::Order#merge!` interface and adding conditional checks during the transitional time on the method arity (the state machine version of `#merge!` will ignore any passed params).

Instead of destroying the merged order, the class `Spree::OrderMerger` now transition the order to the `merged` state.

Merged orders are basically dead orders, they don't have any meaningful information (line items are moved to the surviving order of the merge process ) so there's not much value in displaying them on the backend.

ericsaupe · 2020-02-25T21:54:14Z

core/app/models/spree/order.rb

@@ -179,6 +182,14 @@ def self.not_canceled
      where.not(state: 'canceled')
    end

+    def self.merged


Is there a reason these aren't scopes?

Only for consistency with the ::cancelled and ::not_canceled class methods defined just above.

mamhoff · 2020-02-28T11:23:17Z

We've come across this as well, but we have opted not to add more stuff to the order state machine (which IMO does way too many things already). What we did was create a frontend_viewable boolean column on the orders table. Admin-created orders get that set to false, and only "frontend viewable" orders can be merged.

spaghetticode · 2020-03-06T10:05:32Z

@mamhoff thanks for the feedback. I think you have a point about the state machine, though I don't see adding this new state as a big concern... still this solution goes in the opposite direction of simplifying the SM (BTW, I like your PR!).

I think one big plus for the state machine is that the merged order is locked down, so it will not transition to other states. IMHO this cannot be achieved in a similarly straight forward way by using other approaches.

But more in general, all the possible solutions to this issue have a problem in common: there are (irrelevant?) orders that are not destroyed anymore, and DB queries will start picking them up, unless stores actively remove them in their custom code. So, stopping to destroy these orders looks to me like a significant breaking change.

I'm wondering if, instead of providing a new default approach to the problem, we should just offer to stores a few alternative merging strategies, built-in in Solidus, like this one (and yours) and let them chose their preference. But I'm not sure that a full-featured solution can be limited to just setting one preference, for example the merge strategy class, as different solutions will also need different ways to exclude merged orders from DB queries... maybe an extension would be a more suitable place 🤔

Stale

fthobe · 2025-01-31T20:59:35Z

@kennyadsl has there been made any decision on this? I find merging orders a compelling feature but would like some guidance as it's not something we would extensively invest into. Merged Seems to be a desirable state, but I am not clear if it is wanted by core team.

kennyadsl · 2025-02-03T08:37:38Z

I'm ok with this change, but this is a big one and potentially very problematic in terms of backward compatibility, so I guess it need to go in the next major release (upon @solidusio/core-team consensus).

fthobe · 2025-02-03T09:15:18Z

I'm ok with this change, but this is a big one and potentially very problematic in terms of backward compatibility, so I guess it need to go in the next major release (upon @solidusio/core-team consensus).

I think there should be a way doing it without backwards compatibility issues as it is a new state. I find disappearing orders very concerning as I imagine that there are actually companies connecting solidus directly to xero or other fiscal instruments. Any way to keep a tap on this?

elia · 2025-02-04T15:51:03Z

@fthobe the non-breaking way of merging this would be to put it behind a configuration flag that's not active by default. That would be great although not always trivial.

In any case someone needs to update the PR and rebase on the latest version, so if anyone is willing to take up both task (rebasing and adding the configuration flag) I would be absolutely in favor 👍.

fthobe · 2025-02-04T16:03:40Z

@fthobe the non-breaking way of merging this would be to put it behind a configuration flag that's not active by default. That would be great although not always trivial.

A question came up:
As @spaghetticode said the merge state does not create any destinations from there, where do you see the breaking change other than keeping more orders in DB?

tvdeyen · 2025-02-04T22:09:18Z

Wait. Can we please step back a little bit and talk about the use case of this?

A bit of background on the usual user flow will be helpful I guess.

So, a yet anonymous user comes to the store and puts something in to the cart. A new order record gets created. This is an unusual, but powerful feature of Solidus. Even the cart is already an order record.

The user then either creates a new account or logs into an existing account. Either way the anonymous order (the cart from current session) gets merged with all incomplete orders (due to the fact this happens every time the user logs in it's usually one incomplete order). And the previous anonymous and now merged order gets destroyed, because it does not serve any purpose anymore. The cart has now been transferred into a users order.

If we now keep the anonymous order it will produce a lot of garbage in the database.

What is the use case of keeping them?

This behavior needs to be at least behind a feature flag or even in a separate order state machine. And users need to be informed about this behavior.

But in the first place I would love to hear about the benefits.

fthobe · 2025-02-04T22:29:32Z

Honestly, pure data collection and profiling.

after how many days does a cart lead to a purchase
how often a product leads to a registration but not to a purchase

Half of today's recommendation engines are either based on sessions or abandoned / concluded carts.

Actually I would go many steps further:

is there a desire to bring over the cart start date over to the new order
maybe even track line item events with timestamps

Throw it in data studio or tableau and you have some fascinating metrics to display.

There's a whole order life cycle and long term abandonment data story to tell here and I wouldn't want to drop any of it.

tvdeyen · 2025-02-05T06:49:14Z

All stores I work on use third party tools for that. Even if we would use the state machine for that we would need to track the order the anonymous cart has been merged in. If there is a need for that, you can implement it in your store or create an extension. The order merger class and the state machine are configurable classes and easily adopted to specific needs that way.

fthobe · 2025-02-05T07:41:33Z

I can see your point why this can be done in my store, but I still struggle for any number of reasons with destroying any data without proper documentation. I feel taking up the decision to destroy data is massive compared to cost of storage.

A fully agree to keep a flag, a separate state machine I see as drastically complicating things (might be wrong though). Can we take a look at the current PR and see if it's salvageable (later this year) and put it behind a setting.

kennyadsl · 2025-02-05T07:44:18Z

If I recall correctly, the main driver for this change was that it's very hard to debug issues on orders that are merged and destroyed, so suddenly disappear from the database while still being present in logs prior to the merge.

spaghetticode · 2025-02-05T08:07:21Z

@kennyadsl you're right, that was the original driver for the PR.
We discussed the change internally a few times. We never reached a consensus about moving it forward - as others already said this is a significant behavior change, it's already a sensible default, and custom behavior can always be added on top of it. I see there's some interest in moving it forward, but due to the concerns I'm thinking it may end up in a small extension.

fthobe · 2025-02-05T10:48:51Z

What spaghetti Said.

fthobe · 2025-02-05T10:50:16Z

@tvdeyen A stupid question: Why is it not possible to Carry over the Order, why do we need to destroy it in the First Place?

tvdeyen · 2025-02-05T15:34:04Z

@tvdeyen A stupid question: Why is it not possible to Carry over the Order, why do we need to destroy it in the First Place?

I was not involved or even around when this was introduced into Spree decades ago. My guess is, because it was considered as not useful information, regarding that this is a stale anonymous cart after it has been merged. Without keeping track of where it has been merged into it does not make sense to keep, I guess. It leads to false data, because the cart content has been merged over to a new order and we do not know who it belongs to. It could give a false impression of having lots of abandoned carts, although they have been converted into a payed order later on.

fthobe · 2025-02-05T16:02:57Z

Funny enough this might be a thing that is not breaking if we just update the user of an order instead of destroying it, or am I wrong?

elia · 2025-02-05T16:05:22Z

@spaghetticode if I recall correctly we also had a case in which some payments were attached to the then destroyed order, that alone to me would be a good reason to reverse the default in the next major. Customization can still be implemented either to remove the new state or to add other behaviors.

Funny enough this might be a thing that is not breaking if we just update the user of an order instead of destroying it, or am I wrong?

No, I think this is triggered when you have an anonymous cart and you login to a customer account that also have a current cart, and you want to merge the items from both.

fthobe · 2025-02-05T16:13:46Z

Seems like only bad options here.

@tvdeyen can you live with hiding it behind a setting?

spaghetticode · 2025-02-05T16:23:56Z

@elia that's quite likely - the merged orders issue emerged multiple times while working on that particular store.
And thanks for pointing that out, I'd say that in general, anything attached to the order can become either an orphan record or get destroyed depending on how the association is configured.

fthobe · 2025-02-05T16:30:46Z

And thanks for pointing that out, I'd say that in general, anything attached to the order can become either an orphan record or get destroyed depending on how the association is configured.

Do you have some examples: I mean this applies only to uncompleted orders.

elia · 2025-02-05T16:40:48Z

@fthobe where exactly the payment happens can vary and I'm sure some older payment methods tried to mark an order as complete after the payment was successful. I think something else at that point caused the order to reset to the initial state and then it got merged.

It's admittedly an edge case but we really wished to have a better paper trail at the time.

tvdeyen · 2025-02-05T16:41:05Z

we also had a case in which some payments were attached to the then destroyed order

This seems to be a bug rather than anything else. Only incomplete orders get merged. I would like to not keep garbage just because there might be potential bugs in customers implementations. There are plenty of options (like paper_trail) to keep track of records changes.

fthobe · 2025-02-05T16:43:13Z

It would not be something we touch before June / July. Do we want to leave this open or is it definitively something that has no chance on merging?

elia · 2025-02-05T16:47:47Z

@tvdeyen it was due to how the state machine handles payments rather than a customer implementation bug, and the old concept that an order is complete only after a payment is attached to it.

We are not cleaning up abandoned carts by default, which is something that was needed in every single store I worked on eventually. In that case it's easy to add a recurring cleanup job that drops merged orders older than some date.

Do you see any negative impact besides having additional order data around?

tvdeyen · 2025-02-05T17:35:47Z

@tvdeyen it was due to how the state machine handles payments rather than a customer implementation bug, and the old concept that an order is complete only after a payment is attached to it.

We are not cleaning up abandoned carts by default, which is something that was needed in every single store I worked on eventually. In that case it's easy to add a recurring cleanup job that drops merged orders older than some date.

Do you see any negative impact besides having additional order data around?

No, if we make sure that the orders are clearly marked as what they are I have no objections.

An unobtrusive way could be to introduce a merged_into_order_id foreign key and maybe a merged_at timestamp, so that the data can be reasoned about. Using the state machine does not seem to make much sense to me.

fthobe · 2025-02-05T18:08:38Z

@tvdeyen it was due to how the state machine handles payments rather than a customer implementation bug, and the old concept that an order is complete only after a payment is attached to it.
We are not cleaning up abandoned carts by default, which is something that was needed in every single store I worked on eventually. In that case it's easy to add a recurring cleanup job that drops merged orders older than some date.
Do you see any negative impact besides having additional order data around?

No, if we make sure that the orders are clearly marked as what they are I have no objections.

An unobtrusive way could be to introduce a merged_into_order_id foreign key and maybe a merged_at timestamp, so that the data can be reasoned about. Using the state machine does not seem to make much sense to me.

Yes, that would be the absolute dream. Can we settle on that and gms lists this for the July tasklist?

fthobe · 2025-02-05T18:16:48Z

An unobtrusive way could be to introduce a merged_into_order_id foreign key and maybe a merged_at timestamp, so that the data can be reasoned about. Using the state machine does not seem to make much sense to me.

I think that was pretty much what was made in this PR
has_one :from_merged_order, foreign_key: :merged_to_order_id, class_name: 'Spree::Order', inverse_of: :merged_to_order

It was pointed out by @elia that merged could come from multiple orders.
I would like to point out that this could also make for a real nice feature later on to:

complete orders on the phone with customers
merge multiple stale orders
pay per link

Has lot of potential!

spaghetticode added the type:enhancement Proposed or newly added feature label Jan 24, 2020

spaghetticode self-assigned this Jan 24, 2020

houndci-bot reviewed Jan 24, 2020

View reviewed changes

spaghetticode force-pushed the spaghetticode/order-merged-state branch 2 times, most recently from 9cff561 to 6bace8f Compare January 24, 2020 16:49

spaghetticode mentioned this pull request Jan 31, 2020

Do not destroy orders when merging nebulab/solidus#37

Closed

spaghetticode force-pushed the spaghetticode/order-merged-state branch 5 times, most recently from 4bd51f5 to 8fbe989 Compare January 31, 2020 16:43

elia previously requested changes Feb 4, 2020

View reviewed changes

spaghetticode force-pushed the spaghetticode/order-merged-state branch 2 times, most recently from 87c5cc4 to ff17b85 Compare February 12, 2020 14:09

spaghetticode added 3 commits February 12, 2020 15:36

Transition order to merged state in OrderMerger

67536c4

Instead of destroying the merged order, the class `Spree::OrderMerger` now transition the order to the `merged` state.

Do not show merged orders in admin

9905650

Merged orders are basically dead orders, they don't have any meaningful information (line items are moved to the surviving order of the merge process ) so there's not much value in displaying them on the backend.

spaghetticode force-pushed the spaghetticode/order-merged-state branch from ff17b85 to 9905650 Compare February 12, 2020 14:36

spaghetticode added the Code Review Needed label Feb 14, 2020

ericsaupe reviewed Feb 25, 2020

View reviewed changes

aldesantis added this to the 2.11 milestone May 22, 2020

aldesantis added Needs Core Team Decision and removed Needs Core Team Review labels Jun 26, 2020

kennyadsl removed this from the 2.11 milestone Aug 28, 2020

waiting-for-dev added changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Aug 30, 2022

cesartalves mentioned this pull request Nov 22, 2022

Order Merging Deleting Orders in Backend Unexpectedly #1449

Open

waiting-for-dev removed the Needs Core Team Decision label Dec 1, 2022

	def set_current_order
	if try_spree_current_user && current_order
	try_spree_current_user.orders.by_store(current_store).incomplete.where('id != ?', current_order.id).each do \|order\|
	current_order.merge!(order, try_spree_current_user)
	end
	end
	end

Change order state to merged instead of destroying it when merging orders #3486

Are you sure you want to change the base?

Change order state to merged instead of destroying it when merging orders #3486

Conversation

spaghetticode commented Jan 24, 2020

elia left a comment

Choose a reason for hiding this comment

elia Feb 4, 2020

Choose a reason for hiding this comment

spaghetticode Feb 7, 2020

Choose a reason for hiding this comment

elia Feb 4, 2020

Choose a reason for hiding this comment

spaghetticode Feb 7, 2020

Choose a reason for hiding this comment

ericsaupe Feb 25, 2020

Choose a reason for hiding this comment

spaghetticode Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

mamhoff commented Feb 28, 2020

spaghetticode commented Mar 6, 2020

fthobe commented Jan 31, 2025

kennyadsl commented Feb 3, 2025

fthobe commented Feb 3, 2025

elia commented Feb 4, 2025

fthobe commented Feb 4, 2025

tvdeyen commented Feb 4, 2025

fthobe commented Feb 4, 2025 • edited Loading

tvdeyen commented Feb 5, 2025

fthobe commented Feb 5, 2025

kennyadsl commented Feb 5, 2025

spaghetticode commented Feb 5, 2025

fthobe commented Feb 5, 2025

fthobe commented Feb 5, 2025

tvdeyen commented Feb 5, 2025

fthobe commented Feb 5, 2025

elia commented Feb 5, 2025

fthobe commented Feb 5, 2025

spaghetticode commented Feb 5, 2025

fthobe commented Feb 5, 2025

elia commented Feb 5, 2025

tvdeyen commented Feb 5, 2025

fthobe commented Feb 5, 2025

elia commented Feb 5, 2025

tvdeyen commented Feb 5, 2025

fthobe commented Feb 5, 2025 • edited Loading

fthobe commented Feb 5, 2025

Change order state to `merged` instead of destroying it when merging orders #3486

Change order state to `merged` instead of destroying it when merging orders #3486

spaghetticode Mar 6, 2020 •

edited

Loading

fthobe commented Feb 4, 2025 •

edited

Loading

fthobe commented Feb 5, 2025 •

edited

Loading