You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2021/11/22 19:59:23 UTC
[superset] branch 1.4 updated (7d9f63e -> 0b8507f)
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a change to branch 1.4
in repository https://gitbox.apache.org/repos/asf/superset.git.
from 7d9f63e fix: add fallback and validation for report and cron timezones (#17338)
new c015e66 fix: clear 'delete' confirmation (#17345)
new 0b8507f fix: import should accept old keys (#17330)
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../components/DeleteModal/DeleteModal.test.tsx | 15 +++++++++-
.../src/components/DeleteModal/index.tsx | 34 +++++++++++++++++----
superset/databases/commands/export.py | 17 ++++++++++-
superset/databases/commands/importers/v1/utils.py | 7 +++++
superset/databases/schemas.py | 32 +++++++++++++++++---
.../integration_tests/databases/commands_tests.py | 35 ++++++++++++++++++++++
6 files changed, 128 insertions(+), 12 deletions(-)
[superset] 02/02: fix: import should accept old keys (#17330)
Posted by el...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch 1.4
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0b8507fe77d1f96cdd70cdc21c19bb7e94449e4c
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Wed Nov 3 11:25:30 2021 -0700
fix: import should accept old keys (#17330)
* fix: import should accept old keys
* Fix lint
* Preserve V1 schema
---
superset/databases/commands/export.py | 17 ++++++++++-
superset/databases/commands/importers/v1/utils.py | 7 +++++
superset/databases/schemas.py | 32 +++++++++++++++++---
.../integration_tests/databases/commands_tests.py | 35 ++++++++++++++++++++++
4 files changed, 86 insertions(+), 5 deletions(-)
diff --git a/superset/databases/commands/export.py b/superset/databases/commands/export.py
index 786eb56..134bda5 100644
--- a/superset/databases/commands/export.py
+++ b/superset/databases/commands/export.py
@@ -65,10 +65,25 @@ class ExportDatabasesCommand(ExportModelsCommand):
include_defaults=True,
export_uuids=True,
)
+
+ # https://github.com/apache/superset/pull/16756 renamed ``allow_csv_upload``
+ # to ``allow_file_upload`, but we can't change the V1 schema
+ replacements = {"allow_file_upload": "allow_csv_upload"}
+ # this preserves key order, which is important
+ payload = {replacements.get(key, key): value for key, value in payload.items()}
+
# TODO (betodealmeida): move this logic to export_to_dict once this
# becomes the default export endpoint
if payload.get("extra"):
- payload["extra"] = parse_extra(payload["extra"])
+ extra = payload["extra"] = parse_extra(payload["extra"])
+
+ # ``schemas_allowed_for_csv_upload`` was also renamed to
+ # ``schemas_allowed_for_file_upload``, we need to change to preserve the
+ # V1 schema
+ if "schemas_allowed_for_file_upload" in extra:
+ extra["schemas_allowed_for_csv_upload"] = extra.pop(
+ "schemas_allowed_for_file_upload"
+ )
payload["version"] = EXPORT_VERSION
diff --git a/superset/databases/commands/importers/v1/utils.py b/superset/databases/commands/importers/v1/utils.py
index 6e016d0..6704ccd 100644
--- a/superset/databases/commands/importers/v1/utils.py
+++ b/superset/databases/commands/importers/v1/utils.py
@@ -32,6 +32,13 @@ def import_database(
return existing
config["id"] = existing.id
+ # https://github.com/apache/superset/pull/16756 renamed ``csv`` to ``file``.
+ config["allow_file_upload"] = config.pop("allow_csv_upload")
+ if "schemas_allowed_for_csv_upload" in config["extra"]:
+ config["extra"]["schemas_allowed_for_file_upload"] = config["extra"].pop(
+ "schemas_allowed_for_csv_upload"
+ )
+
# TODO (betodealmeida): move this logic to import_from_dict
config["extra"] = json.dumps(config["extra"])
diff --git a/superset/databases/schemas.py b/superset/databases/schemas.py
index 763e7db..6599e8d 100644
--- a/superset/databases/schemas.py
+++ b/superset/databases/schemas.py
@@ -558,11 +558,19 @@ class ImportV1DatabaseExtraSchema(Schema):
self, data: Dict[str, Any], **kwargs: Any
) -> Dict[str, Any]:
"""
- Fix ``schemas_allowed_for_csv_upload`` being a string.
-
- Due to a bug in the database modal, some databases might have been
- saved and exported with a string for ``schemas_allowed_for_csv_upload``.
+ Fixes for ``schemas_allowed_for_csv_upload``.
"""
+ # Fix for https://github.com/apache/superset/pull/16756, which temporarily
+ # changed the V1 schema. We need to support exports made after that PR and
+ # before this PR.
+ if "schemas_allowed_for_file_upload" in data:
+ data["schemas_allowed_for_csv_upload"] = data.pop(
+ "schemas_allowed_for_file_upload"
+ )
+
+ # Fix ``schemas_allowed_for_csv_upload`` being a string.
+ # Due to a bug in the database modal, some databases might have been
+ # saved and exported with a string for ``schemas_allowed_for_csv_upload``.
schemas_allowed_for_csv_upload = data.get("schemas_allowed_for_csv_upload")
if isinstance(schemas_allowed_for_csv_upload, str):
data["schemas_allowed_for_csv_upload"] = json.loads(
@@ -579,6 +587,22 @@ class ImportV1DatabaseExtraSchema(Schema):
class ImportV1DatabaseSchema(Schema):
+ # pylint: disable=no-self-use, unused-argument
+ @pre_load
+ def fix_allow_csv_upload(
+ self, data: Dict[str, Any], **kwargs: Any
+ ) -> Dict[str, Any]:
+ """
+ Fix for ``allow_csv_upload`` .
+ """
+ # Fix for https://github.com/apache/superset/pull/16756, which temporarily
+ # changed the V1 schema. We need to support exports made after that PR and
+ # before this PR.
+ if "allow_file_upload" in data:
+ data["allow_csv_upload"] = data.pop("allow_file_upload")
+
+ return data
+
database_name = fields.String(required=True)
sqlalchemy_uri = fields.String(required=True)
password = fields.String(allow_none=True)
diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py
index 449dd4c..389b1ac 100644
--- a/tests/integration_tests/databases/commands_tests.py
+++ b/tests/integration_tests/databases/commands_tests.py
@@ -338,6 +338,41 @@ class TestImportDatabasesCommand(SupersetTestCase):
db.session.delete(database)
db.session.commit()
+ def test_import_v1_database_broken_csv_fields(self):
+ """
+ Test that a database can be imported with broken schema.
+
+ https://github.com/apache/superset/pull/16756 renamed some fields, changing
+ the V1 schema. This test ensures that we can import databases that were
+ exported with the broken schema.
+ """
+ broken_config = database_config.copy()
+ broken_config["allow_file_upload"] = broken_config.pop("allow_csv_upload")
+ broken_config["extra"] = {"schemas_allowed_for_file_upload": ["upload"]}
+
+ contents = {
+ "metadata.yaml": yaml.safe_dump(database_metadata_config),
+ "databases/imported_database.yaml": yaml.safe_dump(broken_config),
+ }
+ command = ImportDatabasesCommand(contents)
+ command.run()
+
+ database = (
+ db.session.query(Database).filter_by(uuid=database_config["uuid"]).one()
+ )
+ assert database.allow_file_upload
+ assert database.allow_ctas
+ assert database.allow_cvas
+ assert not database.allow_run_async
+ assert database.cache_timeout is None
+ assert database.database_name == "imported_database"
+ assert database.expose_in_sqllab
+ assert database.extra == '{"schemas_allowed_for_file_upload": ["upload"]}'
+ assert database.sqlalchemy_uri == "sqlite:///test.db"
+
+ db.session.delete(database)
+ db.session.commit()
+
def test_import_v1_database_multiple(self):
"""Test that a database can be imported multiple times"""
num_databases = db.session.query(Database).count()
[superset] 01/02: fix: clear 'delete' confirmation (#17345)
Posted by el...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
elizabeth pushed a commit to branch 1.4
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c015e661a90cb08eb846d94fa7065c18f0165a2a
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Thu Nov 4 16:26:38 2021 -0700
fix: clear 'delete' confirmation (#17345)
* fix: clear 'delete' confirmation
* Add tests
(cherry picked from commit 43f4ab845a9d0c5b70a58b1596319b638081ce54)
---
.../components/DeleteModal/DeleteModal.test.tsx | 15 +++++++++-
.../src/components/DeleteModal/index.tsx | 34 ++++++++++++++++++----
2 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/superset-frontend/src/components/DeleteModal/DeleteModal.test.tsx b/superset-frontend/src/components/DeleteModal/DeleteModal.test.tsx
index 61c4a9a..6cec92e 100644
--- a/superset-frontend/src/components/DeleteModal/DeleteModal.test.tsx
+++ b/superset-frontend/src/components/DeleteModal/DeleteModal.test.tsx
@@ -44,13 +44,23 @@ test('Calling "onHide"', () => {
onHide: jest.fn(),
open: true,
};
- render(<DeleteModal {...props} />);
+ const modal = <DeleteModal {...props} />;
+ render(modal);
expect(props.onHide).toBeCalledTimes(0);
expect(props.onConfirm).toBeCalledTimes(0);
+
+ // type "del" in the input
+ userEvent.type(screen.getByTestId('delete-modal-input'), 'del');
+ expect(screen.getByTestId('delete-modal-input')).toHaveValue('del');
+
+ // close the modal
expect(screen.getByText('×')).toBeVisible();
userEvent.click(screen.getByText('×'));
expect(props.onHide).toBeCalledTimes(1);
expect(props.onConfirm).toBeCalledTimes(0);
+
+ // confirm input has been cleared
+ expect(screen.getByTestId('delete-modal-input')).toHaveValue('');
});
test('Calling "onConfirm" only after typing "delete" in the input', () => {
@@ -75,4 +85,7 @@ test('Calling "onConfirm" only after typing "delete" in the input', () => {
userEvent.type(screen.getByTestId('delete-modal-input'), 'delete');
userEvent.click(screen.getByText('delete'));
expect(props.onConfirm).toBeCalledTimes(1);
+
+ // confirm input has been cleared
+ expect(screen.getByTestId('delete-modal-input')).toHaveValue('');
});
diff --git a/superset-frontend/src/components/DeleteModal/index.tsx b/superset-frontend/src/components/DeleteModal/index.tsx
index 98639cd..f113903 100644
--- a/superset-frontend/src/components/DeleteModal/index.tsx
+++ b/superset-frontend/src/components/DeleteModal/index.tsx
@@ -52,12 +52,35 @@ export default function DeleteModal({
title,
}: DeleteModalProps) {
const [disableChange, setDisableChange] = useState(true);
+ const [confirmation, setConfirmation] = useState<string>('');
+
+ const hide = () => {
+ setConfirmation('');
+ onHide();
+ };
+
+ const confirm = () => {
+ setConfirmation('');
+ onConfirm();
+ };
+
+ const onChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+ const targetValue = event.target.value ?? '';
+ setDisableChange(targetValue.toUpperCase() !== t('DELETE'));
+ setConfirmation(targetValue);
+ };
+
+ const onPressEnter = () => {
+ if (!disableChange) {
+ confirm();
+ }
+ };
return (
<Modal
disablePrimaryButton={disableChange}
- onHide={onHide}
- onHandledPrimaryAction={onConfirm}
+ onHide={hide}
+ onHandledPrimaryAction={confirm}
primaryButtonName={t('delete')}
primaryButtonType="danger"
show={open}
@@ -73,10 +96,9 @@ export default function DeleteModal({
type="text"
id="delete"
autoComplete="off"
- onChange={(event: React.ChangeEvent<HTMLInputElement>) => {
- const targetValue = event.target.value ?? '';
- setDisableChange(targetValue.toUpperCase() !== t('DELETE'));
- }}
+ value={confirmation}
+ onChange={onChange}
+ onPressEnter={onPressEnter}
/>
</StyledDiv>
</Modal>