From b44ae643d7b9956260d2d59cd97d661c0866e300 Mon Sep 17 00:00:00 2001 From: Filippo Liverani Date: Fri, 10 Apr 2020 15:20:22 +0200 Subject: [PATCH 1/3] Deprecate Address firstname, lastname and full_name Add deprecation warnings and update docs to notify when Spree::Address firstname, lastname and full_name attributes are used instead of name. --- api/openapi/api.oas2.yml | 5 ++++ core/app/models/spree/address.rb | 5 ++++ core/spec/models/spree/address_spec.rb | 27 +++++++++++++++++++ .../source/developers/users/addresses.html.md | 4 +-- 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/api/openapi/api.oas2.yml b/api/openapi/api.oas2.yml index 60ea5f155ac..5b50ecff8fb 100644 --- a/api/openapi/api.oas2.yml +++ b/api/openapi/api.oas2.yml @@ -5078,12 +5078,15 @@ definitions: type: string firstname: type: string + description: '*Deprecated: will be removed in Solidus 3.0, please use `name` property*' full_name: type: string + description: '*Deprecated: will be removed in Solidus 3.0, please use `name` property*' id: type: integer lastname: type: string + description: '*Deprecated: will be removed in Solidus 3.0, please use `name` property*' name: type: string phone: @@ -5620,8 +5623,10 @@ definitions: type: string firstname: type: string + description: '*Deprecated: will be removed in Solidus 3.0, please use `name` property*' lastname: type: string + description: '*Deprecated: will be removed in Solidus 3.0, please use `name` property*' address1: type: string address2: diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb index 899f36ee524..225383bdf61 100644 --- a/core/app/models/spree/address.rb +++ b/core/app/models/spree/address.rb @@ -33,6 +33,11 @@ class Address < Spree::Base where(value_attributes(attributes)) end + Spree::Deprecation.deprecate_methods( + Spree::Address, + LEGACY_NAME_ATTRS.product([:name]).to_h + ) + # @return [Address] an address with default attributes def self.build_default(*args, &block) where(country: Spree::Country.default).build(*args, &block) diff --git a/core/spec/models/spree/address_spec.rb b/core/spec/models/spree/address_spec.rb index 2603800eb6f..9eb66457424 100644 --- a/core/spec/models/spree/address_spec.rb +++ b/core/spec/models/spree/address_spec.rb @@ -6,6 +6,11 @@ subject { Spree::Address } context "aliased attributes" do + before do + allow(Spree::Deprecation).to receive(:warn).and_call_original + allow(Spree::Deprecation).to receive(:warn).with(/firstname|lastname/, any_args) + end + let(:address) { Spree::Address.new firstname: 'Ryan', lastname: 'Bigg' } it " first_name" do @@ -400,4 +405,26 @@ it { is_expected.to be_require_phone } end + + context 'deprecations' do + let(:address) { described_class.new } + + specify 'firstname is deprecated' do + expect(Spree::Deprecation).to receive(:warn).with(/firstname/, any_args) + + address.firstname + end + + specify 'lastname is deprecated' do + expect(Spree::Deprecation).to receive(:warn).with(/lastname/, any_args) + + address.lastname + end + + specify 'full_name is deprecated' do + expect(Spree::Deprecation).to receive(:warn).with(/full_name/, any_args) + + address.full_name + end + end end diff --git a/guides/source/developers/users/addresses.html.md b/guides/source/developers/users/addresses.html.md index 1c42837b3bd..26df865bdbd 100644 --- a/guides/source/developers/users/addresses.html.md +++ b/guides/source/developers/users/addresses.html.md @@ -18,8 +18,8 @@ Spree::User.find(1).addresses `Spree::Address` objects have the following attributes: - `name`: The full name for the person at this address. -- `firstname`: The first name for the person at this address. -- `lastname`: The last name for the person at this address. +- `firstname`: *Deprecated: will be removed in Solidus 3.0, please use `name` attribute* - The first name for the person at this address. +- `lastname`: *Deprecated: will be removed in Solidus 3.0, please use `name` attribute* - The last name for the person at this address. - `address1` and `address2`: The street address (with an optional second line). - `city`: The city where the address is. - `zipcode`: The postal code. From ab8bebc9b851859874cd05616fb97f131e505c1f Mon Sep 17 00:00:00 2001 From: Filippo Liverani Date: Fri, 10 Apr 2020 15:20:22 +0200 Subject: [PATCH 2/3] Remove deprecated Address fields from json representation When Spree::Config.use_combined_first_and_last_name_in_address preference is set to true, no Address representation should contain any reference to deprecated legacy name attributes. --- core/app/models/spree/address.rb | 10 ++++++++++ core/spec/models/spree/address_spec.rb | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb index 225383bdf61..0aa2a66139e 100644 --- a/core/app/models/spree/address.rb +++ b/core/app/models/spree/address.rb @@ -191,6 +191,16 @@ def name=(value) write_attribute(:lastname, name_from_value.last_name) end + def as_json(options = {}) + if Spree::Config.use_combined_first_and_last_name_in_address + super(options.merge(except: LEGACY_NAME_ATTRS)).tap do |hash| + hash['name'] = name + end + else + super + end + end + private def validate_name diff --git a/core/spec/models/spree/address_spec.rb b/core/spec/models/spree/address_spec.rb index 9eb66457424..3ff91ba1b7d 100644 --- a/core/spec/models/spree/address_spec.rb +++ b/core/spec/models/spree/address_spec.rb @@ -379,6 +379,12 @@ expect(address.name).to eq('') end + + it 'is included in json representation' do + address = Spree::Address.new(name: 'Jane Von Doe') + + expect(address.as_json).to include('name' => 'Jane Von Doe') + end end context '#state_text' do @@ -409,6 +415,10 @@ context 'deprecations' do let(:address) { described_class.new } + specify 'json representation does not contain deprecated fields' do + expect(address.as_json).not_to include('firstname', 'lastname') + end + specify 'firstname is deprecated' do expect(Spree::Deprecation).to receive(:warn).with(/firstname/, any_args) From 12bdceb69921b15fc056ddf51309ca06d8fab87f Mon Sep 17 00:00:00 2001 From: Filippo Liverani Date: Fri, 10 Apr 2020 15:20:22 +0200 Subject: [PATCH 3/3] Remove any remaining reference to Address deprecated fields --- .../spree/admin/payments_controller_spec.rb | 4 ++-- .../spree/admin/search_controller_spec.rb | 12 +++++++---- core/app/models/spree/address.rb | 7 +++++-- .../factories/address_factory.rb | 3 +-- core/spec/models/spree/address_spec.rb | 20 +++++++++---------- .../spree/concerns/user_address_book_spec.rb | 10 +++++----- 6 files changed, 31 insertions(+), 25 deletions(-) diff --git a/backend/spec/controllers/spree/admin/payments_controller_spec.rb b/backend/spec/controllers/spree/admin/payments_controller_spec.rb index cd73336c7e2..fabae0d7374 100644 --- a/backend/spec/controllers/spree/admin/payments_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/payments_controller_spec.rb @@ -55,7 +55,7 @@ module Admin let(:address_attributes) do { - 'firstname' => address.firstname, + 'name' => address.name, 'address1' => address.address1, 'city' => address.city, 'country_id' => address.country_id, @@ -68,7 +68,7 @@ module Admin it 'associates the address' do expect(order.payments.count).to eq(1) credit_card = order.payments.last.source - expect(credit_card.address.attributes).to include(address_attributes) + expect(credit_card.address.as_json).to include(address_attributes) end end end diff --git a/backend/spec/controllers/spree/admin/search_controller_spec.rb b/backend/spec/controllers/spree/admin/search_controller_spec.rb index adab87f5e28..0b022b6aa05 100644 --- a/backend/spec/controllers/spree/admin/search_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/search_controller_spec.rb @@ -25,23 +25,27 @@ end context 'when searching by user attributes' do - let(:params) { { q: user_attribute } } + let(:params) { { q: search_query } } + + def starting_letters(string) + string.first(3) + end context 'when searching by email' do it_should_behave_like 'user found by search' do - let(:user_attribute) { user.email } + let(:search_query) { starting_letters(user.email) } end end context 'when searching by ship addresss name' do it_should_behave_like 'user found by search' do - let(:user_attribute) { user.ship_address.name } + let(:search_query) { starting_letters(user.ship_address.name) } end end context 'when searching by bill address name' do it_should_behave_like 'user found by search' do - let(:user_attribute) { user.bill_address.name } + let(:search_query) { starting_letters(user.bill_address.name) } end end end diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb index 0aa2a66139e..aa271685cbb 100644 --- a/core/app/models/spree/address.rb +++ b/core/app/models/spree/address.rb @@ -180,7 +180,10 @@ def country_iso # @return [String] the full name on this address def name - Spree::Address::Name.new(firstname, lastname).value + Spree::Address::Name.new( + read_attribute(:firstname), + read_attribute(:lastname) + ).value end def name=(value) @@ -204,7 +207,7 @@ def as_json(options = {}) private def validate_name - return if firstname.present? + return if name.present? name_attribute = if Spree::Config.use_combined_first_and_last_name_in_address :name diff --git a/core/lib/spree/testing_support/factories/address_factory.rb b/core/lib/spree/testing_support/factories/address_factory.rb index 72568e6f41a..9f0a51407db 100644 --- a/core/lib/spree/testing_support/factories/address_factory.rb +++ b/core/lib/spree/testing_support/factories/address_factory.rb @@ -11,8 +11,7 @@ state_code { 'AL' } end - firstname { 'John' } - lastname { nil } + name { 'John Von Doe' } company { 'Company' } address1 { '10 Lovely Street' } address2 { 'Northwest' } diff --git a/core/spec/models/spree/address_spec.rb b/core/spec/models/spree/address_spec.rb index 3ff91ba1b7d..0588f8dee30 100644 --- a/core/spec/models/spree/address_spec.rb +++ b/core/spec/models/spree/address_spec.rb @@ -159,19 +159,19 @@ end it 'accepts other attributes' do - address = Spree::Address.build_default(first_name: 'Ryan') + address = Spree::Address.build_default(name: 'Ryan') expect(address.country).to eq default_country - expect(address.first_name).to eq 'Ryan' + expect(address.name).to eq 'Ryan' end it 'accepts a block' do address = Spree::Address.build_default do |record| - record.first_name = 'Ryan' + record.name = 'Ryan' end expect(address.country).to eq default_country - expect(address.first_name).to eq 'Ryan' + expect(address.name).to eq 'Ryan' end it 'can override the country' do @@ -213,7 +213,7 @@ end end - let(:new_address_attributes) { attributes_for(:address) } + let(:new_address_attributes) { build(:address).attributes } subject { Spree::Address.immutable_merge(existing_address, new_address_attributes) } context "no existing address supplied" do @@ -227,7 +227,7 @@ context 'and there is a matching address in the database' do let(:new_address_attributes) { Spree::Address.value_attributes(matching_address.attributes) } - let!(:matching_address) { create(:address, firstname: 'Jordan') } + let!(:matching_address) { create(:address, name: 'Jordan') } it "returns the matching address" do expect(subject.attributes).to be_address_equivalent_attributes(new_address_attributes) @@ -256,7 +256,7 @@ context 'and changed address matches an existing address' do let(:new_address_attributes) { Spree::Address.value_attributes(matching_address.attributes) } - let!(:matching_address) { create(:address, firstname: 'Jordan') } + let!(:matching_address) { create(:address, name: 'Jordan') } it 'returns the matching address' do expect(subject.attributes).to be_address_equivalent_attributes(new_address_attributes) @@ -319,10 +319,10 @@ describe '.taxation_attributes' do context 'both taxation and non-taxation attributes are present ' do - let(:address) { Spree::Address.new firstname: 'Michael', lastname: 'Jackson', state_id: 1, country_id: 2, zipcode: '12345' } + let(:address) { Spree::Address.new name: 'Michael Jackson', state_id: 1, country_id: 2, zipcode: '12345' } it 'removes the non-taxation attributes' do - expect(address.taxation_attributes).not_to eq('firstname' => 'Michael', 'lastname' => 'Jackson') + expect(address.taxation_attributes).not_to eq('name' => 'Michael Jackson') end it 'returns only the taxation attributes' do @@ -331,7 +331,7 @@ end context 'taxation attributes are blank' do - let(:address) { Spree::Address.new firstname: 'Michael', lastname: 'Jackson' } + let(:address) { Spree::Address.new name: 'Michael Jackson' } it 'returns a subset of the attributes with the correct keys and nil values' do expect(address.taxation_attributes).to eq('state_id' => nil, 'country_id' => nil, 'zipcode' => nil) diff --git a/core/spec/models/spree/concerns/user_address_book_spec.rb b/core/spec/models/spree/concerns/user_address_book_spec.rb index 07f989d8648..5a1cb231101 100644 --- a/core/spec/models/spree/concerns/user_address_book_spec.rb +++ b/core/spec/models/spree/concerns/user_address_book_spec.rb @@ -77,16 +77,16 @@ module Spree end context "and changing another address field at the same time" do - let(:updated_address_attributes) { address.attributes.tap { |value| value[:first_name] = "Newbie" } } + let(:updated_address_attributes) { address.attributes.tap { |value| value[:city] = "Dallas" } } subject { user.save_in_address_book(updated_address_attributes, true) } - it "changes first name" do - expect(subject.first_name).to eq updated_address_attributes[:first_name] + it "changes city" do + expect(subject.city).to eq updated_address_attributes[:city] end - it "preserves last name" do - expect(subject.last_name).to eq address.last_name + it "preserves name" do + expect(subject.name).to eq address.name end it "is a new immutable address instance" do