-
-
Notifications
You must be signed in to change notification settings - Fork 818
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
Update Case.php #31499
base: master
Are you sure you want to change the base?
Update Case.php #31499
Conversation
See https://civicrm.stackexchange.com/questions/48892/is-my-reasoning-correct-in-regard-to-speeding-up-this-core-query-and-if-so-how Coleman suggested using API4, but when I try to do a count in the API4 explorer it comes back as 0, so I was not sure of the right syntax
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
I like your reasoning but there are some test fails because it's not taking the parameters into account for different situations. See also #30178 which is related but also needs a little work because it's not taking all the parameters into account. CRM_Case_BAO_CaseTest.testActiveCaseRole |
So as not to affect situations where a more fine-grained query is needed
NB. I renamed the function from getCaseActivityCountQuery to getCaseCountQuery because even when it take activities into account, it is still returning a count of cases, not a count of activities.
Now only using the simpler query when the request is for all and any cases (as opposed to cases related to a specific user or cases with recent or upcoming activities) |
Hi, There's a secondary bug that's pre-existing and I think has come up before but doesn't seem to happen anywhere in core. The caching of the count is only on $type, and so if you make two consecutive calls to CRM_Case_BAO_Case::getCases with allCases = true and allCases = false, you get an incorrect answer on the second one. That doesn't need to be fixed here I'm just mentioning if it comes up in testing. |
Note that the reason the underlying query is slow is that these joins are nasty - joining a varchar on an INT - in other places we have fixed such that we remove these joins & just lookup the labels using cached values on the php layer (e.g
|
Overview
Query running very slow, doing unnecessary work - see https://civicrm.stackexchange.com/questions/48892/is-my-reasoning-correct-in-regard-to-speeding-up-this-core-query-and-if-so-how
Before
Line 625 calls getCaseActivityCountQuery which in turn calls getCaseActivityQuery
After
Line 625 runs much simpler but equivalent SQL query directly (Coleman suggested using API4, but when I try to do a count in the API4 explorer it comes back as 0, so I was not sure of the right syntax)
Technical Details
I believe the function getCaseActivityCountQuery is not called anywhere else so it seems appropriate to inline it.