From c0df1f3386fa4275eea7c6b3823f14770d99e339 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 12 Dec 2019 16:35:49 +0100 Subject: [PATCH 1/4] Conditionally raise/validate on currency mismatch The current behavior is to raise an exception when the currency of the price that we are trying to set on a line item mismatches with the order currency. However, this is creating issues because it's raising also when the currency is valid. This happens because we don't have control of the order used by Rails to set attributes on the LineItem instance and order is still nil/uset when money_price= is executed. This commit allows to conditionally avoid the raise, switching the behavior to a standard Rails validation. This is better because at validation time, we have the instance with all the attributes set and we are sure to be able to perform the right check. The validation behavior will become the default with future commits and the raise bahavior will be deprecated. --- core/app/models/spree/line_item.rb | 12 ++++++++++-- core/lib/spree/app_configuration.rb | 7 +++++++ core/spec/models/spree/line_item_spec.rb | 23 ++++++++++++++++++++--- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index 380607afb6a..d3608b3403a 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -33,6 +33,7 @@ class CurrencyMismatch < StandardError; end greater_than: -1 } validates :price, numericality: true + validate :price_match_order_currency after_save :update_inventory @@ -42,7 +43,7 @@ class CurrencyMismatch < StandardError; end delegate :name, :description, :sku, :should_track_inventory?, to: :variant delegate :currency, to: :order, allow_nil: true - attr_accessor :target_shipment + attr_accessor :target_shipment, :price_currency self.whitelisted_ransackable_associations = ['variant'] self.whitelisted_ransackable_attributes = ['variant_id'] @@ -111,9 +112,10 @@ def total_excluding_vat def money_price=(money) if !money self.price = nil - elsif money.currency.iso_code != currency + elsif money.currency.iso_code != currency && Spree::Config.raise_with_invalid_currency raise CurrencyMismatch, "Line item price currency must match order currency!" else + self.price_currency = money.currency.iso_code self.price = money.to_d end end @@ -202,5 +204,11 @@ def update_inventory def destroy_inventory_units inventory_units.destroy_all end + + def price_match_order_currency + return if price_currency.blank? || price_currency == currency + + errors.add(:price, "Line item price currency must match order currency!") + end end end diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 807173705a8..700d5889d50 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -116,6 +116,13 @@ class AppConfiguration < Preferences::Configuration # @return [String] ISO 4217 Three letter currency code preference :currency, :string, default: "USD" + # @!attribute [rw] raise_with_invalid_currency + # Whether to raise an exception if trying to set a line item currency + # different from the order currency. When false a validation error + # is added to the instance instead. + # @return [Boolean] (default: +true+) + preference :raise_with_invalid_currency, :boolean, default: true + # @!attribute [rw] default_country_id # @deprecated Use the default country ISO preference instead # @return [Integer,nil] id of {Spree::Country} to be selected by default in dropdowns (default: nil) diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index bc122dfdada..6f843012c93 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -220,10 +220,27 @@ def copy_price context 'when the price has a currency different from the order currency' do let(:currency) { "RUB" } - it 'raises an exception' do - expect { + before do + stub_spree_preferences(raise_with_invalid_currency: raise_with_invalid_currency) + end + + context 'when raise_with_invalid_currency preference is true' do + let(:raise_with_invalid_currency) { true } + + it 'raises an exception' do + expect { + line_item.money_price = new_price + }.to raise_exception Spree::LineItem::CurrencyMismatch + end + end + + context 'when raise_with_invalid_currency preference is false' do + let(:raise_with_invalid_currency) { false } + + it 'is not valid' do line_item.money_price = new_price - }.to raise_exception Spree::LineItem::CurrencyMismatch + expect(line_item).not_to be_valid + end end end end From 54f7451e3866990cea91edc5c324274471ac771b Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 12 Dec 2019 16:44:27 +0100 Subject: [PATCH 2/4] Set raise_with_invalid_currency to false for new app This ensures that the DummyApp that we use for specs and new applications will use the new default behavior, while preserving the old behavior for existing applications that are just upgrading. Setting this new default for the DummyApp allows us to restore a pending spec that was skipped for the inconsistent raise behavior that we want to deprecate. --- .../spree/api/orders_controller_spec.rb | 50 +++++++++++++------ .../templates/config/initializers/spree.rb.tt | 8 +++ core/lib/spree/testing_support/dummy_app.rb | 1 + 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/api/spec/requests/spree/api/orders_controller_spec.rb b/api/spec/requests/spree/api/orders_controller_spec.rb index 8aaaf08e43f..d555dceca9d 100644 --- a/api/spec/requests/spree/api/orders_controller_spec.rb +++ b/api/spec/requests/spree/api/orders_controller_spec.rb @@ -110,9 +110,11 @@ module Spree end context 'when the line items have custom attributes' do - it "can create an order with line items that have custom permitted attributes", :pending do + it "can create an order with line items that have custom permitted attributes" do PermittedAttributes.line_item_attributes << { options: [:some_option] } - expect_any_instance_of(Spree::LineItem).to receive(:some_option=).once.with('4') + without_partial_double_verification do + expect_any_instance_of(Spree::LineItem).to receive(:some_option=).once.with('4') + end post spree.api_orders_path, params: { order: { line_items: { "0" => { variant_id: variant.to_param, quantity: 5, options: { some_option: 4 } } } } } expect(response.status).to eq(201) order = Order.last @@ -419,23 +421,43 @@ module Spree expect(json_response['email']).to eq "guest@spreecommerce.com" end - # Regression test for https://github.com/spree/spree/issues/3404 - it "can specify additional parameters for a line item" do - without_partial_double_verification do - expect_any_instance_of(Spree::LineItem).to receive(:special=).with("foo") + context "specifying additional parameters for a line items" do + # Regression test for https://github.com/spree/spree/issues/3404 + it "is allowed on line item level" do + without_partial_double_verification do + expect_any_instance_of(Spree::LineItem).to receive(:special=).with("foo") + end + + allow_any_instance_of(Spree::Api::OrdersController).to receive_messages(permitted_line_item_attributes: [:id, :variant_id, :quantity, :special]) + post spree.api_orders_path, params: { + order: { + line_items: { + "0" => { + variant_id: variant.to_param, quantity: 5, special: "foo" + } + } + } + } + expect(response.status).to eq(201) end - allow_any_instance_of(Spree::Api::OrdersController).to receive_messages(permitted_line_item_attributes: [:id, :variant_id, :quantity, :special]) - post spree.api_orders_path, params: { - order: { - line_items: { - "0" => { - variant_id: variant.to_param, quantity: 5, special: "foo" + it "is allowed using options parameter" do + without_partial_double_verification do + expect_any_instance_of(Spree::LineItem).to receive(:special=).with("foo") + end + + allow_any_instance_of(Spree::Api::OrdersController).to receive_messages(permitted_line_item_attributes: [:id, :variant_id, :quantity, options: :special]) + post spree.api_orders_path, params: { + order: { + line_items: { + "0" => { + variant_id: variant.to_param, quantity: 5, options: { special: "foo" } + } } } } - } - expect(response.status).to eq(201) + expect(response.status).to eq(201) + end end it "cannot arbitrarily set the line items price" do diff --git a/core/lib/generators/spree/install/templates/config/initializers/spree.rb.tt b/core/lib/generators/spree/install/templates/config/initializers/spree.rb.tt index 7c38954a388..56fd870b9c5 100644 --- a/core/lib/generators/spree/install/templates/config/initializers/spree.rb.tt +++ b/core/lib/generators/spree/install/templates/config/initializers/spree.rb.tt @@ -22,6 +22,14 @@ Spree.config do |config| config.image_attachment_module = 'Spree::Image::PaperclipAttachment' config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment' + # Defaults + + # Set this configuration to `true` to raise an exception when + # an order is populated with a line item with a mismatching + # currency. The `false` value will just add a validation error + # and will be the only behavior accepted in future versions. + # See https://github.com/solidusio/solidus/pull/3456 for more info. + config.raise_with_invalid_currency = false # Permission Sets: diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb index 493bc1f5788..a0f17ddd45a 100644 --- a/core/lib/spree/testing_support/dummy_app.rb +++ b/core/lib/spree/testing_support/dummy_app.rb @@ -108,6 +108,7 @@ class Application < ::Rails::Application Spree.user_class = 'Spree::LegacyUser' Spree.config do |config| config.mails_from = "store@example.com" + config.raise_with_invalid_currency = false end # Raise on deprecation warnings From 13842b2903af0beb9805fa25a61d6edc65f6d21e Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Fri, 13 Dec 2019 15:07:08 +0100 Subject: [PATCH 3/4] Move currency mismatch error message under I18n Now that we have a validation in place we should let users change the error message in their language. For the exception, we always print the error with English locale. --- core/app/models/spree/line_item.rb | 5 +++-- core/config/locales/en.yml | 1 + core/spec/models/spree/line_item_spec.rb | 7 ++++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index d3608b3403a..714b04aba3c 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -113,7 +113,8 @@ def money_price=(money) if !money self.price = nil elsif money.currency.iso_code != currency && Spree::Config.raise_with_invalid_currency - raise CurrencyMismatch, "Line item price currency must match order currency!" + line_item_errors = ActiveModel::Errors.new(self) + raise CurrencyMismatch, line_item_errors.generate_message(:price, :does_not_match_order_currency, locale: :en) else self.price_currency = money.currency.iso_code self.price = money.to_d @@ -208,7 +209,7 @@ def destroy_inventory_units def price_match_order_currency return if price_currency.blank? || price_currency == currency - errors.add(:price, "Line item price currency must match order currency!") + errors.add(:price, :does_not_match_order_currency) end end end diff --git a/core/config/locales/en.yml b/core/config/locales/en.yml index 267eb0cc626..d0618afe577 100644 --- a/core/config/locales/en.yml +++ b/core/config/locales/en.yml @@ -468,6 +468,7 @@ en: attributes: price: not_a_number: is not valid + does_not_match_order_currency: Line item price currency must match order currency! spree/price: attributes: currency: diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 6f843012c93..eed3a7704b3 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -230,7 +230,10 @@ def copy_price it 'raises an exception' do expect { line_item.money_price = new_price - }.to raise_exception Spree::LineItem::CurrencyMismatch + }.to raise_exception( + Spree::LineItem::CurrencyMismatch, + 'Line item price currency must match order currency!' + ) end end @@ -240,6 +243,8 @@ def copy_price it 'is not valid' do line_item.money_price = new_price expect(line_item).not_to be_valid + expect(line_item.errors[:price]) + .to include 'Line item price currency must match order currency!' end end end From d19edf151d141c7663d5aab5e7f205a5635afe51 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Fri, 13 Dec 2019 15:42:10 +0100 Subject: [PATCH 4/4] Deprecate raise_with_invalid_currency set to true This is done into an initializer to be sure users will look at this deprecation warning, even if the exception is not raised. --- core/lib/spree/core/engine.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/core/lib/spree/core/engine.rb b/core/lib/spree/core/engine.rb index 663806db34e..9a601295bf7 100644 --- a/core/lib/spree/core/engine.rb +++ b/core/lib/spree/core/engine.rb @@ -55,6 +55,17 @@ class Engine < ::Rails::Engine end end + config.after_initialize do + if Spree::Config.raise_with_invalid_currency == true + Spree::Deprecation.warn( + 'Spree::Config.raise_with_invalid_currency set to true is ' \ + 'deprecated. Please note that by switching this value, ' \ + 'Spree::LineItem::CurrencyMismatch will not be raised anymore.', + caller + ) + end + end + # Load in mailer previews for apps to use in development. # We need to make sure we call `Preview.all` before requiring our # previews, otherwise any previews the app attempts to add need to be