Skip to content

[Vis: Default Editor] Prevent disabling of the only metrics agg#46575

Merged
sulemanof merged 10 commits intoelastic:masterfrom
maryia-lapata:agg-enabled
Oct 2, 2019
Merged

[Vis: Default Editor] Prevent disabling of the only metrics agg#46575
sulemanof merged 10 commits intoelastic:masterfrom
maryia-lapata:agg-enabled

Conversation

@maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Sep 25, 2019

Summary

Fixes #41571

Sep-26-2019 14-41-30

When there are multiple Metrics aggs with different schema name:

Oct-01-2019 12-30-25

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@maryia-lapata maryia-lapata marked this pull request as ready for review September 26, 2019 12:03
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

export interface DefaultEditorAggGroupProps extends DefaultEditorAggCommonProps {
schemas: Schema[];
addSchema: (schems: Schema) => void;
addSchema: (schemas: Schema) => void;
Copy link
Contributor

@majagrubic majagrubic Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spell checker plugin is a pretty useful thing 👍

0
);

if (!enabledAggsCount && metrics.length) {
Copy link
Contributor

@majagrubic majagrubic Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be clearer to have if enabledAggsCount > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is checked whether there are no enabled aggs, e.g. enabledAggsCount === 0. If it's true, we make the first one is enabled.

0
);
const metricCount = group.reduce((count, aggregation: AggConfig) => {
return aggregation.schema.name === agg.schema.name ? ++count : count;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to modify count here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code calculates how many aggs there are with the same schema name as the current agg has.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot lgtm

@maryia-lapata
Copy link
Contributor Author

I noticed one more case. Aggs in Metrics group can be created with schema name metric (Y-axis) or radius (Dot size). For the correct request it's needed at least one metric agg (in Metrics group). I added a screenshot to the description as well.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, didn't test

Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally on Chrome/Windows. Great! LGTM

@sulemanof sulemanof merged commit d8a5765 into elastic:master Oct 2, 2019
@sulemanof sulemanof deleted the agg-enabled branch October 2, 2019 10:48
sulemanof pushed a commit to sulemanof/kibana that referenced this pull request Oct 2, 2019
…tic#46575)

* Prevent disabling of the only metrics agg

* Add unit tests

* Fix when aggs with different schema name
sulemanof added a commit that referenced this pull request Oct 2, 2019
…) (#47107)

* Prevent disabling of the only metrics agg

* Add unit tests

* Fix when aggs with different schema name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Vis] One of metrics should be enabled

6 participants