Fix for #1573: Extra columns for Contribution Detail report.

[twomice]

Overview

This creates new columns for the report: Employer, Location Type, and Preferred Communication Method

Before

These columns are not available in the report.

After

The columns are available.

Technical Details

No changes are made to the (admittedly complex) existing logic of this report. We're just adding new information about entities that are already clearly defined in the report.

Comments

None.

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

[eileenmcnaughton]

@twomice don't we have a helper function that adds these fields in other reports?

[twomice]

@eileenmcnaughton Do we? Happy to use that, but where would I find it?

[eileenmcnaughton]

check
class CRM_Report_Form_Contact_Summary - although I do see a deprecation notice & a recommendation to use getColumns

[twomice]

Thanks @eileenmcnaughton

1. We could use CRM_Report_Form::getColumns('Contact'), which would have the effect adding a bunch more columns, but would get us 'Current Employer' for free.

2. Same for `getColumns('Address') and 'Location Type'.

3. I don't see a helper to add 'preferred_communication_method', though.

So, do you prefer that I use the approach in "1" and "2" above?

[eileenmcnaughton]

In general we have been moving that way - it doesn't really make sense to have some fields available on some reports & not others

[twomice]

@eileenmcnaughton Okay, I'll give that a go.

[twomice]

@eileenmcnaughton Thanks for the help. Turns out 'employer' already exists in the report, 'location type' is easy with getColumns('Address'), and 'communication method' is the only one we really had to had code for.

However, I've got a bunch of whitespace-only changes in the PR now. Do you have a recommendation for improving that?

[eileenmcnaughton]

@twomice so a reviewer can look at it with https://github.com/civicrm/civicrm-core/pull/16458/files?w=1 to exclude those.

Any reason not to add communication_preferences to the helper fn - it seems if it's useful here it would be useful on other reports too

[twomice]

@eileenmcnaughton Good call on that helper function. That's done now. FYI, I intentionally set 'false' -for is_filter, is_order_by and is_group_by on the 'communication method' field, because those functions don't work 100% perfectly on this array-packed multi-value column. I'd like to leave those for someone to address later if they have need.

Further suggestions?

[eileenmcnaughton]

Code all looks good now

[twomice]

Darn, got some failing tests. Fixing ...

[twomice]

@eileenmcnaughton There's one failing test but it appears to be unrelated. Would you agree, and is there anything otherwise that needs doing before merge?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

agree it looks unrelated

[eileenmcnaughton]

I gave this a spin & the new fields show & render correctly. I didn't spot any issues - merging

[twomice]

Thanks, @eileenmcnaughton !