diff --git a/core/app/models/concerns/spree/user_address_book.rb b/core/app/models/concerns/spree/user_address_book.rb index b1ea00d3e68..8d00818038e 100644 --- a/core/app/models/concerns/spree/user_address_book.rb +++ b/core/app/models/concerns/spree/user_address_book.rb @@ -10,57 +10,87 @@ def find_first_by_address_values(address_attrs) detect { |ua| ua.address == Spree::Address.new(address_attrs) } end - # @note this method enforces only one default address per user - def mark_default(user_address) - # the checks of persisted? allow us to build a User and associate Addresses at once + def mark_default(user_address, address_type: :shipping) + column_for_default = address_type == :shipping ? :default : :default_billing ActiveRecord::Base.transaction do - (self - [user_address]).each do |ua| # update_all would be nice, but it bypasses ActiveRecord callbacks - ua.persisted? ? ua.update!(default: false) : ua.default = false + (self - [user_address]).each do |address| # update_all would be nice, but it bypasses ActiveRecord callbacks + if address.persisted? + address.update!(column_for_default => false) + else + address.write_attribute(column_for_default, false) + end + end + + if user_address.persisted? + user_address.update!(column_for_default => true, archived: false) + else + user_address.write_attribute(column_for_default, true) end - user_address.persisted? ? user_address.update!(default: true, archived: false) : user_address.default = true end end end has_many :addresses, through: :user_addresses - # bill_address is only minimally used now, but we can't get rid of it without a major version release - belongs_to :bill_address, class_name: 'Spree::Address', optional: true + has_one :default_user_bill_address, ->{ default_billing }, class_name: 'Spree::UserAddress', foreign_key: 'user_id' + has_one :bill_address, through: :default_user_bill_address, source: :address - has_one :default_user_address, ->{ default }, class_name: 'Spree::UserAddress', foreign_key: 'user_id' - has_one :default_address, through: :default_user_address, source: :address - alias_method :ship_address, :default_address + has_one :default_user_ship_address, ->{ default_shipping }, class_name: 'Spree::UserAddress', foreign_key: 'user_id' + has_one :ship_address, through: :default_user_ship_address, source: :address end - def bill_address=(address) - # stow a copy in our address book too - address = save_in_address_book(address.attributes) if address - super(address) + def default_address + Spree::Deprecation.warn "#default_address is deprecated. Please start using #ship_address." + ship_address end - def bill_address_attributes=(attributes) - self.bill_address = Spree::Address.immutable_merge(bill_address, attributes) + def default_user_address + Spree::Deprecation.warn "#default_user_address is deprecated. Please start using #default_user_ship_address." + default_user_ship_address end def default_address=(address) - save_in_address_book(address.attributes, true) if address + Spree::Deprecation.warn( + "#default_address= does not take Spree::Config.automatic_default_address into account and is deprecated. " \ + "Please use #ship_address=." + ) + + self.ship_address = address if address end def default_address_attributes=(attributes) # see "Nested Attributes Examples" section of http://apidock.com/rails/ActionView/Helpers/FormHelper/fields_for # this #{fieldname}_attributes= method works with fields_for in the views # even without declaring accepts_nested_attributes_for - self.default_address = Spree::Address.immutable_merge(default_address, attributes) - end + Spree::Deprecation.warn "#default_address_attributes= is deprecated. Please use #ship_address_attributes=." - alias_method :ship_address_attributes=, :default_address_attributes= + self.default_address = Spree::Address.immutable_merge(ship_address, attributes) + end # saves address in address book # sets address to the default if automatic_default_address is set to true # if address is nil, does nothing and returns nil def ship_address=(address) - be_default = Spree::Config.automatic_default_address - save_in_address_book(address.attributes, be_default) if address + if address + save_in_address_book(address.attributes, + Spree::Config.automatic_default_address) + end + end + + def ship_address_attributes=(attributes) + self.ship_address = Spree::Address.immutable_merge(ship_address, attributes) + end + + def bill_address=(address) + if address + save_in_address_book(address.attributes, + Spree::Config.automatic_default_address, + :billing) + end + end + + def bill_address_attributes=(attributes) + self.bill_address = Spree::Address.immutable_merge(bill_address, attributes) end # saves order.ship_address and order.bill_address in address book @@ -73,15 +103,16 @@ def persist_order_address(order) order.ship_address.attributes, Spree::Config.automatic_default_address ) - self.ship_address_id = address.id if address && address.persisted? + self.ship_address_id = address.id if address&.persisted? end if order.bill_address address = save_in_address_book( order.bill_address.attributes, - order.ship_address.nil? && Spree::Config.automatic_default_address + Spree::Config.automatic_default_address, + :billing ) - self.bill_address_id = address.id if address && address.persisted? + self.bill_address_id = address.id if address&.persisted? end save! # In case the ship_address_id or bill_address_id was set @@ -92,8 +123,9 @@ def persist_order_address(order) # treated as value equality to de-dup among existing Addresses # @param default set whether or not this address will show up from # #default_address or not - def save_in_address_book(address_attributes, default = false) - return nil unless address_attributes.present? + def save_in_address_book(address_attributes, default = false, address_type = :shipping) + return nil if address_attributes.blank? + address_attributes = address_attributes.to_h.with_indifferent_access new_address = Spree::Address.factory(address_attributes) @@ -106,7 +138,7 @@ def save_in_address_book(address_attributes, default = false) end user_address = prepare_user_address(new_address) - user_addresses.mark_default(user_address) if default || first_one + user_addresses.mark_default(user_address, address_type: address_type) if default || first_one if persisted? user_address.save! @@ -116,17 +148,32 @@ def save_in_address_book(address_attributes, default = false) # user_addresses need to be reset to get the new ordering based on any changes # {default_,}user_address needs to be reset as its result is likely to have changed. user_addresses.reset - association(:default_user_address).reset - association(:default_address).reset + association(:default_user_ship_address).reset + association(:ship_address).reset + association(:default_user_bill_address).reset + association(:bill_address).reset end user_address.address end def mark_default_address(address) + Spree::Deprecation.warn( + "#mark_default_address is deprecated and it sets the ship_address only. " \ + "Please use #mark_default_ship_address." + ) + + mark_default_ship_address(address) + end + + def mark_default_ship_address(address) user_addresses.mark_default(user_addresses.find_by(address: address)) end + def mark_default_bill_address(address) + user_addresses.mark_default(user_addresses.find_by(address: address), :billing) + end + def remove_from_address_book(address_id) user_address = user_addresses.find_by(address_id: address_id) if user_address diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 4a0f28ed48c..57454b32502 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -744,8 +744,8 @@ def ship_address_attributes=(attributes) # @note This doesn't persist the change bill_address or ship_address def assign_default_user_addresses if user - bill_address = (user.bill_address || user.default_address) - ship_address = (user.ship_address || user.default_address) + bill_address = user.bill_address + ship_address = user.ship_address # this is one of 2 places still using User#bill_address self.bill_address ||= bill_address if bill_address.try!(:valid?) # Skip setting ship address if order doesn't have a delivery checkout step diff --git a/core/app/models/spree/user_address.rb b/core/app/models/spree/user_address.rb index 3e60b46fd82..ed93ebf204a 100644 --- a/core/app/models/spree/user_address.rb +++ b/core/app/models/spree/user_address.rb @@ -6,7 +6,7 @@ class UserAddress < Spree::Base belongs_to :address, class_name: "Spree::Address", optional: true validates_uniqueness_of :address_id, scope: :user_id - validates_uniqueness_of :user_id, conditions: -> { active.default }, message: :default_address_exists, if: :default? + validates_uniqueness_of :user_id, conditions: -> { active.default_shipping }, message: :default_address_exists, if: :default? scope :with_address_values, ->(address_attributes) do joins(:address).merge( @@ -15,9 +15,15 @@ class UserAddress < Spree::Base end scope :all_historical, -> { unscope(where: :archived) } - scope :default, -> { where(default: true) } + scope :default_shipping, -> { where(default: true) } + scope :default_billing, -> { where(default_billing: true) } scope :active, -> { where(archived: false) } + scope :default, -> do + Spree::Deprecation.warn "This scope is deprecated. Please start using ::default_shipping." + default_shipping + end + default_scope -> { order([default: :desc, updated_at: :desc]) } end end diff --git a/core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb b/core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb new file mode 100644 index 00000000000..e8435fbc598 --- /dev/null +++ b/core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true +class AddDefaultBillngFlagToUserAddresses < ActiveRecord::Migration[5.2] + def change + add_column :spree_user_addresses, :default_billing, :boolean, default: false + end +end 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..2907a2592c2 100644 --- a/core/spec/models/spree/concerns/user_address_book_spec.rb +++ b/core/spec/models/spree/concerns/user_address_book_spec.rb @@ -11,56 +11,98 @@ module Spree let!(:user) { create(:user) } describe "#save_in_address_book" do - context "saving a default address" do + context "saving a default shipping address" do let(:user_address) { user.user_addresses.find_first_by_address_values(address.attributes) } - subject { user.save_in_address_book(address.attributes, true) } + subject do + -> { user.save_in_address_book(address.attributes, true) } + end context "the address is a new record" do let(:address) { build(:address) } it "creates a new Address" do - expect { subject }.to change { Spree::Address.count }.by(1) + is_expected.to change { Spree::Address.count }.by(1) end it "creates a UserAddress" do - expect { subject }.to change { Spree::UserAddress.count }.by(1) + is_expected.to change { Spree::UserAddress.count }.by(1) end it "sets the UserAddress default flag to true" do - subject + subject.call expect(user_address.default).to eq true + expect(user_address.default_billing).to be_falsey end - it "adds the address to the user's the associated addresses" do - expect { subject }.to change { user.reload.addresses.count }.by(1) + context "saving a billing address" do + subject { user.save_in_address_book(address.attributes, true, :billing) } + + it "sets the UserAddress default_billing flag to true" do + subject + expect(user_address.default).to be_falsey + expect(user_address.default_billing).to eq true + end + end + + it "adds the address to the user's associated addresses" do + is_expected.to change { user.reload.addresses.count }.by(1) end end - context "user already has a default address" do + context "user already has default addresses" do let(:address) { create(:address) } let(:original_default_address) { create(:ship_address) } + let(:original_default_bill_address) { create(:bill_address) } let(:original_user_address) { user.user_addresses.find_first_by_address_values(original_default_address.attributes) } + let(:original_user_bill_address) { user.user_addresses.find_first_by_address_values(original_default_bill_address.attributes) } before do user.user_addresses.create(address: original_default_address, default: true) + user.user_addresses.create(address: original_default_bill_address, default_billing: true) end - it "makes all the other associated addresses not be the default" do - expect { subject }.to change { original_user_address.reload.default }.from(true).to(false) + context "saving a shipping address" do + context "makes all the other associated shipping addresses not be the default and ignores the billing ones" do + it { is_expected.to change { original_user_address.reload.default }.from(true).to(false) } + it { is_expected.not_to change { original_user_bill_address.reload.default_billing } } + end + + context "an odd flip-flop corner case discovered running backfill rake task" do + before do + user.save_in_address_book(original_default_address.attributes, true) + user.save_in_address_book(address.attributes, true) + end + + it "handles setting 2 addresses as default without a reload of user" do + user.save_in_address_book(original_default_address.attributes, true) + user.save_in_address_book(address.attributes, true) + expect(user.addresses.count).to eq 3 + expect(user.ship_address.address1).to eq address.address1 + end + end end - context "an odd flip-flop corner case discovered running backfill rake task" do - before do - user.save_in_address_book(original_default_address.attributes, true) - user.save_in_address_book(address.attributes, true) + context "saving a billing address" do + subject { -> { user.save_in_address_book(address.attributes, true, :billing) } } + + context "makes all the other associated billing addresses not be the default and ignores the shipping ones" do + it { is_expected.not_to change { original_user_address.reload.default } } + it { is_expected.to change { original_user_bill_address.reload.default_billing }.from(true).to(false) } end - it "handles setting 2 addresses as default without a reload of user" do - user.save_in_address_book(original_default_address.attributes, true) - user.save_in_address_book(address.attributes, true) - expect(user.addresses.count).to eq 2 - expect(user.default_address.address1).to eq address.address1 + context "an odd flip-flop corner case discovered running backfill rake task" do + before do + user.save_in_address_book(original_default_bill_address.attributes, true, :billing) + user.save_in_address_book(address.attributes, true, :billing) + end + + it "handles setting 2 addresses as default without a reload of user" do + user.save_in_address_book(original_default_address.attributes, true, :billing) + user.save_in_address_book(address.attributes, true, :billing) + expect(user.addresses.count).to eq 3 + expect(user.bill_address.address1).to eq address.address1 + end end end end @@ -72,8 +114,15 @@ module Spree user.user_addresses.create(address: address, default: false) end - it "properly sets the default flag" do - expect(subject).to eq user.default_address + context "properly sets the default flag" do + context "shipping address" do + it { expect(subject.call).to eq user.ship_address } + end + + context "billing address" do + subject { user.save_in_address_book(address.attributes, true, :billing) } + it { is_expected.to eq user.bill_address } + end end context "and changing another address field at the same time" do @@ -93,8 +142,15 @@ module Spree expect(subject.id).to_not eq address.id end - it "is the new default" do - expect(subject).to eq user.default_address + context "is the new default" do + context "shipping address" do + it { is_expected.to eq user.ship_address } + end + + context "billing address" do + subject { user.save_in_address_book(address.attributes, true, :billing) } + it { is_expected.to eq user.bill_address } + end end end end @@ -136,16 +192,31 @@ module Spree context "it is not the first address" do before { user.user_addresses.create!(address: create(:address)) } - it "sets the UserAddress default flag to false" do + + it "sets the UserAddress default flags to false" do expect { subject }.to change { Spree::UserAddress.count }.by(1) expect(user_address.default).to eq false + expect(user_address.default_billing).to eq false end end context "it is the first address" do - it "sets the UserAddress default flag to true" do - subject - expect(user_address.default).to eq true + context "shipping address" do + it "sets the UserAddress default flag to true" do + subject + expect(user_address.default).to eq true + expect(user_address.default_billing).to be_falsey + end + end + + context "billing address" do + subject { user.save_in_address_book(address.attributes, false, :billing) } + + it "sets the UserAddress default flag to true" do + subject + expect(user_address.default).to be_falsey + expect(user_address.default_billing).to eq true + end end end @@ -167,9 +238,20 @@ module Spree expect(subject).to eq address end - it "sets it as default" do - subject - expect(user.default_address).to eq address + context "when called with default address_type" do + it "sets the passed address as default shipping address" do + subject + expect(user.ship_address).to eq address + end + end + + context "when called with address_type = :billing" do + subject { user.save_in_address_book(address.attributes, true, :billing) } + + it "sets the passed address as default billing address" do + subject + expect(user.bill_address).to eq address + end end context "via an edit to another address" do @@ -289,52 +371,50 @@ module Spree end context "#persist_order_address" do - it 'will save the bill/ship_address reference if it can' do - order = create :order - user.persist_order_address(order) - - expect( user.bill_address_id ).to eq order.bill_address_id - expect( user.ship_address_id ).to eq order.ship_address_id - expect( user.bill_address_id ).not_to eq user.ship_address_id - end - context "when automatic_default_address preference is at a default of true" do - before do - stub_spree_preferences(automatic_default_address: true) - expect(user).to receive(:save_in_address_book).with(kind_of(Hash), true) - expect(user).to receive(:save_in_address_book).with(kind_of(Hash), false) - end + let(:order) { build :order } - it "does set the default: true flag" do - order = build(:order) + it 'will save both bill/ship_address references' do user.persist_order_address(order) + + expect( user.bill_address_id ).to eq order.bill_address_id + expect( user.ship_address_id ).to eq order.ship_address_id + expect( user.bill_address_id ).not_to eq user.ship_address_id + + expect( user.bill_address).to eq order.bill_address + expect( user.ship_address).to eq order.ship_address end end context "when automatic_default_address preference is false" do + let(:order) { build :order } + before do stub_spree_preferences(automatic_default_address: false) - expect(user).to receive(:save_in_address_book).with(kind_of(Hash), false).twice - # and not the optional 2nd argument end - it "does not set the default: true flag" do - order = build(:order) + it "will save only the default ship address on user as it is the first address at all" do user.persist_order_address(order) + + expect( user.bill_address_id ).to eq order.bill_address_id + expect( user.ship_address_id ).to eq order.ship_address_id + + expect( user.bill_address).to be_nil + expect( user.ship_address).to eq order.ship_address end end - context "when address is nil" do + context "when either ship_address or bill_address is nil" do context "when automatic_default_address preference is at a default of true" do before do stub_spree_preferences(automatic_default_address: true) - expect(user).to receive(:save_in_address_book).with(kind_of(Hash), true).once end it "does not call save_in_address_book on ship address" do order = build(:order) order.ship_address = nil + expect(user).to receive(:save_in_address_book).with(kind_of(Hash), true, :billing).once user.persist_order_address(order) end @@ -342,29 +422,31 @@ module Spree order = build(:order) order.bill_address = nil + expect(user).to receive(:save_in_address_book).with(kind_of(Hash), true).once user.persist_order_address(order) end end - end - context "when automatic_default_address preference is false" do - before do - stub_spree_preferences(automatic_default_address: false) - expect(user).to receive(:save_in_address_book).with(kind_of(Hash), false).once - end + context "when automatic_default_address preference is false" do + let(:order) { build(:order) } - it "does not call save_in_address_book on ship address" do - order = build(:order) - order.ship_address = nil + before do + stub_spree_preferences(automatic_default_address: false) + end - user.persist_order_address(order) - end + it "does not call save_in_address_book on ship address" do + order.ship_address = nil - it "does not call save_in_address_book on bill address" do - order = build(:order) - order.bill_address = nil + expect(user).to receive(:save_in_address_book).with(kind_of(Hash), false, :billing).once + user.persist_order_address(order) + end - user.persist_order_address(order) + it "does not call save_in_address_book on bill address" do + order.bill_address = nil + + expect(user).to receive(:save_in_address_book).with(kind_of(Hash), false).once + user.persist_order_address(order) + end end end end @@ -376,18 +458,14 @@ module Spree it "stores the ship_address" do expect(subject.ship_address).to eq ship_address end - - it "is also available as default_address" do - expect(subject.default_address).to eq ship_address - end end - describe "ship_address=" do + describe "#ship_address=" do let!(:user) { create(:user) } let!(:address) { create(:address) } # https://github.com/solidusio/solidus/issues/1241 - it "resets the association and persists" do + it "resets the association and persists the ship_address" do # Load (which will cache) the has_one association expect(user.ship_address).to be_nil @@ -398,5 +476,145 @@ module Spree expect(user.ship_address).to eq(address) end end + + describe "#bill_address=" do + let!(:user) { create(:user) } + let!(:address) { create(:address) } + + # https://github.com/solidusio/solidus/issues/1241 + it "resets the association and persists the bill_address" do + # Load (which will cache) the has_one association + expect(user.bill_address).to be_nil + + user.update!(bill_address: address) + expect(user.bill_address).to eq(address) + + user.reload + expect(user.bill_address).to eq(address) + end + end + + describe "#default_address" do + let(:deprecation_message) do + "#default_address is deprecated. Please start using #ship_address." + end + + it "calls #ship_address and warns caller of deprecation" do + expect(user).to receive(:ship_address) + + expect(Spree::Deprecation).to receive(:warn).with deprecation_message + + user.default_address + end + end + + describe "#default_user_address" do + let(:deprecation_message) do + "#default_user_address is deprecated. Please start using #default_user_ship_address." + end + + it "calls #ship_address and warns caller of deprecation" do + expect(user).to receive(:default_user_ship_address) + + expect(Spree::Deprecation).to receive(:warn).with deprecation_message + + user.default_user_address + end + end + + describe "#default_address=" do + let(:address) { build :address } + + let(:deprecation_message) do + "#default_address= does not take Spree::Config.automatic_default_address into account and is deprecated. " \ + "Please use #ship_address=." + end + + it "calls #ship_address= and warns caller of deprecation" do + expect(user).to receive(:ship_address=).with address + expect(Spree::Deprecation).to receive(:warn).with deprecation_message + + user.default_address = address + end + end + + describe "#default_address_attributes=" do + let(:deprecation_message) do + "#default_address_attributes= is deprecated. Please use #ship_address_attributes=." + end + + it "warns caller of deprecation" do + expect(Spree::Deprecation).to receive(:warn).ordered.with deprecation_message + expect(Spree::Deprecation).to receive(:warn).ordered + + user.default_address_attributes = {} + end + end + + describe "#ship_address_attributes=" do + let(:attributes) { {} } + let(:address) { build :address } + + before do + allow(Spree::Address).to receive(:immutable_merge).and_return address + end + + it "updates ship_address with its present attributes merged with the passed ones" do + expect(Spree::Address).to receive(:immutable_merge).with(user.ship_address, attributes) + expect(user).to receive(:ship_address=).with address + + user.ship_address_attributes = attributes + end + end + + describe "#mark_default_address" do + let(:address) { build :address } + let(:deprecation_message) do + "#mark_default_address is deprecated and it sets the ship_address only. " \ + "Please use #mark_default_ship_address." + end + + it "calls #mark_default_ship_address and warns caller of deprecation" do + expect(user).to receive(:mark_default_ship_address) + + expect(Spree::Deprecation).to receive(:warn).with deprecation_message + + user.mark_default_address(address) + end + end + + describe "#mark_default_ship_address" do + let(:address) { build :address } + let(:user_address) { double } + let(:user_addresses) { double } + + before do + allow(user).to receive(:user_addresses).and_return user_addresses + allow(user_addresses).to receive(:find_by).and_return user_address + end + + it "calls #mark_default with default address kind" do + expect(user_addresses).to receive(:mark_default).with(user_address) + + user.mark_default_ship_address(address) + end + end + + describe "#mark_default_bill_address" do + let(:address) { build :address } + let(:user_address) { double } + let(:user_addresses) { double } + + before do + allow(user).to receive(:user_addresses).and_return user_addresses + allow(user_addresses).to receive(:find_by).and_return user_address + end + + it "calls #mark_default with billing address kind" do + expect(user_addresses).to receive(:mark_default).with(user_address, :billing) + + user.mark_default_bill_address(address) + end + end end end diff --git a/core/spec/models/spree/order/checkout_spec.rb b/core/spec/models/spree/order/checkout_spec.rb index ceb3b20a663..41d85b96d67 100644 --- a/core/spec/models/spree/order/checkout_spec.rb +++ b/core/spec/models/spree/order/checkout_spec.rb @@ -140,7 +140,7 @@ def assert_state_changed(order, from, to) let(:address_kind) { :ship } before do order.user = FactoryBot.create(:user) - order.user.default_address = default_address + order.user.ship_address = default_address order.next! order.reload end @@ -150,7 +150,7 @@ def assert_state_changed(order, from, to) let(:address_kind) { :bill } before do order.user = FactoryBot.create(:user) - order.user.default_address = default_address + order.user.bill_address = default_address order.next! order.reload end diff --git a/core/spec/models/spree/user_address_spec.rb b/core/spec/models/spree/user_address_spec.rb new file mode 100644 index 00000000000..894f68dc13c --- /dev/null +++ b/core/spec/models/spree/user_address_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::UserAddress, type: :model do + context "::default" do + let(:deprecation_message) do + "This scope is deprecated. Please start using ::default_shipping." + end + + it "calls ::default_shipping and warns caller of deprecation" do + expect(described_class).to receive(:default_shipping) + expect(Spree::Deprecation).to receive(:warn).with deprecation_message + + described_class.default + end + end +end