fixing ordering of series in legend

[ppisljar]

Summary: order of legend for series should be same as provided in the configuration.

Resolves #10543

[thomasneirynck]

whoa, I like the small footprint of the fix a lot, and it makes sense to me to do that correction in tabify.

I only have one minor suggestion, not 100% sure if we should introduce a new side-effect in tabify.

[thomasneirynck]

The reorder call creates a side-effect, fairly significant, and is only called from the tabify module. I'm not a 100% sure if this would have other impacts on the code, but it does break encapsulation somewhat.

I'd consider making the reorder() method return a clone of the buckets object, but with the keys ordered against the filters. So in tabify you'd have something like:

 if (buckets.length) {
    const reorderedBuckets = buckets.orderBucketsAccordingToFilterParams(agg.params);
    //... and then just work with reorderedBuckets...
 }

[Bargs]

Getting reorderedBuckets is undefined errors on this line because orderBucketsAccordingToFilterParams is still mutating this instead of returning a value.

[ppisljar]

@thomasneirynck i moved back to the original approach (changing the Buckets itself)
Buckets is a collection and I think its ok if calling order on the collection does the ordering in place (same as js Array.sort) ... also the implementation gets simplified that way (buckets has a prototype forEach function which does some magic behind the scenes)

i also added support for range and ip range aggregations

[thomasneirynck]

perhaps we can split the difference :)

Can we mutate in the constructor then, and keep this out of the flow of tabify.

//in tabify
const buckets = new Buckets(bucket[agg.id], agg.params);

and then in buckets

unction Buckets(aggResp, params) {
    if (_.has(aggResp, 'buckets')) {
      this.buckets = aggResp.buckets;
    } else {
      // Some Bucket Aggs only return a single bucket (like filter).
      // In those instances, the aggResp is the content of the single bucket.
      this.buckets = [aggResp];
    }

    this.objectMode = _.isPlainObject(this.buckets);
    if (this.objectMode) {
      this._keys = _.keys(this.buckets);
      this.length = this._keys.length;
    } else {
      this.length = this.buckets.length;
    }

 if (this.length){
 this.orderBucketsAccordingToParams(params);
}
  }

[ppisljar]

@Bargs this should be ready for another look
please ignore my last comment, tests are failing

[Bargs]

Looks like they're passing now?

[ppisljar]

yup, good for another look

[Bargs]

LGTM

[alexfrancoeur]

@ppisljar I know we discussed but can't recall where we ended up, did we end up backporting to 5.x or is this fixed in 6.0?

[Bargs]

PRs should have version labels on them