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 2022/08/12 03:18:03 UTC

[GitHub] [superset] Antonio-RiveroMartnez opened a new pull request, #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Antonio-RiveroMartnez opened a new pull request, #21065:
URL: https://github.com/apache/superset/pull/21065

   ### SUMMARY
   Our users are getting an `Internal Server Error 500` when trying to upload files to `GSheets` databases, that's because such databases don't support file upload nor the `Clickhouse` ones thus we need to stop showing such options to the users and prevent that existing ones get displayed in the upload form.
   
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   Before:
   ![error](https://user-images.githubusercontent.com/38889534/184278578-5969753c-d5a3-4ff3-9895-130cfd51d975.gif)
   
   After:
   ![test](https://user-images.githubusercontent.com/38889534/184278136-0788af68-ce22-484a-bc2f-3608517c2b80.gif)
   
   
   ### TESTING INSTRUCTIONS
   1. When selecting a DB type, if GSheet or Clickhouse is selected the `Allow data upload` checkbox is not displayed in the form
   2. Enabled the file upload menu only if there is any exiting DB with allow_file_upload flag set to true and that is not `GSheets` nor `Clickhouse`
   3. When the form to file upload files is opened, the `Database` dropdown must not contain any `GSheets` nor `Clickhouse` DB.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [X] Has associated issue: https://github.com/apache/superset/issues/19247
   - [ ] Required feature flags:
   - [X] 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] github-actions[bot] commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1220094520

   @yousoph Ephemeral environment spinning up at http://54.213.205.154:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951884079


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   PR Updated with the new `supports_file_upload` @AAfghahi  :) 



-- 
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] nytai commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r947311065


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   I think having someone with more experience with these per-db configs, like @villebro or @john-bodley, have a look would be good. 



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951777459


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   The `allow_file_upload` is in the `Database` model and it's set per each user's DB. While the `allows_file_upload` is set in the `DB Engine Spec` model and cannot be set by the user. So, even when a `Database` has its flag set to true (think of existing user's DB), if its `DB Engine Spec` doesn't support it, we prevent file uploading to 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] Antonio-RiveroMartnez closed pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez closed pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files
URL: https://github.com/apache/superset/pull/21065


-- 
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 diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968511787


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   If you look at the line above that for useSelector, could you follow that method instead and import useState?



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r949640424


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   Ok @nytai @villebro , I updated the PR, we were sending some `extra` information in the response but that might is editable by the user so I decided to include a new `engine_information` that currently only has `allows_file_upload` flag. Now, There is no need for updating multiple places but instead just setting such flags as true to prevent file uploading.



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951777459


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   The `allow_file_upload` is in the `Database` model and it's set per each user's DB. While the `allows_file_upload` is set in the `DB Engine Spec` and cannot be set by the user. So, even when a `Database` has its flag set to true (think of existing user's DB), if its `DB Engine Spec` doesn't support it, we prevent file uploading to 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] AAfghahi commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951759755


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   how is this different from the current colum `allow_file_upload`? If they are very different, then should we think about changing the name of this? 
   
   `allows_file_upload` and `allow_file_upload` being different columns that serve different purposes is going to be way too confusing. 



-- 
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] github-actions[bot] commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1218556533

   @yousoph Ephemeral environment spinning up at http://54.201.86.244:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] yousoph commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1218555460

   /testenv up


-- 
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] nytai commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r945410290


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   Rather than maintaining a separate enum for this, why not define this as a property on the db_engine_spec?



-- 
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] nytai commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r947347497


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   @Antonio-RiveroMartnez Thanks for making that change, multiple places in the frontend where you're checking 
   `db?.engine !== Engines.GSheet && db?.engine !== Engines.ClickHouse`. This makes adding a new engine that does not support file upload a pain, since you have to update a bunch of files across both backend and frontend code. Ideally one could just specify in the db_engine_spec  `allows_file_upload=False` and that's it. So maybe adding a new api that you could call from the db modal to check is a supplied connection string or config supports file uploads would be a better approach? 



-- 
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 diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968485050


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   What is the reason for using this versus importing in `useState` and invoking it normally?



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968545276


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   You cannot because if you do something like:
   ```
   jest.mock('react', () => ({
     ...jest.requireActual('react'),
     useState: jest.fn(),
   }));
   ```
   the whole tests are going to fail because `React` itself wont have all what it needs to render and handle cases where we actually need regular useState, and after a while researching I found that spying on native hooks from react would require such usage as we have here. I guess it's the same reason why this other component uses it already in our code base:
   
   https://github.com/apache/superset/tree/master/superset-frontend/src/explore/components/controls/ControlPopover
   
   But let me see if I can find a way that's not a rabbit hole.



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968545276


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   You cannot because if you do something like:
   ```
   jest.mock('react', () => ({
     ...jest.requireActual('react'),
     useState: jest.fn(),
   }));
   ```
   the whole tests are going to fail because `React` itself wont have all what it needs to render, and after a while researching I found that spying on native hooks from react would require such usage as we have here. I guess it's the same reason why this other component uses it already in our code base:
   
   https://github.com/apache/superset/tree/master/superset-frontend/src/explore/components/controls/ControlPopover



-- 
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 diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951807193


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   ok that makes sense! 
   
   Could we name it `supports_file_upload` or something along the like? Having two columns only separated by a singular/plural shift is going to be confusing. 
   



-- 
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] github-actions[bot] commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1226539800

   Storybook has completed and can be viewed at 


-- 
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 #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1212697693

   # [Codecov](https://codecov.io/gh/apache/superset/pull/21065?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 [#21065](https://codecov.io/gh/apache/superset/pull/21065?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b13ffae) into [master](https://codecov.io/gh/apache/superset/commit/2d1ba468441b113c574d6fcc5984e8e09ddbc1c6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d1ba46) will **decrease** coverage by `11.46%`.
   > The diff coverage is `66.66%`.
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #21065       +/-   ##
   ===========================================
   - Coverage   66.27%   54.81%   -11.47%     
   ===========================================
     Files        1770     1770               
     Lines       67509    67520       +11     
     Branches     7177     7177               
   ===========================================
   - Hits        44743    37010     -7733     
   - Misses      20932    28676     +7744     
     Partials     1834     1834               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `53.17% <54.54%> (+<0.01%)` | :arrow_up: |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.07% <54.54%> (+<0.01%)` | :arrow_up: |
   | python | `57.80% <54.54%> (-23.69%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.71% <54.54%> (+<0.01%)` | :arrow_up: |
   
   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/21065?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL0V4dHJhT3B0aW9ucy50c3g=) | `83.33% <ø> (ø)` | |
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/21065/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==) | `31.52% <ø> (ø)` | |
   | [superset/views/database/forms.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvZm9ybXMucHk=) | `84.33% <54.54%> (-4.56%)` | :arrow_down: |
   | [...perset-frontend/src/views/components/RightMenu.tsx](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvUmlnaHRNZW51LnRzeA==) | `63.47% <66.66%> (ø)` | |
   | [...tend/src/views/CRUD/data/database/DatabaseList.tsx](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZUxpc3QudHN4) | `68.36% <100.00%> (ø)` | |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
   | [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
   | [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
   | [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21065/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
   | ... and [287 more](https://codecov.io/gh/apache/superset/pull/21065/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) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] yousoph commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1220092306

   /testenv up


-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968545276


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   You cannot because if you do something like:
   ```
   jest.mock('react', () => ({
     ...jest.requireActual('react'),
     useState: jest.fn(),
   }));
   ```
   the whole tests are going to fail because `React` itself wont have all what it needs to render and handle cases where we actually need regular useState, and after a while researching I found that spying on native hooks from react would require such usage as we have here. I guess it's the same reason why this other component uses it already in our code base:
   
   https://github.com/apache/superset/tree/master/superset-frontend/src/explore/components/controls/ControlPopover



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951812787


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   Yeah, totally! Let me make the change and ILYK. Thanks for the input @AAfghahi 



-- 
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 diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r951815970


##########
superset/db_engine_specs/base.py:
##########
@@ -1594,6 +1598,17 @@ def get_impersonation_key(cls, user: Optional[User]) -> Any:
         """
         return user.username if user else None
 
+    @classmethod
+    def get_public_information(cls) -> Dict[str, Any]:
+        """
+        Construct a Dict with properties we want to expose.
+
+        :returns: Dict with properties of our class like allows_file_upload
+        """
+        return {
+            "allows_file_upload": cls.allows_file_upload,

Review Comment:
   of course! Otherwise this LGTM! 



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968493094


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   So we can `spyOn` during our tests, e.g. `const useStateMock = jest.spyOn(React, 'useState');` https://github.com/apache/superset/pull/21065/files#diff-6a8138bf7b3a4c95d31569dfe4497ceb65ae07faddc47fb944a611ec7fb19776R86



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r946018591


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   Hey @nytai! I just updated the PR with such property in our `base` and each engine now. Thanks for the suggestion.



-- 
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 merged pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
eschutho merged PR #21065:
URL: https://github.com/apache/superset/pull/21065


-- 
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] rusackas commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r946108875


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   @nytai let us know if you want us to recruit additional folks for a review, or if you have time to take a look. Thanks!



-- 
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] github-actions[bot] commented on pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21065:
URL: https://github.com/apache/superset/pull/21065#issuecomment-1241515962

   Ephemeral environment shutdown and build artifacts deleted.


-- 
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] villebro commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r947923915


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   I agree with @nytai here - we should place this flag in the db engine specs, and the UI should accommodate accordingly. I may be wrong, but I believe we're already sending some db engine spec metadata in bootstrap data. Maybe we could just add `allows_file_upload` to `BaseEngineSpec`, override it in the GSheets and ClickHouse specs, and then make sure the flag is made available in bootstrap data? If we're not passing any info about db engine specs, now would probably be a good time to do so.



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r948122969


##########
superset/views/database/forms.py:
##########
@@ -39,6 +40,12 @@
 )
 from superset.models.core import Database
 
+
+class DatabasesNotAllowedToFileUpload(str, Enum):
+    GSHEETS = "gsheets"
+    CLICKHOUSE = "clickhousedb"

Review Comment:
   Hey @nytai @villebro! Yeah, currently the flag is being set in our `BaseEngineSpec` and being overridden in our `GSheets` and `Clickhouse` specs. The missing part is sending it (or using it) in the frontend instead of `db?.engine !== Engines.GSheet && db?.engine !== Engines.ClickHouse`. So, let me check the bootstrap data and if not doing it already let me start sending it. Thank you both for the input.



-- 
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 diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
AAfghahi commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968623095


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   But that test and file still imports useState and uses it instead of doing `React.useState`



-- 
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] Antonio-RiveroMartnez commented on a diff in pull request #21065: fix(databases): GSheets and Clickhouse DBs are not allowed to upload files

Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21065:
URL: https://github.com/apache/superset/pull/21065#discussion_r968636151


##########
superset-frontend/src/views/components/RightMenu.tsx:
##########
@@ -118,8 +118,8 @@ const RightMenu = ({
     ALLOWED_EXTENSIONS,
     HAS_GSHEETS_INSTALLED,
   } = useSelector<any, ExtentionConfigs>(state => state.common.conf);
-  const [showModal, setShowModal] = useState<boolean>(false);
-  const [engine, setEngine] = useState<string>('');
+  const [showModal, setShowModal] = React.useState<boolean>(false);

Review Comment:
   Indeed but the usage of the spied hook is different which might be affecting why these tests cannot do it that way, either way I'll give a spin to see if it works that way, if not possible we need to decide whether we move forward with it as is or not 🤔 I'll keep you posted @AAfghahi , thanks for the input 🎉 



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