[Vtigercrm-developers] long standing bug in discount calculations

[Hamono, Chris (DPC)]

I should have said I am using vtiger 6.0 This issue does not seem to apply to 6.1

I have also noticed the latest version 6.1 goes to great lengths to use vtlib_purify which is excellent :)

Chris


From: vtigercrm-developers-bounces at lists.vtigercrm.com [mailto:vtigercrm-developers-bounces at lists.vtigercrm.com] On Behalf Of Hamono, Chris (DPC)
Sent: Friday, 23 January 2015 9:58 AM
To: vtigercrm-developers at lists.vtigercrm.com
Subject: [Vtigercrm-developers] long standing bug in discount calculations

Bug: Changing discount type on an invoice does not remove the discount

Steps to reproduce

Create an invoice and add a line item
Save the invoice
Edit the invoice
Create a discount for the invoice of 50% (at the invoice level not the line item level)
Save the invoice
Open the invoice
Change the discount to 'Zero Discount'
Invoice displays correct amount
Save the invoice

The invoice now has the percentage reduction still enforced and the discount amount is wrong

Solution
Line 670 in "include\utils\InventoryUtils.php"

Change

if($_REQUEST['discount_type_final'] == 'percentage')
        {
                $updatequery .= " discount_percent=?,";
                array_push($updateparams, $_REQUEST['discount_percentage_final']);
        }
        elseif($_REQUEST['discount_type_final'] == 'amount')
        {
                $discount_amount_final = $_REQUEST['discount_amount_final'];
                $updatequery .= " discount_amount=?,";
                array_push($updateparams, $discount_amount_final);
        }

To this

    switch ((isset($_REQUEST['discount_type_final']) ? $_REQUEST['discount_type_final'] : false)) {
        case 'percentage' :
            $updatequery .= " discount_percent=?,";
            array_push($updateparams, (float) $_REQUEST['discount_percentage_final']);
            break;
        case 'amount' :
            $updatequery .= " discount_percent=0, discount_amount=?,";
            array_push($updateparams, (float) $_REQUEST['discount_amount_final']);
            break;
        default :
            $updatequery .= " discount_percent=0, discount_amount=0,";
    }


In this small piece of code we see a few issues. These are some of the issues that dog vtiger.

The original code references the $_REQUEST object directly.
The original code does not test for the variables existence before trying to use it which is bad form as well as a potential source of unexplained errors.
The original code uses '==' as a comparison operator, It should always use '===' unless there is a good reason not to.

For those that are not aware...

$cat = 'cat';

0 == $cat <= TRUE
0 === $cat <= FALSE

== does a type conversion before evaluating the result, (int)'cat' is 0.
=== and !== compare the type as well as the value, therefore string 'cat' is not equal to integer 0.

This can cause many hard to find bugs

Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.vtigercrm.com/pipermail/vtigercrm-developers/attachments/20150123/9d89f944/attachment-0001.html>