Discounts need saving two times - FIX ADDED

I can confirm this bug. When you *create* a discount, the value in Discount Amount is not saved. You have to then edit the discount and resave it to get it to stick.

Likely the error is in uc_discounts_form_submit() in line 735, which reads:

if (($index = strpos($form_state["values"]["discount_amount"], "%")) !== FALSE)

(Confirmational evidence: If you create a discount that does contain a % character, it is saved properly, and converted to a decimal).

The first problem with this line is that it is missing a { after the conditional. I have a hunch this is causing the else statement below it to never be executed (and without a parse error)! I would first suggest adding the { at the end of line 735, and if that doesn't work, rewriting the conditional as...

    $discount_amount = $form_state["values"]["discount_amount"];
    if (strpos($discount_amount, "%") === TRUE) {
      $discount_amount = floatval(substr($discount_amount, 0, strpos($discount_amount, "%"))) / 100;
    }
    else $discount_amount = floatval($discount_amount);

The current code will only work if ($index = strpos($form_state["values"]["discount_amount"], "%")) preserves type, and who knows if it is?

I've only been programming in PHP for a few months (I started back in the punch-card days), but when I read the definition of strpos() I puked all over a brand-new copy of "PHP in a Nutshell". One of the most commonly used library functions, and it has a massive land-mine built in.

As a matter of programming safety, I never, ever do assignments inside conditionals. They are hard to read, and have caused billions of dollars of lost programmer and user time due to bugs. The C "=" vs "==" operator confusion is by far the single most damaging design decision in the history of computing.

I have confirmed that adding a { at the end of line 735, plus a } at the end of the function (it's missing!) on line 876 or so seems to correct this bug.

So it's two, two, two typos in one!

In addition to the above fix, the line 782 that says:

$form_state["values"]["discount_amount"],

needs to be changed to:

$discount_amount,

It is correct in the call to uc_discounts_insert, but not in the call to uc_discounts_update. This fix allows you to enter a xx% value when editing an existing discount instead of it just working when creating a new discount for the first time.

Thanks. Could you roll a patch implementing these changes?

patch for #2 and #4 attached

I am not familiar with the rules for PHP code. Could someone who is please check these lines?

#6 patch: Doesn't the "else $expiration = 0" line shown in the patch need braces also?

--- ./uc_discounts.admin.inc.0  2010-03-10 06:50:49.097734944 -0600
+++ ./uc_discounts.admin.inc    2010-03-10 10:00:58.818360041 -0600
@@ -732,10 +732,12 @@ function uc_discounts_form_submit($form,
     else $expiration = 0;

Please check this function for missing braces in the ...else lines.

function uc_discounts_delete($form_state, $discount_id = 0) 

Do these types of one line "while" statements need braces?

  //Create products form element
  $options = array();
  $result = db_query("SELECT n.nid as nid, n.title as title, p.model as model" . " FROM {uc_products} p INNER JOIN {node} n ON p.nid=n.nid ORDER BY title"
  );
  $options[ALL_PRODUCTS] = t("<All Products>");
   while ($row = db_fetch_object($result)) $options[$row->nid] = $row->title ." (". $row->model .")";

Patch in #6 works

committed http://drupal.org/cvs?commit=404592