Skip to content
This repository has been archived by the owner on Feb 27, 2021. It is now read-only.

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

Closed
eroy4u opened this issue Sep 14, 2016 · 7 comments
Closed

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

eroy4u opened this issue Sep 14, 2016 · 7 comments

Comments

@eroy4u
Copy link

eroy4u commented Sep 14, 2016

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
Copy link

racx commented Sep 14, 2016

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

@nmccafferty
Copy link
Contributor

nmccafferty commented Sep 16, 2016

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
Copy link
Contributor

nmccafferty commented Sep 16, 2016

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
Copy link

@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
Copy link
Contributor

@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
Copy link

arvindvyas commented Sep 16, 2016

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

@hmphu
Copy link

hmphu commented Feb 7, 2017

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 ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants