Skip to content

Commit

Permalink
Reapply "[Cases] Fix cases flaky tests (#176863)" (#177798)
Browse files Browse the repository at this point in the history
## Summary

This reverts commit cf942f2. It also moves the `appId` and `appTitle`
outside the cases context. The `useApplication` hook is used when needed
to get the `appId` and `appTitle`. Lastly, a new hook called
`useCasesLocalStorage` was created to be used when interacting with the
local storage. It will use the `owner` to namespace the keys of the
local storage and fallback to the `appId` if the `owner` is empty.

Fixes #175204
Fixes #175570

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
cnasikas and kibanamachine authored Mar 9, 2024
1 parent f60b448 commit 48d8230
Show file tree
Hide file tree
Showing 68 changed files with 892 additions and 551 deletions.
2 changes: 0 additions & 2 deletions x-pack/plugins/cases/public/client/ui/get_cases.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export const getCasesLazy = ({
owner,
permissions,
basePath,
onComponentInitialized,
actionsNavigation,
ruleDetailsNavigation,
showAlertDetails,
Expand All @@ -52,7 +51,6 @@ export const getCasesLazy = ({
>
<Suspense fallback={<EuiLoadingSpinner />}>
<CasesRoutesLazy
onComponentInitialized={onComponentInitialized}
actionsNavigation={actionsNavigation}
ruleDetailsNavigation={ruleDetailsNavigation}
showAlertDetails={showAlertDetails}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@ describe('cases transactions', () => {
);
expect(mockAddLabels).toHaveBeenCalledWith({ alert_count: 3 });
});

it('should not start any transactions if the app ID is not defined', () => {
const { result } = renderUseCreateCaseWithAttachmentsTransaction();

result.current.startTransaction();

expect(mockStartTransaction).not.toHaveBeenCalled();
expect(mockAddLabels).not.toHaveBeenCalled();
});
});

describe('useAddAttachmentToExistingCaseTransaction', () => {
Expand All @@ -104,5 +113,14 @@ describe('cases transactions', () => {
);
expect(mockAddLabels).toHaveBeenCalledWith({ alert_count: 3 });
});

it('should not start any transactions if the app ID is not defined', () => {
const { result } = renderUseAddAttachmentToExistingCaseTransaction();

result.current.startTransaction({ attachments: bulkAttachments });

expect(mockStartTransaction).not.toHaveBeenCalled();
expect(mockAddLabels).not.toHaveBeenCalled();
});
});
});
21 changes: 17 additions & 4 deletions x-pack/plugins/cases/public/common/apm/use_cases_transactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ const BULK_ADD_ATTACHMENT_TO_NEW_CASE = 'bulkAddAttachmentsToNewCase' as const;
const ADD_ATTACHMENT_TO_EXISTING_CASE = 'addAttachmentToExistingCase' as const;
const BULK_ADD_ATTACHMENT_TO_EXISTING_CASE = 'bulkAddAttachmentsToExistingCase' as const;

export type StartCreateCaseWithAttachmentsTransaction = (param: {
appId: string;
export type StartCreateCaseWithAttachmentsTransaction = (param?: {
appId?: string;
attachments?: CaseAttachmentsWithoutOwner;
}) => Transaction | undefined;

Expand All @@ -28,11 +28,17 @@ export const useCreateCaseWithAttachmentsTransaction = () => {

const startCreateCaseWithAttachmentsTransaction =
useCallback<StartCreateCaseWithAttachmentsTransaction>(
({ appId, attachments }) => {
({ appId, attachments } = {}) => {
if (!appId) {
return;
}

if (!attachments) {
return startTransaction(`Cases [${appId}] ${CREATE_CASE}`);
}

const alertCount = getAlertCount(attachments);

if (alertCount <= 1) {
return startTransaction(`Cases [${appId}] ${ADD_ATTACHMENT_TO_NEW_CASE}`);
}
Expand All @@ -48,7 +54,7 @@ export const useCreateCaseWithAttachmentsTransaction = () => {
};

export type StartAddAttachmentToExistingCaseTransaction = (param: {
appId: string;
appId?: string;
attachments: CaseAttachmentsWithoutOwner;
}) => Transaction | undefined;

Expand All @@ -59,13 +65,20 @@ export const useAddAttachmentToExistingCaseTransaction = () => {
const startAddAttachmentToExistingCaseTransaction =
useCallback<StartAddAttachmentToExistingCaseTransaction>(
({ appId, attachments }) => {
if (!appId) {
return;
}

const alertCount = getAlertCount(attachments);

if (alertCount <= 1) {
return startTransaction(`Cases [${appId}] ${ADD_ATTACHMENT_TO_EXISTING_CASE}`);
}

const transaction = startTransaction(
`Cases [${appId}] ${BULK_ADD_ATTACHMENT_TO_EXISTING_CASE}`
);

transaction?.addLabels({ alert_count: alertCount });
return transaction;
},
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/cases/public/common/hooks.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import { renderHook } from '@testing-library/react-hooks';

import { TestProviders } from './mock';
import { useIsMainApplication } from './hooks';
import { useApplication } from '../components/cases_context/use_application';
import { useApplication } from './lib/kibana/use_application';

jest.mock('../components/cases_context/use_application');
jest.mock('./lib/kibana/use_application');

const useApplicationMock = useApplication as jest.Mock;

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/cases/public/common/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
*/

import { STACK_APP_ID } from '../../common/constants';
import { useCasesContext } from '../components/cases_context/use_cases_context';
import { useApplication } from './lib/kibana/use_application';

export const useIsMainApplication = () => {
const { appId } = useCasesContext();
const { appId } = useApplication();

return appId === STACK_APP_ID;
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export const useKibana = jest.fn().mockReturnValue({
export const useHttp = jest.fn().mockReturnValue(createStartServicesMock().http);
export const useTimeZone = jest.fn();
export const useDateFormat = jest.fn();
export const useBasePath = jest.fn(() => '/test/base/path');
export const useToasts = jest
.fn()
.mockReturnValue(notificationServiceMock.createStartContract().toasts);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export const useApplication = jest
.fn()
.mockReturnValue({ appId: 'testAppId', appTitle: 'test-title' });
12 changes: 5 additions & 7 deletions x-pack/plugins/cases/public/common/lib/kibana/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ export const useTimeZone = (): string => {
return timeZone === 'Browser' ? moment.tz.guess() : timeZone;
};

export const useBasePath = (): string => useKibana().services.http.basePath.get();

export const useToasts = (): StartServices['notifications']['toasts'] =>
useKibana().services.notifications.toasts;

Expand Down Expand Up @@ -116,12 +114,12 @@ export const useCurrentUser = (): AuthenticatedElasticUser | null => {
* Returns a full URL to the provided page path by using
* kibana's `getUrlForApp()`
*/
export const useAppUrl = (appId: string) => {
export const useAppUrl = (appId?: string) => {
const { getUrlForApp } = useKibana().services.application;

const getAppUrl = useCallback(
(options?: { deepLinkId?: string; path?: string; absolute?: boolean }) =>
getUrlForApp(appId, options),
getUrlForApp(appId ?? '', options),
[appId, getUrlForApp]
);
return { getAppUrl };
Expand All @@ -131,7 +129,7 @@ export const useAppUrl = (appId: string) => {
* Navigate to any app using kibana's `navigateToApp()`
* or by url using `navigateToUrl()`
*/
export const useNavigateTo = (appId: string) => {
export const useNavigateTo = (appId?: string) => {
const { navigateToApp, navigateToUrl } = useKibana().services.application;

const navigateTo = useCallback(
Expand All @@ -144,7 +142,7 @@ export const useNavigateTo = (appId: string) => {
if (url) {
navigateToUrl(url);
} else {
navigateToApp(appId, options);
navigateToApp(appId ?? '', options);
}
},
[appId, navigateToApp, navigateToUrl]
Expand All @@ -156,7 +154,7 @@ export const useNavigateTo = (appId: string) => {
* Returns navigateTo and getAppUrl navigation hooks
*
*/
export const useNavigation = (appId: string) => {
export const useNavigation = (appId?: string) => {
const { navigateTo } = useNavigateTo(appId);
const { getAppUrl } = useAppUrl(appId);
return { navigateTo, getAppUrl };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React from 'react';
import { BehaviorSubject } from 'rxjs';

import type { PublicAppInfo } from '@kbn/core/public';
import { AppStatus } from '@kbn/core/public';
import type { RecursivePartial } from '@elastic/eui/src/components/common';
import { coreMock } from '@kbn/core/public/mocks';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
Expand Down Expand Up @@ -52,7 +53,21 @@ export const createStartServicesMock = ({ license }: StartServiceArgs = {}): Sta

services.application.currentAppId$ = new BehaviorSubject<string>('testAppId');
services.application.applications$ = new BehaviorSubject<Map<string, PublicAppInfo>>(
new Map([['testAppId', { category: { label: 'Test' } } as unknown as PublicAppInfo]])
new Map([
[
'testAppId',
{
id: 'testAppId',
title: 'test-title',
category: { id: 'test-label-id', label: 'Test' },
status: AppStatus.accessible,
visibleIn: ['globalSearch'],
appRoute: `/app/some-id`,
keywords: [],
deepLinks: [],
},
],
])
);

services.triggersActionsUi.actionTypeRegistry.get = jest.fn().mockReturnValue({
Expand Down
108 changes: 108 additions & 0 deletions x-pack/plugins/cases/public/common/lib/kibana/use_application.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type { PublicAppInfo } from '@kbn/core-application-browser';
import { AppStatus } from '@kbn/core-application-browser';
import { renderHook } from '@testing-library/react-hooks';
import { BehaviorSubject, Subject } from 'rxjs';
import type { AppMockRenderer } from '../../mock';
import { createAppMockRenderer } from '../../mock';
import { useApplication } from './use_application';

describe('useApplication', () => {
let appMockRender: AppMockRenderer;

beforeEach(() => {
jest.clearAllMocks();
appMockRender = createAppMockRenderer();
});

const getApp = (props: Partial<PublicAppInfo> = {}): PublicAppInfo => ({
id: 'testAppId',
title: 'Test title',
status: AppStatus.accessible,
visibleIn: ['globalSearch'],
appRoute: `/app/some-id`,
keywords: [],
deepLinks: [],
...props,
});

it('returns the appId and the appTitle correctly', () => {
const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({
appId: 'testAppId',
appTitle: 'Test',
});
});

it('returns undefined appId and appTitle if the currentAppId observable is not defined', () => {
appMockRender.coreStart.application.currentAppId$ = new Subject();

const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({});
});

it('returns undefined appTitle if the applications observable is not defined', () => {
appMockRender.coreStart.application.applications$ = new Subject();

const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({
appId: 'testAppId',
appTitle: undefined,
});
});

it('returns the label as appTitle', () => {
appMockRender.coreStart.application.applications$ = new BehaviorSubject(
new Map([['testAppId', getApp({ category: { id: 'test-label-id', label: 'Test label' } })]])
);

const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({
appId: 'testAppId',
appTitle: 'Test label',
});
});

it('returns the title as appTitle if the categories label is missing', () => {
appMockRender.coreStart.application.applications$ = new BehaviorSubject(
new Map([['testAppId', getApp({ title: 'Test title' })]])
);

const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({
appId: 'testAppId',
appTitle: 'Test title',
});
});

it('gets the value from the default value of the currentAppId observable if it exists', () => {
appMockRender.coreStart.application.currentAppId$ = new BehaviorSubject('new-test-id');

const { result } = renderHook(() => useApplication(), {
wrapper: appMockRender.AppWrapper,
});

expect(result.current).toEqual({ appId: 'new-test-id' });
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import useObservable from 'react-use/lib/useObservable';
import type { BehaviorSubject, Observable } from 'rxjs';
import { useKibana } from '../../common/lib/kibana';
import { useKibana } from './kibana_react';

interface UseApplicationReturn {
appId: string | undefined;
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/cases/public/common/mock/test_providers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import type { ScopedFilesClient } from '@kbn/files-plugin/public';
import { euiDarkVars } from '@kbn/ui-theme';
import { I18nProvider } from '@kbn/i18n-react';
import { createMockFilesClient } from '@kbn/shared-ux-file-mocks';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { QueryClient } from '@tanstack/react-query';

import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { FilesContext } from '@kbn/shared-ux-file-context';
Expand Down Expand Up @@ -108,10 +108,9 @@ const TestProvidersComponent: React.FC<TestProviderProps> = ({
permissions,
getFilesClient,
}}
queryClient={queryClient}
>
<QueryClientProvider client={queryClient}>
<FilesContext client={createMockFilesClient()}>{children}</FilesContext>
</QueryClientProvider>
<FilesContext client={createMockFilesClient()}>{children}</FilesContext>
</CasesProvider>
</MemoryRouter>
</ThemeProvider>
Expand Down Expand Up @@ -191,8 +190,9 @@ export const createAppMockRenderer = ({
releasePhase,
getFilesClient,
}}
queryClient={queryClient}
>
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
{children}
</CasesProvider>
</MemoryRouter>
</ThemeProvider>
Expand Down
Loading

0 comments on commit 48d8230

Please sign in to comment.