chore: Remove obsolete legacy visualizations #24675

john-bodley · 2023-07-12T18:35:25Z

SUMMARY

Whilst working on #24671 I realize that some non-legacy charts (which are invoked via the /api/v1/chart RESTful API endpoint) are still present in superset.viz meaning the logic outlined in said PR for differentiating between legacy and non-legacy charts was incorrect.

This PR removes the following charts (which were marked as deprecated in 3.0 though in actuality were obsolete):

TableViz
BigNumberViz
BigNumberTotalViz

which will ensure that the logic in #24671 will be correct.

Technically (as illustrated by the augmented tests) these visualization types could be invoked either using the legacy or non-legacy RESTful API endpoint, though in actuality (per the frontend logic) they would only ever be invoked under the later.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

I systematically went through all the visualization types in superset/viz.py (all NVD3 and DeckGL charts were bucketed together) and inspected which RESTful API endpoint was being invoked, i.e., any chart using the RESTful /superset/explore_json endpoint (or similar) remained.

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

codecov · 2023-07-12T18:45:15Z

Codecov Report

Merging #24675 (4ebe559) into master (a156816) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 4ebe559 differs from pull request most recent head ac7761a. Consider uploading reports for the commit ac7761a to get more accurate results

@@            Coverage Diff             @@
##           master   #24675      +/-   ##
==========================================
- Coverage   68.97%   68.91%   -0.07%     
==========================================
  Files        1902     1902              
  Lines       74007    73909      -98     
  Branches     8186     8186              
==========================================
- Hits        51047    50931     -116     
- Misses      20839    20857      +18     
  Partials     2121     2121

Flag	Coverage Δ
hive	`54.18% <ø> (+0.05%)`	⬆️
mysql	`79.24% <ø> (-0.11%)`	⬇️
postgres	`79.33% <ø> (-0.12%)`	⬇️
presto	`54.08% <ø> (+0.05%)`	⬆️
python	`83.35% <ø> (-0.10%)`	⬇️
sqlite	`77.91% <ø> (-0.12%)`	⬇️
unit	`54.89% <ø> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
superset/viz.py	`64.51% <ø> (-2.39%)`	⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-07-12T18:56:27Z

tests/integration_tests/cache_tests.py

@@ -46,7 +46,7 @@ def test_no_data_cache(self):
        app.config["DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "NullCache"}
        cache_manager.init_app(app)

-        slc = self.get_slice("Girls", db.session)
+        slc = self.get_slice("Top 10 Girl Name Share", db.session)


Switching out for a legacy chart which invokes the /superset/explore_json RESTful API endpoint.

john-bodley · 2023-07-12T20:25:17Z

tests/integration_tests/charts/api_tests.py

@@ -1715,7 +1715,7 @@ def test_gets_owned_created_favorited_by_me_filter(self):
    )
    def test_warm_up_cache(self):
        self.login()
-        slc = self.get_slice("Girls", db.session)
+        slc = self.get_slice("Top 10 Girl Name Share", db.session)


Currently caching only works with legacy charts and removal of the TableViz now treats the Girls chart as a non-legacy chart. One could argue this is a regression—in terms of fewer charts being eligible for caching—but the problem will be rectified in #24671.

john-bodley · 2023-07-12T23:07:06Z

tests/integration_tests/cache_tests.py

@@ -73,7 +73,7 @@ def test_slice_data_cache(self):
        }
        cache_manager.init_app(app)

-        slc = self.get_slice("Boys", db.session)
+        slc = self.get_slice("Top 10 Girl Name Share", db.session)


See previous comment.

john-bodley · 2023-07-12T23:09:44Z

tests/integration_tests/core_tests.py

@@ -391,7 +393,7 @@ def test_databaseview_edit(self, username="admin"):
    )
    def test_warm_up_cache(self):
        self.login()
-        slc = self.get_slice("Girls", db.session)
+        slc = self.get_slice("Top 10 Girl Name Share", db.session)


See previous comment.

john-bodley · 2023-07-12T23:09:57Z

tests/integration_tests/core_tests.py

@@ -418,10 +420,10 @@ def test_cache_logging(self):
        self.login("admin")
        store_cache_keys = app.config["STORE_CACHE_KEYS_IN_METADATA_DB"]
        app.config["STORE_CACHE_KEYS_IN_METADATA_DB"] = True
-        girls_slice = self.get_slice("Girls", db.session)
-        self.get_json_resp(f"/superset/warm_up_cache?slice_id={girls_slice.id}")
+        slc = self.get_slice("Top 10 Girl Name Share", db.session)


See previous comment.

john-bodley · 2023-07-12T23:11:32Z

tests/integration_tests/security_tests.py

@@ -1680,7 +1680,7 @@ def test_raise_for_access_table(self, mock_can_access):
    def test_raise_for_access_viz(
        self, mock_can_access_schema, mock_can_access, mock_is_owner
    ):
-        test_viz = viz.TableViz(self.get_datasource_mock(), form_data={})
+        test_viz = viz.TimeTableViz(self.get_datasource_mock(), form_data={})


Any legacy viz type works.

john-bodley · 2023-07-12T23:11:59Z

tests/integration_tests/viz_tests.py

@@ -45,7 +45,7 @@ def test_constructor_exception_no_datasource(self):
            viz.BaseViz(datasource, form_data)

    def test_process_metrics(self):
-        # test TableViz metrics in correct order
+        # test TimeTableViz metrics in correct order


Any legacy viz type should work.

michael-s-molina

LGTM. Thanks for the cleanup.

john-bodley · 2023-07-14T06:12:25Z

Closing in favor of #24692.

pull-request-size bot added the size/XL label Jul 12, 2023

john-bodley force-pushed the john-bodley--chore-remove-obsolete-legacy-viz branch from 0888b3c to 612c971 Compare July 12, 2023 18:55

john-bodley commented Jul 12, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--chore-remove-obsolete-legacy-viz branch 2 times, most recently from a04d32e to 6b3902c Compare July 12, 2023 20:22

john-bodley commented Jul 12, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--chore-remove-obsolete-legacy-viz branch from 6b3902c to eea28c3 Compare July 12, 2023 20:42

john-bodley marked this pull request as ready for review July 12, 2023 20:55

john-bodley requested review from michael-s-molina and villebro July 12, 2023 20:55

john-bodley added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 12, 2023

john-bodley commented Jul 12, 2023

View reviewed changes

chore: Remove obsolete legacy visualizations

ac7761a

john-bodley force-pushed the john-bodley--chore-remove-obsolete-legacy-viz branch from eea28c3 to ac7761a Compare July 13, 2023 05:28

michael-s-molina approved these changes Jul 13, 2023

View reviewed changes

john-bodley mentioned this pull request Jul 13, 2023

chore: Remove obsolete legacy visualizations #24692

Closed

9 tasks

john-bodley closed this Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove obsolete legacy visualizations #24675

chore: Remove obsolete legacy visualizations #24675

john-bodley commented Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

michael-s-molina left a comment

john-bodley commented Jul 14, 2023

chore: Remove obsolete legacy visualizations #24675

chore: Remove obsolete legacy visualizations #24675

Conversation

john-bodley commented Jul 12, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

john-bodley Jul 12, 2023

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

john-bodley commented Jul 14, 2023

john-bodley commented Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading