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

Simplify groupContactCache - remove redundant query #17011

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 7, 2020

Overview

Removes an unnecessary query when filling the smart group cache.

Before

This was essentially running the same query twice, once with COUNT and then again with INSERT.

After

Skip the count as the INSERT will do nothing if there are no results.

Comments

This refactoring is toward supporting API-based smart groups. See #17003

@civibot
Copy link

civibot bot commented Apr 7, 2020

(Standard links)

This was essentially running the same query twice, once with COUNT and then again with INSERT.
Should be ok to skip the count as the INSERT will do nothing if there are no results.
@colemanw
Copy link
Member Author

colemanw commented Apr 9, 2020

retest this please

@mattwire
Copy link
Contributor

mattwire commented Apr 9, 2020

@seamuslee001 I'm trying to work out why we put this check in in the first place? Do you see any reason?

@colemanw I want to test this on one of the original sites that we did the cache work on. But I need to upgrade them to 5.24 (this weekend) - so will get back to you on a review asap!

@mattwire mattwire merged commit 1d0ae46 into civicrm:master Apr 9, 2020
@mattwire
Copy link
Contributor

mattwire commented Apr 9, 2020

I've tested this and it seems to be working fine.

@colemanw colemanw deleted the noCount branch April 9, 2020 21:15
@colemanw
Copy link
Member Author

colemanw commented Apr 9, 2020

Thanks @mattwire! Now we can turn our attention to testing out #17003 - it's mostly a new feature although it does involve some cleanup of old code, so we just need to ensure it doesn't cause regressions. If you have a site with smart groups you can test it on, that'd be great!

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