Skip to content

Commit

Permalink
fix bugs with state
Browse files Browse the repository at this point in the history
  • Loading branch information
@eschutho
eschutho committed Oct 14, 2022
1 parent adbe8f0 commit da02b71
Show file tree
Hide file tree
Showing 4 changed files with 168 additions and 49 deletions.
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ const ExtraOptions = ({
<IndeterminateCheckbox
id="cancel_query_on_windows_unload"
indeterminate={false}
checked={!extraJson?.cancel_query_on_windows_unload}
checked={!!extraJson?.cancel_query_on_windows_unload}
onChange={onExtraInputChange}
labelText={t('Cancel query on window unload event')}
/>
Expand Down
94 changes: 88 additions & 6 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
act,
} from 'spec/helpers/testing-library';
import * as hooks from 'src/views/CRUD/hooks';
import DatabaseModal from './index';
import {
DatabaseObject,
CONFIGURATION_METHOD,
Expand Down Expand Up @@ -658,8 +657,6 @@ describe('DatabaseModal', () => {
checkboxOffSVGs[2],
checkboxOffSVGs[3],
checkboxOffSVGs[4],
checkboxOffSVGs[5],
checkboxOffSVGs[6],
tooltipIcons[0],
tooltipIcons[1],
tooltipIcons[2],
Expand Down Expand Up @@ -688,14 +685,13 @@ describe('DatabaseModal', () => {
allowDbExplorationCheckbox,
disableSQLLabDataPreviewQueriesCheckbox,
];

visibleComponents.forEach(component => {
expect(component).toBeVisible();
});
invisibleComponents.forEach(component => {
expect(component).not.toBeVisible();
});
expect(checkboxOffSVGs).toHaveLength(7);
expect(checkboxOffSVGs).toHaveLength(5);
expect(tooltipIcons).toHaveLength(7);
});

Expand Down Expand Up @@ -1311,6 +1307,7 @@ describe('DatabaseModal', () => {
createResource: jest.fn(),
updateResource: jest.fn(),
clearError: jest.fn(),
setResource: jest.fn(),
});

const renderAndWait = async () => {
Expand Down Expand Up @@ -1355,6 +1352,7 @@ describe('DatabaseModal', () => {
createResource: jest.fn(),
updateResource: jest.fn(),
clearError: jest.fn(),
setResource: jest.fn(),
});

const renderAndWait = async () => {
Expand Down Expand Up @@ -1603,6 +1601,88 @@ describe('dbReducer', () => {
foo: true,
});
});

test('it will change state to payload from input change for checkbox', () => {
const action: DBReducerActionType = {
type: ActionType.inputChange,
payload: { name: 'allow_ctas', type: 'checkbox', checked: false },
};
const currentState = dbReducer(
{
...databaseFixture,
allow_ctas: true,
},
action,
);

expect(currentState).toEqual({
...databaseFixture,
allow_ctas: false,
});
});

test('it will add a parameter', () => {
const action: DBReducerActionType = {
type: ActionType.parametersChange,
payload: { name: 'host', value: '127.0.0.1' },
};
const currentState = dbReducer(databaseFixture, action);

expect(currentState).toEqual({
...databaseFixture,
parameters: {
host: '127.0.0.1',
},
});
});

test('it will add a parameter with existing parameters', () => {
const action: DBReducerActionType = {
type: ActionType.parametersChange,
payload: { name: 'port', value: '1234' },
};
const currentState = dbReducer(
{
...databaseFixture,
parameters: {
host: '127.0.0.1',
},
},
action,
);

expect(currentState).toEqual({
...databaseFixture,
parameters: {
host: '127.0.0.1',
port: '1234',
},
});
});

test('it will change a parameter with existing parameters', () => {
const action: DBReducerActionType = {
type: ActionType.parametersChange,
payload: { name: 'host', value: 'localhost' },
};
const currentState = dbReducer(
{
...databaseFixture,
parameters: {
host: '127.0.0.1',
},
},
action,
);

expect(currentState).toEqual({
...databaseFixture,
parameters: {
host: 'localhost',
},
});
});

test('it will set state to payload from parametersChange with catalog', () => {
const action: DBReducerActionType = {
type: ActionType.parametersChange,
Expand Down Expand Up @@ -1633,7 +1713,9 @@ describe('dbReducer', () => {
expect(currentState).toEqual({
...databaseFixture,
parameters: {
foo: 'bar',
catalog: {
'': 'bar',
},
},
});
});
Expand Down
117 changes: 77 additions & 40 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ import {
} from './styles';
import ModalHeader, { DOCUMENTATION_LINK } from './ModalHeader';

const DEFAULT_EXTRA = JSON.stringify({ allows_virtual_table_explore: true });

const engineSpecificAlertMapping = {
[Engines.GSheet]: {
message: 'Why do I need to create a database?',
Expand Down Expand Up @@ -211,12 +213,12 @@ export function dbReducer(
};
let query = {};
let query_input = '';
let extraJson: ExtraJson;
let parametersCatalog;
const extraJson: ExtraJson = JSON.parse(trimmedState.extra || '{}');

switch (action.type) {
case ActionType.extraEditorChange:
// "extra" payload in state is a string
extraJson = JSON.parse(trimmedState.extra || '{}');
return {
...trimmedState,
extra: JSON.stringify({
Expand All @@ -226,7 +228,6 @@ export function dbReducer(
};
case ActionType.extraInputChange:
// "extra" payload in state is a string
extraJson = JSON.parse(trimmedState.extra || '{}');
if (
action.payload.name === 'schema_cache_timeout' ||
action.payload.name === 'table_cache_timeout'
Expand Down Expand Up @@ -275,26 +276,44 @@ export function dbReducer(
[action.payload.name]: action.payload.value,
};
case ActionType.parametersChange:
if (
trimmedState.catalog !== undefined &&
action.payload.type?.startsWith('catalog')
) {
// Formatting wrapping google sheets table catalog
const idx = action.payload.type?.split('-')[1];
const catalogToUpdate = trimmedState?.catalog[idx] || {};
catalogToUpdate[action.payload.name] = action.payload.value;

const paramatersCatalog = {};
// eslint-disable-next-line array-callback-return
trimmedState.catalog?.map((item: CatalogObject) => {
paramatersCatalog[item.name] = item.value;
});

if (action.payload.type?.startsWith('catalog')) {
if (trimmedState.catalog !== undefined) {
// Formatting wrapping google sheets table catalog
const catalogCopy: CatalogObject[] = [...trimmedState.catalog];
const idx = action.payload.type?.split('-')[1];
const catalogToUpdate: CatalogObject = catalogCopy[idx] || {};
catalogToUpdate[action.payload.name] = action.payload.value;

// insert updated catalog to existing state
catalogCopy.splice(parseInt(idx, 10), 1, catalogToUpdate);

// format catalog for state
// eslint-disable-next-line array-callback-return
parametersCatalog = catalogCopy.reduce((obj, item: any) => {
const catalog = { ...obj };
catalog[item.name] = item.value;
return catalog;
}, {});

return {
...trimmedState,
catalog: catalogCopy,
parameters: {
...trimmedState.parameters,
catalog: parametersCatalog,
},
};
}
if (action.payload.name === 'name') {
parametersCatalog = { [action.payload.value as string]: undefined };
} else {
parametersCatalog = { '': action.payload.value };
}
return {
...trimmedState,
parameters: {
...trimmedState.parameters,
catalog: paramatersCatalog,
catalog: parametersCatalog,
},
};
}
Expand All @@ -305,6 +324,7 @@ export function dbReducer(
[action.payload.name]: action.payload.value,
},
};

case ActionType.addTableCatalogSheet:
if (trimmedState.catalog !== undefined) {
return {
Expand Down Expand Up @@ -353,21 +373,25 @@ export function dbReducer(
CONFIGURATION_METHOD.DYNAMIC_FORM
) {
// "extra" payload from the api is a string
extraJson = {
const extraJsonPayload: ExtraJson = {
...JSON.parse((action.payload.extra as string) || '{}'),
};
const engineParamsCatalog = Object.entries(
extraJson?.engine_params?.catalog || {},
).map(([key, value]) => ({
name: key,
value,
}));

const payloadCatalog = extraJsonPayload.engine_params?.catalog;

const engineRootCatalog = Object.entries(payloadCatalog || {}).map(
([name, value]: string[]) => ({ name, value }),
);

return {
...action.payload,
engine: action.payload.backend || trimmedState.engine,
configuration_method: action.payload.configuration_method,
catalog: engineParamsCatalog,
parameters: action.payload.parameters || trimmedState.parameters,
catalog: engineRootCatalog,
parameters: {
...(action.payload.parameters || trimmedState.parameters),
catalog: payloadCatalog,
},
query_input,
};
}
Expand All @@ -381,6 +405,12 @@ export function dbReducer(
};

case ActionType.dbSelected:
// set initial state for blank form
return {
...action.payload,
extra: DEFAULT_EXTRA,
expose_in_sqllab: true,
};
case ActionType.configMethodChange:
return {
...action.payload,
Expand Down Expand Up @@ -545,20 +575,17 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// Clone DB object
const dbToUpdate = { ...(db || {}) };

if (dbToUpdate.catalog) {
// convert catalog to fit /validate_parameters endpoint
dbToUpdate.catalog = Object.assign(
{},
...dbToUpdate.catalog.map((x: { name: string; value: string }) => ({
[x.name]: x.value,
})),
);
} else {
dbToUpdate.catalog = {};
}

if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) {
// Validate DB before saving
if (dbToUpdate?.parameters?.catalog) {
// need to stringify gsheets catalog to allow it to be serialized
dbToUpdate.extra = JSON.stringify({
...JSON.parse(dbToUpdate.extra || '{}'),
engine_params: {
catalog: dbToUpdate.parameters.catalog,
},
});
}
const errors = await getValidation(dbToUpdate, true);
if ((validationErrors && !isEmpty(validationErrors)) || errors) {
return;
Expand Down Expand Up @@ -607,6 +634,16 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
}
}

if (dbToUpdate?.parameters?.catalog) {
// need to stringify gsheets catalog to allow it to be seralized
dbToUpdate.extra = JSON.stringify({
...JSON.parse(dbToUpdate.extra || '{}'),
engine_params: {
catalog: dbToUpdate.parameters.catalog,
},
});
}

setLoading(true);
if (db?.id) {
const result = await updateResource(
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/views/CRUD/data/database/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export type DatabaseObject = {
credentials_info?: string;
service_account_info?: string;
query?: Record<string, string>;
catalog?: Record<string, string>;
catalog?: Record<string, string | undefined>;
properties?: Record<string, any>;
warehouse?: string;
role?: string;
Expand Down Expand Up @@ -158,7 +158,7 @@ export interface ExtraJson {
cost_estimate_enabled?: boolean; // in SQL Lab
disable_data_preview?: boolean; // in SQL Lab
engine_params?: {
catalog?: Record<any, any>;
catalog?: Record<string, string>;
connect_args?: {
http_path?: string;
};
Expand Down

0 comments on commit da02b71

Please sign in to comment.