Merge pull request solidusio#3852 from mamhoff/remove-archived-user-addresses

Remove Spree::UserAddress#archived flag

Author: tvdeyen

api/openapi/solidus-api.oas.yml

@@ -1169,10 +1169,7 @@ paths:
         '422':
           $ref: '#/components/responses/delete-restriction'
       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`.
+      description: Removes an address from a user's address book.
       operationId: remove-address-from-user-address-book
       tags:
         - Address books
@@ -1201,11 +1198,7 @@ paths:
       operationId: update-user-address-book
       tags:
         - 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.
+      description: Updates a user's address book.
       security:
         - api-key: []
       requestBody:

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

@@ -59,7 +59,13 @@ module Spree::Api
             put "/api/users/#{user.id}/address_book",
               params:  { address_book: harry_address_attributes.merge('id' => address.id) },
               headers: { Authorization: 'Bearer galleon' }
-          }.to change { Spree::UserAddress.count }.from(1).to(2)
+          }.to change { Spree::Address.count }.from(1).to(2)
+
+          expect {
+            put "/api/users/#{user.id}/address_book",
+              params:  { address_book: harry_address_attributes.merge('id' => address.id) },
+              headers: { Authorization: 'Bearer galleon' }
+          }.not_to change { Spree::UserAddress.count }.from(1)
 
           expect(response.status).to eq(200)
           expect(JSON.parse(response.body).first).to include(harry_address_attributes)
@@ -119,7 +125,7 @@ module Spree::Api
           end
         end
 
-        it 'archives my address' do
+        it 'removes the address from my address book' do
           address = create(:address)
           user = create(:user, spree_api_key: 'galleon')
           user.save_in_address_book(address.attributes, false)
@@ -168,13 +174,19 @@ module Spree::Api
             put "/api/users/#{other_user.id}/address_book",
             params:  { address_book: updated_harry_address.merge('id' => address.id) },
             headers: { Authorization: 'Bearer galleon' }
-          }.to change { Spree::UserAddress.count }.from(1).to(2)
+          }.to change { Spree::Address.count }.from(1).to(2)
+
+          expect {
+            put "/api/users/#{other_user.id}/address_book",
+            params:  { address_book: updated_harry_address.merge('id' => address.id) },
+            headers: { Authorization: 'Bearer galleon' }
+          }.not_to change { Spree::UserAddress.count }.from(1)
 
           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 "removes the address from the other user's address book" 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
@@ -151,7 +151,7 @@ def remove_from_address_book(address_id)
       user_address = user_addresses.find_by(address_id:)
       if user_address
         remove_user_address_reference(address_id)
-        user_address.update(archived: true, default: false)
+        user_address.destroy!
       else
         false
       end
@@ -160,10 +160,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,18 +6,24 @@ 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 :all_historical, -> {
+      Spree::Deprecation.warn("This scope does not do anything and will be removed from Solidus 5.")
+      all
+    }
     scope :default_shipping, -> { where(default: true) }
     scope :default_billing, -> { where(default_billing: true) }
-    scope :active, -> { where(archived: false) }
+    scope :active, -> {
+      Spree::Deprecation.warn("This scope does not do anything and will be removed from Solidus 5.")
+      all
+    }
 
     default_scope -> { order([default: :desc, updated_at: :desc]) }
   end

core/db/migrate/20220419170826_remove_archived_user_addresses.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class RemoveArchivedUserAddresses < ActiveRecord::Migration[5.2]
+  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, :archived, :boolean, default: false
+  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