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

Fix refreshed dashboard losing time range #20858

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Jul 16, 2018

Fixes #20827

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Failed on:

Jest Tests.packages/kbn-plugin-helpers/tasks/build.creating the build skipInstallDependencies = false installs node_modules as a part of build

and

21:39:10    │ proc  [ftr]          └- ✖ fail: "dashboard app using current data create and add embeddables add new visualization link adds a new visualization"
21:39:10    │ proc  [ftr]          │        tryForTime timeout: Error: expected 0 to sort of equal 4

The second failure showed a screenshot of the homepage. I feel like I've seen this flaky behavior before. Confirming...

jenkins, test this.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Okay failed again on UI Functional Tests.test/functional/apps/dashboard/_create_and_add_embeddables·js.dashboard app using current data create and add embeddables add new visualization link adds a new visualization so that one might not be flaky but a legit failure.

other one passed so will file a flaky test issue for it (#20885)

@stacey-gammon
Copy link
Contributor Author

The test is failing because after saving the visualization it's redirecting to home page. I'm pretty sure I've seen this before and it's flaky. Tests also pass locally.

Try again.

jenkins, test this

@stacey-gammon stacey-gammon changed the title Fix refreshed dashboard losing time range [WIP] Fix refreshed dashboard losing time range Jul 17, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

I now think this might be a legit issue. was able to repro locally by running more teste before the one that failed node scripts/functional_test_runner.js --debug --grep "dashboard app using current data"

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Failed on UI Functional Tests.test/functional/apps/home/_home·js.homepage app Kibana takes you home clicking on console on homepage should take you to console app. I don't see any open issues for a flaky test about this. Could be legit, could be flaky. Lets find out.

jenkins, test this.

@stacey-gammon stacey-gammon changed the title [WIP] Fix refreshed dashboard losing time range Fix refreshed dashboard losing time range Jul 24, 2018
@@ -69,6 +70,7 @@ uiModules
require: '^kbnTopNav',
link: ($scope, element, attributes, kbnTopNav) => {
listenForUpdates($scope);
updateGlobalStateWithTime($scope);

setTimefilterValues($scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

Call to setTimefilterValues is no longer needed since updateGlobalStateWithTime calls setTimefilterValues

@@ -69,6 +70,7 @@ uiModules
require: '^kbnTopNav',
link: ($scope, element, attributes, kbnTopNav) => {
listenForUpdates($scope);
updateGlobalStateWithTime($scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what fixed the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, global state wasn't initialized the first time on page load which only created a problem for dashboards stored with time. As soon as any interaction happened with the time picker, it'd update global state. Everything appeared to work correctly in the UI because that was set up correctly, it was just the URL that didn't match it.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jul 25, 2018

Jenkins, test this - seemingly unrelated navigation error

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review, verified changes fixes the linked issue

@nreese
Copy link
Contributor

nreese commented Jul 25, 2018

fail: "homepage app Kibana takes you home clicking on console on homepage should take you to console app"
12:17:04    │ proc  [ftr]      │        Error: expected false to equal true

jenkins, test this

@stacey-gammon
Copy link
Contributor Author

This should also fix the other blocker, #20764

The problem was that if you visit a dashboard and set the time range to something, say Last 5 years, then go open a dashboard that had time stored with it, the dashboard would show the time saved with it, but the URL (which is also used for reporting) would contain the "last 5 years" time.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor

timroes commented Jul 25, 2018

Jenkins, test this - flaky test

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 looks good to me, tested on Chrome Linux. Could reproduce the issue on master, can confirm this PR fixes it.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Failed on #19743 again

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@stacey-gammon stacey-gammon merged commit 38dcda0 into elastic:master Jul 26, 2018
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jul 26, 2018
* Add test that would have caught the bug

* Initialize global state with current time range

* Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests

* remove unneccessary extra call

* Fix tests that failed due to globally added time in new tests

* Update home page test to not care about any state.
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jul 26, 2018
* Add test that would have caught the bug

* Initialize global state with current time range

* Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests

* remove unneccessary extra call

* Fix tests that failed due to globally added time in new tests

* Update home page test to not care about any state.
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jul 26, 2018
* Add test that would have caught the bug

* Initialize global state with current time range

* Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests

* remove unneccessary extra call

* Fix tests that failed due to globally added time in new tests

* Update home page test to not care about any state.
stacey-gammon added a commit that referenced this pull request Jul 26, 2018
* Add test that would have caught the bug

* Initialize global state with current time range

* Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests

* remove unneccessary extra call

* Fix tests that failed due to globally added time in new tests

* Update home page test to not care about any state.
stacey-gammon added a commit that referenced this pull request Jul 26, 2018
* Add test that would have caught the bug

* Initialize global state with current time range

* Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests

* remove unneccessary extra call

* Fix tests that failed due to globally added time in new tests

* Update home page test to not care about any state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants