-
-
Notifications
You must be signed in to change notification settings - Fork 825
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#720 Remove median & mode stats from contribution summary in order to improve performance #13630
dev/core#720 Remove median & mode stats from contribution summary in order to improve performance #13630
Conversation
(Standard links)
|
@colemanw here it is - this is going to be so good for people's impressions of how fast Civi is. |
@@ -46,14 +46,10 @@ | |||
<th class="left contriTotalRight"> {ts}# Completed{/ts} – {$contributionSummary.total.count}</th> | |||
</tr><tr> | |||
<th class="contriTotalLeft">{ts}Avg{/ts} – {$contributionSummary.total.avg}</th> | |||
<th class="right"> {ts}Median{/ts} – {$contributionSummary.total.median}</th> | |||
<th class="right contriTotalRight"> {ts}Mode{/ts} – {$contributionSummary.total.mode}</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you have to add that dumb contriTotalRight
class to the only th
left in this row, so it will have both contriTotalRight
and contriTotalLeft
classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please do this one too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw can you test something locally - I get the best results for a contact with more than one currency by removing the tpl handling for more than one currency ie
{if $contributionSummary.total.amount}
<th class="contriTotalLeft right">{ts}Total{/ts} – {$contributionSummary.total.amount}</th>
<th class="right"> {ts}# Completed{/ts} – {$contributionSummary.total.count}</th>
<th class="right contriTotalRight"> {ts}Avg{/ts} – {$contributionSummary.total.avg}</th>
{/if}
and NO
{if $contributionSummary.total.currencyCount gt 1}
Might be something we can just strip out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colmanw - I just pushed that up
{else} | ||
<th class="contriTotalLeft right">{ts}Total{/ts} – {$contributionSummary.total.amount}</th> | ||
<th class="right"> {ts}# Completed{/ts} – {$contributionSummary.total.count}</th> | ||
<th class="right"> {ts}Avg{/ts} – {$contributionSummary.total.avg}</th> | ||
<th class="right"> {ts}Median{/ts} – {$contributionSummary.total.median}</th> | ||
<th class="right contriTotalRight"> {ts}Mode{/ts} – {$contributionSummary.total.mode}</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you have to add that dumb contriTotalRight
class to the last th
in this row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colemanw do you think I should put it back or should we do something else once merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead and add the class to the 3rd th
for now. We can kill those dumb classes in a followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - does that look better?
2d8edca
to
1600345
Compare
We MIGHT have reports calling it so to avoid a fatal we will not remove the function just yet
1600345
to
bfac166
Compare
@colemanw this is all passing now |
|
Thanks @colemanw I search for 'all queries of financial type x in January' went from 1.5 minutes to 10 seconds - in this case I did it on our staging server which has no data for January so that extra 1 min 20 seconds was on a no-results query |
Overview
Remove mode & median from stats on contribution search summary and contribution tab on contacts as they take many times longer to render than the rest of the search. This causes a slow user experience on small to medium size sites (up to 100k contributions) and serious lags on larger sites and it was agree that the value of these stats does not warrant the pain caused
https://lab.civicrm.org/dev/core/issues/720
Before
Stats present, slow to render
After
Stats gone - page renders much faster
Technical Details
Full discussion at https://lab.civicrm.org/dev/core/issues/720 - including why the queries cannot be made fast as the annual queries on the contribution summary were
I also move the functions used to calculate the stats over to the one report which calls them. Any custom reports that still call them won't get a fatal error but the stats will not render
Comments