CRM-20555: Incorrect balance amount is shown on participant view page if 'Deferred Revenue' is enabled

[monishdeb]

• CRM-20037: Change Contribution tab to show balance due

---

• CRM-20555: Incorrect balance amount is shown on participant view page if 'Deferred Revenue' is enabled

[monishdeb]

Here's the screenshot that shows the new column Balance Due and removed Premium under Contribution Tab

NOTE: The Balance due column is not sortable as there is a limitation in doing that. Reason why the column title is a bit faded

[pradpnayak]

Tested PR, result in attached SS

[pradpnayak]

However when i enable 'Deferred Revenue' and Purchase a General Membership with completed Contribution status i get

Balance amount seems to be wrong

[eileenmcnaughton]

We'd better get some stats on this on large searches on large sites before merging - we did discuss this on another ticket - ie. the concerns about imposing possibly-unconformant change - I'll try to find the ticket

[seamuslee001]

@eileenmcnaughton I think that might have been the soft credits one?

[JoeMurray]

@eileenmcnaughton @colemanw and I just looked through this function and it does two calls to the db which don't look bad. This .tpl is used on contributions tab and also on search results for Search: Find Contribution. But in the latter it's only going to be perhaps 100 or so records. So we're not too concerned after code review.

[eileenmcnaughton]

So it's a max 300 extra queries per search (if all are participant), or 100 viewing a contribution tab on a donor with 30 or donations?

[monishdeb]

On further research, if we MUST add this improvement i.e. add Balance Due column on Contribution list, then there is no other easy alternative other than using CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType(..) to calculate the balance due amount.

[eileenmcnaughton]

OK - it would actually be easy to do in an extension - ie. add an api in the extension & just add a region in the tpl & call crmAPI from there

[JoeMurray]

So @eileenmcnaughton do you have site to test this on, or are you proposing that JMA create a test site and populate it. Personally I don't think this is an issue. Let's see what the total number of queries per search results page is for 25 results with and without the change, and the differential page load time on a server with reasonable resources, and then discuss with @colemanw ? Recall as well that we are on a deadline on this and we had agreement to move ahead.

[eileenmcnaughton]

Hi - if you have agreed to go ahead with @colemanw then I guess he'll have to honour that - please do it as a setting that is opt-out by default and then no-one else will be affected.

If you DO impose this on all sites then you effectively transfer the work onto me of making it not run extra queries before I can upgrade.

[monishdeb]

For #10204 (comment)

@pradpnayak on closer look CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType(...) doesn't return proper balance due amount if 'Deferred Revenue' is enabled because it is not handled in that BAO function. There's another place where this BAO function is used and that is participant's 'Fee Section' page where it show the same incorrect 'Balance'. So I think we need to raise a separate ticket about adding support for Deferred Revenue in CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType(...) and that will solve the issue in both the places.

@JoeMurray should I create a separate ticket for that?

[monishdeb]

@pradpnayak I have fixed the issue where it shows the incorrect balance amount if 'Deferred Revenue' is enabled (also added unit test) and this is the patch 52d2e89

This has also fixed the issue on Fee Section on Participant's view page. Here's the difference:

1. Before the fix (check the balance and total paid amount)
   

2. After the fix
   

[monishdeb]

@JoeMurray @eileenmcnaughton I have made necessary changes in this PR to add Balance Due column using searchColumns hook. There is a slight change in the position of this new column, which is now placed next to Actions column due to limitation caused by underlying code structure. Here's the screenshot:

And as per the core changes here's my hook definition

function hook_civicrm_searchColumns($contextName, &$columnHeaders, &$rows, $form) {
  if ($contextName == 'contribution') {
    $columnHeaders['hookHeaders'] = array(
      'balance_due' => array(
        'name' => ts('Balance Due'),
      ),
    );

    foreach ($rows as $key => $row) {
      $balanceDue = CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType(
        $row['contribution_id'],
        'contribution',
        FALSE,
        $row['total_amount']
      );
      $rows[$key]['balance_due'] = sprintf("<b>%s</b>", CRM_Utils_Money::format($balanceDue));
    }
  }
}

(this new index hookHeaders should be documented at https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_searchColumns/)

[eileenmcnaughton]

#10295 proposes an alternative approach to the tpl

[monishdeb]

@eileenmcnaughton I have reverted my changes in tpl and php files after merging #10295 Also please check my comment

[eileenmcnaughton]

@monishdeb this seems OK except - it's not fixing the thing the ticket is about anymore, it's fixing something else related to deferred revenue. Can you log a separate ticket that describes what this is fixing & update the PR /commits to refer to it.

[eileenmcnaughton]

also - you've updated the docs for how to do the hook?

[monishdeb]

@eileenmcnaughton I have created a new ticket and updated the title of the PR. Also submitted a minor fix #10334

[eileenmcnaughton]

Getting the JIRA admin sorted was the final issue on this PR - adding merge on pass

[monishdeb]

Jenkin, test this please