Remove Spree::UserAddress#archived flag

This flag is not very useful and gives rise to bugs.

It is not very useful, as we only need it to "resurrect" addresses that
the user inputs newly, but that match an address that has been removed
from the address book in the past for some reason. Why not remove the
join table entry directly? Much simpler, and less code to worry about.

The bug we found that exists only because of the existence of this
column is the following: If an address is added to the address book that
matches an existing user address that is archived, there is a chance of
it throwing a uniqueness validation error, because the uniqueness check
on user/address on the Spree::UserAddress model does not take into
account the "archived" flag.

The third reason why I think this should go is for data protection
reasons: If I call "delete" on a record, I want it to be gone, not
half-gone. And in the case of a join table representing which addresses
a user wants to be presented with again, I very much want it gone.

Not archived.

Note also: Removing this complexity removes explanatory comments in
otherwise straight-forward API endpoints.

api/openapi/solidus-api.oas.yml

@@ -1069,10 +1069,6 @@ paths:
       summary: Remove address from user address book
       description: >-
         Removes an address from a user's address book.
-
-
-        **Note:** Rather than delete a `Spree::UserAddress` record this action
-        set its `archived` attribute to `true`.
       operationId: remove-address-from-user-address-book
       tags:
         - Address books
@@ -1103,15 +1099,6 @@ paths:
         - Address books
       description: >-
         Updates a user's address book.
-
-
-        **Note:** if the passed `id` matches an existing `address` a new
-        `Spree::Address` record will be created and the matched `address`
-        `archived` on `Spree::UserAddress`. For a similar logic, if the passed
-        `id` matches an existing `address` which is in `archived` state, the
-        `Spree::UserAddress#archived` record will be restored to `false`.
-
-        See `user_address_book.rb` for further information.
       security:
         - api-key: []
       requestBody:

api/spec/requests/spree/api/address_books_spec.rb

@@ -59,7 +59,7 @@ module Spree
             put "/api/users/#{user.id}/address_book",
               params:  { address_book: harry_address_attributes.merge('id' => address.id) },
               headers: { Authorization: 'Bearer galleon' }
-          }.to change { UserAddress.count }.from(1).to(2)
+          }.not_to change { UserAddress.count }
 
           expect(response.status).to eq(200)
           expect(JSON.parse(response.body).first).to include(harry_address_attributes)
@@ -100,7 +100,7 @@ module Spree
           end
         end
 
-        it 'archives my address' do
+        it 'deletes my address' do
           address = create(:address)
           user = create(:user, spree_api_key: 'galleon')
           user.save_in_address_book(address.attributes, false)
@@ -149,13 +149,13 @@ module Spree
             put "/api/users/#{other_user.id}/address_book",
             params:  { address_book: updated_harry_address.merge('id' => address.id) },
             headers: { Authorization: 'Bearer galleon' }
-          }.to change { UserAddress.count }.from(1).to(2)
+          }.not_to change { UserAddress.count }
 
           expect(response.status).to eq(200)
           expect(JSON.parse(response.body).first).to include(updated_harry_address)
         end
 
-        it "archives another user's address" do
+        it "deletes another user's address" do
           address = create(:address)
           other_user = create(:user)
           other_user.save_in_address_book(address.attributes, false)

core/app/models/concerns/spree/user_address_book.rb

@@ -5,7 +5,7 @@ module UserAddressBook
     extend ActiveSupport::Concern
 
     included do
-      has_many :user_addresses, -> { active }, foreign_key: "user_id", class_name: "Spree::UserAddress" do
+      has_many :user_addresses, foreign_key: "user_id", class_name: "Spree::UserAddress" do
         def find_first_by_address_values(address_attrs)
           detect { |ua| ua.address == Spree::Address.new(address_attrs) }
         end
@@ -22,7 +22,7 @@ def mark_default(user_address, address_type: :shipping)
             end
 
             if user_address.persisted?
-              user_address.update!(column_for_default => true, archived: false)
+              user_address.update!(column_for_default => true)
             else
               user_address.write_attribute(column_for_default, true)
             end
@@ -178,7 +178,7 @@ def remove_from_address_book(address_id)
       user_address = user_addresses.find_by(address_id: address_id)
       if user_address
         remove_user_address_reference(address_id)
-        user_address.update(archived: true, default: false)
+        user_address.destroy!
       else
         false
       end
@@ -187,10 +187,9 @@ def remove_from_address_book(address_id)
     private
 
     def prepare_user_address(new_address)
-      user_address = user_addresses.all_historical.find_first_by_address_values(new_address.attributes)
+      user_address = user_addresses.find_first_by_address_values(new_address.attributes)
       user_address ||= user_addresses.build
       user_address.address = new_address
-      user_address.archived = false
       user_address
     end
 

core/app/models/spree/user_address.rb

@@ -6,19 +6,25 @@ 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_shipping }, message: :default_address_exists, if: :default?
+    validates_uniqueness_of :user_id, conditions: -> { default_shipping }, message: :default_address_exists, if: :default?
 
     scope :with_address_values, ->(address_attributes) do
       joins(:address).merge(
         Spree::Address.with_values(address_attributes)
       )
     end
 
-    scope :all_historical, -> { unscope(where: :archived) }
     scope :default_shipping, -> { where(default: true) }
     scope :default_billing, -> { where(default_billing: true) }
-    scope :active, -> { where(archived: false) }
 
+    scope :all_historical, -> {
+      Spree::Deprecation.warn "This scope is deprecated. Please start using ::all."
+      all
+    }
+    scope :active, -> {
+      Spree::Deprecation.warn "This scope is deprecated. Please start using ::all."
+      all
+    }
     scope :default, -> do
       Spree::Deprecation.warn "This scope is deprecated. Please start using ::default_shipping."
       default_shipping

core/db/migrate/20201201170826_remove_archived_user_addresses.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class RemoveArchivedUserAddresses < ActiveRecord::Migration[6.0]
+  def up
+    Spree::UserAddress.where(archived: true).delete_all
+    remove_column :spree_user_addresses, :archived, :boolean, default: false
+  end
+
+  def down
+    add_column :spree_user_addresses
+  end
+end

core/spec/models/spree/concerns/user_address_book_spec.rb

@@ -225,78 +225,6 @@ module Spree
           end
         end
       end
-
-      context "resurrecting a previously saved (but now archived) address" do
-        let(:address) { create(:address) }
-        before do
-          user.save_in_address_book(address.attributes, true)
-          user.remove_from_address_book(address.id)
-        end
-        subject { user.save_in_address_book(address.attributes, true) }
-
-        it "returns the address" do
-          expect(subject).to eq address
-        end
-
-        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
-          let(:address2) { create(:address, name: "Different") }
-          let(:edited_attributes) do
-            # conceptually edit address2 to match the values of address
-            edited_attributes = address.attributes
-            edited_attributes[:id] = address2.id
-            edited_attributes
-          end
-
-          before { user.save_in_address_book(address2.attributes, true) }
-
-          subject { user.save_in_address_book(edited_attributes) }
-
-          it "returns the address" do
-            expect(subject).to eq address
-          end
-
-          it "archives address2" do
-            subject
-            user_address_two = user.user_addresses.all_historical.find_by(address_id: address2.id)
-            expect(user_address_two.archived).to be true
-          end
-
-          context "via a new address that matches an archived one" do
-            let(:added_attributes) do
-              added_attributes = address.attributes
-              added_attributes.delete(:id)
-              added_attributes
-            end
-
-            subject { user.save_in_address_book(added_attributes) }
-
-            it "returns the address" do
-              expect(subject).to eq address
-            end
-
-            it "no longer has archived user_addresses" do
-              subject
-              expect(user.user_addresses.all_historical).to eq user.user_addresses
-            end
-          end
-        end
-      end
     end
 
     context "#remove_from_address_book" do
@@ -316,10 +244,9 @@ module Spree
         expect(user.user_addresses.find_first_by_address_values(address1.attributes)).to be_nil
       end
 
-      it "leaves user_address record in an archived state" do
+      it "deletes user_address record" do
         subject
-        archived_user_address = user.user_addresses.all_historical.find_first_by_address_values(address1.attributes)
-        expect(archived_user_address).to be_archived
+        expect(user.user_addresses.map(&:address)).to eq([address2])
       end
 
       it "returns false if the addresses is not there" do