Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

Merged
merged 2 commits into from
Apr 4, 2022
Merged

fix(dashboard list): do not show favorite star for anonymous users #18210 #19409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34820b4
fix: Only show favorite star on dashboard list if user is logged in #…
dudasaron Mar 29, 2022
ced4cea
Fix linter errors
dudasaron Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset-frontend/src/components/ListViewCard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import CertifiedBadge from '../CertifiedBadge';
const ActionsWrapper = styled.div`
width: 64px;
display: flex;
justify-content: space-between;
justify-content: flex-end;
`;

const StyledCard = styled(AntdCard)`
Expand Down
14 changes: 8 additions & 6 deletions superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ interface DashboardCardProps {
saveFavoriteStatus: (id: number, isStarred: boolean) => void;
favoriteStatus: boolean;
dashboardFilter?: string;
userId?: number;
userId?: string | number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I don't think that this should ever be a string? Could you give me more context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it strange too, but it comes from already existing code: DashboardList.tsx:73 and I didn't want to dig deep into this for this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be number. You can take note of it for a follow up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be number. You can take note of it for a follow up.

showThumbnails?: boolean;
handleBulkDashboardExport: (dashboardsToExport: Dashboard[]) => void;
}
Expand Down Expand Up @@ -171,11 +171,13 @@ function DashboardCard({
e.preventDefault();
}}
>
<FaveStar
itemId={dashboard.id}
saveFaveStar={saveFavoriteStatus}
isStarred={favoriteStatus}
/>
{userId && (
<FaveStar
itemId={dashboard.id}
saveFaveStar={saveFavoriteStatus}
isStarred={favoriteStatus}
/>
)}
<AntdDropdown overlay={menu}>
<Icons.MoreVert iconColor={theme.colors.grayscale.base} />
</AntdDropdown>
Expand Down
68 changes: 61 additions & 7 deletions superset-frontend/src/views/CRUD/dashboard/DashboardList.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ import DashboardList from 'src/views/CRUD/dashboard/DashboardList';
import ListView from 'src/components/ListView';
import ListViewCard from 'src/components/ListViewCard';
import PropertiesModal from 'src/dashboard/components/PropertiesModal';
import FaveStar from 'src/components/FaveStar';
import TableCollection from 'src/components/TableCollection';
import CardCollection from 'src/components/ListView/CardCollection';

// store needed for withToasts(DashboardTable)
const mockStore = configureStore([thunk]);
Expand Down Expand Up @@ -104,15 +107,18 @@ describe('DashboardList', () => {
});

const mockedProps = {};
const wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<DashboardList {...mockedProps} user={mockUser} />
</Provider>
</MemoryRouter>,
);
let wrapper;

beforeAll(async () => {
fetchMock.resetHistory();
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<DashboardList {...mockedProps} user={mockUser} />
</Provider>
</MemoryRouter>,
);

await waitForComponentToPaint(wrapper);
});

Expand Down Expand Up @@ -178,6 +184,18 @@ describe('DashboardList', () => {
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', () => {
Expand Down Expand Up @@ -222,3 +240,39 @@ describe('RTL', () => {
expect(importTooltip).toBeInTheDocument();
});
});

describe('DashboardList - anonymous view', () => {
const mockedProps = {};
const mockUserLoggedOut = {};
let wrapper;

beforeAll(async () => {
fetchMock.resetHistory();
wrapper = mount(
<MemoryRouter>
<Provider store={store}>
<DashboardList {...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();
});
});
79 changes: 49 additions & 30 deletions superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
import { styled, SupersetClient, t } from '@superset-ui/core';
import React, { useState, useMemo } from 'react';
import React, { useState, useMemo, useCallback } from 'react';
import { Link } from 'react-router-dom';
import rison from 'rison';
import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
Expand Down Expand Up @@ -95,7 +95,11 @@ const Actions = styled.div`
`;

function DashboardList(props: DashboardListProps) {
const { addDangerToast, addSuccessToast } = props;
const {
addDangerToast,
addSuccessToast,
user: { userId },
} = props;

const {
state: {
Expand Down Expand Up @@ -143,7 +147,6 @@ function DashboardList(props: DashboardListProps) {
addSuccessToast(t('Dashboard imported'));
};

const { userId } = props.user;
// TODO: Fix usage of localStorage keying on the user id
const userKey = dangerouslyGetItemDoNotUse(userId?.toString(), null);

Expand Down Expand Up @@ -232,27 +235,25 @@ function DashboardList(props: DashboardListProps) {

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: 'xs',
},
]
: []),
{
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: {
Expand Down Expand Up @@ -422,10 +423,15 @@ function DashboardList(props: DashboardListProps) {
},
],
[
userId,
canEdit,
canDelete,
canExport,
...(props.user.userId ? [favoriteStatus] : []),
saveFavoriteStatus,
favoriteStatus,
refreshData,
addSuccessToast,
addDangerToast,
],
);

Expand Down Expand Up @@ -500,7 +506,7 @@ function DashboardList(props: DashboardListProps) {
{ label: t('Draft'), value: false },
],
},
...(props.user.userId ? [favoritesFilter] : []),
...(userId ? [favoritesFilter] : []),
{
Header: t('Certified'),
id: 'id',
Expand Down Expand Up @@ -544,8 +550,8 @@ function DashboardList(props: DashboardListProps) {
},
];

function renderCard(dashboard: Dashboard) {
return (
const renderCard = useCallback(
(dashboard: Dashboard) => (
<DashboardCard
dashboard={dashboard}
hasPerm={hasPerm}
Expand All @@ -556,6 +562,7 @@ function DashboardList(props: DashboardListProps) {
? userKey.thumbnails
: isFeatureEnabled(FeatureFlag.THUMBNAILS)
}
userId={userId}
loading={loading}
addDangerToast={addDangerToast}
addSuccessToast={addSuccessToast}
Expand All @@ -564,8 +571,20 @@ function DashboardList(props: DashboardListProps) {
favoriteStatus={favoriteStatus[dashboard.id]}
handleBulkDashboardExport={handleBulkDashboardExport}
/>
);
}
),
[
addDangerToast,
addSuccessToast,
bulkSelectEnabled,
favoriteStatus,
hasPerm,
loading,
userId,
refreshData,
saveFavoriteStatus,
userKey,
],
);

const subMenuButtons: SubMenuProps['buttons'] = [];
if (canDelete || canExport) {
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ export function handleDashboardDelete(
addSuccessToast: (arg0: string) => void,
addDangerToast: (arg0: string) => void,
dashboardFilter?: string,
userId?: number,
userId?: string | number,
) {
return SupersetClient.delete({
endpoint: `/api/v1/dashboard/${id}`,
Expand Down