diff --git a/core/app/models/concerns/spree/default_price.rb b/core/app/models/concerns/spree/default_price.rb index a6507fe4606..9669f2f9788 100644 --- a/core/app/models/concerns/spree/default_price.rb +++ b/core/app/models/concerns/spree/default_price.rb @@ -41,7 +41,11 @@ def default_price end def has_default_price? - default_price.present? && !default_price.discarded? + if Spree::Config.soft_deleted_prices + default_price.present? && !default_price.discarded? + else + default_price.present? + end end end end diff --git a/core/app/models/concerns/spree/deprecated_soft_deletable.rb b/core/app/models/concerns/spree/deprecated_soft_deletable.rb new file mode 100644 index 00000000000..977c07a0385 --- /dev/null +++ b/core/app/models/concerns/spree/deprecated_soft_deletable.rb @@ -0,0 +1,61 @@ +# core/app/models/concerns/spree/deprecated_soft_deletable.rb + +module Spree + module DeprecatedSoftDeletable + extend ActiveSupport::Concern + + SOFT_DELETE_DEPRECATION_MSG = ->(method, suggestion) { + "`Spree::Price##{method}` is deprecated and will be removed in a future version of Solidus. #{suggestion}" + }.freeze + + included do + scope :kept, -> { + Spree.deprecator.warn( + SOFT_DELETE_DEPRECATION_MSG.call( + "kept", + "Filter by variant availability via `joins(:variant).merge(Spree::Variant.kept)` instead." + ) + ) + unscope(where: :deleted_at).where(deleted_at: nil) + } + + scope :discarded, -> { + Spree.deprecator.warn( + SOFT_DELETE_DEPRECATION_MSG.call( + "discarded", + "There will be no discarded prices once the deleted_at column is removed." + ) + ) + unscoped.where.not(deleted_at: nil) + } + + scope :with_discarded, -> { + Spree.deprecator.warn( + SOFT_DELETE_DEPRECATION_MSG.call( + "with_discarded", + "After removal, all prices will be returned by default without filtering." + ) + ) + unscoped + } + end + + INSTANCE_METHOD_SUGGESTIONS = { + discard: "Discard the parent variant with `variant.discard` instead.", + discard!: "Discard the parent variant with `variant.discard!` instead.", + undiscard: "Prices will not be individually soft-deleted; this has no equivalent.", + undiscard!: "Prices will not be individually soft-deleted; this has no equivalent.", + discarded?: "Check the parent variant's discarded state with `variant.discarded?` instead.", + kept?: "Check the parent variant's kept state with `variant.kept?` instead." + }.freeze + + INSTANCE_METHOD_SUGGESTIONS.each_key do |m| + define_method(m) do + Spree.deprecator.warn( + SOFT_DELETE_DEPRECATION_MSG.call(m, INSTANCE_METHOD_SUGGESTIONS[m]) + ) + super() + end + end + end +end diff --git a/core/app/models/concerns/spree/soft_deletable.rb b/core/app/models/concerns/spree/soft_deletable.rb index 8a07fff9363..beca3ed42b5 100644 --- a/core/app/models/concerns/spree/soft_deletable.rb +++ b/core/app/models/concerns/spree/soft_deletable.rb @@ -10,7 +10,7 @@ module SoftDeletable include Discard::Model self.discard_column = :deleted_at - default_scope { kept } + default_scope { where(deleted_at: nil) } end end end diff --git a/core/app/models/spree/price.rb b/core/app/models/spree/price.rb index 36a957468db..659e2e76d97 100644 --- a/core/app/models/spree/price.rb +++ b/core/app/models/spree/price.rb @@ -3,6 +3,7 @@ module Spree class Price < Spree::Base include Spree::SoftDeletable + include DeprecatedSoftDeletable MAXIMUM_AMOUNT = BigDecimal('99_999_999.99') diff --git a/core/app/models/spree/product/scopes.rb b/core/app/models/spree/product/scopes.rb index 2ec103ec7d5..72aa938bdc2 100644 --- a/core/app/models/spree/product/scopes.rb +++ b/core/app/models/spree/product/scopes.rb @@ -179,8 +179,13 @@ def self.property_conditions(property) end scope :with_master_price, -> do - joins(:master).where(Spree::Price.where(Spree::Variant.arel_table[:id].eq(Spree::Price.arel_table[:variant_id])).arel.exists) + price_scope = Spree::Price.where(Spree::Variant.arel_table[:id].eq(Spree::Price.arel_table[:variant_id])) + unless Spree::Config.soft_deleted_prices + price_scope = price_scope.unscope(where: :deleted_at) + end + joins(:master).where(price_scope.arel.exists) end + # Can't use add_search_scope for this as it needs a default argument def self.available(available_on = nil) with_master_price diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index 387d6ba14dc..b2156526613 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -59,7 +59,7 @@ class Variant < Spree::Base has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" has_many :prices, - -> { with_discarded }, + -> { unscope(where: :deleted_at) }, class_name: 'Spree::Price', dependent: :destroy, inverse_of: :variant, @@ -131,18 +131,20 @@ def self.suppliable # @param pricing_options A Pricing Options object as defined on the price selector class # @return [ActiveRecord::Relation] def self.with_prices(pricing_options = Spree::Config.default_pricing_options) - where( - Spree::Price. - where(Spree::Variant.arel_table[:id].eq(Spree::Price.arel_table[:variant_id])). - # This next clause should just be `where(pricing_options.search_arguments)`, but ActiveRecord - # generates invalid SQL, so the SQL here is written manually. - where( - "spree_prices.currency = ? AND (spree_prices.country_iso IS NULL OR spree_prices.country_iso = ?)", - pricing_options.search_arguments[:currency], - pricing_options.search_arguments[:country_iso].compact - ). - arel.exists - ) + price_scope = Spree::Price. + where(Spree::Variant.arel_table[:id].eq(Spree::Price.arel_table[:variant_id])). + # This next clause should just be `where(pricing_options.search_arguments)`, but ActiveRecord + # generates invalid SQL, so the SQL here is written manually. + where( + "spree_prices.currency = ? AND (spree_prices.country_iso IS NULL OR spree_prices.country_iso = ?)", + pricing_options.search_arguments[:currency], + pricing_options.search_arguments[:country_iso].compact + ) + + unless Spree::Config.soft_deleted_prices + price_scope = price_scope.unscope(where: :deleted_at) + end + where(price_scope.arel.exists) end # @return [Spree::TaxCategory] the variant's tax category diff --git a/core/app/models/spree/variant/price_selector.rb b/core/app/models/spree/variant/price_selector.rb index b61d1acc5cb..e9e44cc861b 100644 --- a/core/app/models/spree/variant/price_selector.rb +++ b/core/app/models/spree/variant/price_selector.rb @@ -39,13 +39,18 @@ 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| + prices = variant.prices + + # Filter out discarded prices for undiscarded variants only if we allow soft-deleted prices + if Spree::Config.soft_deleted_prices + prices = prices.select { |price| variant.discarded? || price.kept? } + end + + prices.sort_by do |price| [ price.country_iso.nil? ? 0 : 1, price.updated_at || Time.zone.now, - price.id || Float::INFINITY, + price.id || Float::INFINITY ] end.reverse end diff --git a/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt b/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt index 8512588b178..2a242b64e54 100644 --- a/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt +++ b/core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt @@ -24,6 +24,9 @@ Spree.config do |config| # Uncomment to recalculate cart prices when the cart changes # config.recalculate_cart_prices = true + # Whether there are soft-deleted prices are in the system. Should be off for new apps. + config.soft_deleted_prices = false + # Defaults # Permission Sets: diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 4b285f79c50..a514c6a5687 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -257,6 +257,10 @@ class AppConfiguration < Preferences::Configuration # @return [Boolean] Allows shipments to be ready to ship regardless of the order being paid if false (default: +true+) preference :require_payment_to_ship, :boolean, default: true # Allows shipments to be ready to ship regardless of the order being paid if false + # @!attribute [rw] soft_deleted_prices + # @return [Boolean] Whether there are soft-deleted prices in the system + preference :soft_deleted_prices, :boolean, default: true + # @!attribute [rw] return_eligibility_number_of_days # @return [Integer] default: 365 preference :return_eligibility_number_of_days, :integer, default: 365 diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb index d0c9d7e69de..aca5ce386d8 100644 --- a/core/lib/spree/testing_support/dummy_app.rb +++ b/core/lib/spree/testing_support/dummy_app.rb @@ -164,4 +164,6 @@ class Application < ::Rails::Application config.image_attachment_module = 'Spree::Image::PaperclipAttachment' config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment' end + + config.soft_deleted_prices = false end diff --git a/core/lib/tasks/solidus/delete_soft_deleted_prices.rake b/core/lib/tasks/solidus/delete_soft_deleted_prices.rake new file mode 100644 index 00000000000..32a352ddd70 --- /dev/null +++ b/core/lib/tasks/solidus/delete_soft_deleted_prices.rake @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +namespace :solidus do + desc <<~DESC + Permanently deletes Spree::Price records that have been soft-deleted + (deleted_at IS NOT NULL). Run this before upgrading to Solidus 5.0, + which removes the deleted_at column from spree_prices entirely. + DESC + + task delete_soft_deleted_prices: :environment do + scope = Spree::Price.unscoped.where.not(deleted_at: nil) + count = scope.count + + if count.zero? + puts "No soft-deleted prices found. Nothing to do." + else + puts "Found #{count} soft-deleted price(s). Deleting permanently..." + scope.delete_all + puts "Done." + end + end +end diff --git a/core/spec/lib/spree/app_configuration_spec.rb b/core/spec/lib/spree/app_configuration_spec.rb index e699c195e4e..ad19701a3a1 100644 --- a/core/spec/lib/spree/app_configuration_spec.rb +++ b/core/spec/lib/spree/app_configuration_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Spree::AppConfiguration do - let(:prefs) { Spree::Config } + let(:prefs) { described_class.new } around do |example| with_unfrozen_spree_preference_store do @@ -56,6 +56,10 @@ expect(prefs.mergeable_orders_finder_class).to eq Spree::MergeableOrdersFinder end + it "allows soft-deleted prices by default" do + expect(prefs.soft_deleted_prices).to be true + end + context "deprecated preferences" do around do |example| Spree.deprecator.silence do diff --git a/core/spec/lib/tasks/solidus/delete_soft_deleted_prices_spec.rb b/core/spec/lib/tasks/solidus/delete_soft_deleted_prices_spec.rb new file mode 100644 index 00000000000..15a91a16f5a --- /dev/null +++ b/core/spec/lib/tasks/solidus/delete_soft_deleted_prices_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "solidus" do + describe "delete_soft_deleted_prices" do + include_context( + "rake", + task_path: Spree::Core::Engine.root.join("lib/tasks/solidus/delete_soft_deleted_prices.rake"), + task_name: "solidus:delete_soft_deleted_prices" + ) + + it "removes all prices with non-NULL deleted_at column", :silence_deprecations do + price = create(:price) + + price.discard! + + expect { price.reload }.not_to raise_error + + task.invoke + + expect { price.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end +end diff --git a/core/spec/models/spree/concerns/deprecated_soft_deletable_spec.rb b/core/spec/models/spree/concerns/deprecated_soft_deletable_spec.rb new file mode 100644 index 00000000000..ae38084f307 --- /dev/null +++ b/core/spec/models/spree/concerns/deprecated_soft_deletable_spec.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Spree::DeprecatedSoftDeletable do + let(:hard_deletable_migration) do + Class.new(ActiveRecord::Migration[5.1]) do + def change + create_table(:hard_deletable_items) do |t| + t.primary_key :id + t.timestamp :deleted_at + end + end + end + end + + let(:hard_deletable_item_class) do + Class.new(Spree::Base) do + include Spree::SoftDeletable + include Spree::DeprecatedSoftDeletable + + 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 + + shared_examples "a deprecated soft-deletable method" do |method_names| + it "emits a deprecation warning" do + Array(method_names).each do + expect(Spree.deprecator).to receive(:warn).with( + a_string_including(method_name.to_s) + ).at_least(:once) + end + subject + end + end + + describe "class-level scopes" do + describe ".kept" do + subject { hard_deletable_item_class.kept } + + include_examples "a deprecated soft-deletable method", :kept + + it "returns only non-deleted records", :silence_deprecations do + hard_deletable_item_class.create! + expect(subject.count).to eq(1) + hard_deletable_item_class.update(deleted_at: Time.current) + expect(subject.count).to eq(0) + end + end + + describe ".discarded" do + subject { hard_deletable_item_class.discarded } + + include_examples "a deprecated soft-deletable method", :discarded + + it "returns only deleted records" do + hard_deletable_item_class.create!(deleted_at: Time.current) + allow(Spree.deprecator).to receive(:warn) + expect(subject.count).to eq(1) + end + end + + describe ".with_discarded" do + subject { hard_deletable_item_class.with_discarded } + + include_examples "a deprecated soft-deletable method", :with_discarded + + it "returns all records regardless of deleted_at" do + hard_deletable_item_class.create! + hard_deletable_item_class.create!(deleted_at: Time.current) + allow(Spree.deprecator).to receive(:warn) + expect(subject.count).to eq(2) + end + end + end + + describe "instance methods" do + describe "#discard" do + subject { hard_deletable_item.discard } + + include_examples "a deprecated soft-deletable method", :discard + + it "sets deleted_at" do + allow(Spree.deprecator).to receive(:warn) + expect { subject }.to change { hard_deletable_item.deleted_at }.from(nil) + end + end + + describe "#discard!" do + subject { hard_deletable_item.discard! } + + include_examples "a deprecated soft-deletable method", :discard + + it "sets deleted_at" do + allow(Spree.deprecator).to receive(:warn) + expect { subject }.to change { hard_deletable_item.deleted_at }.from(nil) + end + end + + describe "#undiscard" do + subject { hard_deletable_item.undiscard } + + before { hard_deletable_item.update!(deleted_at: Time.current) } + + include_examples "a deprecated soft-deletable method", :undiscard + + it "clears deleted_at" do + allow(Spree.deprecator).to receive(:warn) + expect { subject }.to change { hard_deletable_item.deleted_at }.to(nil) + end + end + + describe "#undiscard!" do + subject { hard_deletable_item.undiscard! } + + before { hard_deletable_item.update!(deleted_at: Time.current) } + + include_examples "a deprecated soft-deletable method", :undiscard, :discarded + + it "clears deleted_at" do + allow(Spree.deprecator).to receive(:warn) + expect { subject }.to change { hard_deletable_item.deleted_at }.to(nil) + end + end + + describe "#discarded?" do + subject { hard_deletable_item.discarded? } + + include_examples "a deprecated soft-deletable method", :discarded? + + it "returns false when deleted_at is nil" do + allow(Spree.deprecator).to receive(:warn) + expect(subject).to be(false) + end + + it "returns true when deleted_at is set" do + hard_deletable_item.deleted_at = Time.current + allow(Spree.deprecator).to receive(:warn) + expect(subject).to be(true) + end + end + + describe "#kept?" do + subject { hard_deletable_item.kept? } + + include_examples "a deprecated soft-deletable method", :kept? + + it "returns true when deleted_at is nil" do + allow(Spree.deprecator).to receive(:warn) + expect(subject).to be(true) + end + + it "returns false when deleted_at is set" do + hard_deletable_item.deleted_at = Time.current + allow(Spree.deprecator).to receive(:warn) + expect(subject).to be(false) + end + end + end +end diff --git a/core/spec/models/spree/product/scopes_spec.rb b/core/spec/models/spree/product/scopes_spec.rb index d485bb61a66..1ad232ba52a 100644 --- a/core/spec/models/spree/product/scopes_spec.rb +++ b/core/spec/models/spree/product/scopes_spec.rb @@ -166,12 +166,26 @@ end end - context "with soft-deleted master price" do - before { product.master.prices.discard_all } + context "with soft-deleted master price", :silence_deprecations do + before do + stub_spree_preferences(soft_deleted_prices: true) + product.master.prices.discard_all + end it "doesn't include the product" do expect(Spree::Product.available).to match_array([]) end + + context "if soft_deleted_prices preference is false" do + before do + stub_spree_preferences(soft_deleted_prices: false) + product.master.prices.discard_all + end + + it "includes the product" do + expect(Spree::Product.available).to match_array([product]) + end + end end context "with multiple prices" do diff --git a/core/spec/models/spree/variant/scopes_spec.rb b/core/spec/models/spree/variant/scopes_spec.rb index e1c9247415d..e6a45f5f41b 100644 --- a/core/spec/models/spree/variant/scopes_spec.rb +++ b/core/spec/models/spree/variant/scopes_spec.rb @@ -8,6 +8,10 @@ let!(:variant_2) { create(:variant, product:) } describe ".with_prices" do + let(:pricing_options) { Spree::Config.pricing_options_class.new } + + subject { Spree::Variant.with_prices(pricing_options) } + context "when searching for the default pricing options" do it "finds all variants" do expect(Spree::Variant.with_prices).to contain_exactly(product.master, variant_1, variant_2) @@ -16,6 +20,7 @@ context "when searching for different pricing options" do let(:pricing_options) { Spree::Config.pricing_options_class.new(currency: "EUR") } + context "when only one variant has price in Euro" do before do variant_1.prices.create(amount: 99.00, currency: "EUR") @@ -23,7 +28,7 @@ context "and we search for variants with only prices in Euro" do it "finds the one variant with a price in Euro" do - expect(Spree::Variant.with_prices(pricing_options)).to contain_exactly(variant_1) + expect(subject).to contain_exactly(variant_1) end end end @@ -33,8 +38,6 @@ let(:france) { create(:country, iso: "FR") } let(:pricing_options) { Spree::Variant::PricingOptions.new(country_iso: "FR", currency: "EUR") } - subject { Spree::Variant.with_prices(pricing_options) } - before do variant_1.prices.create!(currency: "EUR", country: france, amount: 10) variant_1.prices.create!(currency: "EUR", country: nil, amount: 10) @@ -42,6 +45,23 @@ it { is_expected.to eq([variant_1]) } end + + context "if variant's prices are discarded", :silence_deprecations do + before do + variant_1.prices.discard_all + end + + it { is_expected.to include(variant_1) } + end + + context "if variant's prices are discarded and soft deleted prices is false", :silence_deprecations do + before do + stub_spree_preferences(soft_deleted_prices: true) + variant_1.prices.discard_all + end + + it { is_expected.not_to include(variant_1) } + end end specify ".descend_by_popularity" do diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb index e36c6351cf4..0b5064bd609 100644 --- a/core/spec/models/spree/variant_spec.rb +++ b/core/spec/models/spree/variant_spec.rb @@ -350,7 +350,9 @@ 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 and the price are both soft-deleted", :silence_deprecations do + before { stub_spree_preferences(soft_deleted_prices: true) } + 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 } @@ -358,7 +360,9 @@ end end - context "when the variant is not soft-deleted, but its price is" do + context "when the variant is not soft-deleted, but its price is", :silence_deprecations do + before { stub_spree_preferences(soft_deleted_prices: true) } + it "will not use a deleted price as the default price" do variant = create(:variant) variant.prices.each { |price| price.discard } @@ -397,7 +401,9 @@ end end - context 'when default price is discarded' do + context 'when default price is discarded', :silence_deprecations do + before { stub_spree_preferences(soft_deleted_prices: true) } + it 'returns false' do variant = create(:variant, price: 20) @@ -870,7 +876,7 @@ describe 'default_price' do let!(:previous_variant_price) { variant.default_price } - it "should not discard default_price" do + it "should not discard default_price", :silence_deprecations do variant.discard variant.reload expect(previous_variant_price.reload).not_to be_discarded