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#1217 Fix for failure to respect event_id (or any?) criteria … #16318

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Port of #16313

Note it didn't apply completely cleanup - I had to tweak. I have not tested on 5.21 at this stage

…in smart groups

From what I can tell it's possible to create smart groups from Advanced search & other contact searchs & Membership Search & Event search.

Membership search actually just leverages advanced search but Event Search smart group has it's own code & reloads into Event search.

This has been flawed for months but the current iteration is the criteria are not respected. On digging I found the decision was made in
contact search to store the parsed QueryParams (in [field_name, 'IN', [1,2]] format not the formValues. I think this was probably a mistake.

However, by adjusting the event code to do the same & ensuring the defaults are set it starts working again
@civibot
Copy link

civibot bot commented Jan 17, 2020

(Standard links)

@civibot civibot bot added the 5.21 label Jan 17, 2020
@seamuslee001
Copy link
Contributor

@eileenmcnaughton in the formValues that were stored for my busted smart group attempt it stored as s:8:"event_id";s:3:"3,2" i'm just wondering should w put any handing into like the query object for this or not. This PR works tested creating a smart group or doing an export (that was an earlier regression so just double checking)

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 well - the other regressed 4 months ago I think? So better to fix but if we don't get to it then at least sorting this is worth doing

@eileenmcnaughton
Copy link
Contributor Author

oh - not in the query object though - I'd fix the group in the DB & just handle one thing

@seamuslee001
Copy link
Contributor

@eileenmcnaughton do you think we should merge this even if we don't fix the smart groups in the DB or do we have to do both?

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Jan 17, 2020

@seamuslee001 this is worth doing without the other because is means new ones can be created. Both is better but it's a 4 month old regression or even a never-worked so less urgent

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 note that advanced search works for event smart groups & probably most people use that hence this has not been burning down the house

@seamuslee001
Copy link
Contributor

yeh merging just figured need the question answer documented about reasoning

@seamuslee001 seamuslee001 merged commit 7407bcc into civicrm:5.21 Jan 17, 2020
@seamuslee001 seamuslee001 deleted the 521 branch January 17, 2020 21:44
@eileenmcnaughton
Copy link
Contributor Author

It may never have worked

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.

2 participants