You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/07/08 21:56:49 UTC
[GitHub] [superset] AAfghahi opened a new pull request #15598: feat: Gsheet adding sheets component
AAfghahi opened a new pull request #15598:
URL: https://github.com/apache/superset/pull/15598
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b30f79) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.00%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 6b30f79 differs from pull request most recent head 962e597. Consider uploading reports for the commit 962e597 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.01%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39524 +6
- Misses 11610 11617 +7
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `90.31% <0.00%> (+0.42%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...962e597](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877489911
Can you add more details in the PR description?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668229824
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668226925
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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,
);
+
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {!isEditMode && db?.engine === 'bigquery' && (
Review comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668228146
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
+ <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()
+ }
+ >
+ {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 => {
Review comment:
can we put this onChange in a separate function? It's a little long to be inline.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668814004
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
+ 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>
Review comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi closed pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi closed pull request #15598:
URL: https://github.com/apache/superset/pull/15598
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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**, and you're passing that function to `onChange`.
If you wanted to name that function, you could do this:
```js
const func = (value: string) => setPublicSheets(value);
```
Then you could write:
```js
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668149662
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
Review comment:
We should reuse the same component we have for uploading/pasting credentials in BQ.
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
Review comment:
We should use the same copy as BQ here.
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
##########
@@ -1069,7 +1082,9 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
</StyledAlertMargin>
)}
<DatabaseConnectionForm
+ isPublic={isPublic}
db={db}
+ setPublicSheets={setPublicSheets}
Review comment:
Nit, since these two are related let's keep them together:
```suggestion
db={db}
isPublic={isPublic}
setPublicSheets={setPublicSheets}
```
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
+ 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>
Review comment:
If you set the values to booleans you don't need to convert them from string:
```suggestion
<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>
```
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
Review comment:
Also, I'm a bit worried that we're adding conditional rendering based on the engine name, not on its parameters — this will quickly become unmaintainable as we add more databases.
Maybe we could create separate files for custom DBs like BQ, GSheets?
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
Review comment:
```suggestion
db?.engine === 'bigquery' && (
```
We don't want to risk showing the copy below ('Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.') for non-BQ databases.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668369993
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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,
);
+
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {!isEditMode && db?.engine === 'bigquery' && (
Review comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r667239418
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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);
Review comment:
remove
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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`:
```js
onChange={(value: string) => setPublicSheets(value)}
```
If you wanted to name that function, you could do this:
```js
const func = (value: string) => setPublicSheets(value);
```
Then you could write:
```js
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (459eba3) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.02%`.
> The diff coverage is `18.42%`.
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.94% -0.03%
==========================================================
Files 976 976
Lines 51347 51369 +22
Branches 6907 6919 +12
==========================================================
+ Hits 39518 39524 +6
- Misses 11610 11624 +14
- Partials 219 221 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.38% <18.42%> (-0.04%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.13% <16.12%> (-3.96%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.46% <28.57%> (-0.41%)` | :arrow_down: |
| [...tend/src/views/CRUD/annotation/AnnotationModal.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYW5ub3RhdGlvbi9Bbm5vdGF0aW9uTW9kYWwudHN4) | `60.44% <0.00%> (-2.52%)` | :arrow_down: |
| [...s/CRUD/data/database/DatabaseModal/ModalHeader.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL01vZGFsSGVhZGVyLnRzeA==) | `89.18% <0.00%> (-2.48%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `72.46% <0.00%> (-2.17%)` | :arrow_down: |
| [...ews/CRUD/annotationlayers/AnnotationLayerModal.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYW5ub3RhdGlvbmxheWVycy9Bbm5vdGF0aW9uTGF5ZXJNb2RhbC50c3g=) | `69.47% <0.00%> (-0.32%)` | :arrow_down: |
| [...d/src/views/CRUD/csstemplates/CssTemplateModal.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY3NzdGVtcGxhdGVzL0Nzc1RlbXBsYXRlTW9kYWwudHN4) | `67.30% <0.00%> (-0.32%)` | :arrow_down: |
| [...et-frontend/src/messageToasts/components/Toast.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL21lc3NhZ2VUb2FzdHMvY29tcG9uZW50cy9Ub2FzdC50c3g=) | `92.50% <0.00%> (-0.19%)` | :arrow_down: |
| [...frontend/src/views/CRUD/alert/AlertReportModal.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvQWxlcnRSZXBvcnRNb2RhbC50c3g=) | `61.70% <0.00%> (-0.10%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...459eba3](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0fede91) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.01%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 0fede91 differs from pull request most recent head 6c001ac. Consider uploading reports for the commit 6c001ac to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.02%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39522 +4
- Misses 11610 11619 +9
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...6c001ac](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668870624
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
+ 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>
Review comment:
good idea, I will make that change right now.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668227250
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -104,9 +109,10 @@ const CredentialsInfo = ({
)}
{uploadOption === CredentialInfoOptions.copyPaste ||
isEditMode ||
- editNewDb ? (
+ editNewDb ||
+ (db?.engine === 'gsheets' && !isPublic) ? (
Review comment:
same thing here. We want to avoid a bunch of if/thens by engine.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668248662
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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,
);
+
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {!isEditMode && db?.engine === 'bigquery' && (
Review comment:
We currently return this for BQ:
```json
{
"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`:
```json
"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:
```json
"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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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**, and you're passing that function to `onChange`.
If you wanted to name that function, you could do this:
```js
const func = (value: string) => setPublicSheets(value);
```
Then you could write:
```js
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668370253
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
Review comment:
Ha, I didn’t realize that we both made the same comment. :)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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**, and you're passing that function to `onChange`.
If you wanted to name that function, you could do one of these:
```js
const func = (value: string) => setPublicSheets(value);
function func(value: string) {
return setPublicSheets(value);
}
```
Then you could write:
```js
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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**, and you're passing that function to `onChange`:
```js
onChange={(value: string) => setPublicSheets(value)}
```
If you wanted to name that function, you could do this:
```js
const func = (value: string) => setPublicSheets(value);
```
Then you could write:
```js
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r669030043
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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,
);
+
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {!isEditMode && db?.engine === 'bigquery' && (
Review comment:
👍 Then encrypted_credentials could be a paste/upload component.. no need for the api to define it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668227806
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
+ <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()
Review comment:
can we use a ref to get the element instead of the dom directly?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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, and you're passing that function to `onChange`.
If you wanted to name that function, you could do one of these:
```js
const func = (value: string) => setPublicSheets(value);
function func(value: string) {
return setPublicSheets(value);
}
```
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668802521
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
+ <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()
+ }
+ >
+ {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 => {
+ 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>
+ )
)}
</CredentialInfoForm>
);
};
+const TableCatalog = ({
+ changeMethods,
+ isEditMode,
+ db,
+ editNewDb,
+}: FieldPropTypes) => <>hello! {db?.engine}</>;
Review comment:
yeah this was where i ran out of time
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-883609812
https://github.com/apache/superset/pull/15801/
in favor of this PR
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668828624
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When I do this `onChange = {setPublicSheets(value)`} I get this reference error.
ReferenceError: value is not defined
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0fede91) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.01%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 0fede91 differs from pull request most recent head 84222b2. Consider uploading reports for the commit 84222b2 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.02%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39522 +4
- Misses 11610 11619 +9
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...84222b2](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668864701
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
+ 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>
Review comment:
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:
```js
<Select
style={{ width: '100%' }}
onChange={(value: string) => setPublic(stringToBoolean(value))}
```
Where:
```typescript
const stringToBoolean = (value: string): boolean => value === 'true';
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668228506
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
+ <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()
+ }
+ >
+ {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 => {
+ 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(
Review comment:
can we use a ref instead?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668370989
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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,
);
+
return (
<CredentialInfoForm>
- {!isEditMode && (
+ {!isEditMode && db?.engine === 'bigquery' && (
Review comment:
Right, we could call it anything we want.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9aba8fc) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.00%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 9aba8fc differs from pull request most recent head 6b30f79. Consider uploading reports for the commit 6b30f79 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.01%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39524 +6
- Misses 11610 11617 +7
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `81.29% <ø> (ø)` | |
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
| mysql | `81.56% <ø> (ø)` | |
| postgres | `81.58% <ø> (ø)` | |
| presto | `81.29% <ø> (+<0.01%)` | :arrow_up: |
| python | `82.11% <ø> (+<0.01%)` | :arrow_up: |
| sqlite | `81.19% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `90.31% <0.00%> (+0.42%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...6b30f79](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668230024
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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',
+ )}
</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' && (
+ <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()
+ }
+ >
+ {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 => {
+ 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>
+ )
)}
</CredentialInfoForm>
);
};
+const TableCatalog = ({
+ changeMethods,
+ isEditMode,
+ db,
+ editNewDb,
+}: FieldPropTypes) => <>hello! {db?.engine}</>;
Review comment:
is this placeholder code?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668851542
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -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)}
Review comment:
When you write:
```js
// 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**, and you're passing that function to `onChange`.
If you wanted to name that function, you could do one of these:
```js
const func = (value: string) => setPublicSheets(value);
function func(value: string) {
return setPublicSheets(value);
}
```
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:
```js
onChange={setPublicSheets}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0fede91) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.01%`.
> The diff coverage is `20.00%`.
> :exclamation: Current head 0fede91 differs from pull request most recent head 962e597. Consider uploading reports for the commit 962e597 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.02%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39522 +4
- Misses 11610 11619 +9
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...962e597](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] edited a comment on pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15598:
URL: https://github.com/apache/superset/pull/15598#issuecomment-877488652
# [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15598](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b30f79) into [pexdax/improved-sheets](https://codecov.io/gh/apache/superset/commit/accb48e4296dca9fb6b30ac0eea0ea91de45db9a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (accb48e) will **decrease** coverage by `0.00%`.
> The diff coverage is `20.00%`.
[](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## pexdax/improved-sheets #15598 +/- ##
==========================================================
- Coverage 76.96% 76.95% -0.01%
==========================================================
Files 976 976
Lines 51347 51360 +13
Branches 6907 6914 +7
==========================================================
+ Hits 39518 39524 +6
- Misses 11610 11617 +7
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.40% <20.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../database/DatabaseModal/DatabaseConnectionForm.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0RhdGFiYXNlQ29ubmVjdGlvbkZvcm0udHN4) | `59.78% <16.66%> (-3.32%)` | :arrow_down: |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.74% <40.00%> (-0.12%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15598/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `90.31% <0.00%> (+0.42%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [accb48e...6b30f79](https://codecov.io/gh/apache/superset/pull/15598?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #15598: feat: Gsheet adding sheets component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r667239818
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
##########
@@ -360,7 +361,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
t('database'),
addDangerToast,
);
-
+ console.log(isPublic);
Review comment:
remove
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org