-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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(explore): Filters with custom SQL disappearing #21114
Conversation
...Object.keys(filterBoxData), | ||
...Object.keys(nativeFiltersData), | ||
] | ||
.filter(key => key.match(/adhoc_filter.*/)) |
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.
handles the case of fields like adhoc_filters_b
in mixed chart. Logic similar to superset/utils/core.py
, line 1121
/testenv up |
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
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!
('comparator' in existingFilter && | ||
'comparator' in adhocFilter && | ||
isEqual(existingFilter.comparator, adhocFilter.comparator))), | ||
(existingFilter: AdhocFilter) => |
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.
Can we split this code into equality functions for each filter type to improve readability? You can do it in a follow-up PR.
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.
Code looks good and fix works locally for me!
@geido Container image not yet published for this PR. Please try again when build is complete. |
@geido Ephemeral environment creation failed. Please check the Actions logs for details. |
/testenv up |
@kgabryje Ephemeral environment spinning up at http://54.190.201.200:8080. Credentials are |
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
Codecov Report
@@ Coverage Diff @@
## master #21114 +/- ##
==========================================
+ Coverage 66.27% 66.30% +0.03%
==========================================
Files 1770 1772 +2
Lines 67543 67612 +69
Branches 7185 7204 +19
==========================================
+ Hits 44764 44832 +68
+ Misses 20940 20938 -2
- Partials 1839 1842 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Ephemeral environment shutdown and build artifacts deleted. |
* fix(explore): Filters with custom SQL disappearing * Fix adhoc filter for query b disappearing * Improve test coverage (cherry picked from commit 55304b0)
* fix(explore): Filters with custom SQL disappearing * Fix adhoc filter for query b disappearing * Improve test coverage (cherry picked from commit 55304b0)
SUMMARY
When user added more than 1 filter with custom SQL and saved the chart, all filters with custom SQL except for the first one disappeared. This PR fixes that issue.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2022-08-17.at.18.24.45.mov
After:
Screen.Recording.2022-08-17.at.18.22.55.mov
TESTING INSTRUCTIONS
Open explore and add a few filters with custom sql
Save chart
All added filters should still be there
Open a mixed chart
Add adhoc filters for both queries
Save chart
Make sure that adhoc filters for both queries are still there
ADDITIONAL INFORMATION