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;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Issue tags: +ubercamp sprint
cYu’s picture

Assigned: Unassigned » cYu
cYu’s picture

Status: Active » Needs review
FileSize
1.6 KB

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.

Island Usurper’s picture

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?

cYu’s picture

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().

cha0s’s picture

Status: Needs review » Needs work

The drupal_map_assoc() isn't necessary, because product_get_models() does it for you. Also, the 'Any' model should be masked out.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Ah, yeah. Good call.

sammys’s picture

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.

Island Usurper’s picture

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.

sammys’s picture

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.

sammys’s picture

Status: Needs review » Needs work

Forgot to change to needs work.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
2.43 KB

Allows for the blank value to be passed in, but uses t() only on literal strings.

rszrama’s picture

Issue tags: +Release blocker
Island Usurper’s picture

Assigned: cYu » Island Usurper
FileSize
3.22 KB

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. ;)

Island Usurper’s picture

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.

Island Usurper’s picture

Status: Needs review » Fixed

And it tests out OK. This isn't something that will scale very well, but it'll do well enough for now. Committed.

Status: Fixed » Closed (fixed)
Issue tags: -Release blocker, -CA, -ubercamp sprint

Automatically closed -- issue fixed for 2 weeks with no activity.