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

[Dashboard] Fix Time Range Regression #159337

Merged
merged 13 commits into from
Jun 15, 2023

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Jun 8, 2023

Summary

Fixes #157164.
Fixes #151555.

The linked issue was a regression caused by #144332. In short, when the Dashboard was saved with a time range, but the URL had an explicit time range set in the _g param, the dashboard loading would set the input time range to the saved time, but set the time picker range to the explicit URL time.

This PR fixes this bug, adds a jest test to ensure the input is properly set in this case, and extends the functional test to check both the value of the time picker and that the time was applied to the charts on the dashboard.

Additionally, this PR fixes an issue where changing the global time then pressing back would restore the default time (15 minutes) rather than the time saved with the Dashboard.

Checklist

Delete any items that are not applicable to this PR.

@ThomThomson ThomThomson added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.8.2 Feature:Dashboard Dashboard related features release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort impact:critical This issue should be addressed immediately due to a critical level of impact on the product. and removed v8.8.2 labels Jun 8, 2023
@ThomThomson ThomThomson marked this pull request as ready for review June 13, 2023 13:58
@ThomThomson ThomThomson requested a review from a team as a code owner June 13, 2023 13:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

@Heenawter Heenawter self-requested a review June 13, 2023 18:00
@ThomThomson ThomThomson requested a review from a team as a code owner June 15, 2023 17:18
@ThomThomson ThomThomson removed the request for review from a team June 15, 2023 17:39
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 358.6KB 359.3KB +728.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 410 414 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 493 497 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@Heenawter Heenawter 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 + code review. Everything worked great this time, couldn't find any more weird edge cases - LGTM 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test addition 🎉

*/
const initialTimeRange: TimeRange = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments here make the logic super easy to follow 👍 Makes sense to me

@ThomThomson ThomThomson merged commit f60d43e into elastic:main Jun 15, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 15, 2023
Fixed Dashboard loading with a saved time range when the URL also contains a time range.

(cherry picked from commit f60d43e)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Jun 19, 2023
Fixed Dashboard loading with a saved time range when the URL also contains a time range.

(cherry picked from commit f60d43e)
@ThomThomson
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

ThomThomson added a commit that referenced this pull request Jun 19, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[Dashboard] Fix Time Range Regression
(#159337)](#159337)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Devon
Thomson","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-06-15T20:10:55Z","message":"[Dashboard]
Fix Time Range Regression (#159337)\n\nFixed Dashboard loading with a
saved time range when the URL also contains a time
range.","sha":"f60d43e27b7c28e73f13b122453ac27c8e2e62bd","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Dashboard","release_note:fix","Team:Presentation","loe:hours","impact:critical","backport:prev-minor","v8.9.0","v8.8.2"],"number":159337,"url":"https://github.com/elastic/kibana/pull/159337","mergeCommit":{"message":"[Dashboard]
Fix Time Range Regression (#159337)\n\nFixed Dashboard loading with a
saved time range when the URL also contains a time
range.","sha":"f60d43e27b7c28e73f13b122453ac27c8e2e62bd"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/159337","number":159337,"mergeCommit":{"message":"[Dashboard]
Fix Time Range Regression (#159337)\n\nFixed Dashboard loading with a
saved time range when the URL also contains a time
range.","sha":"f60d43e27b7c28e73f13b122453ac27c8e2e62bd"}},{"branch":"8.8","label":"v8.8.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Dashboard Dashboard related features impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:small Small Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.8.2 v8.9.0
Projects
None yet
5 participants