sorting chart xValues by metric sum

[ppisljar]

fixes #5512
fixes #3314

reorder values on ordinal x axis from largest to smallest

[ppisljar]

currently this will reorder values on x-axis no matter the sorting method selected. sorting method only applies to elasticsearch query (so if for example i am aggregating by geo.src instead of getting the buckets with highest counts i will get first 5 countries alphabetically) but then kibana would always order them from biggest to smallest.

i think its correct we dont take query sort order into account, but maybe we could add another option: sort buckets (or dont) in the visualization.

[ppisljar]

i think im gonna add the option to enable/disable this first thing in the morning

[cjcenizal]

@ppisljar what's your assessment of this zero injection library? There are a few smells here which make me wonder if we should improve this code more fundamentally:

1. We're making changes contrary to the Open/Closed Principle. I think this probably means the code wasn't originally written in a way that's easy to extend. If we're just adjusting sorting logic, shouldn't we ideally be able to call some kind of sorting function and just pass in a new comparator?
2. We have to change the test assertions to reflect the changes in code. Ideally, shouldn't we just be able to add new test cases to reflect the functionality we're adding?

Also, after looking at the code more carefully, I find it confusing and difficult to follow. There are numerous conditions, return values of differing types, variables that hold different types of values based on different conditions, and references to specific array elements without any explanations of what those elements are.

On one hand it would be great to clean this up and make it easier to read (and review). On the other hand, cleaning this code up seems like a major task. Clearly you understood it well enough to make this change, so maybe we can compromise with some comments to document the things you learned about it, if a more thorough cleanup effort isn't worth it at this time?

[ppisljar]

@cjcenizal i agree the library is not the best, but in this PR i am not gonna rewrite it. its not the only part of kibana that cries for rewrite ...

• i think the open/closed principle is hard to apply, but not just in this specific file but in other parts of kibana as well (for sure the vislib part). i did infact update the comparator to order on sum instead of index. the second change is making this sum available on the objects we are sorting and the test fixes ....

• regarding assertions ... the issue this fixes is considered a bug. i changed the original code to make it work and i also needed to change some tests which (from this perspective) were actually checking if the code contains a bug.

  im adding option to enable/disable this .... so the tests will go back to where they were and im gonna add a new one to test for new scenario.

[cjcenizal]

Looks great! Just had a few small suggestions for making the code easier to follow.

[cjcenizal]

I think renaming this to orderBucketsBySum everywhere will make it a little clearer, since technically the buckets are going to be sorted anyway, right? They'll just be sorted by their index by default.

[cjcenizal]

"Correct order" isn't very precise. Let's make this description more explicit, so people reading this will know exactly what to expect of this text:

it('should return an array of values ordered by their sum when orderBucketsBySum is true', function () {

I think it's worth updating the preceding test's description to provide better contrast:

it('should return an array of values ordered by their index by default', function () {

[cjcenizal]

How about "Order buckets by descending sum"?

[thomasneirynck]

LGTM