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

Fix invalid checkout price issue #65

Closed nmccafferty closed 7 years ago

nmccafferty commented 8 years ago

Fix for https://github.com/spree-contrib/spree_multi_currency/issues/64

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 and comes back with the USD object instead of the EUR object.

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.

krzysiek1507 commented 8 years ago

Hi @nmccafferty could you please make these changes against master?

nmccafferty commented 8 years ago

@krzysiek1507 I had initially tried to make the changes against the master but a fresh bundle install fails, colleague has now confirmed this for me, he gets the same:

Bundler could not find compatible versions for gem "actionview":
  In Gemfile:
    spree_auth_devise was resolved to 3.2.0.alpha, which depends on
      devise (~> 4.0.0) was resolved to 4.0.0, which depends on
        railties (< 5.1, >= 4.1.0) was resolved to 4.1.1, which depends on
          actionpack (= 4.1.1) was resolved to 4.1.1, which depends on
            actionview (= 4.1.1)

    rspec-rails (~> 3.4.0) was resolved to 3.4.0, which depends on
      actionpack (< 4.3, >= 3.0) was resolved to 4.1.1, which depends on
        actionview (= 4.1.0)
Bundler could not find compatible versions for gem "activerecord":
  In Gemfile:
    spree was resolved to 3.2.0.beta1, which depends on
      spree_frontend (= 3.2.0.beta1) was resolved to 3.2.0.beta1, which depends on
        canonical-rails (~> 0.1.0) was resolved to 0.1.0, which depends on
          rails (< 5.1, >= 3.1) was resolved to 5.0.0, which depends on
            activerecord (= 4.2.0)

    spree was resolved to 3.2.0.beta1, which depends on
      spree_core (= 3.2.0.beta1) was resolved to 3.2.0.beta1, which depends on
        rails (~> 5.0.0) was resolved to 5.0.0, which depends on
          activerecord (= 5.0.0)
Bundler could not find compatible versions for gem "activesupport":
  In Gemfile:
    spree_auth_devise was resolved to 3.2.0.alpha, which depends on
      devise (~> 4.0.0) was resolved to 4.0.0, which depends on
        railties (< 5.1, >= 4.1.0) was resolved to 4.1.0, which depends on
          actionpack (= 4.1.0) was resolved to 4.1.0, which depends on
            activesupport (= 4.1.0)

    spree_auth_devise was resolved to 3.2.0.alpha, which depends on
      devise (~> 4.0.0) was resolved to 4.0.0, which depends on
        railties (< 5.1, >= 4.1.0) was resolved to 4.1.0, which depends on
          actionpack (= 4.1.0) was resolved to 4.1.0, which depends on
            activesupport (= 4.1.0)

    rspec-rails (~> 3.4.0) was resolved to 3.4.0, which depends on
      actionpack (< 4.3, >= 3.0) was resolved to 4.1.0, which depends on
        activesupport (= 3.0.0)

    factory_girl (~> 4.4) was resolved to 4.7.0, which depends on
      activesupport (>= 3.0.0)

    rspec-rails (~> 3.4.0) was resolved to 3.4.0, which depends on
      activesupport (< 4.3, >= 3.0)
Bundler could not find compatible versions for gem "rspec-expectations":
  In Gemfile:
    guard-rspec (>= 4.2.0) was resolved to 4.7.3, which depends on
      rspec (< 4.0, >= 2.99.0) was resolved to 3.5.0, which depends on
        rspec-expectations (~> 3.5.0)

    rspec-rails (~> 3.4.0) was resolved to 3.4.0, which depends on
      rspec-expectations (~> 3.4.0)
arvindvyas commented 8 years ago

@krzysiek1507

"Hi @nmccafferty could you please make these changes against master?"

why against when this issue fix for 3-1-stable ??

krzysiek1507 commented 8 years ago

@arvindvyas I think we want to fix it also in next versions ;) So we can fix it on master and cherry-pick to previous versions. Do you agree?

arvindvyas commented 8 years ago

@krzysiek1507 Don't you think it is important is fix stable branch first? if you don't fix it for un stable branch it is not that important .

nmccafferty commented 7 years ago

@krzysiek1507 Is there any chance you can merge this in as it really is quite a critical issue in the project? I had tried to follow your request to add to master but the master build would not bundle as I've instructed before. I'll continue working with a patched version until you can sort this out.

arvindvyas commented 7 years ago

@nmccafferty seems they don't care!

damianlegawiec commented 7 years ago

@nmccafferty @arvindvyas merged! Sorry for the long wait!

damianlegawiec commented 7 years ago

@nmccafferty @arvindvyas btw. master branch supports 3.1, 3.2 and 3.3