(dev/core/91) Search Builder Improvements

[monishdeb]

Overview

Earlier there was a requirement where we need to remove specific MySQL operators which are not valid for specific data type e.g. String type doesn't work with >, <, <= and >= operators. And here's the fix 759094b done earlier as per which it is fetching all columns which are String type and passing them into a separate JSON string identified as stringFields and another formatted list of valid string operators as stringOperators. Similarly to identify date fields and to do a specific operation on such fields we are passing another variable dateFields. These JSON object in JS are later used to specific operations. Now on the other hand, recently, I encountered another issue where the Boolean type columns don't work with IS EMPTY/IS NOT EMPTY operator and eventually leads to DB syntax error. So if I follow the same approach I need to assign another JSON string, which won't be an ideal fix. This lead me to do following 3 changes to improve the code:

1. Assign a single list of all data columns which are either Boolean, String or Date (as currently, these are the only kind that needs attention)
2. Use this list (as fieldTypes) in JS to filter the operator or do field specific operations like covert a input search field to datepicker if the chosen search field is date kind.
3. Reduce list of variable assignment.

Before

Using IS EMPTY/IS NOT EMPTY operator against Boolean fields lead to DB Error:

After

Apart from fixing the error there are other scenarios shown in this screencast like datepicker toggle when field/operator is changed to ensure that this doesn't break the existing workflow.

[monishdeb]

ping @lcdservices @colemanw

[eileenmcnaughton]

This is the only place in the code where CRM_Core_SelectValues::getSearchBuilderOperators() is called with $fieldType - I think it makes sense to remove that variable from the function signature

[eileenmcnaughton]

This makes a lot of sense to me - the scope of it seems very much constrained to one screen (search builder) and the code looks sensible. It's 'formy' enough I'm OK with no tests. I would be happy to merge based on @lcdservices testing (I realise that might count as a same shop review but I also know Brian to be very good on search builder & feel it would be appropriate)

[monishdeb]

Plus I scored 4 = -38/+34 cleanup points 😉

[eileenmcnaughton]

lol

[colemanw]

Looks good to me too. Thanks Monish!

[monishdeb]

@colemanw @JoeMurray I wasn't able to comment on the gitlab issue as I cannot sign in and each time
it denies with an error "Connection refused - connect(2) for 45.79.135.8:1389". So let me provide my explanation here. I thought its ok to close https://lab.civicrm.org/dev/core/issues/91 as this PR is merged, which cover all the three points mentioned the description. But then I saw the comment from Joe

I think it would be good practice to provide JSON output with allowed operators for each fieldType, even if some js will be hardcoding how to handle certain fieldTypes.

That’s a good idea, but earlier I checked with all field type, it’s only the String and Boolean field type which don't work with certain operators as hardcoded on the JS and rest all field types works with all kind of SQL operators. So what if we pass another json variable which tells what operators are not valid for only these two kind of field type i.e.

CRM_Core_Resources::singleton()
       ->addScriptFile('civicrm', 'templates/CRM/Contact/Form/Search/Builder.js', 1, 'html-header')
       ->addSetting(array(
         'searchBuilder' => array(
           ...
           'invalidOperators' => [
             ‘Boolean’ => [IS EMPTY, IS NOT EMPTY],
             ’String’ => [>, <, >=, <=],
           ],
         ...
      )

And in JS

if ((field in CRM.searchBuilder.invalidOperators) && 
   (CRM.searchBuilder.fieldTypes[field] in CRM.searchBuilder.invalidOperators)
) {
  CRM.searchBuilder.generalOperators = _.omit(CRM.searchBuilder.generalOperators, CRM.searchBuilder.invalidOperators);
}

But then with this approach we are doing hardcoding in php not in JS. Overall I still feel the current patch is ok unless there is any another issue (other than optimisation) which fall under it.

[eileenmcnaughton]

@monishdeb I think the issue here is mostly the process - ie. the gitlab issue was not framed as what you wanted to solve but how you wanted to solve it - which then got confusing when you used a slightly different approach & it turned out the important thing was not the json but making the bug go away.

I don't have any specific thoughts about 'doing it better next time' since I don't think a small clarification discussion should drive us into navel gazing - but maybe we sometimes are too deep in the solution to describe the problem sometimes when raising tickets :-)

[monishdeb]

Oh I see. At start my intent was only to fix the bug but when I looked at the code it was a bit messy, way it's been handled for string type of fields ( it was me to blame as I was the one who earlier approved and merged the PR #7913). And fixing the bug for Boolean type needs the same treatment like it was done for String kind, but the way this was handled earlier wasn't proper and then I spec out a list of improvements which makes the code more optimised and as a result of which it fixes the bug.

So yes addressing various issues with a single fix has arisen confusion here and I'll keep that in mind will try to be more clear in my issue description about 'what I wanted to solve' and 'how I solved it' and 'how it helped us to address other issues(if any)'

[eileenmcnaughton]

Right - it's easy for us devs to put the cart before the horse sometimes :-)