solidus icon indicating copy to clipboard operation
solidus copied to clipboard

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

Open spaghetticode opened this issue 4 years ago • 2 comments

Description

This is an (opinionated, see later) attempt at https://github.com/solidusio/solidus/issues/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:

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

spaghetticode avatar Jan 24 '20 12:01 spaghetticode

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.

mamhoff avatar Feb 28 '20 11:02 mamhoff

@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 🤔

spaghetticode avatar Mar 06 '20 10:03 spaghetticode