From 3380239451252158ed6b2f1b243446fa133d6424 Mon Sep 17 00:00:00 2001 From: Ross Mabbett <92495987+rtexelm@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:28:10 -0500 Subject: [PATCH] feat(Alerts and Reports): Modal redesign (#26202) Signed-off-by: dependabot[bot] Co-authored-by: fisjac Co-authored-by: Corbin Co-authored-by: Lily Kuang Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- superset-frontend/src/GlobalStyles.tsx | 1 + .../src/components/Button/index.tsx | 11 +- .../src/components/CronPicker/CronPicker.tsx | 115 +- .../src/components/Modal/Modal.tsx | 3 + .../features/alerts/AlertReportModal.test.jsx | 367 ----- .../features/alerts/AlertReportModal.test.tsx | 632 +++++++- .../src/features/alerts/AlertReportModal.tsx | 1296 +++++++++-------- .../alerts/buildErrorTooltipMessage.test.tsx | 70 + .../alerts/buildErrorTooltipMessage.tsx | 49 + .../AlertReportCronScheduler.test.tsx | 153 -- .../components/AlertReportCronScheduler.tsx | 96 +- .../alerts/components/NotificationMethod.tsx | 31 +- .../alerts/components/NumberInput.tsx | 52 + .../alerts/components/StyledPanel.tsx | 75 + .../components/ValidatedPanelHeader.tsx | 53 + .../src/features/alerts/types.ts | 18 + .../features/reports/ReportModal/index.tsx | 9 +- superset/config.py | 2 +- 18 files changed, 1799 insertions(+), 1234 deletions(-) delete mode 100644 superset-frontend/src/features/alerts/AlertReportModal.test.jsx create mode 100644 superset-frontend/src/features/alerts/buildErrorTooltipMessage.test.tsx create mode 100644 superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx delete mode 100644 superset-frontend/src/features/alerts/components/AlertReportCronScheduler.test.tsx create mode 100644 superset-frontend/src/features/alerts/components/NumberInput.tsx create mode 100644 superset-frontend/src/features/alerts/components/StyledPanel.tsx create mode 100644 superset-frontend/src/features/alerts/components/ValidatedPanelHeader.tsx diff --git a/superset-frontend/src/GlobalStyles.tsx b/superset-frontend/src/GlobalStyles.tsx index f983c262d7c20..eceefaff62f2a 100644 --- a/superset-frontend/src/GlobalStyles.tsx +++ b/superset-frontend/src/GlobalStyles.tsx @@ -20,6 +20,7 @@ import React from 'react'; import { css } from '@superset-ui/core'; import { Global } from '@emotion/react'; import { mix } from 'polished'; +import 'react-js-cron/dist/styles.css'; export const GlobalStyles = () => ( & Pick & { - tooltip?: string; + tooltip?: ReactNode; className?: string; buttonSize?: ButtonSize; buttonStyle?: ButtonStyle; @@ -214,11 +213,7 @@ export default function Button(props: ButtonProps) { if (tooltip) { return ( - + {/* wrap the button in a span so that the tooltip shows up when the button is disabled. */} {disabled ? ( diff --git a/superset-frontend/src/components/CronPicker/CronPicker.tsx b/superset-frontend/src/components/CronPicker/CronPicker.tsx index 385062e0cec85..4fe6cbb1fd37a 100644 --- a/superset-frontend/src/components/CronPicker/CronPicker.tsx +++ b/superset-frontend/src/components/CronPicker/CronPicker.tsx @@ -44,7 +44,7 @@ export const LOCALE: Locale = { prefixMonths: t('in'), prefixMonthDays: t('on'), prefixWeekDays: t('on'), - prefixWeekDaysForMonthAndYearPeriod: t('and'), + prefixWeekDaysForMonthAndYearPeriod: t('or'), prefixHours: t('at'), prefixMinutes: t(':'), prefixMinutesForHourPeriod: t('at'), @@ -110,22 +110,99 @@ export const CronPicker = styled((props: CronProps) => ( ))` - .react-js-cron-field { - margin-bottom: 0px; - } - .react-js-cron-select:not(.react-js-cron-custom-select) > div:first-of-type, - .react-js-cron-custom-select { - border-radius: ${({ theme }) => theme.gridUnit}px; - background-color: ${({ theme }) => - theme.colors.grayscale.light4} !important; - } - .react-js-cron-custom-select > div:first-of-type { - border-radius: ${({ theme }) => theme.gridUnit}px; - } - .react-js-cron-custom-select .ant-select-selection-placeholder { - flex: auto; - } - .react-js-cron-custom-select .ant-select-selection-overflow-item { - align-self: center; - } + ${({ theme }) => ` + + /* Boilerplate styling for ReactCronPicker imported explicitly in GlobalStyles.tsx */ + + /* When year period is selected */ + + :has(.react-js-cron-months) { + display: grid !important; + grid-template-columns: repeat(2, 50%); + column-gap: ${theme.gridUnit}px; + row-gap: ${theme.gridUnit * 2}px; + div:has(.react-js-cron-hours) { + grid-column: span 2; + display: flex; + justify-content: space-between; + .react-js-cron-field { + width: 50%; + } + } + } + + /* When month period is selected */ + + :not(:has(.react-js-cron-months)) { + display: grid; + grid-template-columns: repeat(2, 50%); + column-gap: ${theme.gridUnit}px; + row-gap: ${theme.gridUnit * 2}px; + .react-js-cron-period { + grid-column: span 2; + } + div:has(.react-js-cron-hours) { + grid-column: span 2; + display: flex; + justify-content: space-between; + .react-js-cron-field { + width: 50%; + } + } + } + + /* When week period is selected */ + + :not(:has(.react-js-cron-month-days)) { + .react-js-cron-week-days { + grid-column: span 2; + } + } + + /* For proper alignment of inputs and span elements */ + + :not(div:has(.react-js-cron-hours)) { + display: flex; + flex-wrap: nowrap; + } + + div:has(.react-js-cron-hours) { + width: 100%; + } + + .react-js-cron-minutes > span { + padding-left: ${theme.gridUnit}px; + } + + /* Sizing of select container */ + + .react-js-cron-select.ant-select { + width: 100%; + .ant-select-selector { + flex-wrap: nowrap; + } + } + + .react-js-cron-field { + width: 100%; + margin-bottom: 0px; + > span { + margin-left: 0px; + } + } + + .react-js-cron-custom-select .ant-select-selection-placeholder { + flex: auto; + border-radius: ${theme.gridUnit}px; + } + + .react-js-cron-custom-select .ant-select-selection-overflow-item { + align-self: center; + } + + .react-js-cron-select > div:first-of-type, + .react-js-cron-custom-select { + border-radius: ${theme.gridUnit}px; + } + `} `; diff --git a/superset-frontend/src/components/Modal/Modal.tsx b/superset-frontend/src/components/Modal/Modal.tsx index 331f34c1d23fa..831e8ab77ab22 100644 --- a/superset-frontend/src/components/Modal/Modal.tsx +++ b/superset-frontend/src/components/Modal/Modal.tsx @@ -41,6 +41,7 @@ export interface ModalProps { className?: string; children: ReactNode; disablePrimaryButton?: boolean; + primaryTooltipMessage?: ReactNode; primaryButtonLoading?: boolean; onHide: () => void; onHandledPrimaryAction?: () => void; @@ -232,6 +233,7 @@ const defaultResizableConfig = (hideFooter: boolean | undefined) => ({ const CustomModal = ({ children, disablePrimaryButton = false, + primaryTooltipMessage, primaryButtonLoading = false, onHide, onHandledPrimaryAction, @@ -274,6 +276,7 @@ const CustomModal = ({ key="submit" buttonStyle={primaryButtonType} disabled={disablePrimaryButton} + tooltip={primaryTooltipMessage} loading={primaryButtonLoading} onClick={onHandledPrimaryAction} cta diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.jsx b/superset-frontend/src/features/alerts/AlertReportModal.test.jsx deleted file mode 100644 index a91e789920546..0000000000000 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.jsx +++ /dev/null @@ -1,367 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -import React from 'react'; -import { Provider } from 'react-redux'; -import thunk from 'redux-thunk'; -import configureStore from 'redux-mock-store'; -import fetchMock from 'fetch-mock'; -import { act } from 'react-dom/test-utils'; -import Modal from 'src/components/Modal'; -import { Select, AsyncSelect } from 'src/components'; -import { Switch } from 'src/components/Switch'; -import { Radio } from 'src/components/Radio'; -import TextAreaControl from 'src/explore/components/controls/TextAreaControl'; -import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint'; -import { styledMount as mount } from 'spec/helpers/theming'; -import AlertReportModal from './AlertReportModal'; - -const mockData = { - active: true, - id: 1, - name: 'test report', - description: 'test report description', - chart: { id: 1, slice_name: 'test chart', viz_type: 'table' }, - database: { id: 1, database_name: 'test database' }, - sql: 'SELECT NaN', -}; -const FETCH_REPORT_ENDPOINT = 'glob:*/api/v1/report/*'; -const REPORT_PAYLOAD = { result: mockData }; - -fetchMock.get(FETCH_REPORT_ENDPOINT, REPORT_PAYLOAD); - -const mockStore = configureStore([thunk]); -const store = mockStore({}); - -// Report mock is default for testing -const mockedProps = { - addDangerToast: () => {}, - onAdd: jest.fn(() => []), - onHide: () => {}, - show: true, - isReport: true, -}; - -// Related mocks -const ownersEndpoint = 'glob:*/api/v1/alert/related/owners?*'; -const databaseEndpoint = 'glob:*/api/v1/alert/related/database?*'; -const dashboardEndpoint = 'glob:*/api/v1/alert/related/dashboard?*'; -const chartEndpoint = 'glob:*/api/v1/alert/related/chart?*'; - -fetchMock.get(ownersEndpoint, { - result: [], -}); - -fetchMock.get(databaseEndpoint, { - result: [], -}); - -fetchMock.get(dashboardEndpoint, { - result: [], -}); - -fetchMock.get(chartEndpoint, { - result: [{ text: 'table chart', value: 1 }], -}); - -async function mountAndWait(props = mockedProps) { - const mounted = mount( - - - , - { - context: { store }, - }, - ); - - await waitForComponentToPaint(mounted); - - return mounted; -} - -describe('AlertReportModal', () => { - let wrapper; - - beforeAll(async () => { - wrapper = await mountAndWait(); - }); - - it('renders', () => { - expect(wrapper.find(AlertReportModal)).toExist(); - }); - - it('renders a Modal', () => { - expect(wrapper.find(Modal)).toExist(); - }); - - it('render a empty modal', () => { - expect(wrapper.find('input[name="name"]').text()).toEqual(''); - expect(wrapper.find('input[name="description"]').text()).toEqual(''); - }); - - it('renders add header for report when no alert is included, and isReport is true', async () => { - const addWrapper = await mountAndWait(); - - expect( - addWrapper.find('[data-test="alert-report-modal-title"]').text(), - ).toEqual('Add Report'); - }); - - it('renders add header for alert when no alert is included, and isReport is false', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - expect( - addWrapper.find('[data-test="alert-report-modal-title"]').text(), - ).toEqual('Add Alert'); - }); - - it('renders edit modal', async () => { - const props = { - ...mockedProps, - alert: mockData, - }; - - const editWrapper = await mountAndWait(props); - expect( - editWrapper.find('[data-test="alert-report-modal-title"]').text(), - ).toEqual('Edit Report'); - expect(editWrapper.find('input[name="name"]').props().value).toEqual( - 'test report', - ); - expect(editWrapper.find('input[name="description"]').props().value).toEqual( - 'test report description', - ); - }); - - it('renders async select with value in alert edit modal', async () => { - const props = { - ...mockedProps, - alert: mockData, - isReport: false, - }; - - const editWrapper = await mountAndWait(props); - expect( - editWrapper.find('[aria-label="Database"]').at(0).props().value, - ).toEqual({ - value: 1, - label: 'test database', - }); - expect( - editWrapper.find('[aria-label="Chart"]').at(0).props().value, - ).toEqual({ - value: 1, - label: 'test chart', - }); - }); - - // Fields - it('renders input element for name', () => { - expect(wrapper.find('input[name="name"]')).toExist(); - }); - - it('renders four select elements when in report mode', () => { - expect(wrapper.find(Select)).toExist(); - expect(wrapper.find(AsyncSelect)).toExist(); - expect(wrapper.find(Select)).toHaveLength(2); - expect(wrapper.find(AsyncSelect)).toHaveLength(2); - }); - - it('renders Switch element', () => { - expect(wrapper.find(Switch)).toExist(); - }); - - it('renders input element for description', () => { - expect(wrapper.find('input[name="description"]')).toExist(); - }); - - it('renders input element for sql in alert mode only', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - expect(wrapper.find(TextAreaControl)).toHaveLength(0); - expect(addWrapper.find(TextAreaControl)).toExist(); - }); - - it('renders input element for sql with NaN', async () => { - const props = { - ...mockedProps, - alert: mockData, - isReport: false, - }; - - const editWrapper = await mountAndWait(props); - const input = editWrapper.find(TextAreaControl); - expect(input).toExist(); - expect(input.props().initialValue).toEqual('SELECT NaN'); - }); - - it('renders four select element when in report mode', () => { - expect(wrapper.find(Select)).toExist(); - expect(wrapper.find(AsyncSelect)).toExist(); - expect(wrapper.find(Select)).toHaveLength(2); - expect(wrapper.find(AsyncSelect)).toHaveLength(2); - }); - - it('renders six select elements when in alert mode', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - expect(addWrapper.find(Select)).toExist(); - expect(addWrapper.find(AsyncSelect)).toExist(); - expect(addWrapper.find(Select)).toHaveLength(3); - expect(addWrapper.find(AsyncSelect)).toHaveLength(3); - }); - - it('renders value input element when in alert mode', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - expect(wrapper.find('input[name="threshold"]')).toHaveLength(0); - expect(addWrapper.find('input[name="threshold"]')).toExist(); - }); - - it('renders two radio buttons', () => { - expect(wrapper.find(Radio)).toExist(); - expect(wrapper.find(Radio)).toHaveLength(2); - }); - - it('renders text option for text-based charts', async () => { - const props = { - ...mockedProps, - alert: mockData, - }; - const textWrapper = await mountAndWait(props); - - const chartOption = textWrapper.find('input[value="chart"]'); - act(() => { - chartOption.props().onChange({ target: { value: 'chart' } }); - }); - await waitForComponentToPaint(textWrapper); - - expect(textWrapper.find('input[value="TEXT"]')).toExist(); - }); - - it('renders input element for working timeout', () => { - expect(wrapper.find('input[name="working_timeout"]')).toExist(); - }); - - it('renders input element for grace period for alert only', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - expect(addWrapper.find('input[name="grace_period"]')).toExist(); - expect(wrapper.find('input[name="grace_period"]')).toHaveLength(0); - }); - - it('only allows grace period values > 1', async () => { - const props = { - ...mockedProps, - isReport: false, - }; - - const addWrapper = await mountAndWait(props); - - const input = addWrapper.find('input[name="grace_period"]'); - - input.simulate('change', { target: { name: 'grace_period', value: 7 } }); - expect(input.instance().value).toEqual('7'); - - input.simulate('change', { target: { name: 'grace_period', value: 0 } }); - expect(input.instance().value).toEqual(''); - - input.simulate('change', { target: { name: 'grace_period', value: -1 } }); - expect(input.instance().value).toEqual('1'); - }); - - it('only allows working timeout values > 1', () => { - const input = wrapper.find('input[name="working_timeout"]'); - - input.simulate('change', { target: { name: 'working_timeout', value: 7 } }); - expect(input.instance().value).toEqual('7'); - - input.simulate('change', { target: { name: 'working_timeout', value: 0 } }); - expect(input.instance().value).toEqual(''); - - input.simulate('change', { - target: { name: 'working_timeout', value: -1 }, - }); - expect(input.instance().value).toEqual('1'); - }); - - it('allows to add notification method', async () => { - const button = wrapper.find('[data-test="notification-add"]'); - act(() => { - button.props().onClick(); - }); - await waitForComponentToPaint(wrapper); - - // use default config: only show Email as notification option. - expect( - wrapper.find('[data-test="notification-add"]').props().status, - ).toEqual('hidden'); - act(() => { - wrapper - .find('[data-test="select-delivery-method"]') - .last() - .props() - .onSelect('Email'); - }); - await waitForComponentToPaint(wrapper); - expect(wrapper.find('textarea[name="recipients"]')).toHaveLength(1); - }); - - it('renders bypass cache checkbox', async () => { - const bypass = wrapper.find('[data-test="bypass-cache"]'); - expect(bypass).toExist(); - }); - - it('renders no bypass cache checkbox when alert', async () => { - const props = { - ...mockedProps, - alert: mockData, - isReport: false, - }; - - const alertWrapper = await mountAndWait(props); - - const bypass = alertWrapper.find('[data-test="bypass-cache"]'); - expect(bypass).not.toExist(); - }); -}); diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index e4f7454fecea1..ee9504286d8dc 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -17,68 +17,606 @@ * under the License. */ import React from 'react'; -import { render, screen, waitFor } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; -import AlertReportModal from './AlertReportModal'; +import fetchMock from 'fetch-mock'; +import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; +import { buildErrorTooltipMessage } from './buildErrorTooltipMessage'; +import AlertReportModal, { AlertReportModalProps } from './AlertReportModal'; +import { AlertObject } from './types'; -jest.mock('src/components/AsyncAceEditor', () => ({ - ...jest.requireActual('src/components/AsyncAceEditor'), - TextAreaEditor: () =>
, +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: () => true, })); -const onHide = jest.fn(); - -test('allows change to None in log retention', async () => { - render(, { useRedux: true }); - // open the log retention select - userEvent.click(screen.getByText('90 days')); - // change it to 30 days - userEvent.click(await screen.findByText('30 days')); - // open again - userEvent.click(screen.getAllByText('30 days')[0]); - // change it to None - userEvent.click(await screen.findByText('None')); - // get the selected item - const selectedItem = await waitFor(() => - screen - .getAllByLabelText('Log retention')[0] - .querySelector('.ant-select-selection-item'), +jest.mock('src/features/databases/state.ts', () => ({ + useCommonConf: () => ({ + ALERT_REPORTS_NOTIFICATION_METHODS: ['Email', 'Slack'], + }), +})); + +const generateMockPayload = (dashboard = true) => { + const mockPayload = { + active: false, + context_markdown: 'string', + creation_method: 'alerts_reports', + crontab: '0 0 * * *', + custom_width: null, + database: { + database_name: 'examples', + id: 1, + }, + description: 'Some description', + extra: {}, + force_screenshot: true, + grace_period: 14400, + id: 1, + last_eval_dttm: null, + last_state: 'Not triggered', + last_value: null, + last_value_row_json: null, + log_retention: 90, + name: 'Test Alert', + owners: [ + { + first_name: 'Superset', + id: 1, + last_name: 'Admin', + }, + ], + recipients: [ + { + id: 1, + recipient_config_json: '{"target": "test@user.com"}', + type: 'Email', + }, + ], + report_format: 'PNG', + sql: 'Select * From DB', + timezone: 'America/Rainy_River', + type: 'Alert', + validator_config_json: '{"threshold": 10.0, "op": "<"}', + validator_type: 'operator', + working_timeout: 3600, + }; + if (dashboard) { + return { + ...mockPayload, + dashboard: { id: 1, dashboard_title: 'Test Dashboard' }, + }; + } + return { + ...mockPayload, + chart: { + id: 1, + slice_name: 'Test Chart', + viz_type: 'table', + }, + }; +}; + +// mocking resource endpoints +const FETCH_DASHBOARD_ENDPOINT = 'glob:*/api/v1/report/1'; +const FETCH_CHART_ENDPOINT = 'glob:*/api/v1/report/2'; + +fetchMock.get(FETCH_DASHBOARD_ENDPOINT, { result: generateMockPayload(true) }); +fetchMock.get(FETCH_CHART_ENDPOINT, { result: generateMockPayload(false) }); + +// Related mocks +const ownersEndpoint = 'glob:*/api/v1/alert/related/owners?*'; +const databaseEndpoint = 'glob:*/api/v1/alert/related/database?*'; +const dashboardEndpoint = 'glob:*/api/v1/alert/related/dashboard?*'; +const chartEndpoint = 'glob:*/api/v1/alert/related/chart?*'; + +fetchMock.get(ownersEndpoint, { result: [] }); +fetchMock.get(databaseEndpoint, { result: [] }); +fetchMock.get(dashboardEndpoint, { result: [] }); +fetchMock.get(chartEndpoint, { result: [{ text: 'table chart', value: 1 }] }); + +// Create a valid alert with all required fields entered for validation check + +// @ts-ignore will add id in factory function +const validAlert: AlertObject = { + active: false, + changed_on_delta_humanized: 'now', + created_on: '2023-12-12T22:33:25.927764', + creation_method: 'alerts_reports', + crontab: '0 0 * * *', + dashboard_id: 0, + chart_id: 0, + force_screenshot: false, + last_state: 'Not triggered', + name: 'Test Alert', + owners: [ + { + first_name: 'Superset', + id: 1, + last_name: 'Admin', + }, + ], + recipients: [ + { + type: 'Email', + recipient_config_json: { target: 'test@user.com' }, + }, + ], + timezone: 'America/Rainy_River', + type: 'Alert', +}; + +jest.mock('./buildErrorTooltipMessage', () => ({ + buildErrorTooltipMessage: jest.fn(), +})); + +const generateMockedProps = ( + isReport = false, + useValidAlert = false, + useDashboard = true, +): AlertReportModalProps => { + let alert; + // switching ids for endpoint when testing dashboard vs chart edits + if (useDashboard) { + alert = { ...validAlert, id: 1 }; + } else { + alert = { ...validAlert, id: 2 }; + } + + return { + addDangerToast: () => {}, + addSuccessToast: () => {}, + onAdd: jest.fn(() => []), + onHide: jest.fn(), + alert: useValidAlert ? alert : null, + show: true, + isReport, + }; +}; + +// combobox selector for mocking user input +const comboboxSelect = async ( + element: HTMLElement, + value: string, + newElementQuery: Function, +) => { + expect(element).toBeInTheDocument(); + userEvent.type(element, `${value}{enter}`); + await waitFor(() => { + const element = newElementQuery(); + expect(element).toBeInTheDocument(); + }); +}; + +// --------------- TEST SECTION ------------------ +test('properly renders add alert text', () => { + const addAlertProps = generateMockedProps(); + render(, { useRedux: true }); + const addAlertHeading = screen.getByRole('heading', { name: /add alert/i }); + expect(addAlertHeading).toBeInTheDocument(); + const addButton = screen.getByRole('button', { name: /add/i }); + expect(addButton).toBeInTheDocument(); +}); + +test('properly renders edit alert text', async () => { + render(, { + useRedux: true, + }); + const editAlertHeading = screen.getByRole('heading', { + name: /edit alert/i, + }); + expect(editAlertHeading).toBeInTheDocument(); + const saveButton = screen.getByRole('button', { name: /save/i }); + expect(saveButton).toBeInTheDocument(); +}); + +test('properly renders add report text', () => { + render(, { + useRedux: true, + }); + const addReportHeading = screen.getByRole('heading', { + name: /add report/i, + }); + expect(addReportHeading).toBeInTheDocument(); + const addButton = screen.getByRole('button', { name: /add/i }); + expect(addButton).toBeInTheDocument(); +}); + +test('properly renders edit report text', async () => { + render(, { + useRedux: true, + }); + + const editReportHeading = screen.getByRole('heading', { + name: /edit report/i, + }); + expect(editReportHeading).toBeInTheDocument(); + const saveButton = screen.getByRole('button', { name: /save/i }); + expect(saveButton).toBeInTheDocument(); +}); + +test('renders 4 sections for reports', () => { + render(, { + useRedux: true, + }); + const sections = screen.getAllByRole('tab'); + expect(sections.length).toBe(4); +}); + +test('renders 5 sections for alerts', () => { + render(, { + useRedux: true, + }); + + const sections = screen.getAllByRole('tab'); + expect(sections.length).toBe(5); +}); + +// Validation +test('renders 5 checkmarks for a valid alert', async () => { + render(, { + useRedux: true, + }); + const checkmarks = await screen.findAllByRole('img', { + name: /check-circle/i, + }); + expect(checkmarks.length).toEqual(5); +}); + +test('renders single checkmarks when creating a new alert', async () => { + render(, { + useRedux: true, + }); + const checkmarks = await screen.findAllByRole('img', { + name: /check-circle/i, + }); + expect(checkmarks.length).toEqual(1); +}); + +test('disables save when validation fails', () => { + render(, { + useRedux: true, + }); + + expect(screen.getByRole('button', { name: /add/i })).toBeDisabled(); +}); + +test('calls build tooltip', async () => { + render(, { + useRedux: true, + }); + expect(buildErrorTooltipMessage).toHaveBeenCalled(); + expect(buildErrorTooltipMessage).toHaveBeenLastCalledWith({ + alertConditionSection: { + errors: ['database', 'sql', 'alert condition'], + name: 'Alert condition', + hasErrors: true, + }, + contentSection: { + errors: ['content type'], + name: 'Alert contents', + hasErrors: true, + }, + generalSection: { + errors: ['name'], + name: 'General information', + hasErrors: true, + }, + notificationSection: { + errors: ['recipients'], + name: 'Notification method', + hasErrors: true, + }, + scheduleSection: { errors: [], name: 'Schedule', hasErrors: false }, + }); +}); + +// General Section +test('opens General Section on render', async () => { + render(, { + useRedux: true, + }); + const general_header = within( + screen.getByRole('tab', { expanded: true }), + ).queryByText(/general information/i); + expect(general_header).toBeInTheDocument(); +}); + +test('renders all fields in General Section', () => { + render(, { + useRedux: true, + }); + const name = screen.getByPlaceholderText(/enter alert name/i); + const owners = screen.getByTestId('owners-select'); + const description = screen.getByPlaceholderText( + /include description to be sent with alert/i, + ); + const activeSwitch = screen.getByRole('switch'); + + expect(name).toBeInTheDocument(); + expect(owners).toBeInTheDocument(); + expect(description).toBeInTheDocument(); + expect(activeSwitch).toBeInTheDocument(); +}); + +// Alert Condition Section +/* A Note on textbox total numbers: + Because the General Info panel is open by default, the Name and Description textboxes register as being in the document on all tests, thus the total number of textboxes in each subsequent panel's tests will always be n+2. This is most significant in the Alert Condition panel tests because the nature of the SQL field as a TextAreaContol component may only be queried by role */ +test('opens Alert Condition Section on click', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('alert-condition-panel')); + const alertConditionHeader = within( + screen.getByRole('tab', { expanded: true }), + ).queryByText(/alert condition/i); + expect(alertConditionHeader).toBeInTheDocument(); +}); +test('renders all Alert Condition fields', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('alert-condition-panel')); + const database = screen.getByRole('combobox', { name: /database/i }); + const sql = screen.getAllByRole('textbox')[2]; + const condition = screen.getByRole('combobox', { name: /condition/i }); + const threshold = screen.getByRole('spinbutton'); + expect(database).toBeInTheDocument(); + expect(sql).toBeInTheDocument(); + expect(condition).toBeInTheDocument(); + expect(threshold).toBeInTheDocument(); +}); +test('disables condition threshold if not null condition is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('alert-condition-panel')); + await screen.findByText(/smaller than/i); + const condition = screen.getByRole('combobox', { name: /condition/i }); + await comboboxSelect( + condition, + 'not null', + () => screen.getAllByText(/not null/i)[0], ); - // check if None is selected - expect(selectedItem).toHaveTextContent('None'); + expect(screen.getByRole('spinbutton')).toBeDisabled(); }); -test('renders the appropriate dropdown in Message Content section', async () => { - render(, { useRedux: true }); +// Content Section +test('opens Contents Section on click', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + const contentsHeader = within( + screen.getByRole('tab', { expanded: true }), + ).queryByText(/contents/i); + expect(contentsHeader).toBeInTheDocument(); +}); + +test('renders screenshot options when dashboard is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); + expect( + screen.getByRole('combobox', { name: /select content type/i }), + ).toBeInTheDocument(); + expect( + screen.getByRole('combobox', { name: /dashboard/i }), + ).toBeInTheDocument(); + expect(screen.getByRole('spinbutton')).toBeInTheDocument(); + expect( + screen.getByRole('checkbox', { + name: /ignore cache when generating report/i, + }), + ).toBeInTheDocument(); +}); - const chartRadio = screen.getByRole('radio', { name: /chart/i }); +test('changes to content options when chart is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); + const contentTypeSelector = screen.getByRole('combobox', { + name: /select content type/i, + }); + await comboboxSelect(contentTypeSelector, 'Chart', () => + screen.getByRole('combobox', { name: /chart/i }), + ); + expect( + screen.getByRole('combobox', { + name: /select format/i, + }), + ).toBeInTheDocument(); +}); - // Dashboard is initially checked by default +test('removes ignore cache checkbox when chart is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); expect( - await screen.findByRole('radio', { - name: /dashboard/i, + screen.getByRole('checkbox', { + name: /ignore cache when generating report/i, }), - ).toBeChecked(); - expect(chartRadio).not.toBeChecked(); - // Only the dashboard dropdown should show - expect(screen.getByRole('combobox', { name: /dashboard/i })).toBeVisible(); + ).toBeInTheDocument(); + const contentTypeSelector = screen.getByRole('combobox', { + name: /select content type/i, + }); + await comboboxSelect( + contentTypeSelector, + 'Chart', + () => screen.getAllByText(/select chart/i)[0], + ); expect( - screen.queryByRole('combobox', { name: /chart/i }), - ).not.toBeInTheDocument(); + screen.queryByRole('checkbox', { + name: /ignore cache when generating report/i, + }), + ).toBe(null); +}); + +test('does not show screenshot width when csv is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test chart/i); + const contentTypeSelector = screen.getByRole('combobox', { + name: /select content type/i, + }); + await comboboxSelect(contentTypeSelector, 'Chart', () => + screen.getByText(/select chart/i), + ); + const reportFormatSelector = screen.getByRole('combobox', { + name: /select format/i, + }); + await comboboxSelect( + reportFormatSelector, + 'CSV', + () => screen.getAllByText(/Send as CSV/i)[0], + ); + expect(screen.queryByRole('spinbutton')).not.toBeInTheDocument(); +}); - // Click the chart radio option - userEvent.click(chartRadio); +// Schedule Section +test('opens Schedule Section on click', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + const scheduleHeader = within( + screen.getByRole('tab', { expanded: true }), + ).queryAllByText(/schedule/i)[0]; + expect(scheduleHeader).toBeInTheDocument(); +}); +test('renders default Schedule fields', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + const scheduleType = screen.getByRole('combobox', { + name: /schedule type/i, + }); + const timezone = screen.getByRole('combobox', { + name: /timezone selector/i, + }); + const logRetention = screen.getByRole('combobox', { + name: /log retention/i, + }); + const gracePeriod = screen.getByPlaceholderText(/time in seconds/i); + expect(scheduleType).toBeInTheDocument(); + expect(timezone).toBeInTheDocument(); + expect(logRetention).toBeInTheDocument(); + expect(gracePeriod).toBeInTheDocument(); +}); +test('renders working timout as report', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + expect(screen.getByText(/working timeout/i)).toBeInTheDocument(); +}); +test('renders grace period as alert', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + expect(screen.getByText(/grace period/i)).toBeInTheDocument(); +}); +test('shows CRON Expression when CRON is selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + await comboboxSelect( + screen.getByRole('combobox', { name: /schedule type/i }), + 'cron schedule', + () => screen.getByPlaceholderText(/cron expression/i), + ); + expect(screen.getByPlaceholderText(/cron expression/i)).toBeInTheDocument(); +}); +test('defaults to day when CRON is not selected', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('schedule-panel')); + const days = screen.getAllByTitle(/day/i, { exact: true }); + expect(days.length).toBe(2); +}); - await waitFor(() => expect(chartRadio).toBeChecked()); +// Notification Method Section +test('opens Notification Method Section on click', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('notification-method-panel')); + const notificationMethodHeader = within( + screen.getByRole('tab', { expanded: true }), + ).queryAllByText(/notification method/i)[0]; + expect(notificationMethodHeader).toBeInTheDocument(); +}); +test('renders all notification fields', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('notification-method-panel')); + const notificationMethod = screen.getByRole('combobox', { + name: /delivery method/i, + }); + const recipients = screen.getByTestId('recipients'); + const addNotificationMethod = screen.getByText( + /add another notification method/i, + ); + expect(notificationMethod).toBeInTheDocument(); + expect(recipients).toBeInTheDocument(); + expect(addNotificationMethod).toBeInTheDocument(); +}); +test('adds another notification method section after clicking add notification method', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('notification-method-panel')); + const addNotificationMethod = screen.getByText( + /add another notification method/i, + ); + userEvent.click(addNotificationMethod); expect( - await screen.findByRole('radio', { - name: /dashboard/i, - }), - ).not.toBeChecked(); - // Now that chart is checked, only the chart dropdown should show - expect(screen.getByRole('combobox', { name: /chart/i })).toBeVisible(); + screen.getAllByRole('combobox', { + name: /delivery method/i, + }).length, + ).toBe(2); + await comboboxSelect( + screen.getAllByRole('combobox', { + name: /delivery method/i, + })[1], + 'Slack', + () => screen.getAllByRole('textbox')[1], + ); + expect(screen.getAllByTestId('recipients').length).toBe(2); +}); + +test('removes notification method on clicking trash can', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('notification-method-panel')); + const addNotificationMethod = screen.getByText( + /add another notification method/i, + ); + userEvent.click(addNotificationMethod); + await comboboxSelect( + screen.getAllByRole('combobox', { + name: /delivery method/i, + })[1], + 'Email', + () => screen.getAllByRole('textbox')[1], + ); + const images = screen.getAllByRole('img'); + const trash = images[images.length - 1]; + userEvent.click(trash); expect( - screen.queryByRole('combobox', { name: /dashboard/i }), - ).not.toBeInTheDocument(); + screen.getAllByRole('combobox', { name: /delivery method/i }).length, + ).toBe(1); }); diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index 490755fa13897..c9123869f2d93 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -22,6 +22,7 @@ import React, { useEffect, useMemo, useCallback, + ReactNode, } from 'react'; import { css, @@ -35,19 +36,17 @@ import { import rison from 'rison'; import { useSingleViewResource } from 'src/views/CRUD/hooks'; -import Icons from 'src/components/Icons'; import { Input } from 'src/components/Input'; import { Switch } from 'src/components/Switch'; import Modal from 'src/components/Modal'; +import Collapse from 'src/components/Collapse'; import TimezoneSelector from 'src/components/TimezoneSelector'; -import { Radio } from 'src/components/Radio'; import { propertyComparator } from 'src/components/Select/utils'; import withToasts from 'src/components/MessageToasts/withToasts'; import Owner from 'src/types/Owner'; import { AntdCheckbox, AsyncSelect, Select } from 'src/components'; import TextAreaControl from 'src/explore/components/controls/TextAreaControl'; import { useCommonConf } from 'src/features/databases/state'; -import { CustomWidthHeaderStyle } from 'src/features/reports/ReportModal/styles'; import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; import { NotificationMethodOption, @@ -59,11 +58,17 @@ import { Operator, Recipient, AlertsReportsConfig, + ValidationObject, + Sections, } from 'src/features/alerts/types'; import { useSelector } from 'react-redux'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import NumberInput from './components/NumberInput'; import { AlertReportCronScheduler } from './components/AlertReportCronScheduler'; import { NotificationMethod } from './components/NotificationMethod'; +import ValidatedPanelHeader from './components/ValidatedPanelHeader'; +import StyledPanel from './components/StyledPanel'; +import { buildErrorTooltipMessage } from './buildErrorTooltipMessage'; const TIMEOUT_MIN = 1; const TEXT_BASED_VISUALIZATION_TYPES = [ @@ -77,7 +82,7 @@ type SelectValue = { label: string; }; -interface AlertReportModalProps { +export interface AlertReportModalProps { addSuccessToast: (msg: string) => void; addDangerToast: (msg: string) => void; alert?: AlertObject | null; @@ -88,7 +93,7 @@ interface AlertReportModalProps { } const DEFAULT_WORKING_TIMEOUT = 3600; -const DEFAULT_CRON_VALUE = '0 * * * *'; // every hour +const DEFAULT_CRON_VALUE = '0 0 * * *'; // every day const DEFAULT_RETENTION = 90; const DEFAULT_NOTIFICATION_METHODS: NotificationMethodOption[] = ['Email']; @@ -143,62 +148,50 @@ const RETENTION_OPTIONS = [ }, ]; -const StyledModal = styled(Modal)` - max-width: 1200px; - width: 100%; - - .ant-modal-body { - overflow: initial; - } -`; - -const StyledTooltip = styled(InfoTooltipWithTrigger)` - margin-left: ${({ theme }) => theme.gridUnit}px; -`; +const CONTENT_TYPE_OPTIONS = [ + { + label: t('Dashboard'), + value: 'dashboard', + }, + { + label: t('Chart'), + value: 'chart', + }, +]; +const FORMAT_OPTIONS = { + png: { + label: t('Send as PNG'), + value: 'PNG', + }, + csv: { + label: t('Send as CSV'), + value: 'CSV', + }, + txt: { + label: t('Send as text'), + value: 'TEXT', + }, +}; -const StyledIcon = (theme: SupersetTheme) => css` - margin: auto ${theme.gridUnit * 2}px auto 0; - color: ${theme.colors.grayscale.base}; +// Apply to final text input components of each collapse panel +const noMarginBottom = css` + margin-bottom: 0; `; -const StyledSectionContainer = styled.div` - display: flex; - flex-direction: column; +/* +Height of modal body defined here, total width defined at component invocation as antd prop. + */ +const StyledModal = styled(Modal)` + .ant-modal-body { + height: 720px; + } .control-label { margin-top: ${({ theme }) => theme.gridUnit}px; } - .header-section { - display: flex; - flex: 0 0 auto; - align-items: center; - width: 100%; - padding: ${({ theme }) => theme.gridUnit * 4}px; - border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; - } - - .column-section { - display: flex; - flex: 1 1 auto; - - .column { - flex: 1 1 auto; - min-width: calc(33.33% - ${({ theme }) => theme.gridUnit * 8}px); - padding: ${({ theme }) => theme.gridUnit * 4}px; - - .async-select { - margin: 10px 0 20px; - } - - &.condition { - border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; - } - - &.message { - border-left: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; - } - } + .ant-collapse > .ant-collapse-item { + border-bottom: none; } .inline-container { @@ -212,34 +205,6 @@ const StyledSectionContainer = styled.div` > div { flex: 1 1 auto; } - - &.add-margin { - margin-bottom: 5px; - } - - .styled-input { - margin: 0 0 0 10px; - - input { - flex: 0 0 auto; - } - } - } -`; - -const StyledSectionTitle = styled.div` - display: flex; - align-items: center; - margin: ${({ theme }) => theme.gridUnit * 2}px auto - ${({ theme }) => theme.gridUnit * 4}px auto; - - h4 { - margin: 0; - } - - .required { - margin-left: ${({ theme }) => theme.gridUnit}px; - color: ${({ theme }) => theme.colors.error.base}; } `; @@ -254,113 +219,112 @@ const StyledSwitchContainer = styled.div` `; export const StyledInputContainer = styled.div` - flex: 1; - margin-top: 0; + ${({ theme }) => css` + flex: 1; + margin-top: 0px; + margin-bottom: ${theme.gridUnit * 4}px; + + input::-webkit-outer-spin-button, + input::-webkit-inner-spin-button { + -webkit-appearance: none; + margin: 0; + } + input[type='number'] { + -moz-appearance: textfield; + } - input::-webkit-outer-spin-button, - input::-webkit-inner-spin-button { - -webkit-appearance: none; - margin: 0; - } - input[type='number'] { - -moz-appearance: textfield; - } + .helper { + display: block; + color: ${theme.colors.grayscale.base}; + font-size: ${theme.typography.sizes.s}px; + padding: ${theme.gridUnit}px 0; + text-align: left; + } - .helper { - display: block; - color: ${({ theme }) => theme.colors.grayscale.base}; - font-size: ${({ theme }) => theme.typography.sizes.s}px; - padding: ${({ theme }) => theme.gridUnit}px 0; - text-align: left; - } + .required { + margin-left: ${theme.gridUnit / 2}px; + color: ${theme.colors.error.base}; + } - .required { - margin-left: ${({ theme }) => theme.gridUnit / 2}px; - color: ${({ theme }) => theme.colors.error.base}; - } + .input-container { + display: flex; + align-items: center; - .input-container { - display: flex; - align-items: center; + > div { + width: 100%; + } - > div { - width: 100%; - } + label { + display: flex; + margin-right: ${theme.gridUnit * 2}px; + } - label { - display: flex; - margin-right: ${({ theme }) => theme.gridUnit * 2}px; + i { + margin: 0 ${theme.gridUnit}px; + } } - i { - margin: 0 ${({ theme }) => theme.gridUnit}px; + input, + textarea { + flex: 1 1 auto; } - } - input, - textarea { - flex: 1 1 auto; - } - - input[disabled] { - color: ${({ theme }) => theme.colors.grayscale.base}; - } + input[disabled] { + color: ${theme.colors.grayscale.base}; + } - textarea { - height: 300px; - resize: none; - } + textarea { + height: 300px; + resize: none; + } - input::placeholder, - textarea::placeholder { - color: ${({ theme }) => theme.colors.grayscale.light1}; - } + input::placeholder, + textarea::placeholder { + color: ${theme.colors.grayscale.light1}; + } - textarea, - input[type='text'], - input[type='number'] { - padding: ${({ theme }) => theme.gridUnit}px - ${({ theme }) => theme.gridUnit * 2}px; - border-style: none; - border: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; - border-radius: ${({ theme }) => theme.gridUnit}px; + textarea, + input[type='text'], + input[type='number'] { + padding: ${theme.gridUnit}px ${theme.gridUnit * 2}px; + border-style: none; + border: 1px solid ${theme.colors.grayscale.light2}; + border-radius: ${theme.gridUnit}px; - &[name='description'] { - flex: 1 1 auto; + &[name='description'] { + flex: 1 1 auto; + } } - } - .input-label { - margin-left: 10px; - } -`; - -const StyledRadio = styled(Radio)` - display: block; - line-height: ${({ theme }) => theme.gridUnit * 7}px; + .input-label { + margin-left: 10px; + } + `} `; -const StyledRadioGroup = styled(Radio.Group)` - margin-left: ${({ theme }) => theme.gridUnit * 5.5}px; +const StyledCheckbox = styled(AntdCheckbox)` + margin-top: ${({ theme }) => theme.gridUnit * 0}px; `; -const StyledCheckbox = styled(AntdCheckbox)` - margin-top: ${({ theme }) => theme.gridUnit * 2}px; +const StyledTooltip = styled(InfoTooltipWithTrigger)` + margin-left: ${({ theme }) => theme.gridUnit}px; `; // Notification Method components const StyledNotificationAddButton = styled.div` - color: ${({ theme }) => theme.colors.primary.dark1}; - cursor: pointer; + ${({ theme }) => css` + color: ${theme.colors.primary.dark1}; + cursor: pointer; - i { - margin-right: ${({ theme }) => theme.gridUnit * 2}px; - } + i { + margin-right: ${theme.gridUnit * 2}px; + } - &.disabled { - color: ${({ theme }) => theme.colors.grayscale.light1}; - cursor: default; - } + &.disabled { + color: ${theme.colors.grayscale.light1}; + cursor: default; + } + `} `; const StyledNotificationMethodWrapper = styled.div` @@ -369,10 +333,6 @@ const StyledNotificationMethodWrapper = styled.div` } `; -const timezoneHeaderStyle = (theme: SupersetTheme) => css` - margin: ${theme.gridUnit * 3}px 0; -`; - const inputSpacer = (theme: SupersetTheme) => css` margin-right: ${theme.gridUnit * 3}px; `; @@ -385,47 +345,26 @@ interface NotificationMethodAddProps { } export const TRANSLATIONS = { - ADD_NOTIFICATION_METHOD_TEXT: t('Add notification method'), - ADD_DELIVERY_METHOD_TEXT: t('Add delivery method'), - SAVE_TEXT: t('Save'), - ADD_TEXT: t('Add'), - EDIT_REPORT_TEXT: t('Edit Report'), - EDIT_ALERT_TEXT: t('Edit Alert'), - ADD_REPORT_TEXT: t('Add Report'), - ADD_ALERT_TEXT: t('Add Alert'), - REPORT_NAME_TEXT: t('Report name'), - ALERT_NAME_TEXT: t('Alert name'), - OWNERS_TEXT: t('Owners'), - DESCRIPTION_TEXT: t('Description'), - ACTIVE_TEXT: t('Active'), - ALERT_CONDITION_TEXT: t('Alert condition'), - DATABASE_TEXT: t('Database'), - SQL_QUERY_TEXT: t('SQL Query'), - SQL_QUERY_TOOLTIP: t( - 'The result of this query should be a numeric-esque value', + // Panel titles + GENERAL_TITLE: t('General information'), + ALERT_CONDITION_TITLE: t('Alert condition'), + ALERT_CONTENTS_TITLE: t('Alert contents'), + REPORT_CONTENTS_TITLE: t('Report contents'), + SCHEDULE_TITLE: t('Schedule'), + NOTIFICATION_TITLE: t('Notification method'), + // Error text + NAME_ERROR_TEXT: t('name'), + OWNERS_ERROR_TEXT: t('owners'), + CONTENT_ERROR_TEXT: t('content type'), + DATABASE_ERROR_TEXT: t('database'), + SQL_ERROR_TEXT: t('sql'), + ALERT_CONDITION_ERROR_TEXT: t('alert condition'), + CRONTAB_ERROR_TEXT: t('crontab'), + WORKING_TIMEOUT_ERROR_TEXT: t('working timeout'), + RECIPIENTS_ERROR_TEXT: t('recipients'), + ERROR_TOOLTIP_MESSAGE: t( + 'Not all required fields are complete. Please provide the following:', ), - TRIGGER_ALERT_IF_TEXT: t('Trigger Alert If...'), - CONDITION_TEXT: t('Condition'), - VALUE_TEXT: t('Value'), - REPORT_SCHEDULE_TEXT: t('Report schedule'), - ALERT_CONDITION_SCHEDULE_TEXT: t('Alert condition schedule'), - TIMEZONE_TEXT: t('Timezone'), - SCHEDULE_SETTINGS_TEXT: t('Schedule settings'), - LOG_RETENTION_TEXT: t('Log retention'), - WORKING_TIMEOUT_TEXT: t('Working timeout'), - TIME_IN_SECONDS_TEXT: t('Time in seconds'), - SECONDS_TEXT: t('seconds'), - GRACE_PERIOD_TEXT: t('Grace period'), - MESSAGE_CONTENT_TEXT: t('Message content'), - DASHBOARD_TEXT: t('Dashboard'), - CHART_TEXT: t('Chart'), - SEND_AS_PNG_TEXT: t('Send as PNG'), - SEND_AS_CSV_TEXT: t('Send as CSV'), - SEND_AS_TEXT: t('Send as text'), - IGNORE_CACHE_TEXT: t('Ignore cache when generating report'), - CUSTOM_SCREENSHOT_WIDTH_TEXT: t('Screenshot width'), - CUSTOM_SCREENSHOT_WIDTH_PLACEHOLDER_TEXT: t('Input custom width in pixels'), - NOTIFICATION_METHOD_TEXT: t('Notification method'), }; const NotificationMethodAdd: FunctionComponent = ({ @@ -446,8 +385,8 @@ const NotificationMethodAdd: FunctionComponent = ({ {' '} {status === 'active' - ? TRANSLATIONS.ADD_NOTIFICATION_METHOD_TEXT - : TRANSLATIONS.ADD_DELIVERY_METHOD_TEXT} + ? t('Add another notification method') + : t('Add delivery method')} ); }; @@ -470,11 +409,13 @@ const AlertReportModal: FunctionComponent = ({ const currentUser = useSelector( state => state.user, ); + // Check config for alternate notification methods setting const conf = useCommonConf(); const allowedNotificationMethods: NotificationMethodOption[] = conf?.ALERT_REPORTS_NOTIFICATION_METHODS || DEFAULT_NOTIFICATION_METHODS; const [disableSave, setDisableSave] = useState(true); + const [currentAlert, setCurrentAlert] = useState | null>(); const [isHidden, setIsHidden] = useState(true); @@ -497,10 +438,53 @@ const AlertReportModal: FunctionComponent = ({ const [sourceOptions, setSourceOptions] = useState([]); const [dashboardOptions, setDashboardOptions] = useState([]); const [chartOptions, setChartOptions] = useState([]); + // Validation + const [validationStatus, setValidationStatus] = useState({ + [Sections.General]: { + hasErrors: false, + name: TRANSLATIONS.GENERAL_TITLE, + errors: [], + }, + [Sections.Content]: { + hasErrors: false, + name: isReport + ? TRANSLATIONS.REPORT_CONTENTS_TITLE + : TRANSLATIONS.ALERT_CONTENTS_TITLE, + errors: [], + }, + [Sections.Alert]: { + hasErrors: false, + name: TRANSLATIONS.ALERT_CONDITION_TITLE, + errors: [], + }, + [Sections.Schedule]: { + hasErrors: false, + name: TRANSLATIONS.SCHEDULE_TITLE, + errors: [], + }, + [Sections.Notification]: { + hasErrors: false, + name: TRANSLATIONS.NOTIFICATION_TITLE, + errors: [], + }, + }); + const [errorTooltipMessage, setErrorTooltipMessage] = useState(''); + + const updateValidationStatus = (section: Sections, errors: string[]) => { + setValidationStatus(currentValidationData => ({ + ...currentValidationData, + [section]: { + hasErrors: errors.length > 0, + name: currentValidationData[section].name, + errors, + }, + })); + }; // Chart metadata const [chartVizType, setChartVizType] = useState(''); + const reportOrAlert = isReport ? 'report' : 'alert'; const isEditMode = alert !== null; const formatOptionEnabled = contentType === 'chart' && @@ -508,13 +492,12 @@ const AlertReportModal: FunctionComponent = ({ const [notificationAddState, setNotificationAddState] = useState('active'); + const [notificationSettings, setNotificationSettings] = useState< NotificationSetting[] >([]); - const onNotificationAdd = () => { const settings: NotificationSetting[] = notificationSettings.slice(); - settings.push({ recipients: '', options: allowedNotificationMethods, @@ -966,18 +949,14 @@ const AlertReportModal: FunctionComponent = ({ updateAlertState('timezone', timezone); }; - const onContentTypeChange = (event: any) => { - const { target } = event; + const onContentTypeChange = (value: string) => { // When switch content type, reset force_screenshot to false setForceScreenshot(false); - // Gives time to close the select before changing the type - setTimeout(() => setContentType(target.value), 200); + setContentType(value); }; - const onFormatChange = (event: any) => { - const { target } = event; - - setReportFormat(target.value); + const onFormatChange = (value: string) => { + setReportFormat(value); }; const onForceScreenshotChange = (event: any) => { @@ -1001,32 +980,89 @@ const AlertReportModal: FunctionComponent = ({ return hasInfo; }; - const validate = () => { + const validateGeneralSection = () => { + const errors = []; + if (!currentAlert?.name?.length) { + errors.push(TRANSLATIONS.NAME_ERROR_TEXT); + } + if (!currentAlert?.owners?.length) { + errors.push(TRANSLATIONS.OWNERS_ERROR_TEXT); + } + updateValidationStatus(Sections.General, errors); + }; + const validateContentSection = () => { + const errors = []; if ( - currentAlert?.name?.length && - currentAlert?.owners?.length && - currentAlert?.crontab?.length && - currentAlert?.working_timeout !== undefined && - ((contentType === 'dashboard' && !!currentAlert?.dashboard) || - (contentType === 'chart' && !!currentAlert?.chart)) && - checkNotificationSettings() + !( + (contentType === 'dashboard' && !!currentAlert?.dashboard) || + (contentType === 'chart' && !!currentAlert?.chart) + ) ) { - if (isReport) { - setDisableSave(false); - } else if ( - !!currentAlert.database && - currentAlert.sql?.length && - (conditionNotNull || !!currentAlert.validator_config_json?.op) && + errors.push(TRANSLATIONS.CONTENT_ERROR_TEXT); + } + updateValidationStatus(Sections.Content, errors); + }; + const validateAlertSection = () => { + const errors = []; + if (!currentAlert?.database) { + errors.push(TRANSLATIONS.DATABASE_ERROR_TEXT); + } + if (!currentAlert?.sql?.length) { + errors.push(TRANSLATIONS.SQL_ERROR_TEXT); + } + if ( + !( + (conditionNotNull || !!currentAlert?.validator_config_json?.op) && (conditionNotNull || - currentAlert.validator_config_json?.threshold !== undefined) - ) { - setDisableSave(false); - } else { - setDisableSave(true); - } - } else { - setDisableSave(true); + currentAlert?.validator_config_json?.threshold !== undefined) + ) + ) { + errors.push(TRANSLATIONS.ALERT_CONDITION_ERROR_TEXT); } + updateValidationStatus(Sections.Alert, errors); + }; + + const validateScheduleSection = () => { + const errors = []; + if (!currentAlert?.crontab?.length) { + errors.push(TRANSLATIONS.CRONTAB_ERROR_TEXT); + } + if (!currentAlert?.working_timeout) { + errors.push(TRANSLATIONS.WORKING_TIMEOUT_ERROR_TEXT); + } + + updateValidationStatus(Sections.Schedule, errors); + }; + + const validateNotificationSection = () => { + const hasErrors = !checkNotificationSettings(); + const errors = hasErrors ? [TRANSLATIONS.RECIPIENTS_ERROR_TEXT] : []; + updateValidationStatus(Sections.Notification, errors); + }; + + const validateAll = () => { + validateGeneralSection(); + validateContentSection(); + if (!isReport) validateAlertSection(); + validateScheduleSection(); + validateNotificationSection(); + }; + + const enforceValidation = () => { + const sections = [ + Sections.General, + Sections.Content, + isReport ? undefined : Sections.Alert, + Sections.Schedule, + Sections.Notification, + ]; + + const hasErrors = sections.some( + section => section && validationStatus[section].hasErrors, + ); + const tooltip = hasErrors ? buildErrorTooltipMessage(validationStatus) : ''; + setErrorTooltipMessage(tooltip); + setDisableSave(hasErrors); }; // Initialize @@ -1054,7 +1090,13 @@ const AlertReportModal: FunctionComponent = ({ ] : [], }); - setNotificationSettings([]); + setNotificationSettings([ + { + recipients: '', + options: allowedNotificationMethods, + method: 'Email', + }, + ]); setNotificationAddState('active'); } }, [alert]); @@ -1139,7 +1181,7 @@ const AlertReportModal: FunctionComponent = ({ // Validation const currentAlertSafe = currentAlert || {}; useEffect(() => { - validate(); + validateAll(); }, [ currentAlertSafe.name, currentAlertSafe.owners, @@ -1154,403 +1196,495 @@ const AlertReportModal: FunctionComponent = ({ notificationSettings, conditionNotNull, ]); + useEffect(() => { + enforceValidation(); + }, [validationStatus]); // Show/hide if (isHidden && show) { setIsHidden(false); } + const getTitleText = () => { + let titleText; + + switch (true) { + case isEditMode && isReport: + titleText = t('Edit Report'); + break; + case isEditMode: + titleText = t('Edit Alert'); + break; + case isReport: + titleText = t('Add Report'); + break; + default: + titleText = t('Add Alert'); + break; + } + + return titleText; + }; + return ( - {isEditMode ? ( - - ) : ( - - )} - {isEditMode && isReport - ? TRANSLATIONS.EDIT_REPORT_TEXT - : isEditMode - ? TRANSLATIONS.EDIT_ALERT_TEXT - : isReport - ? TRANSLATIONS.ADD_REPORT_TEXT - : TRANSLATIONS.ADD_ALERT_TEXT} - - } + width="500px" + centered + title={

{getTitleText()}

} > - -
- -
- {isReport - ? TRANSLATIONS.REPORT_NAME_TEXT - : TRANSLATIONS.ALERT_NAME_TEXT} - * -
-
- -
- - -
- {TRANSLATIONS.OWNERS_TEXT} - * -
-
- -
- - -
{TRANSLATIONS.DESCRIPTION_TEXT}
-
- -
- - - + -
{TRANSLATIONS.ACTIVE_TEXT}
- -
-
- {!isReport && ( -
- -

{TRANSLATIONS.ALERT_CONDITION_TEXT}

- - -
- {TRANSLATIONS.DATABASE_TEXT} - * -
-
- -
- - -
- {TRANSLATIONS.SQL_QUERY_TEXT} - - * -
- +
+ +
+ {isReport ? t('Report name') : t('Alert name')} + * +
+
+ - -
- -
- {TRANSLATIONS.TRIGGER_ALERT_IF_TEXT} - * -
-
- -
-
-
- )} -
- -

- {isReport - ? TRANSLATIONS.REPORT_SCHEDULE_TEXT - : TRANSLATIONS.ALERT_CONDITION_SCHEDULE_TEXT} -

- * - - updateAlertState('crontab', newVal)} - /> -
{TRANSLATIONS.TIMEZONE_TEXT}
-
timezoneHeaderStyle(theme)} - > - -
- -

{TRANSLATIONS.SCHEDULE_SETTINGS_TEXT}

- +
- {TRANSLATIONS.LOG_RETENTION_TEXT} + {t('Owners')} *
-
- +
+ + + +
+ {isReport ? t('Report is active') : t('Alert is active')} +
+ +
+ + {!isReport && ( + + } + key="condition" + >
- {TRANSLATIONS.WORKING_TIMEOUT_TEXT} + {t('Database')} *
- - {TRANSLATIONS.SECONDS_TEXT}
- {!isReport && ( - + +
+ {t('SQL Query')} + + * +
+ + +
+ +
+ {t('Trigger Alert If...')} + * +
+
+ - - {TRANSLATIONS.SECONDS_TEXT} -
- )} -
-
- -

{TRANSLATIONS.MESSAGE_CONTENT_TEXT}

+
+ + )} + + } + key="contents" + > + +
+ {t('Content type')} * - - - - {TRANSLATIONS.DASHBOARD_TEXT} - - {TRANSLATIONS.CHART_TEXT} - +
+ FORMAT_OPTIONS[key]) + } + placeholder={t('Select format')} + /> )} - {isScreenshot && ( - -
- {TRANSLATIONS.CUSTOM_SCREENSHOT_WIDTH_TEXT} + + {isScreenshot && ( + +
{t('Screenshot width')}
+
+ +
+ + )} + {(isReport || contentType === 'dashboard') && ( +
+ + {t('Ignore cache when generating report')} + +
+ )} + + + } + key="schedule" + > + updateAlertState('crontab', newVal)} + /> + +
+ {t('Timezone')} * +
+ + + +
+ {t('Log retention')} + * +
+
+
- - )} - {(isReport || contentType === 'dashboard') && ( -
- - {TRANSLATIONS.IGNORE_CACHE_TEXT} - -
+ + ) : ( + <> +
{t('Grace period')}
+
+ +
+ )} - -

{TRANSLATIONS.NOTIFICATION_METHOD_TEXT}

- * - - {notificationSettings.map((notificationSetting, i) => ( - - - - ))} - + + -
-
- + } + key="notification" + > + {notificationSettings.map((notificationSetting, i) => ( + + + + ))} + { + // Prohibit 'add notification method' button if only one present + allowedNotificationMethods.length > notificationSettings.length && ( + + ) + } + + ); }; diff --git a/superset-frontend/src/features/alerts/buildErrorTooltipMessage.test.tsx b/superset-frontend/src/features/alerts/buildErrorTooltipMessage.test.tsx new file mode 100644 index 0000000000000..c2463ac069baa --- /dev/null +++ b/superset-frontend/src/features/alerts/buildErrorTooltipMessage.test.tsx @@ -0,0 +1,70 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { render, screen } from 'spec/helpers/testing-library'; +import { buildErrorTooltipMessage } from './buildErrorTooltipMessage'; +import { SectionValidationObject } from './types'; + +const noErrors: SectionValidationObject = { + hasErrors: false, + errors: [], + name: 'No Errors', +}; + +const singleError: SectionValidationObject = { + hasErrors: true, + errors: ['first error'], + name: 'Single Error', +}; + +const threeErrors: SectionValidationObject = { + hasErrors: true, + errors: ['first error', 'second error', 'third error'], + name: 'Triple Error', +}; + +const validation = { noErrors, singleError, threeErrors }; + +test('builds with proper heading', () => { + render(buildErrorTooltipMessage(validation)); + const heading = screen.getByText( + /not all required fields are complete\. please provide the following:/i, + ); + expect(heading).toBeInTheDocument(); +}); + +test('only builds sections that have errors', async () => { + render(buildErrorTooltipMessage(validation)); + const noErrors = screen.queryByText(/no errors: /i); + const singleError = screen.getByText(/single error:/i); + const tripleError = screen.getByText(/triple error:/i); + expect(noErrors).not.toBeInTheDocument(); + expect(singleError).toBeInTheDocument(); + expect(tripleError).toBeInTheDocument(); +}); + +test('properly concatenates errors', async () => { + render(buildErrorTooltipMessage(validation)); + const singleError = screen.getByText(/single error: first error/i); + const tripleError = screen.getByText( + /triple error: first error, second error, third error/i, + ); + expect(singleError).toBeInTheDocument(); + expect(tripleError).toBeInTheDocument(); +}); diff --git a/superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx b/superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx new file mode 100644 index 0000000000000..2011f5fe31e48 --- /dev/null +++ b/superset-frontend/src/features/alerts/buildErrorTooltipMessage.tsx @@ -0,0 +1,49 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React from 'react'; +import { styled } from '@superset-ui/core'; +import { ValidationObject } from './types'; +import { TRANSLATIONS } from './AlertReportModal'; + +const StyledList = styled.ul` + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + padding-inline-start: ${({ theme }) => theme.gridUnit * 3}px; +`; + +export const buildErrorTooltipMessage = ( + validationStatus: ValidationObject, +) => { + const sectionErrors: string[] = []; + Object.values(validationStatus).forEach(section => { + if (section.hasErrors) { + const sectionTitle = `${section.name}: `; + sectionErrors.push(sectionTitle + section.errors.join(', ')); + } + }); + return ( +
+ {TRANSLATIONS.ERROR_TOOLTIP_MESSAGE} + + {sectionErrors.map(err => ( +
  • {err}
    • + ))} +     +
    + ); +}; diff --git a/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.test.tsx b/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.test.tsx deleted file mode 100644 index 5d36c2994dcab..0000000000000 --- a/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.test.tsx +++ /dev/null @@ -1,153 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -import React from 'react'; -import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; -import userEvent from '@testing-library/user-event'; -import { act } from 'react-dom/test-utils'; - -import { - AlertReportCronScheduler, - AlertReportCronSchedulerProps, -} from './AlertReportCronScheduler'; - -const createProps = (props: Partial = {}) => ({ - onChange: jest.fn(), - value: '* * * * *', - ...props, -}); - -test('should render', () => { - const props = createProps(); - render(); - - // Text found in the first radio option - expect(screen.getByText('Every')).toBeInTheDocument(); - // Text found in the second radio option - expect(screen.getByText('CRON Schedule')).toBeInTheDocument(); -}); - -test('only one radio option should be enabled at a time', () => { - const props = createProps(); - const { container } = render(); - - expect(screen.getByTestId('picker')).toBeChecked(); - expect(screen.getByTestId('input')).not.toBeChecked(); - - const pickerContainer = container.querySelector( - '.react-js-cron-select', - ) as HTMLElement; - const inputContainer = screen.getByTestId('input-content'); - - expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeEnabled(); - expect(inputContainer.querySelector('input[name="crontab"]')).toBeDisabled(); - - userEvent.click(screen.getByTestId('input')); - - expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeDisabled(); - expect(inputContainer.querySelector('input[name="crontab"]')).toBeEnabled(); - - userEvent.click(screen.getByTestId('picker')); - - expect(within(pickerContainer).getAllByRole('combobox')[0]).toBeEnabled(); - expect(inputContainer.querySelector('input[name="crontab"]')).toBeDisabled(); -}); - -test('picker mode updates correctly', async () => { - const onChangeCallback = jest.fn(); - const props = createProps({ - onChange: onChangeCallback, - }); - - const { container } = render(); - - expect(screen.getByTestId('picker')).toBeChecked(); - - const pickerContainer = container.querySelector( - '.react-js-cron-select', - ) as HTMLElement; - - const firstSelect = within(pickerContainer).getAllByRole('combobox')[0]; - act(() => { - userEvent.click(firstSelect); - }); - - expect(await within(pickerContainer).findByText('day')).toBeInTheDocument(); - act(() => { - userEvent.click(within(pickerContainer).getByText('day')); - }); - - expect(onChangeCallback).toHaveBeenLastCalledWith('* * * * *'); - - const secondSelect = container.querySelector( - '.react-js-cron-hours .ant-select-selector', - ) as HTMLElement; - await waitFor(() => { - expect(secondSelect).toBeInTheDocument(); - }); - - act(() => { - userEvent.click(secondSelect); - }); - - expect(await screen.findByText('9')).toBeInTheDocument(); - act(() => { - userEvent.click(screen.getByText('9')); - }); - - await waitFor(() => { - expect(onChangeCallback).toHaveBeenLastCalledWith('* 9 * * *'); - }); -}); - -test('input mode updates correctly', async () => { - const onChangeCallback = jest.fn(); - const props = createProps({ - onChange: onChangeCallback, - }); - - render(); - - const inputContainer = screen.getByTestId('input-content'); - userEvent.click(screen.getByTestId('input')); - - const input = inputContainer.querySelector( - 'input[name="crontab"]', - ) as HTMLElement; - await waitFor(() => { - expect(input).toBeEnabled(); - }); - - userEvent.clear(input); - expect(input).toHaveValue(''); - - const value = '* 10 2 * *'; - await act(async () => { - await userEvent.type(input, value, { delay: 1 }); - }); - - await waitFor(() => { - expect(input).toHaveValue(value); - }); - - act(() => { - userEvent.click(inputContainer); - }); - - expect(onChangeCallback).toHaveBeenLastCalledWith(value); -}); diff --git a/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.tsx b/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.tsx index 17652af510a93..821a91f8e65db 100644 --- a/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.tsx +++ b/superset-frontend/src/features/alerts/components/AlertReportCronScheduler.tsx @@ -19,9 +19,8 @@ import React, { useState, useCallback, useRef, FocusEvent } from 'react'; import { t, useTheme } from '@superset-ui/core'; -import { AntdInput, RadioChangeEvent } from 'src/components'; +import { AntdInput, Select } from 'src/components'; import { Input } from 'src/components/Input'; -import { Radio } from 'src/components/Radio'; import { CronPicker, CronError } from 'src/components/CronPicker'; import { StyledInputContainer } from '../AlertReportModal'; @@ -30,18 +29,29 @@ export interface AlertReportCronSchedulerProps { onChange: (change: string) => any; } +enum ScheduleType { + Picker = 'picker', + Input = 'input', +} + +const SCHEDULE_TYPE_OPTIONS = [ + { + label: t('Recurring (every)'), + value: ScheduleType.Picker, + }, + { + label: t('CRON Schedule'), + value: ScheduleType.Input, + }, +]; + export const AlertReportCronScheduler: React.FC< AlertReportCronSchedulerProps > = ({ value, onChange }) => { const theme = useTheme(); const inputRef = useRef(null); - const [scheduleFormat, setScheduleFormat] = useState<'picker' | 'input'>( - 'picker', - ); - - const handleRadioButtonChange = useCallback( - (e: RadioChangeEvent) => setScheduleFormat(e.target.value), - [], + const [scheduleFormat, setScheduleFormat] = useState( + ScheduleType.Picker, ); const customSetValue = useCallback( @@ -67,40 +77,52 @@ export const AlertReportCronScheduler: React.FC< return ( <> - -
    - + +
    + {t('Schedule type')} + * +
    +
    + customSetValue(e.target.value)} + onPressEnter={handlePressEnter} + /> + )} + {scheduleFormat === ScheduleType.Picker && ( -
    -
    - - {t('CRON Schedule')} - -
    - -
    -     -
    - + )} +     ); }; diff --git a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx index 528c7011d78f4..1708c76f76881 100644 --- a/superset-frontend/src/features/alerts/components/NotificationMethod.tsx +++ b/superset-frontend/src/features/alerts/components/NotificationMethod.tsx @@ -35,10 +35,6 @@ const StyledNotificationMethod = styled.div` .inline-container { margin-bottom: 10px; - .input-container { - margin-left: 10px; - } - > div { margin: 0; } @@ -119,6 +115,7 @@ export const NotificationMethod: FunctionComponent = ({
    +
    {t('Notification Method')}