-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat: Add Excel sheet upload #9825
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9825 +/- ##
==========================================
+ Coverage 66.16% 70.87% +4.70%
==========================================
Files 585 585
Lines 30427 30443 +16
Branches 3152 3152
==========================================
+ Hits 20133 21575 +1442
+ Misses 10113 8757 -1356
+ Partials 181 111 -70
Continue to review full report at Codecov.
|
I need your help guys. With this PR the user is able to upload an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! First pass comments.
Codecov Report
@@ Coverage Diff @@
## master #9825 +/- ##
==========================================
- Coverage 70.49% 69.89% -0.60%
==========================================
Files 594 190 -404
Lines 31386 18474 -12912
Branches 3214 0 -3214
==========================================
- Hits 22126 12913 -9213
+ Misses 9144 5561 -3583
+ Partials 116 0 -116
Continue to review full report at Codecov.
|
@blcksrx at least the following files are involved in the CSV upload feature:
I think Uploading of Excel files probably needs to happen via a dedicated dialog, as many of the CSV fields are not applicable for Excel. Having said that, many are, hence it might make sense to have a parent form with the global options (fail, replace, append etc), and then only add/override the ones that are specific to the format in question. |
@villebro So it needs to do copy and pates many same things for that. right? for example, it needs to add a column |
@blcksrx I think we can assume CSV and Excel importing being equal here, so no need to add a new column in table metadata for that. We should perhaps change the name to "allow upload" or similar. But I don't think that needs to be done now. |
@villebro I got it. thanks |
abfa7b7
to
d74fb4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round comments
8d7510a
to
97c9950
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second pass comments
b3058c9
to
83c18eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from one small fix, LGTM! I think there shouldn't be any changes to requirements.txt
now that Excel is an optional dependency. Also, if you can add a note in UPDATING.md
to say that uploading of Excel Sheets can be enabled by the optional excel
flag, people upgrading will be aware of the new feature.
9c541cf
to
63dfc34
Compare
7cd9506
to
2483631
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes requested to UPDATING.md
, other than that LGTM
87e8770
to
5b11937
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @blcksrx and @villebro thanks for bringing this feature to Superset! Today when I first tried this feature it gave me this error: "AttributeError: 'SupersetSecurityManager' object has no attribute 'database_access'", but when I changed |
Hi. it seems, there are some other commits that above my commit that changes the |
SUMMARY
Upload excel file to create table In addition to the csv
![Screenshot from 2020-05-25 22-23-37](https://user-images.githubusercontent.com/35199552/82835127-5c7c5c80-9ed8-11ea-92d0-539d439460d1.png)
![Screenshot from 2020-05-25 22-23-22](https://user-images.githubusercontent.com/35199552/82835132-5e462000-9ed8-11ea-8649-017ae8a2c3be.png)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION