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

Afform - Allow selecting search operator for filter fields #26066

Merged
merged 1 commit into from Apr 21, 2023

Conversation

colemanw
Copy link
Member

Overview

This permits filter operators to be set by the form admin.

Before

No control over filter operators (the implied operator was always inferred from the input type and data type, usually CONTAINS).

After

Operators can be set on the backend of the form:

image

@civibot
Copy link

civibot bot commented Apr 15, 2023

(Standard links)

@demeritcowboy
Copy link
Contributor

Cool it seems to be working but the visual cues to the user aren't quite like I'd expect. I have one on the test site here you can see that I adjusted from one on a real site where this would be neat to have.

  1. It doesn't tell them what the operator is.
  2. If you have a default value, it initially treats it as =.
  3. I'm not sure about the wording of the operators if they are to be displayed on the form to the non-technical users. To them "LIKE" probably doesn't mean anything, and they would expect that's what "CONTAINS" means. "Matches Pattern" probably won't mean anything to them either, and even for techies they might not be sure that means regexp.

Untitled3

@colemanw
Copy link
Member Author

@demeritcowboy TLDR; I've pushed up a fix to item #2. I propose to defer the other two items.

  1. Forcing the operator to be displayed - not everyone might want that, and you can adjust the wording of the field label, or add field help text yourself.
  2. That was a bug, thanks for spotting. I've pushed up a fix.
  3. These are the same operator choices as in SearchKit, so if you think they need adjusting we should do it throughout SearchKit not just here.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Apr 17, 2023
@demeritcowboy
Copy link
Contributor

Ok, I don't want to hold it up over the words. Partly I'm thinking the next obvious step is to have the equivalent of drupal views exposed filters, where the end-users would see/select them. The wording would matter more there.

@yashodha
Copy link
Contributor

@demeritcowboy this should be merged, right?

@UshaMakoa
Copy link
Contributor

Thanks @colemanw nice! I'll test it.

@allinappliadmin
Copy link
Contributor

thanks!

@colemanw
Copy link
Member Author

@UshaMakoa are you ready for this to be merged or do you need to do more testing?

@allinappliadmin
Copy link
Contributor

@colemanw I've tested with the new options and I agree with @demeritcowboy that we should be able to edit the operator on the front because sometimes I need to filter the contacts having the 'volunteer' tag and sometimes I will need to filter those not having the 'volunteer' tag. It doesn't really help me to set it 'hard coded' on the backend of the form.
is it hard job to display the operator on the filter some way like on SearchKit?

image

@colemanw
Copy link
Member Author

Ok guys I think I need to clarify the process of PR review...

The only way for CiviCRM to move forward is for PRs to get merged. When a PR is submitted it needs to be reviewed to answer the basic questions of:

  1. Does it improve CiviCRM?
  2. Does it cause problems?

Generally speaking, if the answers are "yes" and "no" respectively then it should be merged. If there's a desire for additional improvements (and honestly, when is that ever not the case?), then future PRs can be written, but desires for more features (which do not yet exist) shouldn't derail the current PR (which does exist). Or, as someone wiser than me once said "Don't let perfect be the enemy of good."

So the question before us right now is "Should this PR be merged?"

Additionally, I'll note that funding was provided specifically for this feature of back-end configurable filters. So far, no one has yet provided funding for front-end filters.

@demeritcowboy demeritcowboy merged commit d71358b into civicrm:master Apr 21, 2023
3 checks passed
@allinappliadmin
Copy link
Contributor

allinappliadmin commented Apr 21, 2023

Coleman, thanks again for your work and for explaining the merging process and the necessary method behind it.
My mistake if I hadn't made my self clear enough when giving you few specs about what I was expecting and dreaming of. So if the question, should this PR for adding backend operators to filters for FB be merged, I will say YES. And if this work on the backend is necessary to add filters on the frontend then I'll say YES twice. If it doesn't help for the next step - making CiviCRM even better, then I'll say NO because the goal, even if it needs several steps to reach it, is to have operators to filter on the frontend. I've pinged Usha and he might have a different point of view.

@allinappliadmin
Copy link
Contributor

Now it has been merged, the next question is how many € to then make operators on the frontend real? I don't feel like standing now in the middle of river.

@colemanw colemanw deleted the afformSearchOperators branch April 21, 2023 18:33
@colemanw
Copy link
Member Author

Thanks all.
To answer your question @allinappliadmin yes this does get us closer to your goal. I think an additional 5 hours work would be needed to expose operators to the front-end as well as the back-end.

@allinappliadmin
Copy link
Contributor

that seems doable. I've pinged @UshaMakoa to check if he'll be interested in supporting your job with us. Tell you soon.

@UshaMakoa
Copy link
Contributor

Thanks all. To answer your question @allinappliadmin yes this does get us closer to your goal. I think an additional 5 hours work would be needed to expose operators to the front-end as well as the back-end.

ok for 50% for me

@allinappliadmin
Copy link
Contributor

@colemanw so it's GO to have operators on the frontend of filters in Formbuilder so the end user can easily filter within the results :-)

@colemanw
Copy link
Member Author

Ok thanks @allinappliadmin and @UshaMakoa I'll let you know when I have that PR ready.

@colemanw
Copy link
Member Author

colemanw commented Jun 9, 2023

@allinappliadmin @UshaMakoa here is the new PR for exposed operators! #26496

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
5 participants