Uniform bill_address and ship_address behaviour in Spree::UserAddressBook module

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

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

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

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