spree-contrib / spree_multi_currency

Provides UI to allow configuring multiple currencies in Spree.
http://guides.spreecommerce.org
BSD 3-Clause "New" or "Revised" License
41 stars 117 forks source link

[Critical] Invalid Price in Checkout Step 2 in 3-1-stable #64

Closed eroy4u closed 6 years ago

eroy4u commented 8 years ago

This is a very critical bug the price is wrong when checking out in a currency which is different from the currency set in spree admin. For example, in storefront you change the storefront currency to EUR (provided that the "choose currency" settings in Admin is "USD"), buy an item in EUR and checkout, after filling in your address and go to step 2 delivery, you'll find the order summary on the right is showing the EUR sign but using the USD price.

This is very critical as I think it's the most basic use case for multi currency. With this bug this extension is useless. I found the bug on 3-1-stable. I also tried the 2-4-stable branch and same issue. This means this issue has been there for several versions.

Steps to reproduce: 1) "rails new" and do a fresh installation of spree with multi currency on branch 3-1, gem 'spree', github: 'spree/spree', branch: '3-1-stable' gem 'spree_auth_devise', github: 'spree/spree_auth_devise', branch: '3-1-stable' gem 'spree_gateway', github: 'spree/spree_gateway', branch: '3-1-stable' gem 'spree_multi_currency', github: 'spree-contrib/spree_multi_currency', branch: '3-1-stable'

2) go to admin and input "USD,EUR" as the supported currencies, select the boxes "Allow currency change" and "show currency selector" and save 3) go to storefront, change currency to "EUR" then add any product and checkout to the delivery step. screen shot 2016-09-14 at 12 14 16 am

In checkout step 2, you'll find the item price is EUR 14.00 but the item total is EUR 15.99(which is from the 15.99 USD set in the database by default)

racx commented 8 years ago

I tested it, indeed it is happening for me latest 3.1 stable

nmccafferty commented 8 years ago

This is exactly the same issue I've been digging into using the 3.1 stable branch also. This is unusable in current state. Investigations thus far show this happens when method update_line_item_prices! calls update_price against each of the line_items. There are 2 versions of the Price object persisted but the dollar one is being retrieved from the database rather than the Euro one which the order was created against.

nmccafferty commented 8 years ago

The issue seems to stem from the call to price_including_vat_for against the variant. This delegates the method onto it's price object, but from the variant it has no way to know whether the price is a EUR / USD object, it instead uses the Spree::Config[:currency] value to determine which price to use. Therefore I expect a change needs to be applied where this info is known, in the line_item.rb method update_price to get back the correct price object and invoke price_including_vat_for directly. I have tested the following locally and it seems to resolve the basic issue:

def update_price
  currency_price = Spree::Price.where(
    currency: order.currency,
    variant_id: variant_id
  ).first
  self.price = currency_price.price_including_vat_for(tax_zone: tax_zone)
end
arvindvyas commented 8 years ago

@nmccafferty any other update you did on this apart from this, I was also facing same problem but when I have implemented it it is quit working fine but still it is reducing actual price. Let me know if you can help me on it

nmccafferty commented 8 years ago

@arvindvyas There was a mistake in the earlier code, please try with the updated version above and let me know how you get on. I'd plan to add this to the core gem once I patch my own app and have it working first.

arvindvyas commented 8 years ago

@nmccafferty ya I have seen that and fixed that accordingly , thanks

hmphu commented 7 years ago

I have the same issue. This is a very critical bug and the fix from @nmccafferty is working perfectly.

@damianlegawiec can you please merge this ?