From a59e7e14b1fe68524010542eae0a8161e7f50b65 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed <1438972+tskarhed@users.noreply.github.com> Date: Thu, 2 May 2024 10:30:19 +0200 Subject: [PATCH] AnnotationList: Fix link for annotation with no panel or dashboard (#87048) * Handle case for no associated panel or dashboard * Convert to CSS object * Add tests and extend type to cover null use case --- .betterer.results | 3 +- .../grafana-data/src/types/annotations.ts | 3 +- .../panel/annolist/AnnoListPanel.test.tsx | 31 ++++++++++++++++++- .../plugins/panel/annolist/AnnoListPanel.tsx | 18 +++++------ 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/.betterer.results b/.betterer.results index 567a987c963..ca8e33efea0 100644 --- a/.betterer.results +++ b/.betterer.results @@ -5474,8 +5474,7 @@ exports[`better eslint`] = { [0, 0, 0, "Styles should be written using objects.", "6"] ], "public/app/plugins/panel/annolist/AnnoListPanel.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Styles should be written using objects.", "1"] + [0, 0, 0, "Do not use any type assertions.", "0"] ], "public/app/plugins/panel/barchart/BarChartPanel.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"], diff --git a/packages/grafana-data/src/types/annotations.ts b/packages/grafana-data/src/types/annotations.ts index 13b3e0abbbb..e76ddc16d51 100644 --- a/packages/grafana-data/src/types/annotations.ts +++ b/packages/grafana-data/src/types/annotations.ts @@ -26,7 +26,8 @@ export interface AnnotationEvent { id?: string; annotation?: any; dashboardId?: number; - dashboardUID?: string; + /** May be null if it isn't set via the HTTP API */ + dashboardUID?: string | null; panelId?: number; userId?: number; login?: string; diff --git a/public/app/plugins/panel/annolist/AnnoListPanel.test.tsx b/public/app/plugins/panel/annolist/AnnoListPanel.test.tsx index 75cbf6c4d24..a96e2c1fe03 100644 --- a/public/app/plugins/panel/annolist/AnnoListPanel.test.tsx +++ b/public/app/plugins/panel/annolist/AnnoListPanel.test.tsx @@ -59,6 +59,7 @@ async function setupTestContext({ const dashSrv = { getCurrent: () => dash } as DashboardSrv; setDashboardSrv(dashSrv); const pushSpy = jest.spyOn(locationService, 'push'); + const partialSpy = jest.spyOn(locationService, 'partial'); const props: Props = { data: { state: LoadingState.Done, timeRange: getDefaultTimeRange(), series: [] }, @@ -89,7 +90,7 @@ async function setupTestContext({ const { rerender } = render(); await waitFor(() => expect(getMock).toHaveBeenCalledTimes(1)); - return { props, rerender, getMock, pushSpy }; + return { props, rerender, getMock, pushSpy, partialSpy }; } describe('AnnoListPanel', () => { @@ -220,6 +221,34 @@ describe('AnnoListPanel', () => { expect(pushSpy).toHaveBeenCalledTimes(1); expect(pushSpy).toHaveBeenCalledWith('/d/asdkjhajksd/some-dash?from=1609458600000&to=1609459800000'); }); + + it('should default to the current dashboard, if no dashboard is associated with the annotation', async () => { + const { getMock, partialSpy } = await setupTestContext({ + results: [{ ...defaultResult, dashboardUID: null, panelId: 0 }], + }); + getMock.mockClear(); + expect(screen.getByRole('button', { name: /result text/i })).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: /result text/i })); + + expect(getMock).not.toHaveBeenCalled(); + expect(partialSpy).toHaveBeenCalledTimes(1); + expect(partialSpy).toHaveBeenCalledWith({ from: 1609458600000, to: 1609459800000, viewPanel: undefined }); + }); + + it('should default to the current dashboard, if no dashboard is associated with the annotation and navigate to viewPanel', async () => { + const { getMock, partialSpy } = await setupTestContext({ + results: [{ ...defaultResult, dashboardUID: null }], + }); + getMock.mockClear(); + expect(screen.getByRole('button', { name: /result text/i })).toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: /result text/i })); + + expect(getMock).not.toHaveBeenCalled(); + expect(partialSpy).toHaveBeenCalledTimes(1); + expect(partialSpy).toHaveBeenCalledWith({ from: 1609458600000, to: 1609459800000, viewPanel: 13 }); + }); }); describe('and the user clicks on a tag', () => { diff --git a/public/app/plugins/panel/annolist/AnnoListPanel.tsx b/public/app/plugins/panel/annolist/AnnoListPanel.tsx index 2e637d7c3bf..b43aae06250 100644 --- a/public/app/plugins/panel/annolist/AnnoListPanel.tsx +++ b/public/app/plugins/panel/annolist/AnnoListPanel.tsx @@ -146,10 +146,10 @@ export class AnnoListPanel extends PureComponent { const params = { from: this._timeOffset(anno.time, options.navigateBefore, true), to: this._timeOffset(anno.timeEnd ?? anno.time, options.navigateAfter, false), - viewPanel: options.navigateToPanel ? anno.panelId : undefined, + viewPanel: options.navigateToPanel && anno.panelId ? anno.panelId : undefined, }; - if (current?.uid === anno.dashboardUID) { + if (!anno.dashboardUID || current?.uid === anno.dashboardUID) { locationService.partial(params); return; } @@ -296,13 +296,13 @@ export class AnnoListPanel extends PureComponent { } const getStyles = stylesFactory((theme: GrafanaTheme2) => ({ - noneFound: css` - display: flex; - align-items: center; - justify-content: center; - width: 100%; - height: calc(100% - 30px); - `, + noneFound: css({ + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + width: '100%', + height: 'calc(100% - 30px)', + }), filter: css({ alignItems: 'center', display: 'flex',