fix(charts list): do not trigger ListViewError exception for anonymous users #18210 (#20171)

superset-frontend/src/views/CRUD/chart/ChartCard.tsx

@@ -43,7 +43,7 @@ interface ChartCardProps {
   saveFavoriteStatus: (id: number, isStarred: boolean) => void;
   favoriteStatus: boolean;
   chartFilter?: string;
-  userId?: number;
+  userId?: string | number;
   showThumbnails?: boolean;
   handleBulkChartExport: (chartsToExport: Chart[]) => void;
 }
@@ -165,11 +165,13 @@ export default function ChartCard({
               e.preventDefault();
             }}
           >
-            <FaveStar
-              itemId={chart.id}
-              saveFaveStar={saveFavoriteStatus}
-              isStarred={favoriteStatus}
-            />
+            {userId && (
+              <FaveStar
+                itemId={chart.id}
+                saveFaveStar={saveFavoriteStatus}
+                isStarred={favoriteStatus}
+              />
+            )}
             <AntdDropdown overlay={menu}>
               <Icons.MoreVert iconColor={theme.colors.grayscale.base} />
             </AntdDropdown>

superset-frontend/src/views/CRUD/chart/ChartList.test.jsx

@@ -17,6 +17,7 @@
  * under the License.
  */
 import React from 'react';
+import { MemoryRouter } from 'react-router-dom';
 import thunk from 'redux-thunk';
 import configureStore from 'redux-mock-store';
 import { Provider } from 'react-redux';
@@ -34,6 +35,9 @@ import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
 import ListView from 'src/components/ListView';
 import PropertiesModal from 'src/explore/components/PropertiesModal';
 import ListViewCard from 'src/components/ListViewCard';
+import FaveStar from 'src/components/FaveStar';
+import TableCollection from 'src/components/TableCollection';
+import CardCollection from 'src/components/ListView/CardCollection';
 // store needed for withToasts(ChartTable)
 const mockStore = configureStore([thunk]);
 const store = mockStore({});
@@ -105,13 +109,17 @@ describe('ChartList', () => {
   });
   const mockedProps = {};
 
-  const wrapper = mount(
-    <Provider store={store}>
-      <ChartList {...mockedProps} user={mockUser} />
-    </Provider>,
-  );
+  let wrapper;
 
   beforeAll(async () => {
+    wrapper = mount(
+      <MemoryRouter>
+        <Provider store={store}>
+          <ChartList {...mockedProps} user={mockUser} />
+        </Provider>
+      </MemoryRouter>,
+    );
+
     await waitForComponentToPaint(wrapper);
   });
 
@@ -159,6 +167,18 @@ describe('ChartList', () => {
     await waitForComponentToPaint(wrapper);
     expect(wrapper.find(ConfirmStatusChange)).toExist();
   });
+
+  it('renders the Favorite Star column in list view for logged in user', async () => {
+    wrapper.find('[aria-label="list-view"]').first().simulate('click');
+    await waitForComponentToPaint(wrapper);
+    expect(wrapper.find(TableCollection).find(FaveStar)).toExist();
+  });
+
+  it('renders the Favorite Star in card view for logged in user', async () => {
+    wrapper.find('[aria-label="card-view"]').first().simulate('click');
+    await waitForComponentToPaint(wrapper);
+    expect(wrapper.find(CardCollection).find(FaveStar)).toExist();
+  });
 });
 
 describe('RTL', () => {
@@ -201,3 +221,39 @@ describe('RTL', () => {
     expect(importTooltip).toBeInTheDocument();
   });
 });
+
+describe('ChartList - anonymous view', () => {
+  const mockedProps = {};
+  const mockUserLoggedOut = {};
+  let wrapper;
+
+  beforeAll(async () => {
+    fetchMock.resetHistory();
+    wrapper = mount(
+      <MemoryRouter>
+        <Provider store={store}>
+          <ChartList {...mockedProps} user={mockUserLoggedOut} />
+        </Provider>
+      </MemoryRouter>,
+    );
+
+    await waitForComponentToPaint(wrapper);
+  });
+
+  afterAll(() => {
+    cleanup();
+    fetch.resetMocks();
+  });
+
+  it('does not render the Favorite Star column in list view for anonymous user', async () => {
+    wrapper.find('[aria-label="list-view"]').first().simulate('click');
+    await waitForComponentToPaint(wrapper);
+    expect(wrapper.find(TableCollection).find(FaveStar)).not.toExist();
+  });
+
+  it('does not render the Favorite Star in card view for anonymous user', async () => {
+    wrapper.find('[aria-label="card-view"]').first().simulate('click');
+    await waitForComponentToPaint(wrapper);
+    expect(wrapper.find(CardCollection).find(FaveStar)).not.toExist();
+  });
+});

superset-frontend/src/views/CRUD/chart/ChartList.tsx

@@ -22,7 +22,7 @@ import {
   SupersetClient,
   t,
 } from '@superset-ui/core';
-import React, { useMemo, useState } from 'react';
+import React, { useState, useMemo, useCallback } from 'react';
 import rison from 'rison';
 import { uniqBy } from 'lodash';
 import moment from 'moment';
@@ -146,7 +146,11 @@ const Actions = styled.div`
 `;
 
 function ChartList(props: ChartListProps) {
-  const { addDangerToast, addSuccessToast } = props;
+  const {
+    addDangerToast,
+    addSuccessToast,
+    user: { userId },
+  } = props;
 
   const {
     state: {
@@ -180,7 +184,6 @@ function ChartList(props: ChartListProps) {
   const [passwordFields, setPasswordFields] = useState<string[]>([]);
   const [preparingExport, setPreparingExport] = useState<boolean>(false);
 
-  const { userId } = props.user;
   // TODO: Fix usage of localStorage keying on the user id
   const userSettings = dangerouslyGetItemDoNotUse(userId?.toString(), null) as {
     thumbnails: boolean;
@@ -235,27 +238,25 @@ function ChartList(props: ChartListProps) {
 
   const columns = useMemo(
     () => [
-      ...(props.user.userId
-        ? [
-            {
-              Cell: ({
-                row: {
-                  original: { id },
-                },
-              }: any) => (
-                <FaveStar
-                  itemId={id}
-                  saveFaveStar={saveFavoriteStatus}
-                  isStarred={favoriteStatus[id]}
-                />
-              ),
-              Header: '',
-              id: 'id',
-              disableSortBy: true,
-              size: 'sm',
-            },
-          ]
-        : []),
+      {
+        Cell: ({
+          row: {
+            original: { id },
+          },
+        }: any) =>
+          userId && (
+            <FaveStar
+              itemId={id}
+              saveFaveStar={saveFavoriteStatus}
+              isStarred={favoriteStatus[id]}
+            />
+          ),
+        Header: '',
+        id: 'id',
+        disableSortBy: true,
+        size: 'xs',
+        hidden: !userId,
+      },
       {
         Cell: ({
           row: {
@@ -451,10 +452,15 @@ function ChartList(props: ChartListProps) {
       },
     ],
     [
+      userId,
       canEdit,
       canDelete,
       canExport,
-      ...(props.user.userId ? [favoriteStatus] : []),
+      saveFavoriteStatus,
+      favoriteStatus,
+      refreshData,
+      addSuccessToast,
+      addDangerToast,
     ],
   );
 
@@ -552,7 +558,7 @@ function ChartList(props: ChartListProps) {
         fetchSelects: createFetchDatasets,
         paginate: true,
       },
-      ...(props.user.userId ? [favoritesFilter] : []),
+      ...(userId ? [favoritesFilter] : []),
       {
         Header: t('Certified'),
         id: 'id',
@@ -596,8 +602,8 @@ function ChartList(props: ChartListProps) {
     },
   ];
 
-  function renderCard(chart: Chart) {
-    return (
+  const renderCard = useCallback(
+    (chart: Chart) => (
       <ChartCard
         chart={chart}
         showThumbnails={
@@ -611,13 +617,23 @@ function ChartList(props: ChartListProps) {
         addDangerToast={addDangerToast}
         addSuccessToast={addSuccessToast}
         refreshData={refreshData}
+        userId={userId}
         loading={loading}
         favoriteStatus={favoriteStatus[chart.id]}
         saveFavoriteStatus={saveFavoriteStatus}
         handleBulkChartExport={handleBulkChartExport}
       />
-    );
-  }
+    ),
+    [
+      addDangerToast,
+      addSuccessToast,
+      bulkSelectEnabled,
+      favoriteStatus,
+      hasPerm,
+      loading,
+    ],
+  );
+
   const subMenuButtons: SubMenuProps['buttons'] = [];
   if (canDelete || canExport) {
     subMenuButtons.push({

superset-frontend/src/views/CRUD/utils.tsx

@@ -246,7 +246,7 @@ export function handleChartDelete(
   addDangerToast: (arg0: string) => void,
   refreshData: (arg0?: FetchDataConfig | null) => void,
   chartFilter?: string,
-  userId?: number,
+  userId?: string | number,
 ) {
   const filters = {
     pageIndex: 0,