Hard-Delete prices

mamhoff · mamhoff · commit 108699554f19 · 2025-12-26T19:38:08.000+01:00
We don't need soft delete on a model that belongs to an already
soft-deleted model. It'll disappear anyway.
diff --git a/backend/app/controllers/spree/admin/prices_controller.rb b/backend/app/controllers/spree/admin/prices_controller.rb
@@ -8,7 +8,7 @@ class PricesController < ResourceController
       def index
         params[:q] ||= {}
 
-        @search = @product.prices.kept.accessible_by(current_ability, :index).ransack(params[:q])
+        @search = @product.prices.accessible_by(current_ability, :index).ransack(params[:q])
         @master_prices = @search.result
           .currently_valid
           .for_master
diff --git a/backend/app/views/spree/admin/prices/_master_variant_table.html.erb b/backend/app/views/spree/admin/prices/_master_variant_table.html.erb
@@ -20,17 +20,17 @@
       </tr>
     </thead>
     <% master_prices.each do |price| %>
-      <tr id="<%= spree_dom_id price %>" data-hook="prices_row" class="<%= "deleted" if price.discarded? %>">
+      <tr id="<%= spree_dom_id price %>" data-hook="prices_row">
         <td><%= price.display_country %></td>
         <td><%= price.currency %></td>
         <td><%= price.money.to_html %></td>
         <td class="actions">
           <% if can?(:edit, price) %>
-            <%= link_to_edit(price, no_text: true) unless price.discarded? %>
+            <%= link_to_edit(price, no_text: true) %>
           <% end %>
           <% if can?(:destroy, price) %>
             &nbsp;
-            <%= link_to_delete(price, no_text: true) unless price.discarded? %>
+            <%= link_to_delete(price, no_text: true) %>
           <% end %>
         </td>
       </tr>
diff --git a/backend/app/views/spree/admin/prices/_table.html.erb b/backend/app/views/spree/admin/prices/_table.html.erb
@@ -14,18 +14,18 @@
     </thead>
     <tbody>
       <% variant_prices.each do |price| %>
-        <tr id="<%= spree_dom_id price %>" data-hook="prices_row" class="<%= "deleted" if price.discarded? %>">
+        <tr id="<%= spree_dom_id price %>" data-hook="prices_row">
           <td><%= price.variant.descriptive_name %></td>
           <td><%= price.display_country %></td>
           <td><%= price.currency %></td>
           <td><%= price.money.to_html %></td>
           <td class="actions">
             <% if can?(:edit, price) %>
-              <%= link_to_edit(price, no_text: true) unless price.discarded? %>
+              <%= link_to_edit(price, no_text: true) %>
             <% end %>
             <% if can?(:destroy, price) %>
               &nbsp;
-              <%= link_to_delete(price, no_text: true) unless price.discarded? %>
+              <%= link_to_delete(price, no_text: true) %>
             <% end %>
           </td>
         </tr>
diff --git a/backend/spec/controllers/spree/admin/prices_controller_spec.rb b/backend/spec/controllers/spree/admin/prices_controller_spec.rb
@@ -11,7 +11,7 @@
     context "when only given a product" do
       let(:product) { create(:product) }
 
-      let!(:deleted_master_price) { create(:price, variant: product.master).tap(&:discard!) }
+      let!(:deleted_master_price) { create(:price, variant: product.master).tap(&:destroy!) }
 
       subject { get :index, params: { product_id: product.slug } }
 
@@ -21,7 +21,7 @@
         subject
         expect(assigns(:search)).to be_a(Ransack::Search)
         expect(assigns(:variant_prices)).to be_empty
-        expect(assigns(:master_prices)).to eq(product.prices.kept.for_master)
+        expect(assigns(:master_prices)).to eq(product.prices.for_master)
         expect(assigns(:master_prices)).to_not include(deleted_master_price)
         expect(assigns(:product)).to eq(product)
       end
@@ -31,7 +31,7 @@
       let(:variant) { create(:variant) }
       let(:product) { variant.product }
 
-      let!(:deleted_variant_price) { create(:price, variant:).tap(&:discard!) }
+      let!(:deleted_variant_price) { create(:price, variant:).tap(&:destroy!) }
 
       subject { get :index, params: { product_id: product.slug, variant_id: variant.id } }
 
@@ -40,7 +40,7 @@
       it 'assigns usable instance variables' do
         subject
         expect(assigns(:search)).to be_a(Ransack::Search)
-        expect(assigns(:variant_prices)).to eq(product.prices.kept.for_variant)
+        expect(assigns(:variant_prices)).to eq(product.prices.for_variant)
         expect(assigns(:master_prices)).to eq(product.prices.for_master)
         expect(assigns(:variant_prices)).to include(variant.default_price)
         expect(assigns(:variant_prices)).to_not include(deleted_variant_price)
diff --git a/backend/spec/features/admin/products/edit/products_spec.rb b/backend/spec/features/admin/products/edit/products_spec.rb
@@ -61,9 +61,12 @@
 
       click_link 'Prices'
 
-      within "#spree_price_#{product.master.default_price.id}" do
-        accept_alert do
-          click_icon :trash
+      # The deprecation warning will disappear in Solidus 5
+      Spree.deprecator.silence do
+        within "#spree_price_#{product.master.default_price.id}" do
+          accept_alert do
+            click_icon :trash
+          end
         end
       end
       expect(page).to have_content("Price has been successfully removed")
diff --git a/backend/spec/features/admin/products/pricing_spec.rb b/backend/spec/features/admin/products/pricing_spec.rb
@@ -168,19 +168,27 @@
 
       it "will delete the non-default price" do
         subject
-        within "#spree_price_#{other_price.id}" do
-          accept_alert do
-            click_icon :trash
+
+        # The deprecation warning will disappear in Solidus 5
+        Spree.deprecator.silence do
+          within "#spree_price_#{other_price.id}" do
+            accept_alert do
+              click_icon :trash
+            end
           end
         end
         expect(page).to have_content("Price has been successfully removed")
       end
 
       it "does not break when default price is deleted" do
         subject
-        within "#spree_price_#{variant.default_price.id}" do
-          accept_alert do
-            click_icon :trash
+
+        # The deprecation warning will disappear in Solidus 5
+        Spree.deprecator.silence do
+          within "#spree_price_#{variant.default_price.id}" do
+            accept_alert do
+              click_icon :trash
+            end
           end
         end
         expect(page).to have_content("Price has been successfully removed")
diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb
@@ -41,7 +41,7 @@ def default_price
     end
 
     def has_default_price?
-      default_price.present? && !default_price.discarded?
+      default_price.present?
     end
   end
 end
diff --git a/core/app/models/spree/price.rb b/core/app/models/spree/price.rb
@@ -2,7 +2,7 @@
 
 module Spree
   class Price < Spree::Base
-    include Spree::SoftDeletable
+    include Spree::HardDeletable
 
     MAXIMUM_AMOUNT = BigDecimal('99_999_999.99')
 
diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb
@@ -59,7 +59,6 @@ class Variant < Spree::Base
     has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image"
 
     has_many :prices,
-      -> { with_discarded },
       class_name: 'Spree::Price',
       dependent: :destroy,
       inverse_of: :variant,
diff --git a/core/app/models/spree/variant/price_selector.rb b/core/app/models/spree/variant/price_selector.rb
@@ -39,9 +39,7 @@ def price_for_options(price_options)
       #
       # @return [Array<Spree::Price>]
       def sorted_prices_for(variant)
-        variant.prices.select do |price|
-          variant.discarded? || price.kept?
-        end.sort_by do |price|
+        variant.prices.sort_by do |price|
           [
             price.country_iso.nil? ? 0 : 1,
             price.updated_at || Time.zone.now,
diff --git a/core/db/migrate/20251223220550_remove_soft_deleted_prices.rb b/core/db/migrate/20251223220550_remove_soft_deleted_prices.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class RemoveSoftDeletedPrices < ActiveRecord::Migration[7.0]
+  def up
+    discarded_prices = Spree::Price.where.not(deleted_at: nil)
+    affected_variant_ids = discarded_prices.map(&:variant_id).uniq
+    discarded_prices.delete_all
+    Spree::Variant.where(id: affected_variant_ids).find_each(&:touch)
+    remove_column :spree_prices, :deleted_at
+  end
+
+  def down
+    add_column :spree_prices, :deleted_at, :timestamp, null: true
+  end
+end
diff --git a/core/spec/models/spree/product/scopes_spec.rb b/core/spec/models/spree/product/scopes_spec.rb
@@ -166,14 +166,6 @@
         end
       end
 
-      context "with soft-deleted master price" do
-        before { product.master.prices.discard_all }
-
-        it "doesn't include the product" do
-          expect(Spree::Product.available).to match_array([])
-        end
-      end
-
       context "with multiple prices" do
         let!(:second_price) { create(:price, variant: product.master) }
 
diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb
@@ -350,18 +350,17 @@
       expect(variant.default_price.attributes).to eq(price.attributes)
     end
 
-    context "when the variant and the price are both soft-deleted" do
+    context "when the variant is soft-deleted" do
       it "will use a deleted price as the default price" do
         variant = create(:variant, deleted_at: 1.day.ago)
-        variant.prices.each { |price| price.discard }
-        expect(variant.reload.price).to be_present
+        expect(variant.price).to be_present
       end
     end
 
     context "when the variant is not soft-deleted, but its price is" do
       it "will not use a deleted price as the default price" do
         variant = create(:variant)
-        variant.prices.each { |price| price.discard }
+        variant.prices.each { |price| price.destroy }
         expect(variant.reload.price).not_to be_present
       end
     end
@@ -397,13 +396,13 @@
       end
     end
 
-    context 'when default price is discarded' do
+    context 'when default price is destroyed' do
       it 'returns false' do
         variant = create(:variant, price: 20)
 
-        variant.default_price.discard
+        variant.default_price.destroy
 
-        expect(variant.has_default_price?).to be(false)
+        expect(variant.reload.has_default_price?).to be(false)
       end
     end
   end
@@ -873,7 +872,7 @@
       it "should not discard default_price" do
         variant.discard
         variant.reload
-        expect(previous_variant_price.reload).not_to be_discarded
+        expect(previous_variant_price.reload).to be_present
       end
 
       it "should keep its price if deleted" do

Original file line number	Diff line number	Diff line change
`@@ -39,9 +39,7 @@ def price_for_options(price_options)`
`39`	`39`	`#`
`40`	`40`	`# @return [Array<Spree::Price>]`
`41`	`41`	`def sorted_prices_for(variant)`
`42`		`- variant.prices.select do \|price\|`
`43`		`- variant.discarded? \|\| price.kept?`
`44`		`- end.sort_by do \|price\|`
	`42`	`+ variant.prices.sort_by do \|price\|`
`45`	`43`	`[`
`46`	`44`	`price.country_iso.nil? ? 0 : 1,`
`47`	`45`	`price.updated_at \|\| Time.zone.now,`