Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate raising an exception when order and line item currencies mismatch #3456

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 36 additions & 14 deletions api/spec/requests/spree/api/orders_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -419,23 +421,43 @@ module Spree
expect(json_response['email']).to eq "[email protected]"
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
Expand Down
15 changes: 12 additions & 3 deletions core/app/models/spree/line_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class CurrencyMismatch < StandardError; end
greater_than: -1
}
validates :price, numericality: true
validate :price_match_order_currency

after_save :update_inventory

Expand All @@ -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']
Expand Down Expand Up @@ -111,9 +112,11 @@ def total_excluding_vat
def money_price=(money)
if !money
self.price = nil
elsif money.currency.iso_code != currency
raise CurrencyMismatch, "Line item price currency must match order currency!"
elsif money.currency.iso_code != currency && Spree::Config.raise_with_invalid_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
end
end
Expand Down Expand Up @@ -202,5 +205,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
kennyadsl marked this conversation as resolved.
Show resolved Hide resolved

errors.add(:price, :does_not_match_order_currency)
end
end
end
1 change: 1 addition & 0 deletions core/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ Spree.config do |config|
config.image_attachment_module = 'Spree::Image::PaperclipAttachment'
config.taxon_attachment_module = 'Spree::Taxon::PaperclipAttachment'

# Defaults
kennyadsl marked this conversation as resolved.
Show resolved Hide resolved

# 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:

Expand Down
7 changes: 7 additions & 0 deletions core/lib/spree/app_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions core/lib/spree/core/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions core/lib/spree/testing_support/dummy_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class Application < ::Rails::Application
Spree.user_class = 'Spree::LegacyUser'
Spree.config do |config|
config.mails_from = "[email protected]"
config.raise_with_invalid_currency = false
end

# Raise on deprecation warnings
Expand Down
28 changes: 25 additions & 3 deletions core/spec/models/spree/line_item_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,32 @@ 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,
'Line item price currency must match order currency!'
aldesantis marked this conversation as resolved.
Show resolved Hide resolved
)
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
expect(line_item.errors[:price])
.to include 'Line item price currency must match order currency!'
end
end
end
end
Expand Down