Thoughts on adding #replace_in_address_book to Spree::User?

[elia]

In a project we recently added User#replace_in_address_book as a surrogate of edit when dealing with immutable objects like Spree::Address. Was wondering if it would make sense to upstream it.

Implementation

Here's the code:

        def replace_in_address_book(old_address_id, address_params)
          raise ArgumentError, "the first param should a Spree::Address id" if old_address_id.blank?

          transaction do
            only_address = addresses.count == 1
            old_address = addresses.find_by(id: old_address_id)
            new_address = save_in_address_book(address_params)

            return new_address unless new_address.valid?
            return new_address if old_address.nil?
            return new_address if new_address.id == old_address.id

            new_user_address = user_addresses.find_by(address: new_address)

            if old_address == ship_address || only_address
              user_addresses.mark_default(new_user_address, address_type: :shipping)
              association(:default_user_ship_address).reset
              association(:ship_address).reset
            end

            if old_address == bill_address || only_address
              user_addresses.mark_default(new_user_address, address_type: :billing)
              association(:default_user_bill_address).reset
              association(:bill_address).reset
            end

            remove_from_address_book(old_address.id)
            
            new_address
          end
        end

along with specs:

  describe '#replace_in_address_book' do
    it 'swaps the old address with the new in the user address book' do
      user = create(:user)
      old_address = create(:address)
      user.user_addresses.create(address: old_address)
      new_address_params = attributes_for(:address)

      new_address = user.replace_in_address_book(old_address.id, new_address_params)

      aggregate_failures do
        expect(new_address).to be_valid
        expect(new_address).to be_persisted
        expect(user.addresses).to eq([new_address])
      end
    end

    it 'keeps the same address if unchanged' do
      user = create(:user)
      old_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      new_address_params = old_address.attributes.except(:id)

      new_address = user.replace_in_address_book(old_address.id, new_address_params)

      aggregate_failures do
        expect(new_address).to be_valid
        expect(new_address).to be_persisted
        expect(user.addresses).to eq([old_address])
      end
    end

    it 'returns an non-saved address with errors for invalid attributes' do
      user = create(:user)
      old_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      new_address_params = attributes_for(:address, zipcode: nil)

      new_address = user.replace_in_address_book(old_address.id, new_address_params)

      aggregate_failures do
        expect(new_address).not_to be_valid
        expect(new_address).not_to be_persisted
        expect(new_address.errors.full_messages).to eq(["Zip Code can't be blank"])
        expect(user.addresses).to eq([old_address])
      end
    end

    it 'sets the new address as the default for shipping if the old was a default' do
      user = create(:user)
      old_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      other_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      user.mark_default_ship_address(old_address)
      user.mark_default_bill_address(other_address)
      new_address_params = attributes_for(:address)

      new_address = user.replace_in_address_book(old_address.id, new_address_params)

      aggregate_failures do
        expect(new_address).to be_valid
        expect(new_address).to be_persisted
        expect(user.addresses).to contain_exactly(other_address, new_address)
        expect(user.ship_address).to eq(new_address)
        expect(user.bill_address).to eq(other_address)
      end
    end

    it 'sets the new address as the default for billing if the old was a default' do
      user = create(:user)
      old_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      other_address = create(:address).tap { |a| user.user_addresses.create(address: a) }
      user.mark_default_ship_address(other_address)
      user.mark_default_bill_address(old_address)
      new_address_params = attributes_for(:address)

      new_address = user.replace_in_address_book(old_address.id, new_address_params)

      aggregate_failures do
        expect(new_address).to be_valid
        expect(new_address).to be_persisted
        expect(user.addresses).to contain_exactly(other_address, new_address)
        expect(user.ship_address).to eq(other_address)
        expect(user.bill_address).to eq(new_address)
      end
    end
  end

[kennyadsl]

I like it and I think every store that uses address book can benefit from this. It's not clear from which page should we use this new method though, can you help me understand better, please?