From d0f347efaa811d7f4aa86c3ea11a7a15f2cb5dc1 Mon Sep 17 00:00:00 2001 From: Adrien Chauve Date: Thu, 6 Jan 2022 16:37:31 +0100 Subject: [PATCH 01/27] Add migration to split Customers.name into first and last names --- .../20220105085729_split_customers_name.rb | 22 +++++++++++++++++++ db/schema.rb | 4 +++- 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220105085729_split_customers_name.rb diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb new file mode 100644 index 0000000000..2d1cd9f5fb --- /dev/null +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -0,0 +1,22 @@ +class SplitCustomersName < ActiveRecord::Migration[6.1] + class Customer < ActiveRecord::Base + end + + def change + add_column :customers, :first_name, :string + add_column :customers, :last_name, :string + rename_column :customers, :name, :backup_name + reversible do |dir| + dir.up { migrate_customer_name_data } + end + end + + def migrate_customer_name_data + Customer.where("backup_name LIKE '% %'").find_each do |customer| + first_name, last_name = customer.backup_name.split(' ', 2) + customer.first_name = first_name + customer.last_name = last_name + customer.save + end + end +end \ No newline at end of file diff --git a/db/schema.rb b/db/schema.rb index b1c9499ddb..3fdb1f4cbf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -51,9 +51,11 @@ ActiveRecord::Schema.define(version: 2022_01_18_053107) do t.datetime "updated_at", null: false t.integer "bill_address_id" t.integer "ship_address_id" - t.string "name", limit: 255 + t.string "backup_name", limit: 255 t.boolean "allow_charges", default: false, null: false t.datetime "terms_and_conditions_accepted_at" + t.string "first_name" + t.string "last_name" t.index ["bill_address_id"], name: "index_customers_on_bill_address_id" t.index ["email"], name: "index_customers_on_email" t.index ["enterprise_id", "code"], name: "index_customers_on_enterprise_id_and_code", unique: true From 5ca4d549e7b79443071cc5021021aa3da8322081 Mon Sep 17 00:00:00 2001 From: Adrien Chauve Date: Thu, 6 Jan 2022 18:47:51 +0100 Subject: [PATCH 02/27] Update customer creation --- app/models/spree/order.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index c73f4453f0..5380a273d4 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -721,7 +721,8 @@ module Spree enterprise: distributor, email: email_for_customer, user: user, - name: bill_address&.full_name, + first_name: bill_address&.first_name, + last_name: bill_address&.last_name, bill_address: bill_address&.clone, ship_address: ship_address&.clone ) From ba6523bb766aa3d13e28f62fce4e26f5c120652f Mon Sep 17 00:00:00 2001 From: Adrien Chauve Date: Thu, 6 Jan 2022 18:48:07 +0100 Subject: [PATCH 03/27] Add Customer.full_name --- app/models/customer.rb | 4 ++++ spec/models/spree/order_spec.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 96a14d01d2..72f37c66e2 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -36,6 +36,10 @@ class Customer < ApplicationRecord private + def full_name + "#{first_name} #{last_name}".strip + end + def downcase_email email&.downcase! end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index fe31ab000a..b0e9405620 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1054,7 +1054,7 @@ describe Spree::Order do expect(order.customer).to be_nil expect { order.send(:ensure_customer) }.to change{ Customer.count }.by 1 - expect(order.customer.name).to eq order.bill_address.full_name + expect(order.customer.full_name).to eq order.bill_address.full_name expect(order.customer.bill_address.same_as?(order.bill_address)).to be true expect(order.customer.ship_address.same_as?(order.ship_address)).to be true end From 9b93102a961d55147af598eedfc1fe991ddb0781 Mon Sep 17 00:00:00 2001 From: Adrien Chauve Date: Fri, 7 Jan 2022 18:38:17 +0100 Subject: [PATCH 04/27] More fixes --- app/controllers/spree/admin/search_controller.rb | 3 ++- app/models/customer.rb | 2 +- app/serializers/api/admin/customer_serializer.rb | 8 ++++---- app/serializers/api/admin/subscription_serializer.rb | 12 ++++++++---- app/serializers/api/customer_serializer.rb | 2 +- .../spree/admin/search_controller_spec.rb | 2 +- 6 files changed, 17 insertions(+), 12 deletions(-) diff --git a/app/controllers/spree/admin/search_controller.rb b/app/controllers/spree/admin/search_controller.rb index fc3fbad955..1ddb023b3a 100644 --- a/app/controllers/spree/admin/search_controller.rb +++ b/app/controllers/spree/admin/search_controller.rb @@ -21,7 +21,8 @@ module Spree @customers = [] if spree_current_user.enterprises.pluck(:id).include? search_params[:distributor_id].to_i @customers = Customer. - ransack(m: 'or', email_start: search_params[:q], name_start: search_params[:q]). + ransack(m: 'or', email_start: search_params[:q], first_name_start: search_params[:q], + last_name_start: search_params[:q]). result. where(enterprise_id: search_params[:distributor_id].to_i) end diff --git a/app/models/customer.rb b/app/models/customer.rb index 72f37c66e2..3c9cf3715a 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -5,7 +5,7 @@ class Customer < ApplicationRecord acts_as_taggable - searchable_attributes :name, :email, :code + searchable_attributes :first_name, :last_name, :email, :code belongs_to :enterprise belongs_to :user, class_name: "Spree::User" diff --git a/app/serializers/api/admin/customer_serializer.rb b/app/serializers/api/admin/customer_serializer.rb index 30c067349b..14e8478e49 100644 --- a/app/serializers/api/admin/customer_serializer.rb +++ b/app/serializers/api/admin/customer_serializer.rb @@ -3,14 +3,14 @@ module Api module Admin class CustomerSerializer < ActiveModel::Serializer - attributes :id, :email, :enterprise_id, :user_id, :code, :tags, :tag_list, :name, - :allow_charges, :default_card_present? + attributes :id, :email, :enterprise_id, :user_id, :code, :tags, :tag_list, :first_name, + :last_name, :allow_charges, :default_card_present? has_one :ship_address, serializer: Api::AddressSerializer has_one :bill_address, serializer: Api::AddressSerializer - def name - object.name.presence || object.bill_address&.full_name + def full_name + object.full_name.presence || object.bill_address&.full_name end def tag_list diff --git a/app/serializers/api/admin/subscription_serializer.rb b/app/serializers/api/admin/subscription_serializer.rb index c75657ba99..4fab4ef769 100644 --- a/app/serializers/api/admin/subscription_serializer.rb +++ b/app/serializers/api/admin/subscription_serializer.rb @@ -5,8 +5,8 @@ module Api class SubscriptionSerializer < ActiveModel::Serializer attributes :id, :shop_id, :customer_id, :schedule_id, :payment_method_id, :shipping_method_id, :begins_at, :ends_at, - :customer_email, :customer_name, :schedule_name, :edit_path, :canceled_at, :paused_at, :state, - :shipping_fee_estimate, :payment_fee_estimate + :customer_email, :customer_first_name, :customer_last_name, :schedule_name, :edit_path, :canceled_at, + :paused_at, :state, :shipping_fee_estimate, :payment_fee_estimate has_many :subscription_line_items, serializer: Api::Admin::SubscriptionLineItemSerializer has_many :closed_proxy_orders, serializer: Api::Admin::ProxyOrderSerializer @@ -34,8 +34,12 @@ module Api object.customer&.email end - def customer_name - object.customer&.name + def customer_first_name + object.customer&.first_name + end + + def customer_last_name + object.customer&.last_name end def schedule_name diff --git a/app/serializers/api/customer_serializer.rb b/app/serializers/api/customer_serializer.rb index 053313cb30..72423714ba 100644 --- a/app/serializers/api/customer_serializer.rb +++ b/app/serializers/api/customer_serializer.rb @@ -2,7 +2,7 @@ module Api class CustomerSerializer < ActiveModel::Serializer - attributes :id, :enterprise_id, :name, :code, :email, :allow_charges + attributes :id, :enterprise_id, :first_name, :last_name, :code, :email, :allow_charges def attributes hash = super diff --git a/spec/controllers/spree/admin/search_controller_spec.rb b/spec/controllers/spree/admin/search_controller_spec.rb index 7000620af2..370dae9468 100644 --- a/spec/controllers/spree/admin/search_controller_spec.rb +++ b/spec/controllers/spree/admin/search_controller_spec.rb @@ -35,7 +35,7 @@ describe Spree::Admin::SearchController, type: :controller do describe 'searching for customers' do let!(:customer_1) { create(:customer, enterprise: enterprise, email: 'test1@email.com') } - let!(:customer_2) { create(:customer, enterprise: enterprise, name: 'test2') } + let!(:customer_2) { create(:customer, enterprise: enterprise, first_name: 'test2') } let!(:customer_3) { create(:customer, email: 'test3@email.com') } describe 'when search owned enterprises' do From ca4635922495467a173e17977ecbe0f6fb7523d8 Mon Sep 17 00:00:00 2001 From: Adrien Chauve Date: Mon, 10 Jan 2022 16:23:54 +0100 Subject: [PATCH 05/27] More fixes --- app/controllers/admin/customers_controller.rb | 2 +- app/models/customer.rb | 4 ++-- app/views/admin/customers/index.html.haml | 15 ++++++++++----- config/locales/en_GB.yml | 2 ++ .../column_preference_defaults.rb | 3 ++- spec/system/admin/customers_spec.rb | 3 ++- 6 files changed, 19 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/customers_controller.rb b/app/controllers/admin/customers_controller.rb index 71cc8fc2c8..28fcf235ba 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -99,7 +99,7 @@ module Admin def customer_params params.require(:customer).permit( - :enterprise_id, :name, :email, :code, :tag_list, + :enterprise_id, :first_name, :last_name, :email, :code, :tag_list, ship_address_attributes: PermittedAttributes::Address.attributes, bill_address_attributes: PermittedAttributes::Address.attributes, ) diff --git a/app/models/customer.rb b/app/models/customer.rb index 3c9cf3715a..ec4d78235c 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -34,12 +34,12 @@ class Customer < ApplicationRecord attr_accessor :gateway_recurring_payment_client_secret, :gateway_shop_id - private - def full_name "#{first_name} #{last_name}".strip end + private + def downcase_email email&.downcase! end diff --git a/app/views/admin/customers/index.html.haml b/app/views/admin/customers/index.html.haml index e2bff1832f..18bf810ee8 100644 --- a/app/views/admin/customers/index.html.haml +++ b/app/views/admin/customers/index.html.haml @@ -50,7 +50,8 @@ %table.index#customers %col.email{ width: "20%", 'ng-show' => 'columns.email.visible' } - %col.name{ width: "20%", 'ng-show' => 'columns.name.visible' } + %col.first_name{ width: "20%", 'ng-show' => 'columns.first_name.visible' } + %col.last_name{ width: "20%", 'ng-show' => 'columns.last_name.visible' } %col.code{ width: "10%", 'ng-show' => 'columns.code.visible' } %col.tags{ width: "20%", 'ng-show' => 'columns.tags.visible' } %col.bill_address{ width: "10%", 'ng-show' => 'columns.bill_address.visible' } @@ -63,8 +64,10 @@ -# %input{ :type => "checkbox", :name => 'toggle_bulk', 'ng-click' => 'toggleAllCheckboxes()', 'ng-checked' => "allBoxesChecked()" } %th.email{ 'ng-show' => 'columns.email.visible' } %a{ :href => '', 'ng-click' => "sorting.toggle('email')" }=t('admin.email') - %th.name{ 'ng-show' => 'columns.name.visible' } - %a{ :href => '', 'ng-click' => "sorting.toggle('name')" }=t('admin.name') + %th.first_name{ 'ng-show' => 'columns.first_name.visible' } + %a{ :href => '', 'ng-click' => "sorting.toggle('first_name')" }=t('admin.first_name') + %th.last_name{ 'ng-show' => 'columns.last_name.visible' } + %a{ :href => '', 'ng-click' => "sorting.toggle('last_name')" }=t('admin.last_name') %th.code{ 'ng-show' => 'columns.code.visible' } %a{ :href => '', 'ng-click' => "sorting.toggle('code')" }=t('admin.customers.index.code') %th.tags{ 'ng-show' => 'columns.tags.visible' }=t('admin.tags') @@ -78,8 +81,10 @@ %td.email{ 'ng-show' => 'columns.email.visible'} %span{ 'ng-bind' => '::customer.email' } %span.guest-label{ 'ng-show' => 'customer.user_id == null' }= t('.guest_label') - %td.name{ 'ng-show' => 'columns.name.visible'} - %input{ type: 'text', name: 'name', ng: { model: 'customer.name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'} + %td.first_name{ 'ng-show' => 'columns.first_name.visible'} + %input{ type: 'text', name: 'first_name', ng: { model: 'customer.first_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'} + %td.name{ 'ng-show' => 'columns.last_name.visible'} + %input{ type: 'text', name: 'last_name', ng: { model: 'customer.last_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'} %td.code{ 'ng-show' => 'columns.code.visible' } %input{ type: 'text', name: 'code', ng: {model: 'customer.code', change: 'checkForDuplicateCodes()'}, "obj-for-update" => "customer", "attr-for-update" => "code" } %i.icon-warning-sign{ ng: {if: 'duplicate'} } diff --git a/config/locales/en_GB.yml b/config/locales/en_GB.yml index 2e36241b0f..1733e703d9 100644 --- a/config/locales/en_GB.yml +++ b/config/locales/en_GB.yml @@ -411,6 +411,8 @@ en_GB: ends_at: Ends At ends_on: Ends On name: Name + first_name: First name + last_name: Last name on_hand: In Stock on_demand: Unlimited on_demand?: Unlimited? diff --git a/lib/open_food_network/column_preference_defaults.rb b/lib/open_food_network/column_preference_defaults.rb index 3f135f6b61..4f3cc21f3f 100644 --- a/lib/open_food_network/column_preference_defaults.rb +++ b/lib/open_food_network/column_preference_defaults.rb @@ -30,7 +30,8 @@ module OpenFoodNetwork node = 'admin.customers.index' { email: { name: I18n.t("admin.email"), visible: true }, - name: { name: I18n.t("admin.name"), visible: true }, + first_name: { name: I18n.t("admin.first_name"), visible: true }, + last_name: { name: I18n.t("admin.last_name"), visible: true }, code: { name: I18n.t("#{node}.code"), visible: true }, tags: { name: I18n.t("admin.tags"), visible: true }, bill_address: { name: I18n.t("#{node}.bill_address"), visible: true }, diff --git a/spec/system/admin/customers_spec.rb b/spec/system/admin/customers_spec.rb index 3692d90bf4..b7e63e8e2c 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -177,7 +177,8 @@ describe 'Customers' do select2_select managed_distributor1.name, from: "shop_id" within "tr#c_#{customer1.id}" do - expect(find_field('name').value).to eq 'John Doe' + expect(find_field('first_name').value).to eq 'John' + expect(find_field('last_name').value).to eq 'Doe' fill_in "code", with: "new-customer-code" expect(page).to have_css "input[name=code].update-pending" From 7c25127ab60bab03d8709e1f6320822e68dc4c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Thu, 20 Jan 2022 22:56:48 +0100 Subject: [PATCH 06/27] Improve first_name and last_name setup --- .../20220105085729_split_customers_name.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index 2d1cd9f5fb..7cde6e047d 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -12,10 +12,18 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] end def migrate_customer_name_data - Customer.where("backup_name LIKE '% %'").find_each do |customer| - first_name, last_name = customer.backup_name.split(' ', 2) - customer.first_name = first_name - customer.last_name = last_name + Customer.includes(:bill_address).find_each do |customer| + bill_address = customer.bill_address + + if bill_address.present? && bill_address.firstname.present? && bill_address.lastname? + customer.first_name = bill_address.firstname.strip + customer.last_name = bill_address.lastname.strip + else + first_name, last_name = customer.backup_name.strip.split(' ', 2) + customer.first_name = first_name + customer.last_name = last_name + end + customer.save end end From eefd9891d6f2cecf1c9a5e7f6e0f71974e5e0229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Thu, 20 Jan 2022 23:29:27 +0100 Subject: [PATCH 07/27] Add en translations --- config/locales/en.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index 6c23d73022..e639a9faa5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -447,6 +447,8 @@ en: ends_at: Ends At ends_on: Ends On name: Name + first_name: First Name + last_name: Last Name on_hand: On Hand on_demand: On Demand on_demand?: On Demand? From 75345a95affaa3c987a4039ca42168501f832b98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Thu, 20 Jan 2022 23:30:17 +0100 Subject: [PATCH 08/27] Update customer factory --- spec/factories.rb | 8 -------- spec/factories/customer_factory.rb | 13 +++++++++++++ 2 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 spec/factories/customer_factory.rb diff --git a/spec/factories.rb b/spec/factories.rb index f766c0cb19..73a46e9109 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -113,14 +113,6 @@ FactoryBot.define do property end - factory :customer, class: Customer do - email { generate(:random_email) } - enterprise - code { SecureRandom.base64(150) } - user - bill_address { create(:address) } - end - factory :stripe_account do enterprise { FactoryBot.create(:distributor_enterprise) } stripe_user_id { "abc123" } diff --git a/spec/factories/customer_factory.rb b/spec/factories/customer_factory.rb new file mode 100644 index 0000000000..bc4a9e6980 --- /dev/null +++ b/spec/factories/customer_factory.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :customer, class: Customer do + first_name { FFaker::Name.first_name } + last_name { FFaker::Name.last_name } + email { generate(:random_email) } + enterprise + code { SecureRandom.base64(150) } + user + bill_address { create(:address) } + end +end From 23776c7a3ebb7118ebf0c446c7d8c052b82a3966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Thu, 20 Jan 2022 23:31:20 +0100 Subject: [PATCH 09/27] Fix more specs --- app/views/admin/customers/index.html.haml | 6 +++--- .../admin/subscriptions/_table.html.haml | 2 +- .../reports/enterprise_fee_summary/scope.rb | 2 +- .../report_service_spec.rb | 10 +++++----- .../column_preference_defaults.rb | 8 ++++---- spec/system/admin/customers_spec.rb | 20 ++++++++++--------- spec/system/admin/order_spec.rb | 2 +- spec/system/admin/subscriptions_spec.rb | 11 +++++++--- 8 files changed, 34 insertions(+), 27 deletions(-) diff --git a/app/views/admin/customers/index.html.haml b/app/views/admin/customers/index.html.haml index 18bf810ee8..1124120693 100644 --- a/app/views/admin/customers/index.html.haml +++ b/app/views/admin/customers/index.html.haml @@ -82,9 +82,9 @@ %span{ 'ng-bind' => '::customer.email' } %span.guest-label{ 'ng-show' => 'customer.user_id == null' }= t('.guest_label') %td.first_name{ 'ng-show' => 'columns.first_name.visible'} - %input{ type: 'text', name: 'first_name', ng: { model: 'customer.first_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'} - %td.name{ 'ng-show' => 'columns.last_name.visible'} - %input{ type: 'text', name: 'last_name', ng: { model: 'customer.last_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'name'} + %input{ type: 'text', name: 'first_name', ng: { model: 'customer.first_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'first_name'} + %td.last_name{ 'ng-show' => 'columns.last_name.visible'} + %input{ type: 'text', name: 'last_name', ng: { model: 'customer.last_name' }, 'obj-for-update' => 'customer', 'attr-for-update' => 'last_name'} %td.code{ 'ng-show' => 'columns.code.visible' } %input{ type: 'text', name: 'code', ng: {model: 'customer.code', change: 'checkForDuplicateCodes()'}, "obj-for-update" => "customer", "attr-for-update" => "code" } %i.icon-warning-sign{ ng: {if: 'duplicate'} } diff --git a/app/views/admin/subscriptions/_table.html.haml b/app/views/admin/subscriptions/_table.html.haml index c4eb12b891..67dced6881 100644 --- a/app/views/admin/subscriptions/_table.html.haml +++ b/app/views/admin/subscriptions/_table.html.haml @@ -41,7 +41,7 @@ %td.customer.text-center{ ng: { show: 'columns.customer.visible'}} %span{ "ng-bind": '::subscription.customer_email' } %br - %span{ "ng-bind": '::subscription.customer_name' } + %span{ "ng-bind": '::subscription.customer_full_name' } %td.schedule.text-center{ ng: { show: 'columns.schedule.visible', bind: '::subscription.schedule_name' } } %td.items.panel-toggle.text-center{ name: 'products', ng: { show: 'columns.items.visible' } } %h5{ ng: { bind: 'itemCount(subscription)' } } diff --git a/engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb b/engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb index 59a5cd0470..0be3d77921 100644 --- a/engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb +++ b/engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb @@ -374,7 +374,7 @@ module OrderManagement hubs.name AS hub_name, enterprises.name AS enterprise_name, enterprise_fees.fee_type AS fee_type, - customers.name AS customer_name, + TRIM(CONCAT(customers.first_name, ' ', customers.last_name)) AS customer_name, customers.email AS customer_email, enterprise_fees.name AS fee_name, spree_tax_categories.name AS tax_category_name, diff --git a/engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/report_service_spec.rb b/engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/report_service_spec.rb index 44cf80200d..28d240a296 100644 --- a/engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/report_service_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/report_service_spec.rb @@ -36,8 +36,8 @@ describe OrderManagement::Reports::EnterpriseFeeSummary::ReportService do let!(:variant) { prepare_variant } # Create customers. - let!(:customer) { create(:customer, name: "Sample Customer") } - let!(:another_customer) { create(:customer, name: "Another Customer") } + let!(:customer) { create(:customer, first_name: "Sample", last_name: "Customer") } + let!(:another_customer) { create(:customer, first_name: "Another", last_name: "Customer") } # Setup up permissions and report. let!(:current_user) { create(:admin_user) } @@ -438,9 +438,9 @@ describe OrderManagement::Reports::EnterpriseFeeSummary::ReportService do context "filtering by completion date" do let(:timestamp) { Time.zone.local(2018, 1, 5, 14, 30, 5) } - let!(:customer_a) { create(:customer, name: "Customer A") } - let!(:customer_b) { create(:customer, name: "Customer B") } - let!(:customer_c) { create(:customer, name: "Customer C") } + let!(:customer_a) { create(:customer, first_name: "Customer", last_name: "A") } + let!(:customer_b) { create(:customer, first_name: "Customer", last_name: "B") } + let!(:customer_c) { create(:customer, first_name: "Customer", last_name: "C") } let!(:order_placed_before_timestamp) do prepare_order(customer: customer_a).tap do |order| diff --git a/lib/open_food_network/column_preference_defaults.rb b/lib/open_food_network/column_preference_defaults.rb index 4f3cc21f3f..cd162d2638 100644 --- a/lib/open_food_network/column_preference_defaults.rb +++ b/lib/open_food_network/column_preference_defaults.rb @@ -30,10 +30,10 @@ module OpenFoodNetwork node = 'admin.customers.index' { email: { name: I18n.t("admin.email"), visible: true }, - first_name: { name: I18n.t("admin.first_name"), visible: true }, - last_name: { name: I18n.t("admin.last_name"), visible: true }, - code: { name: I18n.t("#{node}.code"), visible: true }, - tags: { name: I18n.t("admin.tags"), visible: true }, + first_name: { name: I18n.t("admin.first_name"), visible: true }, + last_name: { name: I18n.t("admin.last_name"), visible: true }, + code: { name: I18n.t("#{node}.code"), visible: true }, + tags: { name: I18n.t("admin.tags"), visible: true }, bill_address: { name: I18n.t("#{node}.bill_address"), visible: true }, ship_address: { name: I18n.t("#{node}.ship_address"), visible: true }, balance: { name: I18n.t("#{node}.balance"), visible: true } diff --git a/spec/system/admin/customers_spec.rb b/spec/system/admin/customers_spec.rb index b7e63e8e2c..eb4fca8363 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -14,7 +14,9 @@ describe 'Customers' do let(:unmanaged_distributor) { create(:distributor_enterprise) } describe "using the customers index" do - let!(:customer1) { create(:customer, enterprise: managed_distributor1, code: nil) } + let!(:customer1) { + create(:customer, first_name: 'John', last_name: 'Doe', enterprise: managed_distributor1, code: nil) + } let!(:customer2) { create(:customer, enterprise: managed_distributor1, code: nil) } let!(:customer3) { create(:customer, enterprise: unmanaged_distributor) } let!(:customer4) { create(:customer, enterprise: managed_distributor2) } @@ -183,8 +185,8 @@ describe 'Customers' do fill_in "code", with: "new-customer-code" expect(page).to have_css "input[name=code].update-pending" - fill_in "name", with: "customer abc" - expect(page).to have_css "input[name=name].update-pending" + fill_in "first_name", with: "customer abc" + expect(page).to have_css "input[name=first_name].update-pending" find(:css, "tags-input .tags input").set "awesome\n" expect(page).to have_css ".tag_watcher.update-pending" @@ -194,12 +196,12 @@ describe 'Customers' do # Every says it updated expect(page).to have_css "input[name=code].update-success" - expect(page).to have_css "input[name=name].update-success" + expect(page).to have_css "input[name=first_name].update-success" expect(page).to have_css ".tag_watcher.update-success" # And it actually did expect(customer1.reload.code).to eq "new-customer-code" - expect(customer1.reload.name).to eq "customer abc" + expect(customer1.reload.first_name).to eq "customer abc" expect(customer1.tag_list).to eq ["awesome"] # Clearing attributes @@ -207,8 +209,8 @@ describe 'Customers' do fill_in "code", with: "" expect(page).to have_css "input[name=code].update-pending" - fill_in "name", with: "" - expect(page).to have_css "input[name=name].update-pending" + fill_in "first_name", with: "" + expect(page).to have_css "input[name=first_name].update-pending" find("tags-input li.tag-item a.remove-button").click expect(page).to have_css ".tag_watcher.update-pending" @@ -217,12 +219,12 @@ describe 'Customers' do # Every says it updated expect(page).to have_css "input[name=code].update-success" - expect(page).to have_css "input[name=name].update-success" + expect(page).to have_css "input[name=first_name].update-success" expect(page).to have_css ".tag_watcher.update-success" # And it actually did expect(customer1.reload.code).to be nil - expect(customer1.reload.name).to eq '' + expect(customer1.reload.first_name).to eq '' expect(customer1.tag_list).to eq [] end diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index dae71d2d86..7182da0851 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -239,7 +239,7 @@ describe ' new_customer = Customer.last - expect(new_customer.name).to eq('Clark Kent') + expect(new_customer.full_name).to eq('Clark Kent') expect(new_customer.bill_address.address1).to eq('Smallville') expect(new_customer.bill_address.city).to eq('Kansas') expect(new_customer.bill_address.zipcode).to eq('SP1 M11') diff --git a/spec/system/admin/subscriptions_spec.rb b/spec/system/admin/subscriptions_spec.rb index 41807ec0e4..6b74f7cd63 100644 --- a/spec/system/admin/subscriptions_spec.rb +++ b/spec/system/admin/subscriptions_spec.rb @@ -19,7 +19,7 @@ describe 'Subscriptions' do let!(:subscription) { create(:subscription, shop: shop, with_items: true, with_proxy_orders: true) } - let!(:customer) { create(:customer, name: "Customer A") } + let!(:customer) { create(:customer) } let!(:other_subscription) { create(:subscription, shop: shop, customer: customer, with_items: true, with_proxy_orders: true) @@ -81,8 +81,13 @@ describe 'Subscriptions' do expect(page).to have_selector "tr#so_#{other_subscription.id}" expect(page).to have_no_selector "tr#so_#{subscription.id}" - # Using the Quick Search: filter by name - fill_in 'query', with: other_subscription.customer.name + # Using the Quick Search: filter by first_name + fill_in 'query', with: other_subscription.customer.first_name + expect(page).to have_selector "tr#so_#{other_subscription.id}" + expect(page).to have_no_selector "tr#so_#{subscription.id}" + + # Using the Quick Search: filter by last_name + fill_in 'query', with: other_subscription.customer.last_name expect(page).to have_selector "tr#so_#{other_subscription.id}" expect(page).to have_no_selector "tr#so_#{subscription.id}" From 836a60a6c78e797177bf3fbd9996a236cb129fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Sun, 23 Jan 2022 15:30:19 +0100 Subject: [PATCH 10/27] Add missing full_name bind to subscriptions serializer --- app/serializers/api/admin/subscription_serializer.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/serializers/api/admin/subscription_serializer.rb b/app/serializers/api/admin/subscription_serializer.rb index 4fab4ef769..cdaecf432e 100644 --- a/app/serializers/api/admin/subscription_serializer.rb +++ b/app/serializers/api/admin/subscription_serializer.rb @@ -4,9 +4,9 @@ module Api module Admin class SubscriptionSerializer < ActiveModel::Serializer attributes :id, :shop_id, :customer_id, :schedule_id, :payment_method_id, :shipping_method_id, - :begins_at, :ends_at, - :customer_email, :customer_first_name, :customer_last_name, :schedule_name, :edit_path, :canceled_at, - :paused_at, :state, :shipping_fee_estimate, :payment_fee_estimate + :begins_at, :ends_at, :customer_email, :customer_first_name, :customer_last_name, + :customer_full_name, :schedule_name, :edit_path, :canceled_at, :paused_at, :state, + :shipping_fee_estimate, :payment_fee_estimate has_many :subscription_line_items, serializer: Api::Admin::SubscriptionLineItemSerializer has_many :closed_proxy_orders, serializer: Api::Admin::ProxyOrderSerializer @@ -42,6 +42,10 @@ module Api object.customer&.last_name end + def customer_full_name + object.customer&.full_name + end + def schedule_name object.schedule&.name end From b8afb7ec4d3add0772828c34bc4ba5773fc115f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 1 Feb 2022 21:31:23 +0100 Subject: [PATCH 11/27] Add default values for new fields --- db/migrate/20220105085729_split_customers_name.rb | 4 ++-- db/schema.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index 7cde6e047d..19c0a3e594 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -3,8 +3,8 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] end def change - add_column :customers, :first_name, :string - add_column :customers, :last_name, :string + add_column :customers, :first_name, :string, null: false, default: "" + add_column :customers, :last_name, :string, null: false, default: "" rename_column :customers, :name, :backup_name reversible do |dir| dir.up { migrate_customer_name_data } diff --git a/db/schema.rb b/db/schema.rb index 3fdb1f4cbf..468d478a93 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -54,8 +54,8 @@ ActiveRecord::Schema.define(version: 2022_01_18_053107) do t.string "backup_name", limit: 255 t.boolean "allow_charges", default: false, null: false t.datetime "terms_and_conditions_accepted_at" - t.string "first_name" - t.string "last_name" + t.string "first_name", default: "", null: false + t.string "last_name", default: "", null: false t.index ["bill_address_id"], name: "index_customers_on_bill_address_id" t.index ["email"], name: "index_customers_on_email" t.index ["enterprise_id", "code"], name: "index_customers_on_enterprise_id_and_code", unique: true From d016c47789dfcde7a56af3e4b66aee2bf3e21ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 1 Feb 2022 23:34:03 +0100 Subject: [PATCH 12/27] Refactor data migration to use data sets --- .../20220105085729_split_customers_name.rb | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index 19c0a3e594..d52d619747 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -1,29 +1,38 @@ +require 'concerns/address_display' +require 'spree/address' + class SplitCustomersName < ActiveRecord::Migration[6.1] - class Customer < ActiveRecord::Base + class Customer < ApplicationRecord + belongs_to :bill_address, class_name: "Spree::Address" end - def change + def up add_column :customers, :first_name, :string, null: false, default: "" add_column :customers, :last_name, :string, null: false, default: "" rename_column :customers, :name, :backup_name - reversible do |dir| - dir.up { migrate_customer_name_data } - end + + migrate_customer_name_data! end - def migrate_customer_name_data - Customer.includes(:bill_address).find_each do |customer| + def down + remove_column :customers, :first_name + remove_column :customers, :last_name + rename_column :customers, :backup_name, :name + end + + def migrate_customer_name_data! + Customer.includes(:bill_address).where.not(bill_address_id: nil).find_each do |customer| bill_address = customer.bill_address - if bill_address.present? && bill_address.firstname.present? && bill_address.lastname? - customer.first_name = bill_address.firstname.strip - customer.last_name = bill_address.lastname.strip - else - first_name, last_name = customer.backup_name.strip.split(' ', 2) - customer.first_name = first_name - customer.last_name = last_name - end + customer.first_name = bill_address.firstname.strip + customer.last_name = bill_address.lastname.strip + customer.save + end + Customer.where(first_name: "", last_name: "").where.not(backup_name: [nil, ""]).find_each do |customer| + first_name, last_name = customer.backup_name.split(' ', 2) + customer.first_name = first_name + customer.last_name = last_name.to_s customer.save end end From 9682b9256b59629601fe0dc7534987ab2d06fb6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Wed, 2 Feb 2022 21:45:11 +0100 Subject: [PATCH 13/27] Arrange classes loading for good migration --- db/migrate/20220105085729_split_customers_name.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index d52d619747..4d8ddd5349 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -1,9 +1,9 @@ -require 'concerns/address_display' -require 'spree/address' - class SplitCustomersName < ActiveRecord::Migration[6.1] + class Spree::DummyAddress < ApplicationRecord + self.table_name = 'spree_addresses' + end class Customer < ApplicationRecord - belongs_to :bill_address, class_name: "Spree::Address" + belongs_to :bill_address, class_name: "Spree::DummyAddress" end def up From 50302163c0d1827e3b7bcaa6c17466407004f534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Thu, 3 Feb 2022 22:07:47 +0100 Subject: [PATCH 14/27] Update migration with better logic and cosmetics --- .../20220105085729_split_customers_name.rb | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index 4d8ddd5349..bf76a65cec 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -1,9 +1,8 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] - class Spree::DummyAddress < ApplicationRecord - self.table_name = 'spree_addresses' + class SpreeAddress < ApplicationRecord end class Customer < ApplicationRecord - belongs_to :bill_address, class_name: "Spree::DummyAddress" + belongs_to :bill_address, class_name: "SpreeAddress" end def up @@ -21,19 +20,22 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] end def migrate_customer_name_data! - Customer.includes(:bill_address).where.not(bill_address_id: nil).find_each do |customer| + Customer.joins(:bill_address).find_each do |customer| bill_address = customer.bill_address - customer.first_name = bill_address.firstname.strip - customer.last_name = bill_address.lastname.strip - customer.save + customer.update( + first_name: bill_address.firstname.strip, + last_name: bill_address.lastname.strip + ) end Customer.where(first_name: "", last_name: "").where.not(backup_name: [nil, ""]).find_each do |customer| - first_name, last_name = customer.backup_name.split(' ', 2) - customer.first_name = first_name - customer.last_name = last_name.to_s - customer.save + name_words = customer.backup_name.split(' ') + + customer.update( + first_name: name_words.first, + last_name: name_words[1..].join(' ') + ) end end -end \ No newline at end of file +end From 4cb31d04a783140bc7573bdaa1f8066ce62aea2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Fri, 4 Feb 2022 10:03:07 +0100 Subject: [PATCH 15/27] Repair specs with default values on ensure_customer method --- app/models/spree/order.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 5380a273d4..a56028a614 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -717,16 +717,15 @@ module Spree def ensure_customer return if associate_customer - self.customer = Customer.new( + self.customer = Customer.create( enterprise: distributor, email: email_for_customer, user: user, - first_name: bill_address&.first_name, - last_name: bill_address&.last_name, + first_name: bill_address&.first_name.to_s, + last_name: bill_address&.last_name.to_s, bill_address: bill_address&.clone, ship_address: ship_address&.clone ) - customer.save Bugsnag.notify(customer.errors.full_messages.join(", ")) unless customer.persisted? end From 6d986deb2d0d8364949112f25747819bcf456530 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 8 Feb 2022 17:46:12 +0100 Subject: [PATCH 16/27] Remove translation changes from locale file --- config/locales/en_GB.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/locales/en_GB.yml b/config/locales/en_GB.yml index 1733e703d9..2e36241b0f 100644 --- a/config/locales/en_GB.yml +++ b/config/locales/en_GB.yml @@ -411,8 +411,6 @@ en_GB: ends_at: Ends At ends_on: Ends On name: Name - first_name: First name - last_name: Last name on_hand: In Stock on_demand: Unlimited on_demand?: Unlimited? From 5c9dd81595d3f9ad5bde5af4a47ca67422593f06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 8 Feb 2022 17:46:44 +0100 Subject: [PATCH 17/27] Deal with nil attirubtes on bill_address --- db/migrate/20220105085729_split_customers_name.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index bf76a65cec..e219869827 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -24,8 +24,8 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] bill_address = customer.bill_address customer.update( - first_name: bill_address.firstname.strip, - last_name: bill_address.lastname.strip + first_name: bill_address.firstname.to_s.strip, + last_name: bill_address.lastname.to_s.strip ) end From 554a8625e51c0c730ac38761608baf357a9c7c3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 8 Feb 2022 19:01:37 +0100 Subject: [PATCH 18/27] Refactor ensure_customer method --- app/models/spree/order.rb | 12 ++++++------ spec/models/spree/order_spec.rb | 17 +---------------- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index a56028a614..678a0fc8d1 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -711,13 +711,11 @@ module Spree def associate_customer return customer if customer.present? - self.customer = Customer.of(distributor).find_by(email: email_for_customer) + Customer.of(distributor).find_by(email: email_for_customer) end - def ensure_customer - return if associate_customer - - self.customer = Customer.create( + def create_customer + Customer.create( enterprise: distributor, email: email_for_customer, user: user, @@ -726,8 +724,10 @@ module Spree bill_address: bill_address&.clone, ship_address: ship_address&.clone ) + end - Bugsnag.notify(customer.errors.full_messages.join(", ")) unless customer.persisted? + def ensure_customer + self.customer = associate_customer || create_customer end def update_adjustment!(adjustment) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index b0e9405620..a244f1670b 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -969,9 +969,8 @@ describe Spree::Order do context "and a customer for order.distributor and order#email_for_customer already exists" do let!(:customer) { create(:customer, enterprise: distributor, email: "existing@email.com" ) } - it "associates the order with the existing customer, and returns the customer" do + it "returns the customer to associate" do result = order.send(:associate_customer) - expect(order.customer).to eq customer expect(result).to eq customer end end @@ -1035,20 +1034,6 @@ describe Spree::Order do order.ship_address = create(:address) end - context "and the customer is not valid" do - before do - order.distributor = nil - order.user = nil - order.email = nil - end - - it "sends an error to Bugsnag" do - expect(Bugsnag) - .to receive(:notify).with("Email can't be blank, Enterprise can't be blank") - order.send(:ensure_customer) - end - end - context "and the customer is valid" do it "creates a new customer with defaut name and addresses" do expect(order.customer).to be_nil From 07314af3f6c1619b29a070133898e13f20da425a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 9 Feb 2022 13:38:49 +1100 Subject: [PATCH 19/27] Spec customer record association more realistically The newly added specs were tested on the master branch and passed. But the previous commit broke one test case which I marked as pending here. The removed specs are completely replaced by the new ones. Their main downside was to test only small bits of internal behaviour without considering the whole callback chain of the order. The new specs are more realistic and catch bugs like mentioned above. --- spec/models/spree/order_spec.rb | 130 ++++++++++---------------------- 1 file changed, 39 insertions(+), 91 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index a244f1670b..c3dd67e7f2 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -940,110 +940,58 @@ describe Spree::Order do context "when creating an order" do it "does not create a customer" do - order = create(:order, distributor: distributor) - expect(order.customer).to be_nil - end - end - - context "when updating the order" do - let!(:order) { create(:order, distributor: distributor) } - - before do - order.state = "complete" - order.save! - end - - it "creates a customer" do - expect(order.customer).not_to be_nil - end - end - end - - describe "#associate_customer" do - let(:distributor) { create(:distributor_enterprise) } - let!(:order) { create(:order, distributor: distributor) } - - context "when an email address is available for the order" do - before { allow(order).to receive(:email_for_customer) { "existing@email.com" } } - - context "and a customer for order.distributor and order#email_for_customer already exists" do - let!(:customer) { create(:customer, enterprise: distributor, email: "existing@email.com" ) } - - it "returns the customer to associate" do - result = order.send(:associate_customer) - expect(result).to eq customer - end - end - - context "and a customer for order.distributor and order.user.email does not alread exist" do - let!(:customer) { - create(:customer, enterprise: distributor, email: 'some-other-email@email.com') + expect { + create(:order, distributor: distributor) + }.to_not change { + Customer.count } - - it "does not set the customer and returns nil" do - result = order.send(:associate_customer) - expect(order.customer).to be_nil - expect(result).to be_nil - end end - end - context "when an email address is not available for the order" do - let!(:customer) { create(:customer, enterprise: distributor) } - before { allow(order).to receive(:email_for_customer) { nil } } + it "associates an existing customer" do + pending "the last commit broke associating orders" - it "does not set the customer and returns nil" do - result = order.send(:associate_customer) - expect(order.customer).to be_nil - expect(result).to be_nil - end - end - end + customer = create( + :customer, + user: user, + email: user.email, + enterprise: distributor + ) + order = create(:order, user: user, distributor: distributor) - describe "ensuring a customer is linked" do - let(:distributor) { create(:distributor_enterprise) } - let!(:order) { create(:order, distributor: distributor) } - - context "when a customer has already been linked to the order" do - let!(:customer) { create(:customer, enterprise: distributor, email: "existing@email.com" ) } - before { order.update_attribute(:customer_id, customer.id) } - - it "does nothing" do - order.send(:ensure_customer) expect(order.customer).to eq customer end end - context "when a customer not been linked to the order" do - context "but one matching order#email_for_customer already exists" do - let!(:customer) { - create(:customer, enterprise: distributor, email: 'some-other-email@email.com') - } - before { allow(order).to receive(:email_for_customer) { 'some-other-email@email.com' } } - - it "links the customer customer to the order" do - expect(order.customer).to be_nil - expect{ order.send(:ensure_customer) }.to_not change{ Customer.count } - expect(order.customer).to eq customer - end + context "when updating the order" do + before do + order.update!(distributor: distributor) end - context "and order#email_for_customer does not match any existing customers" do - before do - order.bill_address = create(:address) - order.ship_address = create(:address) - end + it "associates an existing customer" do + customer = create( + :customer, + user: user, + email: user.email, + enterprise: distributor + ) - context "and the customer is valid" do - it "creates a new customer with defaut name and addresses" do - expect(order.customer).to be_nil - expect { order.send(:ensure_customer) }.to change{ Customer.count }.by 1 + order.update!(state: "complete") - expect(order.customer.full_name).to eq order.bill_address.full_name - expect(order.customer.bill_address.same_as?(order.bill_address)).to be true - expect(order.customer.ship_address.same_as?(order.ship_address)).to be true - end - end + expect(order.customer).to eq customer + end + + it "doesn't create a customer before needed" do + expect(order.customer).to eq nil + end + + it "creates a customer" do + expect { + order.update!(state: "complete") + }.to change { + Customer.count + }.by(1) + + expect(order.customer).to be_present end end end From d09ba16411f5183305d75b1aa68420ac99db1c28 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 9 Feb 2022 13:45:52 +1100 Subject: [PATCH 20/27] Associate customers again And simplify the before_validation actions. --- app/models/spree/order.rb | 5 +++-- spec/models/spree/order_spec.rb | 2 -- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 678a0fc8d1..2693ef5bb9 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -84,8 +84,7 @@ module Spree before_validation :set_currency before_validation :generate_order_number, on: :create before_validation :clone_billing_address, if: :use_billing? - before_validation :associate_customer, unless: :customer_id? - before_validation :ensure_customer, unless: :customer_is_valid? + before_validation :ensure_customer before_create :link_by_email after_create :create_tax_charge! @@ -715,6 +714,8 @@ module Spree end def create_customer + return if customer_is_valid? + Customer.create( enterprise: distributor, email: email_for_customer, diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index c3dd67e7f2..2a87a04009 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -948,8 +948,6 @@ describe Spree::Order do end it "associates an existing customer" do - pending "the last commit broke associating orders" - customer = create( :customer, user: user, From de4d074cb3a07c6c744e2d6a7b9f83c743953a61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Fri, 11 Feb 2022 19:10:35 +0100 Subject: [PATCH 21/27] Remove bill_address fetching logic --- db/migrate/20220105085729_split_customers_name.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index e219869827..d6affb9a43 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -20,15 +20,6 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] end def migrate_customer_name_data! - Customer.joins(:bill_address).find_each do |customer| - bill_address = customer.bill_address - - customer.update( - first_name: bill_address.firstname.to_s.strip, - last_name: bill_address.lastname.to_s.strip - ) - end - Customer.where(first_name: "", last_name: "").where.not(backup_name: [nil, ""]).find_each do |customer| name_words = customer.backup_name.split(' ') From 9a12957e273f2b6538cc63529a4e0e3f2294924b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Fri, 11 Feb 2022 19:11:10 +0100 Subject: [PATCH 22/27] Handle the empty-space name on customers --- db/migrate/20220105085729_split_customers_name.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index d6affb9a43..50e5409c9f 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -22,6 +22,7 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] def migrate_customer_name_data! Customer.where(first_name: "", last_name: "").where.not(backup_name: [nil, ""]).find_each do |customer| name_words = customer.backup_name.split(' ') + next if name_words.empty? customer.update( first_name: name_words.first, From 68193efcf6d79dbbf36daacf6f2dd327acb87e7b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 11:43:43 +1100 Subject: [PATCH 23/27] Keep old customers.name column for compatibilty Our app code will try to access the old attribute while the migration is running. That would lead to errors during checkout. --- db/migrate/20220105085729_split_customers_name.rb | 9 +++++---- db/schema.rb | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index 50e5409c9f..ccf7bd364e 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -1,6 +1,9 @@ +# frozen_string_literal: true + class SplitCustomersName < ActiveRecord::Migration[6.1] class SpreeAddress < ApplicationRecord end + class Customer < ApplicationRecord belongs_to :bill_address, class_name: "SpreeAddress" end @@ -8,7 +11,6 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] def up add_column :customers, :first_name, :string, null: false, default: "" add_column :customers, :last_name, :string, null: false, default: "" - rename_column :customers, :name, :backup_name migrate_customer_name_data! end @@ -16,12 +18,11 @@ class SplitCustomersName < ActiveRecord::Migration[6.1] def down remove_column :customers, :first_name remove_column :customers, :last_name - rename_column :customers, :backup_name, :name end def migrate_customer_name_data! - Customer.where(first_name: "", last_name: "").where.not(backup_name: [nil, ""]).find_each do |customer| - name_words = customer.backup_name.split(' ') + Customer.where(first_name: "", last_name: "").where.not(name: [nil, ""]).find_each do |customer| + name_words = customer.name.split(' ') next if name_words.empty? customer.update( diff --git a/db/schema.rb b/db/schema.rb index 468d478a93..ef2c12e7d2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -51,7 +51,7 @@ ActiveRecord::Schema.define(version: 2022_01_18_053107) do t.datetime "updated_at", null: false t.integer "bill_address_id" t.integer "ship_address_id" - t.string "backup_name", limit: 255 + t.string "name", limit: 255 t.boolean "allow_charges", default: false, null: false t.datetime "terms_and_conditions_accepted_at" t.string "first_name", default: "", null: false From feaa92406aea283f621706bc88ce45579c7e52e4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 15 Feb 2022 14:52:19 +0000 Subject: [PATCH 24/27] Split migration into two parts for easier testing --- .../20220105085729_split_customers_name.rb | 21 ---------------- .../20220105085730_migrate_customers_data.rb | 24 +++++++++++++++++++ 2 files changed, 24 insertions(+), 21 deletions(-) create mode 100644 db/migrate/20220105085730_migrate_customers_data.rb diff --git a/db/migrate/20220105085729_split_customers_name.rb b/db/migrate/20220105085729_split_customers_name.rb index ccf7bd364e..d52f122b9c 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -1,34 +1,13 @@ # frozen_string_literal: true class SplitCustomersName < ActiveRecord::Migration[6.1] - class SpreeAddress < ApplicationRecord - end - - class Customer < ApplicationRecord - belongs_to :bill_address, class_name: "SpreeAddress" - end - def up add_column :customers, :first_name, :string, null: false, default: "" add_column :customers, :last_name, :string, null: false, default: "" - - migrate_customer_name_data! end def down remove_column :customers, :first_name remove_column :customers, :last_name end - - def migrate_customer_name_data! - Customer.where(first_name: "", last_name: "").where.not(name: [nil, ""]).find_each do |customer| - name_words = customer.name.split(' ') - next if name_words.empty? - - customer.update( - first_name: name_words.first, - last_name: name_words[1..].join(' ') - ) - end - end end diff --git a/db/migrate/20220105085730_migrate_customers_data.rb b/db/migrate/20220105085730_migrate_customers_data.rb new file mode 100644 index 0000000000..096465d7b7 --- /dev/null +++ b/db/migrate/20220105085730_migrate_customers_data.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class MigrateCustomersData < ActiveRecord::Migration[6.1] + class SpreeAddress < ApplicationRecord; end + class Customer < ApplicationRecord + belongs_to :bill_address, class_name: "SpreeAddress" + end + + def up + migrate_customer_name_data! + end + + def migrate_customer_name_data! + Customer.where(first_name: "", last_name: "").where.not(name: [nil, ""]).find_each do |customer| + name_words = customer.name.split(' ') + next if name_words.empty? + + customer.update( + first_name: name_words.first, + last_name: name_words[1..].join(' ') + ) + end + end +end From fd815a6af674e308c876e5ee2b476b73da40834c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 15 Feb 2022 15:41:48 +0000 Subject: [PATCH 25/27] Don't change Customer factory name generating logic --- spec/factories/customer_factory.rb | 2 -- spec/system/admin/subscriptions_spec.rb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/factories/customer_factory.rb b/spec/factories/customer_factory.rb index bc4a9e6980..a661255146 100644 --- a/spec/factories/customer_factory.rb +++ b/spec/factories/customer_factory.rb @@ -2,8 +2,6 @@ FactoryBot.define do factory :customer, class: Customer do - first_name { FFaker::Name.first_name } - last_name { FFaker::Name.last_name } email { generate(:random_email) } enterprise code { SecureRandom.base64(150) } diff --git a/spec/system/admin/subscriptions_spec.rb b/spec/system/admin/subscriptions_spec.rb index 6b74f7cd63..01b9fdab3a 100644 --- a/spec/system/admin/subscriptions_spec.rb +++ b/spec/system/admin/subscriptions_spec.rb @@ -19,7 +19,7 @@ describe 'Subscriptions' do let!(:subscription) { create(:subscription, shop: shop, with_items: true, with_proxy_orders: true) } - let!(:customer) { create(:customer) } + let!(:customer) { create(:customer, first_name: "Timmy", last_name: "Test") } let!(:other_subscription) { create(:subscription, shop: shop, customer: customer, with_items: true, with_proxy_orders: true) From 4c50ca69ea9c2c33ede9b15795a843d052f8bf6e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 15 Feb 2022 15:42:29 +0000 Subject: [PATCH 26/27] Improve data migration logic and add test coverage --- .../20220105085730_migrate_customers_data.rb | 47 ++++++++++++++++--- spec/migrations/split_customer_names_spec.rb | 43 +++++++++++++++++ 2 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 spec/migrations/split_customer_names_spec.rb diff --git a/db/migrate/20220105085730_migrate_customers_data.rb b/db/migrate/20220105085730_migrate_customers_data.rb index 096465d7b7..7e7578e441 100644 --- a/db/migrate/20220105085730_migrate_customers_data.rb +++ b/db/migrate/20220105085730_migrate_customers_data.rb @@ -11,14 +11,47 @@ class MigrateCustomersData < ActiveRecord::Migration[6.1] end def migrate_customer_name_data! - Customer.where(first_name: "", last_name: "").where.not(name: [nil, ""]).find_each do |customer| - name_words = customer.name.split(' ') - next if name_words.empty? + customers_with_bill_addresses.find_each do |customer| + if bill_address_name_matches?(customer) + apply_name_from_bill_address!(customer) + next + end - customer.update( - first_name: name_words.first, - last_name: name_words[1..].join(' ') - ) + split_customer_name!(customer) + end + + customers_without_bill_addresses.find_each do |customer| + split_customer_name!(customer) end end + + def customers_with_bill_addresses + Customer.joins(:bill_address).where(first_name: "", last_name: "").where.not(name: [nil, ""]) + end + + def customers_without_bill_addresses + Customer.where(bill_address_id: nil, first_name: "", last_name: "").where.not(name: [nil, ""]) + end + + def bill_address_name_matches?(customer) + customer.name.delete(" ") == (customer.bill_address.firstname + customer.bill_address.lastname).delete(" ") + end + + def split_customer_name!(customer) + return if (name_parts = customer.name.split(' ')).empty? + + customer.update_columns( + first_name: name_parts.first, + last_name: name_parts[1..].join(' '), + updated_at: Time.zone.now + ) + end + + def apply_name_from_bill_address!(customer) + customer.update_columns( + first_name: customer.bill_address.firstname, + last_name: customer.bill_address.lastname, + updated_at: Time.zone.now + ) + end end diff --git a/spec/migrations/split_customer_names_spec.rb b/spec/migrations/split_customer_names_spec.rb new file mode 100644 index 0000000000..cef063a581 --- /dev/null +++ b/spec/migrations/split_customer_names_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative '../../db/migrate/20220105085730_migrate_customers_data' + +describe MigrateCustomersData do + let!(:customer1) { + create(:customer, name: "Timmy Test", first_name: "", last_name: "", bill_address: nil) + } + let!(:customer2) { + create(:customer, name: "Frank Lee Ridiculous", first_name: "", last_name: "", + bill_address: create(:address, first_name: "Frank Lee", last_name: "Ridiculous")) + } + let!(:customer3) { + create(:customer, name: "Shia Le Boeuf", first_name: "", last_name: "", + bill_address: create(:address, first_name: "Shia", last_name: "Le Boeuf")) + } + let!(:customer4) { + create(:customer, name: "No Eyed Deer", first_name: "", last_name: "", bill_address: nil) + } + let!(:customer5) { + create(:customer, name: " Space Invader ", first_name: "", last_name: "", bill_address: nil) + } + let!(:customer6) { + create(:customer, name: "How Many Names Do You Need?", first_name: "", last_name: "", bill_address: nil) + } + let!(:customer7) { + create(:customer, name: "Customer Name", first_name: "", last_name: "", + bill_address: create(:address, first_name: "Different", last_name: "AddressName")) + } + + it "migrates customer names" do + subject.up + + expect([customer1.reload.first_name, customer1.last_name]).to eq ["Timmy", "Test"] + expect([customer2.reload.first_name, customer2.last_name]).to eq ["Frank Lee", "Ridiculous"] + expect([customer3.reload.first_name, customer3.last_name]).to eq ["Shia", "Le Boeuf"] + expect([customer4.reload.first_name, customer4.last_name]).to eq ["No", "Eyed Deer"] + expect([customer5.reload.first_name, customer5.last_name]).to eq ["Space", "Invader"] + expect([customer6.reload.first_name, customer6.last_name]).to eq ["How", "Many Names Do You Need?"] + expect([customer7.reload.first_name, customer7.last_name]).to eq ["Customer", "Name"] + end +end From a6dee77071a8a2dfb3ae25be5fb8496a58664fda Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 16 Feb 2022 10:33:49 +1100 Subject: [PATCH 27/27] Style --- .../20220105085730_migrate_customers_data.rb | 4 ++- spec/migrations/split_customer_names_spec.rb | 35 ++++++++++++------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/db/migrate/20220105085730_migrate_customers_data.rb b/db/migrate/20220105085730_migrate_customers_data.rb index 7e7578e441..9968d8e400 100644 --- a/db/migrate/20220105085730_migrate_customers_data.rb +++ b/db/migrate/20220105085730_migrate_customers_data.rb @@ -2,6 +2,7 @@ class MigrateCustomersData < ActiveRecord::Migration[6.1] class SpreeAddress < ApplicationRecord; end + class Customer < ApplicationRecord belongs_to :bill_address, class_name: "SpreeAddress" end @@ -34,7 +35,8 @@ class MigrateCustomersData < ActiveRecord::Migration[6.1] end def bill_address_name_matches?(customer) - customer.name.delete(" ") == (customer.bill_address.firstname + customer.bill_address.lastname).delete(" ") + address_name = customer.bill_address.firstname + customer.bill_address.lastname + customer.name.delete(" ") == address_name.delete(" ") end def split_customer_name!(customer) diff --git a/spec/migrations/split_customer_names_spec.rb b/spec/migrations/split_customer_names_spec.rb index cef063a581..76c3803553 100644 --- a/spec/migrations/split_customer_names_spec.rb +++ b/spec/migrations/split_customer_names_spec.rb @@ -8,8 +8,9 @@ describe MigrateCustomersData do create(:customer, name: "Timmy Test", first_name: "", last_name: "", bill_address: nil) } let!(:customer2) { - create(:customer, name: "Frank Lee Ridiculous", first_name: "", last_name: "", - bill_address: create(:address, first_name: "Frank Lee", last_name: "Ridiculous")) + create(:customer, + name: "Frank Lee Ridiculous", first_name: "", last_name: "", + bill_address: create(:address, first_name: "Frank Lee", last_name: "Ridiculous")) } let!(:customer3) { create(:customer, name: "Shia Le Boeuf", first_name: "", last_name: "", @@ -19,25 +20,33 @@ describe MigrateCustomersData do create(:customer, name: "No Eyed Deer", first_name: "", last_name: "", bill_address: nil) } let!(:customer5) { - create(:customer, name: " Space Invader ", first_name: "", last_name: "", bill_address: nil) + create(:customer, name: " Space Invader ", first_name: "", last_name: "", + bill_address: nil) } let!(:customer6) { - create(:customer, name: "How Many Names Do You Need?", first_name: "", last_name: "", bill_address: nil) + create(:customer, name: "How Many Names Do You Need?", first_name: "", last_name: "", + bill_address: nil) } let!(:customer7) { - create(:customer, name: "Customer Name", first_name: "", last_name: "", - bill_address: create(:address, first_name: "Different", last_name: "AddressName")) + create(:customer, + name: "Customer Name", first_name: "", last_name: "", + bill_address: create(:address, first_name: "Different", last_name: "AddressName")) } it "migrates customer names" do subject.up - expect([customer1.reload.first_name, customer1.last_name]).to eq ["Timmy", "Test"] - expect([customer2.reload.first_name, customer2.last_name]).to eq ["Frank Lee", "Ridiculous"] - expect([customer3.reload.first_name, customer3.last_name]).to eq ["Shia", "Le Boeuf"] - expect([customer4.reload.first_name, customer4.last_name]).to eq ["No", "Eyed Deer"] - expect([customer5.reload.first_name, customer5.last_name]).to eq ["Space", "Invader"] - expect([customer6.reload.first_name, customer6.last_name]).to eq ["How", "Many Names Do You Need?"] - expect([customer7.reload.first_name, customer7.last_name]).to eq ["Customer", "Name"] + [ + customer1, customer2, customer3, customer4, + customer5, customer6, customer7, + ].map(&:reload) + + expect([customer1.first_name, customer1.last_name]).to eq ["Timmy", "Test"] + expect([customer2.first_name, customer2.last_name]).to eq ["Frank Lee", "Ridiculous"] + expect([customer3.first_name, customer3.last_name]).to eq ["Shia", "Le Boeuf"] + expect([customer4.first_name, customer4.last_name]).to eq ["No", "Eyed Deer"] + expect([customer5.first_name, customer5.last_name]).to eq ["Space", "Invader"] + expect([customer6.first_name, customer6.last_name]).to eq ["How", "Many Names Do You Need?"] + expect([customer7.first_name, customer7.last_name]).to eq ["Customer", "Name"] end end