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

Search ext: Aggregate field fixes #18520

Merged
merged 3 commits into from
Sep 18, 2020
Merged

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Sep 18, 2020

Overview

Fixes a search extension regression in 5.30 where field labels failed to display in the "Aggregate Fields" section.
Also fixes common crashes caused by MYSQL's only-full-group-by mode, by assigning a default aggregator to non-grouped fields.
Also fixes a bug where aggregating a field would fail to update the ORDER BY clause.

Before

Screen Shot 2020-09-19 at 7 48 39 AM

After

Screen Shot 2020-09-19 at 7 53 33 AM

Comments

Targets 5.30 branch since it addresses a regression in 5.30, and this is still a very new extension.

@civibot
Copy link

civibot bot commented Sep 18, 2020

(Standard links)

@civibot civibot bot added the 5.30 label Sep 18, 2020
@eileenmcnaughton
Copy link
Contributor

ok without test as angular layer only

@eileenmcnaughton
Copy link
Contributor

It works! I added the before & after images to the PR template

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Sep 18, 2020

@colemanw this fixes the problem as described (& so these comments can be for a follow up PR) but I tested to try to

  • get totals of line items grouped by financial type id

Screenshot below but I note

  1. I think we should use group_concat distinct for list rather than group_concat (we might have an option but for default I think distinct is better)
  2. why hasn't it displayed the financial type name?

Screen Shot 2020-09-19 at 7 56 29 AM

@eileenmcnaughton
Copy link
Contributor

Also - the Nan thing.... when you add a $ field to already-displayed-results

Screen Shot 2020-09-19 at 8 13 59 AM

@seamuslee001 seamuslee001 merged commit d1a2fad into civicrm:5.30 Sep 18, 2020
@seamuslee001 seamuslee001 deleted the searchGroupBy branch September 18, 2020 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants