From c6db6062fec7fbeb77cc472e5b41c03bd501af2a Mon Sep 17 00:00:00 2001 From: Juan Cabanas Date: Mon, 31 Oct 2022 12:51:45 -0300 Subject: [PATCH 1/3] orphaned public dashboard script added --- pkg/services/sqlstore/migrations/dashboard_public_mig.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/services/sqlstore/migrations/dashboard_public_mig.go b/pkg/services/sqlstore/migrations/dashboard_public_mig.go index 1dfc3512018e..abbdd1367511 100644 --- a/pkg/services/sqlstore/migrations/dashboard_public_mig.go +++ b/pkg/services/sqlstore/migrations/dashboard_public_mig.go @@ -74,4 +74,7 @@ func addPublicDashboardMigration(mg *Migrator) { Nullable: false, Default: "0", })) + + mg.AddMigration("delete orphaned public dashboards", NewRawSQLMigration( + "DELETE FROM dashboard_public WHERE dashboard_uid NOT IN (SELECT uid FROM dashboard)")) } From 159c43264894cf0a66e6abe869287935a0f1e3a6 Mon Sep 17 00:00:00 2001 From: Juan Cabanas Date: Tue, 1 Nov 2022 15:59:02 -0300 Subject: [PATCH 2/3] orphaned item list added --- .../publicdashboards/database/database.go | 4 +- .../PublicDashboardListTable.test.tsx | 91 +++++++++++++++---- .../PublicDashboardListTable.tsx | 44 +++++++-- 3 files changed, 114 insertions(+), 25 deletions(-) diff --git a/pkg/services/publicdashboards/database/database.go b/pkg/services/publicdashboards/database/database.go index 080bb2f1c1a4..10f2f1bb313d 100644 --- a/pkg/services/publicdashboards/database/database.go +++ b/pkg/services/publicdashboards/database/database.go @@ -38,9 +38,9 @@ func (d *PublicDashboardStoreImpl) FindAll(ctx context.Context, orgId int64) ([] resp := make([]PublicDashboardListResponse, 0) err := d.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { - sess.Table("dashboard_public"). + sess.Table("dashboard_public").Select( + "dashboard_public.uid, dashboard_public.access_token, dashboard.uid as dashboard_uid, dashboard_public.is_enabled, dashboard.title"). Join("LEFT", "dashboard", "dashboard.uid = dashboard_public.dashboard_uid AND dashboard.org_id = dashboard_public.org_id"). - Cols("dashboard_public.uid", "dashboard_public.access_token", "dashboard_public.dashboard_uid", "dashboard_public.is_enabled", "dashboard.title"). Where("dashboard_public.org_id = ?", orgId). OrderBy(" is_enabled DESC, dashboard.title IS NULL, dashboard.title ASC") diff --git a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx index a6fbb91e9078..254ea59467e0 100644 --- a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx +++ b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx @@ -32,6 +32,23 @@ const publicDashboardListResponse: ListPublicDashboardResponse[] = [ }, ]; +const orphanedDashboardListResponse: ListPublicDashboardResponse[] = [ + { + uid: 'SdZwuCZVz2', + accessToken: 'beeaf92f6ab3467f80b2be922c7741ab', + title: '', + dashboardUid: '', + isEnabled: false, + }, + { + uid: 'EuiEbd3nz2', + accessToken: '8687b0498ccf4babb2f92810d8563b33', + title: '', + dashboardUid: '', + isEnabled: true, + }, +]; + const server = setupServer( rest.get('/api/dashboards/public-dashboards', (_, res, ctx) => res(ctx.status(200), ctx.json(publicDashboardListResponse)) @@ -107,7 +124,11 @@ describe('Show table', () => { }); it('renders public dashboards in a good way without trashcan', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(false); - await renderPublicDashboardItems(); + + await renderPublicDashboardTable(true); + publicDashboardListResponse.forEach((pd, idx) => { + renderPublicDashboardItemCorrectly(pd, idx, false); + }); const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); @@ -120,7 +141,11 @@ describe('Show table', () => { }); it('renders public dashboards in a good way with trashcan', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); - await renderPublicDashboardItems(); + + await renderPublicDashboardTable(true); + publicDashboardListResponse.forEach((pd, idx) => { + renderPublicDashboardItemCorrectly(pd, idx, true); + }); const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); @@ -131,17 +156,25 @@ describe('Show table', () => { expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.trashcanButton)); }); }); + it('renders public dashboards items correctly', async () => { + jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); + await renderPublicDashboardTable(true); + + publicDashboardListResponse.forEach((pd, idx) => { + renderPublicDashboardItemCorrectly(pd, idx, true); + }); + }); }); describe('Delete public dashboard', () => { - it('when user does not have public dashboard write permissions, then dashboards are listed without delete button ', async () => { + it('when user does not have public dashboard write permissions, then dashboards are listed without delete button', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(false); await renderPublicDashboardTable(true); const tableBody = screen.getAllByRole('rowgroup')[1]; expect(within(tableBody).queryAllByTestId(selectors.ListItem.trashcanButton)).toHaveLength(0); }); - it('when user has public dashboard write permissions, then dashboards are listed with delete button ', async () => { + it('when user has public dashboard write permissions, then dashboards are listed with delete button', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); await renderPublicDashboardTable(true); @@ -152,20 +185,46 @@ describe('Delete public dashboard', () => { }); }); -const renderPublicDashboardItems = async () => { - await renderPublicDashboardTable(true); +describe('Orphaned public dashboard', () => { + it('renders orphaned and non orphaned public dashboards items correctly', async () => { + const response = [...publicDashboardListResponse, ...orphanedDashboardListResponse]; + server.use( + rest.get('/api/dashboards/public-dashboards', (req, res, ctx) => { + return res(ctx.status(200), ctx.json(response)); + }) + ); + jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); + + await renderPublicDashboardTable(true); + response.forEach((pd, idx) => { + renderPublicDashboardItemCorrectly(pd, idx, true); + }); + }); +}); + +const renderPublicDashboardItemCorrectly = (pd: ListPublicDashboardResponse, idx: number, hasWriteAccess: boolean) => { + const isOrphaned = !pd.dashboardUid; const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); - publicDashboardListResponse.forEach((pd, idx) => { - const tableRow = tableRows[idx]; - const rowDataCells = within(tableRow).getAllByRole('cell'); - expect(rowDataCells).toHaveLength(3); - - expect(within(rowDataCells[0]).getByText(pd.title)); - expect(within(rowDataCells[1]).getByText(pd.isEnabled ? 'enabled' : 'disabled')); - expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.linkButton)); - expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.configButton)); - }); + const tableRow = tableRows[idx]; + const rowDataCells = within(tableRow).getAllByRole('cell'); + expect(rowDataCells).toHaveLength(3); + + const statusTag = within(rowDataCells[1]).getByText(pd.isEnabled ? 'enabled' : 'disabled'); + const linkButton = within(rowDataCells[2]).getByTestId(selectors.ListItem.linkButton); + const configButton = within(rowDataCells[2]).getByTestId(selectors.ListItem.configButton); + const trashcanButton = within(rowDataCells[2]).queryByTestId(selectors.ListItem.trashcanButton); + + expect(within(rowDataCells[0]).getByText(isOrphaned ? 'Orphaned public dashboard' : pd.title)).toBeInTheDocument(); + expect(statusTag).toBeInTheDocument(); + isOrphaned ? expect(statusTag).toHaveStyle('background-color: rgb(110, 110, 110)') : expect(statusTag).toBeEnabled(); + isOrphaned || !pd.isEnabled + ? expect(linkButton).toHaveStyle('pointer-events: none') + : expect(linkButton).not.toHaveStyle('pointer-events: none'); + isOrphaned + ? expect(configButton).toHaveStyle('pointer-events: none') + : expect(configButton).not.toHaveStyle('pointer-events: none'); + hasWriteAccess ? expect(trashcanButton).toBeEnabled() : expect(trashcanButton).toBeNull(); }; diff --git a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx index 3470524a7661..016be5d06698 100644 --- a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx +++ b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx @@ -12,6 +12,8 @@ import { useListPublicDashboardsQuery } from 'app/features/dashboard/api/publicD import { isOrgAdmin } from 'app/features/plugins/admin/permissions'; import { AccessControlAction } from 'app/types'; +import { ListPublicDashboardResponse } from '../../types'; + import { DeletePublicDashboardButton } from './DeletePublicDashboardButton'; export const viewPublicDashboardUrl = (accessToken: string): string => @@ -41,17 +43,30 @@ export const PublicDashboardListTable = () => { - {publicDashboards?.map((pd) => ( + {publicDashboards?.map((pd: ListPublicDashboardResponse) => ( - - - {pd.title} - + + {!!pd.dashboardUid ? ( + + {pd.title} + + ) : ( +
+

Orphaned public dashboard

+ +
+ )} - + @@ -61,7 +76,7 @@ export const PublicDashboardListTable = () => { size={responsiveSize} title={pd.isEnabled ? 'View public dashboard' : 'Public dashboard is disabled'} target="_blank" - disabled={!pd.isEnabled} + disabled={!pd.isEnabled || !pd.dashboardUid} data-testid={selectors.ListItem.linkButton} > @@ -71,6 +86,7 @@ export const PublicDashboardListTable = () => { size={responsiveSize} href={`/d/${pd.dashboardUid}?shareView=share`} title="Configure public dashboard" + disabled={!pd.dashboardUid} data-testid={selectors.ListItem.configButton} > @@ -110,5 +126,19 @@ function getStyles(theme: GrafanaTheme2, isMobile: boolean) { buttonGroup: css` justify-content: ${isMobile ? 'space-between' : 'end'}; `, + orphanedTitle: css` + display: flex; + align-items: center; + + p { + margin: ${theme.spacing(0, 1, 0, 0)}; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + } + `, + orphanedInfoIcon: css` + color: ${theme.colors.text.link}; + `, }; } From b016428d965ef0bd228ba5bb85f924a32bdd365e Mon Sep 17 00:00:00 2001 From: Juan Cabanas Date: Tue, 1 Nov 2022 16:22:13 -0300 Subject: [PATCH 3/3] Revert "orphaned item list added" This reverts commit 159c43264894cf0a66e6abe869287935a0f1e3a6. --- .../publicdashboards/database/database.go | 4 +- .../PublicDashboardListTable.test.tsx | 91 ++++--------------- .../PublicDashboardListTable.tsx | 44 ++------- 3 files changed, 25 insertions(+), 114 deletions(-) diff --git a/pkg/services/publicdashboards/database/database.go b/pkg/services/publicdashboards/database/database.go index 10f2f1bb313d..080bb2f1c1a4 100644 --- a/pkg/services/publicdashboards/database/database.go +++ b/pkg/services/publicdashboards/database/database.go @@ -38,9 +38,9 @@ func (d *PublicDashboardStoreImpl) FindAll(ctx context.Context, orgId int64) ([] resp := make([]PublicDashboardListResponse, 0) err := d.sqlStore.WithDbSession(ctx, func(sess *db.Session) error { - sess.Table("dashboard_public").Select( - "dashboard_public.uid, dashboard_public.access_token, dashboard.uid as dashboard_uid, dashboard_public.is_enabled, dashboard.title"). + sess.Table("dashboard_public"). Join("LEFT", "dashboard", "dashboard.uid = dashboard_public.dashboard_uid AND dashboard.org_id = dashboard_public.org_id"). + Cols("dashboard_public.uid", "dashboard_public.access_token", "dashboard_public.dashboard_uid", "dashboard_public.is_enabled", "dashboard.title"). Where("dashboard_public.org_id = ?", orgId). OrderBy(" is_enabled DESC, dashboard.title IS NULL, dashboard.title ASC") diff --git a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx index 254ea59467e0..a6fbb91e9078 100644 --- a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx +++ b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.test.tsx @@ -32,23 +32,6 @@ const publicDashboardListResponse: ListPublicDashboardResponse[] = [ }, ]; -const orphanedDashboardListResponse: ListPublicDashboardResponse[] = [ - { - uid: 'SdZwuCZVz2', - accessToken: 'beeaf92f6ab3467f80b2be922c7741ab', - title: '', - dashboardUid: '', - isEnabled: false, - }, - { - uid: 'EuiEbd3nz2', - accessToken: '8687b0498ccf4babb2f92810d8563b33', - title: '', - dashboardUid: '', - isEnabled: true, - }, -]; - const server = setupServer( rest.get('/api/dashboards/public-dashboards', (_, res, ctx) => res(ctx.status(200), ctx.json(publicDashboardListResponse)) @@ -124,11 +107,7 @@ describe('Show table', () => { }); it('renders public dashboards in a good way without trashcan', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(false); - - await renderPublicDashboardTable(true); - publicDashboardListResponse.forEach((pd, idx) => { - renderPublicDashboardItemCorrectly(pd, idx, false); - }); + await renderPublicDashboardItems(); const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); @@ -141,11 +120,7 @@ describe('Show table', () => { }); it('renders public dashboards in a good way with trashcan', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); - - await renderPublicDashboardTable(true); - publicDashboardListResponse.forEach((pd, idx) => { - renderPublicDashboardItemCorrectly(pd, idx, true); - }); + await renderPublicDashboardItems(); const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); @@ -156,25 +131,17 @@ describe('Show table', () => { expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.trashcanButton)); }); }); - it('renders public dashboards items correctly', async () => { - jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); - await renderPublicDashboardTable(true); - - publicDashboardListResponse.forEach((pd, idx) => { - renderPublicDashboardItemCorrectly(pd, idx, true); - }); - }); }); describe('Delete public dashboard', () => { - it('when user does not have public dashboard write permissions, then dashboards are listed without delete button', async () => { + it('when user does not have public dashboard write permissions, then dashboards are listed without delete button ', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(false); await renderPublicDashboardTable(true); const tableBody = screen.getAllByRole('rowgroup')[1]; expect(within(tableBody).queryAllByTestId(selectors.ListItem.trashcanButton)).toHaveLength(0); }); - it('when user has public dashboard write permissions, then dashboards are listed with delete button', async () => { + it('when user has public dashboard write permissions, then dashboards are listed with delete button ', async () => { jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); await renderPublicDashboardTable(true); @@ -185,46 +152,20 @@ describe('Delete public dashboard', () => { }); }); -describe('Orphaned public dashboard', () => { - it('renders orphaned and non orphaned public dashboards items correctly', async () => { - const response = [...publicDashboardListResponse, ...orphanedDashboardListResponse]; - server.use( - rest.get('/api/dashboards/public-dashboards', (req, res, ctx) => { - return res(ctx.status(200), ctx.json(response)); - }) - ); - jest.spyOn(contextSrv, 'hasAccess').mockReturnValue(true); - - await renderPublicDashboardTable(true); - response.forEach((pd, idx) => { - renderPublicDashboardItemCorrectly(pd, idx, true); - }); - }); -}); - -const renderPublicDashboardItemCorrectly = (pd: ListPublicDashboardResponse, idx: number, hasWriteAccess: boolean) => { - const isOrphaned = !pd.dashboardUid; +const renderPublicDashboardItems = async () => { + await renderPublicDashboardTable(true); const tableBody = screen.getAllByRole('rowgroup')[1]; const tableRows = within(tableBody).getAllByRole('row'); - const tableRow = tableRows[idx]; - const rowDataCells = within(tableRow).getAllByRole('cell'); - expect(rowDataCells).toHaveLength(3); - - const statusTag = within(rowDataCells[1]).getByText(pd.isEnabled ? 'enabled' : 'disabled'); - const linkButton = within(rowDataCells[2]).getByTestId(selectors.ListItem.linkButton); - const configButton = within(rowDataCells[2]).getByTestId(selectors.ListItem.configButton); - const trashcanButton = within(rowDataCells[2]).queryByTestId(selectors.ListItem.trashcanButton); - - expect(within(rowDataCells[0]).getByText(isOrphaned ? 'Orphaned public dashboard' : pd.title)).toBeInTheDocument(); - expect(statusTag).toBeInTheDocument(); - isOrphaned ? expect(statusTag).toHaveStyle('background-color: rgb(110, 110, 110)') : expect(statusTag).toBeEnabled(); - isOrphaned || !pd.isEnabled - ? expect(linkButton).toHaveStyle('pointer-events: none') - : expect(linkButton).not.toHaveStyle('pointer-events: none'); - isOrphaned - ? expect(configButton).toHaveStyle('pointer-events: none') - : expect(configButton).not.toHaveStyle('pointer-events: none'); - hasWriteAccess ? expect(trashcanButton).toBeEnabled() : expect(trashcanButton).toBeNull(); + publicDashboardListResponse.forEach((pd, idx) => { + const tableRow = tableRows[idx]; + const rowDataCells = within(tableRow).getAllByRole('cell'); + expect(rowDataCells).toHaveLength(3); + + expect(within(rowDataCells[0]).getByText(pd.title)); + expect(within(rowDataCells[1]).getByText(pd.isEnabled ? 'enabled' : 'disabled')); + expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.linkButton)); + expect(within(rowDataCells[2]).getByTestId(selectors.ListItem.configButton)); + }); }; diff --git a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx index 016be5d06698..3470524a7661 100644 --- a/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx +++ b/public/app/features/manage-dashboards/components/PublicDashboardListTable/PublicDashboardListTable.tsx @@ -12,8 +12,6 @@ import { useListPublicDashboardsQuery } from 'app/features/dashboard/api/publicD import { isOrgAdmin } from 'app/features/plugins/admin/permissions'; import { AccessControlAction } from 'app/types'; -import { ListPublicDashboardResponse } from '../../types'; - import { DeletePublicDashboardButton } from './DeletePublicDashboardButton'; export const viewPublicDashboardUrl = (accessToken: string): string => @@ -43,30 +41,17 @@ export const PublicDashboardListTable = () => { - {publicDashboards?.map((pd: ListPublicDashboardResponse) => ( + {publicDashboards?.map((pd) => ( - - {!!pd.dashboardUid ? ( - - {pd.title} - - ) : ( -
-

Orphaned public dashboard

- -
- )} + + + {pd.title} + - + @@ -76,7 +61,7 @@ export const PublicDashboardListTable = () => { size={responsiveSize} title={pd.isEnabled ? 'View public dashboard' : 'Public dashboard is disabled'} target="_blank" - disabled={!pd.isEnabled || !pd.dashboardUid} + disabled={!pd.isEnabled} data-testid={selectors.ListItem.linkButton} > @@ -86,7 +71,6 @@ export const PublicDashboardListTable = () => { size={responsiveSize} href={`/d/${pd.dashboardUid}?shareView=share`} title="Configure public dashboard" - disabled={!pd.dashboardUid} data-testid={selectors.ListItem.configButton} > @@ -126,19 +110,5 @@ function getStyles(theme: GrafanaTheme2, isMobile: boolean) { buttonGroup: css` justify-content: ${isMobile ? 'space-between' : 'end'}; `, - orphanedTitle: css` - display: flex; - align-items: center; - - p { - margin: ${theme.spacing(0, 1, 0, 0)}; - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - } - `, - orphanedInfoIcon: css` - color: ${theme.colors.text.link}; - `, }; }