dev/core#756 - CRM/Contribute - Fix multi-currency soft credit summary

[pfigel]

Overview

This attempts to fix the multi-currency soft credit bug reported in dev/core#756 and some inconsistencies.

Before

Viewing the contribution tab of a contact with soft credits in multiple currencies causes an exception.

After

Soft credits with multiple currencies don't cause an exception and the soft credit summary is rendered more similar to the contribution summary.

Technical Details

This changes the return value signature of CRM_Contribute_BAO_ContributionSoft::getSoftContributionTotals(). Looks like it's only used by one method which I adapted, but I'm not sure what the approach to backwards-compatibility is for something like that. Could also go with new method + deprecation.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@pfigel My only concern here is using CRM_Utils_Money::format() directly in the BAO as it has a habit of causing issues and formatting is only needed when it's actually displayed to the user. Ideally I think we'd see something like {$amount|crmMoney:$currency} on the smarty template - @eileenmcnaughton thoughts?

[pfigel]

@mattwire definitely agree with the sentiment. I went with this approach because:

1. getSoftContributionTotals already has some code that's more of a template thing, i.e. implode(', ', ...
2. It matches what we do in CRM_Contribute_BAO_Contribution for contribution summaries
3. It allows us to DRY up the template a bit (the soft contribution totals table existed in two templates, with some inconsistencies)

(tl;dr: it sucks, but in a consistent way 😄)

Maybe we can use this fix to deal with the regression, and then later on do a bit of refactoring for the whole contribution summary thing where we move the presentation stuff out of BAO.

[eileenmcnaughton]

I tested this & was able to replicate the bug & see this fixes it - merging

[mattwire]

Maybe we can use this fix to deal with the regression, and then later on do a bit of refactoring for the whole contribution summary thing where we move the presentation stuff out of BAO.

@pfigel Agreed :-)