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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
