fix: Added message flash when chart with missing dataset is accessed. #12468

kkucharc · 2021-01-12T19:30:35Z

SUMMARY

Chart with deleted dataset is still available on charts list. While trying to access its explore view page redirects to charts list page. As discussed in issue #12464, I added flash notification message in right bottom corner.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
[Link to previous behaviour video] (https://user-images.githubusercontent.com/25153919/104351996-6655dd00-5506-11eb-929d-a7c1593098e0.mov)

After:

TEST PLAN

Go to charts list and remember choose chart by dataset
Go to list of datasets
Delete chosen dataset
Come back to charts list and try access charts previously chosen (it will have empty dataset name)
It should redirect to charts list and show notification of missing chart and dataset:
The datasource associated with this chart no longer exists

ADDITIONAL INFORMATION

Has associated issue: [listview] Please display message when chart cannot be open because of missing dataset #12464
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

junlincc · 2021-01-12T19:58:18Z

just one quick change - "datasource" to "dataset" in the message 🙏

etr2460

seems legit, although it seems like the redirect wouldn't even be necessary here

nytai · 2021-01-12T20:03:22Z

superset/views/core.py

@@ -702,7 +702,8 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements
            datasource_id, datasource_type = get_datasource_info(
                datasource_id, datasource_type, form_data
            )
-        except SupersetException:
+        except SupersetException as e:
+            flash(e.message, "danger")


This seems like a good pattern in general :)

Thanks @nytai ! :) Unfortunately I updated message after @junlincc comment. Maybe we should change name of variables if those are now datasets? WDYT?

kkucharc · 2021-01-12T20:09:10Z

just one quick change - "datasource" to "dataset" in the message 🙏

It's done 👍

nytai · 2021-01-12T20:15:45Z

@kkucharc I think changing the underlying exception message makes more sense. We're trying to go with "dataset" moving forward. Also please wrap messages in babel translation _() function for i18n

nytai · 2021-01-12T20:16:45Z

superset/views/core.py

@@ -703,6 +703,7 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements
                datasource_id, datasource_type, form_data
            )
        except SupersetException:
+            flash("The dataset associated with this chart no longer exists", "danger")


I think there could be other reasons for an exception, we should use the actual error message

And don't forget to add i18n:

_("Error occurred when opening the chart: %(error)", error=utils.error_msg_from_exception(ex))

Thanks for suggestions! I updated message. I think it would be worth to add an issue to refactor datasource <-> dataset naming convention. WDYT? @nytai @ktmud

@kkucharc I think it's definitely worth creating a separate PR to change the naming for user-facing text and messages, not so sure about the code though.

zuzana-vej · 2021-01-12T21:45:16Z

What is the current behavior? User gets redirected to Explore and then sees an error message because the underlying datasource doesn't exist right?
While not having user go to the chart Explore to find this information out is generally great, there might be some legit cases when user (especially owner) still want to access such chart - for example when they are trying to see the configuration they set up to build a new chart, or if they want to specify new dataset.

adam-stasiak · 2021-01-12T22:21:47Z

@zuzana-vej Now we have situation like on video -
https://user-images.githubusercontent.com/25153919/104351996-6655dd00-5506-11eb-929d-a7c1593098e0.mov
So user stay son listing and does not have information why cannot access

codecov-io · 2021-01-12T22:28:41Z

Codecov Report

Merging #12468 (cb292a4) into master (d8f4443) will decrease coverage by 3.71%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #12468      +/-   ##
==========================================
- Coverage   66.81%   63.10%   -3.72%     
==========================================
  Files        1015      485     -530     
  Lines       49548    29945   -19603     
  Branches     5080        0    -5080     
==========================================
- Hits        33106    18897   -14209     
+ Misses      16312    11048    -5264     
+ Partials      130        0     -130

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`63.10% <0.00%> (-0.98%)`	⬇️

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

Impacted Files	Coverage Δ
superset/views/core.py	`72.84% <0.00%> (-2.54%)`	⬇️
superset/views/utils.py	`82.23% <ø> (ø)`
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/examples/world_bank.py	`30.98% <0.00%> (-69.02%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/db_engine_specs/hive.py	`54.61% <0.00%> (-29.24%)`	⬇️
superset/db_engine_specs/mysql.py	`79.59% <0.00%> (-12.25%)`	⬇️
superset/examples/helpers.py	`85.36% <0.00%> (-12.20%)`	⬇️
superset/db_engine_specs/sqlite.py	`65.62% <0.00%> (-9.38%)`	⬇️
superset/databases/commands/create.py	`83.67% <0.00%> (-8.17%)`	⬇️
... and 557 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8f4443...cb292a4. Read the comment docs.

junlincc

tested, and i noticed charts got completely removed from the list after their underlying dataset is deleted, which is not expected behavior. @adam-stasiak @kkucharc

Expected for this particular change:
User receives a toast message of "The dataset associated with this chart no longer exists" when clicking on a chart which its dataset was deleted from Datasets.
the chart should still be visible on the Charts list view. but the Dataset column should be empty.

adam-stasiak · 2021-01-18T13:58:54Z

@junlincc Could you describe steps to reproduce - > I tried to delete datasets and charts remain.

kkucharc · 2021-01-18T14:04:57Z

Same here:

Before:

After:

Maybe it's something in sorting or Dataset filter?

junlincc

Tested again, LGTM. will merge with two code review approvals.

@kkucharc @adam-stasiak i didn't notice you force pushed. sorry about the confusion! 🙏

…#12468)

superset-github-bot bot added the Polidea label Jan 12, 2021

pull-request-size bot added the size/XS label Jan 12, 2021

etr2460 approved these changes Jan 12, 2021

View reviewed changes

junlincc requested review from rusackas and junlincc January 12, 2021 19:59

nytai reviewed Jan 12, 2021

View reviewed changes

nytai approved these changes Jan 12, 2021

View reviewed changes

nytai reviewed Jan 12, 2021

View reviewed changes

adam-stasiak mentioned this pull request Jan 12, 2021

[dashboard] including chart which is malformed (removed datasource) results in broken links #12476

Closed

pull-request-size bot added size/S and removed size/XS labels Jan 13, 2021

kkucharc closed this Jan 14, 2021

kkucharc reopened this Jan 14, 2021

kkucharc added 4 commits January 15, 2021 09:54

[12464] Added message flash when chart with missing dataset is accessed.

ef783f8

[12464] Updated message.

dad4285

[12464] Changed datasource to dataset in error message.

20f33d4

[12464] Changed error message in test

11b2430

kkucharc force-pushed the 12464-delete-dataset-charts branch from cb292a4 to 11b2430 Compare January 15, 2021 16:34

junlincc reviewed Jan 17, 2021

View reviewed changes

junlincc added the hold:testing! On hold for testing label Jan 17, 2021

junlincc removed the hold:testing! On hold for testing label Jan 19, 2021

junlincc approved these changes Jan 19, 2021

View reviewed changes

junlincc added the listview Namespace | Anything related to lists, such as Dashboards, Charts, Datasets, etc. label Jan 19, 2021

junlincc merged commit ad63823 into apache:master Jan 19, 2021

ktmud mentioned this pull request Jan 23, 2021

feat(explore): allow opening charts with missing dataset #12705

Merged

6 tasks

villebro added the v1.0.1 label Jan 29, 2021

villebro pushed a commit that referenced this pull request Jan 29, 2021

fix: Added message flash when chart with missing dataset is accessed. (…

3b1a84c

…#12468)

mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added message flash when chart with missing dataset is accessed. #12468

fix: Added message flash when chart with missing dataset is accessed. #12468

kkucharc commented Jan 12, 2021

junlincc commented Jan 12, 2021

etr2460 left a comment

nytai Jan 12, 2021

kkucharc Jan 12, 2021

kkucharc commented Jan 12, 2021

nytai commented Jan 12, 2021

nytai Jan 12, 2021

ktmud Jan 12, 2021 •

edited

Loading

kkucharc Jan 13, 2021

ktmud Jan 14, 2021

zuzana-vej commented Jan 12, 2021

adam-stasiak commented Jan 12, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading

junlincc left a comment •

edited

Loading

adam-stasiak commented Jan 18, 2021

kkucharc commented Jan 18, 2021

junlincc left a comment •

edited

Loading

fix: Added message flash when chart with missing dataset is accessed. #12468

fix: Added message flash when chart with missing dataset is accessed. #12468

Conversation

kkucharc commented Jan 12, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

junlincc commented Jan 12, 2021

etr2460 left a comment

Choose a reason for hiding this comment

nytai Jan 12, 2021

Choose a reason for hiding this comment

kkucharc Jan 12, 2021

Choose a reason for hiding this comment

kkucharc commented Jan 12, 2021

nytai commented Jan 12, 2021

nytai Jan 12, 2021

Choose a reason for hiding this comment

ktmud Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

kkucharc Jan 13, 2021

Choose a reason for hiding this comment

ktmud Jan 14, 2021

Choose a reason for hiding this comment

zuzana-vej commented Jan 12, 2021

adam-stasiak commented Jan 12, 2021 • edited Loading

codecov-io commented Jan 12, 2021 • edited Loading

Codecov Report

junlincc left a comment • edited Loading

Choose a reason for hiding this comment

adam-stasiak commented Jan 18, 2021

kkucharc commented Jan 18, 2021

junlincc left a comment • edited Loading

Choose a reason for hiding this comment

ktmud Jan 12, 2021 •

edited

Loading

adam-stasiak commented Jan 12, 2021 •

edited

Loading

codecov-io commented Jan 12, 2021 •

edited

Loading

junlincc left a comment •

edited

Loading

junlincc left a comment •

edited

Loading