CRM-21677 - Report improvements #11550

yashodha · 2018-01-18T10:42:54Z

Overview

Clean up reports that have unnecessary joins to phone, email and address.

Before

unnecessary joins to phone, email and address tables

After

join only when needed.

CRM-21677: Report improvements

yashodha · 2018-01-18T16:12:45Z

test this please

eileenmcnaughton · 2018-01-18T21:32:35Z

CRM/Report/Form/Contribute/Summary.php

@@ -41,7 +41,7 @@ class CRM_Report_Form_Contribute_Summary extends CRM_Report_Form {
  protected $_customGroupExtends = array('Contribution', 'Contact', 'Individual');
  protected $_customGroupGroupBy = TRUE;

-  public $_drilldownReport = array('contribute/detail' => 'Link to Detail Report');


I think this should be in a separate PR as it is a behaviour change & we need to check if it will get buy in (I tend to agree with it). The other changes in this are just technical so better to put on 2 tracks

@yashodha this line is the only thing that I think is blocking merge here - because I think it;s a separate change -
although I think it would be better if we changed joinAddressFromClause to joinAddressFromContactClause - and the same on the other functions. (that would make it more in line with extendedReport and differentiate between joinAddressFromLocation from joinAddressFromContact.

eileenmcnaughton · 2018-01-18T21:41:40Z

@yashodha this looks like a nice clean up! Yay + double yay.

I think we should probably sync the approach here with that in the extended report extension since we are cleaning up. I had been porting a few of the approaches I use in that extension to core over time but it takes a little co-ordination because if the signature changes in core I have to change it in extended report ( I try to just remove the function & rely on core where I can)

In extended report I use the following:

  /**
  * Add join from contact table to email.
   *
   * Prefix will be added to both tables as it's assumed you are using it to get address of a secondary contact.
   *
   * @param string $prefix
  *  @param array $extra Additional options.
  *      Not currently used in core but may be used in override extensions.
   */
  protected function joinEmailFromContact($prefix = '', , $extra = array()) {

   if ($this->isTableSelected($prefix . 'civicrm_email')) {
      $this->_from .= " 
       LEFT JOIN civicrm_email {$this->_aliases[$prefix . 'civicrm_email']}
      ON {$this->_aliases[$prefix . 'civicrm_email']}.contact_id = {$this->_aliases[$prefix . 'civicrm_contact']}.id
     AND {$this->_aliases[$prefix . 'civicrm_email']}.is_primary = 1
";
  }

The differences are the parameters that are passed in & the function name. Because I have similar functions for a lot of joins I use the pattern Join...From - since there might also be a join to email from LocationBlock.

Passing in $prefix permits 2 joins to email table (from contact a & from contact b). The $extra array is a relatively recent one & I haven't used it much but I think for a standard pattern it's better to have it - it might be used to indicate 'is_primary_only' = FALSE in the above for a report that wants all addresses returned.

seamuslee001 · 2018-01-19T21:05:11Z

CRM/Report/Form/Contact/Summary.php

-                      {$this->_aliases['civicrm_phone']}.is_primary = 1 ";
-    }
+    $this->addPhoneFromClause();
+    $this->addEmailFromClause()


missing a ; here @yashodha

@seamuslee001 : fixed

eileenmcnaughton · 2018-01-30T22:04:42Z

test errors are on the contact/detail report

yashodha · 2018-01-31T15:32:31Z

@eileenmcnaughton : the functions now have parameters in the same format.

eileenmcnaughton · 2018-01-31T22:09:08Z

Thanks @yashodha - are you OK changing the names too? I think it is clearer saying what table it's coming from

yashodha · 2018-02-09T13:03:29Z

@eileenmcnaughton yes, sure!

yashodha · 2018-02-20T14:02:02Z

test this please

yashodha · 2018-02-21T06:24:56Z

test this please

yashodha · 2018-02-21T06:26:21Z

@eileenmcnaughton : made the changes

yashodha · 2018-03-05T11:40:03Z

test this please

eileenmcnaughton · 2018-03-15T03:09:37Z

test this please

eileenmcnaughton · 2018-03-15T03:10:34Z

CRM/Report/Form.php

+   * @param array $extra Additional options.
+   *      Not currently used in core but may be used in override extensions.
+   */
+  public function joinCountryFromClause($prefix = '', $extra = array()) {


Can we change this to joinCountryFromAddressClause

eileenmcnaughton · 2018-03-15T03:11:29Z

CRM/Report/Form.php

-  public function addPhoneFromClause() {
-    // include address field if address column is to be included
-    if ($this->isTableSelected('civicrm_phone')) {
+  public function joinPhoneFromClause($prefix = '', $extra = array()) {


Can we change to joinPhoneFromContactClause

eileenmcnaughton · 2018-03-15T03:12:18Z

CRM/Report/Form.php

+   * @param array $extra Additional options.
+   *      Not currently used in core but may be used in override extensions.
+   */
+  public function joinEmailFromClause($prefix = '', $extra = array()) {


Can we change to joinEmailFromContactClause

eileenmcnaughton · 2018-03-15T03:16:19Z

@yashodha there are a few minor things here + it needs squashing. I'm going to squash & put up in an alternate PR with the tweaks. You can then check my reviewers cut is still good & merge

eileenmcnaughton · 2018-03-23T07:08:16Z

I'm closing this in favour of #11814 which is a reviewer's cut of it - note that since that is very big I have started to split off chunks of a few reports so that someone can spend 15-20 minutes testing & confirm a chunk - currently #11867

eileenmcnaughton · 2018-03-25T06:01:11Z

Closing this in favour of #11814 which is a reviewer's cut of this

civicrm-builder added the master label Jan 18, 2018

eileenmcnaughton reviewed Jan 18, 2018

View reviewed changes

seamuslee001 reviewed Jan 19, 2018

View reviewed changes

yashodha force-pushed the CRM-21677 branch from e4c6eb1 to ee909d7 Compare January 30, 2018 04:13

yashodha force-pushed the CRM-21677 branch from ee909d7 to e6c2a7b Compare January 31, 2018 15:11

yashodha added 7 commits February 20, 2018 17:43

CRM-21677 - Report improvements

af98ae6

fix warninga

9d3edef

fixes as per comments

46a835e

CRM-21677 - fix DB error and more clean up

5a04938

CRM-21677 - generalised function

b1c173f

minor fix

ec4ca21

CRM-21677 - keep the naming consistent

8514a55

yashodha force-pushed the CRM-21677 branch from 642b673 to 8514a55 Compare February 20, 2018 12:14

style fix

2bd4199

eileenmcnaughton reviewed Mar 15, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request Mar 15, 2018

CRM-21677 - Remove (some) unnecessary query joins when rendering reports #11814

Merged

eileenmcnaughton mentioned this pull request Mar 22, 2018

CRM-21677 - reduce unnecessary address joins on activity & contact detail reports #11855

Merged

eileenmcnaughton mentioned this pull request Mar 23, 2018

CRM-21677 reduce unnecessary joins in membership reports #11867

Merged

eileenmcnaughton closed this Mar 25, 2018

eileenmcnaughton mentioned this pull request Mar 26, 2018

CRM-21677 reduce unnecessary joins in pledge reports #11875

Merged

yashodha mentioned this pull request Mar 26, 2018

CRM-21677 reduce unnecessary joins in mailing reports #11876

Merged

This was referenced Mar 26, 2018

CRM-21677 reduce unnecessary joins in Case, Grant, Survey reports #11880

Merged

CRM-21677 reduce unnecessary joins in reports #11889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21677 - Report improvements #11550

CRM-21677 - Report improvements #11550

yashodha commented Jan 18, 2018 •

edited by civicrm-builder

Loading

yashodha commented Jan 18, 2018

eileenmcnaughton Jan 18, 2018

eileenmcnaughton Mar 5, 2018

eileenmcnaughton commented Jan 18, 2018

seamuslee001 Jan 19, 2018

yashodha Jan 30, 2018

eileenmcnaughton commented Jan 30, 2018

yashodha commented Jan 31, 2018

eileenmcnaughton commented Jan 31, 2018

yashodha commented Feb 9, 2018

yashodha commented Feb 20, 2018

yashodha commented Feb 21, 2018

yashodha commented Feb 21, 2018

yashodha commented Mar 5, 2018

eileenmcnaughton commented Mar 15, 2018

eileenmcnaughton Mar 15, 2018

eileenmcnaughton Mar 15, 2018

eileenmcnaughton Mar 15, 2018

eileenmcnaughton commented Mar 15, 2018

eileenmcnaughton commented Mar 23, 2018

eileenmcnaughton commented Mar 25, 2018

CRM-21677 - Report improvements #11550

CRM-21677 - Report improvements #11550

Conversation

yashodha commented Jan 18, 2018 • edited by civicrm-builder Loading

Overview

Before

After

yashodha commented Jan 18, 2018

eileenmcnaughton Jan 18, 2018

Choose a reason for hiding this comment

eileenmcnaughton Mar 5, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 18, 2018

seamuslee001 Jan 19, 2018

Choose a reason for hiding this comment

yashodha Jan 30, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 30, 2018

yashodha commented Jan 31, 2018

eileenmcnaughton commented Jan 31, 2018

yashodha commented Feb 9, 2018

yashodha commented Feb 20, 2018

yashodha commented Feb 21, 2018

yashodha commented Feb 21, 2018

yashodha commented Mar 5, 2018

eileenmcnaughton commented Mar 15, 2018

eileenmcnaughton Mar 15, 2018

Choose a reason for hiding this comment

eileenmcnaughton Mar 15, 2018

Choose a reason for hiding this comment

eileenmcnaughton Mar 15, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 15, 2018

eileenmcnaughton commented Mar 23, 2018

eileenmcnaughton commented Mar 25, 2018

yashodha commented Jan 18, 2018 •

edited by civicrm-builder

Loading