From cdaea7df70af7c5fb1e98cc119ea052f8a585809 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 21 Mar 2016 11:03:52 +0100 Subject: [PATCH] Remove LineItem#currency column Somehow, in the past, it was possible to have orders with line items in multiple currencies. The code to remedy this (Order#homogenize_line_item_currencies) disappeared in #635, and now we have a bunch of code in different places that allows setting it, raises errors if it's not there, and so on. This commit delegates the line items currency to the order. Whenever people try *setting* a line item's currency, they get a pretty deprecation warning telling them not to do so. The error handling functionality that seems to be used in the order importer is still present (ie: If we know an imported line item has a currency different from the order, we'll error out). This commit replaces all that behaviour with a simple delegate that always obtains a line item's currency from it's order. Adds a changelog entry. I also refactored `OrderContents#add_to_line_item`, taking code from https://github.com/MountainRoseHerbs/spree/commit/461f97510c29d50c08c6d8b75805b66f12de12af That method should now be much easier to understand. --- CHANGELOG.md | 5 ++ core/app/models/spree/line_item.rb | 25 ++++++---- core/app/models/spree/order_contents.rb | 22 +++++---- ...20190803_remove_currency_from_line_item.rb | 5 ++ core/spec/models/spree/line_item_spec.rb | 47 +++++++++---------- 5 files changed, 60 insertions(+), 44 deletions(-) create mode 100644 core/db/migrate/20160320190803_remove_currency_from_line_item.rb 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