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.

Sign up for GitHub

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

fix: clear modal state after adding dataset #17044

Merged
merged 3 commits into from
Oct 10, 2021
Merged

fix: clear modal state after adding dataset #17044

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc406cd
fix: clear modal state after adding dataset
betodealmeida Oct 8, 2021
de9481c
Fix test
betodealmeida Oct 8, 2021
0844add
Small fixes
betodealmeida Oct 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion superset-frontend/src/components/DatabaseSelector/DatabaseSelector.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,12 @@ import DatabaseSelector from '.';
const SupersetClientGet = jest.spyOn(SupersetClient, 'get');

const createProps = () => ({
db: { id: 1, database_name: 'test', backend: 'test-postgresql' },
db: {
id: 1,
database_name: 'test',
backend: 'test-postgresql',
allow_multi_schema_metadata_fetch: false,
},
formMode: false,
isDatabaseSelectEnabled: true,
readOnly: false,
Expand Down Expand Up @@ -246,6 +251,7 @@ test('Sends the correct db when changing the database', async () => {
id: 2,
database_name: 'test-mysql',
backend: 'mysql',
allow_multi_schema_metadata_fetch: false,
}),
),
);
Expand Down
44 changes: 24 additions & 20 deletions superset-frontend/src/components/DatabaseSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,21 +63,25 @@ type DatabaseValue = {
id: number;
database_name: string;
backend: string;
allow_multi_schema_metadata_fetch: boolean;
};

export type DatabaseObject = {
id: number;
database_name: string;
backend: string;
allow_multi_schema_metadata_fetch: boolean;
};

type SchemaValue = { label: string; value: string };

interface DatabaseSelectorProps {
db?: { id: number; database_name: string; backend: string };
db?: DatabaseObject;
formMode?: boolean;
getDbList?: (arg0: any) => {};
handleError: (msg: string) => void;
isDatabaseSelectEnabled?: boolean;
onDbChange?: (db: {
id: number;
database_name: string;
backend: string;
}) => void;
onDbChange?: (db: DatabaseObject) => void;
onSchemaChange?: (schema?: string) => void;
onSchemasLoad?: (schemas: Array<object>) => void;
readOnly?: boolean;
Expand Down Expand Up @@ -165,20 +169,20 @@ export default function DatabaseSelector({
if (result.length === 0) {
handleError(t("It seems you don't have access to any database"));
}
const options = result.map(
(row: { id: number; database_name: string; backend: string }) => ({
label: (
<SelectLabel
backend={row.backend}
databaseName={row.database_name}
/>
),
value: row.id,
id: row.id,
database_name: row.database_name,
backend: row.backend,
}),
);
const options = result.map((row: DatabaseObject) => ({
label: (
<SelectLabel
backend={row.backend}
databaseName={row.database_name}
/>
),
value: row.id,
id: row.id,
database_name: row.database_name,
backend: row.backend,
allow_multi_schema_metadata_fetch:
row.allow_multi_schema_metadata_fetch,
}));
return {
data: options,
totalCount: options.length,
Expand Down
49 changes: 24 additions & 25 deletions superset-frontend/src/components/TableSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ import { styled, SupersetClient, t } from '@superset-ui/core';
import { Select } from 'src/components';
import { FormLabel } from 'src/components/Form';
import Icons from 'src/components/Icons';
import DatabaseSelector from 'src/components/DatabaseSelector';
import DatabaseSelector, {
DatabaseObject,
} from 'src/components/DatabaseSelector';
import RefreshLabel from 'src/components/RefreshLabel';
import CertifiedIcon from 'src/components/CertifiedIcon';
import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip';
Expand Down Expand Up @@ -76,22 +78,12 @@ const TableLabel = styled.span`

interface TableSelectorProps {
clearable?: boolean;
database?: {
id: number;
database_name: string;
backend: string;
allow_multi_schema_metadata_fetch: boolean;
};
dbId: number;
database?: DatabaseObject;
formMode?: boolean;
getDbList?: (arg0: any) => {};
handleError: (msg: string) => void;
isDatabaseSelectEnabled?: boolean;
onDbChange?: (db: {
id: number;
database_name: string;
backend: string;
}) => void;
onDbChange?: (db: DatabaseObject) => void;
onSchemaChange?: (schema?: string) => void;
onSchemasLoad?: () => void;
onTableChange?: (tableName?: string, schema?: string) => void;
Expand Down Expand Up @@ -150,7 +142,6 @@ const TableOption = ({ table }: { table: Table }) => {

const TableSelector: FunctionComponent<TableSelectorProps> = ({
database,
dbId,
formMode = false,
getDbList,
handleError,
Expand All @@ -165,7 +156,9 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
sqlLabMode = true,
tableName,
}) => {
const [currentDbId, setCurrentDbId] = useState<number | undefined>(dbId);
const [currentDatabase, setCurrentDatabase] = useState<
DatabaseObject | undefined
>(database);
const [currentSchema, setCurrentSchema] = useState<string | undefined>(
schema,
);
Expand All @@ -176,13 +169,22 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
const [tableOptions, setTableOptions] = useState<TableOption[]>([]);

useEffect(() => {
if (currentDbId && currentSchema) {
// reset selections
if (database === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

could this ever be null? Do you want to check a more generic falsy value iow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I'll check for any falsy values.

setCurrentDatabase(undefined);
setCurrentSchema(undefined);
setCurrentTable(undefined);
}
}, [database]);

useEffect(() => {
if (currentDatabase && currentSchema) {
setLoadingTables(true);
const encodedSchema = encodeURIComponent(currentSchema);
const forceRefresh = refresh !== previousRefresh;
// TODO: Would be nice to add pagination in a follow-up. Needs endpoint changes.
const endpoint = encodeURI(
`/superset/tables/${currentDbId}/${encodedSchema}/undefined/${forceRefresh}/`,
`/superset/tables/${currentDatabase.id}/${encodedSchema}/undefined/${forceRefresh}/`,
);

if (previousRefresh !== refresh) {
Expand Down Expand Up @@ -221,7 +223,7 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
// We are using the refresh state to re-trigger the query
// previousRefresh should be out of dependencies array
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [currentDbId, currentSchema, onTablesLoad, refresh]);
}, [currentDatabase, currentSchema, onTablesLoad, refresh]);

function renderSelectRow(select: ReactNode, refreshBtn: ReactNode) {
return (
Expand All @@ -239,12 +241,8 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
}
};

const internalDbChange = (db: {
id: number;
database_name: string;
backend: string;
}) => {
setCurrentDbId(db?.id);
const internalDbChange = (db: DatabaseObject) => {
setCurrentDatabase(db);
if (onDbChange) {
onDbChange(db);
}
Expand All @@ -261,7 +259,8 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
function renderDatabaseSelector() {
return (
<DatabaseSelector
db={database}
key={currentDatabase?.id}
db={currentDatabase}
formMode={formMode}
getDbList={getDbList}
handleError={handleError}
Expand Down
43 changes: 27 additions & 16 deletions superset-frontend/src/views/CRUD/data/dataset/AddDatasetModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { isEmpty, isNil } from 'lodash';
import Modal from 'src/components/Modal';
import TableSelector from 'src/components/TableSelector';
import withToasts from 'src/components/MessageToasts/withToasts';
import { DatabaseObject } from 'src/components/DatabaseSelector';

type DatasetAddObject = {
id: number;
Expand Down Expand Up @@ -50,9 +51,11 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
onHide,
show,
}) => {
const [currentDatabase, setCurrentDatabase] = useState<
DatabaseObject | undefined
>();
const [currentSchema, setSchema] = useState<string | undefined>('');
const [currentTableName, setTableName] = useState('');
const [datasourceId, setDatasourceId] = useState<number>(0);
const [disableSave, setDisableSave] = useState(true);
const { createResource } = useSingleViewResource<Partial<DatasetAddObject>>(
'dataset',
Expand All @@ -61,15 +64,11 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
);

useEffect(() => {
setDisableSave(isNil(datasourceId) || isEmpty(currentTableName));
}, [currentTableName, datasourceId]);
setDisableSave(isNil(currentDatabase) || isEmpty(currentTableName));
Copy link
Member

Choose a reason for hiding this comment

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

should this be isEmpty(currentDatabase)?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can get rid of lodash here, let me do it.

}, [currentTableName, currentDatabase]);

const onDbChange = (db: {
id: number;
database_name: string;
backend: string;
}) => {
setDatasourceId(db.id);
const onDbChange = (db: DatabaseObject) => {
setCurrentDatabase(db);
};

const onSchemaChange = (schema?: string) => {
Expand All @@ -80,9 +79,21 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
setTableName(tableName);
};

const clearModal = () => {
setSchema('');
setTableName('');
setCurrentDatabase(undefined);
setDisableSave(true);
};

const hide = () => {
clearModal();
onHide();
};

const onSave = () => {
const data = {
database: datasourceId,
database: currentDatabase?.id || 0,
Copy link
Member

Choose a reason for hiding this comment

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

what does setting the default to 0 do in this case, rather than undefined or null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me clean this up... here currentDatabase will always be defined, otherwise you can't save.

...(currentSchema ? { schema: currentSchema } : {}),
table_name: currentTableName,
};
Expand All @@ -94,30 +105,30 @@ const DatasetModal: FunctionComponent<DatasetModalProps> = ({
onDatasetAdd({ id: response.id, ...response });
}
addSuccessToast(t('The dataset has been saved'));
onHide();
hide();
});
};

return (
<Modal
disablePrimaryButton={disableSave}
onHandledPrimaryAction={onSave}
onHide={onHide}
onHide={hide}
primaryButtonName={t('Add')}
show={show}
title={t('Add dataset')}
>
<TableSelectorContainer>
<TableSelector
clearable={false}
dbId={datasourceId}
formMode
handleError={addDangerToast}
database={currentDatabase}
schema={currentSchema}
tableName={currentTableName}
onDbChange={onDbChange}
onSchemaChange={onSchemaChange}
onTableChange={onTableChange}
schema={currentSchema}
tableName={currentTableName}
handleError={addDangerToast}
/>
</TableSelectorContainer>
</Modal>
Expand Down