[GitHub] [superset] ktmud commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions

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

ktmud commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850003432


##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
     headers,
     timeout,
     fetchRetryOptions,
-    ignoreUnauthorized,
+    ignoreUnauthorized = false,

Review Comment:
   Be more explicit what is the default. I kind of feel this should probably default to `true` but that's another topic. Async API requests should never implicitly redirect users as it may often cause surprises. There should be better error handling on the API user side instead.
   
   cc @geido @kgabryje could you give more context on why #17597 made redirects on 401 the default?



##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -34,9 +34,11 @@ import {
 import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants';
 
 const defaultUnauthorizedHandler = () => {
-  window.location.href = `/login?next=${
-    window.location.pathname + window.location.search
-  }`;
+  if (!window.location.pathname.startsWith('/login')) {

Review Comment:
   Only redirect when we are not on the login page so we never see infinite redirect caused by the API client ever again.



##########
superset-frontend/src/views/components/SubMenu.tsx:
##########
@@ -240,22 +240,22 @@ const SubMenuComponent: React.FunctionComponent<SubMenuProps> = props => {
             if ((props.usesRouter || hasHistory) && !!tab.usesRouter) {
               return (
                 <Menu.Item key={tab.label}>
-                  <li
+                  <div

Review Comment:
   Bycatch: fix a React warning about nested `<li />`s.



##########
superset-frontend/src/views/components/MenuRight.tsx:
##########
@@ -234,19 +238,21 @@ const RightMenu = ({
   };
 
   const onMenuOpen = (openKeys: string[]) => {
-    if (openKeys.length) {
-      return hasFileUploadEnabled();
+    if (openKeys.length && canUploadData) {
+      return checkAllowUploads();
     }
     return null;
   };
 
   return (
     <StyledDiv align={align}>
-      <DatabaseModal
-        onHide={handleOnHideModal}
-        show={showModal}
-        dbEngine={engine}
-      />
+      {canDatabase && (

Review Comment:
   Bycatch: only render the DatabaseModal when users can create new database connection.



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