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 missing datasource error message #5019

Merged

Conversation

timifasubaa
Copy link
Contributor

When a datasource is deleted, slices show a cryptic error message in image 1.
This PR fixes it so it shows a more intelligible error message as in image 2.

@john-bodley @michellethomas
screen shot 2018-05-16 at 12 23 05 pm

screen shot 2018-05-16 at 12 30 15 pm

@timifasubaa timifasubaa force-pushed the fix_error_message_for_missing_datasource branch from 87156cb to cf374ef Compare May 16, 2018 19:32
@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #5019 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5019      +/-   ##
==========================================
- Coverage   77.34%   77.34%   -0.01%     
==========================================
  Files          44       44              
  Lines        8665     8668       +3     
==========================================
+ Hits         6702     6704       +2     
- Misses       1963     1964       +1
Impacted Files Coverage Δ
superset/views/core.py 74.64% <66.66%> (-0.02%) ⬇️

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 5a64b3f...63115fb. Read the comment docs.

@john-bodley
Copy link
Member

Should we cascade deleted datasource and delete the corresponding slice(s)?

@@ -1229,7 +1229,10 @@ def datasource_info(datasource_id, datasource_type, form_data):
datasource = form_data.get('datasource', '')
if '__' in datasource:
datasource_id, datasource_type = datasource.split('__')
datasource_id = int(datasource_id)
# The case where the datasource has been deleted
datasource_id = None if datasource_id == 'None' else datasource_id
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how datasource_id == 'None' comes into being in the first place. Can we catch the issue upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I was also confused at the beginning. datasource evaluates to 'None__Table', so splitting that gets 'None'. I'll investigate upstream.

@timifasubaa
Copy link
Contributor Author

timifasubaa commented May 17, 2018

@john-bodley In the case where I found the issue, the user wasn't aware the underlying datasource was missing. I think deleting would have been even more confusing.

@timifasubaa
Copy link
Contributor Author

timifasubaa commented May 17, 2018

@john-bodley @mistercrunch Upon furthher investigation, I think the current approach is best for the following reaosns

  1. datasource_info is the first place the datasource is checked from the form data. And it's the first place

  2. The method intentionally does not fail because the error is handled downstream (https://github.com/apache/incubator-superset/blob/master/superset/viz.py#L64), where the intelligible error message (Viz is missing a datasource) is displayed.

@mistercrunch
Copy link
Member

Can we change the error message to something like "The data source associated with this chart no longer exists"?

Also, should we raise right then (asap) instead of waiting for raising in viz.py

@mistercrunch
Copy link
Member

About cascading, I think that would be the desired behavior as long as there's clarity on what is happening. There should be a confirmation screen with all the associated objects that are going to be deleted, and the person performing the operation should have write/delete access to all these objects.

@timifasubaa
Copy link
Contributor Author

timifasubaa commented May 18, 2018

Agree with raising early. Done that.

I agree with the cascading idea of forcing users to delete all dependent slices before deleting the datasource but I'll push it to a future PR.

@apache apache deleted a comment from timifasubaa May 18, 2018
@timifasubaa timifasubaa force-pushed the fix_error_message_for_missing_datasource branch from ae50763 to f52f7aa Compare May 18, 2018 00:40
@timifasubaa timifasubaa merged commit 5505c11 into apache:master May 18, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…_missing_datasource

fix missing datasource error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants