Remove currency from line items #1507

kennyadsl · 2016-10-07T16:05:49Z

This PR removes currency from line items. That field is no more useful since all order contents will have the same currency (the order one).

ref: #1399

TODO:

Understand what happens when the order currency is changed. Can this happen? Shouldn't we recalculate prices for all line items with the new currency? Is this already handled by the order merger? Handled with a separate Issue
Remove currency from spree_line_items database table. I'm a bit concerned about this. What happens if there are users that rely on this field has custom logic related to line item currency (like statistics)? They could loose them. Can we proceed anyway or it's better to leave the field in the database and mark it for a future removal?
Update changelog

There's no need to compare currency on all line items since they all have the same currency, same as their order.

mamhoff

This is really, really good work. I have a couple of more nits, but I think the general direction is great. 👍

mamhoff · 2016-10-08T13:14:45Z

core/app/models/spree/line_item.rb

+    end
+
+    def currency
+      order.currency if order
    end


This can be written shorter as a delegate:

delegate :currency, to: :order

mamhoff · 2016-10-08T13:31:17Z

core/app/models/spree/line_item.rb

    #
-    # @param [Spree::Money] money - the money object to obtain price and currency from
+    # @param [Spree::Money] money - the money object to obtain price from
    def money_price=(money)


I think this should have a Safeguard: It should raise a good error when trying to assign a price with a currency different to the order currency. Otherwise the information would just be lost. So something along these lines:

class CurrencyMismatch < StandardError; end def money_price=(money) return unless money raise CurrencyMismatch, "Line item price currency must match order currency!" if money.currency.iso_code != currency self.price = money.to_d end

done, but assigning nil to line item price when money is nil.

Yes, this is good. I have a feeling one could golf around in this method a bit more, but I'm fine leaving it as is because it does the right things.

mamhoff · 2016-10-08T13:33:36Z

core/app/models/spree/line_item.rb

      end
    end

-    def pricing_options


I like removing things from the public interface, but I'm not sure about moving this method to the private section.

I reverted this change for now. I'm probably missing something but at the moment I cannot see any scenario when something else then the line item itself could need to be aware of these specific line item pricing options. That's why I'd move it to the private section, but I'm ok leaving it in the public one as well.

mamhoff · 2016-10-08T13:34:23Z

core/app/models/spree/line_item.rb

      self.cost_price ||= variant.cost_price
-      self.money_price = variant.price_for(pricing_options) if price.nil?
+      self.money_price = price_from_options if price.nil?


Is there a reason for extracting this method? I wouldn't do that. :)

reading the code again, I think you are right. There's no reason to extract this method. Going to revert this change.

mamhoff · 2016-10-08T13:35:07Z

core/app/models/spree/variant/pricing_options.rb

@@ -33,7 +33,7 @@ def self.default_price_attributes
      def self.from_line_item(line_item)
        tax_address = line_item.order.try!(:tax_address)
        new(
-          currency: line_item.order.try(:currency) || line_item.currency || Spree::Config.currency,
+          currency: line_item.order.try(:currency) || Spree::Config.currency,


With the delegate, this can actually be written shorter as line_item.currency :)

it needs `allow_nil` set to true because the variant pricing options should work even when the line item has no order associated.

Previously if a price with a currency different from the order currency was set on a line item, it was just taking the wrong price amount without explictily rasing an error.

kennyadsl

@mamhoff thanks a lot for the hints! Please let me know if you see anything else that can be improved.

kennyadsl · 2016-10-08T22:17:00Z

core/app/models/spree/line_item.rb

      end
    end

-    def pricing_options


I reverted this change for now. I'm probably missing something but at the moment I cannot see any scenario when something else then the line item itself could need to be aware of these specific line item pricing options. That's why I'd move it to the private section, but I'm ok leaving it in the public one as well.

It's no more allowed to have line item with a currency different from the order currency. The line item currency will be taken from the order directly so this field is no more needed.

kennyadsl · 2016-10-11T07:55:53Z

@mamhoff please check again when you have time, thanks a lot!

mamhoff

Excellent, @kennyadsl - I think this is good to go now!

mamhoff · 2016-10-11T15:58:34Z

core/app/models/spree/line_item.rb

    #
-    # @param [Spree::Money] money - the money object to obtain price and currency from
+    # @param [Spree::Money] money - the money object to obtain price from
    def money_price=(money)


Yes, this is good. I have a feeling one could golf around in this method a bit more, but I'm fine leaving it as is because it does the right things.

mamhoff · 2016-10-11T15:59:17Z

core/app/models/spree/line_item.rb

+      # When price is part of the options we are not going to fetch
+      # it from the variant. Please note that this always allows to set
+      # a price for this line item, even if there is no existing price
+      # for the associated line item in the order currency.


Really good documentation. I wish I'd had that when first reading this method :)

jhawthorn · 2016-10-12T16:17:14Z

core/app/models/spree/line_item.rb

+        self.price = nil
+        return
+      end
+      raise CurrencyMismatch, "Line item price currency must match order currency!" if money.currency.iso_code != currency
      self.price = money.to_d


This is a little tough to parse with the early return inside the unless (which has me thinking about the rest of the method like the double negative unless...else).

I think it would read better as an if/elseif/else

if !money self.price = nil elsif money.currency.iso_code != currency raise CurrencyMismatch, "Line item price currency must match order currency!" else self.price = money.to_d end

@jhawthorn thanks for your review. Agree on the better readability of suggested version. Changed!

jhawthorn

Other than one suggestion this is great.

Thank you @kennyadsl

This prevents to keep storing the old price (valid) when passing a new nil money object (invalid) to the `money_price` setter method. Previously: line_item.price => #<BigDecimal: 33.22> line_item.money_price = nil line_item.price => #<BigDecimal: 33.22> Now: line_item.price => #<BigDecimal: 33.22> line_item.money_price = nil line_item.price => nil

kennyadsl mentioned this pull request Oct 7, 2016

Why do we store currency on line items #1399

Closed

kennyadsl force-pushed the remove-currency-from-line-items branch from 5c0c145 to f2d2dde Compare October 7, 2016 16:07

Merging orders checks order currency instead of line items

71cc66c

There's no need to compare currency on all line items since they all have the same currency, same as their order.

kennyadsl force-pushed the remove-currency-from-line-items branch 2 times, most recently from f4dc323 to 571b3d7 Compare October 8, 2016 10:28

mamhoff suggested changes Oct 8, 2016

View reviewed changes

kennyadsl added 4 commits October 9, 2016 00:12

Remove currency from line items

b6bef7b

Reflect line item's currency removal on Order import specs

b9f7ca6

Delegate line item currency to order

d8056cc

it needs `allow_nil` set to true because the variant pricing options should work even when the line item has no order associated.

Raise when a line item gets the wrong currency

d47b62c

Previously if a price with a currency different from the order currency was set on a line item, it was just taking the wrong price amount without explictily rasing an error.

kennyadsl force-pushed the remove-currency-from-line-items branch from 571b3d7 to d47b62c Compare October 8, 2016 22:13

kennyadsl commented Oct 8, 2016

View reviewed changes

kennyadsl added 2 commits October 9, 2016 16:16

Remove currency from line items database table

9c98e1a

It's no more allowed to have line item with a currency different from the order currency. The line item currency will be taken from the order directly so this field is no more needed.

Update CHANGELOG

34ffb71

kennyadsl changed the title ~~WIP: Remove currency from line items~~ Remove currency from line items Oct 11, 2016

mamhoff approved these changes Oct 11, 2016

View reviewed changes

jhawthorn reviewed Oct 12, 2016

View reviewed changes

jhawthorn approved these changes Oct 12, 2016

View reviewed changes

kennyadsl force-pushed the remove-currency-from-line-items branch from 73660fa to 1d92f39 Compare October 12, 2016 18:39

mamhoff approved these changes Oct 13, 2016

View reviewed changes

jhawthorn merged commit 20fc1b0 into solidusio:master Oct 13, 2016

kennyadsl deleted the remove-currency-from-line-items branch October 14, 2016 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove currency from line items #1507

Remove currency from line items #1507

kennyadsl commented Oct 7, 2016 •

edited

Loading

mamhoff left a comment

mamhoff Oct 8, 2016

mamhoff Oct 8, 2016 •

edited

Loading

kennyadsl Oct 10, 2016

mamhoff Oct 11, 2016

mamhoff Oct 8, 2016

kennyadsl Oct 8, 2016

mamhoff Oct 8, 2016

kennyadsl Oct 8, 2016

mamhoff Oct 8, 2016

kennyadsl left a comment

kennyadsl Oct 8, 2016

kennyadsl commented Oct 11, 2016

mamhoff left a comment

mamhoff Oct 11, 2016

mamhoff Oct 11, 2016

jhawthorn Oct 12, 2016

kennyadsl Oct 12, 2016

jhawthorn left a comment

Remove currency from line items #1507

Remove currency from line items #1507

Conversation

kennyadsl commented Oct 7, 2016 • edited Loading

TODO:

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Oct 11, 2016

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhawthorn left a comment

Choose a reason for hiding this comment

kennyadsl commented Oct 7, 2016 •

edited

Loading

mamhoff Oct 8, 2016 •

edited

Loading