Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove old abandoned carts #6740

Merged
merged 8 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/models/spree/option_values_line_item.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion config/schedule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
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

Expand Down
20 changes: 20 additions & 0 deletions db/migrate/20210127174120_add_cascading_deletes.rb
Original file line number Diff line number Diff line change
@@ -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
8 changes: 4 additions & 4 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1252,9 +1252,9 @@
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"
Expand Down Expand Up @@ -1290,7 +1290,7 @@
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"
Expand Down
35 changes: 31 additions & 4 deletions lib/tasks/data/remove_transient_data.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -12,8 +13,34 @@ class Session < ActiveRecord::Base
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of spree_option_values_line_items as a table to clear as well.

Looking at the DB I think we still have tables related to spree promotions... we could delete those. A different clear up...

Copy link
Contributor Author

@Matt-Yorkley Matt-Yorkley Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought of spree_option_values_line_items as a table to clear as well.

I just had a quick look at this, but I'm a bit nervous about proceeding in deleting those records as part of this task. In most cases option_values seem to be related to variants and products (with clear join tables / classes / associations representing the joins), but I can't see anywhere that explicitly defines or delineates option_values records' connection to line items as being separate from variants. It seems pretty murky. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are missing this:
line_item.rb
has_and_belongs_to_many :option_values, join_table: 'spree_option_values_line_items',
class_name: 'Spree::OptionValue'

and extensively used in concern VariantAndLineItemNaming.

I dont think option_values are directly associated with products but are associated with variants in
spree_option_values_variants is for variants.

I think it's safe to delete stuff from spree_option_values_line_items for the old_line_items you delete here.

Copy link
Contributor Author

@Matt-Yorkley Matt-Yorkley Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've added this in, but it required some raw SQL, as we're using #delete_all on the other queries to avoid instantiating the objects or triggering callbacks and we don't have a "nice" way of calling AR methods directly on the contents of that join table.

Also worth noting: we already have some orphaned records in this join table (spree_option_values_line_items) in production, so those will be cleared up as well 👍

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
41 changes: 37 additions & 4 deletions spec/lib/tasks/data/remove_transient_data_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

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)
Expand All @@ -15,25 +16,57 @@
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