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#2704 SearchKit - Add support for SQL functions #20947

Merged
merged 7 commits into from
Aug 2, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 25, 2021

Overview

Support SQL functions in the SearchKit UI

See https://lab.civicrm.org/dev/core/-/issues/2704

Before

Only aggregate functions supported

After

More functions work

Comments

For now it's limited to functions that take a single field as input.

@civibot
Copy link

civibot bot commented Jul 25, 2021

(Standard links)

@civibot civibot bot added the master label Jul 25, 2021
@colemanw colemanw changed the title SearchKit - Add support for SQL functions dev/core#2704 SearchKit - Add support for SQL functions Jul 26, 2021
@eileenmcnaughton
Copy link
Contributor

@colemanw I'm struggling to figure out how to see the new functions - not in the listing here

image

@eileenmcnaughton
Copy link
Contributor

But I feel like the 'distinct' checkbox is missing

image

@eileenmcnaughton
Copy link
Contributor

OK - some cache clearing and I can see they work with no aggregate in place but when aggregate is on the list is more limited (but distinct is back!)

image

With no group by I get this bigger list

image

And year only works

image

As does time only but the output for 'date only' is confusing - it truncates the date but shows '12 am' instead of nothing

image

The 'is null' one is pretty cool.

'math' and 'text' show - but can't be selected

@seamuslee001
Copy link
Contributor

I'm thinking that we maybe should merge this as it is a clear improvement unless the others can be fixed up easily but this seems to be a good improvement

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 no - it actually causes some things to disappear in aggregate mode at the moment

@colemanw
Copy link
Member Author

@seamuslee001 no - it actually causes some things to disappear in aggregate mode at the moment

What is disappearing @eileenmcnaughton ?

@eileenmcnaughton
Copy link
Contributor

@colemanw - see #20947 (comment) - first image - no MAX etc

@colemanw
Copy link
Member Author

@eileenmcnaughton I can't reproduce that locally. Are you able to reproduce it on the demo site and post a link to that search?

@eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton
Copy link
Contributor

Also - if you remove group by you get the weird thing with 'Text' being there 'as if' it means something but doesn't seem to (in the drop down)

colemanw added 3 commits July 28, 2021 18:09
…bels

This splits the concept of prefix/suffix into prefix, flag_before and flag_after,
Since a prefix like ORDER BY is not the same as a flag like DISTINCT.
Lays the groundwork for exposing more info about SQL functiont to a UI like SearchKIt.
@colemanw
Copy link
Member Author

Ah ok thanks @eileenmcnaughton - found it & fixed both issues.

@colemanw
Copy link
Member Author

@eileenmcnaughton and I just pushed up another commit to fix the date formatting.

@eileenmcnaughton
Copy link
Contributor

@colemanw api\v4\Action\SqlFunctionTest::testGroupAggregates
Failed asserting that false is true.

/home/jenkins/bknix-dfl/build/core-20947-610bn/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Action/SqlFunctionTest.php:69
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

@colemanw
Copy link
Member Author

@eileenmcnaughton fixed

@eileenmcnaughton
Copy link
Contributor

@colemanw the Replace transform isn't working - perhaps you could suppress it long enough to get this merged & then re-add it? I'm otherwise happy with the pr

image

Either way this describes the 3 params, but the new way is better understood by SearchKit,
which to-date cannot handle more than one param.
@colemanw
Copy link
Member Author

@eileenmcnaughton done. It was supposed to be already hidden because this PR doesn't support > 1 field per function.

@eileenmcnaughton
Copy link
Contributor

The things I previously identified are fixed. I did spot one more thing - the functions being offered for amounts are suitable able for text rather than money columns. However, I think that one lends itself to being fixed in a follow up so I'm merging this now @colemanw

image

@eileenmcnaughton eileenmcnaughton merged commit 1ec58fe into civicrm:master Aug 2, 2021
@eileenmcnaughton eileenmcnaughton deleted the sqlFunctions branch August 2, 2021 00:46
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