From adfe20e88a476adc0675b1d879b8728baadbffc9 Mon Sep 17 00:00:00 2001 From: Vikrant Gupta Date: Thu, 18 Jul 2024 12:25:31 +0530 Subject: [PATCH] fix: url params should not propagate across pages (#5417) * fix: dashboards list url query params isolation * feat: order query param old logs explorer isolation * feat: added extra checks in place * fix: refactor the dashboards list page for better performance * chore: add test cases for the dashboards list page * fix: added test cases for dashboards list page * fix: added code comments * fix: added empty state for dashboards and no search state --- .../ListOfDashboard/DashboardsList.tsx | 54 +++-- .../DashboardDescription/index.tsx | 1 + .../mocks-server/__mockdata__/dashboards.ts | 50 +++++ frontend/src/mocks-server/handlers.ts | 5 + .../__tests__/DashboardListPage.test.tsx | 207 ++++++++++++++++++ .../src/providers/Dashboard/Dashboard.tsx | 63 +++++- frontend/src/providers/Dashboard/types.ts | 22 +- frontend/src/store/reducers/logs.ts | 30 ++- 8 files changed, 376 insertions(+), 56 deletions(-) create mode 100644 frontend/src/mocks-server/__mockdata__/dashboards.ts create mode 100644 frontend/src/pages/DashboardsListPage/__tests__/DashboardListPage.test.tsx diff --git a/frontend/src/container/ListOfDashboard/DashboardsList.tsx b/frontend/src/container/ListOfDashboard/DashboardsList.tsx index f3dc600f9b8a..05847cebde53 100644 --- a/frontend/src/container/ListOfDashboard/DashboardsList.tsx +++ b/frontend/src/container/ListOfDashboard/DashboardsList.tsx @@ -73,7 +73,6 @@ import { Dashboard } from 'types/api/dashboard/getAll'; import AppReducer from 'types/reducer/app'; import { isCloudUser } from 'utils/app'; -import useUrlQuery from '../../hooks/useUrlQuery'; import DashboardTemplatesModal from './DashboardTemplates/DashboardTemplatesModal'; import ImportJSON from './ImportJSON'; import { DeleteButton } from './TableComponents/DeleteButton'; @@ -86,7 +85,7 @@ import { // eslint-disable-next-line sonarjs/cognitive-complexity function DashboardsList(): JSX.Element { const { - data: dashboardListResponse = [], + data: dashboardListResponse, isLoading: isDashboardListLoading, error: dashboardFetchError, refetch: refetchDashboardList, @@ -99,12 +98,14 @@ function DashboardsList(): JSX.Element { setListSortOrder: setSortOrder, } = useDashboard(); + const [searchString, setSearchString] = useState( + sortOrder.search || '', + ); const [action, createNewDashboard] = useComponentPermission( ['action', 'create_new_dashboards'], role, ); - const [searchValue, setSearchValue] = useState(''); const [ showNewDashboardTemplatesModal, setShowNewDashboardTemplatesModal, @@ -123,10 +124,6 @@ function DashboardsList(): JSX.Element { false, ); - const params = useUrlQuery(); - const searchParams = params.get('search'); - const [searchString, setSearchString] = useState(searchParams || ''); - const getLocalStorageDynamicColumns = (): DashboardDynamicColumns => { const dashboardDynamicColumnsString = localStorage.getItem('dashboard'); let dashboardDynamicColumns: DashboardDynamicColumns = { @@ -188,14 +185,6 @@ function DashboardsList(): JSX.Element { setDashboards(sortedDashboards); }; - useEffect(() => { - params.set('columnKey', sortOrder.columnKey as string); - params.set('order', sortOrder.order as string); - params.set('page', sortOrder.pagination || '1'); - history.replace({ search: params.toString() }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sortOrder]); - const sortHandle = (key: string): void => { if (!dashboards) return; if (key === 'createdAt') { @@ -204,6 +193,7 @@ function DashboardsList(): JSX.Element { columnKey: 'createdAt', order: 'descend', pagination: sortOrder.pagination || '1', + search: sortOrder.search || '', }); } else if (key === 'updatedAt') { sortDashboardsByUpdatedAt(dashboards); @@ -211,21 +201,19 @@ function DashboardsList(): JSX.Element { columnKey: 'updatedAt', order: 'descend', pagination: sortOrder.pagination || '1', + search: sortOrder.search || '', }); } }; function handlePageSizeUpdate(page: number): void { - setSortOrder((order) => ({ - ...order, - pagination: String(page), - })); + setSortOrder({ ...sortOrder, pagination: String(page) }); } useEffect(() => { const filteredDashboards = filterDashboard( searchString, - dashboardListResponse, + dashboardListResponse || [], ); if (sortOrder.columnKey === 'updatedAt') { sortDashboardsByUpdatedAt(filteredDashboards || []); @@ -236,6 +224,7 @@ function DashboardsList(): JSX.Element { columnKey: 'updatedAt', order: 'descend', pagination: sortOrder.pagination || '1', + search: sortOrder.search || '', }); sortDashboardsByUpdatedAt(filteredDashboards || []); } @@ -245,6 +234,7 @@ function DashboardsList(): JSX.Element { setSortOrder, sortOrder.columnKey, sortOrder.pagination, + sortOrder.search, ]); const [newDashboardState, setNewDashboardState] = useState({ @@ -316,12 +306,15 @@ function DashboardsList(): JSX.Element { const handleSearch = (event: ChangeEvent): void => { setIsFilteringDashboards(true); - setSearchValue(event.target.value); const searchText = (event as React.BaseSyntheticEvent)?.target?.value || ''; - const filteredDashboards = filterDashboard(searchText, dashboardListResponse); + const filteredDashboards = filterDashboard( + searchText, + dashboardListResponse || [], + ); setDashboards(filteredDashboards); setIsFilteringDashboards(false); setSearchString(searchText); + setSortOrder({ ...sortOrder, search: searchText }); }; const [state, setCopy] = useCopyToClipboard(); @@ -412,7 +405,7 @@ function DashboardsList(): JSX.Element { { title: 'Dashboards', key: 'dashboard', - render: (dashboard: Data): JSX.Element => { + render: (dashboard: Data, _, index): JSX.Element => { const timeOptions: Intl.DateTimeFormatOptions = { hour: '2-digit', minute: '2-digit', @@ -461,7 +454,9 @@ function DashboardsList(): JSX.Element { style={{ height: '14px', width: '14px' }} alt="dashboard-image" /> - {dashboard.name} + + {dashboard.name} +
@@ -701,7 +696,7 @@ function DashboardsList(): JSX.Element {
- ) : dashboards?.length === 0 && !searchValue ? ( + ) : dashboards?.length === 0 && !searchString ? (
{ window.open( 'https://signoz.io/docs/userguide/manage-dashboards?utm_source=product&utm_medium=dashboard-list-empty-state', @@ -758,7 +754,7 @@ function DashboardsList(): JSX.Element { } - value={searchValue} + value={searchString} onChange={handleSearch} /> {createNewDashboard && ( @@ -786,7 +782,7 @@ function DashboardsList(): JSX.Element {
img - No dashboards found for {searchValue}. Create a new dashboard? + No dashboards found for {searchString}. Create a new dashboard?
) : ( @@ -808,6 +804,7 @@ function DashboardsList(): JSX.Element { type="text" className={cx('sort-btns')} onClick={(): void => sortHandle('createdAt')} + data-testid="sort-by-last-created" > Last created {sortOrder.columnKey === 'createdAt' && } @@ -816,6 +813,7 @@ function DashboardsList(): JSX.Element { type="text" className={cx('sort-btns')} onClick={(): void => sortHandle('updatedAt')} + data-testid="sort-by-last-updated" > Last updated {sortOrder.columnKey === 'updatedAt' && } @@ -826,7 +824,7 @@ function DashboardsList(): JSX.Element { placement="bottomRight" arrow={false} > - + + res(ctx.status(200), ctx.json(dashboardSuccessResponse)), + ), + rest.get('http://localhost/api/v1/invite', (_, res, ctx) => res(ctx.status(200), ctx.json(inviteUser)), ), diff --git a/frontend/src/pages/DashboardsListPage/__tests__/DashboardListPage.test.tsx b/frontend/src/pages/DashboardsListPage/__tests__/DashboardListPage.test.tsx new file mode 100644 index 000000000000..98bd40ef6259 --- /dev/null +++ b/frontend/src/pages/DashboardsListPage/__tests__/DashboardListPage.test.tsx @@ -0,0 +1,207 @@ +/* eslint-disable sonarjs/no-duplicate-string */ +import ROUTES from 'constants/routes'; +import DashboardsList from 'container/ListOfDashboard'; +import { dashboardEmptyState } from 'mocks-server/__mockdata__/dashboards'; +import { server } from 'mocks-server/server'; +import { rest } from 'msw'; +import { DashboardProvider } from 'providers/Dashboard/Dashboard'; +import { MemoryRouter, useLocation } from 'react-router-dom'; +import { fireEvent, render, waitFor } from 'tests/test-utils'; + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useLocation: jest.fn(), + useRouteMatch: jest.fn().mockReturnValue({ + params: { + dashboardId: 4, + }, + }), +})); + +const mockWindowOpen = jest.fn(); +window.open = mockWindowOpen; + +describe('dashboard list page', () => { + // should render on updatedAt and descend when the column key and order is messed up + it('should render the list even when the columnKey or the order is mismatched', async () => { + const mockLocation = { + pathname: `${process.env.FRONTEND_API_ENDPOINT}/${ROUTES.ALL_DASHBOARD}/`, + search: `columnKey=asgard&order=stones&page=1`, + }; + (useLocation as jest.Mock).mockReturnValue(mockLocation); + const { getByText, getByTestId } = render( + + + + + , + ); + + await waitFor(() => expect(getByText('All Dashboards')).toBeInTheDocument()); + const firstElement = getByTestId('dashboard-title-0'); + expect(firstElement.textContent).toBe('captain america'); + const secondElement = getByTestId('dashboard-title-1'); + expect(secondElement.textContent).toBe('thor'); + }); + + // should render correctly when the column key is createdAt and order is descend + it('should render the list even when the columnKey and the order are given', async () => { + const mockLocation = { + pathname: `${process.env.FRONTEND_API_ENDPOINT}/${ROUTES.ALL_DASHBOARD}/`, + search: `columnKey=createdAt&order=descend&page=1`, + }; + (useLocation as jest.Mock).mockReturnValue(mockLocation); + const { getByText, getByTestId } = render( + + + + + , + ); + + await waitFor(() => expect(getByText('All Dashboards')).toBeInTheDocument()); + const firstElement = getByTestId('dashboard-title-0'); + expect(firstElement.textContent).toBe('thor'); + const secondElement = getByTestId('dashboard-title-1'); + expect(secondElement.textContent).toBe('captain america'); + }); + + // change the sort by order and dashboards list ot be updated accordingly + it('dashboards list should be correctly updated on choosing the different sortBy from dropdown values', async () => { + const { getByText, getByTestId } = render( + + + + + , + ); + + await waitFor(() => expect(getByText('All Dashboards')).toBeInTheDocument()); + + const firstElement = getByTestId('dashboard-title-0'); + expect(firstElement.textContent).toBe('thor'); + const secondElement = getByTestId('dashboard-title-1'); + expect(secondElement.textContent).toBe('captain america'); + + // click on the sort button + const sortByButton = getByTestId('sort-by'); + expect(sortByButton).toBeInTheDocument(); + fireEvent.click(sortByButton!); + + // change the sort order + const sortByUpdatedBy = getByTestId('sort-by-last-updated'); + await waitFor(() => expect(sortByUpdatedBy).toBeInTheDocument()); + fireEvent.click(sortByUpdatedBy!); + + // expect the new order + const updatedFirstElement = getByTestId('dashboard-title-0'); + expect(updatedFirstElement.textContent).toBe('captain america'); + const updatedSecondElement = getByTestId('dashboard-title-1'); + expect(updatedSecondElement.textContent).toBe('thor'); + }); + + // should filter correctly on search string + it('should filter dashboards based on search string', async () => { + const mockLocation = { + pathname: `${process.env.FRONTEND_API_ENDPOINT}/${ROUTES.ALL_DASHBOARD}/`, + search: `columnKey=createdAt&order=descend&page=1&search=tho`, + }; + (useLocation as jest.Mock).mockReturnValue(mockLocation); + const { getByText, getByTestId, queryByText } = render( + + + + + , + ); + + await waitFor(() => expect(getByText('All Dashboards')).toBeInTheDocument()); + const firstElement = getByTestId('dashboard-title-0'); + expect(firstElement.textContent).toBe('thor'); + expect(queryByText('captain america')).not.toBeInTheDocument(); + + // the pagination item should not be present in the list when number of items are less than one page size + expect( + document.querySelector('.ant-table-pagination'), + ).not.toBeInTheDocument(); + }); + + it('dashboard empty search state', async () => { + const mockLocation = { + pathname: `${process.env.FRONTEND_API_ENDPOINT}/${ROUTES.ALL_DASHBOARD}/`, + search: `columnKey=createdAt&order=descend&page=1&search=someRandomString`, + }; + (useLocation as jest.Mock).mockReturnValue(mockLocation); + const { getByText } = render( + + + + + , + ); + + await waitFor(() => + expect( + getByText( + 'No dashboards found for someRandomString. Create a new dashboard?', + ), + ).toBeInTheDocument(), + ); + }); + + it('dashboard empty state', async () => { + const mockLocation = { + pathname: `${process.env.FRONTEND_API_ENDPOINT}/${ROUTES.ALL_DASHBOARD}/`, + search: `columnKey=createdAt&order=descend&page=1`, + }; + (useLocation as jest.Mock).mockReturnValue(mockLocation); + server.use( + rest.get('http://localhost/api/v1/dashboards', (_, res, ctx) => + res(ctx.status(200), ctx.json(dashboardEmptyState)), + ), + ); + const { getByText, getByTestId } = render( + + + + + , + ); + + await waitFor(() => + expect(getByText('No dashboards yet.')).toBeInTheDocument(), + ); + + const learnMoreButton = getByTestId('learn-more'); + expect(learnMoreButton).toBeInTheDocument(); + fireEvent.click(learnMoreButton); + + // test the correct link to be added for the dashboards empty state + await waitFor(() => + expect(mockWindowOpen).toHaveBeenCalledWith( + 'https://signoz.io/docs/userguide/manage-dashboards?utm_source=product&utm_medium=dashboard-list-empty-state', + '_blank', + ), + ); + }); +}); diff --git a/frontend/src/providers/Dashboard/Dashboard.tsx b/frontend/src/providers/Dashboard/Dashboard.tsx index 6e15b9e3b2a2..364fbd194411 100644 --- a/frontend/src/providers/Dashboard/Dashboard.tsx +++ b/frontend/src/providers/Dashboard/Dashboard.tsx @@ -1,3 +1,4 @@ +/* eslint-disable no-nested-ternary */ import { Modal } from 'antd'; import getDashboard from 'api/dashboard/get'; import lockDashboardApi from 'api/dashboard/lockDashboard'; @@ -11,6 +12,7 @@ import useAxiosError from 'hooks/useAxiosError'; import useTabVisibility from 'hooks/useTabFocus'; import useUrlQuery from 'hooks/useUrlQuery'; import { getUpdatedLayout } from 'lib/dashboard/getUpdatedLayout'; +import history from 'lib/history'; import { defaultTo } from 'lodash-es'; import isEqual from 'lodash-es/isEqual'; import isUndefined from 'lodash-es/isUndefined'; @@ -38,7 +40,7 @@ import AppReducer from 'types/reducer/app'; import { GlobalReducer } from 'types/reducer/globalTime'; import { v4 as generateUUID } from 'uuid'; -import { IDashboardContext } from './types'; +import { DashboardSortOrder, IDashboardContext } from './types'; import { sortLayout } from './util'; const DashboardContext = createContext({ @@ -52,7 +54,12 @@ const DashboardContext = createContext({ layouts: [], panelMap: {}, setPanelMap: () => {}, - listSortOrder: { columnKey: 'createdAt', order: 'descend', pagination: '1' }, + listSortOrder: { + columnKey: 'createdAt', + order: 'descend', + pagination: '1', + search: '', + }, setListSortOrder: () => {}, setLayouts: () => {}, setSelectedDashboard: () => {}, @@ -68,6 +75,7 @@ interface Props { dashboardId: string; } +// eslint-disable-next-line sonarjs/cognitive-complexity export function DashboardProvider({ children, }: PropsWithChildren): JSX.Element { @@ -82,17 +90,50 @@ export function DashboardProvider({ exact: true, }); - const params = useUrlQuery(); - const orderColumnParam = params.get('columnKey'); - const orderQueryParam = params.get('order'); - const paginationParam = params.get('page'); - - const [listSortOrder, setListSortOrder] = useState({ - columnKey: orderColumnParam || 'updatedAt', - order: orderQueryParam || 'descend', - pagination: paginationParam || '1', + const isDashboardListPage = useRouteMatch({ + path: ROUTES.ALL_DASHBOARD, + exact: true, }); + // added extra checks here in case wrong values appear use the default values rather than empty dashboards + const supportedOrderColumnKeys = ['createdAt', 'updatedAt']; + + const supportedOrderKeys = ['ascend', 'descend']; + + const params = useUrlQuery(); + // since the dashboard provider is wrapped at the very top of the application hence it initialises these values from other pages as well. + // pick the below params from URL only if the user is on the dashboards list page. + const orderColumnParam = isDashboardListPage && params.get('columnKey'); + const orderQueryParam = isDashboardListPage && params.get('order'); + const paginationParam = isDashboardListPage && params.get('page'); + const searchParam = isDashboardListPage && params.get('search'); + + const [listSortOrder, setListOrder] = useState({ + columnKey: orderColumnParam + ? supportedOrderColumnKeys.includes(orderColumnParam) + ? orderColumnParam + : 'updatedAt' + : 'updatedAt', + order: orderQueryParam + ? supportedOrderKeys.includes(orderQueryParam) + ? orderQueryParam + : 'descend' + : 'descend', + pagination: paginationParam || '1', + search: searchParam || '', + }); + + function setListSortOrder(sortOrder: DashboardSortOrder): void { + if (!isEqual(sortOrder, listSortOrder)) { + setListOrder(sortOrder); + } + params.set('columnKey', sortOrder.columnKey as string); + params.set('order', sortOrder.order as string); + params.set('page', sortOrder.pagination || '1'); + params.set('search', sortOrder.search || ''); + history.replace({ search: params.toString() }); + } + const dispatch = useDispatch>(); const globalTime = useSelector( diff --git a/frontend/src/providers/Dashboard/types.ts b/frontend/src/providers/Dashboard/types.ts index d72c1839f518..e19c00e42268 100644 --- a/frontend/src/providers/Dashboard/types.ts +++ b/frontend/src/providers/Dashboard/types.ts @@ -1,9 +1,15 @@ import dayjs from 'dayjs'; -import { Dispatch, SetStateAction } from 'react'; import { Layout } from 'react-grid-layout'; import { UseQueryResult } from 'react-query'; import { Dashboard } from 'types/api/dashboard/getAll'; +export interface DashboardSortOrder { + columnKey: string; + order: string; + pagination: string; + search: string; +} + export interface IDashboardContext { isDashboardSliderOpen: boolean; isDashboardLocked: boolean; @@ -15,18 +21,8 @@ export interface IDashboardContext { layouts: Layout[]; panelMap: Record; setPanelMap: React.Dispatch>>; - listSortOrder: { - columnKey: string; - order: string; - pagination: string; - }; - setListSortOrder: Dispatch< - SetStateAction<{ - columnKey: string; - order: string; - pagination: string; - }> - >; + listSortOrder: DashboardSortOrder; + setListSortOrder: (sortOrder: DashboardSortOrder) => void; setLayouts: React.Dispatch>; setSelectedDashboard: React.Dispatch< React.SetStateAction diff --git a/frontend/src/store/reducers/logs.ts b/frontend/src/store/reducers/logs.ts index 0d0a69a1a454..4e46d00e6977 100644 --- a/frontend/src/store/reducers/logs.ts +++ b/frontend/src/store/reducers/logs.ts @@ -1,3 +1,4 @@ +import ROUTES from 'constants/routes'; import { parseQuery } from 'lib/logql'; import { OrderPreferenceItems } from 'pages/Logs/config'; import { @@ -29,6 +30,30 @@ import { } from 'types/actions/logs'; import { ILogsReducer } from 'types/reducer/logs'; +const supportedLogsOrder = [ + OrderPreferenceItems.ASC, + OrderPreferenceItems.DESC, +]; + +function getLogsOrder(): OrderPreferenceItems { + // set the value of order from the URL only when order query param is present and the user is landing on the old logs explorer page + if (window.location.pathname === ROUTES.OLD_LOGS_EXPLORER) { + const orderParam = new URLSearchParams(window.location.search).get('order'); + + if (orderParam) { + // check if the order passed is supported else pass the default order + if (supportedLogsOrder.includes(orderParam as OrderPreferenceItems)) { + return orderParam as OrderPreferenceItems; + } + + return OrderPreferenceItems.DESC; + } + return OrderPreferenceItems.DESC; + } + + return OrderPreferenceItems.DESC; +} + const initialState: ILogsReducer = { fields: { interesting: [], @@ -51,10 +76,7 @@ const initialState: ILogsReducer = { liveTailStartRange: 15, selectedLogId: null, detailedLog: null, - order: - (new URLSearchParams(window.location.search).get( - 'order', - ) as ILogsReducer['order']) ?? OrderPreferenceItems.DESC, + order: getLogsOrder(), }; export const LogsReducer = (