PCP report - fix number of donors and total committed. #13252

jmcclelland · 2018-12-11T21:45:57Z

Overview

Ensure number of donors and committed amount fields are properly calculated in the PCP
report.

See: https://lab.civicrm.org/dev/core/issues/586

Before

Instead of displaying the sum of all committed contributions, the report shows just one committed contribution.

Instead of showing the total count of all donors, the report shows the soft contribution id of one of the donors.

After

Data is correct.

Technical Details

I'm simply adding a dbAlias line for the two fields.

civibot · 2018-12-11T21:45:59Z

(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 · 2018-12-12T02:56:18Z

We should get a test on this - it feels like something else isn't working if those lines are needed & when it gets fixed this might break....

Tests are in api_v3_ReportTemplateTest

jmcclelland · 2018-12-12T17:19:34Z

Thanks Eileen for the tip on where to put the test (and hunt for something to imitate). Saved me a lot of time!

eileenmcnaughton · 2018-12-13T02:56:18Z

tests/phpunit/CiviTest/CiviUnitTestCase.php

+   * params needed to create the pcpBlock.
+   *
+   */
+  public function pcpBlockParams() {


I'm increasingly using Traits in the test classes to share functions like this that you want in more than one class but not in EVERY class

Oh, very nice. I've never used traits before. I just make that switch.

eileenmcnaughton · 2018-12-13T03:02:05Z

If this passed locally & not remotely FULL GROUP BY might be hurting you. The approach of adding stuff like this to the group by

  public function groupBy() {
    $this->_groupBy = CRM_Contact_BAO_Query::getGroupByFromSelectColumns($this->_selectClauses, "{$this->_aliases['civicrm_pcp']}.id");
  }

turned out to be a bit of a disaster :-(

I'd start by ripping that out & if it still fails then try

adding

  /**
   * Can this report use the sql mode ONLY_FULL_GROUP_BY.
   * @var bool
   */
  public $optimisedForOnlyFullGroupBy = FALSE;

Bookkeeping extended sets a default group by like this

  public function storeGroupByArray() {
    parent::storeGroupByArray();
    if (empty($this->_groupByArray)) {
      $this->_groupByArray = array(
        "{$this->_aliases['civicrm_entity_financial_trxn']}.id",
        "{$this->_aliases['civicrm_line_item']}.id",
      );
      $this->_rollup = FALSE;
    }
  }

Which I think it a reasonable approach.

jmcclelland · 2018-12-14T16:47:20Z

Just to document what I think is the full story (correct me if I'm wrong)...

Thanks to a change in behavior of mysql old versions of mysql allow you to include fields in select, having or order by that are not explicitly included in the group by clause. With new versions it is optional and dis-allowed by default. The setting is controlled by the sql mode ONLY_FULL_GROUP_BY.

CRM_Contact_BAO_Query::getGroupByFromSelectColumns tries to determine what sql mode you are using and create the group by that will work with your version of mysql.

By "ripping out that function" I think you mean replace it with:

$this->_groupBy = ' GROUP BY ' .  "{$this->_aliases['civicrm_pcp']}.id";

Which I've just done. The test passes locally, so let's see if it passes on the server. I presume that the server is running a version of mysql with ONLY_FULL_GROUP_BY (and my test installation is not) and it will or won't fail based on whether we include in select, etc fields not explicitly covered by the civicrm_pcp.id group by clause.

If it does fail.. I'll investigate your other suggestion.

jmcclelland · 2018-12-17T19:30:12Z

This attempt failed with:

nativecode=1055 ** Expression #5 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'core132523_2uhwk.contribution_civireport.id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_b

I've now added:

 $optimisedForOnlyFullGroupBy = FALSE;

I also rebased to eliminate conflicts.

jmcclelland · 2018-12-18T15:38:37Z

Still failing with: "nativecode=1055 ** Expression #5 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'core132523_c2hlt.contribution_civireport.id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by"

jmcclelland · 2018-12-18T15:46:02Z

I've just enabled ONLY_FULL_GROUP_BY on my dev server so I can work on this without having to push a commit every time. I did it with:

MariaDB [(none)]> SELECT @@GLOBAL.sql_mode; 
+--------------------------------------------+
| @@GLOBAL.sql_mode                          |
+--------------------------------------------+
| NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION |
+--------------------------------------------+
1 row in set (0.00 sec)

MariaDB [(none)]> SET GLOBAL sql_mode = "NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION,ONLY_FULL_GROUP_BY";
Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]>

jmcclelland · 2018-12-18T16:53:42Z

Anyone who truly understands how to naviage ONLY_FULL_GROUP_BY and the various versions of mysql and mariadb should immediately stop working on CiviCRM and pursue the cure to cancer or something that will make better use of your rare intellect.

As for the rest of us... I'm really not sure this fix is the way to go, but, I can now pass the test with my version of mariadb (10.1.26) with both ONLY_FULL_GROUP_BY enabled and disabled.

jmcclelland · 2018-12-18T16:55:53Z

CRM/Contact/BAO/Query.php

-    //return if ONLY_FULL_GROUP_BY is not enabled.
-    if (CRM_Utils_SQL::supportsFullGroupBy() && !empty($sqlMode) && in_array('ONLY_FULL_GROUP_BY', explode(',', $sqlMode))) {
+    // If ONLY_FULL_GROUP_BY is enabled, add fields to group by statement.
+    if (!empty($sqlMode) && in_array('ONLY_FULL_GROUP_BY', explode(',', $sqlMode))) {
      $regexToExclude = '/(ROUND|AVG|COUNT|GROUP_CONCAT|SUM|MAX|MIN|IF)[[:blank:]]*\(/i';


This change is a bit terrifying, but CRM_Utils_SQL::supportsFullGroupBy is deprecated and, in addition to checking if a SQL server is configured to use ONLY_FULL_GROUP_BY it also checks if the server supports ANY_VALUE, which mariadb doesn't, so it always returns FALSE. I don't think this function depends on ANY_VALUE, so I'm simplifying the check to only determine if ONLY_FULL_GROUP_BY is in place.

@jmcclelland if we turn off FGB for the report we won't need this I don't think

jmcclelland · 2018-12-18T16:57:00Z

CRM/Report/Form/Contribute/PCP.php

-            'name' => 'id',
-            'no_display' => TRUE,
-            'required' => TRUE,
-          ),
          'receive_date' => array(


I don't know what the bigger impact of this change will be. I had to include it because otherwise the contribution id is added to the GROUP BY clause when ONLY_FULL_GROUP_BY is enabled, and that causes the aggregate functions to fail since we really don't want to ever group by contribution id, we want to group by PCP page id.

So I would get around this by declaring the report as not supporting FULL GROUP BY rather than changing the report. Once that is done the mode will be turned off preliminary to running the query, rather than us butchering the query to support FGB mode

eileenmcnaughton · 2018-12-20T04:25:54Z

@jmcclelland so if you have this parameter on the report

  /**
   * Can this report use the sql mode ONLY_FULL_GROUP_BY.
   * @var bool
   */
  public $optimisedForOnlyFullGroupBy = FALSE;

It should set it to disable FULL GROUP BY mode - what we have learnt is that retrofitting FULL GROUP BY standards to queries is pretty fraught so we are trying NOT to rewrite queries now.

So in this case I would probably just disable FGB for this report rather than try to fix the BAO_Query function - unless that is tangental - ie. you hit that when working on this but it really should be it's own PR

jmcclelland · 2018-12-21T15:36:36Z

Sorry to waste time! I really did read your initial suggestion and I thought I followed it and it didn't work. On closer examination, I didn't actually follow it correctly so ended up wasting a lot of my time too. Now the test is passing on my dev workstation with a sql database that has ONLY_FULL_GROUP_BY so hopefully we are done.
I notice that the test failed, but it looks like that is due to some problem with the test infrastructure.

seamuslee001 · 2018-12-21T20:30:21Z

Jenkins re test this please

eileenmcnaughton · 2018-12-29T22:31:40Z

@jmcclelland test fail seems related - I tried it locally and removing the groupBy clause & relying on the parent groupby worked for me.

Stacktrace
api_v3_ReportTemplateTest::testPcpReportTotals
Total commited should be $100
Failed asserting that '25.00' matches expected 100.0.

I'm not actually sure that is the correct fix - I think we should be ensuring that the getGroupByFromSelectColumns function is not called at all - but I'm also happy to merge this once it passes tests as long as you are happy the UI experience is improved - because I think the main asset here is the test that you have added.

The addition of getGroupByFromSelectColumns was a technical mistake :-(. You are not supposed to have columns in your select that are not in the group by when group by is used as the results may be partially incorrect - however, adding the columns to the group by turned out to be too much of a sledge hammer & sometimes changed technically incorrect queries that gave the right result to technically correct queries that give the wrong result.

jmcclelland · 2019-01-02T16:56:39Z

Thank you for the feedback Eileen.

I think we need the groupBy clause - since this report should always be grouped by the PCP page.

I just modified the test to create two PCP pages instead of one. Two contributions are made against the first PCP page and one contribution is made against the second page.

The first row of results should only combine the two contributions made against the first PCP page ($100).

When I remove the groupBy function, i get $300 (totalling all the contributions).

I'm still not sure why the tests are failing on the test infrastructure (they are passing on my dev machine).

eileenmcnaughton · 2019-01-02T20:04:26Z

@jmcclelland if you look at how the Contribute_Detail form handles that is declares contribution.id as a group by field defaulting to TRUE

(it is technically possible for someone to unset it but I don't think that is a problem

        'group_bys' => array(
          'contribution_id' => array(
            'name' => 'id',
            'required' => TRUE,
            'default' => TRUE,
            'title' => ts('Contribution'),
          ),
        ),
        'grouping' => 'contri-fields',

jmcclelland · 2019-01-04T15:40:17Z

I think we have finally come full circle to your original thesis Eileen:

The report is broken
We need a test
My original fix was wrong

I removed the orderBy function and added the order_bys array as you suggested. I removed my initial fix of adding the select functions. Then I made a small tweak to my test and it passed (locally at least). Hopefully it will pass here as well!

jmcclelland · 2019-01-04T19:43:56Z

OMG. It hard when my dev test results are different then the test results here :(.

eileenmcnaughton · 2019-01-06T22:06:15Z

@jmcclelland adding this patch makes it work with full group by enabled

diff --git a/CRM/Report/Form.php b/CRM/Report/Form.php
index 1dfb9becd2..1c0269b915 100644
--- a/CRM/Report/Form.php
+++ b/CRM/Report/Form.php
@@ -2865,7 +2865,14 @@ WHERE cg.extends IN ('" . implode("','", $this->_customGroupExtends) . "') AND
     $this->storeGroupByArray();
 
     if (!empty($this->_groupByArray)) {
-      $this->_groupBy = CRM_Contact_BAO_Query::getGroupByFromSelectColumns($this->_selectClauses, $this->_groupByArray);
+      if ($this->optimisedForOnlyFullGroupBy) {
+        // We should probably deprecate this code path. What happens here is that
+        // the group by is amended to reflect the select columns. This often breaks the
+        // results. Retrofitting group strict group by onto existing report classes
+        // went badly.
+        $this->_groupBy = CRM_Contact_BAO_Query::getGroupByFromSelectColumns($this->_selectClauses, $this->_groupByArray);
+      }
+      else $this->_groupBy = ' GROUP BY ' . implode($this->_groupByArray);
     }
   }

jmcclelland · 2019-01-07T15:13:28Z

Thanks again Eileen. Still passing for me locally, fingers crossed it will pass here as well.

eileenmcnaughton · 2019-01-07T20:16:27Z

@jmcclelland it's a pass! Can you squash all these to a single commit?

ensure number of donors and committed are properly calculated in the PCP report. See: https://lab.civicrm.org/dev/core/issues/586

jmcclelland · 2019-01-07T20:41:15Z

Hooray! Thanks for all your help Eileen.

eileenmcnaughton · 2019-01-07T20:42:16Z

@jmcclelland if it makes you feel any better learning these FGB lessons were just as painful for me / Monish etc :-)

eileenmcnaughton · 2019-01-07T22:52:23Z

@jmcclelland merging!

jmcclelland · 2019-01-08T15:46:21Z

Hooray! thanks!

civibot bot added the master label Dec 11, 2018

eileenmcnaughton reviewed Dec 13, 2018

View reviewed changes

jmcclelland force-pushed the issue586 branch from 7fa1c5c to a0fc471 Compare December 17, 2018 19:29

jmcclelland commented Dec 18, 2018

View reviewed changes

PCP report - fix number of donors and total committed.

e40ce31

ensure number of donors and committed are properly calculated in the PCP report. See: https://lab.civicrm.org/dev/core/issues/586

jmcclelland force-pushed the issue586 branch from 5c3ef0c to e40ce31 Compare January 7, 2019 20:40

eileenmcnaughton added the merge on pass label Jan 7, 2019

eileenmcnaughton merged commit e9d8133 into civicrm:master Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PCP report - fix number of donors and total committed. #13252

PCP report - fix number of donors and total committed. #13252

jmcclelland commented Dec 11, 2018

civibot bot commented Dec 11, 2018

eileenmcnaughton commented Dec 12, 2018

jmcclelland commented Dec 12, 2018

eileenmcnaughton Dec 13, 2018

jmcclelland Dec 14, 2018

eileenmcnaughton commented Dec 13, 2018

jmcclelland commented Dec 14, 2018

jmcclelland commented Dec 17, 2018

jmcclelland commented Dec 18, 2018

jmcclelland commented Dec 18, 2018

jmcclelland commented Dec 18, 2018

jmcclelland Dec 18, 2018

eileenmcnaughton Dec 20, 2018

jmcclelland Dec 18, 2018

eileenmcnaughton Dec 20, 2018

eileenmcnaughton commented Dec 20, 2018

jmcclelland commented Dec 21, 2018

seamuslee001 commented Dec 21, 2018

eileenmcnaughton commented Dec 29, 2018

jmcclelland commented Jan 2, 2019

eileenmcnaughton commented Jan 2, 2019

jmcclelland commented Jan 4, 2019

jmcclelland commented Jan 4, 2019

eileenmcnaughton commented Jan 6, 2019

jmcclelland commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

jmcclelland commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

jmcclelland commented Jan 8, 2019

PCP report - fix number of donors and total committed. #13252

PCP report - fix number of donors and total committed. #13252

Conversation

jmcclelland commented Dec 11, 2018

Overview

Before

After

Technical Details

civibot bot commented Dec 11, 2018

eileenmcnaughton commented Dec 12, 2018

jmcclelland commented Dec 12, 2018

eileenmcnaughton Dec 13, 2018

Choose a reason for hiding this comment

jmcclelland Dec 14, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Dec 13, 2018

jmcclelland commented Dec 14, 2018

jmcclelland commented Dec 17, 2018

jmcclelland commented Dec 18, 2018

jmcclelland commented Dec 18, 2018

jmcclelland commented Dec 18, 2018

jmcclelland Dec 18, 2018

Choose a reason for hiding this comment

eileenmcnaughton Dec 20, 2018

Choose a reason for hiding this comment

jmcclelland Dec 18, 2018

Choose a reason for hiding this comment

eileenmcnaughton Dec 20, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Dec 20, 2018

jmcclelland commented Dec 21, 2018

seamuslee001 commented Dec 21, 2018

eileenmcnaughton commented Dec 29, 2018

jmcclelland commented Jan 2, 2019

eileenmcnaughton commented Jan 2, 2019

jmcclelland commented Jan 4, 2019

jmcclelland commented Jan 4, 2019

eileenmcnaughton commented Jan 6, 2019

jmcclelland commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

jmcclelland commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

eileenmcnaughton commented Jan 7, 2019

jmcclelland commented Jan 8, 2019