From 3063caeac8d95624f93b060844f77fbf792a10e5 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Fri, 11 Mar 2022 14:15:52 -0500 Subject: [PATCH] Preset io ch28954 refactor reports (#19129) * pexdax refactor (#16333) * refactor progress (#16339) * fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson * code dry (#16358) * Fetch bug fixed (#16376) * continued refactoring (#16377) * pexdax refactor (#16333) * refactor progress (#16339) * fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson * code dry (#16358) * Fetch bug fixed (#16376) * continued refactoring (#16377) * refactor: Arash/new state report (#16987) * code dry (#16358) * pexdax refactor (#16333) * refactor progress (#16339) * fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson * Fetch bug fixed (#16376) * continued refactoring (#16377) * refactor(reports): Arash/refactor reports (#16855) * pexdax refactor (#16333) * refactor progress (#16339) * fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson * code dry (#16358) * Fetch bug fixed (#16376) * continued refactoring (#16377) * refactor: Reports - ReportModal (#16622) * refactoring progress * removed consoles * Working, but with 2 fetches * report pickup Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Co-authored-by: Elizabeth Thompson * refactor(reports): Arash/again refactor reports (#16872) * pexdax refactor (#16333) * refactor progress (#16339) * fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson * code dry (#16358) * Fetch bug fixed (#16376) * continued refactoring (#16377) * refactor: Reports - ReportModal (#16622) * refactoring progress * removed consoles * Working, but with 2 fetches * it is still not working Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Co-authored-by: Elizabeth Thompson * next changes Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Co-authored-by: Elizabeth Thompson * refactor: Reports code clean 10-29 (#17424) * Add delete functionality * Report schema restructure progress * Fix lint * Removed console.log * fix(Explore): Remove changes to the properties on cancel (#17184) * Remove on close * Fix lint * Add tests * fix(dashboard): don't show report modal for anonymous user (#17106) * Added sunburst echart * fix(dashboard):Hide reports modal for anonymous users * Address comments * Make prettier happy Co-authored-by: Mayur * fix(explore): Metric control breaks when saved metric deleted from dataset (#17503) * Add functionality is now working (#17578) * refactoring reports * ready for review * added testing * removed user reducer * elizabeth suggestions Co-authored-by: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Co-authored-by: Elizabeth Thompson Co-authored-by: Geido <60598000+geido@users.noreply.github.com> Co-authored-by: Mayur Co-authored-by: Mayur Co-authored-by: Kamil Gabryjelski --- .../HeaderReportDropdown/index.test.tsx | 167 +++++++++++++++ .../HeaderReportDropdown/index.tsx | 196 ++++++++++++++++++ .../ReportModal/ReportModal.test.tsx | 84 +++++++- .../src/components/ReportModal/index.tsx | 66 ++---- .../components/Header/Header.test.tsx | 175 ---------------- .../src/dashboard/components/Header/index.jsx | 7 +- .../src/dashboard/reducers/types.ts | 10 + .../src/dashboard/util/constants.ts | 5 + .../ExploreChartHeader.test.tsx | 4 +- .../components/ExploreChartHeader/index.jsx | 2 +- .../components/PropertiesModal/index.tsx | 2 +- .../src/reports/actions/reports.js | 6 +- .../src/reports/reducers/reports.js | 141 ++----------- superset-frontend/src/reports/types.ts | 5 + superset-frontend/src/types/Owner.ts | 1 + .../src/views/CRUD/alert/types.ts | 34 ++- superset-frontend/src/views/CRUD/hooks.ts | 11 + 17 files changed, 556 insertions(+), 360 deletions(-) create mode 100644 superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx create mode 100644 superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx 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..18c27c242143 --- /dev/null +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx @@ -0,0 +1,167 @@ +/** + * 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 'src/featureFlags'; +import { FeatureFlag } from '@superset-ui/core'; +import HeaderReportDropdown, { HeaderReportProps } from '.'; + +let isFeatureEnabledMock: jest.MockInstance; + +const createProps = () => ({ + toggleActive: jest.fn(), + deleteActiveReport: jest.fn(), + dashboardId: 1, +}); + +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: FeatureFlag) => featureFlag === 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', () => { + 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); + expect(screen.getByText('Edit Email Report')).toBeInTheDocument(); + }); + + 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('New 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..d8c76b609a28 --- /dev/null +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx @@ -0,0 +1,196 @@ +/** + * 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 } 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'; +import ReportModal from 'src/components/ReportModal'; +import { ChartState } from 'src/explore/types'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { fetchUISpecificReport } from 'src/reports/actions/reports'; +import { reportSelector } from 'src/views/CRUD/hooks'; +import { ReportType } from 'src/dashboard/util/constants'; + +const deleteColor = (theme: SupersetTheme) => css` + color: ${theme.colors.error.base}; +`; + +export interface HeaderReportProps { + toggleActive: (data: AlertObject, isActive: boolean) => void; + deleteActiveReport: (data: AlertObject) => void; + dashboardId?: number; + chart?: ChartState; +} + +export default function HeaderReportDropDown({ + toggleActive, + deleteActiveReport, + dashboardId, + chart, +}: HeaderReportProps) { + const dispatch = useDispatch(); + + const report = useSelector(state => { + const resourceType = dashboardId + ? ReportType.DASHBOARDS + : ReportType.CHARTS; + return reportSelector(state, resourceType, dashboardId || chart?.id); + }); + const user: UserWithPermissionsAndRoles = useSelector< + any, + UserWithPermissionsAndRoles + >(state => state.user || state.explore?.user); + 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) { + toggleActive(data, checked); + } + }; + + const handleReportDelete = (report: AlertObject) => { + deleteActiveReport(report); + setCurrentReportDeleting(null); + }; + + 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 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 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')} + + + ); + return ( + <> + {canAddReports() && ( + <> + setShowModal(false)} + userEmail={user.email} + dashboardId={dashboardId} + chart={chart} + /> + {report ? ( + <> + + triggerNode.closest('.action-button') + } + > + + + + + {currentReportDeleting && ( + { + if (currentReportDeleting) { + handleReportDelete(currentReportDeleting); + } + }} + onHide={() => setCurrentReportDeleting(null)} + open + title={t('Delete Report?')} + /> + )} + + ) : ( + setShowModal(true)} + > + + + )} + + )} + + ); +} diff --git a/superset-frontend/src/components/ReportModal/ReportModal.test.tsx b/superset-frontend/src/components/ReportModal/ReportModal.test.tsx index 44e3d0ef65c5..1656e220d67c 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 = { @@ -33,16 +39,13 @@ const defaultProps = { addReport: NOOP, onHide: NOOP, onReportAdd: NOOP, - show: true, + showModal: true, userId: 1, userEmail: 'test@test.com', dashboardId: 1, - creationMethod: 'charts_dashboards', - props: { - chart: { - sliceFormData: { - viz_type: 'table', - }, + chart: { + sliceFormData: { + viz_type: 'table', }, }, }; @@ -107,4 +110,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 c9f113a1519d..1ccba513e07b 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -20,7 +20,6 @@ import React, { useState, useEffect, useReducer, - FunctionComponent, useCallback, useMemo, } from 'react'; @@ -28,20 +27,17 @@ import { t, SupersetTheme } from '@superset-ui/core'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; import { useDispatch, useSelector } from 'react-redux'; 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, -} from 'src/reports/types'; +import { ReportCreationMethod, ReportType } from 'src/reports/types'; +import { ReportObject, NOTIFICATION_FORMATS } from 'src/views/CRUD/alert/types'; +import { reportSelector } from 'src/views/CRUD/hooks'; import { antDErrorAlertStyles, StyledModal, @@ -60,29 +56,6 @@ 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) => {}; @@ -105,12 +78,6 @@ const TEXT_BASED_VISUALIZATION_TYPES = [ 'paired_ttest', ]; -const NOTIFICATION_FORMATS = { - TEXT: 'TEXT', - PNG: 'PNG', - CSV: 'CSV', -}; - const INITIAL_STATE = { crontab: '0 12 * * 1', }; @@ -123,7 +90,7 @@ type ReportObjectState = Partial & { isSubmitting?: boolean; }; -const ReportModal: FunctionComponent = ({ +function ReportModal({ onReportAdd, onHide, show = false, @@ -132,8 +99,9 @@ const ReportModal: FunctionComponent = ({ userId, userEmail, creationMethod, - ...props -}) => { + dashboardName, + chartName, +}: ReportProps) { const vizType = chart?.sliceFormData?.viz_type; const isChart = !!chart; const isTextBasedChart = @@ -141,7 +109,7 @@ const ReportModal: FunctionComponent = ({ 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, @@ -172,18 +140,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 + ? ReportType.DASHBOARDS + : ReportType.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 @@ -369,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 52cb505be758..6099c8630c7d 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -34,7 +34,7 @@ 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'; @@ -553,7 +553,7 @@ class Header extends React.PureComponent { )} - )} 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/dashboard/util/constants.ts b/superset-frontend/src/dashboard/util/constants.ts index 640028eb4e94..ab2b2b8a3bcb 100644 --- a/superset-frontend/src/dashboard/util/constants.ts +++ b/superset-frontend/src/dashboard/util/constants.ts @@ -77,3 +77,8 @@ export enum DashboardStandaloneMode { HIDE_NAV_AND_TITLE = 2, REPORT = 3, } + +export enum ReportType { + DASHBOARDS = 'dashboards', + CHARTS = 'charts', +} diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 86ebb49ffc10..324a6965a453 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -24,7 +24,9 @@ import userEvent from '@testing-library/user-event'; import fetchMock from 'fetch-mock'; import ExploreHeader from '.'; -fetchMock.get('http://localhost/api/v1/chart/318', {}); +const chartEndpoint = 'glob:*api/v1/chart/*'; + +fetchMock.get(chartEndpoint, { json: 'foo' }); const createProps = () => ({ chart: { diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 497ee932cad3..7d8fcb1c498d 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -20,7 +20,6 @@ import React from 'react'; import { connect } from 'react-redux'; import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; -import Icons from 'src/components/Icons'; import { Tooltip } from 'src/components/Tooltip'; import { CategoricalColorNamespace, @@ -33,6 +32,7 @@ import { chartPropShape } from 'src/dashboard/util/propShapes'; import AlteredSliceTag from 'src/components/AlteredSliceTag'; import FaveStar from 'src/components/FaveStar'; import Button from 'src/components/Button'; +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'; 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 10e65d0c4331..ddd428c17c20 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -25,8 +25,8 @@ 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) { + return { type: SET_REPORT, report, resourceId, creationMethod }; } export const DELETE_REPORT = 'DELETE_REPORT'; @@ -64,7 +64,7 @@ export function fetchUISpecificReport({ endpoint: `/api/v1/report/?q=${queryParams}`, }) .then(({ json }) => { - dispatch(setReport(json)); + dispatch(setReport(json, resourceId, creationMethod)); }) .catch(() => dispatch( diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index fc8f5754a116..d011cb2dc603 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -18,153 +18,52 @@ */ /* eslint-disable camelcase */ // eslint-disable-next-line import/no-extraneous-dependencies -import { report } from 'process'; -import { - SET_REPORT, - ADD_REPORT, - EDIT_REPORT, - DELETE_REPORT, -} from '../actions/reports'; - -/* -- Report schema -- -reports: { - dashboards: { - [dashboardId]: {...reportObject} - }, - charts: { - [chartId]: {...reportObject} - }, -} -*/ +import { SET_REPORT, ADD_REPORT, EDIT_REPORT } from '../actions/reports'; export default function reportsReducer(state = {}, action) { const actionHandlers = { [SET_REPORT]() { - // Grabs the first report with a dashboard id that - // matches the parameter report's dashboard_id - const reportWithDashboard = action.report.result?.find( - report => !!report.dashboard_id, - ); - // Grabs the first report with a chart id that - // matches the parameter report's chart.id - const reportWithChart = action.report.result?.find( - report => !!report.chart?.id, - ); + const { report, resourceId, creationMethod } = action; + + const reportObject = report.result?.find(report => !!report[resourceId]); - // This organizes report by its type, dashboard or chart - // and indexes it by the dashboard/chart id - if (reportWithDashboard) { - return { - ...state, - dashboards: { - ...state.dashboards, - [reportWithDashboard.dashboard_id]: reportWithDashboard, - }, - }; - } - if (reportWithChart) { - return { - ...state, - charts: { - ...state.chart, - [reportWithChart.chart.id]: reportWithChart, - }, - }; - } return { ...state, + [creationMethod]: { + ...state[creationMethod], + [resourceId]: reportObject, + }, }; }, [ADD_REPORT]() { const { result, id } = action.json; const report = { ...result, id }; + const reportId = report.dashboard || report.chart; - if (result.dashboard) { - return { - ...state, - dashboards: { - ...state.dashboards, - [report.id]: report, - }, - }; - } - if (result.chart) { - return { - ...state, - charts: { - ...state.chart, - [report.id]: report, - }, - }; - } return { ...state, + [report.creation_method]: { + ...state[report.creation_method], + [reportId]: report, + }, }; }, [EDIT_REPORT]() { - // Grab first matching report by matching dashboard id - // FIX THESE, THEY'RE OBJECTS, NOT ARRAYS, NO FIND - const reportWithDashboard = action.json.result?.find( - report => !!report.dashboard_id, - ); - // Assign the report's id - reportWithDashboard.id = action.json.id; - - // Grab first matching report by matching chart id - const reportWithChart = action.json.result?.find( - report => !!report.chart.id, - ); - // Assign the report's id - reportWithChart.id = action.json.id; - - // This updates the report by its type, dashboard or chart - if (reportWithDashboard) { - return { - ...state, - dashboards: { - ...state.dashboards, - [reportWithDashboard.dashboard_id]: report, - }, - }; - } - return { - ...state, - charts: { - ...state.chart, - [reportWithChart.chart.id]: report, - }, + const report = { + ...action.json.result, + id: action.json.id, }; - }, - - [DELETE_REPORT]() { - // Grabs the first report with a dashboard id that - // matches the parameter report's dashboard_id - const reportWithDashboard = action.report.result?.find( - report => !!report.dashboard_id, - ); + const reportId = report.dashboard || report.chart; - // This deletes the report by its type, dashboard or chart - if (reportWithDashboard) { - return { - ...state, - dashboards: { - ...state.dashboards.filter(report => report.id !== action.reportId), - }, - }; - } return { ...state, - charts: { - ...state.charts.filter(chart => chart.id !== action.reportId), + [report.creation_method]: { + ...state[report.creation_method], + [reportId]: report, }, }; - - // state.users.filter(item => item.id !== action.payload) - // return { - // ...state.filter(report => report.id !== action.reportId), - // }; }, }; diff --git a/superset-frontend/src/reports/types.ts b/superset-frontend/src/reports/types.ts index cde02544c00c..ac635000f670 100644 --- a/superset-frontend/src/reports/types.ts +++ b/superset-frontend/src/reports/types.ts @@ -24,3 +24,8 @@ export type ReportScheduleType = 'Alert' | 'Report'; export type ReportCreationMethod = 'charts' | 'dashboards' | 'alerts_reports'; export type ReportRecipientType = 'Email' | 'Slack'; + +export enum ReportType { + DASHBOARDS = 'dashboards', + CHARTS = 'charts', +} 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 99a9c480abef..538ffdee3b85 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 { ReportRecipientType, ReportScheduleType } from 'src/reports/types'; type user = { id: number; @@ -59,6 +60,7 @@ export type Operator = '<' | '>' | '<=' | '>=' | '==' | '!=' | 'not null'; export type AlertObject = { active?: boolean; + creation_method?: string; chart?: MetaObject; changed_by?: user; changed_on_delta_humanized?: string; @@ -81,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; @@ -89,8 +91,38 @@ export type AlertObject = { }; validator_type?: string; working_timeout?: number; + error?: string; }; +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; +} + export type LogObject = { end_dttm: string; error_message: string; 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 {}; +};