Show warning notification when sorting in asc order by count in term agg #8050
Conversation
|
Hey @spalger for some reason I'm seeing two warning notifications. It appears that the |
|
I wonder if that's related to #7960 |
|
Or more specifically, #7945 |
|
Yeah, it would seem so. I'm testing it out... |
|
Actually, the write function is called on both sides of an elasticsearch request. |
No, that doesn't appear to be it. I backported those changes in my local copy and I still see the double notifications. |
|
|
Maybe I should just have a check to show the notification on one of those calls then? |
|
I'm fine with the counter incrementing by 2, personally |
| const orderBy = orderAgg.type.name; | ||
| const sortOrder = agg.params.order.val; | ||
| if (orderBy === 'count' && sortOrder === 'asc') { | ||
| const notify = new Notifier(); |
There was a problem hiding this comment.
Can we keep this at a higher scope? No reason to recreate it all the time. Perhaps you could also use this as a way to debounce the calls...
|
two totally minor things |
|
LGTM |
1 similar comment
|
LGTM |
|
@spalger @ppisljar Thanks for reviewing. I wasn't too happy about the double notification so I looked into this some more. With the latest commit (d78eb51) I was able to conditionally append "[DEPRECATED]" to the "Ascending" select list option itself, when the terms aggregation was being ordered by Count. What do you think of this solution? |
|
i like (prefer) this solution as i am able to see the option is deprecated before clicking "go" ... LGTM |
|
The main reason I suggested using notifications is to warn anyone using the visualization. Modifying the option display text only warns the user when they are creating the visualization. IMO the deprecation warning should be loud, and hard to miss, else people will just ignore it and their visualization will break when they upgrade (This is why I'm fine with the repeating). Also, I don't think it's a good idea to change the display text for the order agg in the controller used for the editor of that agg. The directive code shouldn't be reaching in and changing the visType definition, which should be treated as immutable. |
|
Ah yes, good point about notifying users already using the visualization. I'll revert my last two commits and merge. Thanks again both of you! |
This reverts commit fe2a6f8.
…e itself" This reverts commit d78eb51.
|
I think this notification is too chatty to ship like this. If you have the visualization on a dashboard with autorefresh enabled, it fires the notification on each interval, which means it would be practically impossible to have this visualization on a dashboard without constantly nagging about it. I'd much prefer a more detailed error message that encourages people to switch to desc that happens only once per route change or something. |
|
I think making the notification trigger only once per route change shouldn't be too difficult. |
epixa
changed the title
Resolves #6833
If the user tries to use a Terms aggregation and sort in Ascending order of Count, a warning toast notification is shown.