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: Validation of out of scope filters and interaction with Clear All #24610

Merged

Conversation

michael-s-molina
Copy link
Member

SUMMARY

Changes the validation of required filters to only be applicable to filters in scope. It also changes the Clear All feature to only affect filters in scope.

Fixes #24496

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-07-06.at.16.46.45.mov

TESTING INSTRUCTIONS

Check the video and original issue for instructions.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 7, 2023
@kgabryje
Copy link
Member

When you click clear all, the query is run for the chart even though the filter is required. I don't think it's expected behaviour since we can't run the query with Apply button.

@michael-s-molina
Copy link
Member Author

michael-s-molina commented Jul 10, 2023

When you click clear all, the query is run for the chart even though the filter is required. I don't think it's expected behaviour since we can't run the query with Apply button.

@kgabryje this bug already exists on master but I added a fix here anyway. Clear all won't fire ANY query if there's an in scope required filter that needs to be filled. Keep in mind that no chart will be updated until this state is first resolved, independently if a chart is affected by the in scope required filter.

Screen.Recording.2023-07-10.at.12.17.08.mov

@yousoph @kgabryje @rusackas A possible alternative would be to exclude required filters from the Clear All action and only clear non-required filters. Let me know what you think about it.

@kgabryje
Copy link
Member

I think it's an optimal solution. The other approach might be to reset the required filter to the default value, but I think it's less intuitive than just clearing the control

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina michael-s-molina merged commit 0efb884 into apache:master Jul 11, 2023
michael-s-molina added a commit that referenced this pull request Jul 26, 2023
@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
3 participants