Merge pull request #6740 from Matt-Yorkley/carts-cleanup

Remove old abandoned carts

app/models/spree/option_values_line_item.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+module Spree
+  class OptionValuesLineItem < ActiveRecord::Base
+    belongs_to :line_item, class_name: 'Spree::LineItem'
+    belongs_to :option_value, class_name: 'Spree::OptionValue'
+  end
+end

config/schedule.rb

@@ -11,7 +11,7 @@ env "MAILTO", app_config["SCHEDULE_NOTIFICATIONS"] if app_config["SCHEDULE_NOTIF
 job_type :run_file, "cd :path; :environment_variable=:environment bundle exec script/rails runner :task :output"
 job_type :enqueue_job,  "cd :path; :environment_variable=:environment bundle exec script/enqueue :task :priority :output"
 
-every 1.month do
+every 1.month, at: '4:30am' do
   rake 'ofn:data:remove_transient_data'
 end
 

db/migrate/20210127174120_add_cascading_deletes.rb

@@ -0,0 +1,20 @@
+class AddCascadingDeletes < ActiveRecord::Migration
+  def change
+    # Updates foreign key definitions between orders, shipments, and inventory_units
+    # to allow for cascading deletes at database level. If an order is intentionally
+    # deleted *without callbacks*, it's shipments and inventory units will be removed
+    # cleanly without throwing foreign key errors.
+
+    remove_foreign_key :spree_shipments, name: "spree_shipments_order_id_fk"
+    add_foreign_key :spree_shipments, :spree_orders, column: 'order_id',
+                    name: 'spree_shipments_order_id_fk', on_delete: :cascade
+
+    remove_foreign_key :spree_inventory_units, name: 'spree_inventory_units_shipment_id_fk'
+    add_foreign_key :spree_inventory_units, :spree_shipments, column: 'shipment_id',
+                    name: 'spree_inventory_units_shipment_id_fk', on_delete: :cascade
+
+    remove_foreign_key :spree_inventory_units, name: 'spree_inventory_units_order_id_fk'
+    add_foreign_key :spree_inventory_units, :spree_orders, column: 'order_id',
+                    name: 'spree_inventory_units_order_id_fk', on_delete: :cascade
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20210125123000) do
+ActiveRecord::Schema.define(version: 20210127174120) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1252,9 +1252,9 @@ ActiveRecord::Schema.define(version: 20210125123000) do
   add_foreign_key "proxy_orders", "subscriptions", name: "proxy_orders_subscription_id_fk"
   add_foreign_key "spree_addresses", "spree_countries", column: "country_id", name: "spree_addresses_country_id_fk"
   add_foreign_key "spree_addresses", "spree_states", column: "state_id", name: "spree_addresses_state_id_fk"
-  add_foreign_key "spree_inventory_units", "spree_orders", column: "order_id", name: "spree_inventory_units_order_id_fk"
+  add_foreign_key "spree_inventory_units", "spree_orders", column: "order_id", on_delete: :cascade
   add_foreign_key "spree_inventory_units", "spree_return_authorizations", column: "return_authorization_id", name: "spree_inventory_units_return_authorization_id_fk"
-  add_foreign_key "spree_inventory_units", "spree_shipments", column: "shipment_id", name: "spree_inventory_units_shipment_id_fk"
+  add_foreign_key "spree_inventory_units", "spree_shipments", column: "shipment_id", on_delete: :cascade
   add_foreign_key "spree_inventory_units", "spree_variants", column: "variant_id", name: "spree_inventory_units_variant_id_fk"
   add_foreign_key "spree_line_items", "spree_orders", column: "order_id", name: "spree_line_items_order_id_fk"
   add_foreign_key "spree_line_items", "spree_variants", column: "variant_id", name: "spree_line_items_variant_id_fk"
@@ -1290,7 +1290,7 @@ ActiveRecord::Schema.define(version: 20210125123000) do
   add_foreign_key "spree_roles_users", "spree_roles", column: "role_id", name: "spree_roles_users_role_id_fk"
   add_foreign_key "spree_roles_users", "spree_users", column: "user_id", name: "spree_roles_users_user_id_fk"
   add_foreign_key "spree_shipments", "spree_addresses", column: "address_id", name: "spree_shipments_address_id_fk"
-  add_foreign_key "spree_shipments", "spree_orders", column: "order_id", name: "spree_shipments_order_id_fk"
+  add_foreign_key "spree_shipments", "spree_orders", column: "order_id", on_delete: :cascade
   add_foreign_key "spree_state_changes", "spree_users", column: "user_id", name: "spree_state_changes_user_id_fk"
   add_foreign_key "spree_states", "spree_countries", column: "country_id", name: "spree_states_country_id_fk"
   add_foreign_key "spree_tax_rates", "spree_tax_categories", column: "tax_category_id", name: "spree_tax_rates_tax_category_id_fk"

lib/tasks/data/remove_transient_data.rb

@@ -1,7 +1,8 @@
 # frozen_string_literal: true
 
 class RemoveTransientData
-  RETENTION_PERIOD = 6.months.ago.to_date
+  MEDIUM_RETENTION = 6.months.ago.to_date
+  SHORT_RETENTION = 3.months.ago.to_date
 
   # This model lets us operate on the sessions DB table using ActiveRecord's
   # methods within the scope of this service. This relies on the AR's
@@ -12,8 +13,34 @@ class RemoveTransientData
   def call
     Rails.logger.info("#{self.class.name}: processing")
 
-    Spree::StateChange.where("created_at < ?", RETENTION_PERIOD).delete_all
-    Spree::LogEntry.where("created_at < ?", RETENTION_PERIOD).delete_all
-    Session.where("updated_at < ?", RETENTION_PERIOD).delete_all
+    Spree::StateChange.where("created_at < ?", MEDIUM_RETENTION).delete_all
+    Spree::LogEntry.where("created_at < ?", MEDIUM_RETENTION).delete_all
+    Session.where("updated_at < ?", SHORT_RETENTION).delete_all
+
+    clear_old_cart_data!
+  end
+
+  private
+
+  def clear_old_cart_data!
+    old_carts = Spree::Order.
+      where("spree_orders.state = 'cart' AND spree_orders.updated_at < ?", SHORT_RETENTION).
+      merge(orders_without_payments)
+
+    old_cart_line_items = Spree::LineItem.where(order_id: old_carts)
+    old_line_item_options = Spree::OptionValuesLineItem.where(line_item_id: old_cart_line_items)
+    old_cart_adjustments = Spree::Adjustment.where(order_id: old_carts)
+
+    old_cart_adjustments.delete_all
+    old_line_item_options.delete_all
+    old_cart_line_items.delete_all
+    old_carts.delete_all
+  end
+
+  def orders_without_payments
+    # Carts with failed payments are ignored, as they contain potentially useful data
+    Spree::Order.
+      joins("LEFT OUTER JOIN spree_payments ON spree_orders.id = spree_payments.order_id").
+      where("spree_payments.id IS NULL")
   end
 end

spec/lib/tasks/data/remove_transient_data_spec.rb

@@ -5,7 +5,8 @@ require 'tasks/data/remove_transient_data'
 
 describe RemoveTransientData do
   describe '#call' do
-    let(:retention_period) { RemoveTransientData::RETENTION_PERIOD }
+    let(:medium_retention) { RemoveTransientData::MEDIUM_RETENTION }
+    let(:short_retention) { RemoveTransientData::SHORT_RETENTION }
 
     before do
       allow(Spree::StateChange).to receive(:delete_all)
@@ -15,25 +16,57 @@ describe RemoveTransientData do
     end
 
     it 'deletes state changes older than rentention_period' do
-      Spree::StateChange.create(created_at: retention_period - 1.day)
+      Spree::StateChange.create(created_at: medium_retention - 1.day)
 
       RemoveTransientData.new.call
       expect(Spree::StateChange.all).to be_empty
     end
 
     it 'deletes log entries older than retention_period' do
-      Spree::LogEntry.create(created_at: retention_period - 1.day)
+      Spree::LogEntry.create(created_at: medium_retention - 1.day)
 
       expect { RemoveTransientData.new.call }
         .to change(Spree::LogEntry, :count).by(-1)
     end
 
     it 'deletes sessions older than retention_period' do
-      RemoveTransientData::Session.create(session_id: 1, updated_at: retention_period - 1.day)
+      RemoveTransientData::Session.create(session_id: 1, updated_at: short_retention - 1.day)
 
       RemoveTransientData.new.call
 
       expect(RemoveTransientData::Session.all).to be_empty
     end
+
+    describe "deleting old carts" do
+      let(:product) { create(:product) }
+      let(:variant) { product.variants.first }
+
+      let!(:cart) { create(:order, state: 'cart') }
+      let!(:line_item) { create(:line_item, order: cart, variant: variant) }
+      let!(:adjustment) { create(:adjustment, order: cart) }
+
+      let!(:old_cart) { create(:order, state: 'cart', updated_at: short_retention - 1.day) }
+      let!(:old_line_item) { create(:line_item, order: old_cart, variant: variant) }
+      let!(:old_adjustment) { create(:adjustment, order: old_cart) }
+
+      it 'deletes cart orders and related objects older than retention_period' do
+        RemoveTransientData.new.call
+
+        expect{ cart.reload }.to_not raise_error
+        expect{ line_item.reload }.to_not raise_error
+        expect{ adjustment.reload }.to_not raise_error
+
+        expect{ old_cart.reload }.to raise_error ActiveRecord::RecordNotFound
+        expect{ old_line_item.reload }.to raise_error ActiveRecord::RecordNotFound
+        expect{ old_adjustment.reload }.to raise_error ActiveRecord::RecordNotFound
+      end
+
+      it "removes any defunct line item option value records" do
+        line_item.delete
+
+        expect{ RemoveTransientData.new.call }.
+          to change{ Spree::OptionValuesLineItem.count }.by(-1)
+      end
+    end
   end
 end