-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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: safe removal of empty tab with scoped filters #15650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
@kgabryje if you keep a personal "rainy day" backlog, you might be a good candidate to fix the root cause... the filter seems to still be scoped to a deleted chart. That's probably not good in the long run, but doesn't seem to be a problem for now. |
Codecov Report
@@ Coverage Diff @@
## master #15650 +/- ##
==========================================
+ Coverage 76.86% 76.91% +0.04%
==========================================
Files 977 978 +1
Lines 51343 51487 +144
Branches 6920 6951 +31
==========================================
+ Hits 39466 39602 +136
- Misses 11654 11661 +7
- Partials 223 224 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
🏷 2021.27 |
/testenv up |
@junlincc Ephemeral environment spinning up at http://54.213.230.210:8080. Credentials are |
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true |
@rusackas Ephemeral environment spinning up at http://54.245.2.8:8080. Credentials are |
LGTM! thanks for the quick fix! did spot a UX issue not caused by this PR, will file separately After Screen.Recording.2021-07-12.at.10.25.45.PM.movBefore(unnecessary long video please skip watching lol ) Screen.Recording.2021-07-12.at.10.27.46.PM.movScreen.Recording.2021-07-12.at.10.29.32.PM.mov |
Tested with multiple filter some scope to all, some scope to specific tabs. ✅ Product, QA sign off! |
Ephemeral environment shutdown and build artifacts deleted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested issue fixed. Tabs with native filter can be deleted successfully.
✅ QA Product sign off
…chart (apache#15650) (cherry picked from commit 52ad779)
SUMMARY
Fixes a bug where some tabs couldn't be deleted due to native filters that point to no-longer existing charts within the tab.
This PR fixes the acute issue, but there's room for improvement! Whenever a chart is removed, we should
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
![before](https://user-images.githubusercontent.com/812905/125387559-5d31b580-e35b-11eb-82a7-eb54af3fa94f.gif)
After:
![After](https://user-images.githubusercontent.com/812905/125387575-628f0000-e35b-11eb-9f80-5a17ed4a6c87.gif)
TESTING INSTRUCTIONS
To repro the issue (prior to this PR):
With this PR, you should be able to remove the tab with or without the chart.
ADDITIONAL INFORMATION