From 9b48adc7e380d4d72c570b493d5138c998833a93 Mon Sep 17 00:00:00 2001 From: Filippo Liverani <dev@mailvore.com> Date: Tue, 18 Feb 2020 17:00:54 +0100 Subject: [PATCH] Enable Address name usage in backend Favor name attribute usage in backend over firstname and lastname. To avoid internally relying on deprecated code for backwards compatibility, deprecated fields are rendered in HTML views and API responses instead of name field only when `show_deprecated_attributes` preference is set to true. --- .../orders/customer_details/autocomplete.hbs | 4 +- .../javascripts/spree/backend/user_picker.js | 3 +- .../spree/backend/views/order/address.js | 2 +- .../backend/views/order/customer_select.js | 3 +- .../spree/backend/sections/_orders.scss | 6 ++- .../spree/admin/search_controller.rb | 3 +- .../views/spree/admin/orders/index.html.erb | 23 ++++++---- .../spree/admin/search/users.json.jbuilder | 16 ++++++- .../spree/admin/shared/_address_form.html.erb | 23 ++++++---- .../spree/admin/search_controller_spec.rb | 22 +-------- .../spree/admin/users_controller_spec.rb | 21 ++++----- .../admin/orders/customer_details_spec.rb | 17 +++---- .../features/admin/orders/new_order_spec.rb | 3 +- .../features/admin/orders/payments_spec.rb | 46 ++++++++++--------- .../app/models/concerns/spree/user_methods.rb | 3 +- core/app/models/spree/address.rb | 3 +- core/app/models/spree/order.rb | 3 +- core/config/locales/en.yml | 1 + 18 files changed, 107 insertions(+), 95 deletions(-) diff --git a/backend/app/assets/javascripts/spree/backend/templates/orders/customer_details/autocomplete.hbs b/backend/app/assets/javascripts/spree/backend/templates/orders/customer_details/autocomplete.hbs index f15fc508d82..db6b4eff4a1 100644 --- a/backend/app/assets/javascripts/spree/backend/templates/orders/customer_details/autocomplete.hbs +++ b/backend/app/assets/javascripts/spree/backend/templates/orders/customer_details/autocomplete.hbs @@ -1,9 +1,9 @@ <div class='customer-autocomplete-item'> <div class='customer-details'> <span class="customer-email">{{customer.email}}</span> - {{#if bill_address.firstname }} + {{#if bill_address.name }} <strong>{{t 'bill_address' }}</strong> - {{bill_address.firstname}} {{bill_address.lastname}}<br> + {{bill_address.name}}<br> {{bill_address.address1}}, {{bill_address.address2}}<br> {{bill_address.city}}<br> {{#if bill_address.state_id }} diff --git a/backend/app/assets/javascripts/spree/backend/user_picker.js b/backend/app/assets/javascripts/spree/backend/user_picker.js index 47de8625e99..dd7bcbb3e20 100644 --- a/backend/app/assets/javascripts/spree/backend/user_picker.js +++ b/backend/app/assets/javascripts/spree/backend/user_picker.js @@ -28,8 +28,7 @@ $.fn.userAutocomplete = function () { q: { m: 'or', email_start: term, - addresses_firstname_start: term, - addresses_lastname_start: term + name_start: term } }; }, diff --git a/backend/app/assets/javascripts/spree/backend/views/order/address.js b/backend/app/assets/javascripts/spree/backend/views/order/address.js index ae35a7c7fc8..e7cd6376e70 100644 --- a/backend/app/assets/javascripts/spree/backend/views/order/address.js +++ b/backend/app/assets/javascripts/spree/backend/views/order/address.js @@ -23,7 +23,7 @@ Spree.Views.Order.Address = Backbone.View.extend({ eachField: function(callback){ var view = this; - var fields = ["firstname", "lastname", "company", "address1", "address2", + var fields = ["name", "company", "address1", "address2", "city", "zipcode", "phone", "country_id", "state_name"]; _.each(fields, function(field) { var el = view.$('[name$="[' + field + ']"]'); diff --git a/backend/app/assets/javascripts/spree/backend/views/order/customer_select.js b/backend/app/assets/javascripts/spree/backend/views/order/customer_select.js index d55d0e1d284..801963a1eb1 100644 --- a/backend/app/assets/javascripts/spree/backend/views/order/customer_select.js +++ b/backend/app/assets/javascripts/spree/backend/views/order/customer_select.js @@ -34,8 +34,7 @@ Spree.Views.Order.CustomerSelect = Backbone.View.extend({ q: { m: 'or', email_start: term, - addresses_firstname_start: term, - addresses_lastname_start: term + name_start: term } } }, diff --git a/backend/app/assets/stylesheets/spree/backend/sections/_orders.scss b/backend/app/assets/stylesheets/spree/backend/sections/_orders.scss index b5eb68a216f..b29088a1935 100644 --- a/backend/app/assets/stylesheets/spree/backend/sections/_orders.scss +++ b/backend/app/assets/stylesheets/spree/backend/sections/_orders.scss @@ -65,7 +65,11 @@ } label[for="order_bill_address_attributes_firstname"] { - margin-top: 45px; + margin-top: 53.5px; +} + +label[for="order_bill_address_attributes_name"] { + margin-top: 53.5px; } #risk_analysis legend { diff --git a/backend/app/controllers/spree/admin/search_controller.rb b/backend/app/controllers/spree/admin/search_controller.rb index 80f2f54c94a..9ca798bfa62 100644 --- a/backend/app/controllers/spree/admin/search_controller.rb +++ b/backend/app/controllers/spree/admin/search_controller.rb @@ -16,8 +16,7 @@ def users @users = Spree.user_class.ransack({ m: 'or', email_start: params[:q], - addresses_firstname_start: params[:q], - addresses_lastname_start: params[:q] + name_start: params[:q] }).result.limit(10) end end diff --git a/backend/app/views/spree/admin/orders/index.html.erb b/backend/app/views/spree/admin/orders/index.html.erb index ca4d830e63c..76b09818f67 100644 --- a/backend/app/views/spree/admin/orders/index.html.erb +++ b/backend/app/views/spree/admin/orders/index.html.erb @@ -65,14 +65,21 @@ </div> <div class="col-12 col-xl-6"> - <div class="field"> - <%= label_tag :q_bill_address_firstname_start, t('spree.first_name_begins_with') %> - <%= f.text_field :bill_address_firstname_start, size: 25 %> - </div> - <div class="field"> - <%= label_tag :q_bill_address_lastname_start, t('spree.last_name_begins_with') %> - <%= f.text_field :bill_address_lastname_start, size: 25%> - </div> + <% if Spree::Config.use_combined_first_and_last_name_in_address %> + <div class="field"> + <%= label_tag :q_bill_address_name_start, t('spree.name_contains') %> + <%= f.text_field :bill_address_name_start, size: 25 %> + </div> + <% else %> + <div class="field"> + <%= label_tag :q_bill_address_firstname_start, t('spree.first_name_begins_with') %> + <%= f.text_field :bill_address_firstname_start, size: 25 %> + </div> + <div class="field"> + <%= label_tag :q_bill_address_lastname_start, t('spree.last_name_begins_with') %> + <%= f.text_field :bill_address_lastname_start, size: 25 %> + </div> + <% end %> </div> <div class="col-12 col-xl-6"> diff --git a/backend/app/views/spree/admin/search/users.json.jbuilder b/backend/app/views/spree/admin/search/users.json.jbuilder index 24f87bb0445..83f70c666ab 100644 --- a/backend/app/views/spree/admin/search/users.json.jbuilder +++ b/backend/app/views/spree/admin/search/users.json.jbuilder @@ -4,7 +4,21 @@ json.array!(@users) do |user| json.id user.id json.email user.email - address_fields = [:firstname, :lastname, :address1, :address2, :city, :zipcode, :phone, :state_name, :state_id, :country_id, :company] + address_fields = [ + :name, + :address1, + :address2, + :city, + :zipcode, + :phone, + :state_name, + :state_id, + :country_id, + :company + ] + unless Spree::Config.use_combined_first_and_last_name_in_address + address_fields.push(:firstname, :lastname) + end json.ship_address do if user.ship_address json.(user.ship_address, *address_fields) diff --git a/backend/app/views/spree/admin/shared/_address_form.html.erb b/backend/app/views/spree/admin/shared/_address_form.html.erb index cb1e1dd9293..dda71610d3b 100644 --- a/backend/app/views/spree/admin/shared/_address_form.html.erb +++ b/backend/app/views/spree/admin/shared/_address_form.html.erb @@ -1,15 +1,22 @@ <% s_or_b = type.chars.first %> <div id="<%= type %>" data-hook="address_fields"> - <div class="field <%= "#{type}-row" %>"> - <%= f.label :firstname %> - <%= f.text_field :firstname, class: 'fullwidth' %> - </div> + <% if Spree::Config.use_combined_first_and_last_name_in_address %> + <div class="field <%= "#{type}-row" %>"> + <%= f.label :name %> + <%= f.text_field :name, class: 'fullwidth' %> + </div> + <% else %> + <div class="field <%= "#{type}-row" %>"> + <%= f.label :firstname %> + <%= f.text_field :firstname, class: 'fullwidth' %> + </div> - <div class="field <%= "#{type}-row" %>"> - <%= f.label :lastname %> - <%= f.text_field :lastname, class: 'fullwidth' %> - </div> + <div class="field <%= "#{type}-row" %>"> + <%= f.label :lastname %> + <%= f.text_field :lastname, class: 'fullwidth' %> + </div> + <% end %> <% if Spree::Config[:company] %> <div class="field <%= "#{type}-row" %>"> diff --git a/backend/spec/controllers/spree/admin/search_controller_spec.rb b/backend/spec/controllers/spree/admin/search_controller_spec.rb index 199795dffd2..adab87f5e28 100644 --- a/backend/spec/controllers/spree/admin/search_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/search_controller_spec.rb @@ -39,27 +39,9 @@ end end - context 'when searching by ship addresss first name' do + context 'when searching by bill address name' do it_should_behave_like 'user found by search' do - let(:user_attribute) { user.ship_address.firstname } - end - end - - context 'when searching by ship address last name' do - it_should_behave_like 'user found by search' do - let(:user_attribute) { user.ship_address.lastname } - end - end - - context 'when searching by bill address first name' do - it_should_behave_like 'user found by search' do - let(:user_attribute) { user.bill_address.firstname } - end - end - - context 'when searching by bill address last name' do - it_should_behave_like 'user found by search' do - let(:user_attribute) { user.bill_address.lastname } + let(:user_attribute) { user.bill_address.name } end end end diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 415d7a935b8..d0f772fce08 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/users_controller_spec.rb @@ -8,8 +8,7 @@ let(:state) { create(:state, state_code: 'NY') } let(:valid_address_attributes) do { - firstname: 'Foo', - lastname: 'Bar', + name: 'Foo Bar', city: "New York", country_id: state.country.id, state_id: state.id, @@ -89,24 +88,24 @@ def user end it "can create user with roles" do - post :create, params: { user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } } + post :create, params: { user: { name: "Bob", spree_role_ids: [dummy_role.id] } } expect(user.spree_roles).to eq([dummy_role]) end it "can create user without roles" do - post :create, params: { user: { first_name: "Bob" } } + post :create, params: { user: { name: "Bob" } } expect(user.spree_roles).to eq([]) end end context "when the user cannot manage roles" do it "cannot assign users roles" do - post :create, params: { user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } } + post :create, params: { user: { name: "Bob", spree_role_ids: [dummy_role.id] } } expect(user.spree_roles).to eq([]) end it "can create user without roles" do - post :create, params: { user: { first_name: "Bob" } } + post :create, params: { user: { name: "Bob" } } expect(user.spree_roles).to eq([]) end end @@ -145,21 +144,21 @@ def user it "can set roles" do expect { - put :update, params: { id: user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } } + put :update, params: { id: user.id, user: { name: "Bob", spree_role_ids: [dummy_role.id] } } }.to change { user.reload.spree_roles.to_a }.to([dummy_role]) end it "can clear roles" do user.spree_roles << dummy_role expect { - put :update, params: { id: user.id, user: { first_name: "Bob", spree_role_ids: [""] } } + put :update, params: { id: user.id, user: { name: "Bob", spree_role_ids: [""] } } }.to change { user.reload.spree_roles.to_a }.to([]) end context 'when no role params are present' do it 'does not clear all present user roles' do user.spree_roles << dummy_role - put :update, params: { id: user.id, user: { first_name: "Bob" } } + put :update, params: { id: user.id, user: { name: "Bob" } } expect(user.reload.spree_roles).to_not be_empty end end @@ -168,14 +167,14 @@ def user context "when the user cannot manage roles" do it "cannot set roles" do expect { - put :update, params: { id: user.id, user: { first_name: "Bob", spree_role_ids: [dummy_role.id] } } + put :update, params: { id: user.id, user: { name: "Bob", spree_role_ids: [dummy_role.id] } } }.not_to change { user.reload.spree_roles.to_a } end it "cannot clear roles" do user.spree_roles << dummy_role expect { - put :update, params: { id: user.id, user: { first_name: "Bob" } } + put :update, params: { id: user.id, user: { name: "Bob" } } }.not_to change { user.reload.spree_roles.to_a } end end diff --git a/backend/spec/features/admin/orders/customer_details_spec.rb b/backend/spec/features/admin/orders/customer_details_spec.rb index 6ed9af845f7..195aad5356a 100644 --- a/backend/spec/features/admin/orders/customer_details_spec.rb +++ b/backend/spec/features/admin/orders/customer_details_spec.rb @@ -14,8 +14,8 @@ let!(:product) { create(:product_in_stock) } # We need a unique name that will appear for the customer dropdown - let!(:ship_address) { create(:address, country: country, state: state, first_name: "Rumpelstiltskin") } - let!(:bill_address) { create(:address, country: country, state: state, first_name: "Rumpelstiltskin") } + let!(:ship_address) { create(:address, country: country, state: state, name: "Rumpelstiltskin") } + let!(:bill_address) { create(:address, country: country, state: state, name: "Rumpelstiltskin") } let!(:user) { create(:user, email: 'foobar@example.com', ship_address: ship_address, bill_address: bill_address) } @@ -37,8 +37,7 @@ # Regression test for https://github.com/spree/spree/issues/3335 and https://github.com/spree/spree/issues/5317 it "associates a user when not using guest checkout" do # 5317 - Address prefills using user's default. - expect(page).to have_field('First Name', with: user.bill_address.firstname) - expect(page).to have_field('Last Name', with: user.bill_address.lastname) + expect(page).to have_field('Name', with: user.bill_address.name) expect(page).to have_field('Street Address', with: user.bill_address.address1) expect(page).to have_field("Street Address (cont'd)", with: user.bill_address.address2) expect(page).to have_field('City', with: user.bill_address.city) @@ -120,7 +119,7 @@ order.update!(ship_address_id: nil) click_link "Customer" click_button "Update" - expect(page).to have_content("Shipping address first name can't be blank") + expect(page).to have_content("Shipping address name can't be blank") end context "for an order in confirm state with a user" do @@ -171,9 +170,8 @@ specify do click_link "Customer" # Need to fill in valid information so it passes validations - fill_in "order_ship_address_attributes_firstname", with: "John 99" - fill_in "order_ship_address_attributes_lastname", with: "Doe" - fill_in "order_ship_address_attributes_lastname", with: "Company" + fill_in "order_ship_address_attributes_name", with: "John 99 Doe" + fill_in "order_ship_address_attributes_company", with: "Company" fill_in "order_ship_address_attributes_address1", with: "100 first lane" fill_in "order_ship_address_attributes_address2", with: "#101" fill_in "order_ship_address_attributes_city", with: "Bethesda" @@ -190,8 +188,7 @@ end def fill_in_address - fill_in "First Name", with: "John 99" - fill_in "Last Name", with: "Doe" + fill_in "Name", with: "John 99 Doe" fill_in "Company", with: "Company" fill_in "Street Address", with: "100 first lane" fill_in "Street Address (cont'd)", with: "#101" diff --git a/backend/spec/features/admin/orders/new_order_spec.rb b/backend/spec/features/admin/orders/new_order_spec.rb index f95af00380b..98d261ebb1b 100644 --- a/backend/spec/features/admin/orders/new_order_spec.rb +++ b/backend/spec/features/admin/orders/new_order_spec.rb @@ -367,8 +367,7 @@ end def fill_in_address - fill_in "First Name", with: "John 99" - fill_in "Last Name", with: "Doe" + fill_in "Name", with: "John 99 Doe" fill_in "Street Address", with: "100 first lane" fill_in "Street Address (cont'd)", with: "#101" fill_in "City", with: "Bethesda" diff --git a/backend/spec/features/admin/orders/payments_spec.rb b/backend/spec/features/admin/orders/payments_spec.rb index 5f897917f42..dd86bb9870c 100644 --- a/backend/spec/features/admin/orders/payments_spec.rb +++ b/backend/spec/features/admin/orders/payments_spec.rb @@ -7,18 +7,6 @@ let(:state) { 'checkout' } - def create_payment(opts = {}) - create( - :payment, - { - order: order, - amount: order.outstanding_balance, - payment_method: create(:credit_card_payment_method), - state: state - }.merge(opts) - ) - end - context "with a pre-existing payment" do let!(:payment) { create_payment } @@ -205,10 +193,7 @@ def create_payment(opts = {}) end it "is able to create a new credit card payment with valid information", js: true do - fill_in_with_force "Card Number", with: "4111 1111 1111 1111" - fill_in "Name", with: "Test User" - fill_in_with_force "Expiration", with: "09 / #{Time.current.year + 1}" - fill_in "Card Code", with: "007" + fill_in_credit_card_form # Regression test for https://github.com/spree/spree/issues/4277 expect(page).to have_css('.ccType[value="visa"]', visible: false) click_button "Continue" @@ -290,10 +275,7 @@ def create_payment(opts = {}) it "is able to create a new payment", js: true do choose payment_method.name - fill_in_with_force "Card Number", with: "4111 1111 1111 1111" - fill_in "Name", with: "Test User" - fill_in_with_force "Expiration", with: "09 / #{Time.current.year + 1}" - fill_in "Card Code", with: "007" + fill_in_credit_card_form click_button "Continue" expect(page).to have_content("Payment has been successfully created!") end @@ -319,12 +301,32 @@ def create_payment(opts = {}) it "displays an error" do choose payment_method.name + fill_in_credit_card_form + click_button "Continue" + expect(page).to have_content I18n.t('spree.insufficient_stock_for_order') + end + end + + private + + def create_payment(opts = {}) + create( + :payment, + { + order: order, + amount: order.outstanding_balance, + payment_method: create(:credit_card_payment_method), + state: state + }.merge(opts) + ) + end + + def fill_in_credit_card_form + within('.js-new-credit-card-form') do fill_in_with_force "Card Number", with: "4111 1111 1111 1111" fill_in "Name", with: "Test User" fill_in_with_force "Expiration", with: "09 / #{Time.current.year + 1}" fill_in "Card Code", with: "007" - click_button "Continue" - expect(page).to have_content I18n.t('spree.insufficient_stock_for_order') end end end diff --git a/core/app/models/concerns/spree/user_methods.rb b/core/app/models/concerns/spree/user_methods.rb index e50e5d90ca3..dc2e01c210c 100644 --- a/core/app/models/concerns/spree/user_methods.rb +++ b/core/app/models/concerns/spree/user_methods.rb @@ -34,8 +34,9 @@ module UserMethods include Spree::RansackableAttributes unless included_modules.include?(Spree::RansackableAttributes) + ransack_alias :name, :addresses_firstname_or_addresses_lastname self.whitelisted_ransackable_associations = %w[addresses spree_roles] - self.whitelisted_ransackable_attributes = %w[id email created_at] + self.whitelisted_ransackable_attributes = %w[name id email created_at] end def wallet diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb index ea041d76bdf..a9a8387735f 100644 --- a/core/app/models/spree/address.rb +++ b/core/app/models/spree/address.rb @@ -27,7 +27,8 @@ class Address < Spree::Base TAXATION_ATTRS = %w(state_id country_id zipcode) DEPRECATED_ATTRS = %w(firstname lastname full_name) - self.whitelisted_ransackable_attributes = %w[firstname lastname] + ransack_alias :name, :firstname_or_lastname + self.whitelisted_ransackable_attributes = %w[name firstname lastname] scope :with_values, ->(attributes) do where(value_attributes(attributes)) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 020e98ba18d..d968e594447 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -42,8 +42,9 @@ class CannotRebuildShipments < StandardError; end go_to_state :confirm end + ransack_alias :bill_address_name, :bill_address_firstname_or_bill_address_lastname self.whitelisted_ransackable_associations = %w[shipments user order_promotions promotions bill_address ship_address line_items] - self.whitelisted_ransackable_attributes = %w[completed_at created_at email number state payment_state shipment_state total store_id] + self.whitelisted_ransackable_attributes = %w[bill_address_name completed_at created_at email number state payment_state shipment_state total store_id] attr_reader :coupon_code attr_accessor :temporary_address diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index d2cf4043c88..9749fd6204d 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -1570,6 +1570,7 @@ en: my_account: My Account my_orders: My Orders name: Name + name_contains: Name Contains name_on_card: Name on card name_or_sku: Name or SKU (enter at least first 4 characters of product name) negative_movement_absent_item: Cannot create negative movement for absent stock