Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 28, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 26, 2018

Overview

Reviewers cut of #11550 - reduce unnecessary joins to address tables in reports
Campaign/SurveyDetails
Case/Demographics
Case/Detail
Grant/Detail
Grant/Statistics

Before

unnecessary joins to phone, email and address tables

After

join only when needed, code standardised

Technical Details

This also aligns the approach with that used in the extended report extension - there might be some notices as I have updated the extension where the signature differs from core but will need to drop a new release & people will need to be on it.

Comments

@yashodha back to you :-) This takes us down to 15 reports


@jitendrapurohit
Copy link
Contributor

reviewing this now

LEFT JOIN civicrm_country country
ON {$this->_aliases['civicrm_address']}.country_id =
country.id
LEFT JOIN civicrm_worldregion {$this->_aliases['civicrm_world_region']}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results into DB error on Grant Statistics report.

Database Error Code: Unknown column 'country.region_id' in 'on clause', 1054

Maybe, we should keep the left join of civicrm_country to connect with worldregion table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - I'll take a look....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jitendrapurohit I pushed an update - altered start of country join to

  protected function joinCountryFromAddress($prefix = '', $extra = array()) {
    // include country field if country column is to be included
    if ($this->isTableSelected($prefix . 'civicrm_country') || $this->isTableSelected($prefix . 'civicrm_worldregion')) {
      if (empty($this->_aliases[$prefix . 'civicrm_country'])) {
        $this->_aliases[$prefix . 'civicrm_country'] = $prefix . '_report_country';
      }

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted issue has been fixed. Tested all the reports. They seems to work fine.

@eileenmcnaughton eileenmcnaughton merged commit 1f1d0ed into civicrm:master Mar 28, 2018
@eileenmcnaughton eileenmcnaughton deleted the min_report branch March 28, 2018 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants