Conditional actions not allowing Adjustment SKU selection
thill_ - June 23, 2009 - 19:36
| Project: | Ubercart |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Island Usurper |
| Status: | closed |
| Issue tags: | CA, Release blocker, ubercamp sprint |
Description
In this wonderful code below we are not returning adjusted skus.
Perhaps this is a feature request vs a bug, but allowing actions on all skus seems important.
function uc_order_condition_has_products_form($form_state, $settings = array('required' => 0, 'forbidden' => 0)) {
$form['required'] = array('#type' => 'radios',
'#title' => t('Require selected products'),
'#options' => array(
0 => t('Order has any of these products.'),
1 => t('Order has all of these products.'),
),
'#default_value' => $settings['required'],
);
$form['forbidden'] = array('#type' => 'radios',
'#title' => t('Forbid other products'),
'#options' => array(
0 => t('Order may have other products.'),
1 => t('Order has only these products.'),
),
'#default_value' => $settings['forbidden'],
);
$options = array();
$result = db_query("SELECT nid, model FROM {uc_products}");
while ($product = db_fetch_object($result)) {
$options[$product->nid] = $product->model;
}
$form['products'] = array('#type' => 'select',
'#title' => t('Products'),
'#options' => $options,
'#default_value' => $settings['products'],
'#multiple' => TRUE,
);
return $form;
}
#1
#2
#3
This patch allows for the SKU to be specified instead of the nid when using the "Check an order's products" condition. The only downside to this is that someone who previously had a product with 100 different skus for one product would have previously been able to select just the nid but will now need to select all 100 skus in their condition.
#4
Hmm. There is a function for us to use to find all of the possible SKUs that a product has. This can kind of future-proof this condition a little bit. But it looks not nearly as efficient as just reading the adjustments table directly.
What do you guys think?
#5
Lyle, I think your method is worth the overhead. I didn't like doing a module_exists for uc_attribute and querying directly since any other contrib module could work the same way in adjusting and creating new SKUs, but was unaware of uc_product_get_models().
#6
The drupal_map_assoc() isn't necessary, because product_get_models() does it for you. Also, the 'Any' model should be masked out.
#7
Ah, yeah. Good call.
#8
Updated the patch to use += instead of array_merge(). Also changed uc_product_get_models() to a more suitable form by adding the $add_blank parameter.
#9
I don't know that we need to allow the "Any" option to be any string that is passed in. Either way, t() isn't supposed to be used on variables, just string literals. Patch reflects this change.
This condition that we're working on is really similar to the one that checks the number of a particular product in an order. Are they similar enough that we ought to combine them? I could write an update function to change people's predicates that use one to the use the other.
#10
The ability for any caller to select the string placed at the top of the list is important for reusability. Other modules (outside of core) may want to put something different there. Core really should allow the default string to be modified with all lists affected. This is a small step towards a solid API.
No argument regarding the t() call.
#11
Forgot to change to needs work.
#12
Allows for the blank value to be passed in, but uses t() only on literal strings.
#13
#14
I was looking over this and was just about ready to commit it when I realized it needed an upgrade path. Everybody's predicates that use this condition would suddenly stop working, or be pointing at the wrong products, when their settings have nids, but the callback expects model numbers.
This update was a bear to write because conditions are stored in a tree structure, and that inevitably requires some kind of recursive walk through the array. The helper function that is added will probably be abstracted out whenever we need to fix another condition's settings, but that will certainly wait until later. In other words, let's not do anything else like this for a while. ;)
#15
Somehow, I left out the changes to uc_product.module in that last patch. Here it is again, along with a fix so that the form field will actually display all of the SKUs.
#16
And it tests out OK. This isn't something that will scale very well, but it'll do well enough for now. Committed.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.