arita37 commented on pull request #14449:
URL: https://github.com/apache/superset/pull/14449#issuecomment-892261069
It will be a very very useful features
for many people.
> On Aug 3, 2021, at 23:08, Ville Brofeldt ***@***.***> wrote:
>
>
> @villebro requested changes on this pull request.
>
> Great work! Just a few (hopefully) last comments - I'll prioritize seeing
this through, so as soon as you can make the necessary updates let me know!
>
> In superset/views/database/forms.py:
>
> > +class ColumnarToDatabaseForm(DynamicForm):
> + # pylint: disable=E0211
> + def columnar_allowed_dbs() -> List[Database]: # type: ignore
> + # TODO: change allow_csv_upload to allow_file_upload
> + columnar_enabled_dbs = (
> +
db.session.query(Database).filter_by(allow_csv_upload=True).all()
> + )
> + return [
> + columnar_enabled_db
> + for columnar_enabled_db in columnar_enabled_dbs
> + if ColumnarToDatabaseForm.at_least_one_schema_is_allowed(
> + columnar_enabled_db
> + )
> + ]
> There's unnecessary duplication here: we could abstract this into a class
UploadToDatabaseForm(DynamicForm) with all the repeated boilerplate and then
extend those into class CsvToDatabaseForm(UploadToDatabaseForm), class
ColumnarToDatabaseForm(UploadToDatabaseForm) etc. But this was here before you
already, so not your fault! But if you feel like doing some valuable cleanup
work, it would be great to do some of this cleanup in a follow-up PR.
>
> In
superset/templates/superset/form_view/columnar_to_database_view/edit.html:
>
> > + var db = $("#con");
> + var schema = $("#schema");
> +
> + // this element is a text input
> + // copy it here so it can be reused later
> + var any_schema_is_allowed = schema.clone();
> +
> + update_schemas_allowed_for_columnar_upload(db.val());
> + db.change(function(){
> + update_schemas_allowed_for_columnar_upload(db.val());
> + });
> +
> + function update_schemas_allowed_for_columnar_upload(db_id) {
> + $.ajax({
> + method: "GET",
> + url: "/superset/schemas_access_for_columnar_upload",
> This endpoint is called schemas_access_for_csv_upload and will cause a 404
(this same error happens in the Excel upload form) - either use that or rename
the original to schemas_access_for_file_upload.
>
> In superset/views/database/forms.py:
>
> > + # pylint: disable=E0211
> + def columnar_allowed_dbs() -> List[Database]: # type: ignore
> I wonder why this wasn't @staticmethod?? Can we update that here and in
the other classes
>
> In superset/views/database/forms.py:
>
> > + usecols = JsonListField(
> + _("Use Columns"),
> + default=None,
> + description=_(
> + "Json list of the column names that should be read. "
> + "If not None, only these columns will be read from the file."
> + ),
> + validators=[Optional()],
> + )
> Yes, let's keep this, I think it's a great addition 👍
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.
> Triage notifications on the go with GitHub Mobile for iOS or Android.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
