From 04206ac8b49d7c2c319293be282455bd300b18f8 Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Tue, 22 Nov 2022 03:14:57 -0500 Subject: [PATCH 1/3] fix(custom-recordings): add error-view & refactor target selection listener --- .../CreateRecording/CustomRecordingForm.tsx | 71 +++++++++++++++++-- src/app/ErrorView/ErrorView.tsx | 3 + .../CustomRecordingForm.test.tsx | 36 +++++++++- 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/src/app/CreateRecording/CustomRecordingForm.tsx b/src/app/CreateRecording/CustomRecordingForm.tsx index 1624ea6c4..a9f2e2f5a 100644 --- a/src/app/CreateRecording/CustomRecordingForm.tsx +++ b/src/app/CreateRecording/CustomRecordingForm.tsx @@ -60,7 +60,7 @@ import { } from '@patternfly/react-core'; import { HelpIcon } from '@patternfly/react-icons'; import { useHistory } from 'react-router-dom'; -import { concatMap, first } from 'rxjs/operators'; +import { concatMap, first, filter } from 'rxjs/operators'; import { EventTemplate } from './CreateRecording'; import { RecordingOptions, RecordingAttributes } from '@app/Shared/Services/Api.service'; import { DurationPicker } from '@app/DurationPicker/DurationPicker'; @@ -70,6 +70,8 @@ import { RecordingLabelFields } from '@app/RecordingMetadata/RecordingLabelField import { useSubscriptions } from '@app/utils/useSubscriptions'; import { LoadingPropsType } from '@app/Shared/ProgressIndicator'; import { TemplateType } from '@app/Shared/Services/Api.service'; +import { authFailMessage, ErrorView, isAuthFail } from '@app/ErrorView/ErrorView'; +import { NO_TARGET } from '@app/Shared/Services/Target.service'; export interface CustomRecordingFormProps { prefilled?: { @@ -105,6 +107,7 @@ export const CustomRecordingForm: React.FunctionComponent { @@ -277,31 +280,72 @@ export const CustomRecordingForm: React.FunctionComponent { + const refreshTemplates = React.useCallback(() => { addSubscription( context.target .target() .pipe( + filter((target) => target !== NO_TARGET), + first(), concatMap((target) => context.api.doGet(`targets/${encodeURIComponent(target.connectUrl)}/templates`) ) ) - .subscribe(setTemplates) + .subscribe({ + next: (templates) => { + setTemplates(templates); + setErrorMessage(''); + }, + error: (error) => { + setErrorMessage(error.message); + setTemplates([]); + }, + }) ); - }, [addSubscription, context.target, context.api, setTemplates]); + }, [addSubscription, context.target, context.api, setTemplates, setErrorMessage]); - React.useEffect(() => { + const refreshRecordingOptions = React.useCallback(() => { addSubscription( context.target .target() .pipe( + filter((target) => target !== NO_TARGET), + first(), concatMap((target) => context.api.doGet(`targets/${encodeURIComponent(target.connectUrl)}/recordingOptions`) ) ) - .subscribe(setRecordingOptions) + .subscribe({ + next: (options) => { + setRecordingOptions(options); + setErrorMessage(''); + }, + error: (error) => { + setErrorMessage(error.message); + setRecordingOptions({}); + }, + }) ); - }, [addSubscription, context.api, context.target, setRecordingOptions]); + }, [addSubscription, context.target, context.api, setRecordingOptions, setErrorMessage]); + + React.useEffect(() => { + addSubscription( + context.target.authFailure().subscribe(() => { + setErrorMessage(authFailMessage); + setTemplates([]); + setRecordingOptions({}); + }) + ); + }, [context.target, setErrorMessage, addSubscription, setTemplates, setRecordingOptions]); + + React.useEffect(() => { + addSubscription( + context.target.target().subscribe(() => { + refreshTemplates(); + refreshRecordingOptions(); + }) + ); + }, [addSubscription, context.target, refreshTemplates, refreshRecordingOptions]); const isFormInvalid: boolean = React.useMemo(() => { return ( @@ -335,6 +379,19 @@ export const CustomRecordingForm: React.FunctionComponent { + context.target.setAuthRetry(); + }, [context.target]); + + if (errorMessage != '') { + return ( + + ); + } return ( <> diff --git a/src/app/ErrorView/ErrorView.tsx b/src/app/ErrorView/ErrorView.tsx index bf72f6cd8..9ba956096 100644 --- a/src/app/ErrorView/ErrorView.tsx +++ b/src/app/ErrorView/ErrorView.tsx @@ -49,6 +49,9 @@ import { import { ExclamationCircleIcon } from '@patternfly/react-icons'; export const authFailMessage = 'Auth failure'; + +export const missingSSLMessage = 'Missing SSL Certificates'; + export const isAuthFail = (message: string) => message === authFailMessage; export interface ErrorViewProps { title: string | React.ReactNode; diff --git a/src/test/CreateRecording/CustomRecordingForm.test.tsx b/src/test/CreateRecording/CustomRecordingForm.test.tsx index c014b8610..8ae9a5143 100644 --- a/src/test/CreateRecording/CustomRecordingForm.test.tsx +++ b/src/test/CreateRecording/CustomRecordingForm.test.tsx @@ -38,12 +38,12 @@ import * as React from 'react'; import { createMemoryHistory } from 'history'; -import { screen, cleanup, waitFor } from '@testing-library/react'; +import { screen, cleanup, act as doAct } from '@testing-library/react'; import renderer, { act } from 'react-test-renderer'; -import { ServiceContext } from '@app/Shared/Services/Services'; +import { ServiceContext, Services } from '@app/Shared/Services/Services'; import { defaultServices } from '@app/Shared/Services/Services'; import { EventTemplate, RecordingAttributes, RecordingOptions } from '@app/Shared/Services/Api.service'; -import { of } from 'rxjs'; +import { of, Subject } from 'rxjs'; import { renderWithServiceContext } from '../Common'; jest.mock('@patternfly/react-core', () => ({ @@ -54,6 +54,7 @@ jest.mock('@patternfly/react-core', () => ({ import { CustomRecordingForm } from '@app/CreateRecording/CustomRecordingForm'; import { NotificationsContext, NotificationsInstance } from '@app/Notifications/Notifications'; +import { TargetService } from '@app/Shared/Services/Target.service'; const mockConnectUrl = 'service:jmx:rmi://someUrl'; const mockTarget = { connectUrl: mockConnectUrl, alias: 'fooTarget' }; @@ -83,6 +84,8 @@ jest .mockReturnValueOnce(of([mockCustomEventTemplate])) // should show correct helper texts in metadata label editor .mockReturnValueOnce(of(mockRecordingOptions)); +jest.spyOn(defaultServices.target, 'authFailure').mockReturnValue(of()); + const history = createMemoryHistory({ initialEntries: ['/recordings/create'] }); jest.mock('react-router-dom', () => ({ @@ -162,4 +165,31 @@ describe('', () => { expect(helperText).toBeInTheDocument(); expect(helperText).toBeVisible(); }); + + it('should show error view if failing to retrieve templates or recording options', async () => { + const subj = new Subject(); + const mockTargetSvc = { + target: () => of(mockTarget), + authFailure: () => subj.asObservable(), + } as TargetService; + const services: Services = { + ...defaultServices, + target: mockTargetSvc, + }; + renderWithServiceContext(, { services: services }); + + await doAct(async () => subj.next()); + + const failTitle = screen.getByText('Error displaying recording creation form'); + expect(failTitle).toBeInTheDocument(); + expect(failTitle).toBeVisible(); + + const authFailText = screen.getByText('Auth failure'); + expect(authFailText).toBeInTheDocument(); + expect(authFailText).toBeVisible(); + + const retryButton = screen.getByText('Retry'); + expect(retryButton).toBeInTheDocument(); + expect(retryButton).toBeVisible(); + }); }); From 7dc40c28418afc93b3dcc1e16172759b5d178322 Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Tue, 22 Nov 2022 12:43:01 -0500 Subject: [PATCH 2/3] fix(snapshot-recording): add error view --- .../CreateRecording/SnapshotRecordingForm.tsx | 49 +++++++ src/app/ErrorView/ErrorView.tsx | 2 +- .../SnapshotRecordingForm.test.tsx | 128 ++++++++++++++++++ .../SnapshotRecordingForm.test.tsx.snap | 62 +++++++++ 4 files changed, 240 insertions(+), 1 deletion(-) create mode 100644 src/test/CreateRecording/SnapshotRecordingForm.test.tsx create mode 100644 src/test/CreateRecording/__snapshots__/SnapshotRecordingForm.test.tsx.snap diff --git a/src/app/CreateRecording/SnapshotRecordingForm.tsx b/src/app/CreateRecording/SnapshotRecordingForm.tsx index ccc2d39c9..e9bcd82a6 100644 --- a/src/app/CreateRecording/SnapshotRecordingForm.tsx +++ b/src/app/CreateRecording/SnapshotRecordingForm.tsx @@ -42,6 +42,7 @@ import { useSubscriptions } from '@app/utils/useSubscriptions'; import { ServiceContext } from '@app/Shared/Services/Services'; import { first } from 'rxjs'; import { LoadingPropsType } from '@app/Shared/ProgressIndicator'; +import { authFailMessage, ErrorView, isAuthFail, missingSSLMessage } from '@app/ErrorView/ErrorView'; export interface SnapshotRecordingFormProps {} @@ -50,6 +51,7 @@ export const SnapshotRecordingForm: React.FunctionComponent { setLoading(true); @@ -76,6 +78,53 @@ export const SnapshotRecordingForm: React.FunctionComponent { + addSubscription( + context.target.sslFailure().subscribe(() => { + // also triggered if api calls in Custom Recording form fail + setErrorMessage(missingSSLMessage); + }) + ); + }, [context.target, setErrorMessage, addSubscription]); + + React.useEffect(() => { + addSubscription( + context.target.authRetry().subscribe(() => { + setErrorMessage(''); // Reset on retry + }) + ); + }, [context.target, setErrorMessage, addSubscription]); + + React.useEffect(() => { + addSubscription( + context.target.authFailure().subscribe(() => { + // also triggered if api calls in Custom Recording form fail + setErrorMessage(authFailMessage); + }) + ); + }, [context.target, setErrorMessage, addSubscription]); + + React.useEffect(() => { + addSubscription( + context.target.target().subscribe(() => { + setErrorMessage(''); // Reset on change + }) + ); + }, [context.target, setErrorMessage, addSubscription]); + + const authRetry = React.useCallback(() => { + context.target.setAuthRetry(); + }, [context.target]); + + if (errorMessage != '') { + return ( + + ); + } return ( <>
diff --git a/src/app/ErrorView/ErrorView.tsx b/src/app/ErrorView/ErrorView.tsx index 9ba956096..9195f85ec 100644 --- a/src/app/ErrorView/ErrorView.tsx +++ b/src/app/ErrorView/ErrorView.tsx @@ -50,7 +50,7 @@ import { ExclamationCircleIcon } from '@patternfly/react-icons'; export const authFailMessage = 'Auth failure'; -export const missingSSLMessage = 'Missing SSL Certificates'; +export const missingSSLMessage = 'Bad Gateway'; export const isAuthFail = (message: string) => message === authFailMessage; export interface ErrorViewProps { diff --git a/src/test/CreateRecording/SnapshotRecordingForm.test.tsx b/src/test/CreateRecording/SnapshotRecordingForm.test.tsx new file mode 100644 index 000000000..3b4f35a78 --- /dev/null +++ b/src/test/CreateRecording/SnapshotRecordingForm.test.tsx @@ -0,0 +1,128 @@ +/* + * Copyright The Cryostat Authors + * + * The Universal Permissive License (UPL), Version 1.0 + * + * Subject to the condition set forth below, permission is hereby granted to any + * person obtaining a copy of this software, associated documentation and/or data + * (collectively the "Software"), free of charge and under any and all copyright + * rights in the Software, and any and all patent rights owned or freely + * licensable by each licensor hereunder covering either (i) the unmodified + * Software as contributed to or provided by such licensor, or (ii) the Larger + * Works (as defined below), to deal in both + * + * (a) the Software, and + * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if + * one is included with the Software (each a "Larger Work" to which the Software + * is contributed by such licensors), + * + * without restriction, including without limitation the rights to copy, create + * derivative works of, display, perform, and distribute the Software and make, + * use, sell, offer for sale, import, export, have made, and have sold the + * Software and the Larger Work(s), and to sublicense the foregoing rights on + * either these or other terms. + * + * This license is subject to the following condition: + * The above copyright notice and either this complete permission notice or at + * a minimum a reference to the UPL must be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +import * as React from 'react'; +import { createMemoryHistory } from 'history'; +import { screen, cleanup, act as doAct } from '@testing-library/react'; +import renderer, { act } from 'react-test-renderer'; +import { ServiceContext, Services } from '@app/Shared/Services/Services'; +import { defaultServices } from '@app/Shared/Services/Services'; +import { of, Subject } from 'rxjs'; +import { renderWithServiceContext } from '../Common'; +import { NotificationsContext, NotificationsInstance } from '@app/Notifications/Notifications'; +import { TargetService } from '@app/Shared/Services/Target.service'; +import { SnapshotRecordingForm } from '@app/CreateRecording/SnapshotRecordingForm'; + +const mockConnectUrl = 'service:jmx:rmi://someUrl'; +const mockTarget = { connectUrl: mockConnectUrl, alias: 'fooTarget' }; + +const history = createMemoryHistory({ initialEntries: ['/recordings/create'] }); + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useRouteMatch: () => ({ url: history.location.pathname }), + useHistory: () => history, +})); + +jest.spyOn(defaultServices.target, 'authFailure').mockReturnValue(of()); +jest.spyOn(defaultServices.target, 'target').mockReturnValue(of(mockTarget)); +jest.spyOn(defaultServices.target, 'sslFailure').mockReturnValue(of()); +jest.spyOn(defaultServices.target, 'authRetry').mockReturnValue(of()); + +describe('', () => { + beforeEach(() => { + history.go(-history.length); + }); + + afterEach(cleanup); + + it('renders correctly', async () => { + let tree; + await act(async () => { + tree = renderer.create( + + + + + + ); + }); + expect(tree.toJSON()).toMatchSnapshot(); + }); + + it('should create recording when create is clicked', async () => { + const onCreateSpy = jest.spyOn(defaultServices.api, 'createSnapshot').mockReturnValue(of(true)); + const { user } = renderWithServiceContext(); + + const createButton = screen.getByText('Create'); + expect(createButton).toBeInTheDocument(); + expect(createButton).toBeVisible(); + + await user.click(createButton); + + expect(onCreateSpy).toHaveBeenCalledTimes(1); + expect(history.entries.map((entry) => entry.pathname)).toStrictEqual(['/recordings/create', '/recordings']); + }); + + it('should show error view if failing to retrieve templates or recording options', async () => { + const authSubj = new Subject(); + const mockTargetSvc = { + ...defaultServices.target, + authFailure: () => authSubj.asObservable(), + } as TargetService; + const services: Services = { + ...defaultServices, + target: mockTargetSvc, + }; + renderWithServiceContext(, { services: services }); + + await doAct(async () => authSubj.next()); + + const failTitle = screen.getByText('Error displaying recording creation form'); + expect(failTitle).toBeInTheDocument(); + expect(failTitle).toBeVisible(); + + const authFailText = screen.getByText('Auth failure'); + expect(authFailText).toBeInTheDocument(); + expect(authFailText).toBeVisible(); + + const retryButton = screen.getByText('Retry'); + expect(retryButton).toBeInTheDocument(); + expect(retryButton).toBeVisible(); + }); +}); diff --git a/src/test/CreateRecording/__snapshots__/SnapshotRecordingForm.test.tsx.snap b/src/test/CreateRecording/__snapshots__/SnapshotRecordingForm.test.tsx.snap new file mode 100644 index 000000000..b28ef22be --- /dev/null +++ b/src/test/CreateRecording/__snapshots__/SnapshotRecordingForm.test.tsx.snap @@ -0,0 +1,62 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` renders correctly 1`] = ` + +

+ A Snapshot recording is one which contains all information about all events that have been captured in the current session by + + other,  non-Snapshot + + recordings. Snapshots do not themselves define which events are enabled, their thresholds, or any other options. A Snapshot is only ever in the STOPPED state from the moment it is created. +

+
+
+
+ + +
+
+
+ +`; From e706f70fb5329fd4a2a17cec68b3a414ba6c9b56 Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Mon, 28 Nov 2022 03:07:56 -0500 Subject: [PATCH 3/3] fix(create-recording): use forkJoin --- .../CreateRecording/CustomRecordingForm.tsx | 53 ++++++------------- 1 file changed, 17 insertions(+), 36 deletions(-) diff --git a/src/app/CreateRecording/CustomRecordingForm.tsx b/src/app/CreateRecording/CustomRecordingForm.tsx index a9f2e2f5a..24875e781 100644 --- a/src/app/CreateRecording/CustomRecordingForm.tsx +++ b/src/app/CreateRecording/CustomRecordingForm.tsx @@ -72,6 +72,7 @@ import { LoadingPropsType } from '@app/Shared/ProgressIndicator'; import { TemplateType } from '@app/Shared/Services/Api.service'; import { authFailMessage, ErrorView, isAuthFail } from '@app/ErrorView/ErrorView'; import { NO_TARGET } from '@app/Shared/Services/Target.service'; +import { forkJoin } from 'rxjs'; export interface CustomRecordingFormProps { prefilled?: { @@ -280,7 +281,7 @@ export const CustomRecordingForm: React.FunctionComponent { + const refeshFormOptions = React.useCallback(() => { addSubscription( context.target .target() @@ -288,45 +289,30 @@ export const CustomRecordingForm: React.FunctionComponent target !== NO_TARGET), first(), concatMap((target) => - context.api.doGet(`targets/${encodeURIComponent(target.connectUrl)}/templates`) + forkJoin({ + templates: context.api.doGet( + `targets/${encodeURIComponent(target.connectUrl)}/templates` + ), + recordingOptions: context.api.doGet( + `targets/${encodeURIComponent(target.connectUrl)}/recordingOptions` + ), + }) ) ) .subscribe({ - next: (templates) => { - setTemplates(templates); + next: (formOptions) => { setErrorMessage(''); + setTemplates(formOptions.templates); + setRecordingOptions(formOptions.recordingOptions); }, error: (error) => { - setErrorMessage(error.message); + setErrorMessage(error.message); // If both throw, first error will be shown setTemplates([]); - }, - }) - ); - }, [addSubscription, context.target, context.api, setTemplates, setErrorMessage]); - - const refreshRecordingOptions = React.useCallback(() => { - addSubscription( - context.target - .target() - .pipe( - filter((target) => target !== NO_TARGET), - first(), - concatMap((target) => - context.api.doGet(`targets/${encodeURIComponent(target.connectUrl)}/recordingOptions`) - ) - ) - .subscribe({ - next: (options) => { - setRecordingOptions(options); - setErrorMessage(''); - }, - error: (error) => { - setErrorMessage(error.message); setRecordingOptions({}); }, }) ); - }, [addSubscription, context.target, context.api, setRecordingOptions, setErrorMessage]); + }, [addSubscription, context.target, context.api, setTemplates, setRecordingOptions, setErrorMessage]); React.useEffect(() => { addSubscription( @@ -339,13 +325,8 @@ export const CustomRecordingForm: React.FunctionComponent { - addSubscription( - context.target.target().subscribe(() => { - refreshTemplates(); - refreshRecordingOptions(); - }) - ); - }, [addSubscription, context.target, refreshTemplates, refreshRecordingOptions]); + addSubscription(context.target.target().subscribe(refeshFormOptions)); + }, [addSubscription, context.target, refeshFormOptions]); const isFormInvalid: boolean = React.useMemo(() => { return (