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.

Sign up for GitHub

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

Jump to bottom

[Dashboard] Fix pinned filters being backed up into Session Storage #142262

Merged
merged 1 commit into from Sep 29, 2022
Merged

[Dashboard] Fix pinned filters being backed up into Session Storage #142262

merged 1 commit into from Sep 29, 2022

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Sep 29, 2022 •
edited by kibanamachine

Summary

Fixes #142161 by ensuring that no Pinned filters are backed up into session storage, and that no pinned filters are considered unsaved changes.

@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. v8.5.0 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:fix and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Sep 29, 2022
@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 418.2KB 418.3KB +88.0B

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

@ThomThomson ThomThomson marked this pull request as ready for review September 29, 2022 19:43
@ThomThomson ThomThomson requested a review from a team as a code owner September 29, 2022 19:43
@elasticmachine
Copy link
Contributor

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

Heenawter
Heenawter approved these changes Sep 29, 2022
View reviewed changes
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.

Code review + tested locally - everything worked great 🎉

Tested the original bug + the following:

  • Pinning and unpinning a filter has the expected impact on the unsaved state
  • Adding and removing pinned and unpinned filters has the expected impact on the unsaved state
  • Drilldown with pinned and unpinned filters where source dashboard was in unsaved state worked as expected

@ThomThomson ThomThomson merged commit 1bf14d7 into elastic:main Sep 29, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.5 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 142262

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 30, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 142262 locally

WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 2, 2022
@ThomThomson @WafaaNasr
Fixed pinned filters being backed up into session storage (elastic#14…
f6c8ffd 
…2262)
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 142262 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 142262 locally

@Heenawter Heenawter added the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label Oct 4, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.5 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 142262

Questions ?

Please refer to the Backport tool documentation

@Heenawter Heenawter added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to the previous minor version (i.e. one version back from main) labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Heenawter Heenawter Heenawter approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.5.0 v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Dashboard] Stuck in unsaved changes state when adding combination of pinned + unpinned filters
5 participants
@ThomThomson @kibana-ci @elasticmachine @kibanamachine @Heenawter