Hey,

If I choose the condition "Check an order's products" I get a list of SKUs (including SKU adjustments).

If I choose the condition "Check an order's number of products" I get a list of only the base SKUs, with no adjustments.

The first one, uc_order_condition_has_products_form() uses this code:

$options = array();
  $result = db_query("SELECT nid FROM {uc_products}");
  while ($product = db_fetch_object($result)) {
    $options += uc_product_get_models(node_load($product->nid), FALSE);
  }

While uc_order_condition_count_products_form() uses:

$options = array('all' => t('- All products -'));
  $result = db_query("SELECT nid, model FROM {uc_products} ORDER BY model");
  while ($product = db_fetch_object($result)) {
    $options[$product->nid] = $product->model;
  }

We should use the first method at all times, as it allows us greater flexibility in choosing SKU and SKU adjustments. I believe the fix is to just replace the first method with the second.

Files: 
CommentFileSizeAuthor
#6 rules_product_options-809344-6.patch8.35 KBDanZ
PASSED: [[SimpleTest]]: [MySQL] 2,796 pass(es). View
#4 rules_product_options-809344-4.patch1.34 KBDanZ
PASSED: [[SimpleTest]]: [MySQL] 2,800 pass(es). View

Comments

torgosPizza’s picture

Sorry, that last line should read "replace the second method with the first" as the first method is the one that provides us with more model numbers.

torgosPizza’s picture

I just ran into some memory exhaustion errors when using this method in both "order has products / order has number of products" so I think the underlying function uc_product_get_models() needs to be refactored somehow. Will investigate more ASAP.

TR’s picture

Title: Conditional Actions uses inconsistent methods for getting model numbers. » Rules uses inconsistent methods for getting model numbers.
Version: 6.x-2.2 » 7.x-3.x-dev

This behavior is unchanged in Ubercart 7.x-3.x using Rules, so let's address it there first.

Note uc_product_get_models() was refactored in #853072: uc_product_get_models() causes memory exhaustion with large # of SKUs to solve the memory exhaustion errors.

DanZ’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 2,800 pass(es). View

Like this?

This patch causes the conditions uc_order_condition_products_weight, uc_order_condition_count_products, and uc_order_condition_has_products to use the better listing technique to generate options, and removes the old listing technique.

longwave’s picture

Status: Active » Needs work

Looks like we still need the 'all' case for some callers of uc_order_condition_products_options()

DanZ’s picture

FileSize
8.35 KB
PASSED: [[SimpleTest]]: [MySQL] 2,796 pass(es). View

Hmm, good point. I put the "all" back.

The comments for these functions were very wrong (left over from D6 conditional actions, I'm guessing), so I fixed them. But then, the remaining wrong comments bothered me, too, so I updated almost all of them.

longwave’s picture

Status: Needs review » Needs work

"Check an order's products" expects a list of SKUs, so it can handle product attribute combinations if needed. "Check an order's number of products" and "Check an order's total weight" expect a list of nids, so they cannot handle product variants - and I don't think we can easily change this now without breaking existing uses of these conditions.

longwave’s picture

Status: Needs work » Closed (works as designed)

Looking at this again I don't think we can change this now, and although it's inconsistent it sort-of makes sense to me - checking for specific products checks for SKUs, while counting products or weight only cares about the base product. It would be nice if this were more flexible, but that should be solved in #1907748: Evaluate conditions on lists of items (products, packages, etc.) instead.

Problue Solutions’s picture

Issue summary: View changes

Do comments #7 and #8 mean that the patch in #6 just doesn't work at all?

I would be happy if the base product SKU applied to all variants in these circumstances, but it doesn't - so I cannot check for a quantity of variant SKUs on an order which is a big problem.

The SKU must match the variant SKU, matching the base SKU does not work.

Problue Solutions’s picture

Status: Closed (works as designed) » Active
TR’s picture

Status: Active » Closed (works as designed)

As longwave said, we're not going to change the signature of those functions at this point because it would break existing uses of those functions - that's not something we do in minor point releases.

You can always write/use your own function if you have a custom need - the code in the patch should be a good starting point for doing that, but of course you will have to test to see if it does exactly what you want. longwave also gave a pointer to an issue where it shows how you can evaluate conditions on a list of items as an alternative.