Remove summary row from charts, when rollup used

[bhahumanists]

Issue: https://lab.civicrm.org/dev/report/-/issues/40

Overview

Graphs on the Contribution Summary report will include the grand total in some circumstances. This makes the final column or wedge incorrect.

Before

Graphs on the Contribution Summary report include the summary row, when rollup is used.

After

Graphs on the Contribution Summary report do not include the grand total, when rollup is used.

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Anything else you would like the reviewer to note

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

[seamuslee001]

@bhahumanists can you fix the style issue? https://test.civicrm.org/job/CiviCRM-Core-PR/34249/checkstyleResult/new/

[bhahumanists]

Damn, I missed this. Can we run again to see the style issue? Amazed I've managed to mess up 3 lines :-) I'll sort

[demeritcowboy]

You can type "jenkins test this please" and it will rerun the tests.

[demeritcowboy]

In case it disappears again, it says

Summary.php:777,           EndLine, Priority: High
--
Whitespace found at end of line

i.e. line 777 isn't actually a completely blank line and has some spaces in it.

[bhahumanists]

jenkins test this please

[yashodha]

@bhahumanists Can you please squash your commits so that this is ready to merge?

[demeritcowboy]

@yashodha yes you're right jut I was actually just reviewing this just now and don't think this is the right fix. So @bhahumanists maybe hold off for a bit.

[demeritcowboy]

Hi,
This got complicated, so I'm going to ask an admin to add the "needs-work" tag and we can take it back to the lab ticket for further thinking, unless someone has a better idea.

• Administrivia:

  • When you do a git push jenkins automatically restarts tests so you don't have to ask it again, although I find if I say it out loud as I push I get better test results (grin).
  • Also as @yashodha mentioned for final merging PR's you need to squash commits.

• General standards

  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] Issue PASS
    • There's some pre-existing weirdness with the pie chart, but for example add a contribution dated "last month" and another the month before (i.e. May 2020 and April 2020), and then run the pie chart. It's missing a month. It also behaves weirdly before the patch, and I admit I haven't investigated further, but the patch seemed to affect it too.

• Developer standards

  • [r-tech] Needs thinking
    • In theory this problem affects any report that has both rollup and charts, since the chart will be receiving the extra rollup rows and overwriting earlier rows which have the same relevant field values. This suggests the fix should at least partly be higher up somewhere. However when I started looking for other examples, both LYBUNT and SYBUNT go and do their own thing, the membership summary chart seems to not be working at all, the event income report does something similar to LYBUNT/SYBUNT, and then I stopped looking. My thinking was along the lines that the sql should also return the column that is used for the rollup, since by definition it will be null for the rollup rows and the purpose of that is so you can identify them. And then the chart and higher functions could deal with them appropriately by checking if that field is null. (In a way this is what the LYBUNT report does, but then excludes those itself.)
  • [r-code] Issue
    • I'm concerned about the way $rows is passed in by reference and then this changes it, so hooks and other functions that come later will no longer be receiving that row. Can't change the function signature without changing other reports. This would be easy enough to work around by making a copy of the variable first, but there's the other issues.
  • [r-maint] Undecided
  • [r-test] PASS

[demeritcowboy]

@bhahumanists - I take back what I said about the pie chart. I was confused. The patch fixes the pie chart too.

I also think it's going to be hard to fix properly higher up, or to return the group-by columns in the select. Especially if the report were to support grouping by more than one rollup field (at the moment it only does it for date).

So rather than hold this up I'm going to suggest the following variation instead, which then at least doesn't affect the hooks and later functions:

--- a/CRM/Report/Form/Contribute/Summary.php
+++ b/CRM/Report/Form/Contribute/Summary.php
@@ -766,7 +766,7 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
    *
    * @param array $rows
    */
-  public function buildChart(&$rows) {
+  public function buildChart(&$original_rows) {
     $graphRows = [];

     if (!empty($this->_params['charts'])) {
@@ -775,6 +775,14 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
         $contrib = !empty($this->_params['fields']['total_amount']);
         $softContrib = !empty($this->_params['fields']['soft_amount']);

+        // Make a copy so that we don't affect what gets passed later to hooks etc.
+        $rows = $original_rows;
+        if ($this->_rollup) {
+          // Remove the total row otherwise it overwrites the real last month's data since it has the
+          // same date.
+          array_pop($rows);
+        }
+
         foreach ($rows as $key => $row) {
           if ($row['civicrm_contribution_receive_date_subtotal']) {
             $graphRows['receive_date'][] = $row['civicrm_contribution_receive_date_start'];

Unless someone sees a better way.

[seamuslee001]

I have updated this based on @demeritcowboy 's suggestion and squashed commits adding MOP