WIP: dev/core#326: WIP: Fix for fatal error and for missing dollar totals …

[twomice]

…in section headers.

Overview

On Contribution detail report when section header is enabled (under Sort tab), you get a fatal error:
Error code : Database Error Code: Unknown column 'civicrm_contribution_total_amount' in 'field list', 1054

Before

Fatal error. Reference this animation from the ticket:

After

Fatal error is gone. Section totals include dollar amounts. Screenshot:

Technical Details

I've got questions about specific parts of this PR, which I'll outline in the comments.

Comments

Thanks to @eileenmcnaughton for getting me started with this.

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

[twomice]

@eileenmcnaughton Obviously this method was created for a reason (in e6bab5ea#diff-585f40aa84b2e15ef6b0768dd64bfd0cR612), and I'm not sure what that was. So it probably can't just be removed like this. But it is having the side effect of causing the civicrm_contribution_total_amount_sum _selectAlias to be renamed to civicrm_contribution_total_amount, which causes the fatal error and (even with the "hack alert" patch at https://lab.civicrm.org/dev/core/issues/326#note_8150) causes dollar totals to be omitted in section headers.

I've tried a few things, related to Soft Credit options, to break the report after making this change, but all seems well in spite of it. Why was this logic added here?

[eileenmcnaughton]

To stop weird grouping on that softcredit I guess. TBH this report is badly designed from the start. In extended reports there is a copy of it & I recently decided to strip out all the soft credit handling due to it's awfulness. The only way I can see to end the whackamole is to add tests on each fix religiously.

My understanding is that without this part we go from totally broken to partially broken so I probably still would merge in chunks - since the main part of this locks in the query not failing & then we need to figure out how to test the headers.

The current action that is tested does support some metadata retrieval

  /**
   * Test getrows on contact summary report.
   */
  public function testReportTemplateGetRowsContactSummary() {
    $description = "Retrieve rows from a report template (optionally providing the instance_id).";
    $result = $this->callAPIAndDocument('report_template', 'getrows', array(
      'report_id' => 'contact/summary',
      'options' => array('metadata' => array('labels', 'title')),
    ), __FUNCTION__, __FILE__, $description, 'Getrows');
    $this->assertEquals('Contact Name', $result['metadata']['labels']['civicrm_contact_sort_name']);

although in extended reports I recently added a getmetadata action thinking that might be a better way

https://github.com/eileenmcnaughton/nz.co.fuzion.extendedreport/commit/443812912ea9e1fca3d3f739b75312f6a96d8839#diff-bfc47780dcc610e8169649bb84456669R257

[eileenmcnaughton]

As an aside -I am fairly sure a test will fail if this change breaks something. Also I think since the sectionTotals() function is already a massive hack more hacking in there is ok

[twomice]

Jenkins test this please.

[twomice]

Jenkins test this please.

[mlutfy]

jenkins, test this please

[eileenmcnaughton]

I think the test fail kind of answers the error question about why that code exists...

[twomice]

@eileenmcnaughton Agreed, I'll address that. BTW, have you found a way to get alerts on test failures like this, or are you just manually checking the results by viewing the PR on github.com?

[eileenmcnaughton]

@twomice the latter

[twomice]

Closed in favor of #12766