diff --git a/CHANGELOG.md b/CHANGELOG.md index f8d17fe4345..cc9a1d3f56c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ ## Solidus 1.3.0 (unreleased) +* Remove currency attribute on `Spree::LineItem` + + A valid line item always has its order's currency. Therefore it is not necessary to save + that in the database. + * 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..4e82200149c 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -37,6 +37,16 @@ class LineItem < Spree::Base # item, if there is one delegate :product, to: :variant + # This is just here to invalidate the line item when a currency different to the orders currency + # is passed in. + # @deprecated The order already has a currency. + attr_writer :currency + deprecate :currency= => "Assigning a currency to a line item is deprecated. It will always take the one from the order", + deprecator: Spree::Deprecation + + # @note Every valid line item will always have the currency of its order. + delegate :currency, to: :order + attr_accessor :target_shipment self.whitelisted_ransackable_associations = ['variant'] @@ -104,15 +114,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 - - currency = opts.delete(:currency) || order.currency - - self.currency = currency - self.price = variant.price_in(currency).amount + - variant.price_modifier_amount_in(currency, opts) + self.price = variant.price_in(currency).amount + + variant.price_modifier_amount_in(currency, options) - assign_attributes opts + assign_attributes options end private @@ -136,7 +141,6 @@ 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.cost_price ||= variant.cost_price self.price ||= variant.price end @@ -175,7 +179,8 @@ def update_tax_charge end def ensure_proper_currency - unless currency == order.currency + # Only if the deprecated setter has been called. Otherwise the check is moot. + if @currency && @currency != order.currency 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..b1565d0a9a3 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,19 @@ 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 + ) + + 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/db/migrate/20160320190803_remove_currency_from_line_item.rb b/core/db/migrate/20160320190803_remove_currency_from_line_item.rb new file mode 100644 index 00000000000..0fe02b5c79e --- /dev/null +++ b/core/db/migrate/20160320190803_remove_currency_from_line_item.rb @@ -0,0 +1,5 @@ +class RemoveCurrencyFromLineItem < ActiveRecord::Migration + def change + remove_column :spree_line_items, :currency, :string + end +end diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 858f49e6d43..a58a45bf30e 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -94,6 +94,10 @@ subject(:line_item) { Spree::LineItem.new(variant: variant, order: order) } + it 'delegates currency to order' do + expect(line_item.currency).to eq(order.currency) + end + # Tests for https://github.com/spree/spree/issues/3391 context 'before validation' do before { line_item.valid? } @@ -106,10 +110,6 @@ 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) - end - # Test for https://github.com/spree/spree/issues/3481 it 'copies the variants tax category' do expect(line_item.tax_category).to eq(line_item.variant.tax_category) @@ -121,7 +121,6 @@ before(:context) do Spree::LineItem.class_eval do def copy_price - self.currency = "USD" self.cost_price = 10 self.price = 20 end @@ -164,12 +163,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 +181,29 @@ 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 } - expect(line_item.error_on(:currency).size).to eq(0) + before do + expect(Spree::Deprecation).to receive(:warn).at_least(:once) 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 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(1) + expect(line_item.error_on(:currency).size).to eq(0) + end + end + + context "currency different than order.currency" do + it "is not a valid line item" do + line_item.currency = "no currency" + line_item.valid? + + expect(line_item.error_on(:currency).size).to eq(1) + end end end