Hello, hunziker! I haven't had a chance to use the module myself yet, but I have a couple of developers who have been digging into working with Commerce Coupon to create some custom types of discounts for a client. From what they've been telling me, it sounds like we might need a bit of an architectural rewrite to support proper price calculation for at least line item specific discounts. It sounds like you've already begun to implement the workflow I'm about to describe in a submodule, but maybe some of that code needs to become part of the base module.

So, real briefly, let me describe how coupons can work in the context of the core pricing system and interact with shopping carts:

  1. We all know coupons come in various types. Some affect the order total, some affect particular product prices, some discount shipping, and so on. What I want to propose is that the Commerce Coupon module should solely be concerned with the following items:
    • Defining coupon codes (including bulk generation of codes that all point to the same base coupon)
    • Validating coupon code user input
    • Providing a way to associate these coupon codes with orders
    • Integrate with Rules via a condition that lets you see if a coupon has been applied to an order
    • Create line items only for coupons that arbitrarily affect the order total or transcend particular line items
    • Create price components that can be used for line item based discounts without worrying about the actual application of the discount
  2. The actual application of a coupon to the line items of an order or via the creation of a coupon line item for an order needs to be fluid until the order completes checkout. As long as the cart is being refreshed on load, the coupons should be getting recalculated. This happens as long as the order is in a status whose property array has 'cart' => TRUE.
  3. This is also where the Rules condition comes into play. The coupon module should primarily be concerned with determining what coupons there are and whether or not they're assigned to orders. We already have a product pricing system, and that same system should be used when possible for things like particular line item discounts or percentage discounts. I can create a product pricing rule that has a condition "Order has coupon" and an action to discount the line item by 10%. The product pricing rule can specify what price component to use, meaning the discount will be applied so that it shows up in the order total area as coming from the particular coupon. It's important that these calculations happen through the product pricing system so you get the free refresh and also so coupon application can be reordered with respect to other types of discounts and taxes. We always had problems with coupons and VAT in Ubercart precisely because the two couldn't interact through a single system.
  4. Now, that doesn't do anything for arbitrary order discounts (e.g. a $10 off coupon). You can't reasonably handle these at a line item level, so you still need a way to add line items to the order for them. The line item would have a negative unit price, a quantity of 1, and would use the coupon price component. As long as it uses the coupon's price component, it will show up in the order totals section whether you actually alter the View to list out the coupon line items or not (or include them in the checkout pane as you are now).
  5. These line items still need to be refreshed during the cart order refresh process. Whether you use Rules or not for these is up for debate, but at the very least they should be re-validated / calculated during the order refresh and updated if they've changed somehow. For example, the coupon might be for $10 off before a certain date. If the user is shopping with that coupon applied to their order and the date passes, the coupon is no longer valid and the line item would need to be deleted. So, you should recalculate the coupon and delete or update the coupon line item as necessary.
  6. This also means you should keep track of which coupons are applied to an order separately. You shouldn't just track which coupons have been applied through price component or line item comparison. There needs to be either a separate coupon reference field or (at a minimum) a part of the order data array where you keep track of which coupons have been applied to an order. This would support using as many coupons as you want on an order, but you can have UI logic that determines whether coupons can be combined or not.

I understand this is a huge wall of text, but please feel free to ask questions for clarification or let me know if you need help. The main idea here is that we're respecting the current shopping cart refresh process and product pricing system while still acknowledging that the line item approach will be necessary for certain types of coupons.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hunziker’s picture

Hi Ryan,

First of all thank you for your feedback. I know that there are some problems inside of the module. I start rewriting the core behavior. I test multiple ways to handle coupons in Drupal Commerce. We can categories most of the coupons as percentage or fixed amounts. The percentage one will be applied to all (or subset) of line items, whereas the fixed amount is applied to the order.

The fixed amount should be implemented in any case as a line item, because you don't know exactly which line items should be reduced and how much. Since we know these things for percentage amounts, the first approach was to add a price component to reduced the line items. The problem here is that we can't reduce the tax for example or at least it becomes very messy. If you think here different, then checkout an old version and try it out. You will agree me then.

The percentage case is in the latest dev implemented as a separate line item. This gives us the flexibility to handle also more complex cases and we have always the same "output" (namely the line item) independent of the implementation (fixed or percentage). See this issue #1245676: Coupons with percent amounts are not handled.

The recalculation is also now done every time the order is stored. What is missing here is the check, that we are in the checkout process.

I track every redemption of a coupon in a separate table which coupon is applied to which order. This is since the first release. Currently there are some open issues on how we can provide a better interface.

I think most of your "wishes" / requirements are implemented already. It would be nice if you can take a deeper look into it and give an exact feedback what is not ok for the current implementation, because I don't see currently what I have done wrong.

rszrama’s picture

"The problem here is that we can't reduce the tax for example or at least it becomes very messy."

That's true, the price manipulation actions do not do anything with existing price components. However, if the price adjustment determined by a coupon happens through the same event as taxes ("Calculating the sell price of a product"), then you could reorder your rules so the coupon gets applied before the tax is calculated. Then you have the calculations working as expected. Doesn't this address what you're describing? I don't think the problem was that you were applying the discount at the line item level; it's that you weren't doing it in the same scope as other discounts / taxes where items could be reordered as necessary.

"The recalculation is also now done every time the order is stored. What is missing here is the check, that we are in the checkout process."

This is part of the reason I wrote the initial issue. I was advising based on developer feedback, and they mentioned you were recalculating on the order presave. This is dangerous reasons beyond the one you mentioned being that you don't know when or where this order is being saved. However, if you have these calculations going through the same process as all other product price adjustments, you don't have these problem. They get recalculated on the shopping cart refresh. That's the existing place where everything of this nature should be calculated, and there's a separate hook you'd use for full order based refresh where you can recalculate line item based adjustments to the order.

Again, I think it's incorrect to not handle line item based discounts through the price calculation system. We really need coupon based adjustments to be coming in through the calculation system so they can be reordered relative to other rules. Not having this is what always prevented us from properly handling VAT and coupon discounts in Ubercart.

As I summarized in the initial post, the most important thing you can do for Coupon is respect the current shopping cart refresh process and product pricing system while still acknowledging that the line item approach will be necessary for certain types of coupons.

hunziker’s picture

"That's true, the price manipulation actions do not do anything with existing price components. However, if the price adjustment determined by a coupon happens through the same event as taxes ("Calculating the sell price of a product"), then you could reorder your rules so the coupon gets applied before the tax is calculated. Then you have the calculations working as expected. Doesn't this address what you're describing? I don't think the problem was that you were applying the discount at the line item level; it's that you weren't doing it in the same scope as other discounts / taxes where items could be reordered as necessary."

This is not usable in practice because you need to show the customer exactly how much you have reduced the item including the tax reduction. Another problem you have with price components is, that you can not show how much you have reduce the order by each coupon. This is not clear for the customer. These problems and the much better architecture with the line item based approach are the reasons, why I will not change the behavior back to price component based implementation for percentage amounts. You got the same flexibility with for price calculation this way. I see here no downsides of this approach, because VAT rules etc. can be applied on coupon line items as good as one price components, if not even better.

"[...] They get recalculated on the shopping cart refresh. That's the existing place where everything of this nature should be calculated, and there's a separate hook you'd use for full order based refresh where you can recalculate line item based adjustments to the order."
I understand this. Which hook should I use for this?

rszrama’s picture

"This is not usable in practice because you need to show the customer exactly how much you have reduced the item including the tax reduction. Another problem you have with price components is, that you can not show how much you have reduce the order by each coupon. This is not clear for the customer."

If you use a custom price component for each coupon, it will itemize the information just fine. It's the same principle as with taxes - each tax has a unique price component, so when you add them all together through the line items and order total field, they display itemized to the end user. This should do exactly what you're trying to accomplish with line items.

Additionally, even if you're using line items, your line item prices should be using unique price components per coupon. There shouldn't technically be any reason to ever display the coupon line items. If you want an example of this, refer to the shipping module. Even though it adds shipping charges to an order via a custom line item, it never actually displays the line item. Instead it's using a custom price component type per shipping method so that shipping charges show up in the order total price field displaying w/ the price components.

On a side note, I'm not sure why you think a coupon amount needs to have tax included in it. Practically speaking, the discount either adjusts the pre-tax amount or post-tax amount (depending on tax jurisdiction and nature of the coupon), but the value of the coupon itself wouldn't have any tax related to it. The way you're going sounds like an accounting problem waiting to happen. Again, this is about where and how coupons fit into the price calculation process, and there's absolutely nothing that should separate them from any other discount you're adding to a product via Rules. Separating them out will cause problems, as has been proven with Ubercart's coupon module in the past.

The bigger reason still exists - you must be able to apply coupon discounts in the same scope as other product line item price adjustments to these things can be sorted related to one another. I suppose you could do the same for line item calculations, but it still needs to be coming through the same event.

As for the hook to use, it's hook_commerce_cart_order_refresh(). See the function commerce_cart_order_refresh() for more context.

AdamGerthel’s picture

Subscribing

bojanz’s picture

Okay, so I've taken some time to understand this issue, and I agree with rszrama.
His idea is cleaner, and makes it possible to avoid some pitfalls of the current code.

I've struggled with percentage coupons today.
Had to add an additional rule so that it adds a tax component that it gets taken out of VAT (VAT should be on subtotal - coupon).
If it worked on the line item level like proposed, by changing the rule weight in the Rules UI I could decide whether I want it to go before or after tax.
I believe Ryan already listed the actual and important reasons for making this a part of the price calculation process and not doing it on presave, so I won't repeat myself.
Since he's the person who worked on similar solutions in Ubercart, as well as the one who wrote the pricing system (and Commerce itself) in the first place, he's a better authority on it than me.

olragon’s picture

Subscribing

raystuart’s picture

+1 for price components for each coupon, as well as hook_commerce_cart_order_refresh()

recidive’s picture

I agree with what Ryan says here, if there are APIs that were developed with this in mind we should be using them. I see the pricing calculation thing being also useful for URL passed tokens when you need the prices to be displayed discounted. E.g. it's common for pricing comparison sites to pass a URL with a coupon, say for 5% discount, and the user see all prices in the site discounted. This also can be fancier done using HTTP referer. Also common for by-email discounts. This may need to be implemented as separately module though.

For using the appropriate cart refresh hook, I've filed this: #1291770: Use hook_commerce_cart_order_refresh() instead of hook_entity_presave().

webmasterkai’s picture

Priority: Major » Critical

I'll be working on this over the next week. Marking as a critical task so it's completed before the 1.0 release.

rszrama’s picture

Wonderful news, Kai. Let me know what me and my team can do to help!

hunziker’s picture

Since webmasterkai does some implementation on this module, I will try to give you some intuitions about the problems we are faced with the coupon module.

Some cases you need to get managed with the chosen architecture. Since the fixed amount coupon is not that hard, we focus here on percentage coupons:

Taxes

If a coupon is applied to a line item, the taxes need to be reduced.

Two coupons applied to one order

If two percentage coupons are applied to an order, then each line item must be reduced twice. (The input for the second reduction is the output of the first reduction. This is important to recognize. )

Coupons changed during the checkout process

Coupons can be changed during the checkout process. So the coupons need to be recalculated every time an order is updated. The price recalculation is not enough. Think of the following situation:
You have coupon that is bound to a shipping country. The customer change the shipping country. There is no guarantee , that the price is then recalculated.

After payment, the coupon amount must be immutable

When the order is finished (payment is done) the coupon amount should be never changed, with exception the administrator change it.

Show the coupon amount for each coupon

The customer needs to see the amount of each coupon applied to the order.

Implications of the above constrains / cases

You need to recalculate the amount when the order is changed. The price recalculation hook is not enough (see "Coupons changed during the checkout process"). Perhaps, you can assume that each change of the order implies always a price recalculation, but for that you need to study the whole code of commerce and for each release that is done, you need to recheck this.

You can not use the price component, because you need to calculate each coupon independent (see "Two coupons applied to one order"). My solution to this problem was to put the amount in a new line item. Ryan and others think this breaks the architecture of commerce. They prefer the solution with price components. But this implies that you need for each coupon (not coupon type) a new price component. Don't try this, because you need to load all coupons into memory to pass the coupons over to the price components hook. This can consume much of your memory. Think about a website with thousands of coupons...

You need to track coupon redemptions. This can be done in the separate table (coupon_logs), as I done partially.

I hope, I can clarify why the implementation is done in this way.

@Ryan: I know you design the whole price component thing for the coupon module, but it has some critical drawbacks as mentioned some time go in another post in spring 2011. One of the major problem is, the fact that it is not in a normal form (http://en.wikipedia.org/wiki/Relational_database#Normalization). If it would be in a normal form in the database, you could get managed the multiple coupon case, because you can write the coupon into the database and you do not need to load it into memory.
There are other drawbacks: For example you are not able to build the order total block with views. But we do not need to discus about the actual implementation of the price component, we should try to get a solution that can manage all occurring cases.

ASupinski’s picture

Subscribing

fearlsgroove’s picture

Status: Active » Needs review
FileSize
11.51 KB

Here's a first pass at implementing percentage coupons as a line item component. It's still kind of rough.

  • Adds a new action to commerce_coupon to provide a list of coupons for an order
  • Drops the entity_presave altogether
  • Changes the rule action provided in commerce coupon basic to apply the discount as a price component
  • Adds a new default rule to basic on calculate sell price of an item that will iterate over coupons attached to the line item order and apply percentage coupons by invoking the percentage action
  • Takes suprising little code -- in fact mostly removes code -- and adds LOTS of good new possibilities and flexibility, not to mention allowing proper calculation of sales tax.

It doesn't touch the fixed price coupon at all. It will also surely need some cleanup -- the default rule is just a copy and paste from rule export instead of the proper rules api.

fearlsgroove’s picture

Last patch had a typo in the default rule declaration

fearlsgroove’s picture

Make coupon price components inclusive

webmasterkai’s picture

jearlsgroove, thanks for helping on this!

I'll have time for review and commit later today. Thanks again.

hunziker’s picture

@fearlsgroove: Do you tested if two percentage coupons can be applied to one order?

AdamGerthel’s picture

Will there be a need for an upgrade path? After reading through everything it sounds as the changes are rather comprehensive

rszrama’s picture

Agreed on checking for an upgrade path. No need to commit it right away - I'll try to get a review in as well.

fearlsgroove’s picture

No upgrade path yet but there should be one before it goes in. Attached patch fixes percentage recalculating on commerce_total instead of unit price.

@hunziker: I did just now, and it works but compounds the percentages, which is probably not the desired effect. Maybe Ryan or someone could provide a hint at the right way to get at the original base price? Iterating through the unit price components?

fearlsgroove’s picture

Here's a better attempt. It allows you to configure the component against which the percentage coupon will apply. This fixes the issue where the percentage coupons compound each other. It also lets you do something like create a 100% shipping coupon theoretically.

It also adds commerce_round and a config option for that.

fago’s picture

Status: Needs review » Needs work

I gave the patch a test with the latest commerce dev version. I tried making use of a 50% coupon. When using the coupon I got this error:

Recoverable fatal error: Object of class CommerceCouponType could not be converted to string in commerce_coupon_basic_apply_percentage_coupon_to_item_line() (line 83 of ..../modules/commerce_coupon/modules/basic/commerce_coupon_basic.rules.inc).

I fixed that by using $coupon->type->type->value()

Then though, the coupon cannot be applied. The rule gives the following errors:

0 ms Rule Apply percentage coupons to line item fires.
0.239 ms Evaluating the action commerce_coupon_action_get_coupons_for_order. [edit]
0.707 ms Added the provided variable order_coupons of type list [edit]
0.807 ms Looping over the list items of order-coupons [edit]
1.12 ms Evaluating the action commerce_coupon_basic_apply_to_item_line. [edit]
6.176 ms Unable to get a data value. Error: Unknown data property type.
6.788 ms Unable to evaluate action commerce_coupon_basic_apply_to_item_line. [edit]
6.88 ms Rule Apply percentage coupons to line item has fired.

When editing the action commerce_coupon_basic_apply_to_item_line, the options list for the "Price component type" parameter is broken.

fearlsgroove’s picture

Here's another shot. $coupon->type->raw() seems to have resolved the issue mentioned.

fearlsgroove’s picture

Status: Needs work » Needs review

nr

fago’s picture

Status: Needs review » Needs work
+++ b/modules/basic/commerce_coupon_basic.rules.inc
@@ -55,38 +68,23 @@ function commerce_coupon_basic_apply_percentage_coupon_to_item_line($line_item,
+      'commerce_coupon_' . $coupon->type->raw(),

Why raw() and not value()? Raw doesn't make much sense for the type.

+++ b/modules/basic/commerce_coupon_basic.rules.inc
@@ -55,38 +68,23 @@ function commerce_coupon_basic_apply_percentage_coupon_to_item_line($line_item,
+      true

should be upper case.

fago’s picture

Also, I gave it a test and noted the checkout listing doesn't fit yet.
I tried using a 50% coupon:

1. The total of each product line item is already reduced by 50%, but shouldn't. I should see regular prices and get the coupon value reduced at last.
2. subtotal, coupon, vat look good to me, though it would make more sense to me if the coupon would be listed as last item.

fearlsgroove’s picture

Status: Needs work » Needs review
FileSize
14.52 KB

Thanks for the review. Attached patch fixes the capitals issue, and also uses a the proper rules api to create the default coupon rule.

Why raw() and not value()? Raw doesn't make much sense for the type.

Both $coupon->type->value() and $coupon->type->type->value() return a coupon type entity. This might be a separate bug, but the raw string value of type is exactly what we want here, so I don't see any harm.

1. The total of each product line item is already reduced by 50%, but shouldn't. I should see regular prices and get the coupon value reduced at last.

Agreed but this is a limitation of the price formatter. I'm using http://drupal.org/project/commerce_price_components to work around this for now.

2. subtotal, coupon, vat look good to me, though it would make more sense to me if the coupon would be listed as last item.

Agreed again, but as far as I can tell these are ordered by the order in which they are added and there's no way to change them. I could be wrong; didn't spend a lot of time on it.

fearlsgroove’s picture

I'm thinking about how to deal with fixed amount coupons as well and leaning towards making the coupons apply against item prices. The reason is US style sales taxes. Sales taxes must be calculated against line items/products, since some products are taxable while others aren't. If the coupon is implemented as a line item, sales taxes get calculated against an undiscounted price. This results in an overpayment.

Instead I'm thinking of changing the fixed price method to apply against a line items. I would change it to work much like the percentage coupon, except it would evaluate in a loop, keeping track of how much of the discount has been applied and reducing items in order.

Any thoughts?

rszrama’s picture

Hmm, I'm not sure. My hunch is it would be overly complex to try and divide / maintain the flat discount across multiple product line items w/ various quantities. What if instead your coupon line item also had the tax applied to it so it subtracted $10 worth of tax?

fearlsgroove’s picture

What if instead your coupon line item also had the tax applied to it so it subtracted $10 worth of tax?

That might work but I'm not sure it would in a fairly significant number of edge cases. For example, suppose someone buys two items, one is taxable, but the other is not. If the non-taxable item were more expensive, you'd end up undercharging sales tax.

An approach that applies the coupon amount to each individual line item also allows you to selectively allow the coupon based on attributes of the product. So it allows you to do $10 Widget xyz, rather than $10 off any order total. I'm already using this capability with the percentage coupon.

ASupinski’s picture

It does seem like there is a need for two types of coupons, line item coupons and order coupons. The only way I could see making coupons work in all scenarios when the coupon is a separate line item would require:
1. using rules to keep the coupons as the highest delta in attachment to the order (so they are considered last in calculating the order total)
2. having a complicated price calculation for the coupon that could consider product applicability and tax.
3. having some sort of coupon weight to control the order of coupon application in the case of multiple coupons being used.

Even given the above it seems like the coupons with product applicability, percentages and or tax would be end up being a lot more confusing to an end user than adding components to the individual line items for the coupon affect.

Just my two cents.

fago’s picture

Both $coupon->type->value() and $coupon->type->type->value() return a coupon type entity. This might be a separate bug, but the raw string value of type is exactly what we want here, so I don't see any harm.

I don't think $coupon->type->type->value() really returns an entity, as it would be a really major bug + $coupon->type->type->value() worked for me.

Agreed but this is a limitation of the price formatter. I'm using http://drupal.org/project/commerce_price_components to work around this for now.

I'm not sure. For me it's not natural that the coupon changes the line item prices. I'd expect a 10$ off coupon to not change any price, but just add a new line item that shows me I get 10$ off.

Instead I'm thinking of changing the fixed price method to apply against a line items. I would change it to work much like the percentage coupon, except it would evaluate in a loop, keeping track of how much of the discount has been applied and reducing items in order.

I think usually coupons are applied to the whole order, thus being substracted at the end. At least that's the way it is common on the European side of the sea. However, with the solution outlined in the issue (#0) by rszrama, I think flexible per line-items discounts should be perfectly doable as well. Just add the discount during price calculation depending on the order coupons.

Thus, I'd suggest going for coupons being attached to orders and result in new line items for percentage coupons as wells as for fixed amount coupons + doing the condition for allow flexible per line-item coupons. That makes the average use-case simple, while still keeping the possibility to implement the advanced stuff.

To solve the sales tax problem and taxes in general, I guess we should get order total and apply the tax to the discounted value to the same proportions as to the order total. Then, the finally applied tax rate should be fine.

@current-patch:
I noted, it runs on the event commerce_product_calculate_sell_price, which is a per product event, but then applies coupons to all products of the order. Doesn't that result in multiple applications of the coupon if there are multiple products?

fearlsgroove’s picture

using the entity -dev from 12/1,
$coupon->type->type->value() : unitialized/suppressed entity metadata wrapper exception
$coupon->type->value()->type : gives the type string as expected
$coupon->type->raw() : also gives the type string. Why is type->value()->type better? You're the entity expert so I'll use line two if that's what you recommend.

I think usually coupons are applied to the whole order, thus being substracted at the end. At least that's the way it is common on the European side of the sea. However, with the solution outlined in the issue (#0) by rszrama, I think flexible per line-items discounts should be perfectly doable as well. Just add the discount during price calculation depending on the order coupons.

Thus, I'd suggest going for coupons being attached to orders and result in new line items for percentage coupons as wells as for fixed amount coupons + doing the condition for allow flexible per line-item coupons. That makes the average use-case simple, while still keeping the possibility to implement the advanced stuff.

At least with US style sales taxes -- and I'd imagine in some if not most VAT systems -- some products are taxable, while some products are not. For this reason, taxes are applied against line items, not against the order total. It's impossible as far as I can tell to use the line item method to properly calculate a discount given those potential variables. They might be DISPLAYED to the user as a line item, but under the hood the discount applies against the applicable product line item(s). That's why I describe it as a limitation of the formatter, not the a limitation of the coupon discount implementation. This is exactly why rfay's made that video about using a VAT hack to simulate discounts. I think something like the price formatter that allows you to select what components are displayed is going to be required at some point in the near future.

This means at least in the US there is no simple use case. Nearly all US retailers must handle sales tax for at least some of their orders (usually in-state), which means if they want to offer coupons at all it will have to account for that issue. Apologies for the US centric focus on my part, but that's the specific issue (client) i'm trying to solve this for.

To solve the sales tax problem and taxes in general, I guess we should get order total and apply the tax to the discounted value to the same proportions as to the order total. Then, the finally applied tax rate should be fine.

As I said above -- taxes are applied against line items, not an order total. There is no order total tax value to discount against as far as I can tell. Maybe Ryan can tell me if I'm off the mark here?

I noted, it runs on the event commerce_product_calculate_sell_price, which is a per product event, but then applies coupons to all products of the order. Doesn't that result in multiple applications of the coupon if there are multiple products?

Not multiple applications of a single coupon. The calculate sell price event starts from scratch every time it's run, and each percentage coupon applies against each product only once.

edit: fix my quotes

fago’s picture

$coupon->type->type->value() : unitialized/suppressed entity metadata wrapper exception

That's strange. What does it say?
It should work just fine if there is an according type. If it doesn't exist though it's going to throw the exception. Else maybe just use $coupon->type->getIdentifier().

$coupon->type->type->value() works for me though, with the latest dev versions.

As I said above -- taxes are applied against line items, not an order total. There is no order total tax value to discount against as far as I can tell. Maybe Ryan can tell me if I'm off the mark here?

Isn't that what we are seeing at the checkout workflow? The order total price having multiple price components, which are summed up out of the line item price components.

Thus, cannot we just create a new line item, reducing the desired amount in the same proportions of the order total? Of course, we'd have to make sure the coupon price is calculated only once (so it doesn't include itself in its calculations).

kiwimind’s picture

How far along is this now? It seems like the patch in #28 still needs some work according to the comments that follow it. If it's ready, then I'm happy to test it and feedback as necessary, but am reluctant to use it if there is a load more to be done.

I was going to go down the route that bojanz took in #6, but it seems like there is going to be a much more elegant way of dealing with this.

Thanks all for your hard work on this.

fearlsgroove’s picture

I've been using the patch in #28 for some time and it's working great for percentage type coupons. It doesn't address issues with fixed price coupons, where there was some disagreement over the best approach. I hope to get back to this soon (another project coming up that will use it -- so that's always a good impetus).

If you're going to use percentage coupons I'd recommend 28.

fearlsgroove’s picture

Ooops double post

kiwimind’s picture

Thanks for the response. I am only intending on using percentage coupons for my current project, so will give the patch a go and let you know how I get on.

kiwimind’s picture

I can't get this patch to apply using git apply. Which version are you running it against? I've tried -beta4 and -dev.

kiwimind’s picture

I hope that this summary of what I'm trying to accomplish helps move this along as I see the below points as being necessary to a commerce solution.

The store in question is UK based, so is applying VAT at the current rate of 20%, which works fine. I have tweaked the layout of the order summary block so that things make more sense in there.

We are charging shipping by weight, so I have multiple flat rates set up, which are being called by Rules as necessary.

The site has already launched, but I'm being pressured a lot to get the discount coupons up and running as the store admins want to do a mail out to their members offering 10% off their order. This is where I'm finding issues.

The commerce_coupon module seems to work fine, although there is no allowance for VAT. It seems that bojanz and hunziker started talking about this over at #1245676: Coupons with percent amounts are not handled but that thread has now deferred to this one.

I was hoping to be able to pick Rules apart and apply the "Apply a percentage Coupon amount" component in to the VAT rule somehow, but I've not been able to figure that out. Is this even the right way to be trying to approach this?

As mentioned above, I can't get the patch to apply (although I haven't tried applying it manually), so am at a bit of a loss as to where to go next. I know that there are many kinds of stores, but I would have thought that a lot of them would want to include products, tax, shipping and discounts.

I would appreciate any thoughts and/or feedback on this so that I can try to help move this forwards.

fearlsgroove’s picture

Here's a reroll of #28. This has been working pretty well for us for percentage coupons. Glad to see some momentum in this module again.

fearlsgroove’s picture

Last patch was missing code from the rules action

kiwimind’s picture

Does this patch address the VAT issues? This is still a deal breaker for me as our client doesn't want to have to pick through the products to redo the pricing to exclude VAT.

Thanks for all the work on this.

fearlsgroove’s picture

Yes it could. This patch along with commerce_price_components gives you much more control over what values are displayed and against what values tax is calculated.

[EDIT]: .. for percentage coupons only, so far. This patch doesn't address fixed price coupons yet.

kiwimind’s picture

The ongoing issue that I've got is that the coupon module does not affect the final VAT price. The VAT is still being applied to the pre-discount rate.

I have seen this issue in other queues, but it seems like there is no currently solution to tackle this, certainly when the prices are added included VAT.

pcambra’s picture

Ok, made a review of all the comments and code around. This code works great but I made some tweaks regarding the rule sorting and price calculation. Also minor code fixes that includes a proper use of $coupon->type->type->value() as we've fixed the type property declaration in another issue.

Attaching both a diff and a interdiff for better tracking.

I think this needs three major things to get committed but we're close:

  • First of all a review by rszrama as he has a clear idea about what the % coupons should be like.
  • I strongly think we need this change in #1372434: refactor "basic coupon" to "fixed coupon" and "variable coupon" prior to committing this one in order to have separate types for pct coupons and fixed ones.
  • We need to settle what's the behavior for fixed amount coupons, I tend to think that it's kind of ok the way it is but probably we could address that in a separate issue.
fearlsgroove’s picture

I like #1372434 as well .. not sure they need to be separate modules per se, but separate coupon types for sure. There's no real reason to conflate them.

AdamGerthel’s picture

@pcambra: I haven't been following all the posts here too closely because I'm not a developer, so forgive me if I'm off in this, but here's my five cents regarding fixed coupons. The current issue with fixed amount coupons is that there are lots of accounting issues related to it. What happens if someone purchases two products for €100 and €20 respectively, uses a coupon for €50 and then wants to return the item worth €100. In the case of percentage discounts this can be handled, but for fixed amounts, it's an issue. Should the discount be split evenly over the products? Should it be on the order total?

lukus’s picture

@AdamGerthel - In my mind, there are two distinct cases for fixed price coupons. In one case the fixed price coupon will be a straight swap for real-world cash, and in the other it will be equivalent to a discount.

I've had a quick think and came up with the following .. not sure how useful it is - or whether it covers everything?

If the coupon is purchased (i.e. as a gift certificate)
I think the value of the coupon needs to be deducted from the order total first.

--

If the customer spends €120 on two products and uses a €50 coupon, they've spent €70.

  • If they then return the product that cost €100; they'd receive the €70 they spent, and possibly a new coupon for €30 / store credit depending on the store policy.
  • If they return the product that cost €20; they'd receive the €20 they spent.

Promotional fixed price coupons
If fixed price coupons are going to be issued for promotional purposes;

Restrictions are going to need to be introduced to prevent abuse of the system (e.g. minimum spend, specific product eligibility).
The total discount will need to be split proportionally according to the items purchased.

--

Once again, if the customer spends €120 on two products and uses a €50 coupon, they've spent €70.

  • They've received approx. 42% off the total cost
  • The discount need to be split proportionally.
  • Product A €20 -> roughly 17% of the order total -> €8.5 discount
  • Product B €100 -> roughly 83% of the order total -> €41.50 discount

And therefore ..

  • If they then return the product that cost €100; they bought the product for €58.50, so they'd receive that amount back.
  • If they return the product that cost €20; they bought the product for €11.50, so they'd receive that amount back.
pcambra’s picture

Just as brainstorming, I think we have a couple more of use cases for 'fixed' coupons:

- First a slight variation of this case where coupons are applied only to specific products. They will be displayed in the bottom but taking out the product will remove the coupon as well. And refunds affect only to that product.
- Second, coupons that reduce other elements that do not affect products or refunds, i.e. free shipping coupons.

I think we could have a "mode" for fixed coupons, pct ones only have one way to work, but we could expose a setting in the coupon type form for fixed coupons that allows the user to choose between "Promotional" and "Certificate" way of work.

pcambra’s picture

Status: Needs review » Needs work

I've committed #47 as it is working for percentage coupons and that would be enough for some people.

I'm also working in splitting the basic coupon into two different things, see #1372434: refactor "basic coupon" to "fixed coupon" and "variable coupon". There's some work to do mostly in the fixed coupons part (see #50) but it's going to be easier to manage those changes in different queues.

No upgrade path needed at the moment, but it will be needed as soon as we change the coupon type.

Let's keep this open for further discussion, review and feedback

pvhee’s picture

Refering to #46, it seems that the VAT is still applied to the price before applying the coupon discount, resulting in a much higher VAT than actually should be. For example, assume the order total is 100EUR, of which 20EUR is VAT. If a 50% off coupon is applied, the total order will be 50EUR, but the VAT will still be 20EUR while 10EUR would be expected.

What would be the correct flow here to calculate the VAT in combination with couponing?

pcambra’s picture

Status: Needs work » Fixed

After committing #1547440: Kill coupon log entity I'd say that all the matters discussed here are fixed now, please open separate issues for specific extra requirements.

Feedback & testing is highly appreciated.

Thanks to everyone involved!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.