{"action":"created","issue":{"url":"https://api.github.com/repos/civicrm/civicrm-core/issues/11954","repository_url":"https://api.github.com/repos/civicrm/civicrm-core","labels_url":"https://api.github.com/repos/civicrm/civicrm-core/issues/11954/labels{/name}","comments_url":"https://api.github.com/repos/civicrm/civicrm-core/issues/11954/comments","events_url":"https://api.github.com/repos/civicrm/civicrm-core/issues/11954/events","html_url":"https://github.com/civicrm/civicrm-core/pull/11954","id":311835040,"number":11954,"title":"Fix contribution detail report to work with FULL GROUP BY mode","user":{"login":"eileenmcnaughton","id":336308,"avatar_url":"https://avatars2.githubusercontent.com/u/336308?v=4","gravatar_id":"","url":"https://api.github.com/users/eileenmcnaughton","html_url":"https://github.com/eileenmcnaughton","followers_url":"https://api.github.com/users/eileenmcnaughton/followers","following_url":"https://api.github.com/users/eileenmcnaughton/following{/other_user}","gists_url":"https://api.github.com/users/eileenmcnaughton/gists{/gist_id}","starred_url":"https://api.github.com/users/eileenmcnaughton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eileenmcnaughton/subscriptions","organizations_url":"https://api.github.com/users/eileenmcnaughton/orgs","repos_url":"https://api.github.com/users/eileenmcnaughton/repos","events_url":"https://api.github.com/users/eileenmcnaughton/events{/privacy}","received_events_url":"https://api.github.com/users/eileenmcnaughton/received_events","type":"User","site_admin":false},"labels":[{"id":246037889,"url":"https://api.github.com/repos/civicrm/civicrm-core/labels/master","name":"master","color":"0052cc","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2018-04-06T03:10:03Z","updated_at":"2018-04-09T01:46:28Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/civicrm/civicrm-core/pulls/11954","html_url":"https://github.com/civicrm/civicrm-core/pull/11954","diff_url":"https://github.com/civicrm/civicrm-core/pull/11954.diff","patch_url":"https://github.com/civicrm/civicrm-core/pull/11954.patch"},"body":"Overview\r\n----------------------------------------\r\nFix the contribution detail report so that it works in full group by mode\r\n\r\nBefore\r\n----------------------------------------\r\nWorks when full group by is not enabled, fatal errors when it is.\r\n\r\nAfter\r\n----------------------------------------\r\nWorks in both full group by and non  full group by mode\r\n\r\nTechnical Details\r\n----------------------------------------\r\nFULL GROUP BY mode is a mysql configuration that is enabled out of the box for mysql 5.7. It enforces a level of query logic that prevents queries from returning unreliable results.\r\n\r\nOur test suite shows that some of our tests fail with it (https://test.civicrm.org/job/CiviCRM-Core-Matrix/CIVIVER=5.1,label=ubuntu1604/lastCompletedBuild/testReport/). In some cases these fails are for good reason - eg disabling 'search by primary only' is not reliable with full group by mode enabled and our tests demonstrate that. Having turned full group by mode on locally I have hit other fatal errors & think we should be very clear that it is NOT supported. \r\n\r\nHowever, it does make sense to work towards supporting it and this patch takes us in that direction. The approach is one that was developed in the extended reports extension, less because of the full group by issue & more because of the unreliable results that can be returned if you don't write queries to that standard - ie. the first value for a field might be returned only.\r\n\r\nThe change is intended to be extendable to other reports - but they have to opt-in (by setting $this->groupConcatTested to TRUE). What it does is the select clause it will add wrap any fields in GROUP_CONCAT that are not already wrapped in an aggregate function or in the groupBy selection, if there is a group by clause.\r\n\r\nThe other change is that the parent class added the order by fields to the select clause if they were section headers & the child class did the same for all order by fields. This makes the parent do it for all order by fields. I feel fairly confident about that but I think a merge straight after the rc has been cut would be good timing.\r\n\r\nComments\r\n----------------------------------------\r\nI also added handling for the metadata to declare a field a 'pseudofield' & have select function ignore it (also from extended reports)\r\n\r\n@monishdeb @seamuslee001 ping  - I keep seeing attempts at this so would be good to get this approach tested & agreed\r\n"},"comment":{"url":"https://api.github.com/repos/civicrm/civicrm-core/issues/comments/379603700","html_url":"https://github.com/civicrm/civicrm-core/pull/11954#issuecomment-379603700","issue_url":"https://api.github.com/repos/civicrm/civicrm-core/issues/11954","id":379603700,"user":{"login":"eileenmcnaughton","id":336308,"avatar_url":"https://avatars2.githubusercontent.com/u/336308?v=4","gravatar_id":"","url":"https://api.github.com/users/eileenmcnaughton","html_url":"https://github.com/eileenmcnaughton","followers_url":"https://api.github.com/users/eileenmcnaughton/followers","following_url":"https://api.github.com/users/eileenmcnaughton/following{/other_user}","gists_url":"https://api.github.com/users/eileenmcnaughton/gists{/gist_id}","starred_url":"https://api.github.com/users/eileenmcnaughton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eileenmcnaughton/subscriptions","organizations_url":"https://api.github.com/users/eileenmcnaughton/orgs","repos_url":"https://api.github.com/users/eileenmcnaughton/repos","events_url":"https://api.github.com/users/eileenmcnaughton/events{/privacy}","received_events_url":"https://api.github.com/users/eileenmcnaughton/received_events","type":"User","site_admin":false},"created_at":"2018-04-09T01:46:28Z","updated_at":"2018-04-09T01:46:28Z","author_association":"CONTRIBUTOR","body":"@seamuslee001 I think perhaps the reason the FULL GROUP BY doesn't work with 5.6 for me locally is that we have used the ANY_VALUE hack instead of fixing the queries & it's not applying to 5.6.\r\n\r\nFor example it's complaining about this very complaint worthy query\r\n\r\n```\r\nINSERT INTO civicrm_mailing_recipients (mailing_id, contact_id, email_id)\r\nSELECT 1, i.contact_id, i.email_id\r\nFROM civicrm_contact contact_a\r\n INNER JOIN included_recipients_tempbc71 i ON contact_a.id = i.contact_id \r\n INNER JOIN civicrm_email e ON e.id = i.email_id \r\nWHERE (contact_a.is_deleted = 0)\r\nGROUP BY e.email\r\nORDER BY MIN(i.contact_id), MIN(i.email_id)\r\n```\r\n\r\nI can get past it locally with this patch \r\n\r\n\r\n   /**\r\ndiff --git a/CRM/Mailing/BAO/Mailing.php b/CRM/Mailing/BAO/Mailing.php\r\nindex 3a0f080..9bc77c2 100644\r\n--- a/CRM/Mailing/BAO/Mailing.php\r\n+++ b/CRM/Mailing/BAO/Mailing.php\r\n@@ -326,11 +326,9 @@ class CRM_Mailing_BAO_Mailing extends CRM_Mailing_DAO_Mailing {\r\n \r\n     $query = CRM_Utils_SQL_Select::from('civicrm_contact contact_a')->join('i', \" INNER JOIN {$includedTempTablename} i ON contact_a.id = i.contact_id \");\r\n     if (!$isSMSmode && $mailingObj->dedupe_email) {\r\n-      $orderBy = array(\"MIN(i.contact_id)\", \"MIN(i.$entityColumn)\");\r\n+      $selectClause = array('#mailingID', 'MIN(i.contact_id) contact_id', \"MIN(i.$entityColumn) $entityColumn\", \"e.email\");\r\n+      $orderBy = array(\"contact_id\", \"$entityColumn\");\r\n       $query = $query->join('e', \" INNER JOIN civicrm_email e ON e.id = i.email_id \")->groupBy(\"e.email\");\r\n-      if (CRM_Utils_SQL::supportsFullGroupBy()) {\r\n-        $selectClause = array('#mailingID', 'ANY_VALUE(i.contact_id) contact_id', \"ANY_VALUE(i.$entityColumn) $entityColumn\", \"e.email\");\r\n-      }\r\n     }\r\n \r\n\r\nBUT, I'm not keen on ad hoc patching - I think we need to \r\n1) fix these reports which are failing on our test matrix (this is 1/3 & the other 2 are easy once this is done)\r\n2) change the CI test instance to use full group by, disabling the export test which  just proves that there is a bug when trying to export with not only primary search enabled and full group by - leave that out of scope for now because it's not our priority to fix bugs in newly added features \r\n3) enable mysql 5.7 full group by o default for CI\r\n4) require that ALL full group by fixes have unit tests AND do not accept any further patches that only apply changes if full group by is enabled\r\n5) recommend that full group by is disabled & make it clear full group by is only partially supported"}}
{"actor":{"display_login":"eileenmcnaughton"}}