dev/core#2684 Fix filters tpl in civi reports to permit more than one table in a grouping #20820

eileenmcnaughton · 2021-07-09T04:56:49Z

Overview

This fixes the handling of 'grouping' in civireport table definition to do what it was seemingly intended to do - but didn't

Before

If you attempt to assign more than one table to the same 'Grouping' in the report you wind up with duplicate accordians rather than one accordian with one header & both tables in it . You can also wind up with broken html in some cases

After

In this example the Contact, phone and email tables are all grouped as 'contact'. Note the PR doesn't make this change to any core reports - but this PR in extended reports uses it eileenmcnaughton/nz.co.fuzion.extendedreport#490

-

Technical Details

In 'classic' CiviCRM fashion we were assigning an array of 'filterGroupings' and separately an array or filters and trying to cross reference them in the tpl layer to figure out when to open and close the accordians. This switched to only using the 'filterGroupings' array and adding the values from the 'filters' array into that array at the php layer - leaving a single iteration at the tpl layer

Comments

To test this make the below changes - note I'm not including this change or any change that actually changes output in this patch (and I don't necessarily see us changing any core reports) - but by applying the patch it can be seen the filters can be directed to go in an accordion

--- a/CRM/Report/Form/Contribute/Detail.php
+++ b/CRM/Report/Form/Contribute/Detail.php
@@ -77,6 +77,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
         'fields_required' => ['id'],
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Contribution'),
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Contribution'),
             ],
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Contribution'),
             ],
           ],
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Contribution'),
             ],
           ],
-          'grouping' => 'contri-fields',
:
         'filters_defaults' => ['is_deleted' => 0],
         'no_field_disambiguation' => TRUE,
+        'grouping' => 'contact-fields',
+        'group_title' => ts('Contact fields'),
       ]),
       [
         'civicrm_email' => [
@@ -253,7 +255,8 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Contribution'),
             ],
           ],
-          'grouping' => 'contri-fields',
+          'grouping' => 'contribution-fields',
+          'group_title' => ts('Contribution fields')
         ],
         'civicrm_contribution_soft' => [
           'dao' => 'CRM_Contribute_DAO_ContributionSoft',
@@ -276,9 +279,11 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
               'title' => ts('Soft Credit'),
             ],
           ],
+          'grouping' => 'contribution-fields',
         ],
         'civicrm_financial_trxn' => [
           'dao' => 'CRM_Financial_DAO_FinancialTrxn',
+          'grouping' => 'contribution-fields',
           'fields' => [
             'card_type_id' => [
               'title' => ts('Credit Card Type'),
@@ -296,7 +301,7 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
         ],
         'civicrm_batch' => [
           'dao' => 'CRM_Batch_DAO_EntityBatch',
-          'grouping' => 'contri-fields',
+          'grouping' => 'contribution-fields',
           'fields' => [
             'batch_id' => [
               'name' => 'batch_id',
@@ -316,6 +321,7 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
         'civicrm_contribution_ordinality' => [
           'dao' => 'CRM_Contribute_DAO_Contribution',
           'alias' => 'cordinality',
+          'grouping' => 'contribution-fields',
           'filters' => [
             'ordinality' => [
               'title' => ts('Contribution Ordinality'),
@@ -330,6 +336,7 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
         ],
         'civicrm_note' => [
           'dao' => 'CRM_Core_DAO_Note',
+          'grouping' => 'contribution-fields',
           'fields' => [
             'contribution_note' => [
               'name' => 'note',
@@ -347,6 +354,7 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
         ],
         'civicrm_pledge_payment' => [
           'dao' => 'CRM_Pledge_DAO_PledgePayment',
+          'grouping' => 'contribution-fields',
           'fields' => [
             'pledge_id' => [
               'title' => ts('Pledge ID'),
@@ -360,7 +368,7 @@ class CRM_Report_Form_Contribute_Detail extends CRM_Report_Form {
           ],
         ],
       ],
-      $this->getColumns('Address')
+      $this->getColumns('Address', ['grouping' => 'contact-fields'])
     );
     // The tests test for this variation of the sort_name field. Don't argue with the tests :-).
     $this->_columns['civicrm_contact']['fields']['sort_name']['title'] = ts('Donor Name');

civibot · 2021-07-09T04:56:51Z

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

This requires / works with civicrm/civicrm-core#20820

eileenmcnaughton · 2021-07-11T23:36:19Z

test this please

demeritcowboy · 2021-07-14T21:39:39Z

So far so good, just noticing it does seem to rearrange the filters on the contribution detail report, almost into virtual accordions. Not necessarily bad just checking it's all still working.

eileenmcnaughton · 2021-07-14T21:56:16Z

@demeritcowboy yeah - that's why I haven't added it to any core reports - it just is a way to see what it does

demeritcowboy · 2021-07-14T22:01:01Z

I mean even without the additional patch for seeing the accordions, a stock install with this PR applied has different order/grouping of filters on that report.

eileenmcnaughton · 2021-07-14T22:47:37Z

Hmm - the ordering should be according to the grouping key - else that key shouldn't be set. But we shouldn't be showing changes in the use or otherwise of accordions in core

demeritcowboy · 2021-07-14T23:21:01Z

templates/CRM/Report/Form/Tabs/Filters.tpl

      {assign var="counter" value=1}
-      {foreach from=$filters item=table key=tableName}


Minor thing: Because there's no more tableName, down on line 37/31 it doesn't like it. Also 42/36.

thanks - updated

demeritcowboy · 2021-07-14T23:24:54Z

I think it's ok function-wise it's just what it's doing is putting anything with a different 'grouping' key into its own <table>. It's easier to see if you also give the group a group_title. It happens in participant listing and others too. So there's a visual change but I'm personally ok with it.

eileenmcnaughton · 2021-07-14T23:30:50Z

@demeritcowboy yeah I believe that was the original intent of the 'grouping' parameter - it was just poorly implemented

demeritcowboy · 2021-07-15T14:03:10Z

If you want to address the missing tableName above I'm ok to merge this.

… table in a grouping

eileenmcnaughton · 2021-07-15T21:10:35Z

I think I fixed it

demeritcowboy · 2021-07-15T22:36:13Z

jenkins retest this please

(api.v4.Action.CreateWithOptionGroupTest.testWithCustomDataForMultipleContacts)

demeritcowboy · 2021-07-16T00:26:47Z

jenkins retest this please

This is a new one:

CRM_Core_DAOTest::testSqlModePresent
Failed asserting that an array contains 'ONLY_FULL_GROUP_BY'.

/home/jenkins/bknix-dfl/build/core-20820-5chgm/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/DAOTest.php:385

eileenmcnaughton · 2021-07-16T01:46:22Z

Yeah - it's popping up a bit now thought!

eileenmcnaughton · 2021-07-16T02:58:47Z

test this please

eileenmcnaughton · 2021-07-16T02:59:08Z

Fail still seems unrelated but let's see what happens

api\v4\Action\CreateWithOptionGroupTest::testWithCustomDataForMultipleContacts
PEAR_Exception: DB Error: already exists

eileenmcnaughton · 2021-07-16T05:23:15Z

Back to just the unrelated sqlmode failure

civibot bot added the master label Jul 9, 2021

eileenmcnaughton added a commit to eileenmcnaughton/nz.co.fuzion.extendedreport that referenced this pull request Jul 9, 2021

Fix filter grouping

f0fc18d

This requires / works with civicrm/civicrm-core#20820

eileenmcnaughton mentioned this pull request Jul 9, 2021

Fix filter grouping eileenmcnaughton/nz.co.fuzion.extendedreport#490

Merged

demeritcowboy reviewed Jul 14, 2021

View reviewed changes

dev/core#2684 Fix filters tpl in civi reports to permit more than one…

cd3936f

… table in a grouping

eileenmcnaughton force-pushed the tpl branch from 04f3326 to cd3936f Compare July 15, 2021 21:09

demeritcowboy added the merge on pass label Jul 15, 2021

eileenmcnaughton merged commit b00ecc5 into civicrm:master Jul 16, 2021

eileenmcnaughton deleted the tpl branch July 16, 2021 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#2684 Fix filters tpl in civi reports to permit more than one table in a grouping #20820

dev/core#2684 Fix filters tpl in civi reports to permit more than one table in a grouping #20820

eileenmcnaughton commented Jul 9, 2021 •

edited

Loading

civibot bot commented Jul 9, 2021

eileenmcnaughton commented Jul 11, 2021

demeritcowboy commented Jul 14, 2021

eileenmcnaughton commented Jul 14, 2021

demeritcowboy commented Jul 14, 2021

eileenmcnaughton commented Jul 14, 2021

demeritcowboy Jul 14, 2021 •

edited

Loading

eileenmcnaughton Jul 15, 2021

demeritcowboy commented Jul 14, 2021 •

edited

Loading

eileenmcnaughton commented Jul 14, 2021

demeritcowboy commented Jul 15, 2021

eileenmcnaughton commented Jul 15, 2021

demeritcowboy commented Jul 15, 2021

demeritcowboy commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

		{assign var="counter" value=1}
		{foreach from=$filters item=table key=tableName}

dev/core#2684 Fix filters tpl in civi reports to permit more than one table in a grouping #20820

dev/core#2684 Fix filters tpl in civi reports to permit more than one table in a grouping #20820

Conversation

eileenmcnaughton commented Jul 9, 2021 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 9, 2021

eileenmcnaughton commented Jul 11, 2021

demeritcowboy commented Jul 14, 2021

eileenmcnaughton commented Jul 14, 2021

demeritcowboy commented Jul 14, 2021

eileenmcnaughton commented Jul 14, 2021

demeritcowboy Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Jul 15, 2021

Choose a reason for hiding this comment

demeritcowboy commented Jul 14, 2021 • edited Loading

eileenmcnaughton commented Jul 14, 2021

demeritcowboy commented Jul 15, 2021

eileenmcnaughton commented Jul 15, 2021

demeritcowboy commented Jul 15, 2021

demeritcowboy commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 16, 2021

eileenmcnaughton commented Jul 9, 2021 •

edited

Loading

demeritcowboy Jul 14, 2021 •

edited

Loading

demeritcowboy commented Jul 14, 2021 •

edited

Loading