Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Split name column in customers table #8763

Merged
merged 27 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d0f347e
Add migration to split Customers.name into first and last names
achauve Jan 6, 2022
5ca4d54
Update customer creation
achauve Jan 6, 2022
ba6523b
Add Customer.full_name
achauve Jan 6, 2022
9b93102
More fixes
achauve Jan 7, 2022
ca46359
More fixes
achauve Jan 10, 2022
7c25127
Improve first_name and last_name setup
Jan 20, 2022
eefd989
Add en translations
Jan 20, 2022
75345a9
Update customer factory
Jan 20, 2022
23776c7
Fix more specs
Jan 20, 2022
836a60a
Add missing full_name bind to subscriptions serializer
Jan 23, 2022
b8afb7e
Add default values for new fields
Feb 1, 2022
d016c47
Refactor data migration to use data sets
Feb 1, 2022
9682b92
Arrange classes loading for good migration
Feb 2, 2022
5030216
Update migration with better logic and cosmetics
Feb 3, 2022
4cb31d0
Repair specs with default values on ensure_customer method
Feb 4, 2022
6d986de
Remove translation changes from locale file
Feb 8, 2022
5c9dd81
Deal with nil attirubtes on bill_address
Feb 8, 2022
554a862
Refactor ensure_customer method
Feb 8, 2022
07314af
Spec customer record association more realistically
mkllnk Feb 9, 2022
d09ba16
Associate customers again
mkllnk Feb 9, 2022
de4d074
Remove bill_address fetching logic
Feb 11, 2022
9a12957
Handle the empty-space name on customers
Feb 11, 2022
68193ef
Keep old customers.name column for compatibilty
mkllnk Feb 15, 2022
feaa924
Split migration into two parts for easier testing
Matt-Yorkley Feb 15, 2022
fd815a6
Don't change Customer factory name generating logic
Matt-Yorkley Feb 15, 2022
4c50ca6
Improve data migration logic and add test coverage
Matt-Yorkley Feb 15, 2022
a6dee77
Style
mkllnk Feb 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/admin/customers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/spree/admin/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion app/models/customer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -34,6 +34,10 @@ class Customer < ApplicationRecord

attr_accessor :gateway_recurring_payment_client_secret, :gateway_shop_id

def full_name
"#{first_name} #{last_name}".strip
end

private

def downcase_email
Expand Down
19 changes: 10 additions & 9 deletions app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down Expand Up @@ -711,23 +710,25 @@ 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
Comment on lines 710 to 714
Copy link
Member

Choose a reason for hiding this comment

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

associate_customer is a before_validation action. After your change, it doesn't have any effect any more. But if ensure_customer is called then we don't need to call associate_customer beforehand anyway. So maybe we can remove that action.

before_validation :associate_customer, unless: :customer_id?
before_validation :ensure_customer, unless: :customer_is_valid?

I'm just wondering about edge cases now. So if ensure_customer is executed, it includes associate_customer and everything is good. The logic would only change if ensure_customer is not executed but associate_customer would. The conditions for that is:

  • no customer id present
  • customer is valid:
    • the order is a new record or in cart state and therefore we don't need a customer record yet
    • customer is present (impossible due to first condition above) and other conditions that don't matter

So the only case in which we would have tried to associate a customer without creating a new record is when the order is new or in cart state. I could imagine that this is needed to associate tags during shopping to that customer specific products, discounts or fees can be applied.

I'm not sure if this is handled somewhere else but you might be introducing a bug here. I do think that this part of the code needs refactoring but maybe the full refactor is out of scope. I would leave the self.customer = in the code to keep the old behaviour here and then we clean it up another time.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think that I have an idea how to resolve this. I'll add a commit to your branch and we need to extend the testing notes with the case above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, the associate_user method is used in order_cart_reset.rb so I guess I am introducing a bug here. I was too optimistic with the private section. I should have checked more carefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw about the mix between associate_customer and the ensure_customer methods but I was not sure how to refactor it quickly maintaining the existing specs. I should have refactor the specs first as you did, looks great.


def ensure_customer
return if associate_customer
def create_customer
return if customer_is_valid?

self.customer = Customer.new(
Customer.create(
enterprise: distributor,
email: email_for_customer,
user: user,
name: bill_address&.full_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
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)
Expand Down
8 changes: 4 additions & 4 deletions app/serializers/api/admin/customer_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 12 additions & 4 deletions app/serializers/api/admin/subscription_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ 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_name, :schedule_name, :edit_path, :canceled_at, :paused_at, :state,
: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
Expand Down Expand Up @@ -34,8 +34,16 @@ 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
Comment on lines +37 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the SubscriptionSerializer actually use the customer_first_name and customer_last_name attributes anywhere? I think we can drop them?


def customer_full_name
object.customer&.full_name
end

def schedule_name
Expand Down
2 changes: 1 addition & 1 deletion app/serializers/api/customer_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 10 additions & 5 deletions app/views/admin/customers/index.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
Expand All @@ -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')
Expand All @@ -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' => 'first_name'}
%td.last_name{ 'ng-show' => 'columns.last_name.visible'}
jibees marked this conversation as resolved.
Show resolved Hide resolved
%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'} }
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/subscriptions/_table.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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)' } }
Expand Down
2 changes: 2 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
13 changes: 13 additions & 0 deletions db/migrate/20220105085729_split_customers_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true

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: ""
end

def down
remove_column :customers, :first_name
remove_column :customers, :last_name
end
end
59 changes: 59 additions & 0 deletions db/migrate/20220105085730_migrate_customers_data.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# frozen_string_literal: true

class MigrateCustomersData < ActiveRecord::Migration[6.1]
Copy link
Member

Choose a reason for hiding this comment

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

Good call to split! It could have been MigrateCustomersName...

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!
customers_with_bill_addresses.find_each do |customer|
Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I thought we could do and then Lynne suggested that we don't need this. But okay, it's done now. Nice work.

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)
address_name = customer.bill_address.firstname + customer.bill_address.lastname
customer.name.delete(" ") == address_name.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
2 changes: 2 additions & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
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
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down Expand Up @@ -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|
Expand Down
7 changes: 4 additions & 3 deletions lib/open_food_network/column_preference_defaults.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ def customers_index_columns
node = 'admin.customers.index'
{
email: { name: I18n.t("admin.email"), visible: true },
name: { name: I18n.t("admin.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 }
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/spree/admin/search_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions spec/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

pacodelaluna marked this conversation as resolved.
Show resolved Hide resolved
factory :stripe_account do
enterprise { FactoryBot.create(:distributor_enterprise) }
stripe_user_id { "abc123" }
Expand Down
11 changes: 11 additions & 0 deletions spec/factories/customer_factory.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

FactoryBot.define do
factory :customer, class: Customer do
email { generate(:random_email) }
enterprise
code { SecureRandom.base64(150) }
user
bill_address { create(:address) }
end
end
Loading