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

dev/core#651 Fix group by on export soft credits (possible recent regression, clearly wrong). #13536

Merged
merged 1 commit into from
Feb 6, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 5, 2019

Overview

Fix for exporting contribution search results not grouping soft credits correctly

In my test I used this search
screenshot 2019-02-05 19 13 43

and this mapping
screenshot 2019-02-05 19 13 32

Before

only 3 rows exported - grouped across scredits

screenshot 2019-02-05 19 12 45

After

mo better rows

screenshot 2019-02-05 19 14 48

Technical Details

As pointed out by the reporter the group by is being calculated as if it were a string but it's an array, this fixes.

This code has been touched recently so it might be a recent regression. 5.10 is the first release in a long time where
export is working in some mysql / output configs after a big refactor to get rid of wide temp tables

Comments

@civibot
Copy link

civibot bot commented Feb 5, 2019

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

unrelated fails

@mattwire
Copy link
Contributor

mattwire commented Feb 5, 2019

@eileenmcnaughton Who reported this? If they can test it and comment here we can merge. It's obviously correctly from looking at the code but I won't be able to test for a couple of days and don't currently have a database with suitable data

@@ -157,7 +157,7 @@ public static function getGroupBy($processor, $returnProperties, $query) {
$groupBy = "civicrm_activity.id ";
}

return $groupBy ? ' GROUP BY ' . $groupBy : '';
return $groupBy ? ' GROUP BY ' . implode(', ', (array) $groupBy) : '';
Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton can we set $groupBy = NULL at L127 and then:

return !$groupBy ?  '' : ' GROUP BY ' . implode(', ', (array) $groupBy);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monishdb I don't think that is any different? I just changed it to null since it seems a little cleaner but it still evaluates to FALSE either way.

The return line is just the same thing re-ordered?

…ression, clearly wrong).

As pointed out by the reporter the group by is being calculated as if it were a string but it's an array, this fixes.

This code has been touched recently so it might be a recent regression. 5.10 is the first release in a long time where
export is working in some mysql / output configs after a big refactor to get rid of wide temp tables
@eileenmcnaughton
Copy link
Contributor Author

@mattwire I kinda feel like they DID test it since they proposed the change pretty close to the way I've rendered it

@monishdeb
Copy link
Member

Tested, working fine. Also checked with other exports just in case. Test failure is unrelated.

@monishdeb monishdeb merged commit 1d34d76 into civicrm:5.10 Feb 6, 2019
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