Skip to content
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

fixing date_histogram to correctly work inside plugins without global time picker #21955

Merged
merged 8 commits into from
Aug 22, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Aug 14, 2018

resolves #21954

This PR removes the dependency to timeFilter from date histogram aggregations. So charts embedded via the visualize loader will still work in apps where no timeFilter is present.

For QA: This PR should not change any functionality in Kibana.

@ppisljar ppisljar added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.5.0 labels Aug 14, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

One adjustment in code. Also I would like, if we could split out the non time based index into it's own esarchiver image, since it's such a similar name to the other index, and I would rather not load that for all tests, since I see how this might lead to easy confusions.

If we split that index pattern into it's own archive, we could use:

const esArchiver = getService('esArchiver');
// ...
before(async () => {
  await esArchiver.load('visualize-nontimeindex');
});

after(async () => {
  await esArchiver.unload('visualize-nontimeindex');
});

In those two tests, to actually load and unload that.

Maybe so we don't need to load this for every test suite individually, we could add two separate describe blocks visualize app and visualize app non time based into the index.js file for all visualize tests, so we can load this for the whole test block.

@@ -541,6 +541,14 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
async setInterval(newValue) {
const input = await find.byCssSelector('select[ng-model="agg.params.interval"]');
await input.type(newValue);
await remote.pressKeys('\uE006');
Copy link
Contributor

Choose a reason for hiding this comment

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

use:

import Keys from 'leadfoot/keys';

Keys.RETURN

Copy link
Member

@markov00 markov00 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. Only a small typo on test description.
Can you please provide here, or on the issue, a way to reproduce the problem so I can test it easily.

expect(isApplyButtonEnabled).to.be(true);
});

it('should allow resseting changed params', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

typo resseting

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@timroes timroes 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, I really wish we would have the possibility to test custom plugin to test this more, but the added tests about non time based data look good for me as a coverage.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

looks relevant failiure, but just to be sure:

retest

@ppisljar
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

jenkins, test this

@ppisljar
Copy link
Member Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar ppisljar merged commit c38e948 into elastic:master Aug 22, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 22, 2018
@ppisljar ppisljar deleted the fix/timeRange branch August 22, 2018 10:44
ppisljar added a commit that referenced this pull request Aug 22, 2018
@ppisljar ppisljar restored the fix/timeRange branch September 26, 2018 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

visualizeLoader doesn't work inside plugins without time picker
4 participants