Skip to content

Commit

Permalink
Enable Address name usage in backend
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
filippoliverani committed Feb 28, 2020
1 parent 99b6229 commit 9b48adc
Show file tree
Hide file tree
Showing 18 changed files with 107 additions and 95 deletions.
Original file line number Diff line number Diff line change
@@ -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 }}
Expand Down
3 changes: 1 addition & 2 deletions backend/app/assets/javascripts/spree/backend/user_picker.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ $.fn.userAutocomplete = function () {
q: {
m: 'or',
email_start: term,
addresses_firstname_start: term,
addresses_lastname_start: term
name_start: term
}
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 + ']"]');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions backend/app/controllers/spree/admin/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 15 additions & 8 deletions backend/app/views/spree/admin/orders/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
16 changes: 15 additions & 1 deletion backend/app/views/spree/admin/search/users.json.jbuilder
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 15 additions & 8 deletions backend/app/views/spree/admin/shared/_address_form.html.erb
Original file line number Diff line number Diff line change
@@ -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" %>">
Expand Down
22 changes: 2 additions & 20 deletions backend/spec/controllers/spree/admin/search_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 10 additions & 11 deletions backend/spec/controllers/spree/admin/users_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
17 changes: 7 additions & 10 deletions backend/spec/features/admin/orders/customer_details_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: '[email protected]', ship_address: ship_address, bill_address: bill_address) }

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand Down
3 changes: 1 addition & 2 deletions backend/spec/features/admin/orders/new_order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
46 changes: 24 additions & 22 deletions backend/spec/features/admin/orders/payments_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
Loading

0 comments on commit 9b48adc

Please sign in to comment.