From f1964972f23fa072dd4fb6cb5ca8eec9ce822a43 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Wed, 18 Aug 2021 15:58:10 -0400 Subject: [PATCH 1/8] pexdax refactor (#16333) --- .../HeaderReportActionsDropdown/index.tsx | 100 ++++++++++++------ .../src/dashboard/components/Header/index.jsx | 57 ++++------ .../explore/components/ExploreChartHeader.jsx | 32 +----- 3 files changed, 91 insertions(+), 98 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 2ffc6f28600e7..4d9ceb86fc5d3 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -26,6 +26,7 @@ import { Menu, NoAnimationDropdown } from 'src/common/components'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import DeleteModal from 'src/components/DeleteModal'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; const deleteColor = (theme: SupersetTheme) => css` color: ${theme.colors.error.base}; @@ -40,9 +41,15 @@ export default function HeaderReportActionsDropDown({ toggleActive: (data: AlertObject, checked: boolean) => void; deleteActiveReport: (data: AlertObject) => void; }) { - const reports = useSelector(state => state.reports); + const reports: Record = useSelector( + state => state.reports, + ); + const user: UserWithPermissionsAndRoles = useSelector< + any, + UserWithPermissionsAndRoles + >(state => state.user || state.explore?.user); const reportsIds = Object.keys(reports); - const report = reports[reportsIds[0]]; + const report: AlertObject = reports[reportsIds[0]]; const [ currentReportDeleting, setCurrentReportDeleting, @@ -60,6 +67,23 @@ export default function HeaderReportActionsDropDown({ setCurrentReportDeleting(null); }; + const canAddReports = () => { + if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { + return false; + } + if (!user) { + // 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 menu = () => ( @@ -82,36 +106,48 @@ export default function HeaderReportActionsDropDown({ ); - return isFeatureEnabled(FeatureFlag.ALERT_REPORTS) ? ( - <> - - triggerNode.closest('.action-button') - } + return canAddReports() ? ( + report ? ( + <> + + triggerNode.closest('.action-button') + } + > + + + + + {currentReportDeleting && ( + { + if (currentReportDeleting) { + handleReportDelete(currentReportDeleting); + } + }} + onHide={() => setCurrentReportDeleting(null)} + open + title={t('Delete Report?')} + /> + )} + + ) : ( + - - - - - {currentReportDeleting && ( - { - if (currentReportDeleting) { - handleReportDelete(currentReportDeleting); - } - }} - onHide={() => setCurrentReportDeleting(null)} - open - title={t('Delete Report?')} - /> - )} - + + + ) ) : null; } diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 27dc4e7842c71..eeb7ddc255e3c 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -164,7 +164,6 @@ class Header extends React.PureComponent { 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() { @@ -400,29 +399,6 @@ class Header extends React.PureComponent { this.setState({ showingReportModal: false }); } - renderReportModal() { - const attachedReportExists = !!Object.keys(this.props.reports).length; - return attachedReportExists ? ( - - ) : ( - <> - - - - - ); - } - canAddReports() { if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; @@ -472,7 +448,6 @@ class Header extends React.PureComponent { const userCanEdit = dashboardInfo.dash_edit_perm; const userCanShare = dashboardInfo.dash_share_perm; const userCanSaveAs = dashboardInfo.dash_save_perm; - const shouldShowReport = !editMode && this.canAddReports(); const refreshLimit = dashboardInfo.common.conf.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT; const refreshWarning = @@ -570,27 +545,31 @@ class Header extends React.PureComponent { )} )} - {editMode && ( + {editMode ? ( - )} - - {!editMode && userCanEdit && ( + ) : ( <> - - - + {userCanEdit && ( + + + + )} + )} - {shouldShowReport && this.renderReportModal()} {this.state.showingPropertiesModal && ( - ) : ( - <> - - - - - ); - } - canAddReports() { if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; @@ -287,7 +261,11 @@ export class ExploreChartHeader extends React.PureComponent { isRunning={chartStatus === 'loading'} status={CHART_STATUS_MAP[chartStatus]} /> - {this.canAddReports() && this.renderReportModal()} + Date: Thu, 19 Aug 2021 12:19:33 -0500 Subject: [PATCH 2/8] refactor progress (#16339) --- .../HeaderReportActionsDropdown/index.tsx | 97 +++++++++++-------- .../src/components/ReportModal/index.tsx | 19 ++-- .../src/dashboard/components/Header/index.jsx | 14 +-- .../src/reports/actions/reports.js | 4 +- 4 files changed, 70 insertions(+), 64 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 4d9ceb86fc5d3..bbc30c80a26a4 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -26,6 +26,7 @@ import { Menu, NoAnimationDropdown } from 'src/common/components'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import DeleteModal from 'src/components/DeleteModal'; +import ReportModal from 'src/components/ReportModal'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; const deleteColor = (theme: SupersetTheme) => css` @@ -33,13 +34,13 @@ const deleteColor = (theme: SupersetTheme) => css` `; export default function HeaderReportActionsDropDown({ - showReportModal, toggleActive, deleteActiveReport, + dashboardId, }: { - showReportModal: () => void; toggleActive: (data: AlertObject, checked: boolean) => void; deleteActiveReport: (data: AlertObject) => void; + dashboardId?: number; }) { const reports: Record = useSelector( state => state.reports, @@ -55,6 +56,7 @@ export default function HeaderReportActionsDropDown({ setCurrentReportDeleting, ] = useState(null); const theme = useTheme(); + const [showModal, setShowModal] = useState(false); const toggleActiveKey = async (data: AlertObject, checked: boolean) => { if (data?.id) { @@ -96,7 +98,9 @@ export default function HeaderReportActionsDropDown({ css={{ marginLeft: theme.gridUnit * 2 }} /> - {t('Edit email report')} + setShowModal(true)}> + {t('Edit email report')} + setCurrentReportDeleting(report)} css={deleteColor} @@ -106,48 +110,59 @@ export default function HeaderReportActionsDropDown({ ); - return canAddReports() ? ( - report ? ( + return ( + canAddReports() && ( <> - - triggerNode.closest('.action-button') - } - > - + setShowModal(false)} + userId={user.userId} + userEmail={user.email} + dashboardId={dashboardId} + /> + {report ? ( + <> + + triggerNode.closest('.action-button') + } + > + + + + + {currentReportDeleting && ( + { + if (currentReportDeleting) { + handleReportDelete(currentReportDeleting); + } + }} + onHide={() => setCurrentReportDeleting(null)} + open + title={t('Delete Report?')} + /> + )} + + ) : ( + setShowModal(true)} + > - - {currentReportDeleting && ( - { - if (currentReportDeleting) { - handleReportDelete(currentReportDeleting); - } - }} - onHide={() => setCurrentReportDeleting(null)} - open - title={t('Delete Report?')} - /> )} - ) : ( - - - ) - ) : null; + ); } diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index e24d1d756c63f..2bf0f6196ed07 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -97,7 +97,6 @@ interface ReportProps { userEmail: string; dashboardId?: number; chart?: ChartObject; - creationMethod: string; props: any; } @@ -169,10 +168,14 @@ const ReportModal: FunctionComponent = ({ onReportAdd, onHide, show = false, + dashboardId, + chart, + userId, + userEmail, ...props }) => { - const vizType = props.props.chart?.sliceFormData?.viz_type; - const isChart = !!props.props.chart; + const vizType = chart?.sliceFormData?.viz_type; + const isChart = !!chart; const defaultNotificationFormat = isChart && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) ? NOTIFICATION_FORMATS.TEXT @@ -211,19 +214,19 @@ const ReportModal: FunctionComponent = ({ // Create new Report const newReportValues: Partial = { crontab: currentReport?.crontab, - dashboard: props.props.dashboardId, - chart: props.props.chart?.id, + dashboard: dashboardId, + chart: chart?.id, description: currentReport?.description, name: currentReport?.name, - owners: [props.props.userId], + owners: [userId], recipients: [ { - recipient_config_json: { target: props.props.userEmail }, + recipient_config_json: { target: userEmail }, type: 'Email', }, ], type: 'Report', - creation_method: props.props.creationMethod, + creation_method: dashboardId ? 'dashboards' : 'charts', active: true, report_format: currentReport?.report_format || defaultNotificationFormat, timezone: currentReport?.timezone, diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index eeb7ddc255e3c..e3440026c4796 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -162,8 +162,6 @@ 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); } componentDidMount() { @@ -176,7 +174,6 @@ class Header extends React.PureComponent { 'dashboard_id', 'dashboards', dashboardInfo.id, - user.email, ); } } @@ -206,7 +203,6 @@ class Header extends React.PureComponent { 'dashboard_id', 'dashboards', nextProps.dashboardInfo.id, - user.email, ); } } @@ -391,14 +387,6 @@ class Header extends React.PureComponent { this.setState({ showingPropertiesModal: false }); } - showReportModal() { - this.setState({ showingReportModal: true }); - } - - hideReportModal() { - this.setState({ showingReportModal: false }); - } - canAddReports() { if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; @@ -564,9 +552,9 @@ class Header extends React.PureComponent { )} )} diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 7b3bc814ca0e8..28600df3f0af2 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -31,14 +31,14 @@ export function fetchUISpecificReport( userId, filter_field, creation_method, - dashboardId, + resourceId, ) { const queryParams = rison.encode({ filters: [ { col: filter_field, opr: 'eq', - value: dashboardId, + value: resourceId, }, { col: 'creation_method', From 171d254c99f702dcd2deaaa22732e85891cbcf3a Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Thu, 19 Aug 2021 13:23:42 -0400 Subject: [PATCH 3/8] fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson --- .../HeaderReportActionsDropdown/index.tsx | 4 +- .../components/Header/Header.test.tsx | 47 ++++++++++--------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index bbc30c80a26a4..026a262792e52 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -49,8 +49,8 @@ export default function HeaderReportActionsDropDown({ any, UserWithPermissionsAndRoles >(state => state.user || state.explore?.user); - const reportsIds = Object.keys(reports); - const report: AlertObject = reports[reportsIds[0]]; + const reportsIds = Object.keys(reports || []); + const report: AlertObject = reports?.[reportsIds[0]]; const [ currentReportDeleting, setCurrentReportDeleting, diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index 8a9ecdb5be46f..1ff6e4849085d 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -58,6 +58,7 @@ const createProps = () => ({ dashboardTitle: 'Dashboard Title', charts: {}, layout: {}, + reports: {}, expandedSlices: {}, css: '', customCss: '', @@ -134,23 +135,23 @@ async function openActionsDropdown() { test('should render', () => { const mockedProps = createProps(); - const { container } = render(setup(mockedProps)); + const { container } = render(setup(mockedProps), { useRedux: true }); expect(container).toBeInTheDocument(); }); test('should render the title', () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); expect(screen.getByText('Dashboard Title')).toBeInTheDocument(); }); test('should render the editable title', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); expect(screen.getByDisplayValue('Dashboard Title')).toBeInTheDocument(); }); test('should edit the title', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); const editableTitle = screen.getByDisplayValue('Dashboard Title'); expect(editableProps.onChange).not.toHaveBeenCalled(); userEvent.click(editableTitle); @@ -163,12 +164,12 @@ test('should edit the title', () => { test('should render the "Draft" status', () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); expect(screen.getByText('Draft')).toBeInTheDocument(); }); test('should publish', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); const draft = screen.getByText('Draft'); expect(editableProps.savePublished).not.toHaveBeenCalled(); userEvent.click(draft); @@ -176,12 +177,12 @@ test('should publish', () => { }); test('should render the "Undo" action as disabled', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); expect(screen.getByTitle('Undo').parentElement).toBeDisabled(); }); test('should undo', () => { - render(setup(undoProps)); + render(setup(undoProps), { useRedux: true }); const undo = screen.getByTitle('Undo'); expect(undoProps.onUndo).not.toHaveBeenCalled(); userEvent.click(undo); @@ -190,19 +191,19 @@ test('should undo', () => { test('should undo with key listener', () => { undoProps.onUndo.mockReset(); - render(setup(undoProps)); + render(setup(undoProps), { useRedux: true }); expect(undoProps.onUndo).not.toHaveBeenCalled(); fireEvent.keyDown(document.body, { key: 'z', code: 'KeyZ', ctrlKey: true }); expect(undoProps.onUndo).toHaveBeenCalledTimes(1); }); test('should render the "Redo" action as disabled', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); expect(screen.getByTitle('Redo').parentElement).toBeDisabled(); }); test('should redo', () => { - render(setup(redoProps)); + render(setup(redoProps), { useRedux: true }); const redo = screen.getByTitle('Redo'); expect(redoProps.onRedo).not.toHaveBeenCalled(); userEvent.click(redo); @@ -211,19 +212,19 @@ test('should redo', () => { test('should redo with key listener', () => { redoProps.onRedo.mockReset(); - render(setup(redoProps)); + render(setup(redoProps), { useRedux: true }); expect(redoProps.onRedo).not.toHaveBeenCalled(); fireEvent.keyDown(document.body, { key: 'y', code: 'KeyY', ctrlKey: true }); expect(redoProps.onRedo).toHaveBeenCalledTimes(1); }); test('should render the "Discard changes" button', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); expect(screen.getByText('Discard changes')).toBeInTheDocument(); }); test('should render the "Save" button as disabled', () => { - render(setup(editableProps)); + render(setup(editableProps), { useRedux: true }); expect(screen.getByText('Save').parentElement).toBeDisabled(); }); @@ -232,7 +233,7 @@ test('should save', () => { ...editableProps, hasUnsavedChanges: true, }; - render(setup(unsavedProps)); + render(setup(unsavedProps), { useRedux: true }); const save = screen.getByText('Save'); expect(unsavedProps.onSave).not.toHaveBeenCalled(); userEvent.click(save); @@ -245,13 +246,13 @@ test('should NOT render the "Draft" status', () => { ...mockedProps, isPublished: true, }; - render(setup(publishedProps)); + render(setup(publishedProps), { useRedux: true }); expect(screen.queryByText('Draft')).not.toBeInTheDocument(); }); test('should render the unselected fave icon', () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); expect(mockedProps.fetchFaveStar).toHaveBeenCalled(); expect( screen.getByRole('img', { name: 'favorite-unselected' }), @@ -264,7 +265,7 @@ test('should render the selected fave icon', () => { ...mockedProps, isStarred: true, }; - render(setup(favedProps)); + render(setup(favedProps), { useRedux: true }); expect( screen.getByRole('img', { name: 'favorite-selected' }), ).toBeInTheDocument(); @@ -276,7 +277,7 @@ test('should NOT render the fave icon on anonymous user', () => { ...mockedProps, user: undefined, }; - render(setup(anonymousUserProps)); + render(setup(anonymousUserProps), { useRedux: true }); expect(() => screen.getByRole('img', { name: 'favorite-unselected' }), ).toThrowError('Unable to find'); @@ -287,7 +288,7 @@ test('should NOT render the fave icon on anonymous user', () => { test('should fave', async () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); const fave = screen.getByRole('img', { name: 'favorite-unselected' }); expect(mockedProps.saveFaveStar).not.toHaveBeenCalled(); userEvent.click(fave); @@ -303,7 +304,7 @@ test('should toggle the edit mode', () => { dash_edit_perm: true, }, }; - render(setup(canEditProps)); + render(setup(canEditProps), { useRedux: true }); const editDashboard = screen.getByTitle('Edit dashboard'); expect(screen.queryByTitle('Edit dashboard')).toBeInTheDocument(); userEvent.click(editDashboard); @@ -312,13 +313,13 @@ test('should toggle the edit mode', () => { test('should render the dropdown icon', () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); expect(screen.getByRole('img', { name: 'more-horiz' })).toBeInTheDocument(); }); test('should refresh the charts', async () => { const mockedProps = createProps(); - render(setup(mockedProps)); + render(setup(mockedProps), { useRedux: true }); await openActionsDropdown(); userEvent.click(screen.getByText('Refresh dashboard')); expect(mockedProps.onRefresh).toHaveBeenCalledTimes(1); From 3d76011ca0be6fceba7192e28160e7ad76828195 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Fri, 20 Aug 2021 13:09:40 -0400 Subject: [PATCH 4/8] code dry (#16358) --- .../HeaderReportActionsDropdown/index.tsx | 24 +++++++- .../src/components/ReportModal/index.tsx | 22 ++------ .../src/dashboard/components/Header/index.jsx | 44 +-------------- .../components/DataTablesPane/index.tsx | 2 +- .../explore/components/ExploreChartHeader.jsx | 56 +------------------ .../src/reports/actions/reports.js | 12 ++-- 6 files changed, 34 insertions(+), 126 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 026a262792e52..cf2ae7f89abb1 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -16,18 +16,19 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useState } from 'react'; -import { useSelector } from 'react-redux'; +import React, { useState, useEffect } from 'react'; +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, NoAnimationDropdown } from 'src/common/components'; 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'; const deleteColor = (theme: SupersetTheme) => css` color: ${theme.colors.error.base}; @@ -37,11 +38,14 @@ export default function HeaderReportActionsDropDown({ toggleActive, deleteActiveReport, dashboardId, + chart, }: { toggleActive: (data: AlertObject, checked: boolean) => void; deleteActiveReport: (data: AlertObject) => void; dashboardId?: number; + chart?: ChartState; }) { + const dispatch = useDispatch(); const reports: Record = useSelector( state => state.reports, ); @@ -86,6 +90,19 @@ export default function HeaderReportActionsDropDown({ return permissions[0].length > 0; }; + useEffect(() => { + if (canAddReports()) { + dispatch( + fetchUISpecificReport({ + userId: user.userId, + filterField: dashboardId ? 'dashboard_id' : 'chart_id', + creationMethod: dashboardId ? 'dashboards' : 'charts', + resourceId: dashboardId || chart?.id, + }), + ); + } + }, []); + const menu = () => ( @@ -119,6 +136,7 @@ export default function HeaderReportActionsDropDown({ userId={user.userId} userEmail={user.email} dashboardId={dashboardId} + chart={chart} /> {report ? ( <> diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 2bf0f6196ed07..4f4b8c991d75a 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -36,6 +36,7 @@ import Icons from 'src/components/Icons'; import withToasts from 'src/messageToasts/enhancers/withToasts'; import { CronError } from 'src/components/CronPicker'; import { RadioChangeEvent } from 'src/common/components'; +import { ChartState } from 'src/explore/types'; import { StyledModal, StyledTopSection, @@ -72,20 +73,6 @@ export interface ReportObject { working_timeout: number; creation_method: string; } - -interface ChartObject { - id: number; - chartAlert: string; - chartStatus: string; - chartUpdateEndTime: number; - chartUpdateStartTime: number; - latestQueryFormData: object; - queryController: { abort: () => {} }; - queriesResponse: object; - triggerQuery: boolean; - lastRendered: number; -} - interface ReportProps { addDangerToast: (msg: string) => void; addSuccessToast: (msg: string) => void; @@ -96,7 +83,7 @@ interface ReportProps { userId: number; userEmail: string; dashboardId?: number; - chart?: ChartObject; + chart?: ChartState; props: any; } @@ -172,12 +159,11 @@ const ReportModal: FunctionComponent = ({ chart, userId, userEmail, - ...props }) => { const vizType = chart?.sliceFormData?.viz_type; const isChart = !!chart; const defaultNotificationFormat = - isChart && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) + vizType && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) ? NOTIFICATION_FORMATS.TEXT : NOTIFICATION_FORMATS.PNG; const [currentReport, setCurrentReport] = useReducer< @@ -287,7 +273,7 @@ const ReportModal: FunctionComponent = ({ }} value={currentReport?.report_format || defaultNotificationFormat} > - {TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) && ( + {vizType && TEXT_BASED_VISUALIZATION_TYPES.includes(vizType) && ( {t('Text embedded in email')} diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index e3440026c4796..9acbcb82afd8e 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -28,8 +28,6 @@ import { 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'; @@ -165,21 +163,11 @@ class Header extends React.PureComponent { } 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. - this.props.fetchUISpecificReport( - user.userId, - 'dashboard_id', - 'dashboards', - dashboardInfo.id, - ); - } } UNSAFE_componentWillReceiveProps(nextProps) { - const { user } = this.props; if ( UNDO_LIMIT - nextProps.undoLength <= 0 && !this.state.didNotifyMaxUndoHistoryToast @@ -193,18 +181,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, - ); - } } componentWillUnmount() { @@ -387,24 +363,6 @@ class Header extends React.PureComponent { this.setState({ showingPropertiesModal: false }); } - canAddReports() { - if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { - return false; - } - const { user } = this.props; - if (!user) { - // 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, diff --git a/superset-frontend/src/explore/components/DataTablesPane/index.tsx b/superset-frontend/src/explore/components/DataTablesPane/index.tsx index 98792c822a2cd..79f69060f099d 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/index.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/index.tsx @@ -204,7 +204,7 @@ export const DataTablesPane = ({ }, [queryFormData], ); - + console.log(queryFormData); useEffect(() => { setInLocalStorage(STORAGE_KEYS.isOpen, panelOpen); }, [panelOpen]); diff --git a/superset-frontend/src/explore/components/ExploreChartHeader.jsx b/superset-frontend/src/explore/components/ExploreChartHeader.jsx index 1f2bb10bcb013..77bf6c3dbc490 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader.jsx @@ -22,13 +22,11 @@ import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; import { styled, t } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; -import ReportModal from 'src/components/ReportModal'; import { fetchUISpecificReport, toggleActive, deleteActiveReport, } from 'src/reports/actions/reports'; -import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; import { chartPropShape } from '../../dashboard/util/propShapes'; import ExploreActionButtons from './ExploreActionButtons'; @@ -106,25 +104,9 @@ export class ExploreChartHeader extends React.PureComponent { super(props); this.state = { isPropertiesModalOpen: false, - showingReportModal: false, }; this.openPropertiesModal = this.openPropertiesModal.bind(this); this.closePropertiesModal = this.closePropertiesModal.bind(this); - this.showReportModal = this.showReportModal.bind(this); - this.hideReportModal = this.hideReportModal.bind(this); - } - - componentDidMount() { - 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, - ); - } } getSliceName() { @@ -153,32 +135,6 @@ export class ExploreChartHeader extends React.PureComponent { }); } - showReportModal() { - this.setState({ showingReportModal: true }); - } - - hideReportModal() { - this.setState({ showingReportModal: false }); - } - - canAddReports() { - if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { - return false; - } - const { user } = this.props; - if (!user) { - // 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 { user, form_data: formData } = this.props; const { @@ -262,20 +218,10 @@ export class ExploreChartHeader extends React.PureComponent { status={CHART_STATUS_MAP[chartStatus]} /> - Date: Fri, 20 Aug 2021 12:51:29 -0500 Subject: [PATCH 5/8] Fetch bug fixed (#16376) --- .../HeaderReportActionsDropdown/index.tsx | 1 + .../components/Header/Header.test.tsx | 1 - .../src/dashboard/components/Header/index.jsx | 1 - .../explore/components/ExploreChartHeader.jsx | 8 ++----- .../src/reports/actions/reports.js | 24 +++++++++---------- 5 files changed, 15 insertions(+), 20 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index cf2ae7f89abb1..6e42b07531b4c 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -55,6 +55,7 @@ export default function HeaderReportActionsDropDown({ >(state => state.user || state.explore?.user); const reportsIds = Object.keys(reports || []); const report: AlertObject = reports?.[reportsIds[0]]; + console.log(report); const [ currentReportDeleting, setCurrentReportDeleting, diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index 1ff6e4849085d..e59bfa9d57c9e 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -58,7 +58,6 @@ const createProps = () => ({ dashboardTitle: 'Dashboard Title', charts: {}, layout: {}, - reports: {}, expandedSlices: {}, css: '', customCss: '', diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 9acbcb82afd8e..3514e7dbfdc6e 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -73,7 +73,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, diff --git a/superset-frontend/src/explore/components/ExploreChartHeader.jsx b/superset-frontend/src/explore/components/ExploreChartHeader.jsx index 77bf6c3dbc490..6bb61c8d32ded 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader.jsx @@ -22,11 +22,7 @@ import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; import { styled, t } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; -import { - fetchUISpecificReport, - toggleActive, - deleteActiveReport, -} from 'src/reports/actions/reports'; +import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports'; import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; import { chartPropShape } from '../../dashboard/util/propShapes'; import ExploreActionButtons from './ExploreActionButtons'; @@ -243,7 +239,7 @@ ExploreChartHeader.propTypes = propTypes; function mapDispatchToProps(dispatch) { return bindActionCreators( - { sliceUpdated, fetchUISpecificReport, toggleActive, deleteActiveReport }, + { sliceUpdated, toggleActive, deleteActiveReport }, dispatch, ); } diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 03620ed542b72..17f5e4a5b0b76 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -76,22 +76,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, + }), ); } }; From 6878bf9938fdfe11532b6a7a32cd4deecca83289 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Fri, 20 Aug 2021 18:04:57 -0400 Subject: [PATCH 6/8] continued refactoring (#16377) --- .../HeaderReportActionsDropdown/index.tsx | 22 ++++++++++++++++--- .../components/DataTablesPane/index.tsx | 1 - 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 6e42b07531b4c..f9fdca066556e 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import { useSelector, useDispatch } from 'react-redux'; import { t, SupersetTheme, css, useTheme } from '@superset-ui/core'; import Icons from 'src/components/Icons'; @@ -55,14 +55,13 @@ export default function HeaderReportActionsDropDown({ >(state => state.user || state.explore?.user); const reportsIds = Object.keys(reports || []); const report: AlertObject = reports?.[reportsIds[0]]; - console.log(report); const [ currentReportDeleting, setCurrentReportDeleting, ] = useState(null); const theme = useTheme(); const [showModal, setShowModal] = useState(false); - + const dashboardIdRef = useRef(dashboardId); const toggleActiveKey = async (data: AlertObject, checked: boolean) => { if (data?.id) { toggleActive(data, checked); @@ -104,6 +103,23 @@ export default function HeaderReportActionsDropDown({ } }, []); + useEffect(() => { + if ( + canAddReports() && + dashboardId && + dashboardId !== dashboardIdRef.current + ) { + dispatch( + fetchUISpecificReport({ + userId: user.userId, + filterField: 'dashboard_id', + creationMethod: 'dashboards', + resourceId: dashboardId, + }), + ); + } + }, [dashboardId]); + const menu = () => ( diff --git a/superset-frontend/src/explore/components/DataTablesPane/index.tsx b/superset-frontend/src/explore/components/DataTablesPane/index.tsx index 79f69060f099d..a8c7c50dbf44f 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/index.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/index.tsx @@ -204,7 +204,6 @@ export const DataTablesPane = ({ }, [queryFormData], ); - console.log(queryFormData); useEffect(() => { setInLocalStorage(STORAGE_KEYS.isOpen, panelOpen); }, [panelOpen]); From 0f36532016c02fb13735ebf57233d33d6dd9b8a2 Mon Sep 17 00:00:00 2001 From: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Date: Thu, 23 Sep 2021 14:59:36 -0500 Subject: [PATCH 7/8] refactor: Reports - ReportModal (#16622) * refactoring progress * removed consoles * Working, but with 2 fetches --- .../HeaderReportActionsDropdown/index.tsx | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index f9fdca066556e..b81f225b5ac6e 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -92,6 +92,7 @@ export default function HeaderReportActionsDropDown({ useEffect(() => { if (canAddReports()) { + dashboardIdRef.current = dashboardId; dispatch( fetchUISpecificReport({ userId: user.userId, @@ -101,25 +102,27 @@ export default function HeaderReportActionsDropDown({ }), ); } - }, []); - - useEffect(() => { - if ( - canAddReports() && - dashboardId && - dashboardId !== dashboardIdRef.current - ) { - dispatch( - fetchUISpecificReport({ - userId: user.userId, - filterField: 'dashboard_id', - creationMethod: 'dashboards', - resourceId: dashboardId, - }), - ); - } }, [dashboardId]); + // (TODO: lyndsiWilliams): Leaving this in case we decide we need it after all + // useEffect(() => { + // if ( + // canAddReports() && + // dashboardId && + // dashboardId !== dashboardIdRef.current + // ) { + // dashboardIdRef.current = dashboardId; + // dispatch( + // fetchUISpecificReport({ + // userId: user.userId, + // filterField: 'dashboard_id', + // creationMethod: 'dashboards', + // resourceId: dashboardId, + // }), + // ); + // } + // }, [dashboardId]); + const menu = () => ( From f62a012033e4db11fcc78795cb2dfbd8fb586f4e Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 24 Sep 2021 17:56:26 -0400 Subject: [PATCH 8/8] report pickup --- .../HeaderReportActionsDropdown/index.tsx | 50 ++++++++----------- .../src/components/ReportModal/index.tsx | 18 +++---- .../src/dashboard/components/Header/index.jsx | 1 + superset/reports/api.py | 2 + 4 files changed, 30 insertions(+), 41 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index b81f225b5ac6e..723faf4c24650 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useState, useEffect, useRef } from 'react'; +import React, { useState, useEffect } from 'react'; import { useSelector, useDispatch } from 'react-redux'; import { t, SupersetTheme, css, useTheme } from '@superset-ui/core'; import Icons from 'src/components/Icons'; @@ -46,22 +46,26 @@ export default function HeaderReportActionsDropDown({ chart?: ChartState; }) { const dispatch = useDispatch(); - const reports: Record = useSelector( - state => state.reports, + const reports: any = useSelector(state => + Object.values(state.reports).filter((report: any) => + dashboardId + ? report.dashboard_id === dashboardId + : report.chart_id === chart?.id, + ), ); + console.log('this is reports with the filter', reports); + const report: AlertObject = Object.values(reports)[0]; + console.log('this is the individual report', report); const user: UserWithPermissionsAndRoles = useSelector< any, UserWithPermissionsAndRoles >(state => state.user || state.explore?.user); - const reportsIds = Object.keys(reports || []); - const report: AlertObject = reports?.[reportsIds[0]]; const [ currentReportDeleting, setCurrentReportDeleting, ] = useState(null); const theme = useTheme(); - const [showModal, setShowModal] = useState(false); - const dashboardIdRef = useRef(dashboardId); + const [showModal, setShowModal] = useState(false); const toggleActiveKey = async (data: AlertObject, checked: boolean) => { if (data?.id) { toggleActive(data, checked); @@ -92,7 +96,6 @@ export default function HeaderReportActionsDropDown({ useEffect(() => { if (canAddReports()) { - dashboardIdRef.current = dashboardId; dispatch( fetchUISpecificReport({ userId: user.userId, @@ -101,27 +104,12 @@ export default function HeaderReportActionsDropDown({ resourceId: dashboardId || chart?.id, }), ); + console.log('mounted', dashboardId); } - }, [dashboardId]); - - // (TODO: lyndsiWilliams): Leaving this in case we decide we need it after all - // useEffect(() => { - // if ( - // canAddReports() && - // dashboardId && - // dashboardId !== dashboardIdRef.current - // ) { - // dashboardIdRef.current = dashboardId; - // dispatch( - // fetchUISpecificReport({ - // userId: user.userId, - // filterField: 'dashboard_id', - // creationMethod: 'dashboards', - // resourceId: dashboardId, - // }), - // ); - // } - // }, [dashboardId]); + return () => { + console.log('unmounted', dashboardId); + }; + }, []); const menu = () => ( @@ -151,9 +139,9 @@ export default function HeaderReportActionsDropDown({ canAddReports() && ( <> setShowModal(false)} userId={user.userId} + showModal={showModal} + onHide={() => setShowModal(false)} userEmail={user.email} dashboardId={dashboardId} chart={chart} @@ -170,6 +158,7 @@ export default function HeaderReportActionsDropDown({ > + {dashboardId} {currentReportDeleting && ( @@ -198,6 +187,7 @@ export default function HeaderReportActionsDropDown({ onClick={() => setShowModal(true)} > + {dashboardId} )} diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 4f4b8c991d75a..041f0e31a42b3 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -25,8 +25,7 @@ import React, { FunctionComponent, } from 'react'; import { t, SupersetTheme } from '@superset-ui/core'; -import { bindActionCreators } from 'redux'; -import { connect, useDispatch, useSelector } from 'react-redux'; +import { useDispatch, useSelector } from 'react-redux'; import { addReport, editReport } from 'src/reports/actions/reports'; import { AlertObject } from 'src/views/CRUD/alert/types'; @@ -77,14 +76,14 @@ interface ReportProps { addDangerToast: (msg: string) => void; addSuccessToast: (msg: string) => void; addReport: (report?: ReportObject) => {}; - onHide: () => {}; + onHide: () => void; onReportAdd: (report?: ReportObject) => {}; - show: boolean; + showModal: boolean; userId: number; userEmail: string; dashboardId?: number; chart?: ChartState; - props: any; + props?: any; } interface ReportPayloadType { @@ -154,7 +153,7 @@ const reportReducer = ( const ReportModal: FunctionComponent = ({ onReportAdd, onHide, - show = false, + showModal = false, dashboardId, chart, userId, @@ -291,7 +290,7 @@ const ReportModal: FunctionComponent = ({ return ( = ({ ); }; -const mapDispatchToProps = (dispatch: any) => - bindActionCreators({ addReport, editReport }, dispatch); - -export default connect(null, mapDispatchToProps)(withToasts(ReportModal)); +export default withToasts(ReportModal); diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 3514e7dbfdc6e..8d62b89c7f8dd 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -509,6 +509,7 @@ class Header extends React.PureComponent { )} 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",