Replace acts_as_paranoid/paranoia with discard

[jhawthorn]

Our acts_as_paranoid models have been the cause of a lot of pain and confusion.

paranoia overrides activerecord's delete and destroy methods, which can surprise users expecting a real delete, but also surprises users when a record isn't declared acts_as_paranoid (or they use a method acts_as_paranoid doesn't override, like delete_all). It also has callback issues, dependent: :destroy and any other after_destroy callbacks happen on a soft-destroy or a real destroy.

Ideally, these would all be switched to use discard, my extremely simple alternative: <40 LOC and no ActiveRecord overrides. But it's in no way a drop-in replacement.

A possible stepping stone is to switch to paranoia_delete first, which has much more similar semantics to discard (no dependent: :destroy callbacks run).

To start planning how to replace it I've cataloged all the models in core it is used on, and the effects either hard or soft deletes have.

• Product/Variant/StockItem/Price Convert product, variant, stock item, prices to discard #2396
• Promotion actions Convert promotion actions from paranoia to discard #2398
• Shipping methods Convert shipping methods, payment methods, and tax rates to discard #2487
• Payment methods Convert shipping methods, payment methods, and tax rates to discard #2487
• Tax rates Convert shipping methods, payment methods, and tax rates to discard #2487
• Store credits Convert store credits to discard #2489
• Stock Transfers (Removed by Extract stock transfers to solidus_stock_transfers extension #2379)

Product models

• Product
  • Hard-deletes: variants, product_promotion_rules, classifications, product_properties, product_option_types
  • Soft-deletes: variants
  • Orphaned on real destroy: variant_property_rules
• Variant
  • Hard-deletes: images
  • Soft-deletes: stock_items, prices
  • Orphaned on real destroy: inventory_units, line_items, variant_property_rules
  • Hard-deletes on real destroy: option_values_variants
• StockItem
  • Orphaned: stock_movements
• Price

This is the most complicated group of models. Also the most frequently used, with the most possible admin interactions.

I'm not 100% sure why Price is acts_as_paranoid, but I suspect it's just to prevent them being destroyed when a variant is soft-destroyed. Other than that I think it would be safe for them to be hard-destroyed. Maybe there is a need to keep them around for auditing (in which case we should represent them better and show the "historic" prices in the admin)

Product and Variant should only ever be soft-destroyed. Soft-destroying a product will soft-destroy all the variants, which seems reasonable (although makes it harder to distinguish variants which were removed from a product from variants whose product was deleted. Might not matter.)

I'm not sure about StockItem deletion. It happens when a variant is deleted, but I'm not sure there it is desirable outside of that. The API allows it, but we have no interface in the admin, and it is never done.

Payment Method

• PaymentMethod
  • Orphaned on real destroy: payments, credit_cards, store_payment_methods

These should only ever be soft-destroyed.

There are no dependent: :destroys so we should be able to switch to discard.

This is inheritable, so there may be additional issues.

Shipping Method

• ShippingMethod
  • Hard-deletes: shipping_method_categories, shipping_method_zones, shipping_method_stock_locations
  • Orphaned on real destroy: shipping_rates, cartons

These should only ever be soft-destroyed.

I'm not sure if the dependent: destroys are desirable here, they seem to be
part of the information we might want to keep, but it might make for faster
queries coming from the other direction (needs more consideration).

These are a good candidate for paranoia_delete or discard

Taxation

• TaxCategory
  • Hard-deletes: tax_rate_tax_categories
  • Orphaned on real destroy: products
• TaxRate
  • Hard-deletes: tax_rate_tax_categories
  • Orphaned on real destroy: shipping_rate_taxes, adjustments

These both have potential leave orphaned records on a hard-destroy, and so are
only suitable for a soft destroy.

I'm not sure if the destroy on tax_rate_tax_categories is necessary, but it
might make querying for tax rates faster (needs more consideration)

Promotion Actions

• CreateAdjustment
  • Orphaned on real destroy: adjustments
• CreateItemAdjustments
  • Orphaned on real destroy: adjustments
• CreateQuantityAdjustments
  • Hard-deletes: line_item_actions
• FreeShipping
  • Orphaned on real destroy: adjustments

These only use a soft-destroy.

Promotion actions aren't explicitly delted, but are deleted through assignment
(promotion.promotion_actions=). This makes it hard to change the behaviour.

Other than that they would be a good candidate to switch to discard.

Store Credits

• StoreCredit
  • Orphaned on real destroy: store_credit_events
  • Validation: Can't delete if it has a positive value
• StoreCreditEvent

Neither of these have any dependent: :destroys, so we can probably just switch to either paranoia_delete or discard

Stock Transfers (to be removed in #2379)

• StockTransfer
  • Orphaned on real destroy: stock_movements, transfer_items
  • Validation: Can't delete if transfer is finalized
• TransferItem
  • Validation: Can't delete if transfer is finalized

We might be extracting stock transfers to an extension.

These can probably both just be hard-deleted (unless they have been finalized or have a stock movement).

[jordan-brough]

Great summary. I think it'd be great to figure out how to move things over to discard.

[cbrunsdon]

Aye, I've been burned too many times by our paranoia stuff to want to keep it around. I imagine most of these are individually not giant changes, and there isn't any reason we couldn't try it with one model first and see how much work its going to be, right?

I also can't imagine too many cases where extensions are really going to care about what we're using for deletion.

[aitbw]

I think this can be safely closed 🎉