CRM-21364 First batches of test fixes for using ONLY_FULL_GROUP_BY sq… #11207
Conversation
|
ping @eileenmcnaughton i believe these are fairly simple fixes for tests that are failing on ubu1604 now |
| @@ -1450,7 +1450,7 @@ public function subscriptionURL($entityID = NULL, $entity = NULL, $action = 'can | |||
| FROM civicrm_contribution_recur rec | |||
| INNER JOIN civicrm_contribution con ON ( con.contribution_recur_id = rec.id ) | |||
| WHERE rec.id = %1 | |||
| GROUP BY rec.id"; | |||
| GROUP BY rec.id, con.contact_id"; | |||
There was a problem hiding this comment.
only issue here would be is if a contribution in the civicrm_contribution table can be linked to multiple contacts
There was a problem hiding this comment.
@seamuslee001 I would rather say what if we change SELECT MAX(con.contact_id) ? @eileenmcnaughton will there be any consequence?
There was a problem hiding this comment.
guess so or should it be min(con.contact_id)
There was a problem hiding this comment.
MAX would be to fetch the contact of latest contribution linked to a recurring, wouldn't that be correct?
There was a problem hiding this comment.
which should AFAIK be the same as the first given its a series right?
There was a problem hiding this comment.
Hmm right ... let's wait for Eileen's reply.
There was a problem hiding this comment.
Ok so looking at this the point of that join is to filter out recurring transactions where there are no contributions. However, it's not using that as a criteria to determine whether to show the url or anything that seems meaningful to how it's treated. Instead it's being used to determine whether to include a checksum. I think the right answer is to drop the INNER join and the group by
There was a problem hiding this comment.
so do where IN on the civicrm_contribution table?
| @@ -196,13 +196,13 @@ public function fillTable() { | |||
| if ($this->params && !$this->noRules) { | |||
| $tempTableQuery = "CREATE TEMPORARY TABLE dedupe (id1 int, weight int, UNIQUE UI_id1 (id1)) ENGINE=InnoDB"; | |||
| $insertClause = "INSERT INTO dedupe (id1, weight)"; | |||
| $groupByClause = "GROUP BY id1"; | |||
| $groupByClause = "GROUP BY id1, weight"; | |||
There was a problem hiding this comment.
the other option could be max(weight) i suppose
There was a problem hiding this comment.
I would say MAX(weight) would be safe.
There was a problem hiding this comment.
I'm finding it harder to get comfortable with this - I'd probably prefer the rule group changes in a separate PR so I can get my head around them without blocking the ohters
| $orderBy = "sort_name ASC, {$open}.time_stamp DESC"; | ||
| $orderBy = "sort_name ASC "; | ||
| if (!$is_distinct) { | ||
| $orderBy .= ", {$open}.time_stamp DESC"; |
There was a problem hiding this comment.
This doesn't make sense if we are asking for distinct row as then we will only have 1 timestamp for each contact
There was a problem hiding this comment.
i don't think so the incompatiable column is civicrm_mailing_event_opened.time_stamp which is in the ORDER BY is not in the group By
There was a problem hiding this comment.
Oops you are right, my bad. Yup makes sense. Deleted my comment.
| @@ -329,7 +329,7 @@ public function testbyGroupType() { | |||
| foreach ($useCases as $case) { | |||
| $query = new CRM_Contact_BAO_Query(CRM_Contact_BAO_Query::convertFormValues($case['form_value'])); | |||
| list($select, $from, $where, $having) = $query->query(); | |||
| $groupContacts = CRM_Core_DAO::executeQuery("SELECT DISTINCT contact_a.id $from $where ORDER BY contact_a.first_name")->fetchAll(); | |||
| $groupContacts = CRM_Core_DAO::executeQuery("SELECT DISTINCT contact_a.id, contact_a.first_name $from $where ORDER BY contact_a.first_name")->fetchAll(); | |||
There was a problem hiding this comment.
just adds first_name into the select criteria, given that id should be unique anyway shouldn't make an issue
There was a problem hiding this comment.
Agree afterall its UT, and you are fixing the query in a right way :D
| $this->assertEquals('xyzabx0000-00-00 00:00:00', $result['id']); | ||
| $this->assertEquals('xyzabx0000-00-00 00:00:00', $result['id']); | ||
| $this->assertEquals('xyzabx2017-01-01 00:00:00', $result['id']); | ||
| $this->assertEquals('xyzabx2017-01-01 00:00:00', $result['id']); |
There was a problem hiding this comment.
this is about no zero date specifically so just set it to the first of Jan 2017. Test then passes locally
|
this is superceded. Closing |
…lMode
Overview
since testing on Only_full_group_by sqlMode it now shows there are some test failures using this sqlMode
This is pulled out of https://github.com/civicrm/civicrm-core/pull/11053/files as the ones that aren't breaking tests so far
@monishdeb i have left the other breaking tests issues in 11053