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 00000000000..2d1cd9f5fbc --- /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 b1c9499ddbf..3fdb1f4cbff 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -51,9 +51,11 @@ 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 c73f4453f0d..5380a273d41 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -721,7 +721,8 @@ def ensure_customer 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 96a14d01d25..72f37c66e22 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 fe31ab000a1..b0e94056203 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1054,7 +1054,7 @@ 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 fc3fbad9555..1ddb023b3a5 100644 --- a/app/controllers/spree/admin/search_controller.rb +++ b/app/controllers/spree/admin/search_controller.rb @@ -21,7 +21,8 @@ def customers @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 72f37c66e22..3c9cf3715a6 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 30c067349b5..14e8478e49b 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 c75657ba992..4fab4ef7695 100644 --- a/app/serializers/api/admin/subscription_serializer.rb +++ b/app/serializers/api/admin/subscription_serializer.rb @@ -5,8 +5,8 @@ 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_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 @@ def customer_email 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 053313cb30d..72423714ba9 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 7000620af28..370dae94681 100644 --- a/spec/controllers/spree/admin/search_controller_spec.rb +++ b/spec/controllers/spree/admin/search_controller_spec.rb @@ -35,7 +35,7 @@ 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 71cc8fc2c81..28fcf235ba9 100644 --- a/app/controllers/admin/customers_controller.rb +++ b/app/controllers/admin/customers_controller.rb @@ -99,7 +99,7 @@ def ams_prefix_whitelist 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 3c9cf3715a6..ec4d78235c5 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 e2bff1832fa..18bf810ee8a 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 2e36241b0fb..1733e703d92 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 3f135f6b61a..4f3cc21f3f8 100644 --- a/lib/open_food_network/column_preference_defaults.rb +++ b/lib/open_food_network/column_preference_defaults.rb @@ -30,7 +30,8 @@ def customers_index_columns 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 3692d90bf46..b7e63e8e2c9 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -177,7 +177,8 @@ 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 2d1cd9f5fbc..7cde6e047d2 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -12,10 +12,18 @@ def change 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 6c23d730221..e639a9faa5f 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 f766c0cb195..73a46e9109c 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -113,14 +113,6 @@ 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 00000000000..bc4a9e69806 --- /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 18bf810ee8a..11241206936 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 c4eb12b891a..67dced6881e 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 59a5cd04709..0be3d779212 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 @@ def select_attributes 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 44cf80200d5..28d240a296b 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 @@ 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 @@ 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 4f3cc21f3f8..cd162d26383 100644 --- a/lib/open_food_network/column_preference_defaults.rb +++ b/lib/open_food_network/column_preference_defaults.rb @@ -30,10 +30,10 @@ def customers_index_columns 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 b7e63e8e2c9..eb4fca83635 100644 --- a/spec/system/admin/customers_spec.rb +++ b/spec/system/admin/customers_spec.rb @@ -14,7 +14,9 @@ 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 @@ 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 @@ # 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 @@ 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 @@ # 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 dae71d2d86f..7182da08511 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -239,7 +239,7 @@ def new_order_with_distribution(distributor, order_cycle) 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 41807ec0e44..6b74f7cd63f 100644 --- a/spec/system/admin/subscriptions_spec.rb +++ b/spec/system/admin/subscriptions_spec.rb @@ -19,7 +19,7 @@ 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 @@ 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 4fab4ef7695..cdaecf432e0 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 @@ def customer_last_name 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 7cde6e047d2..19c0a3e5944 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -3,8 +3,8 @@ class Customer < ActiveRecord::Base 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 3fdb1f4cbff..468d478a932 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -54,8 +54,8 @@ 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 19c0a3e5944..d52d619747b 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 d52d619747b..4d8ddd53491 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 4d8ddd53491..bf76a65cecb 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 @@ def down 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 5380a273d41..a56028a614d 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -717,16 +717,15 @@ def associate_customer 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 1733e703d92..2e36241b0fb 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 bf76a65cecb..e219869827d 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -24,8 +24,8 @@ def migrate_customer_name_data! 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 a56028a614d..678a0fc8d1f 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -711,13 +711,11 @@ def email_for_customer 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 @@ def ensure_customer 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 b0e94056203..a244f1670bb 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -969,9 +969,8 @@ 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 @@ 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 | 122 +++++++++----------------------- 1 file changed, 35 insertions(+), 87 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index a244f1670bb..c3dd67e7f2a 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -940,110 +940,58 @@ context "when creating an order" do it "does not create a customer" do - order = create(:order, distributor: distributor) - expect(order.customer).to be_nil + expect { + create(:order, distributor: distributor) + }.to_not change { + Customer.count + } end - end - context "when updating the order" do - let!(:order) { create(:order, distributor: distributor) } + it "associates an existing customer" do + pending "the last commit broke associating orders" - 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') - } + customer = create( + :customer, + user: user, + email: user.email, + enterprise: distributor + ) + order = create(:order, user: user, distributor: distributor) - 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 + expect(order.customer).to eq customer 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 "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 + context "when updating the order" do + before do + order.update!(distributor: distributor) end - end - end - describe "ensuring a customer is linked" do - let(:distributor) { create(:distributor_enterprise) } - let!(:order) { create(:order, distributor: distributor) } + it "associates an existing customer" do + customer = create( + :customer, + user: user, + email: user.email, + enterprise: 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) } + order.update!(state: "complete") - 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 + it "doesn't create a customer before needed" do + expect(order.customer).to eq nil 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 - - 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 + it "creates a customer" do + expect { + order.update!(state: "complete") + }.to change { + Customer.count + }.by(1) - 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 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 678a0fc8d1f..2693ef5bb9d 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -84,8 +84,7 @@ def states 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 @@ def associate_customer 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 c3dd67e7f2a..2a87a040098 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -948,8 +948,6 @@ 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 e219869827d..d6affb9a433 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -20,15 +20,6 @@ def down 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 d6affb9a433..50e5409c9f2 100644 --- a/db/migrate/20220105085729_split_customers_name.rb +++ b/db/migrate/20220105085729_split_customers_name.rb @@ -22,6 +22,7 @@ def down 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 50e5409c9f2..ccf7bd364e0 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 Customer < ApplicationRecord 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 @@ def up 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 468d478a932..ef2c12e7d22 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -51,7 +51,7 @@ 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 ccf7bd364e0..d52f122b9cb 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 00000000000..096465d7b74 --- /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 bc4a9e69806..a6612551469 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 6b74f7cd63f..01b9fdab3ab 100644 --- a/spec/system/admin/subscriptions_spec.rb +++ b/spec/system/admin/subscriptions_spec.rb @@ -19,7 +19,7 @@ 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 | 49 ++++++++++++++++--- spec/migrations/split_customer_names_spec.rb | 43 ++++++++++++++++ 2 files changed, 84 insertions(+), 8 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 096465d7b74..7e7578e4414 100644 --- a/db/migrate/20220105085730_migrate_customers_data.rb +++ b/db/migrate/20220105085730_migrate_customers_data.rb @@ -11,14 +11,47 @@ def up 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(' ') - ) + customers_with_bill_addresses.find_each do |customer| + if bill_address_name_matches?(customer) + apply_name_from_bill_address!(customer) + next + end + + 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 00000000000..cef063a581b --- /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 7e7578e4414..9968d8e4001 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 @@ def customers_without_bill_addresses 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 cef063a581b..76c38035536 100644 --- a/spec/migrations/split_customer_names_spec.rb +++ b/spec/migrations/split_customer_names_spec.rb @@ -8,8 +8,9 @@ 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 @@ 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