We have an issue where every single coupon entity gets loaded in checkout:payment.
When there's thousands, checkout fails.

To reproduce: create 12000 one-time coupons using commerce_coupon_batch and then try to check out, with or without using a code.

Comments

Priority:Normal» Critical

Priority:Critical» Normal

Let's keep it in an addecuate priority, not every site uses thousands of coupons.

I can reproduce this by creating 15 thousand coupons in my site, order edit or view are not affected, just the checkout phase, not only payment step which is normally a redirect, but review as well.

I was able to reproduce the problem but not anymore, I can now browse all the site and go through checkout normally after a first error... It'd be important to have a consistent scenario to reproduce this.

Coupons are loaded on commerce_coupon_commerce_price_component_type_info() - a hook_commerce_price_component_type_info() implementation.
The error is a php script timeout so it's normal it occurs only at first time since the query results are then cached.
I think it's a design problem. I'm not very familiar with commerce_price module or even commerce_coupon but why does it loads all coupons as price components? Is it really necessary?

Thanks @marcofernandes, I've cleared cache and I kind of can reproduce the error in the review step, even if it doesn't crash, it takes a loong time for my dev box,

That decision was on design, to be able to separate the coupons and know the price in every case. Commerce Discount is the future for this module and it takes a different approach, I'd happy to include this way of working in the 1.x branch of coupons, but the 2.x will work like that by default.
The approach of Commerce Discounts is to use the component discount and add there an array with data relating with the discount type.

As I said, patches are welcome for this.

I've made a simple patch to workaround this problem. It just adds an option on commerce coupon settings to disable the use of coupons as price components. If it's active then coupons won't be loaded as price components.

Version:7.x-1.0-beta6» 7.x-1.x-dev
Status:Active» Needs work

I don't think this is the best way to do this, adding a setting like this will disable the get of the coupons but it will still create them and use them.
The option here would be to implement the component info in the data array of some other component or to use a different strategy for displaying the price in checkout.

Version:7.x-1.x-dev» 7.x-1.0-beta6
StatusFileSize
new2.02 KB

@pcambra: I'm sure this is not the best way to deal with this issue. Of course if I had more time (to dig deeper in commerce/commerce coupon internals) I would fix it properly. Anyway, here goes another patch. This one disables price components by coupon type.

Status:Needs work» Needs review
StatusFileSize
new1.12 KB

I have taken a different approach to this problem.

I don't think it makes sense to have the default behavior be that a separate price component should be created for each coupon that's ever going to be created. In few cases maybe it could work, but for the most part, doing that is just a big performance overhead, since you need to load all of the coupons and you end up creating a massive coupon price component list.

The module itself doesn't even use the price components anyways (but they are used in the modules for fix and percentage coupons). For starters I think we can change the code in commerce coupon module, and since the other modules doesn't have a release yet, I don't think this change should be a problem.

Those that wish to recreate it can do so pretty easily anyways.

StatusFileSize
new1.12 KB

The first patch had a small bug which is fixed in this patch.

@googletorp: I totally agree with you that this is a better approach. Having all coupons load at checkout is a performance hit, even with small numbers. It does not seem like a big modification making commerce_coupon_pct & commerce_coupon_fixed_value work eiher.

I was under pressure and the patch on #9 saved my skin. I had to upload ~90,000 coupon codes which would have taken the checkout process to WSOD lala land. This patch works.
All coupon codes also get loaded at /admin/commerce/orders/(order#) and this patch fixes that too.
Thank you all

Status:Needs review» Reviewed & tested by the community

Should this be RTBC then?

Without a corresponding fix in pct and fixed discounts modules I don't think this should be RTBC :(

Plus, we'd need a update on database for existing coupons, probably just fixed and pct, but if someone has created their own relying on the price components, it's going to be a pain...

Comment from #8:

The module itself doesn't even use the price components anyways (but they are used in the modules for fix and percentage coupons). For starters I think we can change the code in commerce coupon module, and since the other modules doesn't have a release yet, I don't think this change should be a problem.

Version:7.x-1.0-beta6» 7.x-1.x-dev
Status:Reviewed & tested by the community» Needs review

Status:Needs review» Needs work

Here's an extension of the #8 patch. commerce_coupon_get_properties() for 'price_componenet_name' has to mirror the component key in commerce_coupon_commerce_price_component_type_info().

I added a hook_commerce_price_formatted_components_alter() to alter the component label of coupons so that it will work for old and new coupons.

For new coupons using the coupon_type component type, the display will look like: http://drupal.org/files/new_coupon_components.png

For old coupons that were using the old component type, the display will look like: http://drupal.org/files/old_coupon_component.png

No db updates were necessary. If an order containing an old coupon has to be edited, they need to remove and re-add the coupon.

I'll +1 the need for this; I have a client moving into using coupons and in our case, it's not particularly important they be displayed in an itemized fashion, anyway, since we're limiting customers to only one coupon.

Speaking of other modules that may be affected by this, see #1980776: Price component option in rule config is ignored where I just posted a patch having to do with commerce_coupon_pct's kinda price component handling.

There's also an assumption in the various commerce_coupon modules that the coupon will be necessarily stored on its "own" component; if you store it elsewhere the core functionality is OK but you get zero values on the coupon summary view's "Granted Amount" column, for instance.

Status:Needs work» Needs review

#16 looks very interesting, I had missed that, setting to CNR

#16 is working for me on a live site with 4,000+ coupons. No performance issues during checkout.

Status:Needs review» Needs work

Almost there thanks a lot for the patch and tests to everyone, some things to solve:

+++ commerce_coupon.module (working copy)
@@ -68,8 +68,7 @@
-      $coupon_machine_name = commerce_coupon_machine_name_code($coupon);
-      return $coupon->type . '_' . $coupon_machine_name;
+      return 'commerce_coupon_' . $coupon->type;

Does this part need to be changed?

+++ commerce_coupon.module (working copy)
@@ -1115,3 +1111,36 @@
+ * ¶

Extra space

+++ commerce_coupon.module (working copy)
@@ -1115,3 +1111,36 @@
+          $component_item['title'] .= '<ul><li>'.  implode('</li><li>', $coupons[$coupon_type]) . '</li></ul>';

Let's expose this as a theming function so it can be easily overriden

+++ commerce_coupon.module (working copy)
@@ -1115,3 +1111,36 @@
+          // Display ¶

Display...?

Here's an updated patch.

This part needs to be changed so that the price component name gets saved in the price_component_field in the coupon entity:

+++ commerce_coupon.module (working copy)
@@ -68,8 +68,7 @@
-      $coupon_machine_name = commerce_coupon_machine_name_code($coupon);
-      return $coupon->type . '_' . $coupon_machine_name;
+      return 'commerce_coupon_' . $coupon->type;

Status:Needs work» Needs review

changing to needs review

Status:Needs review» Needs work

Thanks for keeping the ball rolling @ivanbueno :)

This part needs to be changed so that the price component name gets saved in the price_component_field in the coupon entity:

Ok, got it, the only thing I'm still worried about is the "commerce_coupon_" part that then needs a preg_match, couldn't we separate the thing with a pipe | and use list() + explode() afterwards? What if someone somewhere calls its coupon type "commerce_coupon_whatever"?

One minor thing left:

+++ b/commerce_coupon.module
@@ -1119,3 +1113,58 @@ function commerce_coupon_action_get_coupons_for_order($commerce_order) {
+          $old_coupon_component = explode('_', $component_type);
+          $component_item['title'] .= 'Coupon: ' . end($old_coupon_component);

It's better to use list() here and name the variables of the component.

Status:Needs work» Needs review
StatusFileSize
new4.01 KB

Here's a patch. The price component name has been changed to "commerce_coupon|".

I'm hesitant to use list() on the previous component name convention, "{$coupon->type}_{$coupon_machine_name}" to grab the coupon_machine_name. Someone might have named their coupon type as commerce_coupon_store_credit vs [commerce_coupon_fixed|commerce_coupon_pct]. The only sure way of grabbing the machine name from the component is to use end().

+++ b/commerce_coupon.module
@@ -1119,3 +1113,58 @@ function commerce_coupon_action_get_coupons_for_order($commerce_order) {
+          $old_coupon_component = explode('_', $component_type);
+          $component_item['title'] .= 'Coupon: ' . end($old_coupon_component);

I can confirm that this helps a lot if you have 20k coupons.

We use coupons in a special way and don't have a lot of rules conditions or similar, so I can't comment whether that works but the patch does seem to work fine for us.

@ivanbueno does patch #24 have any bug? if not why not changing issue status to "reviewed and tested by the community"?