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:

<?php
$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:

<?php
$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
StatusFileSize
new1.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

StatusFileSize
new8.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.