diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts index a853718aff07..bf4bd319c1dd 100644 --- a/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts @@ -103,7 +103,7 @@ describe('Dashboard edit action', () => { .click() .then(() => { // assert that modal edit window has closed - cy.get('.ant-modal-body').should('not.be.visible'); + cy.get('.ant-modal-body').should('not.exist'); // assert title has been updated cy.get('.editable-title input').should('have.value', dashboardTitle); diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx deleted file mode 100644 index 53de9e4d8c36..000000000000 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ /dev/null @@ -1,116 +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, { useState } from 'react'; -import { useSelector } from 'react-redux'; -import { t, SupersetTheme, css, useTheme } from '@superset-ui/core'; -import Icons from 'src/components/Icons'; -import { Switch } from 'src/components/Switch'; -import { AlertObject } from 'src/views/CRUD/alert/types'; -import { Menu } from 'src/components/Menu'; -import { NoAnimationDropdown } from 'src/components/Dropdown'; -import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; - -import DeleteModal from 'src/components/DeleteModal'; - -const deleteColor = (theme: SupersetTheme) => css` - color: ${theme.colors.error.base}; -`; - -export default function HeaderReportActionsDropDown({ - showReportModal, - toggleActive, - deleteActiveReport, -}: { - showReportModal: () => void; - toggleActive: (data: AlertObject, checked: boolean) => void; - deleteActiveReport: (data: AlertObject) => void; -}) { - const reports = useSelector(state => state.reports); - const reportsIds = Object.keys(reports); - const report = reports[reportsIds[0]]; - const [currentReportDeleting, setCurrentReportDeleting] = - useState(null); - const theme = useTheme(); - - const toggleActiveKey = async (data: AlertObject, checked: boolean) => { - if (data?.id) { - toggleActive(data, checked); - } - }; - - const handleReportDelete = (report: AlertObject) => { - deleteActiveReport(report); - setCurrentReportDeleting(null); - }; - - const menu = () => ( - - - {t('Email reports active')} - toggleActiveKey(report, checked)} - size="small" - css={{ marginLeft: theme.gridUnit * 2 }} - /> - - {t('Edit email report')} - setCurrentReportDeleting(report)} - css={deleteColor} - > - {t('Delete email report')} - - - ); - - return isFeatureEnabled(FeatureFlag.ALERT_REPORTS) ? ( - <> - - triggerNode.closest('.action-button') - } - > - - - - - {currentReportDeleting && ( - { - if (currentReportDeleting) { - handleReportDelete(currentReportDeleting); - } - }} - onHide={() => setCurrentReportDeleting(null)} - open - title={t('Delete Report?')} - /> - )} - - ) : null; -} diff --git a/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx new file mode 100644 index 000000000000..b97b7e5d00bd --- /dev/null +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx @@ -0,0 +1,198 @@ +/** + * 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 * as React from 'react'; +import userEvent from '@testing-library/user-event'; +import { render, screen, act } from 'spec/helpers/testing-library'; +import * as featureFlags from '@superset-ui/core'; +import HeaderReportDropdown, { HeaderReportProps } from '.'; + +let isFeatureEnabledMock: jest.MockInstance; + +const createProps = () => ({ + dashboardId: 1, + useTextMenu: false, + isDropdownVisible: false, + setIsDropdownVisible: jest.fn, + setShowReportSubMenu: jest.fn, +}); + +const stateWithOnlyUser = { + explore: { + user: { + email: 'admin@test.com', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + createdOn: '2022-01-12T10:17:37.801361', + roles: { Admin: [['menu_access', 'Manage']] }, + userId: 1, + username: 'admin', + }, + }, + reports: {}, +}; + +const stateWithUserAndReport = { + explore: { + user: { + email: 'admin@test.com', + firstName: 'admin', + isActive: true, + lastName: 'admin', + permissions: {}, + createdOn: '2022-01-12T10:17:37.801361', + roles: { Admin: [['menu_access', 'Manage']] }, + userId: 1, + username: 'admin', + }, + }, + reports: { + dashboards: { + 1: { + id: 1, + result: { + active: true, + creation_method: 'dashboards', + crontab: '0 12 * * 1', + dashboard: 1, + name: 'Weekly Report', + owners: [1], + recipients: [ + { + recipient_config_json: { + target: 'admin@test.com', + }, + type: 'Email', + }, + ], + type: 'Report', + }, + }, + }, + }, +}; + +function setup(props: HeaderReportProps, initialState = {}) { + render( +
+ +
, + { useRedux: true, initialState }, + ); +} + +describe('Header Report Dropdown', () => { + beforeAll(() => { + isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation( + (featureFlag: featureFlags.FeatureFlag) => + featureFlag === featureFlags.FeatureFlag.ALERT_REPORTS, + ); + }); + + afterAll(() => { + // @ts-ignore + isFeatureEnabledMock.restore(); + }); + + it('renders correctly', () => { + const mockedProps = createProps(); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); + expect(screen.getByRole('button')).toBeInTheDocument(); + }); + + it('renders the dropdown correctly', () => { + const mockedProps = createProps(); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); + const emailReportModalButton = screen.getByRole('button'); + userEvent.click(emailReportModalButton); + expect(screen.getByText('Email reports active')).toBeInTheDocument(); + expect(screen.getByText('Edit email report')).toBeInTheDocument(); + expect(screen.getByText('Delete email report')).toBeInTheDocument(); + }); + + it('opens an edit modal', async () => { + const mockedProps = createProps(); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); + const emailReportModalButton = screen.getByRole('button'); + userEvent.click(emailReportModalButton); + const editModal = screen.getByText('Edit email report'); + userEvent.click(editModal); + const textBoxes = await screen.findAllByText('Edit email report'); + expect(textBoxes).toHaveLength(2); + }); + + it('opens a delete modal', () => { + const mockedProps = createProps(); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); + const emailReportModalButton = screen.getByRole('button'); + userEvent.click(emailReportModalButton); + const deleteModal = screen.getByText('Delete email report'); + userEvent.click(deleteModal); + expect(screen.getByText('Delete Report?')).toBeInTheDocument(); + }); + + it('renders a new report modal if there is no report', () => { + const mockedProps = createProps(); + act(() => { + setup(mockedProps, stateWithOnlyUser); + }); + const emailReportModalButton = screen.getByRole('button'); + userEvent.click(emailReportModalButton); + expect(screen.getByText('Schedule a new email report')).toBeInTheDocument(); + }); + + it('renders Manage Email Reports Menu if textMenu is set to true and there is a report', () => { + let mockedProps = createProps(); + mockedProps = { + ...mockedProps, + useTextMenu: true, + isDropdownVisible: true, + }; + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); + expect(screen.getByText('Email reports active')).toBeInTheDocument(); + expect(screen.getByText('Edit email report')).toBeInTheDocument(); + expect(screen.getByText('Delete email report')).toBeInTheDocument(); + }); + + it('renders Schedule Email Reports if textMenu is set to true and there is a report', () => { + let mockedProps = createProps(); + mockedProps = { + ...mockedProps, + useTextMenu: true, + isDropdownVisible: true, + }; + act(() => { + setup(mockedProps, stateWithOnlyUser); + }); + expect(screen.getByText('Set up an email report')).toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx new file mode 100644 index 000000000000..9e75b7fbaf71 --- /dev/null +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx @@ -0,0 +1,299 @@ +/** + * 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, { useState, useEffect } from 'react'; +import { usePrevious } from 'src/hooks/usePrevious'; +import { useSelector, useDispatch } from 'react-redux'; +import { + t, + SupersetTheme, + css, + useTheme, + FeatureFlag, + isFeatureEnabled, +} from '@superset-ui/core'; +import Icons from 'src/components/Icons'; +import { Switch } from 'src/components/Switch'; +import { AlertObject } from 'src/views/CRUD/alert/types'; +import { Menu } from 'src/components/Menu'; +import Checkbox from 'src/components/Checkbox'; +import { noOp } from 'src/utils/common'; +import { NoAnimationDropdown } from 'src/components/Dropdown'; +import DeleteModal from 'src/components/DeleteModal'; +import ReportModal from 'src/components/ReportModal'; +import { ChartState } from 'src/explore/types'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + fetchUISpecificReport, + toggleActive, + deleteActiveReport, +} from 'src/reports/actions/reports'; +import { reportSelector } from 'src/views/CRUD/hooks'; +import { MenuItemWithCheckboxContainer } from 'src/explore/components/ExploreAdditionalActionsMenu/index'; + +const deleteColor = (theme: SupersetTheme) => css` + color: ${theme.colors.error.base}; +`; + +const onMenuHover = (theme: SupersetTheme) => css` + & .ant-menu-item { + padding: 5px 12px; + margin-top: 0px; + margin-bottom: 4px; + :hover { + color: ${theme.colors.grayscale.dark1}; + } + } + :hover { + background-color: ${theme.colors.secondary.light5}; + } +`; + +const onMenuItemHover = (theme: SupersetTheme) => css` + &:hover { + color: ${theme.colors.grayscale.dark1}; + background-color: ${theme.colors.secondary.light5}; + } +`; +export enum CreationMethod { + CHARTS = 'charts', + DASHBOARDS = 'dashboards', +} +export interface HeaderReportProps { + dashboardId?: number; + chart?: ChartState; + useTextMenu?: boolean; + setShowReportSubMenu?: (show: boolean) => void; + setIsDropdownVisible?: (visible: boolean) => void; + isDropdownVisible?: boolean; + showReportSubMenu?: boolean; +} + +export default function HeaderReportDropDown({ + dashboardId, + chart, + useTextMenu = false, + setShowReportSubMenu, + setIsDropdownVisible, + isDropdownVisible, +}: HeaderReportProps) { + const dispatch = useDispatch(); + const report = useSelector(state => { + const resourceType = dashboardId + ? CreationMethod.DASHBOARDS + : CreationMethod.CHARTS; + return reportSelector(state, resourceType, dashboardId || chart?.id); + }); + + const isReportActive: boolean = report?.active || false; + const user: UserWithPermissionsAndRoles = useSelector< + any, + UserWithPermissionsAndRoles + >(state => state.user || state.explore?.user); + const canAddReports = () => { + if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { + return false; + } + + if (!user?.userId) { + // this is in the case that there is an anonymous user. + return false; + } + const roles = Object.keys(user.roles || []); + const permissions = roles.map(key => + user.roles[key].filter( + perms => perms[0] === 'menu_access' && perms[1] === 'Manage', + ), + ); + return permissions[0].length > 0; + }; + + const [currentReportDeleting, setCurrentReportDeleting] = + useState(null); + const theme = useTheme(); + const prevDashboard = usePrevious(dashboardId); + const [showModal, setShowModal] = useState(false); + const toggleActiveKey = async (data: AlertObject, checked: boolean) => { + if (data?.id) { + dispatch(toggleActive(data, checked)); + } + }; + + const handleReportDelete = async (report: AlertObject) => { + await dispatch(deleteActiveReport(report)); + setCurrentReportDeleting(null); + }; + + const shouldFetch = + canAddReports() && + !!((dashboardId && prevDashboard !== dashboardId) || chart?.id); + + useEffect(() => { + if (shouldFetch) { + dispatch( + fetchUISpecificReport({ + userId: user.userId, + filterField: dashboardId ? 'dashboard_id' : 'chart_id', + creationMethod: dashboardId ? 'dashboards' : 'charts', + resourceId: dashboardId || chart?.id, + }), + ); + } + }, []); + + const showReportSubMenu = report && setShowReportSubMenu && canAddReports(); + + useEffect(() => { + if (showReportSubMenu) { + setShowReportSubMenu(true); + } else if (!report && setShowReportSubMenu) { + setShowReportSubMenu(false); + } + }, [report]); + + const handleShowMenu = () => { + if (setIsDropdownVisible) { + setIsDropdownVisible(false); + setShowModal(true); + } + }; + + const handleDeleteMenuClick = () => { + if (setIsDropdownVisible) { + setIsDropdownVisible(false); + setCurrentReportDeleting(report); + } + }; + + const textMenu = () => + report ? ( + isDropdownVisible && ( + + toggleActiveKey(report, !isReportActive)} + > + + + {t('Email reports active')} + + + + {t('Edit email report')} + + + {t('Delete email report')} + + + ) + ) : ( + + + {t('Set up an email report')} + + + + ); + const menu = () => ( + + + {t('Email reports active')} + toggleActiveKey(report, checked)} + size="small" + css={{ marginLeft: theme.gridUnit * 2 }} + /> + + setShowModal(true)}> + {t('Edit email report')} + + setCurrentReportDeleting(report)} + css={deleteColor} + > + {t('Delete email report')} + + + ); + + const iconMenu = () => + report ? ( + <> + + triggerNode.closest('.action-button') + } + > + + + + + + ) : ( + setShowModal(true)} + > + + + ); + + return ( + <> + {canAddReports() && ( + <> + setShowModal(false)} + userEmail={user.email} + dashboardId={dashboardId} + chart={chart} + creationMethod={ + dashboardId ? CreationMethod.DASHBOARDS : CreationMethod.CHARTS + } + /> + {useTextMenu ? textMenu() : iconMenu()} + {currentReportDeleting && ( + { + if (currentReportDeleting) { + handleReportDelete(currentReportDeleting); + } + }} + onHide={() => setCurrentReportDeleting(null)} + open + title={t('Delete Report?')} + /> + )} + + )} + + ); +} diff --git a/superset-frontend/src/components/ReportModal/ReportModal.test.tsx b/superset-frontend/src/components/ReportModal/ReportModal.test.tsx index 44e3d0ef65c5..1db3aa0fd6c5 100644 --- a/superset-frontend/src/components/ReportModal/ReportModal.test.tsx +++ b/superset-frontend/src/components/ReportModal/ReportModal.test.tsx @@ -16,15 +16,21 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; +import * as React from 'react'; import userEvent from '@testing-library/user-event'; +import sinon from 'sinon'; +import fetchMock from 'fetch-mock'; import { render, screen } from 'spec/helpers/testing-library'; import * as featureFlags from 'src/featureFlags'; +import * as actions from 'src/reports/actions/reports'; import { FeatureFlag } from '@superset-ui/core'; import ReportModal from '.'; let isFeatureEnabledMock: jest.MockInstance; +const REPORT_ENDPOINT = 'glob:*/api/v1/report*'; +fetchMock.get(REPORT_ENDPOINT, {}); + const NOOP = () => {}; const defaultProps = { @@ -37,12 +43,10 @@ const defaultProps = { userId: 1, userEmail: 'test@test.com', dashboardId: 1, - creationMethod: 'charts_dashboards', - props: { - chart: { - sliceFormData: { - viz_type: 'table', - }, + creationMethod: 'dashboards', + chart: { + sliceFormData: { + viz_type: 'table', }, }, }; @@ -107,4 +111,69 @@ describe('Email Report Modal', () => { expect(reportNameTextbox).toHaveDisplayValue(''); expect(addButton).toBeDisabled(); }); + + describe('Email Report Modal', () => { + let isFeatureEnabledMock: any; + let dispatch: any; + + beforeEach(async () => { + isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation(() => true); + dispatch = sinon.spy(); + }); + + afterAll(() => { + isFeatureEnabledMock.mockRestore(); + }); + + it('creates a new email report', async () => { + // ---------- Render/value setup ---------- + const reportValues = { + id: 1, + result: { + active: true, + creation_method: 'dashboards', + crontab: '0 12 * * 1', + dashboard: 1, + name: 'Weekly Report', + owners: [1], + recipients: [ + { + recipient_config_json: { + target: 'test@test.com', + }, + type: 'Email', + }, + ], + type: 'Report', + }, + }; + // This is needed to structure the reportValues to match the fetchMock return + const stringyReportValues = `{"id":1,"result":{"active":true,"creation_method":"dashboards","crontab":"0 12 * * 1","dashboard":${1},"name":"Weekly Report","owners":[${1}],"recipients":[{"recipient_config_json":{"target":"test@test.com"},"type":"Email"}],"type":"Report"}}`; + // Watch for report POST + fetchMock.post(REPORT_ENDPOINT, reportValues); + + // Click "Add" button to create a new email report + const addButton = screen.getByRole('button', { name: /add/i }); + userEvent.click(addButton); + + // Mock addReport from Redux + const makeRequest = () => { + const request = actions.addReport(reportValues); + return request(dispatch); + }; + + return makeRequest().then(() => { + // 🐞 ----- There are 2 POST calls at this point ----- 🐞 + + // addReport's mocked POST return should match the mocked values + expect(fetchMock.lastOptions()?.body).toEqual(stringyReportValues); + // Dispatch should be called once for addReport + expect(dispatch.callCount).toBe(2); + const reportCalls = fetchMock.calls(REPORT_ENDPOINT); + expect(reportCalls).toHaveLength(2); + }); + }); + }); }); diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 0601c4b86773..632f9f91ec42 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -20,28 +20,28 @@ import React, { useState, useEffect, useReducer, - FunctionComponent, useCallback, useMemo, } from 'react'; import { t, SupersetTheme } from '@superset-ui/core'; -import { getClientErrorObject } from 'src/utils/getClientErrorObject'; import { useDispatch, useSelector } from 'react-redux'; +import { getClientErrorObject } from 'src/utils/getClientErrorObject'; import { addReport, editReport } from 'src/reports/actions/reports'; - import Alert from 'src/components/Alert'; import TimezoneSelector from 'src/components/TimezoneSelector'; import LabeledErrorBoundInput from 'src/components/Form/LabeledErrorBoundInput'; import Icons from 'src/components/Icons'; -import withToasts from 'src/components/MessageToasts/withToasts'; import { CronError } from 'src/components/CronPicker'; import { RadioChangeEvent } from 'src/components'; +import withToasts from 'src/components/MessageToasts/withToasts'; import { ChartState } from 'src/explore/types'; import { ReportCreationMethod, - ReportRecipientType, - ReportScheduleType, + ReportObject, + NOTIFICATION_FORMATS, } from 'src/reports/types'; +import { reportSelector } from 'src/views/CRUD/hooks'; +import { CreationMethod } from './HeaderReportDropdown'; import { antDErrorAlertStyles, StyledModal, @@ -60,32 +60,8 @@ import { StyledRadioGroup, } from './styles'; -export interface ReportObject { - id?: number; - active: boolean; - crontab: string; - dashboard?: number; - chart?: number; - description?: string; - log_retention: number; - name: string; - owners: number[]; - recipients: [ - { recipient_config_json: { target: string }; type: ReportRecipientType }, - ]; - report_format: string; - timezone: string; - type: ReportScheduleType; - validator_config_json: {} | null; - validator_type: string; - working_timeout: number; - creation_method: string; - force_screenshot: boolean; -} - interface ReportProps { onHide: () => {}; - onReportAdd: (report?: ReportObject) => {}; addDangerToast: (msg: string) => void; show: boolean; userId: number; @@ -95,6 +71,7 @@ interface ReportProps { dashboardId?: number; dashboardName?: string; creationMethod: ReportCreationMethod; + props: any; } const TEXT_BASED_VISUALIZATION_TYPES = [ @@ -104,12 +81,6 @@ const TEXT_BASED_VISUALIZATION_TYPES = [ 'paired_ttest', ]; -const NOTIFICATION_FORMATS = { - TEXT: 'TEXT', - PNG: 'PNG', - CSV: 'CSV', -}; - const INITIAL_STATE = { crontab: '0 12 * * 1', }; @@ -122,20 +93,25 @@ type ReportObjectState = Partial & { isSubmitting?: boolean; }; -const ReportModal: FunctionComponent = ({ - onReportAdd, +function ReportModal({ onHide, show = false, - ...props -}) => { - const vizType = props.chart?.sliceFormData?.viz_type; - const isChart = !!props.chart; + dashboardId, + chart, + userId, + userEmail, + creationMethod, + dashboardName, + chartName, +}: ReportProps) { + const vizType = chart?.sliceFormData?.viz_type; + const isChart = !!chart; const isTextBasedChart = isChart && vizType && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType); const defaultNotificationFormat = isTextBasedChart ? NOTIFICATION_FORMATS.TEXT : NOTIFICATION_FORMATS.PNG; - const entityName = props.dashboardName || props.chartName; + const entityName = dashboardName || chartName; const initialState: ReportObjectState = useMemo( () => ({ ...INITIAL_STATE, @@ -166,18 +142,22 @@ const ReportModal: FunctionComponent = ({ const [cronError, setCronError] = useState(); const dispatch = useDispatch(); - const reports = useSelector(state => state.reports); - const isEditMode = reports && Object.keys(reports).length; + // Report fetch logic + const report = useSelector(state => { + const resourceType = dashboardId + ? CreationMethod.DASHBOARDS + : CreationMethod.CHARTS; + return reportSelector(state, resourceType, dashboardId || chart?.id); + }); + const isEditMode = report && Object.keys(report).length; useEffect(() => { if (isEditMode) { - const reportsIds = Object.keys(reports); - const report = reports[reportsIds[0]]; setCurrentReport(report); } else { setCurrentReport('reset'); } - }, [isEditMode, reports]); + }, [isEditMode, report]); const onSave = async () => { // Create new Report @@ -185,13 +165,13 @@ const ReportModal: FunctionComponent = ({ type: 'Report', active: true, force_screenshot: false, - creation_method: props.creationMethod, - dashboard: props.dashboardId, - chart: props.chart?.id, - owners: [props.userId], + creation_method: creationMethod, + dashboard: dashboardId, + chart: chart?.id, + owners: [userId], recipients: [ { - recipient_config_json: { target: props.userEmail }, + recipient_config_json: { target: userEmail }, type: 'Email', }, ], @@ -217,8 +197,6 @@ const ReportModal: FunctionComponent = ({ setCurrentReport({ error }); } setCurrentReport({ isSubmitting: false }); - - if (onReportAdd) onReportAdd(); }; const wrappedTitle = ( @@ -363,6 +341,6 @@ const ReportModal: FunctionComponent = ({ )} ); -}; +} export default withToasts(ReportModal); diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index ea94aceead17..e5851fb2d500 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -19,11 +19,7 @@ import React from 'react'; import { render, screen, fireEvent } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; -import sinon from 'sinon'; import fetchMock from 'fetch-mock'; -import * as actions from 'src/reports/actions/reports'; -import * as featureFlags from 'src/featureFlags'; -import mockState from 'spec/fixtures/mockStateWithoutUser'; import { HeaderProps } from './types'; import Header from '.'; @@ -112,10 +108,7 @@ const redoProps = { redoLength: 1, }; -const REPORT_ENDPOINT = 'glob:*/api/v1/report*'; - fetchMock.get('glob:*/csstemplateasyncmodelview/api/read', {}); -fetchMock.get(REPORT_ENDPOINT, {}); function setup(props: HeaderProps, initialState = {}) { return render( @@ -323,171 +316,3 @@ test('should refresh the charts', async () => { userEvent.click(screen.getByText('Refresh dashboard')); expect(mockedProps.onRefresh).toHaveBeenCalledTimes(1); }); - -describe('Email Report Modal', () => { - let isFeatureEnabledMock: any; - let dispatch: any; - - beforeEach(async () => { - isFeatureEnabledMock = jest - .spyOn(featureFlags, 'isFeatureEnabled') - .mockImplementation(() => true); - dispatch = sinon.spy(); - }); - - afterAll(() => { - isFeatureEnabledMock.mockRestore(); - }); - - it('creates a new email report', async () => { - // ---------- Render/value setup ---------- - const mockedProps = createProps(); - setup(mockedProps); - - const reportValues = { - id: 1, - result: { - active: true, - creation_method: 'dashboards', - crontab: '0 12 * * 1', - dashboard: mockedProps.dashboardInfo.id, - name: 'Weekly Report', - owners: [mockedProps.user.userId], - recipients: [ - { - recipient_config_json: { - target: mockedProps.user.email, - }, - type: 'Email', - }, - ], - type: 'Report', - }, - }; - // This is needed to structure the reportValues to match the fetchMock return - const stringyReportValues = `{"id":1,"result":{"active":true,"creation_method":"dashboards","crontab":"0 12 * * 1","dashboard":${mockedProps.dashboardInfo.id},"name":"Weekly Report","owners":[${mockedProps.user.userId}],"recipients":[{"recipient_config_json":{"target":"${mockedProps.user.email}"},"type":"Email"}],"type":"Report"}}`; - // Watch for report POST - fetchMock.post(REPORT_ENDPOINT, reportValues); - - screen.logTestingPlaygroundURL(); - // ---------- Begin tests ---------- - // Click calendar icon to open email report modal - const emailReportModalButton = screen.getByRole('button', { - name: /schedule email report/i, - }); - userEvent.click(emailReportModalButton); - - // Click "Add" button to create a new email report - const addButton = screen.getByRole('button', { name: /add/i }); - userEvent.click(addButton); - - // Mock addReport from Redux - const makeRequest = () => { - const request = actions.addReport(reportValues); - return request(dispatch); - }; - - return makeRequest().then(() => { - // 🐞 ----- There are 2 POST calls at this point ----- 🐞 - - // addReport's mocked POST return should match the mocked values - expect(fetchMock.lastOptions()?.body).toEqual(stringyReportValues); - // Dispatch should be called once for addReport - expect(dispatch.callCount).toBe(2); - const reportCalls = fetchMock.calls(REPORT_ENDPOINT); - expect(reportCalls).toHaveLength(2); - }); - }); - - it('edits an existing email report', async () => { - // TODO (lyndsiWilliams): This currently does not work, see TODOs below - // The modal does appear with the edit title, but the PUT call is not registering - - // ---------- Render/value setup ---------- - const mockedProps = createProps(); - const editedReportValues = { - active: true, - creation_method: 'dashboards', - crontab: '0 12 * * 1', - dashboard: mockedProps.dashboardInfo.id, - name: 'Weekly Report edit', - owners: [mockedProps.user.userId], - recipients: [ - { - recipient_config_json: { - target: mockedProps.user.email, - }, - type: 'Email', - }, - ], - type: 'Report', - }; - - // getMockStore({ reports: reportValues }); - setup(mockedProps, mockState); - // TODO (lyndsiWilliams): currently fetchMock detects this PUT - // address as 'glob:*/api/v1/report/undefined', is not detected - // on fetchMock.calls() - fetchMock.put(`glob:*/api/v1/report*`, editedReportValues); - - // Mock fetchUISpecificReport from Redux - // const makeFetchRequest = () => { - // const request = actions.fetchUISpecificReport( - // mockedProps.user.userId, - // 'dashboard_id', - // 'dashboards', - // mockedProps.dashboardInfo.id, - // ); - // return request(dispatch); - // }; - - // makeFetchRequest(); - - dispatch(actions.setReport(editedReportValues)); - - // ---------- Begin tests ---------- - // Click calendar icon to open email report modal - const emailReportModalButton = screen.getByRole('button', { - name: /schedule email report/i, - }); - userEvent.click(emailReportModalButton); - - const nameTextbox = screen.getByTestId('report-name-test'); - userEvent.type(nameTextbox, ' edit'); - - const saveButton = screen.getByRole('button', { name: /save/i }); - userEvent.click(saveButton); - - // TODO (lyndsiWilliams): There should be a report in state at this porint, - // which would render the HeaderReportActionsDropDown under the calendar icon - // BLOCKER: I cannot get report to populate, as its data is handled through redux - expect.anything(); - }); - - it('Should render report header', async () => { - const mockedProps = createProps(); - setup(mockedProps); - expect( - screen.getByRole('button', { name: 'Schedule email report' }), - ).toBeInTheDocument(); - }); - - it('Should not render report header even with menu access for anonymous user', async () => { - const mockedProps = createProps(); - const anonymousUserProps = { - ...mockedProps, - user: { - roles: { - Public: [['menu_access', 'Manage']], - }, - permissions: { - datasource_access: ['[examples].[birth_names](id:2)'], - }, - }, - }; - setup(anonymousUserProps); - expect( - screen.queryByRole('button', { name: 'Schedule email report' }), - ).not.toBeInTheDocument(); - }); -}); diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index b1ae9cb4b148..f60a0d85d7ae 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -22,25 +22,22 @@ import React from 'react'; import PropTypes from 'prop-types'; import { styled, t, getSharedLabelColor } from '@superset-ui/core'; import ButtonGroup from 'src/components/ButtonGroup'; - +import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import { LOG_ACTIONS_PERIODIC_RENDER_DASHBOARD, LOG_ACTIONS_FORCE_REFRESH_DASHBOARD, LOG_ACTIONS_TOGGLE_EDIT_DASHBOARD, } from 'src/logger/LogUtils'; -import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; - import Icons from 'src/components/Icons'; import Button from 'src/components/Button'; import EditableTitle from 'src/components/EditableTitle'; import FaveStar from 'src/components/FaveStar'; import { safeStringify } from 'src/utils/safeStringify'; import HeaderActionsDropdown from 'src/dashboard/components/Header/HeaderActionsDropdown'; -import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; +import HeaderReportDropdown from 'src/components/ReportModal/HeaderReportDropdown'; import PublishedStatus from 'src/dashboard/components/PublishedStatus'; import UndoRedoKeyListeners from 'src/dashboard/components/UndoRedoKeyListeners'; import PropertiesModal from 'src/dashboard/components/PropertiesModal'; -import ReportModal from 'src/components/ReportModal'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import { @@ -78,7 +75,6 @@ const propTypes = { onChange: PropTypes.func.isRequired, fetchFaveStar: PropTypes.func.isRequired, fetchCharts: PropTypes.func.isRequired, - fetchUISpecificReport: PropTypes.func.isRequired, saveFaveStar: PropTypes.func.isRequired, savePublished: PropTypes.func.isRequired, updateDashboardTitle: PropTypes.func.isRequired, @@ -153,7 +149,6 @@ class Header extends React.PureComponent { didNotifyMaxUndoHistoryToast: false, emphasizeUndo: false, showingPropertiesModal: false, - showingReportModal: false, }; this.handleChangeText = this.handleChangeText.bind(this); @@ -165,26 +160,11 @@ class Header extends React.PureComponent { this.overwriteDashboard = this.overwriteDashboard.bind(this); this.showPropertiesModal = this.showPropertiesModal.bind(this); this.hidePropertiesModal = this.hidePropertiesModal.bind(this); - this.showReportModal = this.showReportModal.bind(this); - this.hideReportModal = this.hideReportModal.bind(this); - this.renderReportModal = this.renderReportModal.bind(this); } componentDidMount() { - const { refreshFrequency, user, dashboardInfo } = this.props; + const { refreshFrequency } = this.props; this.startPeriodicRender(refreshFrequency * 1000); - if (this.canAddReports()) { - // this is in case there is an anonymous user. - if (Object.entries(dashboardInfo).length) { - this.props.fetchUISpecificReport( - user.userId, - 'dashboard_id', - 'dashboards', - dashboardInfo.id, - user.email, - ); - } - } } componentDidUpdate(prevProps) { @@ -195,7 +175,6 @@ class Header extends React.PureComponent { } UNSAFE_componentWillReceiveProps(nextProps) { - const { user } = this.props; if ( UNDO_LIMIT - nextProps.undoLength <= 0 && !this.state.didNotifyMaxUndoHistoryToast @@ -209,19 +188,6 @@ class Header extends React.PureComponent { ) { this.props.setMaxUndoHistoryExceeded(); } - if ( - this.canAddReports() && - nextProps.dashboardInfo.id !== this.props.dashboardInfo.id - ) { - // this is in case there is an anonymous user. - this.props.fetchUISpecificReport( - user?.userId, - 'dashboard_id', - 'dashboards', - nextProps?.dashboardInfo?.id, - user?.email, - ); - } } componentWillUnmount() { @@ -414,14 +380,6 @@ class Header extends React.PureComponent { this.setState({ showingPropertiesModal: false }); } - showReportModal() { - this.setState({ showingReportModal: true }); - } - - hideReportModal() { - this.setState({ showingReportModal: false }); - } - showEmbedModal = () => { this.setState({ showingEmbedModal: true }); }; @@ -430,47 +388,6 @@ class Header extends React.PureComponent { this.setState({ showingEmbedModal: false }); }; - renderReportModal() { - const attachedReportExists = !!Object.keys(this.props.reports).length; - return attachedReportExists ? ( - - ) : ( - <> - - - - - ); - } - - canAddReports() { - if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { - return false; - } - const { user } = this.props; - if (!user?.userId) { - // this is in the case that there is an anonymous user. - return false; - } - const roles = Object.keys(user.roles || []); - const permissions = roles.map(key => - user.roles[key].filter( - perms => perms[0] === 'menu_access' && perms[1] === 'Manage', - ), - ); - return permissions[0].length > 0; - } - render() { const { dashboardTitle, @@ -500,6 +417,7 @@ class Header extends React.PureComponent { lastModifiedTime, filterboxMigrationState, } = this.props; + const userCanEdit = dashboardInfo.dash_edit_perm && filterboxMigrationState !== FILTER_BOX_MIGRATION_STATES.REVIEWING && @@ -511,7 +429,6 @@ class Header extends React.PureComponent { const userCanCurate = isFeatureEnabled(FeatureFlag.EMBEDDED_SUPERSET) && findPermission('can_set_embedded', 'Dashboard', user.roles); - const shouldShowReport = !editMode && this.canAddReports(); const refreshLimit = dashboardInfo.common?.conf?.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT; const refreshWarning = @@ -625,48 +542,41 @@ class Header extends React.PureComponent { )} )} - {editMode && ( + {editMode ? ( - )} - - {!editMode && userCanEdit && ( + ) : ( <> - - - + {userCanEdit && ( + + + + )} + )} - {shouldShowReport && this.renderReportModal()} - - - {this.state.showingReportModal && ( - )} diff --git a/superset-frontend/src/dashboard/containers/DashboardHeader.jsx b/superset-frontend/src/dashboard/containers/DashboardHeader.jsx index 0605237cc55f..121215fe8bfe 100644 --- a/superset-frontend/src/dashboard/containers/DashboardHeader.jsx +++ b/superset-frontend/src/dashboard/containers/DashboardHeader.jsx @@ -56,11 +56,7 @@ import { import { logEvent } from 'src/logger/actions'; import { DASHBOARD_HEADER_ID } from 'src/dashboard/util/constants'; -import { - fetchUISpecificReport, - toggleActive, - deleteActiveReport, -} from 'src/reports/actions/reports'; +import { fetchUISpecificReport } from 'src/reports/actions/reports'; function mapStateToProps({ dashboardLayout: undoableLayout, @@ -134,8 +130,6 @@ function mapDispatchToProps(dispatch) { dashboardTitleChanged, updateDataMask, fetchUISpecificReport, - toggleActive, - deleteActiveReport, }, dispatch, ); diff --git a/superset-frontend/src/dashboard/reducers/types.ts b/superset-frontend/src/dashboard/reducers/types.ts index c1c723cf0179..7b3aec25241e 100644 --- a/superset-frontend/src/dashboard/reducers/types.ts +++ b/superset-frontend/src/dashboard/reducers/types.ts @@ -34,6 +34,16 @@ export type ChartConfiguration = { }; }; +export type User = { + email: string; + firstName: string; + isActive: boolean; + lastName: string; + permissions: Record; + roles: Record; + userId: number; + username: string; +}; export interface DashboardInfo { id: number; json_metadata: string; diff --git a/superset-frontend/src/explore/components/DataTablesPane/index.tsx b/superset-frontend/src/explore/components/DataTablesPane/index.tsx index 22f39328013c..fc01483703c7 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/index.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/index.tsx @@ -336,7 +336,6 @@ export const DataTablesPane = ({ }, [queryFormData, columnNames], ); - useEffect(() => { setItem(LocalStorageKeys.is_datapanel_open, panelOpen); }, [panelOpen]); diff --git a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreAdditionalActionsMenu.test.tsx b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreAdditionalActionsMenu.test.tsx index 93c6b9d9b57d..2b323db4bcad 100644 --- a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreAdditionalActionsMenu.test.tsx +++ b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreAdditionalActionsMenu.test.tsx @@ -80,8 +80,8 @@ const createProps = () => ({ chartStatus: 'rendered', onOpenPropertiesModal: jest.fn(), onOpenInEditor: jest.fn(), - canAddReports: false, canDownloadCSV: false, + showReportSubMenu: false, }); fetchMock.post( @@ -120,18 +120,6 @@ test('Should open a menu', () => { expect(screen.queryByText('Manage email report')).not.toBeInTheDocument(); }); -test('Menu has email report item if user can add report', () => { - const props = createProps(); - props.canAddReports = true; - render(, { - useRedux: true, - }); - - userEvent.click(screen.getByRole('button')); - expect(screen.queryByText('Manage email report')).not.toBeInTheDocument(); - expect(screen.getByText('Set up an email report')).toBeInTheDocument(); -}); - test('Should open download submenu', async () => { const props = createProps(); render(, { @@ -174,31 +162,6 @@ test('Should open share submenu', async () => { expect(await screen.findByText('Share chart by email')).toBeInTheDocument(); }); -test('Should open report submenu if report exists', async () => { - const props = createProps(); - props.canAddReports = true; - render(, { - useRedux: true, - initialState: { - reports: { - '1': { name: 'Test report' }, - }, - }, - }); - - userEvent.click(screen.getByRole('button')); - - expect(screen.queryByText('Email reports active')).not.toBeInTheDocument(); - expect(screen.queryByText('Edit email report')).not.toBeInTheDocument(); - expect(screen.queryByText('Download as image')).not.toBeInTheDocument(); - - expect(screen.getByText('Manage email report')).toBeInTheDocument(); - userEvent.hover(screen.getByText('Manage email report')); - expect(await screen.findByText('Email reports active')).toBeInTheDocument(); - expect(await screen.findByText('Edit email report')).toBeInTheDocument(); - expect(await screen.findByText('Delete email report')).toBeInTheDocument(); -}); - test('Should call onOpenPropertiesModal when click on "Edit chart properties"', () => { const props = createProps(); render(, { diff --git a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx deleted file mode 100644 index 4a4c00248648..000000000000 --- a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/ExploreReport.tsx +++ /dev/null @@ -1,87 +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, { useCallback } from 'react'; -import { useDispatch, useSelector } from 'react-redux'; -import pick from 'lodash/pick'; -import { t } from '@superset-ui/core'; -import ReportModal from 'src/components/ReportModal'; -import { ExplorePageState } from 'src/explore/reducers/getInitialState'; -import DeleteModal from 'src/components/DeleteModal'; -import { deleteActiveReport } from 'src/reports/actions/reports'; - -type ReportMenuItemsProps = { - report: Record; - isVisible: boolean; - onHide: () => void; - isDeleting: boolean; - setIsDeleting: (isDeleting: boolean) => void; -}; -export const ExploreReport = ({ - report, - isVisible, - onHide, - isDeleting, - setIsDeleting, -}: ReportMenuItemsProps) => { - const dispatch = useDispatch(); - const { chart, chartName } = useSelector((state: ExplorePageState) => ({ - chart: Object.values(state.charts || {})[0], - chartName: state.explore.sliceName, - })); - - const { userId, email } = useSelector< - ExplorePageState, - { userId?: number; email?: string } - >(state => pick(state.explore.user, ['userId', 'email'])); - - const handleReportDelete = useCallback(() => { - dispatch(deleteActiveReport(report)); - setIsDeleting(false); - }, [dispatch, report, setIsDeleting]); - - return ( - <> - - {isDeleting && ( - { - if (report) { - handleReportDelete(); - } - }} - onHide={() => setIsDeleting(false)} - open - title={t('Delete Report?')} - /> - )} - - ); -}; diff --git a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx index bd6e4ad5b56d..2fa9ab99c8c6 100644 --- a/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx +++ b/superset-frontend/src/explore/components/ExploreAdditionalActionsMenu/index.jsx @@ -17,7 +17,7 @@ * under the License. */ import React, { useCallback, useState } from 'react'; -import { useDispatch, useSelector } from 'react-redux'; +import { useSelector } from 'react-redux'; import PropTypes from 'prop-types'; import { FileOutlined, FileImageOutlined } from '@ant-design/icons'; import { css, styled, t, useTheme } from '@superset-ui/core'; @@ -27,15 +27,12 @@ import Icons from 'src/components/Icons'; import ModalTrigger from 'src/components/ModalTrigger'; import Button from 'src/components/Button'; import withToasts from 'src/components/MessageToasts/withToasts'; -import Checkbox from 'src/components/Checkbox'; import { exportChart } from 'src/explore/exploreUtils'; import downloadAsImage from 'src/utils/downloadAsImage'; -import { noOp } from 'src/utils/common'; import { getChartPermalink } from 'src/utils/urlUtils'; -import { toggleActive } from 'src/reports/actions/reports'; +import HeaderReportDropDown from 'src/components/ReportModal/HeaderReportDropdown'; import ViewQueryModal from '../controls/ViewQueryModal'; import EmbedCodeContent from '../EmbedCodeContent'; -import { ExploreReport } from './ExploreReport'; import copyTextToClipboard from '../../../utils/copy'; const propTypes = { @@ -69,7 +66,7 @@ const MENU_KEYS = { const VIZ_TYPES_PIVOTABLE = ['pivot_table', 'pivot_table_v2']; -const MenuItemWithCheckboxContainer = styled.div` +export const MenuItemWithCheckboxContainer = styled.div` ${({ theme }) => css` display: flex; align-items: center; @@ -86,7 +83,7 @@ const MenuItemWithCheckboxContainer = styled.div` `} `; -const MenuTrigger = styled(Button)` +export const MenuTrigger = styled(Button)` ${({ theme }) => css` width: ${theme.gridUnit * 8}px; height: ${theme.gridUnit * 8}px; @@ -112,25 +109,22 @@ const ExploreAdditionalActionsMenu = ({ slice, onOpenInEditor, onOpenPropertiesModal, - canAddReports, }) => { const theme = useTheme(); + const [showReportSubMenu, setShowReportSubMenu] = useState(null); const [isDropdownVisible, setIsDropdownVisible] = useState(false); const [openSubmenus, setOpenSubmenus] = useState([]); - const [showReportModal, setShowReportModal] = useState(false); - const [showDeleteReportModal, setShowDeleteReportModal] = useState(false); - const dispatch = useDispatch(); - const report = useSelector(state => { - if (!state.reports) { + const chart = useSelector(state => { + if (!state.charts) { return undefined; } - const reports = Object.values(state?.reports); - if (reports.length > 0) { - return reports[0]; + const charts = Object.values(state.charts); + if (charts.length > 0) { + return charts[0]; } return undefined; }); - const isReportActive = report?.active; + const { datasource } = latestQueryFormData; const sqlSupported = datasource && datasource.split('__')[1] === 'table'; @@ -240,23 +234,6 @@ const ExploreAdditionalActionsMenu = ({ setIsDropdownVisible(false); setOpenSubmenus([]); break; - case MENU_KEYS.SET_UP_REPORT: - setShowReportModal(true); - setIsDropdownVisible(false); - break; - case MENU_KEYS.SET_REPORT_ACTIVE: - dispatch(toggleActive(report, !isReportActive)); - break; - case MENU_KEYS.EDIT_REPORT: - setShowReportModal(true); - setIsDropdownVisible(false); - setOpenSubmenus([]); - break; - case MENU_KEYS.DELETE_REPORT: - setShowDeleteReportModal(true); - setIsDropdownVisible(false); - setOpenSubmenus([]); - break; case MENU_KEYS.VIEW_QUERY: setIsDropdownVisible(false); break; @@ -270,15 +247,12 @@ const ExploreAdditionalActionsMenu = ({ }, [ copyLink, - dispatch, exportCSV, exportCSVPivoted, exportJson, - isReportActive, latestQueryFormData, onOpenInEditor, onOpenPropertiesModal, - report, shareByEmail, slice?.slice_name, ], @@ -372,35 +346,31 @@ const ExploreAdditionalActionsMenu = ({ - {canAddReports && ( + {showReportSubMenu ? ( <> - {report ? ( - - - - - {t('Email reports active')} - - - - {t('Edit email report')} - - - {t('Delete email report')} - - - ) : ( - - {t('Set up an email report')} - - )} + + + + ) : ( + + + )} - - setShowReportModal(false)} - isDeleting={showDeleteReportModal} - setIsDeleting={setShowDeleteReportModal} - /> ); }; diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 8f298dce7676..324a6965a453 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -20,8 +20,14 @@ import React from 'react'; import { render, screen } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; + +import fetchMock from 'fetch-mock'; import ExploreHeader from '.'; +const chartEndpoint = 'glob:*api/v1/chart/*'; + +fetchMock.get(chartEndpoint, { json: 'foo' }); + const createProps = () => ({ chart: { latestQueryFormData: { diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 1be9dd77a2be..92b949d78548 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -20,18 +20,14 @@ import React from 'react'; import { connect } from 'react-redux'; import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; +import { Tooltip } from 'src/components/Tooltip'; import { CategoricalColorNamespace, css, SupersetClient, t, } from '@superset-ui/core'; -import { - fetchUISpecificReport, - toggleActive, - deleteActiveReport, -} from 'src/reports/actions/reports'; -import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; +import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import AlteredSliceTag from 'src/components/AlteredSliceTag'; import FaveStar from 'src/components/FaveStar'; @@ -40,7 +36,6 @@ import Icons from 'src/components/Icons'; import PropertiesModal from 'src/explore/components/PropertiesModal'; import { sliceUpdated } from 'src/explore/actions/exploreActions'; import CertifiedBadge from 'src/components/CertifiedBadge'; -import { Tooltip } from 'src/components/Tooltip'; import ExploreAdditionalActionsMenu from '../ExploreAdditionalActionsMenu'; import { ChartEditableTitle } from './ChartEditableTitle'; @@ -124,16 +119,6 @@ export class ExploreChartHeader extends React.PureComponent { componentDidMount() { const { dashboardId } = this.props; - if (this.canAddReports()) { - const { user, chart } = this.props; - // this is in the case that there is an anonymous user. - this.props.fetchUISpecificReport( - user.userId, - 'chart_id', - 'charts', - chart.id, - ); - } if (dashboardId) { this.fetchChartDashboardData(); } @@ -201,24 +186,6 @@ export class ExploreChartHeader extends React.PureComponent { }); } - canAddReports() { - if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { - return false; - } - const { user } = this.props; - if (!user?.userId) { - // this is in the case that there is an anonymous user. - return false; - } - const roles = Object.keys(user.roles || []); - const permissions = roles.map(key => - user.roles[key].filter( - perms => perms[0] === 'menu_access' && perms[1] === 'Manage', - ), - ); - return permissions[0].length > 0; - } - render() { const { actions, @@ -312,7 +279,6 @@ export class ExploreChartHeader extends React.PureComponent { slice={slice} canDownloadCSV={canDownload} latestQueryFormData={latestQueryFormData} - canAddReports={this.canAddReports()} /> @@ -324,7 +290,7 @@ ExploreChartHeader.propTypes = propTypes; function mapDispatchToProps(dispatch) { return bindActionCreators( - { sliceUpdated, fetchUISpecificReport, toggleActive, deleteActiveReport }, + { sliceUpdated, toggleActive, deleteActiveReport }, dispatch, ); } diff --git a/superset-frontend/src/explore/components/PropertiesModal/index.tsx b/superset-frontend/src/explore/components/PropertiesModal/index.tsx index 7fa4ef4580ec..de1639894b77 100644 --- a/superset-frontend/src/explore/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/explore/components/PropertiesModal/index.tsx @@ -83,7 +83,7 @@ function PropertiesModal({ }); const chart = response.json.result; setSelectedOwners( - chart.owners.map((owner: any) => ({ + chart?.owners?.map((owner: any) => ({ value: owner.id, label: `${owner.first_name} ${owner.last_name}`, })), diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index f0ae4012b12f..6fbd8ca4ec0f 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -25,27 +25,27 @@ import { } from 'src/components/MessageToasts/actions'; export const SET_REPORT = 'SET_REPORT'; -export function setReport(report) { - return { type: SET_REPORT, report }; +export function setReport(report, resourceId, creationMethod, filterField) { + return { type: SET_REPORT, report, resourceId, creationMethod, filterField }; } -export function fetchUISpecificReport( +export function fetchUISpecificReport({ userId, - filter_field, - creation_method, - dashboardId, -) { + filterField, + creationMethod, + resourceId, +}) { const queryParams = rison.encode({ filters: [ { - col: filter_field, + col: filterField, opr: 'eq', - value: dashboardId, + value: resourceId, }, { col: 'creation_method', opr: 'eq', - value: creation_method, + value: creationMethod, }, { col: 'created_by', @@ -59,7 +59,7 @@ export function fetchUISpecificReport( endpoint: `/api/v1/report/?q=${queryParams}`, }) .then(({ json }) => { - dispatch(setReport(json)); + dispatch(setReport(json, resourceId, creationMethod, filterField)); }) .catch(() => dispatch( @@ -78,22 +78,22 @@ const structureFetchAction = (dispatch, getState) => { const { user, dashboardInfo, charts, explore } = state; if (dashboardInfo) { dispatch( - fetchUISpecificReport( - user.userId, - 'dashboard_id', - 'dashboards', - dashboardInfo.id, - ), + fetchUISpecificReport({ + userId: user.userId, + filterField: 'dashboard_id', + creationMethod: 'dashboards', + resourceId: dashboardInfo.id, + }), ); } else { const [chartArr] = Object.keys(charts); dispatch( - fetchUISpecificReport( - explore.user.userId, - 'chart_id', - 'charts', - charts[chartArr].id, - ), + fetchUISpecificReport({ + userId: explore.user.userId, + filterField: 'chart_id', + creationMethod: 'charts', + resourceId: charts[chartArr].id, + }), ); } }; diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index 8b582d0d0cc1..1896250f5cde 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -17,32 +17,65 @@ * under the License. */ /* eslint-disable camelcase */ +// eslint-disable-next-line import/no-extraneous-dependencies import { SET_REPORT, ADD_REPORT, EDIT_REPORT } from '../actions/reports'; export default function reportsReducer(state = {}, action) { const actionHandlers = { [SET_REPORT]() { - return { - ...action.report.result.reduce( - (obj, report) => ({ ...obj, [report.id]: report }), - {}, - ), - }; + const { report, resourceId, creationMethod, filterField } = action; + // For now report count should only be one, but we are checking in case + // functionality changes. + const reportObject = report.result?.find( + report => report[filterField] === resourceId, + ); + + if (reportObject) { + return { + ...state, + [creationMethod]: { + ...state[creationMethod], + [resourceId]: reportObject, + }, + }; + } + if (state?.[creationMethod]?.[resourceId]) { + // remove the empty report from state + const newState = { ...state }; + delete newState[creationMethod][resourceId]; + return newState; + } + return { ...state }; }, + [ADD_REPORT]() { - const report = action.json.result; - report.id = action.json.id; + const { result, id } = action.json; + const report = { ...result, id }; + const reportTypeId = report.dashboard || report.chart; + // this is the id of either the chart or the dashboard associated with the report. + return { ...state, - [action.json.id]: report, + [report.creation_method]: { + ...state[report.creation_method], + [reportTypeId]: report, + }, }; }, + [EDIT_REPORT]() { - const report = action.json.result; - report.id = action.json.id; + const report = { + ...action.json.result, + id: action.json.id, + }; + const reportTypeId = report.dashboard || report.chart; + return { ...state, - [action.json.id]: report, + [report.creation_method]: { + ...state[report.creation_method], + [reportTypeId]: report, + }, }; }, }; diff --git a/superset-frontend/src/reports/types.ts b/superset-frontend/src/reports/types.ts index cde02544c00c..38cb3865cf84 100644 --- a/superset-frontend/src/reports/types.ts +++ b/superset-frontend/src/reports/types.ts @@ -24,3 +24,37 @@ export type ReportScheduleType = 'Alert' | 'Report'; export type ReportCreationMethod = 'charts' | 'dashboards' | 'alerts_reports'; export type ReportRecipientType = 'Email' | 'Slack'; + +export enum ReportType { + DASHBOARDS = 'dashboards', + CHARTS = 'charts', +} + +export enum NOTIFICATION_FORMATS { + TEXT = 'TEXT', + PNG = 'PNG', + CSV = 'CSV', +} +export interface ReportObject { + id?: number; + active: boolean; + crontab: string; + dashboard?: number; + chart?: number; + description?: string; + log_retention: number; + name: string; + owners: number[]; + recipients: [ + { recipient_config_json: { target: string }; type: ReportRecipientType }, + ]; + report_format: string; + timezone: string; + type: ReportScheduleType; + validator_config_json: {} | null; + validator_type: string; + working_timeout: number; + creation_method: string; + force_screenshot: boolean; + error?: string; +} diff --git a/superset-frontend/src/types/Owner.ts b/superset-frontend/src/types/Owner.ts index 8e7d63f25b92..b7548ec62900 100644 --- a/superset-frontend/src/types/Owner.ts +++ b/superset-frontend/src/types/Owner.ts @@ -26,4 +26,5 @@ export default interface Owner { id: number; last_name: string; username: string; + email?: string; } diff --git a/superset-frontend/src/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index ef320b5f7b9a..ec59bb5a16b4 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -18,6 +18,7 @@ */ import Owner from 'src/types/Owner'; +import { NOTIFICATION_FORMATS } from 'src/reports/types'; type user = { id: number; @@ -59,13 +60,16 @@ export type Operator = '<' | '>' | '<=' | '>=' | '==' | '!=' | 'not null'; export type AlertObject = { active?: boolean; + creation_method?: string; chart?: MetaObject; changed_by?: user; changed_on_delta_humanized?: string; + chart_id: number; created_by?: user; created_on?: string; crontab?: string; dashboard?: MetaObject; + dashboard_id?: number; database?: MetaObject; description?: string; force_screenshot: boolean; @@ -79,7 +83,7 @@ export type AlertObject = { sql?: string; timezone?: string; recipients?: Array; - report_format?: 'PNG' | 'CSV' | 'TEXT'; + report_format?: NOTIFICATION_FORMATS; type?: string; validator_config_json?: { op?: Operator; @@ -87,6 +91,7 @@ export type AlertObject = { }; validator_type?: string; working_timeout?: number; + error?: string; }; export type LogObject = { diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index ba544909cbea..18349b305c21 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -788,3 +788,14 @@ export function useDatabaseValidation() { return [validationErrors, getValidation, setValidationErrors] as const; } + +export const reportSelector = ( + state: Record, + resourceType: string, + resourceId?: number, +) => { + if (resourceId) { + return state.reports[resourceType]?.[resourceId]; + } + return {}; +}; diff --git a/superset/reports/api.py b/superset/reports/api.py index 645cd2489463..4694e228743b 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -125,12 +125,14 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "changed_by.last_name", "changed_on", "changed_on_delta_humanized", + "chart_id", "created_by.first_name", "created_by.last_name", "created_on", "creation_method", "crontab", "crontab_humanized", + "dashboard_id", "description", "id", "last_eval_dttm", diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 948fe519e1be..b111a19110fc 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -273,11 +273,13 @@ def test_get_list_report_schedule(self): "changed_by", "changed_on", "changed_on_delta_humanized", + "chart_id", "created_by", "created_on", "creation_method", "crontab", "crontab_humanized", + "dashboard_id", "description", "id", "last_eval_dttm",