feat: Gsheet adding sheets component #15598

AAfghahi · 2021-07-08T21:56:39Z

SUMMARY

This creates the basic structure for the gsheets component for the database modal.

It also adds the Table_Catalog feature, which still needs to be fleshed out.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-07-09.at.10.59.20.AM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

codecov · 2021-07-09T22:27:31Z

Codecov Report

Merging #15598 (0fede91) into pexdax/improved-sheets (accb48e) will decrease coverage by 0.01%.
The diff coverage is 20.00%.

❗ Current head 0fede91 differs from pull request most recent head 84222b2. Consider uploading reports for the commit 84222b2 to get more accurate results

@@                    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%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../database/DatabaseModal/DatabaseConnectionForm.tsx	`59.78% <16.66%> (-3.32%)`	⬇️
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`47.74% <40.00%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update accb48e...84222b2. Read the comment docs.

eschutho · 2021-07-09T22:29:34Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

 }: FieldPropTypes) => {
  const [uploadOption, setUploadOption] = useState<number>(
    CredentialInfoOptions.jsonUpload.valueOf(),
  );
  const [fileToUpload, setFileToUpload] = useState<string | null | undefined>(
    null,
  );
+  console.log('in credentials', isPublic);


eschutho · 2021-07-09T22:31:04Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -360,7 +361,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
    t('database'),
    addDangerToast,
  );
-
+  console.log(isPublic);


eschutho · 2021-07-09T22:31:22Z

Can you add more details in the PR description?

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

betodealmeida · 2021-07-12T18:18:20Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+          <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>


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

Suggested change

<Select.Option value="true" key={1}>

Publicly shared sheets only

</Select.Option>

<Select.Option value="false" key={2}>

Public and privately shared sheets

</Select.Option>

<Select.Option value={true} key={1}>

Publicly shared sheets only

</Select.Option>

<Select.Option value={false} key={2}>

Public and privately shared sheets

</Select.Option>

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?

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

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

Where:

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

good idea, I will make that change right now.

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

eschutho · 2021-07-12T20:17:45Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

  return (
    <CredentialInfoForm>
-      {!isEditMode && (
+      {!isEditMode && db?.engine === 'bigquery' && (


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?

We currently return this for BQ:

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

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

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

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

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

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

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

@eschutho, thoughts?

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?

Right, we could call it anything we want.

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

eschutho · 2021-07-12T20:18:20Z

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) ? (


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

eschutho · 2021-07-12T20:19:14Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+              <Button
+                className="input-upload-btn"
+                onClick={() =>
+                  document?.getElementById('selectedFile')?.click()


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

eschutho · 2021-07-12T20:19:47Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+              id="selectedFile"
+              className="input-upload"
+              type="file"
+              onChange={async event => {


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

eschutho · 2021-07-12T20:20:22Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+                    checked: false,
+                  },
+                });
+                (document.getElementById(


can we use a ref instead?

eschutho · 2021-07-12T20:22:31Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+        <FormLabel required>{t('Type of Google Sheets Allowed')}</FormLabel>
+        <Select
+          style={{ width: '100%' }}
+          onChange={(value: string) => setPublicSheets(value)}


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.

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

ReferenceError: value is not defined

When you write:

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

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

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

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

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

Then you could write:

onChange={func}

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

So all you need to do is:

onChange={setPublicSheets}

eschutho · 2021-07-12T20:22:51Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx

+  isEditMode,
+  db,
+  editNewDb,
+}: FieldPropTypes) => <>hello! {db?.engine}</>;


is this placeholder code?

yeah this was where i ran out of time

…atabaseConnectionForm.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

…ndex.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

eschutho

left feedback re: conditionals with engine name.

AAfghahi · 2021-07-20T18:37:19Z

#15801

in favor of this PR

first draft

eb5f607

pull-request-size bot added the size/L label Jul 8, 2021

added table_catalog

459eba3

AAfghahi marked this pull request as ready for review July 9, 2021 22:09

AAfghahi changed the base branch from master to pexdax/improved-sheets July 9, 2021 22:09

eschutho reviewed Jul 9, 2021

View reviewed changes

remove console.log

9aba8fc

betodealmeida reviewed Jul 12, 2021

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx Show resolved Hide resolved

betodealmeida reviewed Jul 12, 2021

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Outdated Show resolved Hide resolved

betodealmeida reviewed Jul 12, 2021

View reviewed changes

superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx Outdated Show resolved Hide resolved

eschutho reviewed Jul 12, 2021

View reviewed changes

AAfghahi and others added 4 commits July 13, 2021 10:10

Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/D…

6b30f79

…atabaseConnectionForm.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/i…

962e597

…ndex.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

revisions

0e4f249

revisions

84222b2

AAfghahi force-pushed the ch18764_gsheetsAdd branch from 6c001ac to 84222b2 Compare July 13, 2021 16:56

eschutho requested changes Jul 16, 2021

View reviewed changes

AAfghahi closed this Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Gsheet adding sheets component #15598

feat: Gsheet adding sheets component #15598

AAfghahi commented Jul 8, 2021 •

edited

codecov bot commented Jul 9, 2021 •

edited

eschutho Jul 9, 2021

eschutho Jul 9, 2021

eschutho commented Jul 9, 2021

betodealmeida Jul 12, 2021

AAfghahi Jul 13, 2021

betodealmeida Jul 13, 2021

AAfghahi Jul 13, 2021

eschutho Jul 12, 2021

betodealmeida Jul 12, 2021

eschutho Jul 13, 2021

betodealmeida Jul 13, 2021

eschutho Jul 13, 2021

eschutho Jul 12, 2021

eschutho Jul 12, 2021

eschutho Jul 12, 2021

eschutho Jul 12, 2021

eschutho Jul 12, 2021

AAfghahi Jul 13, 2021

betodealmeida Jul 13, 2021 •

edited

eschutho Jul 12, 2021

AAfghahi Jul 13, 2021

eschutho left a comment

AAfghahi commented Jul 20, 2021

feat: Gsheet adding sheets component #15598

feat: Gsheet adding sheets component #15598

Conversation

AAfghahi commented Jul 8, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 9, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida Jul 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

AAfghahi commented Jul 20, 2021

AAfghahi commented Jul 8, 2021 •

edited

codecov bot commented Jul 9, 2021 •

edited

betodealmeida Jul 13, 2021 •

edited