dev/core/issues/577: Activity Summary report fix for db column count error with section header (without ONLY_FULL_GROUP_BY)

[davejenx]

Overview

See dev/core/issues/577: Fatal db error in Activity Summary report when Sorting uses Section Header (without ONLY_FULL_GROUP_BY).

A fatal db error occurs in Activity Summary report when sorting by a field that has not been selected in Columns, if Section Header is checked. This occurs with or without ONLY_FULL_GROUP_BY.

Steps to replicate:

1. From the standard pre-defined Contact Reports, go to Activity Summary.
2. Leave Columns as default, with Contact Name not checked.
3. Leave Grouping as default (Activity Type + Activity Status).
4. In Sorting, select Contact Name and check "Section Header / Group By".
5. Click "Refresh results".

Before

DB Error: value count on row
Database Error Code: Column count doesn't match value count at row 1, 1136

• Full error details in Gitlab issue.

After

Report runs, no db error.

Technical Details

Order-by columns not selected for display were being added to SELECT but not to the temporary table columns. This PR rectifies that.

Comments

Note that if you are using ONLY_FULL_GROUP_BY, after getting past this error you will be hit by dev/core/issues/578 Fatal db error in Activity Summary report for some Sorting fields (with ONLY_FULL_GROUP_BY).

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

[eileenmcnaughton]

@davejenx this looks good - I think I've stuck my nose into it a few times & concluded it's good but needs a test - the class we add them to is api_v3_ReportTemplateTest

[eileenmcnaughton]

@monishdeb it might have just been this - the bigger one seems to be non-activity-report

[eileenmcnaughton]

@davejenx any hope of getting a test on this

[davejenx]

@eileenmcnaughton Will try to find some time for that this week.

[davejenx]

@eileenmcnaughton Looking in api_v3_ReportTemplateTest, I see two mentions of "activitySummary":

• In getReportTemplates(), activitySummary is in $reportsToSkip:
  'activitySummary' => 'We use temp tables for the main query generation and name are dynamic. These names are not available in stats() when called directly.',
  Does this mean special measures are needed in writing a test for this report?

• testActivitySummary() says "Test activity summary report", however it specifies:
  $params['report_id'] = 'Activity';
  and in my dmaster db that is the Activity Details report:

mysql> select id, title, report_id, name from civicrm_report_instance where report_id like '%activity%';
+----+------------------+-----------------+------+
| id | title            | report_id       | name |
+----+------------------+-----------------+------+
|  3 | Activity Details | activity        | NULL |
|  6 | Activity Summary | activitySummary | NULL |
+----+------------------+-----------------+------+

[eileenmcnaughton]

@davejenx looks like testActivitySummary should be renamed.

If we can get the activitySummary out of $reportsToSkip that would be enough for this round / pr

You can see I have a PR doing that for one of the others

#14098 - generally it's all about getting stuff into shared functions like

beginPostProcessCommon and buildQuery

[davejenx]

@eileenmcnaughton Thanks for the pointers. To see what we're dealing with, I tried removing activitySummary from $reportsToSkip in getReportTemplates(), then running ReportTemplateTest.php . There were two failures (vs none with activitySummary skipped): one related and in line with the description in $reportsToSkip, the other not directly related.

Failure on unrelated report:

1. api_v3_ReportTemplateTest::testLoggingReportWithHookRemovalOfCustomDataTable
   Failure in api call for custom_field create: DB Error: unknown error
   ALTER TABLE dmastertes_1pclu.log_civicrm_value_new_custom_gr_1 ADD field_one_1 varchar(255) COLLATE utf8_unicode_ci DEFAULT 'defaultValue' [nativecode=1060 ** Duplicate column name 'field_one_1']

This occurs on subsequent test runs even if I re-comment-out 'activitySummary', so appears irrelevant. Presumably logging db isn't getting reset.

Failure on activitySummary:
2) api_v3_ReportTemplateTest::testReportTemplateGetStatisticsAllReports with data set #27 ('activitySummary')
Failure in api call for report_template getstatistics: DB Error: syntax error
SELECT .civicrm_activity_activity_type_id,
.civicrm_activity_id_count,
.civicrm_activity_duration_total
FROM INNER JOIN
ON (.id = .id) [nativecode=1064 ** You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '
.civicrm_activity_id_count,
.civicrm_activity_duration_total
' at line 1]

This indicates that {$this->_tempTableName} is not set, in line with the description in $reportsToSkip:
"We use temp tables for the main query generation and name are dynamic. These names are not available in stats() when called directly."
When the report runs normally, looks like statistics() is called from CRM_Report_Form::doTemplateAssignment(), called from CRM_Report_Form_ActivitySummary::postProcess() after the temp table is created. But when run from tests, CRM_Report_Form_ActivitySummary->statistics() is called directly from the API, so the temp table isn't created.
I would think it would be a biggish change to get the report doing its stats without the temp table in place - unless you know of a cunning ploy? So would it make sense to specifically exclude this report in testReportTemplateGetStatisticsAllReports() and remove it from $reportsToSkip in getReportTemplates() ?

[eileenmcnaughton]

@davejenx I feel like most of what is in the postProcess function should be in the buildQuery function instead - which would solve the test problem & probably mean postProcess would not need to be overwritten & a more targetted overwrite could happen

I added #14364 as a very minor cleanup to help a little

[davejenx]

Thanks @eileenmcnaughton . I wasn't clear which other methods get called when statistics() is called via API from the test framework.

I've created #14375 restructuring Activity Summary so that it's no longer skipped from unit tests.

[eileenmcnaughton]

@davejenx this needs to be adjusted now.

Once #14098 is merged we might have all reports under testing (I think that last one is disabled & perhaps we should fully remove but won't hopefully block testing)

[eileenmcnaughton]

@davejenx I removed 'needs test' & added 'has test' since you've improved testing in a separate PR & while it doesn't specifically cover this fix I think it meets the 'improves test cover' requirement

It still needs rebasing

[davejenx]

Rebasing and reapplying the fix leaves the fix no longer working, needs more work.

[davejenx]

@eileenmcnaughton I have rebased and applied some further fixes:

• current master was showing a new issue where sort_name appeared twice in SELECT;
• when sorting by email, the main query was OK but the duration query had a SQL error due to sort field's table being absent.

Tested successfully on local dmaster.

Note that this does NOT fix problems with ONLY_FULL_GROUP_BY described in dev/core/issues/578.

[davejenx]

Jenkins test this please (not this Jenkins, the fella in the suit.)

[eileenmcnaughton]

@davejenx yes - agree this fixes - I'm gonna merge this & also put up a quick PR to disable FGB mode in this report (which we do on existing breakage but we expect new code to be FGB compliant)

[eileenmcnaughton]

@davejenx I added #14745 to very narrowly address the FGB issue