Ensure the civicrm_financial_item records get created correctly

[omarabuhussein]

Before

For line items with tax amounts, two civicrm_financial_item records should be created, one for the line item total_amount and the other for the tax amount. Currently only one record for the tax amount get created.

After

IF a line item has a tax amount, then now two civicrm_financial_item records get created one for the total_amount and the other for the tax amount.

Example

If for example we configured the webform to create two memberships for contact 1, where one membership has tax on it where the other not, here is how currently it will look like in line items view page :

which is correct.

After submitting the webform, the following two line items will get created :

which is also correct.

But if we look at civicrm_financial_item table then this is the only created records :

as you can see only two civicrm_financial_item records are created, but 3 records should be created instead.

Here is how things looks like after applying this patch :

as u can see, the line items are still the same which is correct, but now 3 civicrm_financial_item records will be created.

Technical Details

This regressions was introucded in this PR : #105 , in this line https://github.com/colemanw/webform_civicrm/pull/105/files#diff-59be2e079a691088e15ab3a80e0640a0L2054

this code :

$result = CRM_Financial_BAO_FinancialItem::add($lineItemRecord, $contributionResult);
if (!is_null($this->tax_rate)) {
  $result = CRM_Financial_BAO_FinancialItem::add($lineItemRecord, $contributionResult, TRUE);
}

was replaced by this one :

$taxTrxnID = !empty($lineItemRecord->tax_amount);
CRM_Financial_BAO_FinancialItem::add($lineItemRecord, $contributionResult, $taxTrxnID);

which means if the line item has a tax amount, true will be passed as a 3rd parameter to CRM_Financial_BAO_FinancialItem::add() method which means only the tax civicrm_financial_item record will be created. I reverted the code to how it was before where there are two calls to CRM_Financial_BAO_FinancialItem::add() method, one to create the civicrm_financial_item record for the total_amount and the other for the tax amount.

[KarinG]

@mattwire - can you please have look?

[mattwire]

Can you add a comment here to explain why we need to call CRM_Financial_BAO_FinancialItem::add twice (I understand now, but I didn't at the time as the code is not really clear on the CiviCRM side).

[mattwire]

@KarinG @omarabuhussein I've tested and confirmed that this fixes the described issue. If we can get a comment added to the changed code explaining why we have to call it twice then it's good to merge.

[KarinG]

Thanks Matt! Omar: if you can add that comment then I’ll merge this

[omarabuhussein]

Thanks @mattwire and @KarinG , I added a comment to explain that and squashed all the commits into one commit.

[KarinG]

Ok @mattwire ?

[mattwire]

@KarinG Yes, approved.

[KarinG]

Thank you all!