fix: show_filters URL parameter is not working

[hexcafe]

SUMMARY

The show_filters URL param is no longer working (logic to handle it was removed with #23228).

This PR re-introduces the logic pertinent to this flow from that PR + updates DashboardBuilder so that it works with current master.

This fix should also reflect to embedded when setting filters.visible to false through the SDK configuration.

Fixes #26758.
Fixes #30538.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Tested both with horizontal and vertical filter bar:

Embedded:

TESTING INSTRUCTIONS

Tests added. For manual testing:

1. Create a dashboard and configure at least one filter to it.
2. Copy the dashboard URL, add show_filters=false as a URL parameter and access it.

ADDITIONAL INFORMATION

• Has associated issue: Filters.visible= false Still shows the filters on embedded iFrame #26758, Adding show_filters= to a dashboard URL doesn't do anything #30538

[Vitor-Avila]

Thanks for the PR, @hexcafe. Hey @michael-s-molina @john-bodley,
I believe this is a continuation of #25804 which was aiming to fix #24195 (used to work until #23228).

I checked out this PR locally and tested and it works ✅ but since I'm just getting familiar with frontend I wanted to get your thoughts.

I didn't think so, but this also works with the horizontal filter bar 🙌

Thanks!

[michael-s-molina]

@hexcafe @Vitor-Avila Shouldn't we just revert this part that was deleted in #23228?

[RoBYCoNTe]

Any news about this PR?

[grzaks]

Would be great to see this one merged into next release.

[nawar]

Can we please get this reviewed?

[Vitor-Avila]

@michael-s-molina would you prefer to have that code block restored as opposed to using the approach in this PR?

[michael-s-molina]

@michael-s-molina would you prefer to have that code block restored as opposed to using the approach in this PR?

@Vitor-Avila It seems we should restore that code given that nativeFiltersEnabled, which is used for many things when calculating the dashboard state, used to depend on that parameter.

[zslegl]

Any progress on reviewing this fix?

[rusackas]

Just rebased this, and it looks fine to me. Let's see if CI (still) passes.

[rusackas]

I'm not sure if @Vitor-Avila or @michael-s-molina want to revisit restoring the old code block (which GitHub won't let me view at the moment for some reason 😢) but this PR looks pretty harmless to me in the meantime.

[Vitor-Avila]

@rusackas not sure if @hexcafe is around to assess potentially re-introducing that code. Let me check real quick if that would still work.

[sfirke]

I would like to prioritize getting this fix into the next release. If @hexcafe or someone else is up for restoring the old code block, great, but let's not let that hold this up further if no one can deliver that soon. (I just took an attempt but didn't get it working)

[Vitor-Avila]

@rusackas @michael-s-molina I restored the code from that PR locally (on top of master) to superset-frontend/src/dashboard/components/DashboardBuilder/state.ts:

• declared this const:

const showNativeFilters = useSelector<RootState, boolean>(
    state => (getUrlParam(URL_PARAMS.showFilters) ?? true),
  );

• Updated the definition for nativeFiltersEnabled to:

const nativeFiltersEnabled =
  showNativeFilters &&
  (canEdit || (!canEdit && filterValues.length !== 0));

Still, when loading a dashboard with show_filters=false, the filter bar is visible and expanded. It seems we also need a change in superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx similar to this PR:

const showFilterBar = !editMode && nativeFiltersEnabled;

Would it be better to build on top of this open PR or create a new one?

[Vitor-Avila]

@sfirke happy to help here :) just want to confirm if I should open a new/separate PR or if I should build on top of this one. I'd say the latter since we also need a similar change to @hexcafe and that should preserve his contribution to the work

[sfirke]

I think just build on this PR to preserve hexcafe's contribution while also moving it forward while you're looking at the code

[Vitor-Avila]

@rusackas @sfirke @michael-s-molina I believe this should be all good now. Could I get a new review? Tried adding some tests, but manual tests are always encouraged haha 🙏

[github-actions]

@sfirke Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

[github-actions]

@sfirke Ephemeral environment spinning up at http://18.236.110.2:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

[michael-s-molina]

@Vitor-Avila The original code had && state.dashboardInfo.metadata?.show_native_filters. Isn't that needed anymore?

[Vitor-Avila]

@michael-s-molina my understanding is that the original PR fully removed metadata.show_native_filters (from all places). I suspect the reason for it is that this metadata config was intended to permanently hide the native filters on dashboards that had filter box, until it was fully migrated. So I don't think we need that back. If we want to bring that logic, we would need to re-update the schemas, and logic in other places too.

Not sure if you had a chance to test in that ephemeral but I believe things are working properly with these changes.

[Vitor-Avila]

@sfirke I noticed you created an ephemeral here -- let me know when you're good with testing and I'll merge this one. 🙌

[sfirke]

Hi @Vitor-Avila - I just tested in the ephemeral and it looks ready to merge 🚀 Thanks for this fix!

[sfirke]

This actually closes two issues, I just linked the other in the top post.

[Vitor-Avila]

awesome! thanks for taking the time to test it. Just merged it