refactor: Removes the Filter Box code #26328

michael-s-molina · 2023-12-21T17:23:56Z

SUMMARY

As part of the 4.0 approved initiatives, this PR removes the Filter Box code and it's associated dependencies react-select and array-move. It also removes the DeprecatedSelect and AsyncSelect components that were exclusively used by filter boxes. Existing filter boxes will be automatically migrated to native filters.

TESTING INSTRUCTIONS

CI should be sufficient for merging this PR. We'll do a complete testing of 4.0 after all approved proposals are merged.

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

john-bodley · 2023-12-21T17:37:48Z

@michael-s-molina thanks for tackling this. I think there's more dead code which could be removed, i.e., the changeFilter logic seems to be filter-box related. I also suspect that the entirety of dashboardFilters.js et al. could be removed.

codecov · 2023-12-21T17:38:47Z

Codecov Report

Attention: 76 lines in your changes are missing coverage. Please review.

Comparison is base (8f8c435) 69.28% compared to head (894baf5) 69.57%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
superset/migrations/shared/native_filters.py	37.50%	70 Missing ⚠️
...t-frontend/src/dashboard/actions/dashboardState.js	0.00%	1 Missing ⚠️
...ard/components/filterscope/FilterScopeSelector.jsx	0.00%	0 Missing and 1 partial ⚠️
...d/src/dashboard/util/logging/childChartsDidLoad.js	0.00%	1 Missing ⚠️
superset/commands/chart/importers/v1/__init__.py	50.00%	1 Missing ⚠️
...perset/commands/dashboard/importers/v1/__init__.py	90.90%	1 Missing ⚠️
superset/commands/importers/v1/assets.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26328      +/-   ##
==========================================
+ Coverage   69.28%   69.57%   +0.28%     
==========================================
  Files        1909     1893      -16     
  Lines       74875    74220     -655     
  Branches     8398     8263     -135     
==========================================
- Hits        51876    51635     -241     
+ Misses      20856    20504     -352     
+ Partials     2143     2081      -62

Flag	Coverage Δ
hive	`53.77% <9.85%> (+0.20%)`	⬆️
javascript	`56.84% <78.57%> (+0.05%)`	⬆️
mysql	`78.19% <48.59%> (+0.36%)`	⬆️
postgres	`78.32% <48.59%> (+0.39%)`	⬆️
presto	`53.72% <9.85%> (+0.20%)`	⬆️
python	`83.20% <48.59%> (+0.41%)`	⬆️
sqlite	`77.90% <48.59%> (+0.39%)`	⬆️
unit	`56.34% <35.91%> (+0.31%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

UPDATING.md

michael-s-molina · 2023-12-27T11:19:42Z

@michael-s-molina thanks for tackling this. I think there's more dead code which could be removed, i.e., the changeFilter logic seems to be filter-box related. I also suspect that the entirety of dashboardFilters.js et al. could be removed.

@john-bodley I also thought this was related but in fact it's used by the feature that shows which filters are applied to a chart.

john-bodley · 2023-12-28T19:40:07Z

@michael-s-molina I also thought that, however the changeFilter method doesn’t fire (confirmed by the lack of the logged event) and thus I suspect there’s some additional dead code there.

michael-s-molina · 2023-12-28T19:56:03Z

@michael-s-molina I also thought that, however the changeFilter method doesn’t fire (confirmed by the lack of the logged event) and thus I suspect there’s some additional dead code there.

You're correct. The changeFilter is not being called.

I also suspect that the entirety of dashboardFilters.js et al. could be removed.

The addFilter and removeFilter are being called when you add/remove charts to/from the dashboard.

michael-s-molina · 2023-12-29T11:34:49Z

@john-bodley changeFilter is called from some plugins.

Screen.Recording.2023-12-29.at.08.33.29.mov

justinpark

LGTM

john-bodley · 2024-01-08T22:38:02Z

@michael-s-molina both this PR and #26329 handle aspects of removing filter-box charts in favor of the native dashboard filters. These PRs seem highly coupled and per my comments in the later I wonder if there's any incentive to combine them and/or (for review purposes) make one PR are precursor (base branch) of the other?

Additionally, though necessary, I don't believe these two PRs are sufficient for the removal of the legacy filter box charts, i.e., a migration is necessary. The native filter CLI commands should provide the necessary logic which (thankfully) has no dependencies on any of the changes outlined in these two PRs.

I'm not sure where best to add this, be it in a separate PR, or including with the proposed logic if combined into a single PR, but it seems there should be an Alembic migration added where the upgrade is akin to:

from superset.cli import native_filters


def upgrade() -> None:
    native_filters.upgrade(["--all"], standalone_mode=False)
    native_filters.cleanup(["--all"], standalone_mode=False)


def downgrade() -> None:
    pass

michael-s-molina · 2024-01-10T12:10:23Z

@michael-s-molina both this PR and #26329 handle aspects of removing filter-box charts in favor of the native dashboard filters. These PRs seem highly coupled and per my comments in the later I wonder if there's any incentive to combine them and/or (for review purposes) make one PR are precursor (base branch) of the other?

Although related, the features work independently of each other which allow us to have separate PRs. Having said that, pretty much any 4.0 PR will need to be rebased once another one is merged to master given the number of touched files in these types of PRs. In other words, if we merge the native filters one first, this one will need to be rebased and vice versa.

michael-s-molina · 2024-01-10T12:58:13Z

I'm not sure where best to add this, be it in a separate PR, or including with the proposed logic if combined into a single PR, but it seems there should be an Alembic migration added where the upgrade is akin to:

I added a migration script to forcefully migrate legacy filter boxes. I copied the logic from the CLI command into the migration file and made the adjustments such as binding the session and using pagination. This allows us to encapsulate filter box dependencies inside a migration script and remove the CLI command.

john-bodley · 2024-01-10T17:34:09Z

@michael-s-molina regarding,

I added a migration script to forcefully migrate legacy filter boxes. I copied the logic from the CLI command into the migration file and made the adjustments such as binding the session and using pagination. This allows us to encapsulate filter box dependencies inside a migration script and remove the CLI command.

There's still the need to cleanup the dashboard metadata (the temporary fields allows one to downgrade) as well as removal of the filter box charts.

I'm not sure if it makes more sense to make this migration either:

Irreversible (akin to refactor: Removes the Filter Box code #26328 (comment) where I included the cleanup logic in the upgrade and made the downgrade a no-op—this was under the assumption that keeping around dangling filter-box charts would be problematic without the supporting code).
Reversible (akin to the logic you currently have) with a future migration in 4.1 or 5.0 which executes the cleanup logic.

If we opt for the later, the issue is that the cleanup logic no longer exists. My sense is we will need to preserve the cleanup portion of the CLI command—though delete the upgrade/downgrade logic.

rusackas · 2024-01-10T19:22:05Z

superset-frontend/package.json

@@ -192,7 +191,6 @@
    "react-reverse-portal": "^2.1.1",
    "react-router-dom": "^5.3.4",
    "react-search-input": "^0.11.3",
-    "react-select": "^3.2.0",


YAAAAAAAAAAAAYYYYY!!!!

michael-s-molina · 2024-01-12T13:15:32Z

Give that this test for the legacy v0 dashboard importer I was wondering whether we just skip this test for now? I've likely already committed numerous hours to this PR and (due to other work requirements) likely couldn't revisit it until towards the end of next week.

@john-bodley Given that this test is named test_import_override_dashboard_slice_reset_ownership and the previous one is test_import_new_dashboard_slice_reset_ownership, I just copied the logic from the previous test given that they should be similar tests with the only difference being a overridden dashboard instead of a new one.

john-bodley · 2024-01-13T20:01:26Z

tests/integration_tests/import_export_tests.py

@@ -542,9 +542,11 @@ def test_import_override_dashboard_slice_reset_ownership(self):
        self.assertEqual(imported_slc.owners, [gamma_user])

        # re-import with another user shouldn't change the permissions
-        g.user = admin_user


@michael-s-molina I don't believe your logic is correct, i.e., the original logic was actually correct. The intention of the test (as far as I can tell) is to import a dashboard into Superset (as the gamma user) and then reimport the same dashboard (as the admin user). Given that the dashboard existed the creator and owner should not change.

I presume the changed_by doesn't change because none of the dashboard attributes (metadata) changed, but that's simply a hunch.

Let me know how you want to proceed here. I assumed the tests were similar because they have the exact names with the only difference being new vs override. Given that you wanted to skip the test, I would keep it as is.

@michael-s-molina I wonder if we should leave the test as-is and just skip it. It's the legacy version of the importer as well and thus it's likely unused. Maybe @betodealmeida would have some thoughts regarding this?

Yeah, it's a common pattern, where admins will manage dashboards under source control and periodically sync them to Superset. In that case you want to preserve the original ownership.

But like John said, if this is for the v0 importer (I'm on my phone, hard to see) then I wouldn't worry about it too much, since a lot of the behavior there is not strictly defined.

@john-bodley I reverted and skipped the test.

kgabryje · 2024-01-19T09:47:51Z

superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts

-   * Formerly called "filters", which was misleading because it is actually
-   * initial values of the filter_box and table vis
-   */
+  /** Initial values */


Nit: maybe we should just remove this comment?

kgabryje · 2024-01-19T09:49:43Z

superset-frontend/src/components/TelemetryPixel/index.tsx

@@ -47,7 +47,7 @@ const TelemetryPixel = ({
  const pixelPath = `https://apachesuperset.gateway.scarf.sh/pixel/${PIXEL_ID}/${version}/${sha}/${build}`;
  return process.env.SCARF_ANALYTICS === 'false' ? null : (
    <img
-      referrerPolicy="no-referrer-when-downgrade"
+      referrerPolicy="no-referrer"


Is this change related to the pr or just a rebase mistake?

It was a rebase mistake! Reverted!

kgabryje · 2024-01-19T09:51:13Z

superset-frontend/src/dashboard/actions/dashboardState.js

@@ -581,7 +581,7 @@ export function addSliceToDashboard(id, component) {
    ]).then(() => {
      dispatch(addSlice(selectedSlice));

-      if (selectedSlice && selectedSlice.viz_type === 'filter_box') {
+      if (selectedSlice) {


If the code in the if block was meant to run only for filter box, shouldn’t we remove it?

Great call! By removing this I was able to remove even more code!

kgabryje · 2024-01-19T09:51:39Z

superset-frontend/src/dashboard/actions/dashboardState.js

@@ -591,7 +591,7 @@ export function addSliceToDashboard(id, component) {
 export function removeSliceFromDashboard(id) {
  return (dispatch, getState) => {
    const sliceEntity = getState().sliceEntities.slices[id];
-    if (sliceEntity && sliceEntity.viz_type === 'filter_box') {
+    if (sliceEntity) {


Same as above

kgabryje

Amazing work! LGTM

Co-authored-by: John Bodley <john.bodley@gmail.com>

michael-s-molina added risk:breaking-change Issues or PRs that will introduce breaking changes v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch labels Dec 21, 2023

pull-request-size bot added the size/XXL label Dec 21, 2023

michael-s-molina requested review from justinpark and john-bodley December 21, 2023 17:24

michael-s-molina added the hold! On hold label Dec 21, 2023

john-bodley reviewed Dec 21, 2023

View reviewed changes

UPDATING.md Outdated Show resolved Hide resolved

michael-s-molina requested a review from a team December 28, 2023 19:28

michael-s-molina marked this pull request as ready for review January 5, 2024 18:55

michael-s-molina requested review from jinghua-qa, geido, eschutho, rusackas, betodealmeida and kgabryje as code owners January 5, 2024 18:55

john-bodley mentioned this pull request Jan 8, 2024

refactor: Removes the deprecated DASHBOARD_NATIVE_FILTERS feature flag #26329

Merged

9 tasks

justinpark approved these changes Jan 8, 2024

View reviewed changes

michael-s-molina force-pushed the remove-filter-box branch from 9e4d575 to fa7ab9c Compare January 10, 2024 13:00

michael-s-molina requested a review from john-bodley January 10, 2024 14:33

rusackas reviewed Jan 10, 2024

View reviewed changes

michael-s-molina requested a review from betodealmeida January 12, 2024 13:18

john-bodley reviewed Jan 13, 2024

View reviewed changes

michael-s-molina removed the hold! On hold label Jan 16, 2024

betodealmeida approved these changes Jan 17, 2024

View reviewed changes

michael-s-molina and others added 8 commits January 18, 2024 15:46

refactor: Removes Filter Box code

9a61901

Adds PR number to UPDATING.md

b19ac31

Removes FilterBoxViz test

eea4808

Fixes E2E test

c7b51aa

Adds a migration

af4d3b3

Restructure migration/import logic

8ca9e3a

Fixes test

3a80389

Changes migration order

0f73318

michael-s-molina force-pushed the remove-filter-box branch 2 times, most recently from 7a69b1a to a098249 Compare January 18, 2024 19:24

Reverts test and skip it

37d3340

michael-s-molina force-pushed the remove-filter-box branch from a098249 to 37d3340 Compare January 18, 2024 19:30

kgabryje reviewed Jan 19, 2024

View reviewed changes

Addresses comments

894baf5

michael-s-molina requested a review from kgabryje January 19, 2024 11:35

kgabryje approved these changes Jan 19, 2024

View reviewed changes

michael-s-molina merged commit d9a3c3e into apache:master Jan 19, 2024
35 checks passed

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

refactor: Removes the Filter Box code (apache#26328)

1d22ac4

Co-authored-by: John Bodley <john.bodley@gmail.com>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Removes the Filter Box code #26328

refactor: Removes the Filter Box code #26328

michael-s-molina commented Dec 21, 2023 •

edited

Loading

john-bodley commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

michael-s-molina commented Dec 27, 2023

john-bodley commented Dec 28, 2023

michael-s-molina commented Dec 28, 2023

michael-s-molina commented Dec 29, 2023

justinpark left a comment

john-bodley commented Jan 8, 2024

michael-s-molina commented Jan 10, 2024

michael-s-molina commented Jan 10, 2024 •

edited

Loading

john-bodley commented Jan 10, 2024

rusackas Jan 10, 2024

michael-s-molina commented Jan 12, 2024

john-bodley Jan 13, 2024

michael-s-molina Jan 15, 2024

john-bodley Jan 16, 2024

betodealmeida Jan 17, 2024

michael-s-molina Jan 18, 2024

kgabryje Jan 19, 2024

michael-s-molina Jan 19, 2024

kgabryje Jan 19, 2024

michael-s-molina Jan 19, 2024

kgabryje Jan 19, 2024

michael-s-molina Jan 19, 2024 •

edited

Loading

kgabryje Jan 19, 2024

michael-s-molina Jan 19, 2024

kgabryje left a comment

refactor: Removes the Filter Box code #26328

refactor: Removes the Filter Box code #26328

Conversation

michael-s-molina commented Dec 21, 2023 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

john-bodley commented Dec 21, 2023 • edited Loading

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

michael-s-molina commented Dec 27, 2023

john-bodley commented Dec 28, 2023

michael-s-molina commented Dec 28, 2023

michael-s-molina commented Dec 29, 2023

justinpark left a comment

Choose a reason for hiding this comment

john-bodley commented Jan 8, 2024

michael-s-molina commented Jan 10, 2024

michael-s-molina commented Jan 10, 2024 • edited Loading

john-bodley commented Jan 10, 2024

Choose a reason for hiding this comment

michael-s-molina commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

michael-s-molina commented Dec 21, 2023 •

edited

Loading

john-bodley commented Dec 21, 2023 •

edited

Loading

codecov bot commented Dec 21, 2023 •

edited

Loading

michael-s-molina commented Jan 10, 2024 •

edited

Loading

michael-s-molina Jan 19, 2024 •

edited

Loading