Allow chart dropdown labels to be translatable.

[braders]

Overview

Allow chart dropdown labels to be translatable.

Before

Prior to this change, the strings 'Tabular', 'Bar Chart' and 'Pie Chart' were hardcoded. This affects a handful of reports, which support bar and pie chart views:

After

It is possible for the strings to be translated, or for word replacement to be performed against the strings.

Technical Details

The labels were previously hardcoded within the $_charts array property against each report. However, in PHP properties cannot be directly assigned from a function call, so this would not have worked:

protected $_charts = [
  '' => ts('Tabular'),
  'barChart' => ts('Bar Chart'),
  'pieChart' => ts('Pie Chart'),
];

As a result, I have re-worked the $_charts property to only contain information about the types of charts supported, not the labels. The labels are added in the new getChartLabels method.

Every report supports a tabular display, and this is the default value. Therefore, this is no longer defined in the $_charts property, but instead gets added by default in getChartLabels().

[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).

[demeritcowboy]

ts() for Tabular?

It might be more "standard" to do the assignment of _charts in each subclass's constructor, and then you don't need this function, but this seems ok too.

[braders]

Oh yeah, extra ts added. I was so focused on then other two that I missed this one.

This approach seemed like it had the advantage that the strings couldn't get out of sync between each report - I have seen a few instances where repeated strings don't match in terms of spelling, capitalisation etc. But I'm not overly fussed either way.

[braders]

Hm, I have noticed that at least one third-party extension has instroduced their own keys, and other extensions are using the structure that CiviCRM core is using at the moment:

https://github.com/SyedShahiq/mainpeople/blob/master/sites/default/civicrm/extensions/net.ourpowerbase.report.advancedfundraising/CRM/Advancedfundraising/Form/Report/Contribute/Recovery.php

https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/blob/master/CRM/Extendedreport/Form/Report/Contribute/Overview.php

I could extend getChartLabels to continue to support the old array strucutre, but might be that moving the labels to the constuctor of each report is cleaner whilst still retaining backwards compatiability. I'll have a think about the best way forward.

[braders]

Based on this thread I've reworked the code so that the translations are defined in the constructor of each report.

[seamuslee001]

@braders your passing through $this->_charts as a function param but the function in L1594 doesn't accept parameters

[eileenmcnaughton]

test fails were new-year related - test this please

[demeritcowboy]

Looks good. Ran a couple. Good call on support for extensions still using the property.

Interestingly I can't see how to make the dropdown appear for the soft-credit report. The trigger doesn't seem to be the _charts variable on its own but some combination of that and chartSupported and/or a field having 'chart' => TRUE. But doesn't change anything about the PR.

[braders]

Looks good. Ran a couple. Good call on support for extensions still using the property.

Interestingly I can't see how to make the dropdown appear for the soft-credit report. The trigger doesn't seem to be the _charts variable on its own but some combination of that and chartSupported and/or a field having 'chart' => TRUE. But doesn't change anything about the PR.

Hm, good spot. It appears that each of the charts with _charts also have a $this->assign('chartSupported', TRUE);. This isn't called by the soft credit report. I tried adding chartSupported to soft-credit, but charts didn't actually appear. So I think soft-credit report doesn't actually support charts, and probably shouldn't be setting the _charts array.