-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
CRM-21806 Search builder NOT Empty does not work #11746
Conversation
@civicrm-builder retest this please |
FYI - I have spoken to @lcdservices about him assigning a resource (guess who :-) to write a UT for this since they have an interest |
There is definitely a regression in search builder - it's not just the 'not empty' clause either. I couldn't get any of search builder to work in the latest master. |
Yes, we've seen the issue to be broader than just NOT EMPTY. Working on a fix. |
(Baking) UT will be ready soon :) On my initial test result this patch fix the regression not only for @JKingsnorth you can test the patch on the test build instance - http://core-11746-6snv5.test-ubu1204-5.civicrm.org/ (creds - pradmin / pradmin1234) |
Added unit test #11751 for this fix |
Hiah, I'm not able to do a full QA on this at the moment. But I can confirm that search builder does appear to be working again on that build, thanks @monishdeb |
Merging as it fixes the issue and doesn't cause any unintended regression to other Search forms. Also the UT is added in #11751 |
This commit breaks the CiviRules custom search. Database Error Code: Unknown column 'contact_a.id' in 'field list', 1054
|
@monishdeb I am running Ubuntu 17.10 with civicrm current master via buildkit. |
Overview
Incorrect empty results on Search builder search
Before
Steps to recreate the issue:
After
Results show
Technical Details
Replaces hacky method of guessing the select clause (which turned out to be wrong)
Comments
I took a look at this because it was marked critical but have not determined if it was a regression or a never worked. I'm also concious we probably need a unit test to make this mergeable but I'm also feeling unable to do that on this at the moment as my unfunded time is a bit tight (ie. I don't have any skin in this - I looked at it because it is a critical issue). I don't have an answer for that - ie. I don't think it should be merged without a test but don't know that a test is iminent