Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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;
}
Comment | File | Size | Author |
---|---|---|---|
#15 | 500134_model_condition.patch | 5.47 KB | Island Usurper |
#14 | 500134_model_condition.patch | 3.22 KB | Island Usurper |
#12 | 500134_model_condition.patch | 2.43 KB | Island Usurper |
#9 | 500134_model_condition.patch | 2.76 KB | Island Usurper |
#8 | 500134_200907111714-0400.patch | 2.67 KB | sammys |
Comments
Comment #1
rszrama CreditAttribution: rszrama commentedComment #2
cYu CreditAttribution: cYu commentedComment #3
cYu CreditAttribution: cYu commentedThis 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.
Comment #4
Island Usurper CreditAttribution: Island Usurper commentedHmm. 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?
Comment #5
cYu CreditAttribution: cYu commentedLyle, 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().
Comment #6
cha0s CreditAttribution: cha0s commentedThe drupal_map_assoc() isn't necessary, because product_get_models() does it for you. Also, the 'Any' model should be masked out.
Comment #7
Island Usurper CreditAttribution: Island Usurper commentedAh, yeah. Good call.
Comment #8
sammys CreditAttribution: sammys commentedUpdated 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.
Comment #9
Island Usurper CreditAttribution: Island Usurper commentedI 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.
Comment #10
sammys CreditAttribution: sammys commentedThe 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.
Comment #11
sammys CreditAttribution: sammys commentedForgot to change to needs work.
Comment #12
Island Usurper CreditAttribution: Island Usurper commentedAllows for the blank value to be passed in, but uses t() only on literal strings.
Comment #13
rszrama CreditAttribution: rszrama commentedComment #14
Island Usurper CreditAttribution: Island Usurper commentedI 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. ;)
Comment #15
Island Usurper CreditAttribution: Island Usurper commentedSomehow, 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.
Comment #16
Island Usurper CreditAttribution: Island Usurper commentedAnd it tests out OK. This isn't something that will scale very well, but it'll do well enough for now. Committed.