From 4230d46a065a0ae96e2d1930c53cf0fdfd4d2d24 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 25 Jan 2021 17:25:53 +0000
Subject: [PATCH 1/8] Remove carts older than 6 months

---
 lib/tasks/data/remove_transient_data.rb       |  9 +++++++
 .../tasks/data/remove_transient_data_spec.rb  | 25 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index 96eaef027b..b3716efb10 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/lib/tasks/data/remove_transient_data.rb
@@ -15,5 +15,14 @@ class RemoveTransientData
     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
+
+    # Clear old carts and associated records
+    old_carts = Spree::Order.where("state = 'cart' AND updated_at < ?", RETENTION_PERIOD)
+    old_cart_line_items = Spree::LineItem.where(order_id: old_carts)
+    old_cart_adjustments = Spree::Adjustment.where(order_id: old_carts)
+
+    old_cart_adjustments.delete_all
+    old_cart_line_items.delete_all
+    old_carts.delete_all
   end
 end
diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb
index 9e61b3f65b..5fe6056edc 100644
--- a/spec/lib/tasks/data/remove_transient_data_spec.rb
+++ b/spec/lib/tasks/data/remove_transient_data_spec.rb
@@ -35,5 +35,30 @@ describe RemoveTransientData do
 
       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: retention_period - 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
+    end
   end
 end

From 0a88712926dd0c145d90cb3d17f3c3b793ad1ba2 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 14:44:34 +0000
Subject: [PATCH 2/8] Clear orphaned records in join table
 spree_option_value_line_items

---
 lib/tasks/data/remove_transient_data.rb       | 11 +++++++++++
 .../tasks/data/remove_transient_data_spec.rb  | 19 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index b3716efb10..f15514a846 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/lib/tasks/data/remove_transient_data.rb
@@ -24,5 +24,16 @@ class RemoveTransientData
     old_cart_adjustments.delete_all
     old_cart_line_items.delete_all
     old_carts.delete_all
+
+    # Clear option values for deleted line items
+    ActiveRecord::Base.connection.execute <<-SQL
+      DELETE FROM spree_option_values_line_items
+      WHERE line_item_id IN (
+        SELECT line_item_id FROM spree_option_values_line_items
+        LEFT OUTER JOIN spree_line_items
+        ON spree_option_values_line_items.line_item_id = spree_line_items.id
+        WHERE spree_line_items.id IS NULL
+      );
+    SQL
   end
 end
diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb
index 5fe6056edc..3cb0f7029a 100644
--- a/spec/lib/tasks/data/remove_transient_data_spec.rb
+++ b/spec/lib/tasks/data/remove_transient_data_spec.rb
@@ -59,6 +59,25 @@ describe RemoveTransientData do
         expect{ old_line_item.reload }.to raise_error ActiveRecord::RecordNotFound
         expect{ old_adjustment.reload }.to raise_error ActiveRecord::RecordNotFound
       end
+
+      context "removing defunct line item option value records" do
+        let(:connection) { ActiveRecord::Base.connection }
+        let(:query) {
+          <<-SQL
+            SELECT * FROM spree_option_values_line_items
+            LEFT OUTER JOIN spree_line_items
+            ON spree_option_values_line_items.line_item_id = spree_line_items.id
+            WHERE spree_line_items.id IS NULL;
+          SQL
+        }
+
+        it "removes the records" do
+          line_item.delete
+
+          expect{ RemoveTransientData.new.call }.
+            to change{ connection.execute(query).count }.by(-1)
+        end
+      end
     end
   end
 end

From 3fddaba4bfd291e9f2ed054fd06bc03962a001b0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 14:47:10 +0000
Subject: [PATCH 3/8] Extract private methods

---
 lib/tasks/data/remove_transient_data.rb | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index f15514a846..db971543ee 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/lib/tasks/data/remove_transient_data.rb
@@ -16,7 +16,13 @@ class RemoveTransientData
     Spree::LogEntry.where("created_at < ?", RETENTION_PERIOD).delete_all
     Session.where("updated_at < ?", RETENTION_PERIOD).delete_all
 
-    # Clear old carts and associated records
+    clear_old_cart_data!
+    clear_line_item_option_values!
+  end
+
+  private
+
+  def clear_old_cart_data!
     old_carts = Spree::Order.where("state = 'cart' AND updated_at < ?", RETENTION_PERIOD)
     old_cart_line_items = Spree::LineItem.where(order_id: old_carts)
     old_cart_adjustments = Spree::Adjustment.where(order_id: old_carts)
@@ -24,8 +30,9 @@ class RemoveTransientData
     old_cart_adjustments.delete_all
     old_cart_line_items.delete_all
     old_carts.delete_all
+  end
 
-    # Clear option values for deleted line items
+  def clear_line_item_option_values!
     ActiveRecord::Base.connection.execute <<-SQL
       DELETE FROM spree_option_values_line_items
       WHERE line_item_id IN (

From e6c59fbd962faa5ac9d5ce87e7309cfa26d6e707 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 14:57:54 +0000
Subject: [PATCH 4/8] Update data retention periods

Sessions and cart data are removed if older than 3 months, instead of 6.
---
 lib/tasks/data/remove_transient_data.rb           | 11 ++++++-----
 spec/lib/tasks/data/remove_transient_data_spec.rb | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index db971543ee..d1ed670d39 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/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,9 +13,9 @@ 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!
     clear_line_item_option_values!
@@ -23,7 +24,7 @@ class RemoveTransientData
   private
 
   def clear_old_cart_data!
-    old_carts = Spree::Order.where("state = 'cart' AND updated_at < ?", RETENTION_PERIOD)
+    old_carts = Spree::Order.where("state = 'cart' AND updated_at < ?", SHORT_RETENTION)
     old_cart_line_items = Spree::LineItem.where(order_id: old_carts)
     old_cart_adjustments = Spree::Adjustment.where(order_id: old_carts)
 
diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb
index 3cb0f7029a..34811682c1 100644
--- a/spec/lib/tasks/data/remove_transient_data_spec.rb
+++ b/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,21 +16,21 @@ 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
 
@@ -44,7 +45,7 @@ describe RemoveTransientData do
       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: retention_period - 1.day) }
+      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) }
 

From 85c489d3034f0ce2469d8de8953448eba127cbb8 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 16:44:17 +0000
Subject: [PATCH 5/8] Ignore carts with failed payments in cleanup

---
 lib/tasks/data/remove_transient_data.rb | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index d1ed670d39..e4fe755f23 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/lib/tasks/data/remove_transient_data.rb
@@ -24,7 +24,10 @@ class RemoveTransientData
   private
 
   def clear_old_cart_data!
-    old_carts = Spree::Order.where("state = 'cart' AND updated_at < ?", SHORT_RETENTION)
+    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_cart_adjustments = Spree::Adjustment.where(order_id: old_carts)
 
@@ -44,4 +47,11 @@ class RemoveTransientData
       );
     SQL
   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

From d502320b1467a1ce46c4c63398db58c6cf341597 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 18:02:11 +0000
Subject: [PATCH 6/8] Enable cascading deletes

---
 .../20210127174120_add_cascading_deletes.rb   | 20 +++++++++++++++++++
 db/schema.rb                                  |  8 ++++----
 2 files changed, 24 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20210127174120_add_cascading_deletes.rb

diff --git a/db/migrate/20210127174120_add_cascading_deletes.rb b/db/migrate/20210127174120_add_cascading_deletes.rb
new file mode 100644
index 0000000000..3babe886b0
--- /dev/null
+++ b/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
diff --git a/db/schema.rb b/db/schema.rb
index 7e26c00970..f3c76493eb 100644
--- a/db/schema.rb
+++ b/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"

From 4f7c8062a19d883375f41a22b2de14399ecf07ba Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 27 Jan 2021 19:01:28 +0000
Subject: [PATCH 7/8] Create class to map join table and simplify code

---
 app/models/spree/option_values_line_item.rb   |  8 ++++++++
 lib/tasks/data/remove_transient_data.rb       | 15 ++------------
 .../tasks/data/remove_transient_data_spec.rb  | 20 ++++---------------
 3 files changed, 14 insertions(+), 29 deletions(-)
 create mode 100644 app/models/spree/option_values_line_item.rb

diff --git a/app/models/spree/option_values_line_item.rb b/app/models/spree/option_values_line_item.rb
new file mode 100644
index 0000000000..4f1ab992a8
--- /dev/null
+++ b/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
diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb
index e4fe755f23..a0b8128f04 100644
--- a/lib/tasks/data/remove_transient_data.rb
+++ b/lib/tasks/data/remove_transient_data.rb
@@ -18,7 +18,6 @@ class RemoveTransientData
     Session.where("updated_at < ?", SHORT_RETENTION).delete_all
 
     clear_old_cart_data!
-    clear_line_item_option_values!
   end
 
   private
@@ -29,25 +28,15 @@ class RemoveTransientData
       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 clear_line_item_option_values!
-    ActiveRecord::Base.connection.execute <<-SQL
-      DELETE FROM spree_option_values_line_items
-      WHERE line_item_id IN (
-        SELECT line_item_id FROM spree_option_values_line_items
-        LEFT OUTER JOIN spree_line_items
-        ON spree_option_values_line_items.line_item_id = spree_line_items.id
-        WHERE spree_line_items.id IS NULL
-      );
-    SQL
-  end
-
   def orders_without_payments
     # Carts with failed payments are ignored, as they contain potentially useful data
     Spree::Order.
diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb
index 34811682c1..fddf33aef7 100644
--- a/spec/lib/tasks/data/remove_transient_data_spec.rb
+++ b/spec/lib/tasks/data/remove_transient_data_spec.rb
@@ -61,23 +61,11 @@ describe RemoveTransientData do
         expect{ old_adjustment.reload }.to raise_error ActiveRecord::RecordNotFound
       end
 
-      context "removing defunct line item option value records" do
-        let(:connection) { ActiveRecord::Base.connection }
-        let(:query) {
-          <<-SQL
-            SELECT * FROM spree_option_values_line_items
-            LEFT OUTER JOIN spree_line_items
-            ON spree_option_values_line_items.line_item_id = spree_line_items.id
-            WHERE spree_line_items.id IS NULL;
-          SQL
-        }
+      it "removes any defunct line item option value records" do
+        line_item.delete
 
-        it "removes the records" do
-          line_item.delete
-
-          expect{ RemoveTransientData.new.call }.
-            to change{ connection.execute(query).count }.by(-1)
-        end
+        expect{ RemoveTransientData.new.call }.
+          to change{ Spree::OptionValuesLineItem.count }.by(-1)
       end
     end
   end

From 97912877121116ee00939c0a651c417d48a35609 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 12:02:10 +0000
Subject: [PATCH 8/8] Run data cleanup job at 4:30am

---
 config/schedule.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config/schedule.rb b/config/schedule.rb
index 94fcacc54d..a72cec32ca 100644
--- a/config/schedule.rb
+++ b/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