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

Jump to bottom

feat: Gsheet adding sheets component #15598

Closed
wants to merge 7 commits into from
Closed

feat: Gsheet adding sheets component #15598

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb5f607
first draft
AAfghahi Jul 8, 2021
459eba3
added table_catalog
AAfghahi Jul 9, 2021
9aba8fc
remove console.log
hughhhh Jul 12, 2021
6b30f79
Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/D…
AAfghahi Jul 13, 2021
962e597
Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/i…
AAfghahi Jul 13, 2021
0e4f249
revisions
AAfghahi Jul 13, 2021
84222b2
revisions
AAfghahi Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
202 changes: 125 additions & 77 deletions superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export const FormFieldOrder = [
'password',
'database_name',
'credentials_info',
'table_catalog',
'query',
'encryption',
];
Expand All @@ -66,23 +67,27 @@ interface FieldPropTypes {
sslForced?: boolean;
defaultDBName?: string;
editNewDb?: boolean;
setPublicSheets: (value: string) => void;
isPublic?: boolean;
}

const CredentialsInfo = ({
changeMethods,
isEditMode,
db,
editNewDb,
isPublic,
}: FieldPropTypes) => {
const [uploadOption, setUploadOption] = useState<number>(
CredentialInfoOptions.jsonUpload.valueOf(),
);
const [fileToUpload, setFileToUpload] = useState<string | null | undefined>(
null,
);
console.log('in credentials', isPublic);
Copy link
Member

Choose a reason for hiding this comment

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

remove

return (
<CredentialInfoForm>
{!isEditMode && (
{!isEditMode && db?.engine === 'bigquery' && (
Copy link
Member

Choose a reason for hiding this comment

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

By making these conditionals by engine, we're heading towards what we tried to avoid originally which was to make a large if/then block with a bunch of engines. Can we instead make this more dynamic and look for some property of the engine that is relevant to the code below, like uploadOption?

Copy link
Member

Choose a reason for hiding this comment

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

We currently return this for BQ:

{
  "available_drivers": [
    "bigquery"
  ],
  "default_driver": "bigquery",
  "engine": "bigquery",
  "name": "Google BigQuery",
  "parameters": {
    "properties": {
      "credentials_info": {
        "description": "Contents of BigQuery JSON credentials.",
        "type": "string",
        "x-encrypted-extra": true
      }
    },
    "required": [
      "credentials_info"
    ],
    "type": "object"
  },
  "preferred": false,
  "sqlalchemy_uri_placeholder": "bigquery://{project_id}"
}

Note how for credentials_info we have extra metadata informing that the value should be sent via encrypted_extra:

      "credentials_info": {
        "description": "Contents of BigQuery JSON credentials.",
        "type": "string",
        "x-encrypted-extra": true
      }

We can add more extra metadata in the BE to inform the FE that this should be an upload/past form, eg:

      "credentials_info": {
        "description": "Contents of BigQuery JSON credentials.",
        "type": "string",
        "x-encrypted-extra": true,
        "x-special-input-types": ["paste", "upload"]
      }

So that the FE knows to render the component without hardcoding the engine.

For GSheets this is a bit more complicated, because we want to have conditional rendering of the component. Not sure how to proceed there.

@eschutho, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why we have to use ‘credentials_info’ as the key for the field? For most fields, that key is the return value for the api, but since we’re renaming it, can’t we call it something like encrypted_credentials?

Copy link
Member

Choose a reason for hiding this comment

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

Right, we could call it anything we want.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Then encrypted_credentials could be a paste/upload component.. no need for the api to define it.

<>
<FormLabel required>
{t('How do you want to enter service account credentials?')}
Expand All @@ -104,9 +109,10 @@ const CredentialsInfo = ({
)}
{uploadOption === CredentialInfoOptions.copyPaste ||
isEditMode ||
editNewDb ? (
editNewDb ||
(db?.engine === 'gsheets' && !isPublic) ? (
Copy link
Member

Choose a reason for hiding this comment

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

same thing here. We want to avoid a bunch of if/thens by engine.

<div className="input-container">
<FormLabel required>{t('Service Account')}</FormLabel>
<FormLabel required>{t('Service Account Information')}</FormLabel>
<textarea
className="input-form"
name="credentials_info"
Expand All @@ -115,78 +121,90 @@ const CredentialsInfo = ({
placeholder="Paste content of service credentials JSON file here"
/>
<span className="label-paste">
{t('Copy and paste the entire service account .json file here')}
{t(
'Copy and paste the entire service account .json file here to enable connection',
eschutho marked this conversation as resolved.
Show resolved Hide resolved
)}
</span>
</div>
) : (
<div
className="input-container"
css={(theme: SupersetTheme) => infoTooltip(theme)}
>
<div css={{ display: 'flex', alignItems: 'center' }}>
<FormLabel required>{t('Upload Credentials')}</FormLabel>
<InfoTooltip
tooltip={t(
'Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.',
)}
viewBox="0 0 24 24"
/>
</div>

{!fileToUpload && (
<Button
className="input-upload-btn"
onClick={() => document?.getElementById('selectedFile')?.click()}
>
{t('Choose File')}
</Button>
)}
{fileToUpload && (
<div className="input-upload-current">
{fileToUpload}
<DeleteFilled
onClick={() => {
setFileToUpload(null);
changeMethods.onParametersChange({
target: {
name: 'credentials_info',
value: '',
},
});
}}
db?.engine !== 'gsheets' && (
AAfghahi marked this conversation as resolved.
Show resolved Hide resolved
<div
className="input-container"
css={(theme: SupersetTheme) => infoTooltip(theme)}
>
<div css={{ display: 'flex', alignItems: 'center' }}>
<FormLabel required>{t('Upload Credentials')}</FormLabel>
<InfoTooltip
tooltip={t(
'Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.',
)}
viewBox="0 0 24 24"
/>
</div>
)}

<input
id="selectedFile"
className="input-upload"
type="file"
onChange={async event => {
let file;
if (event.target.files) {
file = event.target.files[0];
}
setFileToUpload(file?.name);
changeMethods.onParametersChange({
target: {
type: null,
name: 'credentials_info',
value: await file?.text(),
checked: false,
},
});
(document.getElementById(
'selectedFile',
) as HTMLInputElement).value = null as any;
}}
/>
</div>
)
{!fileToUpload && (
<Button
className="input-upload-btn"
onClick={() =>
document?.getElementById('selectedFile')?.click()
Copy link
Member

Choose a reason for hiding this comment

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

can we use a ref to get the element instead of the dom directly?

}
>
{t('Choose File')}
</Button>
)}
{fileToUpload && (
<div className="input-upload-current">
{fileToUpload}
<DeleteFilled
onClick={() => {
setFileToUpload(null);
changeMethods.onParametersChange({
target: {
name: 'credentials_info',
value: '',
},
});
}}
/>
</div>
)}
<input
id="selectedFile"
className="input-upload"
type="file"
onChange={async event => {
Copy link
Member

Choose a reason for hiding this comment

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

can we put this onChange in a separate function? It's a little long to be inline.

let file;
if (event.target.files) {
file = event.target.files[0];
}
setFileToUpload(file?.name);
changeMethods.onParametersChange({
target: {
type: null,
name: 'credentials_info',
value: await file?.text(),
checked: false,
},
});
(document.getElementById(
Copy link
Member

Choose a reason for hiding this comment

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

can we use a ref instead?

'selectedFile',
) as HTMLInputElement).value = null as any;
}}
/>
</div>
)
)}
</CredentialInfoForm>
);
};

const TableCatalog = ({
changeMethods,
isEditMode,
db,
editNewDb,
}: FieldPropTypes) => <>hello! {db?.engine}</>;
Copy link
Member

Choose a reason for hiding this comment

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

is this placeholder code?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this was where i ran out of time


const hostField = ({
required,
changeMethods,
Expand Down Expand Up @@ -299,19 +317,42 @@ const displayField = ({
getValidation,
validationErrors,
db,
setPublicSheets,
}: FieldPropTypes) => (
<ValidatedInput
id="database_name"
name="database_name"
required
value={db?.database_name}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.database_name}
placeholder=""
label="Display Name"
onChange={changeMethods.onChange}
helpText={t('Pick a nickname for this database to display as in Superset.')}
/>
<>
<ValidatedInput
id="database_name"
name="database_name"
required
value={db?.database_name}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.database_name}
placeholder=""
label="Display Name"
onChange={changeMethods.onChange}
helpText={t(
'Pick a nickname for this database to display as in Superset.',
)}
/>

{db?.engine === 'gsheets' && (
<>
<FormLabel required>{t('Type of Google Sheets Allowed')}</FormLabel>
<Select
style={{ width: '100%' }}
onChange={(value: string) => setPublicSheets(value)}
Copy link
Member

Choose a reason for hiding this comment

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

if you're not manipulating the arguments at all, you should be able to just pass the function itself, rather than create a new arrow function.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I do this onChange = {setPublicSheets(value)} I get this reference error.

ReferenceError: value is not defined

Copy link
Member

Choose a reason for hiding this comment

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

When you write:

// this is a function
(value: string) => setPublicSheets(value)

You're defining an anonymous function that takes a string called value and calls setPublicSheets on it. You're passing that function to onChange:

onChange={(value: string) => setPublicSheets(value)}

If you wanted to name that function, you could do this:

const func = (value: string) => setPublicSheets(value);

Then you could write:

onChange={func}

But why do you need to create that anonymous function and pass to onChange? You already have a function that takes a string called value and returns the result of passing it to setPublicSheets... it's setPublicSheets itself!

So all you need to do is:

onChange={setPublicSheets}

defaultValue="true"
>
<Select.Option value="true" key={1}>
Publicly shared sheets only
</Select.Option>
<Select.Option value="false" key={2}>
Public and privately shared sheets
</Select.Option>
Copy link
Member

Choose a reason for hiding this comment

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

If you set the values to booleans you don't need to convert them from string:

Suggested change
<Select.Option value="true" key={1}>
Publicly shared sheets only
</Select.Option>
<Select.Option value="false" key={2}>
Public and privately shared sheets
</Select.Option>
<Select.Option value={true} key={1}>
Publicly shared sheets only
</Select.Option>
<Select.Option value={false} key={2}>
Public and privately shared sheets
</Select.Option>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I tried this but the antd component only allows string or number:
https://3x.ant.design/components/select/

That's why I did the string conversion. Can we change that locally?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. I would convert it as soon as possible then, passing around a string that should really be a boolean is prone to bugs:

<Select
          style={{ width: '100%' }}
          onChange={(value: string) => setPublic(stringToBoolean(value))}

Where:

const stringToBoolean = (value: string): boolean => value === 'true';

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 idea, I will make that change right now.

</Select>
</>
)}
</>
);

const queryField = ({
Expand Down Expand Up @@ -375,6 +416,7 @@ const FORM_FIELD_MAP = {
query: queryField,
encryption: forceSSLField,
credentials_info: CredentialsInfo,
table_catalog: TableCatalog,
};

const DatabaseConnectionForm = ({
Expand All @@ -388,10 +430,14 @@ const DatabaseConnectionForm = ({
isEditMode = false,
sslForced,
editNewDb,
setPublicSheets,
isPublic,
}: {
isEditMode?: boolean;
sslForced: boolean;
editNewDb?: boolean;
isPublic?: boolean;
setPublicSheets: (value: string) => void;
dbModel: DatabaseForm;
db: Partial<DatabaseObject> | null;
onParametersChange: (
Expand Down Expand Up @@ -434,6 +480,8 @@ const DatabaseConnectionForm = ({
isEditMode,
sslForced,
editNewDb,
setPublicSheets,
isPublic,
}),
)}
</div>
Expand Down
17 changes: 16 additions & 1 deletion superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const [dbName, setDbName] = useState('');
const [editNewDb, setEditNewDb] = useState<boolean>(false);
const [isLoading, setLoading] = useState<boolean>(false);
const [isPublic, setPublic] = useState<boolean>(true);
const conf = useCommonConf();
const dbImages = getDatabaseImages();
const connectionAlert = getConnectionAlert();
Expand All @@ -360,7 +361,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
t('database'),
addDangerToast,
);

console.log(isPublic);
Copy link
Member

Choose a reason for hiding this comment

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

remove

const isDynamic = (engine: string | undefined) =>
availableDbs?.databases.filter(
(DB: DatabaseObject) => DB.backend === engine || DB.engine === engine,
Expand Down Expand Up @@ -530,6 +531,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
}
};

const setPublicSheets = (value: string) => {
if (value === 'true') {
setPublic(true);
} else {
setPublic(false);
}
};

const setDatabaseModel = (engine: string) => {
const selectedDbModel = availableDbs?.databases.filter(
(db: DatabaseObject) => db.engine === engine,
Expand Down Expand Up @@ -799,9 +808,11 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
}
return (
<DatabaseConnectionForm
isPublic={isPublic}
isEditMode
sslForced={sslForced}
dbModel={dbModel}
setPublicSheets={setPublicSheets}
db={db as DatabaseObject}
onParametersChange={({ target }: { target: HTMLInputElement }) =>
onChange(ActionType.parametersChange, {
Expand Down Expand Up @@ -912,6 +923,8 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
) : (
<DatabaseConnectionForm
isEditMode
isPublic={isPublic}
setPublicSheets={setPublicSheets}
sslForced={sslForced}
dbModel={dbModel}
db={db as DatabaseObject}
Expand Down Expand Up @@ -1069,7 +1082,9 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
</StyledAlertMargin>
)}
<DatabaseConnectionForm
isPublic={isPublic}
db={db}
setPublicSheets={setPublicSheets}
AAfghahi marked this conversation as resolved.
Show resolved Hide resolved
sslForced={sslForced}
dbModel={dbModel}
onParametersChange={({
Expand Down