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

farald’s picture

Priority:Normal» Critical
pcambra’s picture

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.

marcofernandes’s picture

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?

pcambra’s picture

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.

marcofernandes’s picture

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.

pcambra’s picture

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.

marcofernandes’s picture

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.

googletorp’s picture

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.

googletorp’s picture

StatusFileSize
new1.12 KB

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

farald’s picture

@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.

cruzeazy’s picture

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

torgosPizza’s picture

Status:Needs review» Reviewed & tested by the community

Should this be RTBC then?

pcambra’s picture

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.

pcambra’s picture

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

Status:Needs review» Needs work
ivanbueno’s picture

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.

bradjones1’s picture

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.

pcambra’s picture

Status:Needs work» Needs review

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

ivanbueno’s picture

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

pcambra’s picture

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...?

ivanbueno’s picture

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;
ivanbueno’s picture

Status:Needs work» Needs review

changing to needs review

pcambra’s picture

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.

ivanbueno’s picture

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);
Berdir’s picture

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.

sepehr.sadatifar’s picture

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

mihai_brb’s picture

Priority:Normal» Major
Issue summary:View changes

Other than the problem that coupons load on checkout there is another issue with components.

I am using this module with commerce_coupon_fixed_amount that implements a price field.
commerce_coupon_commerce_price_component_type_info adds every single coupon as a component, therefor entity property info grows like crazy. We have 3 coupon bundles, therefor there are 3 instances of this field. And for every field the components are present in property info.
We are only testing with a few coupons.
Cache table saves very large records for entity_property_info:{lang} and this happens on every page load.
I don't even want to imagine what happens when we go live and the number of generated coupons grow.

When no coupon price components are defined, the page load time decreased with almost 1sec. And we're testing on a dedicated high performance server.

UPDATE I had some time to look into this and the problem is when using commerce_price_components module with commerce coupon.

torgosPizza’s picture

We have 10,000+ one-time coupons (gift cards using Commerce GC) and we have not experienced this issue. Is this issue still valid? Does it have to do with the Discount coupon type only?

torgosPizza’s picture

Upon further inspection this is for Coupon 1.x. I would highly recommend updating to the latest 2.x if at all possible.

pcambra’s picture

I would highly recommend updating to the latest 2.x if at all possible.

Couldn't agree more.