Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix StoreCredit with multiple currencies #2183

Merged

Conversation

jordan-brough
Copy link
Contributor

@jordan-brough jordan-brough commented Aug 23, 2017

Please see individual commits.

High level overview:

  • Only apply StoreCredit to an Order if the StoreCredit currency matches the Order currency
  • Group StoreCredits by currency in the admin

It took some effort to not attempt a refactor of a bunch of the scary or weird-looking StoreCredit code I noticed while working on this.

No functionality changes.
- StoreCredits can only be used with orders of the same currency.
- Summing up amounts of different currencies gives a meaningless number.
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense to me

This will replace `User#total_available_store_credit`.
This fixes this scenario:

- Joe gets $5 USD in store credit
- Joe starts an order in JPY
- Joe starts checking out and `Spree::Order#add_store_credit_payments` applies
Joe's $5 store credit as ¥5 (which is around $0.05 currently)
- Joe continues checkout and tries to complete his order but the payment
auth/capture fails when Solidus realizes the store credit currency doesn't match
the order currency.  Joe cannot check out because he has no way to prevent
Solidus from trying to apply his store credit.
To fix potential currency problems when a User has store credits that don't
match the currency of their current Order.
And the corresponding User#display_total_available_store_credit
@jordan-brough
Copy link
Contributor Author

jordan-brough commented Aug 24, 2017

FYI: I force-pushed after Thomas's review to fix some specs and add some specs. No code changes except for the last minor commit.

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too, thanks for following through jordan.

@jordan-brough jordan-brough merged commit 8e17226 into solidusio:master Aug 28, 2017
@jordan-brough jordan-brough deleted the store-credit-currency-fix branch August 28, 2017 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants