[GitHub] [superset] betodealmeida commented on a change in pull request #16628: feat: Add Private Google Sheets to dynamic form

[GitBox <gi...@apache.org>]

betodealmeida commented on a change in pull request #16628:
URL: https://github.com/apache/superset/pull/16628#discussion_r715088746



##########
File path: superset/models/core.py
##########
@@ -246,9 +248,17 @@ def parameters(self) -> Dict[str, Any]:
             parameters = self.db_engine_spec.get_parameters_from_uri(uri, encrypted_extra=encrypted_extra)  # type: ignore # pylint: disable=line-too-long,useless-suppression
         except Exception:  # pylint: disable=broad-except
             parameters = {}
-
         return parameters
 
+    @property
+    def parameters_schema(self) -> Dict[str, Any]:
+        print("*********************************")

Review comment:
       ```suggestion
   ```

##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -32,14 +32,30 @@ import {
   infoTooltip,
   StyledFooterButton,
   StyledCatalogTable,
+  labelMarginBotton,
 } from './styles';
 import { CatalogObject, DatabaseForm, DatabaseObject } from '../types';
 
+// These are the columns that are going to be added to encrypted extra, they differ in name based
+// on the engine, however we want to use the same component for each of them. Make sure to add the
+// the engine specific name here.
+export const encryptedCredentialsMap = {
+  gsheets: 'service_account_info',
+  bigquery: 'credentials_info',
+};
+
 enum CredentialInfoOptions {
   jsonUpload,
   copyPaste,
 }
 
+const setStringToBoolean = (optionValue: string) => {
+  if (optionValue === 'true') {
+    return true;
+  }
+  return false;

Review comment:
       When you write an `if` block that returns a boolean you can always simplify it:
   
   ```suggestion
     return optionValue === 'true';
   ```

##########
File path: superset/db_engine_specs/gsheets.py
##########
@@ -45,10 +45,15 @@
 
 class GSheetsParametersSchema(Schema):
     catalog = fields.Dict()
+    service_account_info = EncryptedString(
+        required=False,
+        description="Contents of GSheets JSON credentials.",
+        field_name="service_account_info",

Review comment:
       I'm pretty sure you only need to specify `field_name` if you want to name it differently from the attribute you assigned the field to.
   
   ```suggestion
   ```

##########
File path: superset/databases/api.py
##########
@@ -920,7 +921,6 @@ def available(self) -> Response:
         for engine_spec, drivers in get_available_engine_specs().items():
             if not drivers:
                 continue
-

Review comment:
       Ditto.

##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -88,9 +105,40 @@ const CredentialsInfo = ({
   const [fileToUpload, setFileToUpload] = useState<string | null | undefined>(
     null,
   );
+  const [isPublic, setIsPublic] = useState<boolean>(true);
+  const showCredentialsInfo =
+    db?.engine === 'gsheets' ? !isEditMode && !isPublic : !isEditMode;
+  const isEncrypted = db?.encrypted_extra && db?.encrypted_extra?.length > 2;

Review comment:
       Why isn't an `encrypted_extra` field with a single key/value pair considered encrypted?

##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -32,14 +32,30 @@ import {
   infoTooltip,
   StyledFooterButton,
   StyledCatalogTable,
+  labelMarginBotton,
 } from './styles';
 import { CatalogObject, DatabaseForm, DatabaseObject } from '../types';
 
+// These are the columns that are going to be added to encrypted extra, they differ in name based
+// on the engine, however we want to use the same component for each of them. Make sure to add the
+// the engine specific name here.
+export const encryptedCredentialsMap = {
+  gsheets: 'service_account_info',
+  bigquery: 'credentials_info',
+};
+
 enum CredentialInfoOptions {
   jsonUpload,
   copyPaste,
 }
 
+const setStringToBoolean = (optionValue: string) => {

Review comment:
       ```suggestion
   const castStringToBoolean = (optionValue: string) => {
   ```

##########
File path: superset-frontend/src/views/CRUD/hooks.ts
##########
@@ -277,7 +277,6 @@ export function useSingleViewResource<D extends object = any>(
       updateState({
         loading: true,
       });
-

Review comment:
       Code also needs to breath... let's leave these empty lines! Usually empty lines separate logical blocks that are (slightly) unrelated.

##########
File path: superset/databases/api.py
##########
@@ -943,7 +943,6 @@ def available(self) -> Response:
                 payload[
                     "sqlalchemy_uri_placeholder"
                 ] = engine_spec.sqlalchemy_uri_placeholder  # type: ignore
-

Review comment:
       Ditto. Is your editor automatically removing these lines?




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