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

feat: new CSV upload form and API #27840

Merged

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Apr 1, 2024

SUMMARY

Continuing the work on deprecating/removing server side rendered pages, this adds a new React/Antd Form and backend API for CSV uploads.

maintains all the current functionality and adds the following benefits:
Backend:

  • better backend parameter validation.
  • Aligned API with api/v1 and command pattern.
  • Improved error handling.

Frontend:

  • Better looking ;)
  • schemas is a drop down related with the databases dropdown
  • Will initially try to infer the CSV header and load the recognised columns.
Screenshot 2024-04-12 at 14 08 00
  • Columns to read is a multi select dropdown (source from the loaded CSV header)
  • Columns as dates is as multi select dropdown (source from the loaded CSV header)
  • Column index is dropdown (source from the loaded CSV header)
  • Null values is a multi select dropdown pre populated
  • delimiter is now a dropdown
Screen.Recording.2024-04-12.at.17.28.21.mov

Still todo on this PR:

  • Deprecate or remove the old view?
  • migrate the old view permission (depends on the previous item)
  • Remove the old view from the menu
  • After we settle on the UI add more frontend tests

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screenshot 2024-04-05 at 15 42 51

After

Screenshot 2024-04-12 at 16 59 11

TESTING INSTRUCTIONS

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

@john-bodley john-bodley added review:checkpoint Last PR reviewed during the daily review standup review:draft labels Apr 1, 2024
@github-actions github-actions bot added the api Related to the REST API label Apr 2, 2024
@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Apr 2, 2024
@dpgaspar dpgaspar marked this pull request as ready for review April 5, 2024 14:50
@dpgaspar
Copy link
Member Author

dpgaspar commented Apr 5, 2024

/testenv up

Copy link
Contributor

github-actions bot commented Apr 5, 2024

@dpgaspar Ephemeral environment spinning up at http://52.24.73.183:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@dpgaspar dpgaspar requested review from eschutho and yousoph April 5, 2024 15:37
@rusackas rusackas requested a review from justinpark April 5, 2024 16:51
@michael-s-molina
Copy link
Member

2 - Totally agree, I think that the correct design should be imposed at the component level, then we can remove most of the custom styles.ts. So, I don't think that doing more style tweaking on this PR makes sense

Agreed @dpgaspar. We can definitely do a separate PR just to make all these modals look the same and use the same component.

@michael-s-molina
Copy link
Member

Agreed @dpgaspar. We can definitely do a separate PR just to make all these modals look the same and use the same component.

@dpgaspar Assuming this 👆🏼, I think the only design changes left would be to fix the spacing between inputs

Screenshot 2024-04-12 at 10 14 43 Screenshot 2024-04-12 at 10 14 28

and make the tooltip icons vertically aligned.

Screenshot 2024-04-12 at 10 19 02

@dpgaspar
Copy link
Member Author

Agreed @dpgaspar. We can definitely do a separate PR just to make all these modals look the same and use the same component.

@dpgaspar Assuming this 👆🏼, I think the only design changes left would be to fix the spacing between inputs

Screenshot 2024-04-12 at 10 14 43 Screenshot 2024-04-12 at 10 14 28
and make the tooltip icons vertically aligned.

Screenshot 2024-04-12 at 10 19 02

ok, done!

@eschutho
Copy link
Member

/testenv up

Copy link
Contributor

@eschutho Ephemeral environment spinning up at http://34.216.119.147:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dpgaspar!

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for all the work @dpgaspar and for addressing the comments! This is a great improvement to the project 👏🏼

@dpgaspar dpgaspar merged commit 54387b4 into apache:master Apr 15, 2024
30 checks passed
@dpgaspar dpgaspar deleted the danielgaspar/sc-59711/migrate-csv-data-upload branch April 15, 2024 08:38
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

EnxDev pushed a commit to EnxDev/superset that referenced this pull request Apr 15, 2024
qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024
jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:db-migration PRs that require a DB migration size/XXL 🚢 4.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants