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: Added message flash when chart with missing dataset is accessed. #12468

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

kkucharc
Copy link
Contributor

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:
Large GIF (1366x756)

TEST PLAN

  1. Go to charts list and remember choose chart by dataset
  2. Go to list of datasets
  3. Delete chosen dataset
  4. Come back to charts list and try access charts previously chosen (it will have empty dataset name)
  5. 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

@junlincc
Copy link
Member

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

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

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

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good pattern in general :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@kkucharc
Copy link
Contributor Author

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

It's done 👍

@nytai
Copy link
Member

nytai commented Jan 12, 2021

@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

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@ktmud ktmud Jan 12, 2021

Choose a reason for hiding this comment

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

And don't forget to add i18n:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

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
Copy link
Contributor

adam-stasiak commented Jan 12, 2021

@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
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

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.

@junlincc junlincc added the hold:testing! On hold for testing label Jan 17, 2021
@adam-stasiak
Copy link
Contributor

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

@kkucharc
Copy link
Contributor Author

Same here:

Before:
Screenshot 2021-01-18 at 14 36 37

After:
Screenshot 2021-01-18 at 14 41 06

Maybe it's something in sorting or Dataset filter?

@junlincc junlincc removed the hold:testing! On hold for testing label Jan 19, 2021
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

Screen Shot 2021-01-18 at 10 36 45 PM

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

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

@junlincc junlincc added the listview Namespace | Anything related to lists, such as Dashboards, Charts, Datasets, etc. label Jan 19, 2021
@junlincc junlincc merged commit ad63823 into apache:master Jan 19, 2021
@mistercrunch 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
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 listview Namespace | Anything related to lists, such as Dashboards, Charts, Datasets, etc. size/S v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants