Clean up hook_nodeapi

Thanks for the patch, tszcheetah.

This is something I've done a little work on and have changed it in a version of uc_discounts I'm running on a live site. I agree with what you've done but it seems the patch is against a previous version of uc_discounts. Could you reroll against the latest DRUPAL-5?

It seems I had changed this function and not committed it. CVS reported that uc_discounts.module needed patching so I reverted and found this patch is actually against the latest DRUPAL-5. Sorry for the confusion.

I've used the patch from the original post and come up with this:

<?php
function uc_discounts_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  if ($op == 'view') {
    $display_discount = variable_get('uc_discounts_product_discount_price', 0);
    $product_types = uc_product_node_info();
    if (isset($product_types[$node->type]) && $display_discount) {
      $discount_price = uc_discounts_product_discount_price($node);
      $teaser = ($a3) ? TRUE : FALSE;
      $node->content['display_price']['#value'] = theme('uc_discounts_display_price', $discount_price);
      $node->content['sell_price']['#value'] = theme('uc_discounts_sell_price', $node->sell_price, $discount_price, $teaser);
    }
  }
}

function theme_uc_discounts_display_price($price) {
  $output = '<div class="display_price">';
  $output .= uc_currency_format($price);
  $output .= '</div>';
  return $output;
}

function theme_uc_discounts_sell_price($price, $discount_price, $teaser) {
  if ($discount_price != $price && variable_get('uc_discounts_product_full_price', 0)) {
    if ($teaser) {
      $output = '<span class="sell_price">';
      $output .= t('Price: !discount (discounted from !price)', array('!discount' => uc_currency_format($discount_price), '!price' => uc_currency_format($price))); 
      $output .= '</span>';
    }
    else {
      $output = '<div class="sell_price">';
      $output .= t('Price: !discount (discounted from !price)', array('!discount' => uc_currency_format($discount_price), '!price' => uc_currency_format($price))); 
      $output .= '</div>';
    }
  }
  else {
    $output = theme('uc_product_sell_price', $discount_price, $teaser);
  }
  return $output;
}
?>

The reason I did it like this was because the existing functions theme_uc_product_sell_price() and theme_uc_product_display_price() are like so:

<?php
function theme_uc_product_sell_price($price, $teaser) {
  if ($teaser) {
    $output = '<span class="sell_price">';
    $output .= uc_currency_format($price);
    $output .= '</span>';
  }
  else {
    $output = '<div class="sell_price">';
    $output .= t('Price: !price', array('!price' => uc_currency_format($price)));
    $output .= '</div>';
  }
  return $output;
}

function theme_uc_product_display_price($price) {
  $output = '<div class="display_price">';
  $output .= uc_currency_format($price);
  $output .= '</div>';
  return $output;
}
?>

Does anyone have any comments on this? This is certainly an improvement on what it was before but I'm still not happy with it. As a developer I just want sell price and discounted price in the $node object. So $node->sell_price would be the original price and $node->discount_price would be the discount price.

How do we add $node->discount_price? I tried but was unsuccessful.

Hi tszcheetah, thanks again for commenting. I agree with you about using the original theme function, so will go with that now until we find a need to have the uc_discounts_display_price one.

I'll commit this as it is since it does improve the situation and we can always make further changes later.

I'll see if I can add that $node->discount_price now.

I've made a few improvements to this now:

<?php
function uc_discounts_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
  $product_types = uc_product_node_info();
  if (isset($product_types[$node->type])) {
    switch ($op) {
      case 'load':
        $node->discount_price = uc_currency_format(uc_discounts_product_discount_price($node), FALSE);
        break;
      case 'view':
        $discount_price = uc_discounts_product_discount_price($node);
        if (variable_get('uc_discounts_product_discount_price', 0)) {
          $node->content['display_price']['#value'] = theme('uc_product_display_price', $discount_price);
          $node->content['sell_price']['#value'] = theme('uc_discounts_sell_price', $node->sell_price, $discount_price, $a3);
        }
        break;
    }
  }
}

function theme_uc_discounts_sell_price($price, $discount_price, $teaser) {
  if ($discount_price != $price && variable_get('uc_discounts_product_full_price', 0)) {
    $discount_price = uc_currency_format($discount_price);
    $price = uc_currency_format($price);
    if ($teaser) {
      $output = '<span class="sell_price">';
      $output .= t('!discount (discounted from !price)', array('!discount' => $discount_price, '!price' => $price));
      $output .= '</span>';
    }
    else {
      $output = '<div class="sell_price">';
      $output .= t('Price: !discount (discounted from !price)', array('!discount' => $discount_price, '!price' => $price));
      $output .= '</div>';
    }
  }
  else {
    $output = theme('uc_product_sell_price', $discount_price, $teaser);
  }
  return $output;
}
?>

The main differences are:

• Added $node->discount_price to the node object (formatted the same as $node->sell_price).
• Change to using theme_uc_product_display_price() for display_price.
• Removed "Price: " from the teaser sell_price in line with theme_uc_product_sell_price().

I'll commit this although it's still open for debate if anyone has any suggestions.

We're mixing SPAN and DIV, but this is to match current UC output.
Filed a bug on UC here: http://drupal.org/node/325106
When that gets fixed we can follow to match.

http://drupal.org/node/325106 has been fixed, so we should make that change to the SPAN.

Nodeapi is looking for a valid node->type before applying any discount actions to the node.

Right now, it's calling uc_product_node_info() to get a list of valid types. However, this function doesn't include custom node modules which implement hook_product_types to declare the node as a product.

Instead of:

  $product_types = uc_product_node_info();
  if (isset($product_types[$node->type])) {

You could use:

  $product_types = module_invoke_all('product_types');
  if (in_array($node->type, $product_types)) {

Can you think of any reason not to go with this approach?