CRM-21656: Fix Backend Membership with Priceset Applies Taxes Twice

[mlutfy]

Overview

Steps to reproduce:

• Enable Taxes, Add a tax account with a 10% tax
• Add the tax account to the Membership financial type
• Create a new priceset for memberships, with radio options for the main membership options.
• From a contact record, add a new membership for that contact, select an option from the priceset.

Notice how to total amount is OK, but when we view the contribution, the line
item shows a unit_price with the tax_amount included, so then the total_line
ends up adding the taxes again.

This was caused by a hack for entering backend memberships using the default
dropdown options (not a priceset), which makes it possible to enter an arbitrary
total_amount directly.

(the JIRA issue has screenshots and rainbows)

Comments

I tested creating a new membership using a priceset, as well as without a priceset, and with entering an arbitrary total_amount.

---

• CRM-21656: Backend Membership with Priceset applies taxes twice to line_item

[mlutfy]

odd.. example fatal error, from test-ubu1204-5:

Jan 13 00:01:29  [info] $Fatal Error Details = Array
(
    [callback] => Array
        (
            [0] => CRM_Core_Error
            [1] => exceptionHandler
        )

    [code] => -1
    [message] => DB Error: unknown error
    [mode] => 16
    [debug_info] => INSERT INTO civicrm_sms_provider (domain_id ) VALUES ( 1 )  [nativecode=1364 ** Field 'api_type' doesn't have a default value]
    [type] => DB_Error
    [user_info] => INSERT INTO civicrm_sms_provider (domain_id ) VALUES ( 1 )  [nativecode=1364 ** Field 'api_type' doesn't have a default value]
    [to_string] => [db_error: message="DB Error: unknown error" code=-1 mode=callback callback=CRM_Core_Error::exceptionHandler prefix="" info="INSERT INTO civicrm_sms_provider (domain_id ) VALUES ( 1 )  [nativecode=1364 ** Field 'api_type' doesn't have a default value]"]
)

other error:

Jan 13 00:01:28  [info] $Fatal Error Details = Array
(
    [callback] => Array
        (
            [0] => CRM_Core_Error
            [1] => exceptionHandler
        )

    [code] => -1
    [message] => DB Error: unknown error
    [mode] => 16
    [debug_info] => INSERT INTO civicrm_participant_status_type () VALUES ()  [nativecode=1364 ** Field 'weight' doesn't have a default value]
    [type] => DB_Error
    [user_info] => INSERT INTO civicrm_participant_status_type () VALUES ()  [nativecode=1364 ** Field 'weight' doesn't have a default value]
    [to_string] => [db_error: message="DB Error: unknown error" code=-1 mode=callback callback=CRM_Core_Error::exceptionHandler prefix="" info="INSERT INTO civicrm_participant_status_type () VALUES ()  [nativecode=1364 ** Field 'weight' doesn't have a default value]"]
)

[mlutfy]

jenkins, test this please

[mlutfy]

This might be duplicating #11461 ? (in description, but not in code)

[eileenmcnaughton]

I'm very wary of this as a fix - the $isQuick flag is really a display setting that has been mis-used in many other ways - I regard it's use outside the code layer as a code smell.

More of an issue is that we have been pretty strict on requiring tests for any processing of financial stuff - there are submit unit tests for all forms that use moment

@monishdeb @pradpnayak FYI

[monishdeb]

I agree with Eileen that to fix this we shouldn't rely on isQuickConfig to be used as a flag in here. Also, this patch is causing fatal error on backoffice contribution submit, which uses Contribution price-set with radio price field. The error is

#1 /Users/monish/src/civicrm/CRM/Core/DAO.php(1117): CRM_Core_Error::fatal()
#2 /Users/monish/src/civicrm/CRM/Price/BAO/PriceSet.php(704): CRM_Core_DAO::getFieldValue("CRM_Price_DAO_PriceSet", NULL, "is_quick_config")

Caused by NULL $priceSetID which is used to fetch is_quick_config setting.

As per my investigation I found that this issue is affecting ONLY Membership price-set using radio price field. So this must be something to do with some buggy code responsible to format line-item information on backoffice membership submit. I will send a PR soon with UT.

[eileenmcnaughton]

Thanks @monishdeb

[monishdeb]

Closing in favor of #11521 Please have a look at this patch