diff --git a/plugins/cost-insights/src/alerts/KubernetesMigrationAlert.tsx b/plugins/cost-insights/src/alerts/KubernetesMigrationAlert.tsx index 7b775a4a96..8c59d0c1a5 100644 --- a/plugins/cost-insights/src/alerts/KubernetesMigrationAlert.tsx +++ b/plugins/cost-insights/src/alerts/KubernetesMigrationAlert.tsx @@ -52,9 +52,15 @@ export interface MigrationAlert extends Alert { * by status in a collapsed view below Alert Insights section and a badge will appear in Action Items * showing the total alerts of that status. * - * Default forms can be overridden by providing a valid React form component. Form components - * must return valid form elements, and accept a ref and onSubmit event handler. See /forms - * for example implementations. Custom forms must implement a corresponding event hook. + * Customizing Alerts + * Default forms can be overridden in two ways - by setting a form property to null or defining a custom component. + * + * If a form property is set to null, the Dialog will not render a form. This can be useful in scenarios + * where data isn't needed from the user such as when a user accepts an action item's recommendation. + * + * If a form property is set to a React component, the Dialog will render the form component in place of the default form. + * Form components must return valid form elements, and accept a ref and onSubmit event handler. + * Custom forms must implement the corresponding event hook. See /forms for example implementations. */ export class KubernetesMigrationAlert implements MigrationAlert { @@ -64,7 +70,10 @@ export class KubernetesMigrationAlert implements MigrationAlert { subtitle = 'Services running on Kubernetes are estimated to save 50% or more compared to Compute Engine.'; - // Override default dismiss form with a custom form component. + // Dialog will not render a form if form property set to null. + AcceptForm = null; + + // Overrides default Dismiss form with a custom form component. DismissForm: AlertForm< MigrationAlert, MigrationDismissFormData @@ -100,7 +109,7 @@ export class KubernetesMigrationAlert implements MigrationAlert { ); } - /* Fires when the onSubmit event is raised on a DismissAlert form. Displays a custom dismiss form. */ + /* Fires when the onSubmit event is raised on a Dismiss form. Displays custom dismiss form. */ async onDismissed( options: AlertOptions, ): Promise { @@ -117,7 +126,7 @@ export class KubernetesMigrationAlert implements MigrationAlert { ); } - /* Fires when the onSubmit event is raised on an SnoozeAlert form. Displays default snooze form. */ + /* Fires when the onSubmit event is raised on a Snooze form. Displays default snooze form. */ async onSnoozed( options: AlertOptions, ): Promise { @@ -134,7 +143,7 @@ export class KubernetesMigrationAlert implements MigrationAlert { ); } - /* Fires when the onSubmit event is raised on an AcceptAlert form. Displays default accept form. */ + /* Fires when the Accept button is clicked. Dialog does not render a form. See KubernetesMigrationAlert.AcceptForm */ async onAccepted(options: AlertOptions): Promise { const alerts = await this.api.getAlerts(options.group); return new Promise(resolve => diff --git a/plugins/cost-insights/src/components/AlertInsights/AlertDialog.test.tsx b/plugins/cost-insights/src/components/AlertInsights/AlertDialog.test.tsx index 2e4cb53681..72ff757cee 100644 --- a/plugins/cost-insights/src/components/AlertInsights/AlertDialog.test.tsx +++ b/plugins/cost-insights/src/components/AlertInsights/AlertDialog.test.tsx @@ -16,154 +16,169 @@ import React from 'react'; import { AlertDialog } from './AlertDialog'; import { render } from '@testing-library/react'; -import { - Alert, - AlertFormProps, - AlertSnoozeOptions, - AlertDismissOptions, -} from '../../types'; +import { Alert, AlertFormProps } from '../../types'; type MockFormDataProps = AlertFormProps; -const MockForm = React.forwardRef( - (props, ref) => ( +function createForm(title: string) { + return React.forwardRef((props, ref) => (
- You. Complete. Me. + You. {title}. Me.
- ), -); + )); +} +const snoozableAlert: Alert = { + title: 'title', + subtitle: 'test-subtitle', + onSnoozed: jest.fn(), +}; + +const dimissableAlert: Alert = { + title: 'title', + subtitle: 'subtitle', + onDismissed: jest.fn(), +}; + +const acceptAlert: Alert = { + title: 'title', + subtitle: 'subtitle', + onAccepted: jest.fn(), +}; + +const customSnoozeAlert: Alert = { + title: 'title', + subtitle: 'subtitle', + onSnoozed: jest.fn(), + SnoozeForm: createForm('Snooze'), +}; + +const customDismissAlert: Alert = { + title: 'title', + subtitle: 'subtitle', + onDismissed: jest.fn(), + DismissForm: createForm('Dismiss'), +}; + +const customAcceptAlert: Alert = { + title: 'title', + subtitle: 'test-subtitle', + onAccepted: jest.fn(), + AcceptForm: createForm('Accept'), +}; + +const nullAcceptAlert: Alert = { + title: 'title', + subtitle: 'test-subtitle', + onAccepted: jest.fn(), + AcceptForm: null, +}; + +const nullDismissAlert: Alert = { + title: 'title', + subtitle: 'test-subtitle', + onDismissed: jest.fn(), + DismissForm: null, +}; + +const nullSnoozeAlert: Alert = { + title: 'title', + subtitle: 'test-subtitle', + onSnoozed: jest.fn(), + SnoozeForm: null, +}; describe('', () => { - const snoozableAlert: Alert = { - title: 'title', - subtitle: 'test-subtitle', - onSnoozed: jest.fn(), - }; + describe.each` + accepted | dismissed | snoozed | action | text + ${acceptAlert} | ${null} | ${null} | ${['Accept', 'accepted']} | ${'My team can commit to making this change soon, or has already.'} + ${null} | ${dimissableAlert} | ${null} | ${['Dismiss', 'dismissed']} | ${'Reason for dismissing?'} + ${null} | ${null} | ${snoozableAlert} | ${['Snooze', 'snoozed']} | ${'For how long?'} + `( + 'Default forms', + ({ accepted, dismissed, snoozed, action: [action, actioned], text }) => { + it(`Displays a default ${action} form`, () => { + const { getByText } = render( + , + ); + expect(getByText(text)).toBeInTheDocument(); + expect(getByText(`${action} this action item?`)).toBeInTheDocument(); + expect( + getByText(`This action item will be ${actioned} for all of Ramones.`), + ).toBeInTheDocument(); + }); + }, + ); - const dimissableAlert: Alert = { - title: 'title', - subtitle: 'subtitle', - onDismissed: jest.fn(), - }; + describe.each` + accepted | dismissed | snoozed | action + ${customAcceptAlert} | ${null} | ${null} | ${['Accept', 'accepted']} + ${null} | ${customDismissAlert} | ${null} | ${['Dismiss', 'dismissed']} + ${null} | ${null} | ${customSnoozeAlert} | ${['Snooze', 'snoozed']} + `( + 'Custom forms', + ({ accepted, dismissed, snoozed, action: [Action, actioned] }) => { + it(`Displays a custom ${Action} form`, () => { + const { getByText } = render( + , + ); + expect(getByText(`You. ${Action}. Me.`)).toBeInTheDocument(); + expect(getByText(`${Action} this action item?`)).toBeInTheDocument(); + expect( + getByText(`This action item will be ${actioned} for all of Ramones.`), + ).toBeInTheDocument(); + }); + }, + ); - const customSnoozeAlert: Alert = { - title: 'title', - subtitle: 'subtitle', - onSnoozed: jest.fn(), - SnoozeForm: MockForm, - }; - - const customDismissAlert: Alert = { - title: 'title', - subtitle: 'subtitle', - onDismissed: jest.fn(), - DismissForm: MockForm, - }; - - const customAcceptAlert: Alert = { - title: 'title', - subtitle: 'test-subtitle', - onAccepted: jest.fn(), - AcceptForm: MockForm, - }; - - it('Displays a default snooze form', () => { - const { getByText } = render( - , - ); - expect(getByText('For how long?')).toBeInTheDocument(); - expect(getByText('Snooze this action item?')).toBeInTheDocument(); - expect( - getByText('This action item will be snoozed for all of Ramones.'), - ).toBeInTheDocument(); - AlertSnoozeOptions.forEach(a => - expect(getByText(a.label)).toBeInTheDocument(), - ); - }); - - it('Displays a custom snooze form', () => { - const { getByText } = render( - , - ); - expect(getByText('You. Complete. Me.')).toBeInTheDocument(); - expect(getByText('Snooze this action item?')).toBeInTheDocument(); - expect( - getByText('This action item will be snoozed for all of Ramones.'), - ).toBeInTheDocument(); - }); - - it('Displays a default dismiss form', () => { - const { getByText } = render( - , - ); - expect(getByText('Dismiss this action item?')).toBeInTheDocument(); - expect( - getByText('This action item will be dismissed for all of Ramones.'), - ).toBeInTheDocument(); - AlertDismissOptions.forEach(a => - expect(getByText(a.label)).toBeInTheDocument(), - ); - }); - - it('Displays a custom dismiss form', () => { - const { getByText } = render( - , - ); - expect(getByText('Dismiss this action item?')).toBeInTheDocument(); - expect(getByText('You. Complete. Me.')).toBeInTheDocument(); - expect( - getByText('This action item will be dismissed for all of Ramones.'), - ).toBeInTheDocument(); - }); - - it('Displays a custom accept form', () => { - const { getByText } = render( - , - ); - expect(getByText('Accept this action item?')).toBeInTheDocument(); - expect(getByText('You. Complete. Me.')).toBeInTheDocument(); - expect( - getByText('This action item will be accepted for all of Ramones.'), - ).toBeInTheDocument(); - }); + describe.each` + accepted | dismissed | snoozed | action | text + ${nullAcceptAlert} | ${null} | ${null} | ${['Accept', 'accept', 'accepted']} | ${'My team can commit to making this change soon, or has already.'} + ${null} | ${nullDismissAlert} | ${null} | ${['Dismiss', 'dismiss', 'dismissed']} | ${'Reason for dismissing?'} + ${null} | ${null} | ${nullSnoozeAlert} | ${['Snooze', 'snooze', 'snoozed']} | ${'For how long?'} + `( + 'Null forms', + ({ + accepted, + dismissed, + snoozed, + action: [Action, action, actioned], + text, + }) => { + it(`Does NOT display a ${Action} form`, () => { + const { getByText, getByRole, queryByText } = render( + , + ); + expect(queryByText(text)).not.toBeInTheDocument(); + expect(getByRole('button', { name: action })).toBeInTheDocument(); + expect(getByText(`${Action} this action item?`)).toBeInTheDocument(); + expect( + getByText(`This action item will be ${actioned} for all of Ramones.`), + ).toBeInTheDocument(); + }); + }, + ); }); diff --git a/plugins/cost-insights/src/components/AlertInsights/AlertDialog.tsx b/plugins/cost-insights/src/components/AlertInsights/AlertDialog.tsx index aa5b8c59fb..f7154602b9 100644 --- a/plugins/cost-insights/src/components/AlertInsights/AlertDialog.tsx +++ b/plugins/cost-insights/src/components/AlertInsights/AlertDialog.tsx @@ -33,7 +33,7 @@ import { } from '../../forms'; import { useAlertDialogStyles as useStyles } from '../../utils/styles'; import { choose } from '../../utils/alerts'; -import { Alert, Maybe } from '../../types'; +import { Alert, AlertForm, Maybe } from '../../types'; const DEFAULT_FORM_ID = 'alert-form'; @@ -79,20 +79,41 @@ export const AlertDialog = ({ setDisabled(true); } - const SnoozeForm = snoozed?.SnoozeForm ?? AlertSnoozeForm; - const AcceptForm = accepted?.AcceptForm ?? AlertAcceptForm; - const DismissForm = dismissed?.DismissForm ?? AlertDismissForm; + const SnoozeForm: Maybe = snoozed?.SnoozeForm ?? AlertSnoozeForm; + const AcceptForm: Maybe = accepted?.AcceptForm ?? AlertAcceptForm; + const DismissForm: Maybe = + dismissed?.DismissForm ?? AlertDismissForm; - const isSnoozeFormDisplayed = !!snoozed?.onSnoozed; - const isAcceptFormDisplayed = !!accepted?.onAccepted; - const isDismissFormDisplayed = !!dismissed?.onDismissed; + const isSnoozingEnabled = !!snoozed?.onSnoozed; + const isAcceptingEnabled = !!accepted?.onAccepted; + const isDismissingEnabled = !!dismissed?.onDismissed; + + const isSnoozeFormDisabled = snoozed?.SnoozeForm === null; + const isAcceptFormDisabled = accepted?.AcceptForm === null; + const isDismissFormDisabled = dismissed?.DismissForm === null; + const isFormDisabled = + isSnoozeFormDisabled || isAcceptFormDisabled || isDismissFormDisabled; const status = [ - isAcceptFormDisplayed, - isSnoozeFormDisplayed, - isDismissFormDisplayed, + isSnoozingEnabled, + isAcceptingEnabled, + isDismissingEnabled, ] as const; + const [Action, action, actioned] = + choose(status, [ + ['Snooze', 'snooze', 'snoozed'], + ['Accept', 'accept', 'accepted'], + ['Dismiss', 'dismiss', 'dismissed'], + ]) ?? []; + + const [title, subtitle] = + choose(status, [ + [snoozed?.title, snoozed?.subtitle], + [accepted?.title, accepted?.subtitle], + [dismissed?.title, dismissed?.subtitle], + ]) ?? []; + const TransitionProps = { mountOnEnter: true, unmountOnExit: true, @@ -122,24 +143,20 @@ export const AlertDialog = ({ - + - - {choose(status, ['Accept', 'Snooze', 'Dismiss'])} this action - item? - + {Action} this action item? - This action item will be{' '} - {choose(status, ['accepted', 'snoozed', 'dismissed'])} for all of{' '} - {group}. + This action item will be {actioned} for all of {group}. @@ -152,23 +169,11 @@ export const AlertDialog = ({ borderRadius={4} > - - {choose(status, [ - accepted?.title, - snoozed?.title, - dismissed?.title, - ])} - - - - {choose(status, [ - accepted?.subtitle, - snoozed?.subtitle, - dismissed?.subtitle, - ])} + {title} + {subtitle} - {isSnoozeFormDisplayed && ( + {isSnoozingEnabled && !isSnoozeFormDisabled && ( )} - {isDismissFormDisplayed && ( + {isDismissingEnabled && !isDismissFormDisabled && ( )} - {isAcceptFormDisplayed && ( + {isAcceptingEnabled && !isAcceptFormDisabled && ( - + {isFormDisabled ? ( + + ) : ( + + )} ); diff --git a/plugins/cost-insights/src/types/Alert.ts b/plugins/cost-insights/src/types/Alert.ts index 21e1a779a8..eb8919b71a 100644 --- a/plugins/cost-insights/src/types/Alert.ts +++ b/plugins/cost-insights/src/types/Alert.ts @@ -40,9 +40,9 @@ export type Alert = { status?: AlertStatus; url?: string; buttonText?: string; // Default: View Instructions - SnoozeForm?: AlertForm; - AcceptForm?: AlertForm; - DismissForm?: AlertForm; + SnoozeForm?: Maybe; + AcceptForm?: Maybe; + DismissForm?: Maybe; onSnoozed?(options: AlertOptions): Promise; onAccepted?(options: AlertOptions): Promise; onDismissed?(options: AlertOptions): Promise; diff --git a/plugins/cost-insights/src/utils/alerts.tsx b/plugins/cost-insights/src/utils/alerts.tsx index b8dfddf79b..e10740fbfe 100644 --- a/plugins/cost-insights/src/utils/alerts.tsx +++ b/plugins/cost-insights/src/utils/alerts.tsx @@ -29,7 +29,7 @@ export const sumOfAllAlerts = (sum: number, alerts: Alert[]) => export function choose( status: readonly [boolean, boolean, boolean], values: [T, T, T], -): T | null { +): T | undefined { const i = status.indexOf(true); - return i < 0 ? null : values[i]; + return values[i]; }