From 32ed8cdf845924015dc10e96dc87a5c7492afcb4 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 24 Mar 2016 22:32:13 +0100 Subject: [PATCH] Always set a line item's currency to order currency Before, a line item could under certain circumstances be assigned the store's default currency, rather than its order's currency. With this commit, we're making sure Solidus does not do that, as well as deprecating setting the currency manually to something other than the order's currency. --- CHANGELOG.md | 7 +++++ core/app/models/spree/line_item.rb | 18 +++++------ core/app/models/spree/order_contents.rb | 23 ++++++++------ core/spec/models/spree/line_item_spec.rb | 40 +++++++++++------------- 4 files changed, 47 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8d17fe4345..0c365e4c320 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ ## Solidus 1.3.0 (unreleased) +* Deprecate setting a line item's currency by hand + + Previously, a line item's currency could be set directly, and differently from the line item's + order's currency. This would result in an error. It still does, but is also now explicitly + deprecated. In the future, we might delete the line item's `currency` column and just delegate + to the line item's order. + * Taxes for carts now configurable via the `Spree::Store` object In VAT countries, carts (orders without addresses) have to be shown with diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index f0af132b90a..db63644c186 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -104,15 +104,10 @@ def insufficient_stock? def options=(options = {}) return unless options.present? - opts = options.dup # we will be deleting from the hash, so leave the caller's copy intact + self.price = variant.price_in(currency).amount + + variant.price_modifier_amount_in(currency, options) - currency = opts.delete(:currency) || order.currency - - self.currency = currency - self.price = variant.price_in(currency).amount + - variant.price_modifier_amount_in(currency, opts) - - assign_attributes opts + assign_attributes options end private @@ -136,7 +131,7 @@ def set_pricing_attributes # If the legacy method #copy_price has been overridden, handle that gracefully return handle_copy_price_override if respond_to?(:copy_price) - self.currency ||= variant.currency + self.currency ||= order.currency self.cost_price ||= variant.cost_price self.price ||= variant.price end @@ -175,7 +170,10 @@ def update_tax_charge end def ensure_proper_currency - unless currency == order.currency + if currency != order.currency + Spree::Deprecation.warn "The line items currency is different from it's order currency. " \ + "This behavior is not supported anymore and will be deleted soon.", + caller errors.add(:currency, :must_match_order_currency) end end diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index 7cbb934c4ad..83faad38122 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -1,6 +1,6 @@ module Spree class OrderContents - attr_accessor :order, :currency + attr_accessor :order def initialize(order) @order = order @@ -79,17 +79,20 @@ def reload_totals def add_to_line_item(variant, quantity, options = {}) line_item = grab_line_item_by_variant(variant, false, options) - if line_item - line_item.quantity += quantity.to_i - line_item.currency = currency unless currency.nil? - else - opts = { currency: order.currency }.merge ActionController::Parameters.new(options).permit(PermittedAttributes.line_item_attributes).to_h - line_item = order.line_items.new(quantity: quantity, - variant: variant, - options: opts) + line_item ||= order.line_items.new( + quantity: 0, + variant: variant, + currency: order.currency + ) + + line_item.quantity += quantity.to_i + line_item.options = ActionController::Parameters.new(options).permit(PermittedAttributes.line_item_attributes) + + if line_item.new_record? create_order_stock_locations(line_item, options[:stock_location_quantities]) end - line_item.target_shipment = options[:shipment] if options.key? :shipment + + line_item.target_shipment = options[:shipment] line_item.save! line_item end diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 858f49e6d43..ad84996596a 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -106,8 +106,8 @@ expect(line_item.cost_price).to eq(variant.cost_price) end - it 'copies the variants currency' do - expect(line_item.currency).to eq(variant.currency) + it "copies the order's currency" do + expect(line_item.currency).to eq(order.currency) end # Test for https://github.com/spree/spree/issues/3481 @@ -164,12 +164,6 @@ def copy_price end end - describe '.currency' do - it 'returns the globally configured currency' do - line_item.currency == 'USD' - end - end - describe ".money" do before do line_item.price = 3.50 @@ -188,23 +182,27 @@ def copy_price end end - context "currency same as order.currency" do - it "is a valid line item" do - line_item = order.line_items.first - line_item.currency = order.currency - line_item.valid? + context 'setting the currency' do + let(:line_item) { order.line_items.first } + + context "currency same as order.currency" do + it "is a valid line item" do + line_item.currency = order.currency + line_item.valid? - expect(line_item.error_on(:currency).size).to eq(0) + expect(line_item.error_on(:currency).size).to eq(0) + end end - end - context "currency different than order.currency" do - it "is not a valid line item" do - line_item = order.line_items.first - line_item.currency = "no currency" - line_item.valid? + context "currency different than order.currency" do + it "is not a valid line item" do + expect(Spree::Deprecation).to receive(:warn).at_least(:once) - expect(line_item.error_on(:currency).size).to eq(1) + line_item.currency = "no currency" + line_item.valid? + + expect(line_item.error_on(:currency).size).to eq(1) + end end end