From be31b9a897d5539bcb1e4de82062c9453a31dc60 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 7 Sep 2022 14:44:08 +1000 Subject: [PATCH 1/5] Document purpose of customer model --- app/models/customer.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/models/customer.rb b/app/models/customer.rb index de37b5094a..933a41dee4 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -1,5 +1,12 @@ # frozen_string_literal: true +# A customer record is created the first time a person orders from a shop. +# +# It's a relationship between a user and an enterprise but for guest orders it +# can also be between an email address and an enterprise. +# +# The main purpose is tagging of customers to access private shops, receive +# discounts et cetera. A customer record is also needed for subscriptions. class Customer < ApplicationRecord include SetUnusedAddressFields From aec28e08078d78fc0e0e4f8caa69cb90ac2ace28 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 7 Sep 2022 14:48:43 +1000 Subject: [PATCH 2/5] Simplify customer model requiring enterprise --- app/models/customer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 933a41dee4..678c4a2d12 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -14,7 +14,7 @@ class Customer < ApplicationRecord searchable_attributes :first_name, :last_name, :email, :code - belongs_to :enterprise + belongs_to :enterprise, optional: false belongs_to :user, class_name: "Spree::User" has_many :orders, class_name: "Spree::Order" before_destroy :update_orders_and_delete_canceled_subscriptions @@ -33,7 +33,6 @@ class Customer < ApplicationRecord validates :code, uniqueness: { scope: :enterprise_id, allow_nil: true } validates :email, presence: true, 'valid_email_2/email': true, uniqueness: { scope: :enterprise_id, message: I18n.t('validation_msg_is_associated_with_an_exising_customer') } - validates :enterprise, presence: true scope :of, ->(enterprise) { where(enterprise_id: enterprise) } From 591f901935e445bbd11ab6ae1842145c80c98ce2 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 7 Sep 2022 16:08:53 +1000 Subject: [PATCH 3/5] Delete duplicate customer entries --- ...220907055044_delete_duplicate_customers.rb | 62 +++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220907055044_delete_duplicate_customers.rb diff --git a/db/migrate/20220907055044_delete_duplicate_customers.rb b/db/migrate/20220907055044_delete_duplicate_customers.rb new file mode 100644 index 0000000000..2598ab4658 --- /dev/null +++ b/db/migrate/20220907055044_delete_duplicate_customers.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +class DeleteDuplicateCustomers < ActiveRecord::Migration[6.1] + class Customer < ActiveRecord::Base + belongs_to :bill_address, class_name: "SpreeAddress", dependent: :destroy + belongs_to :ship_address, class_name: "SpreeAddress", dependent: :destroy + end + + class SpreeAddress < ActiveRecord::Base + end + + class SpreeOrder < ActiveRecord::Base + end + + class Subscription < ActiveRecord::Base + end + + def up + say "#{grouped_duplicates.keys.count} customers with duplicates." + + grouped_duplicates.map do |key, customers| + chosen = customers.first + others = customers - [chosen] + + say "- #{key}" + + # We can't tell which attributes or associations are the correct ones. + # Selection has been random so far and it's no regression to randomly + # select the attributes of the first customer record. + # + # However, we do need to update references to the customer. + update_references(others, chosen) + + others.each(&:destroy!) + end + end + + def grouped_duplicates + @grouped_duplicates ||= duplicate_records.group_by do |customer| + [customer.email, customer.enterprise_id] + end + end + + def duplicate_records + customer_duplicates = <<~SQL + JOIN customers AS copy + ON customers.id != copy.id + AND customers.email = copy.email + AND customers.enterprise_id = copy.enterprise_id + SQL + + Customer.joins(customer_duplicates) + end + + def update_references(source_customers, destination_customer) + SpreeOrder.where(customer_id: source_customers.map(&:id)). + update_all(customer_id: destination_customer.id) + + Subscription.where(customer_id: source_customers.map(&:id)). + update_all(customer_id: destination_customer.id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 6e3b0a3c87..6c9b1b00fe 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_07_13_195433) do +ActiveRecord::Schema.define(version: 2022_09_07_055044) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" From 286dade66ff9a22c048416aab9725f09370ffd8d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 8 Sep 2022 16:17:44 +1000 Subject: [PATCH 4/5] Don't enforce ApplicationRecord in migrations We shouldn't use app code in migrations because app code changes while migrations should always do the same to enable old servers to upgrade. We have some existing migrations which use ApplicationRecord but I found that changing them retrospectively breaks them already. So let's leave them alone. --- .rubocop_styleguide.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index 42857db556..15f52ac883 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -44,6 +44,11 @@ Metrics/BlockLength: "xdescribe", ] +Rails/ApplicationRecord: + Exclude: + # Migrations should not contain application code: + - "db/migrate/*.rb" + Rails/SkipsModelValidations: AllowedMethods: - "touch" From 4e33bd2713a43fcc87333a04adf63154e253c1de Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 27 Sep 2022 13:13:26 +1000 Subject: [PATCH 5/5] Avoid trying to delete address referenced elsewhere --- ...220907055044_delete_duplicate_customers.rb | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/db/migrate/20220907055044_delete_duplicate_customers.rb b/db/migrate/20220907055044_delete_duplicate_customers.rb index 2598ab4658..f28faf0874 100644 --- a/db/migrate/20220907055044_delete_duplicate_customers.rb +++ b/db/migrate/20220907055044_delete_duplicate_customers.rb @@ -2,8 +2,28 @@ class DeleteDuplicateCustomers < ActiveRecord::Migration[6.1] class Customer < ActiveRecord::Base - belongs_to :bill_address, class_name: "SpreeAddress", dependent: :destroy - belongs_to :ship_address, class_name: "SpreeAddress", dependent: :destroy + belongs_to :bill_address, class_name: "SpreeAddress" + belongs_to :ship_address, class_name: "SpreeAddress" + + after_destroy do + destroy_unused_address(bill_address) + destroy_unused_address(ship_address) + end + + def destroy_unused_address(record) + return unless record + return if in_use?(SpreeOrder, record) + return if in_use?(Customer, record) + return if in_use?(SpreeUser, record) + + record.destroy + end + + def in_use?(model, record) + model.where(bill_address_id: record).or( + model.where(ship_address_id: record) + ).present? + end end class SpreeAddress < ActiveRecord::Base @@ -15,6 +35,12 @@ class DeleteDuplicateCustomers < ActiveRecord::Migration[6.1] class Subscription < ActiveRecord::Base end + class Customer < ActiveRecord::Base + end + + class SpreeUser < ActiveRecord::Base + end + def up say "#{grouped_duplicates.keys.count} customers with duplicates."