Comments

tnfno’s picture

Agree. Also the % off discount sometimes has issues with multicurrency. The total amount are sometimes off due to rounding of decimals, this gives a payment error since the line item total does not add up.

bardzusny’s picture

StatusFileSize
new1.42 KB

Hi,

I didn't really have problems with percentage discounts (so far...) - but indeed, fixed discounts on orders in different currencies than default site currency (the one in which we define discount amount) are simply not being handled.

I tinkered up a quick solution, patch included.

Basically: if commerce_multicurrency (the one we need anyway) is enabled, make sure discount is in order currency before applying it to it.

There is still no way to enter discount amount separately for each enabled currency, but it still seems to be enough for majority of use-cases. Comments and suggestions welcome.

skyredwang’s picture

Status: Active » Needs review
millionleaves’s picture

I've tried the patch in #2 and it appears to give the correct result - thanks. I've handed it to my customer for more thorough testing and will report back if any issues arise.

One point I'd note is that the patch is currently hard coded to assume the Discount module is in a Commerce Kickstart installation:

diff --git a/profiles/commerce_kickstart/modules/contrib/commerce_discount/commerce_discount.rules.inc b/profiles/commerce_kickstart/modules/contrib/commerce_discount/commerce_discount.rules.inc
index e3fde74..eb3aa40 100644
--- a/profiles/commerce_kickstart/modules/contrib/commerce_discount/commerce_discount.rules.inc
+++ b/profiles/commerce_kickstart/modules/contrib/commerce_discount/commerce_discount.rules.inc

When I supplied the correct path/filename, the patch installed without any further problems.

joelpittet’s picture

Status: Needs review » Postponed (maintainer needs more info)

This feels like a rabbit hole... Are there other parts of commerce_discount that will need the same check? Can we do this upstream in commerce instead, maybe with an alter hook that commerce_multicurrency can implement to adjust the order currency?

I'd rather not liter drupal_commerce with multicurrency checks, at most if we must do it here we can add an alter hook that allows them to adjust fix it there but I think I'd prefer commerce to deal with this problem in a more generic fashion.

bardzusny’s picture

One point I'd note is that the patch is currently hard coded to assume the Discount module is in a Commerce Kickstart installation:

Oops, that is correct - this is just a raw diff from installation I happened to work with. Feel invited to upload version with generic file paths.

This feels like a rabbit hole... Are there other parts of commerce_discount that will need the same check? Can we do this upstream in commerce instead, maybe with an alter hook that commerce_multicurrency can implement to adjust the order currency?

I'm not very enthusiastic about this solution myself, to be honest. Multi-currency support in Commerce core seems very limited; this patch is just a mere workaround. It's not up to me to decide whether it's good enough of a solution to get included in the main branch - at the very least, you're not the only one having doubts. :)

millionleaves’s picture

Follow up to my comment in #4.

I have a site with 5 currencies. NZD is the default.

I've created a Discount with a Coupon that will discount an order by NZ$40 if the total order value is $118 or higher.

If I add a product worth NZD$199 to the cart and enter the coupon, it applies successfully and discounts the order correctly.

If I change the currency to GBP, the order value changes to £84 and the voucher no longer works. It appears to be applying the discount based on the absolute number value of the order rather than converting the qualifying rule (>NZD118) to the equivalent GBP before evaluating whether the order qualifies for the discount.

I've got the patch applied, but the issue persists.

It looks like I can go and edit the rule manually to create OR conditions and supply a minimum value for each currency, but it would be tidier if the module handled the currency conversion of the order value on the fly - or allowed the conditions to be set within the Discount settings rather than hacking the rule after the fact.

japerry’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new6.94 KB

Here is a patch with a slightly different approach.

We make the commerce module have a context aware default currency, which helps set the currency for a discount. We add a currency row for the offer type, which helps us get a default currency for the offer.

There is also an alter to the view to fetch the currency dynamically, instead of from the entity label cache, which can only really bet set globally.

No interdiff because this really is different from what the original patch was looking to do.

Status: Needs review » Needs work

The last submitted patch, 8: 2345311-commerce_discount_multicurrency-8.patch, failed testing.

bohemier’s picture

@japerry: I tried your patch but I am not getting a currency selector when adding a discount... Should I expect one?

joelpittet’s picture

Still think this issue belongs on commerce. I'm not getting a clear picture of what scenario we need to manually change currencies. The discount should use the order or line item's currency, shouldn't it not?

czigor’s picture

@joelpittet We might not need to change the currency we just need to set it once on discount creation (admin/commerce/discounts/add). Our use case is separate discounts for each currency. Do you think that's possible without patching commerce_discount?

czigor’s picture

Status: Needs work » Needs review
StatusFileSize
new978 bytes

I have given this a look and found that this issue is just about field widget of the commerce_fixed_amount field on the commerce_discount_usage entity fixed_amount bundle. The attached patch changes the widget from 'commerce_price_simple' to 'commerce_price_full'.

czigor’s picture

(To make it clear: the patch changes the fixed default currency value and adds a currency dropdown on admin/commerce/discounts/add and discount edit pages.)

Status: Needs review » Needs work

The last submitted patch, 13: commerce_discount-2345311-13-currency.patch, failed testing.

czigor’s picture

Status: Needs work » Needs review
czigor’s picture

Tests pass locally, this might be just a bot issue.

The last submitted patch, 2: commerce_discount_multicurrency.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: commerce_discount-2345311-13-currency.patch, failed testing.

The last submitted patch, 13: commerce_discount-2345311-13-currency.patch, failed testing.

joelpittet’s picture

Our use case is separate discounts for each currency.

That sounds like extra work, pardon my ignorance but couldn't you set discount conditions on currency for when to apply the amount change. Why does the discount amount need a currency?

bohemier’s picture

What I have experimented is that the discount is considered to be in the store default currency (USD in my case). So when a customer buys in Euros, the USD amount is converted to the EUR equivalent. This makes it impossible to have for instance 10 off for 100 to 199 value in cart, independent of currency. It works for USD but for EUR it would give something like 8.96.

Also, I couldn't set the amount with rules, as it will pick the amount from the discount configuration.

bohemier’s picture

Thanks for the patch czigor... Multicurrency discounts are now possible!
I think the patch should also include a currency for the condition ('Apply to') amount in the discount config... (although this can be changed by editing the rule, it's not obvious)

czigor’s picture

@joelpittet The discount condition on currency is still needed I guess, i.e. the discount currency has no functionality out-of-the-box, we need to set up one with rules. I just think it's not a very good UX to display always the default currency for the discount when in fact it's something else.

czigor’s picture

For the record, I could not find an inline condition for the order currency so I ended up implementing hook_commerce_coupon_condition_outcome_alter() to check if the discount has the same currency as the order. Note, that this would have been *very* tedious without a currency information on the discount: I would have to maintain a list of discounts for each currency. So I really think this patch has a valid use case.

The last submitted patch, 13: commerce_discount-2345311-13-currency.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new528 bytes
new1001 bytes

Attempt to resolve "D7 CI error" - using array() instead of [].

Status: Needs review » Needs work

The last submitted patch, 27: commerce_discount-2345311-27-currency.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new528 bytes
new983 bytes
jofitz’s picture

Also allow selection of currency when setting Order Discount Conditions (e.g. "Total amount" condition).

mirom’s picture

StatusFileSize
new3.31 KB

I tested patch in #30 and it works nice. The only problem we realised during testing is that coupon is applied also for orders with different currency. Yes, this can be validated through custom rule, but if we expect non-drupal people to create coupons, this needs to be automatic. In our patch we validate this during compatibility check.

Status: Needs review » Needs work

The last submitted patch, 31: multi_currency_support-2345311-31.patch, failed testing.

joelpittet’s picture

May be unrelated but I had this added to commerce to help with default currency changing
#2415237: Change site's default currency through a 'commerce_default_currency' alter hook

Some of you may find this useful

emileacroweb’s picture

A useful comment at https://www.drupal.org/node/1869722#comment-11861978 fixes a bug where a % discount seems to force the order total into the default site currency.

czigor’s picture

Status: Needs work » Needs review
StatusFileSize
new3.13 KB

Reroll of #31.

czigor’s picture

Fixing tests.

czigor’s picture

Rerolling patch to apply to latest dev.

Status: Needs review » Needs work

The last submitted patch, 37: commerce_discount-2345311-36-currency.patch, failed testing. View results

czigor’s picture

Status: Needs work » Needs review
StatusFileSize
new3.31 KB

Uploaded the same patch, this is the correct one.

bramdriesen’s picture

Re-rolled the patch because there was a conflict. Any updates on this matter? :)

Status: Needs review » Needs work

The last submitted patch, 40: commerce_discount-2345311-40-currency.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bramdriesen’s picture

Status: Needs work » Needs review
StatusFileSize
new3.65 KB
new502 bytes

Okay... applied the codesniffer remark. Patch and interdiff attached.

Status: Needs review » Needs work

The last submitted patch, 42: commerce_discount-2345311-42-currency.patch, failed testing. View results

bramdriesen’s picture

I don't really understand why the tests are failing as I just did a re-roll of the previous patch.

Anyway I did some testing and discovered that you can apply a $ discount coupon to a € price. Meaning there is no validation if the currency of the coupon matches the price of the product/order. This is something I think should get added!

berenddeboer’s picture

Status: Needs work » Reviewed & tested by the community

Patch worked fine here. I think this can be released. There currently is no validation anyway of applying a discount in default currency against any other currency, so that issue is not relevan.t

dinakaran.ilango’s picture

I have Applied the patch provided in #42. It has provided me the currency drop down while adding the discount, But when I try to set the fixed amount discount for the Currency Other Then the default currency the coupon applies but the price doesn't change(it actually increases the discount amount with the unit price and removes the same amount from the updated unit price) am i missing something? is there some thing i need to check on the pricing rules?

dinakaran.ilango’s picture

Status: Reviewed & tested by the community » Active

I found that the issue is with the custom code that i wrote, not with the patch. So changing the status back

dinakaran.ilango’s picture

Status: Active » Reviewed & tested by the community
bramdriesen’s picture

Any idea when this could be put in the next release? :)

mnakov’s picture

I've made a little addition to change the "@currency off" label to "Currency off". It was misleading, because the user will always see the default currency.

mnakov’s picture

StatusFileSize
new658 bytes

Adding interdiff file between #42 and #50.

mnakov’s picture

Adding interdiff file between #42 and latest.

grahl’s picture

Found another issue in this, currencies don't get their decimals assigned correctly:

-    '#default_value' => commerce_currency_amount_to_decimal($settings['total']['amount'], $default_currency['code']),
+    '#default_value' => commerce_currency_amount_to_decimal($settings['total']['amount'], $settings['total']['currency_code'] ?: $default_currency['code']),

Sorry, don't have the time to update the patch at the moment.