feat: new CSV upload form and API

[dpgaspar]

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.

• 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

https://github.com/apache/superset/assets/4025227/30c25b35-3a56-4d3c-a8ef-5922780ccfa5

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:

After

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

[dpgaspar]

/testenv up

[github-actions[bot]]

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

[sfirke]

This looks great! It gets me wondering about "upload columnar file to database" and "upload Excel file to database." My users mostly use the Excel one. Would some aspects of this overhaul be applicable to those upload options?

[dpgaspar]

This looks great! It gets me wondering about "upload columnar file to database" and "upload Excel file to database." My users mostly use the Excel one. Would some aspects of this overhaul be applicable to those upload options?

I'll do the excel and columnar modals next, and yes they should be applicable then

[kasiazjc]

Thanks @michael-s-molina for the review and @dpgaspar for working on this!

1 - I know it's not part of this PR, but could you use this opportunity to improve how the menu is displaying the disabled state? Right now, when hovering, it's showing the text as clickable, and when you click, nothing happens. In my opinion, disabled items should be always grey. @kasiazjc

agreed, disabled items should always be grayed out and show 🚫 cursor instead of pointy finger, not sure if it can be easily done globally though?

2 - Can you match the modal design with the alerts/reports?

I believe it's important to match the same width, dropdown arrow positions, etc. @kasiazjc

yes, all of the properties should be the same as in the alerts/reports. Additionally, each select/input should be in its own row (like in alerts modal)

4 - The Select button should be one of our predefined buttons and it looks a little lost on the screen. I think once we change the modal width and rearrange the fields, it will look better. @kasiazjc

Agreed (in the mocks below)

5 - How does loaded columns work with 30+ columns? @kasiazjc can we make sure we cover this case?

@dpgaspar I think columns should be under the loaded file, with ability to uncollapse/collapse the columns (if more than fits in a row) so it would be something like this (perfect scenario):

I would also skip "basic" header (if it's not needed from you point of view) + descriptions under the fields in general section (for schema we can move it to the tooltip)

What do you think? Is it doable? (also do not mind things like labels not looking like what we have in the code)

[michael-s-molina]

@kasiazjc @dpgaspar One thing I forgot to mention was about the loaded columns. Apart from their names, their types are equality important because they will inform users if they need to apply any transformation before loading them. It's the same principle we have when we create a dataset:

We also have a Column Data Types field that allows type manipulation:

@kasiazjc I think we can resolve both requirements if we present a similar table that allows users to edit their types as they would using the Column Data Types field.

[dpgaspar]

@kasiazjc @dpgaspar One thing I forgot to mention was about the loaded columns. Apart from their names, their types are equality important because they will inform users if they need to apply any transformation before loading them. It's the same principle we have when we create a dataset:

We also have a Column Data Types field that allows type manipulation: @kasiazjc I think we can resolve both requirements if we present a similar table that allows users to edit their types as they would using the Column Data Types field.

On the loaded columns we can't infer the type yet, as for the Column Data Types field I would vote on improving that field on another PR

[dpgaspar]

@michael-s-molina @kasiazjc I've addressed basically all your comments:

@michael-s-molina 1 - Fixed the display disabled at the component level 2 - fixed the width, did not fix the dropdown icons etc because I want to avoid customizing the colapse panels (so it follows database modal) 3 - done 4 - done 5 - Followed @kasiazjc comment

6 - Agree removed the code

[eschutho]

/testenv up

[github-actions[bot]]

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

[dpgaspar]

Thank you for the improvements @dpgaspar. Here are the comments for the second review:

1 - Fixed the display disabled at the component level

The cursor is correctly displaying the forbidden sign but the items are still becoming blue when hovered which I don't think should be the case because it gives the impression that they leave the forbidden state and are now clickable. I also noticed that the first item is not disabled. |

2 - fixed the width, did not fix the dropdown icons etc because I want to avoid customizing the colapse panels (so it follows database modal)

@kasiazjc @eschutho @dpgaspar We just need to decide which design is the correct one and apply it consistently:

We need to match spacing between header and description, borders, arrow position, the X color. These modals should all be using the same component to render the collapse panels.

3 - done

Would be possible to move all toggles to last, grouping them, to improve the design? I think you can do this for all collapse panels.

> 5 - Followed @kasiazjc comment

The columns are breaking instead of overflowing to the next line. I also think we need some space between the lines.

@kasiazjc Shouldn't we use the +N pattern that we use for chart's list and the Select component instead of the black arrow? 7 - I think these should be in just one row and the tooltip icon needs to be vertically aligned. > On the loaded columns we can't infer the type yet, as for the Column Data Types field I would vote on improving that field on another PR

I'm ok with doing that in another PR. Inferring column types would be a great addition.

1 - The first item is disabled, I'll remove the hovered blue.

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

3 - Sure!

5 - Will do, @kasiazjc do you agree?

7 - Will do.

Thank you for the great reviews!

[michael-s-molina]

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]

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

and make the tooltip icons vertically aligned.

[dpgaspar]

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

and make the tooltip icons vertically aligned.

ok, done!

[eschutho]

/testenv up

[github-actions[bot]]

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

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.