diff --git a/backend/app/controllers/spree/admin/prices_controller.rb b/backend/app/controllers/spree/admin/prices_controller.rb index 8bcd204973d..496d404ffa3 100644 --- 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 index 9ec6d3dd993..f1d0f8c027b 100644 --- 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 @@ <% master_prices.each do |price| %> - "> + <%= price.display_country %> <%= price.currency %> <%= price.money.to_html %> <% 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) %>   - <%= link_to_delete(price, no_text: true) unless price.discarded? %> + <%= link_to_delete(price, no_text: true) %> <% end %> diff --git a/backend/app/views/spree/admin/prices/_table.html.erb b/backend/app/views/spree/admin/prices/_table.html.erb index 604810ec848..e0fdb946b59 100644 --- a/backend/app/views/spree/admin/prices/_table.html.erb +++ b/backend/app/views/spree/admin/prices/_table.html.erb @@ -14,18 +14,18 @@ <% variant_prices.each do |price| %> - "> + <%= price.variant.descriptive_name %> <%= price.display_country %> <%= price.currency %> <%= price.money.to_html %> <% 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) %>   - <%= link_to_delete(price, no_text: true) unless price.discarded? %> + <%= link_to_delete(price, no_text: true) %> <% end %> diff --git a/backend/spec/controllers/spree/admin/prices_controller_spec.rb b/backend/spec/controllers/spree/admin/prices_controller_spec.rb index e2e785b24d2..4b7fee501cf 100644 --- 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 index 5af44051524..75de8eef741 100644 --- a/backend/spec/features/admin/products/edit/products_spec.rb +++ b/backend/spec/features/admin/products/edit/products_spec.rb @@ -61,6 +61,8 @@ click_link 'Prices' + # The deprecation warning will disappear in Solidus 5 + expect(Spree.deprecator).to receive(:warn) within "#spree_price_#{product.master.default_price.id}" do accept_alert do click_icon :trash diff --git a/backend/spec/features/admin/products/pricing_spec.rb b/backend/spec/features/admin/products/pricing_spec.rb index ac069b5fdae..5b92bf8aad6 100644 --- a/backend/spec/features/admin/products/pricing_spec.rb +++ b/backend/spec/features/admin/products/pricing_spec.rb @@ -168,6 +168,9 @@ it "will delete the non-default price" do subject + + # The deprecation warning will disappear in Solidus 5 + expect(Spree.deprecator).to receive(:warn) within "#spree_price_#{other_price.id}" do accept_alert do click_icon :trash @@ -178,12 +181,16 @@ it "does not break when default price is deleted" do subject + + # The deprecation warning will disappear in Solidus 5 + expect(Spree.deprecator).to receive(:warn) within "#spree_price_#{variant.default_price.id}" do accept_alert do click_icon :trash end end expect(page).to have_content("Price has been successfully removed") + visit spree.admin_products_path expect(page).to have_selector("#spree_product_#{product.id}") end diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index a6507fe4606..45c939d4abd 100644 --- 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/concerns/spree/hard_deletable.rb b/core/app/models/concerns/spree/hard_deletable.rb new file mode 100644 index 00000000000..b62557f919d --- /dev/null +++ b/core/app/models/concerns/spree/hard_deletable.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Spree + # Implements all of the methods that Discard implements, but without a deleted_at column. + # Deprecates all usages of Discard methods. + module HardDeletable + extend ActiveSupport::Concern + + def discarded? = false + def undiscarded? = true + def kept? = true + def deleted_at = nil + + included do + def self.kept + all + end + + def self.discarded + none + end + + def self.discard_all + destroy_all + end + + def self.with_discarded + all + end + class << self + deprecate :kept, :discarded, :with_discarded, :discard_all, deprecator: Spree.deprecator + end + + alias_method :discard, :destroy + alias_method :discard!, :destroy + + deprecate :discard, :discard!, deprecator: Spree.deprecator + deprecate :discarded?, :undiscarded?, :kept?, :deleted_at, deprecator: Spree.deprecator + end + end +end diff --git a/core/app/models/spree/price.rb b/core/app/models/spree/price.rb index 36a957468db..a72bbd19413 100644 --- 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 index 387d6ba14dc..0d5bb70c765 100644 --- 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 index b61d1acc5cb..9492238d9ce 100644 --- 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] 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 new file mode 100644 index 00000000000..287b50be33e --- /dev/null +++ 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/hard_deletable_spec.rb b/core/spec/models/spree/hard_deletable_spec.rb new file mode 100644 index 00000000000..1e35d42d66e --- /dev/null +++ b/core/spec/models/spree/hard_deletable_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::HardDeletable do + let(:hard_deletable_migration) do + Class.new(ActiveRecord::Migration[5.1]) do + def change + create_table(:hard_deletable_items) + end + end + end + + let(:hard_deletable_item_class) do + Class.new(Spree::Base) do + include Spree::HardDeletable + + def self.name + "HardDeletableItem" + end + end + end + + let(:hard_deletable_item) { hard_deletable_item_class.new } + + around do |example| + hard_deletable_migration.migrate(:up) + example.run + hard_deletable_migration.migrate(:down) + end + + before do + expect(Spree.deprecator).to receive(:warn) + end + + describe ".with_discarded" do + it "is deprecated and returns all" do + expect(hard_deletable_item_class.with_discarded.to_sql).to eq(hard_deletable_item_class.all.to_sql) + end + end + + describe ".kept" do + it "is deprecated and returns all" do + expect(hard_deletable_item_class.kept.to_sql).to eq(hard_deletable_item_class.all.to_sql) + end + end + + describe ".discarded" do + it "is deprecated and returns none" do + expect(hard_deletable_item_class.discarded.to_sql).to eq(hard_deletable_item_class.none.to_sql) + end + end + + describe ".discard_all" do + it "is deprecated and calls #destroy_all" do + expect(hard_deletable_item_class).to receive(:destroy_all) + hard_deletable_item_class.discard_all + end + end + + describe "#deleted_at" do + subject { hard_deletable_item.deleted_at } + it { is_expected.to be nil } + end + + describe "#discarded?" do + subject { hard_deletable_item.discarded? } + it { is_expected.to be false } + end + + describe "#undiscarded?" do + subject { hard_deletable_item.undiscarded? } + it { is_expected.to be true } + end + + describe "#kept?" do + subject { hard_deletable_item.kept? } + it { is_expected.to be true } + end +end diff --git a/core/spec/models/spree/product/scopes_spec.rb b/core/spec/models/spree/product/scopes_spec.rb index d485bb61a66..019a9837ab5 100644 --- 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 index e36c6351cf4..4cc00d015da 100644 --- 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