From 615dffd2bb92a297bc4421c2f839226c61d5d8e8 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 01/23] pexdax refactor (#16333) --- .../HeaderReportActionsDropdown/index.tsx | 106 ++++++++++++------ .../src/dashboard/components/Header/index.jsx | 57 +++------- .../components/ExploreChartHeader/index.jsx | 34 +----- 3 files changed, 96 insertions(+), 101 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 6fc39283600b1..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,11 +41,19 @@ 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 [currentReportDeleting, setCurrentReportDeleting] = - useState(null); + const report: AlertObject = reports[reportsIds[0]]; + const [ + currentReportDeleting, + setCurrentReportDeleting, + ] = useState(null); const theme = useTheme(); const toggleActiveKey = async (data: AlertObject, checked: boolean) => { @@ -58,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 = () => ( @@ -80,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 a67061832ddaf..516359a9c42f7 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -165,7 +165,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() { @@ -411,29 +410,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; @@ -488,7 +464,6 @@ class Header extends React.PureComponent { const userCanSaveAs = dashboardInfo.dash_save_perm && filterboxMigrationState !== FILTER_BOX_MIGRATION_STATES.REVIEWING; - const shouldShowReport = !editMode && this.canAddReports(); const refreshLimit = dashboardInfo.common?.conf?.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT; const refreshWarning = @@ -602,27 +577,31 @@ class Header extends React.PureComponent { )} )} - {editMode && ( + {editMode ? ( - )} - - {!editMode && userCanEdit && ( + ) : ( <> - - - + {userCanEdit && ( + + + + )} + )} - {shouldShowReport && this.renderReportModal()} { categoricalNamespace.setColor(label, labelColors[label]); @@ -207,30 +205,6 @@ export class ExploreChartHeader 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; @@ -348,7 +322,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 02/23] refactor progress (#16339) --- .../HeaderReportActionsDropdown/index.tsx | 97 +++++++++++-------- .../src/components/ReportModal/index.tsx | 19 ++-- .../src/dashboard/components/Header/index.jsx | 17 +--- .../src/reports/actions/reports.js | 4 +- 4 files changed, 74 insertions(+), 63 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 240d3176ccec9..79c2e196faeaf 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -98,7 +98,6 @@ interface ReportProps { userEmail: string; dashboardId?: number; chart?: ChartObject; - creationMethod: string; props: any; } @@ -182,10 +181,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 @@ -221,19 +224,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 516359a9c42f7..00feb09a0a917 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -163,8 +163,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() { @@ -178,7 +176,6 @@ class Header extends React.PureComponent { 'dashboard_id', 'dashboards', dashboardInfo.id, - user.email, ); } } @@ -215,8 +212,12 @@ class Header extends React.PureComponent { user?.userId, 'dashboard_id', 'dashboards', +<<<<<<< HEAD nextProps?.dashboardInfo?.id, user?.email, +======= + nextProps.dashboardInfo.id, +>>>>>>> refactor progress (#16339) ); } } @@ -402,14 +403,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; @@ -596,9 +589,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 8f23e283522df..26a16319f44fe 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -33,14 +33,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 6d89d8c77ce21eb70dea8ee85c52500929a8f519 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 03/23] fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson --- .../ReportModal/HeaderReportActionsDropdown/index.tsx | 4 ++-- .../src/dashboard/components/Header/Header.test.tsx | 1 + 2 files changed, 3 insertions(+), 2 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 ea94aceead179..1a7ae856f95c6 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -57,6 +57,7 @@ const createProps = () => ({ dashboardTitle: 'Dashboard Title', charts: {}, layout: {}, + reports: {}, expandedSlices: {}, css: '', customCss: '', From 5a35f4c7681a2d1f7085de2dc1b1a6435f3fcfbf 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 04/23] code dry (#16358) --- .../HeaderReportActionsDropdown/index.tsx | 24 +++++++-- .../src/components/ReportModal/index.tsx | 22 ++------ .../src/dashboard/components/Header/index.jsx | 25 ++------- .../components/DataTablesPane/index.tsx | 2 +- .../components/ExploreChartHeader/index.jsx | 53 +------------------ .../src/reports/actions/reports.js | 12 ++--- 6 files changed, 37 insertions(+), 101 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 79c2e196faeaf..8362f24534f5f 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -37,6 +37,7 @@ import Icons from 'src/components/Icons'; import withToasts from 'src/components/MessageToasts/withToasts'; import { CronError } from 'src/components/CronPicker'; import { RadioChangeEvent } from 'src/common/components'; +import { ChartState } from 'src/explore/types'; import { StyledModal, StyledTopSection, @@ -75,20 +76,6 @@ export interface ReportObject { force_screenshot: boolean; error?: 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 { addReport: (report?: ReportObject) => {}; onHide: () => {}; @@ -97,7 +84,7 @@ interface ReportProps { userId: number; userEmail: string; dashboardId?: number; - chart?: ChartObject; + chart?: ChartState; props: any; } @@ -185,12 +172,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< @@ -302,7 +288,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 00feb09a0a917..61d757019d35c 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'; @@ -166,7 +164,7 @@ 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. @@ -189,7 +187,6 @@ class Header extends React.PureComponent { } UNSAFE_componentWillReceiveProps(nextProps) { - const { user } = this.props; if ( UNDO_LIMIT - nextProps.undoLength <= 0 && !this.state.didNotifyMaxUndoHistoryToast @@ -203,23 +200,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', -<<<<<<< HEAD - nextProps?.dashboardInfo?.id, - user?.email, -======= - nextProps.dashboardInfo.id, ->>>>>>> refactor progress (#16339) - ); - } } componentWillUnmount() { @@ -403,6 +383,7 @@ class Header extends React.PureComponent { this.setState({ showingPropertiesModal: false }); } +<<<<<<< HEAD canAddReports() { if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; @@ -421,6 +402,8 @@ class Header extends React.PureComponent { return permissions[0].length > 0; } +======= +>>>>>>> code dry (#16358) render() { const { dashboardTitle, diff --git a/superset-frontend/src/explore/components/DataTablesPane/index.tsx b/superset-frontend/src/explore/components/DataTablesPane/index.tsx index d6cfcc257e24b..1b0aa9c0db11a 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/index.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/index.tsx @@ -295,7 +295,7 @@ export const DataTablesPane = ({ }, [queryFormData, columnNames], ); - + console.log(queryFormData); useEffect(() => { setItem(LocalStorageKeys.is_datapanel_open, panelOpen); }, [panelOpen]); diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index c1bb3c0da03d6..8458befdca18d 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -28,13 +28,11 @@ import { 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 'src/dashboard/util/propShapes'; import EditableTitle from 'src/components/EditableTitle'; @@ -113,27 +111,14 @@ 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); this.fetchChartDashboardData = this.fetchChartDashboardData.bind(this); } 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(); } @@ -197,32 +182,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?.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 { user, form_data: formData, slice } = this.props; const { @@ -323,20 +282,10 @@ export class ExploreChartHeader extends React.PureComponent { status={CHART_STATUS_MAP[chartStatus]} /> - Date: Fri, 20 Aug 2021 12:51:29 -0500 Subject: [PATCH 05/23] Fetch bug fixed (#16376) --- .../HeaderReportActionsDropdown/index.tsx | 1 + .../components/Header/Header.test.tsx | 1 - .../src/dashboard/components/Header/index.jsx | 1 - .../components/ExploreChartHeader/index.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 1a7ae856f95c6..ea94aceead179 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -57,7 +57,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 61d757019d35c..2137d905ccbc9 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -74,7 +74,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/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 8458befdca18d..8c45838eb655c 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -28,11 +28,7 @@ import { 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 'src/dashboard/util/propShapes'; import EditableTitle from 'src/components/EditableTitle'; @@ -307,7 +303,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 cdf9102ee468d..669c7201927d8 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -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, + }), ); } }; From 80c936ec218f2b3d1eef139c7108747ff5a5f4ef 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 06/23] 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 1b0aa9c0db11a..2712135b4b47b 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/index.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/index.tsx @@ -295,7 +295,6 @@ export const DataTablesPane = ({ }, [queryFormData, columnNames], ); - console.log(queryFormData); useEffect(() => { setItem(LocalStorageKeys.is_datapanel_open, panelOpen); }, [panelOpen]); From 18a04cd785d2275325bc29c2528cb580e70b80a8 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 07/23] pexdax refactor (#16333) --- .../HeaderReportActionsDropdown/index.tsx | 2 +- .../src/dashboard/components/Header/index.jsx | 60 +++++++++---------- .../components/ExploreChartHeader/index.jsx | 2 + 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index f9fdca066556e..aa0372791ce84 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -27,8 +27,8 @@ 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 { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; const deleteColor = (theme: SupersetTheme) => css` color: ${theme.colors.error.base}; diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 2137d905ccbc9..a1975ed659288 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -160,6 +160,8 @@ 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() { @@ -382,27 +384,6 @@ class Header extends React.PureComponent { this.setState({ showingPropertiesModal: false }); } -<<<<<<< HEAD - 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; - } - -======= ->>>>>>> code dry (#16358) render() { const { dashboardTitle, @@ -578,16 +559,33 @@ class Header extends React.PureComponent { )} - + {this.state.showingPropertiesModal && ( + { + const { + dashboardInfoChanged, + dashboardTitleChanged, + } = this.props; + dashboardInfoChanged({ + slug: updates.slug, + metadata: JSON.parse(updates.jsonMetadata), + }); + setColorSchemeAndUnsavedChanges(updates.colorScheme); + dashboardTitleChanged(updates.title); + if (updates.slug) { + window.history.pushState( + { event: 'dashboard_properties_changed' }, + '', + `/superset/dashboard/${updates.slug}/`, + ); + } + }} + /> + )} {this.state.showingReportModal && ( Date: Thu, 19 Aug 2021 12:19:33 -0500 Subject: [PATCH 08/23] refactor progress (#16339) --- superset-frontend/src/dashboard/components/Header/index.jsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index a1975ed659288..3d9eaf8f2eb71 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -160,8 +160,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() { From 3c8fda44371ce2b2ff32833a19e7c842aa99a08c 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 09/23] fix: Header Actions test refactor (#16336) * fixed tests * Update index.tsx Co-authored-by: Elizabeth Thompson --- .../src/dashboard/components/Header/Header.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index ea94aceead179..1a7ae856f95c6 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -57,6 +57,7 @@ const createProps = () => ({ dashboardTitle: 'Dashboard Title', charts: {}, layout: {}, + reports: {}, expandedSlices: {}, css: '', customCss: '', From 39f6f8a70651e5f0b1cc683c48a795bd3d11bcfe 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 10/23] code dry (#16358) --- .../ReportModal/HeaderReportActionsDropdown/index.tsx | 2 +- .../src/dashboard/components/Header/index.jsx | 11 ----------- .../explore/components/ExploreChartHeader/index.jsx | 7 +++++-- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index aa0372791ce84..f9fdca066556e 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -27,8 +27,8 @@ 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 { fetchUISpecificReport } from 'src/reports/actions/reports'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { fetchUISpecificReport } from 'src/reports/actions/reports'; const deleteColor = (theme: SupersetTheme) => css` color: ${theme.colors.error.base}; diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 3d9eaf8f2eb71..64f353d70359c 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -165,17 +165,6 @@ class Header extends React.PureComponent { componentDidMount() { 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, - ); - } - } } componentDidUpdate(prevProps) { diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 685b693a2efc1..f77a41ba09060 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -22,7 +22,6 @@ import { bindActionCreators } from 'redux'; import PropTypes from 'prop-types'; import Icons from 'src/components/Icons'; import { Tooltip } from 'src/components/Tooltip'; -import ReportModal from 'src/components/ReportModal'; import { CategoricalColorNamespace, SupersetClient, @@ -30,7 +29,11 @@ import { t, } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; -import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports'; +import { + fetchUISpecificReport, + toggleActive, + deleteActiveReport, +} from 'src/reports/actions/reports'; import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import EditableTitle from 'src/components/EditableTitle'; From 9ee469da24927c8722be5c1079c57e066dec2b99 Mon Sep 17 00:00:00 2001 From: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Date: Fri, 20 Aug 2021 12:51:29 -0500 Subject: [PATCH 11/23] Fetch bug fixed (#16376) --- .../ReportModal/HeaderReportActionsDropdown/index.tsx | 1 + .../src/dashboard/components/Header/Header.test.tsx | 1 - .../src/explore/components/ExploreChartHeader/index.jsx | 6 +----- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index f9fdca066556e..5c467985b8014 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 1a7ae856f95c6..ea94aceead179 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -57,7 +57,6 @@ const createProps = () => ({ dashboardTitle: 'Dashboard Title', charts: {}, layout: {}, - reports: {}, expandedSlices: {}, css: '', customCss: '', diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index f77a41ba09060..124463581b701 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -29,11 +29,7 @@ import { 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 'src/dashboard/util/propShapes'; import EditableTitle from 'src/components/EditableTitle'; From 4387935d6f2366b8957a7dc3d6830727183ccf87 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 12/23] continued refactoring (#16377) --- .../components/ReportModal/HeaderReportActionsDropdown/index.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 5c467985b8014..f9fdca066556e 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -55,7 +55,6 @@ 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, From 82004da00cdcb47405ba56b38bff73bd24aacfa5 Mon Sep 17 00:00:00 2001 From: AAfghahi <48933336+AAfghahi@users.noreply.github.com> Date: Thu, 28 Oct 2021 16:39:14 -0400 Subject: [PATCH 13/23] 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 --- .../HeaderReportActionsDropdown/index.tsx | 32 +++++++++---------- .../src/components/ReportModal/index.tsx | 18 ++++------- .../src/dashboard/components/Header/index.jsx | 8 +---- .../src/reports/reducers/reports.js | 3 ++ .../src/views/CRUD/alert/types.ts | 2 ++ superset/reports/api.py | 2 ++ 6 files changed, 31 insertions(+), 34 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index f9fdca066556e..5eb4448e5b4a5 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'; @@ -49,19 +49,23 @@ export default function HeaderReportActionsDropDown({ const reports: Record = useSelector( state => state.reports, ); + const report: AlertObject = Object.values(reports).filter(report => { + if (dashboardId) { + return report.dashboard_id === dashboardId; + } + return report.chart_id === chart?.id; + })[0]; + 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); @@ -104,17 +108,13 @@ export default function HeaderReportActionsDropDown({ }, []); useEffect(() => { - if ( - canAddReports() && - dashboardId && - dashboardId !== dashboardIdRef.current - ) { + if (canAddReports()) { dispatch( fetchUISpecificReport({ userId: user.userId, - filterField: 'dashboard_id', - creationMethod: 'dashboards', - resourceId: dashboardId, + filterField: dashboardId ? 'dashboard_id' : 'chart_id', + creationMethod: dashboardId ? 'dashboards' : 'charts', + resourceId: dashboardId || chart?.id, }), ); } @@ -148,14 +148,14 @@ export default function HeaderReportActionsDropDown({ canAddReports() && ( <> setShowModal(false)} userId={user.userId} + showModal={showModal} + onHide={() => setShowModal(false)} userEmail={user.email} dashboardId={dashboardId} chart={chart} /> - {report ? ( + {reports ? ( <> {}; - onHide: () => {}; + onHide: () => void; onReportAdd: (report?: ReportObject) => {}; - show: boolean; + showModal: boolean; userId: number; userEmail: string; dashboardId?: number; chart?: ChartState; - props: any; + props?: any; } interface ReportPayloadType { @@ -167,7 +166,7 @@ const reportReducer = ( const ReportModal: FunctionComponent = ({ onReportAdd, onHide, - show = false, + showModal = false, dashboardId, chart, userId, @@ -306,7 +305,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 64f353d70359c..57e61d1e0cb50 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -167,13 +167,6 @@ class Header extends React.PureComponent { this.startPeriodicRender(refreshFrequency * 1000); } - componentDidUpdate(prevProps) { - if (this.props.refreshFrequency !== prevProps.refreshFrequency) { - const { refreshFrequency } = this.props; - this.startPeriodicRender(refreshFrequency * 1000); - } - } - UNSAFE_componentWillReceiveProps(nextProps) { if ( UNDO_LIMIT - nextProps.undoLength <= 0 && @@ -539,6 +532,7 @@ class Header extends React.PureComponent { )} ({ ...obj, [report.id]: report }), {}, diff --git a/superset-frontend/src/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index ef320b5f7b9a9..99a9c480abef4 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -62,10 +62,12 @@ export type AlertObject = { 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; diff --git a/superset/reports/api.py b/superset/reports/api.py index 33559312e02e0..a3fe27eaff564 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", From d8f4733fd2b81d96aa488d689d9d8aa5991acbf2 Mon Sep 17 00:00:00 2001 From: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Date: Fri, 19 Nov 2021 11:31:20 -0600 Subject: [PATCH 14/23] refactor: Reports code clean 10-29 (#17424) * Add delete functionality * Report schema restructure progress * Fix lint * Removed console.log --- .../HeaderReportActionsDropdown/index.tsx | 2 +- .../src/reports/actions/reports.js | 7 +- .../src/reports/reducers/reports.js | 145 ++++++++++++++++-- 3 files changed, 140 insertions(+), 14 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index 5eb4448e5b4a5..c85262f6c6126 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -155,7 +155,7 @@ export default function HeaderReportActionsDropDown({ dashboardId={dashboardId} chart={chart} /> - {reports ? ( + {report ? ( <> { - dispatch(structureFetchAction); + dispatch(deleteReport(report.id)); dispatch(addSuccessToast(t('Deleted: %s', report.name))); }); }; diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index 54cf493fd5cea..de23f572a94fc 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -17,36 +17,157 @@ * under the License. */ /* eslint-disable camelcase */ -import { SET_REPORT, ADD_REPORT, EDIT_REPORT } from '../actions/reports'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { report } from 'process'; +// import { allowCrossDomain } from 'src/utils/hostNamesConfig'; +import { + SET_REPORT, + ADD_REPORT, + EDIT_REPORT, + DELETE_REPORT, +} from '../actions/reports'; -// Talk about the delete +/* -- Report schema -- +reports: { + dashboards: { + [dashboardId]: {...reportObject} + }, + charts: { + [chartId]: {...reportObject} + }, +} +*/ 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, + ); + + // 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, + }, + }; + } return { ...state, - ...action.report.result.reduce( - (obj, report) => ({ ...obj, [report.id]: report }), - {}, - ), + charts: { + ...state.chart, + [reportWithChart.chart.id]: reportWithChart, + }, }; }, + [ADD_REPORT]() { - const report = action.json.result; - report.id = action.json.id; + // Grab first matching report by matching dashboard id + 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 adds the report by its type, dashboard or chart + if (reportWithDashboard) { + return { + ...state, + dashboards: { + ...state.dashboards, + [reportWithDashboard.dashboard_id]: report, + }, + }; + } return { ...state, - [action.json.id]: report, + charts: { + ...state.chart, + [reportWithChart.chart.id]: report, + }, }; }, + [EDIT_REPORT]() { - const report = action.json.result; - report.id = action.json.id; + // Grab first matching report by matching dashboard id + 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, + }, + }; + }, + + [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, + ); + + // 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, - [action.json.id]: report, + charts: { + ...state.charts.filter(chart => chart.id !== action.reportId), + }, }; + + // state.users.filter(item => item.id !== action.payload) + // return { + // ...state.filter(report => report.id !== action.reportId), + // }; }, }; From a17d5ca6453e1f5557e3595a9d25a680f6f0e801 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Tue, 2 Nov 2021 19:27:56 +0200 Subject: [PATCH 15/23] fix(Explore): Remove changes to the properties on cancel (#17184) * Remove on close * Fix lint * Add tests --- .../ExploreChartHeader/ExploreChartHeader.test.tsx | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 2033d3368e175..702498d39bc38 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -21,8 +21,12 @@ import React from 'react'; import { Slice } from 'src/types/Chart'; import { render, screen } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; + +import fetchMock from 'fetch-mock'; import ExploreHeader from '.'; +fetchMock.get('http://localhost/api/v1/chart/318', {}); + const createProps = () => ({ chart: { latestQueryFormData: { @@ -45,7 +49,11 @@ const createProps = () => ({ }, chartStatus: 'rendered', }, +<<<<<<< HEAD slice: { +======= + slice: ({ +>>>>>>> fix(Explore): Remove changes to the properties on cancel (#17184) cache_timeout: null, changed_on: '2021-03-19T16:30:56.750230', changed_on_humanized: '7 days ago', @@ -82,7 +90,11 @@ const createProps = () => ({ slice_id: 318, slice_name: 'Age distribution of respondents', slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20318%7D', +<<<<<<< HEAD } as unknown as Slice, +======= + } as unknown) as Slice, +>>>>>>> fix(Explore): Remove changes to the properties on cancel (#17184) slice_name: 'Age distribution of respondents', actions: { postChartFormData: () => null, From 98bb7f9f4d36d44de73b604b9669b9b529cfaa13 Mon Sep 17 00:00:00 2001 From: Mayur Date: Thu, 11 Nov 2021 16:20:12 +0530 Subject: [PATCH 16/23] 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 --- .../ReportModal/HeaderReportActionsDropdown/index.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index c85262f6c6126..f558010ecd9fd 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -60,10 +60,8 @@ export default function HeaderReportActionsDropDown({ any, UserWithPermissionsAndRoles >(state => state.user || state.explore?.user); - const [ - currentReportDeleting, - setCurrentReportDeleting, - ] = useState(null); + const [currentReportDeleting, setCurrentReportDeleting] = + useState(null); const theme = useTheme(); const [showModal, setShowModal] = useState(false); const toggleActiveKey = async (data: AlertObject, checked: boolean) => { @@ -81,7 +79,7 @@ export default function HeaderReportActionsDropDown({ if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; } - if (!user) { + if (!user?.userId) { // this is in the case that there is an anonymous user. return false; } From 29081699d61fa358d01f4db5074fe900aea2f9fc Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 24 Nov 2021 13:06:11 +0100 Subject: [PATCH 17/23] fix(explore): Metric control breaks when saved metric deleted from dataset (#17503) --- .../HeaderReportActionsDropdown/index.tsx | 20 ++++++---- .../src/components/ReportModal/index.tsx | 4 ++ .../src/dashboard/components/Header/index.jsx | 7 ++++ .../components/ExploreChartHeader/index.jsx | 39 ++++++++----------- .../src/reports/reducers/reports.js | 20 ++++++---- 5 files changed, 53 insertions(+), 37 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index f558010ecd9fd..b9de984bbd70f 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -46,15 +46,21 @@ export default function HeaderReportActionsDropDown({ chart?: ChartState; }) { const dispatch = useDispatch(); - const reports: Record = useSelector( - state => state.reports, - ); - const report: AlertObject = Object.values(reports).filter(report => { + const report: AlertObject = useSelector(state => { if (dashboardId) { - return report.dashboard_id === dashboardId; + return state.reports.dashboards?.[dashboardId]; + } + if (chart?.id) { + return state.reports.charts?.[chart.id]; } - return report.chart_id === chart?.id; - })[0]; + return {}; + }); + // const report: ReportObject = Object.values(reports).filter(report => { + // if (dashboardId) { + // return report.dashboards?.[dashboardId]; + // } + // // return report.charts?.[chart?.id] + // })[0]; const user: UserWithPermissionsAndRoles = useSelector< any, diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 33feb118149bb..79f471cdb7016 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -84,7 +84,11 @@ interface ReportProps { userEmail: string; dashboardId?: number; chart?: ChartState; +<<<<<<< HEAD props?: any; +======= + props: any; +>>>>>>> be2e1ecf6... code dry (#16358) } interface ReportPayloadType { diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 57e61d1e0cb50..a1cdf01320594 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -167,6 +167,13 @@ class Header extends React.PureComponent { this.startPeriodicRender(refreshFrequency * 1000); } + componentDidUpdate(prevProps) { + if (this.props.refreshFrequency !== prevProps.refreshFrequency) { + const { refreshFrequency } = this.props; + this.startPeriodicRender(refreshFrequency * 1000); + } + } + UNSAFE_componentWillReceiveProps(nextProps) { if ( UNDO_LIMIT - nextProps.undoLength <= 0 && diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 124463581b701..3e5aa4d9c0f37 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -123,31 +123,26 @@ export class ExploreChartHeader extends React.PureComponent { async fetchChartDashboardData() { const { dashboardId, slice } = this.props; - await SupersetClient.get({ + const response = await SupersetClient.get({ endpoint: `/api/v1/chart/${slice.slice_id}`, - }) - .then(res => { - const response = res?.json?.result; - if (response && response.dashboards && response.dashboards.length) { - const { dashboards } = response; - const dashboard = - dashboardId && - dashboards.length && - dashboards.find(d => d.id === dashboardId); + }); + const chart = response.json.result; + const dashboards = chart.dashboards || []; + const dashboard = + dashboardId && + dashboards.length && + dashboards.find(d => d.id === dashboardId); - if (dashboard && dashboard.json_metadata) { - // setting the chart to use the dashboard custom label colors if any - const labelColors = - JSON.parse(dashboard.json_metadata).label_colors || {}; - const categoricalNamespace = CategoricalColorNamespace.getNamespace(); + if (dashboard && dashboard.json_metadata) { + // setting the chart to use the dashboard custom label colors if any + const labelColors = + JSON.parse(dashboard.json_metadata).label_colors || {}; + const categoricalNamespace = CategoricalColorNamespace.getNamespace(); - Object.keys(labelColors).forEach(label => { - categoricalNamespace.setColor(label, labelColors[label]); - }); - } - } - }) - .catch(() => {}); + Object.keys(labelColors).forEach(label => { + categoricalNamespace.setColor(label, labelColors[label]); + }); + } } getSliceName() { diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index de23f572a94fc..a18d72e92c8f0 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -43,14 +43,13 @@ export default function reportsReducer(state = {}, action) { [SET_REPORT]() { // Grabs the first report with a dashboard id that // matches the parameter report's dashboard_id - const reportWithDashboard = action.report.result.find( + 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 reportWithChart = action.report.result?.find( + report => !!report.chart?.id, ); // This organizes report by its type, dashboard or chart @@ -64,12 +63,17 @@ export default function reportsReducer(state = {}, action) { }, }; } + if (reportWithChart) { + return { + ...state, + charts: { + ...state.chart, + [reportWithChart.chart.id]: reportWithChart, + }, + }; + } return { ...state, - charts: { - ...state.chart, - [reportWithChart.chart.id]: reportWithChart, - }, }; }, From 20e54c08c35e976a0c856a2f33d682ca7443e608 Mon Sep 17 00:00:00 2001 From: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Date: Tue, 7 Dec 2021 15:17:47 -0600 Subject: [PATCH 18/23] Add functionality is now working (#17578) --- .../src/reports/reducers/reports.js | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index a18d72e92c8f0..fc8f5754a116a 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -19,7 +19,6 @@ /* eslint-disable camelcase */ // eslint-disable-next-line import/no-extraneous-dependencies import { report } from 'process'; -// import { allowCrossDomain } from 'src/utils/hostNamesConfig'; import { SET_REPORT, ADD_REPORT, @@ -78,49 +77,43 @@ export default function reportsReducer(state = {}, action) { }, [ADD_REPORT]() { - // Grab first matching report by matching dashboard id - const reportWithDashboard = action.json.result.find( - report => !!report.dashboard_id, - ); - // Assign the report's id - reportWithDashboard.id = action.json.id; + const { result, id } = action.json; + const report = { ...result, 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 adds the report by its type, dashboard or chart - if (reportWithDashboard) { + if (result.dashboard) { return { ...state, dashboards: { ...state.dashboards, - [reportWithDashboard.dashboard_id]: report, + [report.id]: report, + }, + }; + } + if (result.chart) { + return { + ...state, + charts: { + ...state.chart, + [report.id]: report, }, }; } return { ...state, - charts: { - ...state.chart, - [reportWithChart.chart.id]: report, - }, }; }, [EDIT_REPORT]() { // Grab first matching report by matching dashboard id - const reportWithDashboard = action.json.result.find( + // 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( + const reportWithChart = action.json.result?.find( report => !!report.chart.id, ); // Assign the report's id @@ -148,7 +141,7 @@ export default function reportsReducer(state = {}, action) { [DELETE_REPORT]() { // Grabs the first report with a dashboard id that // matches the parameter report's dashboard_id - const reportWithDashboard = action.report.result.find( + const reportWithDashboard = action.report.result?.find( report => !!report.dashboard_id, ); From fd66de281675ddff4853ec82e4a0d11363924c33 Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 21 Jan 2022 14:19:46 -0500 Subject: [PATCH 19/23] refactoring reports --- .../HeaderReportActionsDropdown/index.tsx | 27 +++++--- .../src/components/ReportModal/index.tsx | 17 ++--- .../src/dashboard/components/Header/index.jsx | 6 +- .../components/ExploreChartHeader/index.jsx | 2 - .../src/reports/actions/reports.js | 6 +- .../src/reports/reducers/reports.js | 67 ++++++------------- superset-frontend/src/types/Owner.ts | 1 + .../src/views/CRUD/alert/types.ts | 4 +- 8 files changed, 53 insertions(+), 77 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index b9de984bbd70f..a7a7eefd2c5fa 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -17,6 +17,7 @@ * 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'; @@ -46,21 +47,17 @@ export default function HeaderReportActionsDropDown({ chart?: ChartState; }) { const dispatch = useDispatch(); - const report: AlertObject = useSelector(state => { + + const findReport = useSelector(state => { if (dashboardId) { return state.reports.dashboards?.[dashboardId]; } if (chart?.id) { - return state.reports.charts?.[chart.id]; + return state.reports.charts?.[chart?.id]; } return {}; }); - // const report: ReportObject = Object.values(reports).filter(report => { - // if (dashboardId) { - // return report.dashboards?.[dashboardId]; - // } - // // return report.charts?.[chart?.id] - // })[0]; + const report = findReport; const user: UserWithPermissionsAndRoles = useSelector< any, @@ -69,6 +66,7 @@ export default function HeaderReportActionsDropDown({ 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) { @@ -98,8 +96,18 @@ export default function HeaderReportActionsDropDown({ return permissions[0].length > 0; }; + const canRender = () => { + if (dashboardId) { + return prevDashboard !== dashboardId; + } + if (chart?.id) { + return true; + } + return false; + }; + useEffect(() => { - if (canAddReports()) { + if (canAddReports() && canRender()) { dispatch( fetchUISpecificReport({ userId: user.userId, @@ -158,6 +166,7 @@ export default function HeaderReportActionsDropDown({ userEmail={user.email} dashboardId={dashboardId} chart={chart} + findReport={findReport} /> {report ? ( <> diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 79f471cdb7016..835cbb487d7e1 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -28,8 +28,6 @@ import { t, SupersetTheme } from '@superset-ui/core'; import { useDispatch, useSelector } from 'react-redux'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; import { addReport, editReport } from 'src/reports/actions/reports'; -import { AlertObject } from 'src/views/CRUD/alert/types'; - import TimezoneSelector from 'src/components/TimezoneSelector'; import LabeledErrorBoundInput from 'src/components/Form/LabeledErrorBoundInput'; import Icons from 'src/components/Icons'; @@ -84,11 +82,7 @@ interface ReportProps { userEmail: string; dashboardId?: number; chart?: ChartState; -<<<<<<< HEAD - props?: any; -======= - props: any; ->>>>>>> be2e1ecf6... code dry (#16358) + findReport: ReportObject; } interface ReportPayloadType { @@ -175,6 +169,7 @@ const ReportModal: FunctionComponent = ({ chart, userId, userEmail, + findReport, }) => { const vizType = chart?.sliceFormData?.viz_type; const isChart = !!chart; @@ -191,13 +186,11 @@ const ReportModal: FunctionComponent = ({ const [cronError, setCronError] = useState(); const dispatch = useDispatch(); // Report fetch logic - const reports = useSelector(state => state.reports); - const isEditMode = reports && Object.keys(reports).length; + const report = findReport; + const isEditMode = report && Object.keys(report).length; useEffect(() => { if (isEditMode) { - const reportsIds = Object.keys(reports); - const report = reports[reportsIds[0]]; setCurrentReport({ type: ActionType.fetched, payload: report, @@ -207,7 +200,7 @@ const ReportModal: FunctionComponent = ({ type: ActionType.reset, }); } - }, [reports]); + }, [report]); const onSave = async () => { // Create new Report diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index a1cdf01320594..c87a6f60bd96b 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -554,10 +554,8 @@ class Header extends React.PureComponent { onHide={this.hidePropertiesModal} colorScheme={this.props.colorScheme} onSubmit={updates => { - const { - dashboardInfoChanged, - dashboardTitleChanged, - } = this.props; + const { dashboardInfoChanged, dashboardTitleChanged } = + this.props; dashboardInfoChanged({ slug: updates.slug, metadata: JSON.parse(updates.jsonMetadata), diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 3e5aa4d9c0f37..9b809fa88cd4e 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, @@ -28,7 +27,6 @@ import { styled, t, } from '@superset-ui/core'; -import { Tooltip } from 'src/components/Tooltip'; import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports'; import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; import { chartPropShape } from 'src/dashboard/util/propShapes'; diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 158d8a815710a..9e7f7c2a9c105 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -30,8 +30,8 @@ export function setReport(report) { } export const DELETE_REPORT = 'DELETE_REPORT'; -export function deleteReport(reportId) { - return { type: DELETE_REPORT, reportId }; +export function deleteReport(report) { + return { type: DELETE_REPORT, report }; } export function fetchUISpecificReport({ @@ -164,7 +164,7 @@ export function deleteActiveReport(report) { dispatch(addDangerToast(t('Your report could not be deleted'))); }) .finally(() => { - dispatch(deleteReport(report.id)); + dispatch(deleteReport(report)); dispatch(addSuccessToast(t('Deleted: %s', report.name))); }); }; diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index fc8f5754a116a..04aac19f9710e 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -18,7 +18,6 @@ */ /* eslint-disable camelcase */ // eslint-disable-next-line import/no-extraneous-dependencies -import { report } from 'process'; import { SET_REPORT, ADD_REPORT, @@ -26,17 +25,6 @@ import { DELETE_REPORT, } from '../actions/reports'; -/* -- Report schema -- -reports: { - dashboards: { - [dashboardId]: {...reportObject} - }, - charts: { - [chartId]: {...reportObject} - }, -} -*/ - export default function reportsReducer(state = {}, action) { const actionHandlers = { [SET_REPORT]() { @@ -79,13 +67,12 @@ export default function reportsReducer(state = {}, action) { [ADD_REPORT]() { const { result, id } = action.json; const report = { ...result, id }; - if (result.dashboard) { return { ...state, dashboards: { ...state.dashboards, - [report.id]: report, + [result.dashboard]: report, }, }; } @@ -94,7 +81,7 @@ export default function reportsReducer(state = {}, action) { ...state, charts: { ...state.chart, - [report.id]: report, + [result.chart]: report, }, }; } @@ -104,28 +91,17 @@ export default function reportsReducer(state = {}, action) { }, [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; + const report = { + ...action.json.result, + id: action.json.id, + }; - // This updates the report by its type, dashboard or chart - if (reportWithDashboard) { + if (action.json.result.dashboard) { return { ...state, dashboards: { ...state.dashboards, - [reportWithDashboard.dashboard_id]: report, + [report.dashboard]: report, }, }; } @@ -133,38 +109,37 @@ export default function reportsReducer(state = {}, action) { ...state, charts: { ...state.chart, - [reportWithChart.chart.id]: report, + [report.chart]: report, }, }; }, [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 reportWithDashboard = !!action.report.dashboard; - // This deletes the report by its type, dashboard or chart if (reportWithDashboard) { + const { dashboard } = action.report; + // making a shallow copy so as to not directly delete state + const { ...dashboardReports } = state.dashboards; + delete dashboardReports[dashboard]; return { ...state, dashboards: { - ...state.dashboards.filter(report => report.id !== action.reportId), + dashboardReports, }, }; } + + const { chart } = action.report; + // making a shallow copy so as to not directly delete state + const { ...chartReports } = state.charts; + delete chartReports[chart]; return { ...state, charts: { - ...state.charts.filter(chart => chart.id !== action.reportId), + chartReports, }, }; - - // state.users.filter(item => item.id !== action.payload) - // return { - // ...state.filter(report => report.id !== action.reportId), - // }; }, }; diff --git a/superset-frontend/src/types/Owner.ts b/superset-frontend/src/types/Owner.ts index 8e7d63f25b92e..b3645990e1b97 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 99a9c480abef4..5a96493641bb2 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -59,6 +59,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 +82,7 @@ export type AlertObject = { sql?: string; timezone?: string; recipients?: Array; - report_format?: 'PNG' | 'CSV' | 'TEXT'; + report_format?: 'PNG' | 'CSV' | 'TEXT' | string; type?: string; validator_config_json?: { op?: Operator; @@ -89,6 +90,7 @@ export type AlertObject = { }; validator_type?: string; working_timeout?: number; + error?: string; }; export type LogObject = { From e0b92684bfc040ab54432f7de54b56c69cada436 Mon Sep 17 00:00:00 2001 From: Arash Date: Mon, 24 Jan 2022 11:58:10 -0500 Subject: [PATCH 20/23] ready for review --- .../HeaderReportActionsDropdown/index.tsx | 44 +++--------- .../src/components/ReportModal/index.test.tsx | 11 ++- .../src/components/ReportModal/index.tsx | 55 ++++++-------- .../src/dashboard/components/Header/index.jsx | 21 ++---- .../src/dashboard/util/constants.ts | 5 ++ .../ExploreChartHeader.test.tsx | 8 --- .../src/reports/actions/reports.js | 13 ++-- .../src/reports/reducers/reports.js | 72 +++---------------- superset-frontend/src/types/Owner.ts | 2 +- .../src/views/CRUD/alert/types.ts | 29 +++++++- superset-frontend/src/views/CRUD/hooks.ts | 11 +++ 11 files changed, 99 insertions(+), 172 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx index a7a7eefd2c5fa..e2f4e478e6042 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx @@ -30,6 +30,8 @@ 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}; @@ -48,16 +50,12 @@ export default function HeaderReportActionsDropDown({ }) { const dispatch = useDispatch(); - const findReport = useSelector(state => { - if (dashboardId) { - return state.reports.dashboards?.[dashboardId]; - } - if (chart?.id) { - return state.reports.charts?.[chart?.id]; - } - return {}; + const report = useSelector(state => { + const resourceType = dashboardId + ? ReportType.DASHBOARDS + : ReportType.CHARTS; + return reportSelector(state, resourceType, dashboardId || chart?.id); }); - const report = findReport; const user: UserWithPermissionsAndRoles = useSelector< any, @@ -96,18 +94,12 @@ export default function HeaderReportActionsDropDown({ return permissions[0].length > 0; }; - const canRender = () => { - if (dashboardId) { - return prevDashboard !== dashboardId; - } - if (chart?.id) { - return true; - } - return false; - }; + const shouldFetch = + canAddReports() && + !!((dashboardId && prevDashboard !== dashboardId) || chart?.id); useEffect(() => { - if (canAddReports() && canRender()) { + if (shouldFetch) { dispatch( fetchUISpecificReport({ userId: user.userId, @@ -119,19 +111,6 @@ export default function HeaderReportActionsDropDown({ } }, []); - useEffect(() => { - if (canAddReports()) { - dispatch( - fetchUISpecificReport({ - userId: user.userId, - filterField: dashboardId ? 'dashboard_id' : 'chart_id', - creationMethod: dashboardId ? 'dashboards' : 'charts', - resourceId: dashboardId || chart?.id, - }), - ); - } - }, [dashboardId]); - const menu = () => ( @@ -166,7 +145,6 @@ export default function HeaderReportActionsDropDown({ userEmail={user.email} dashboardId={dashboardId} chart={chart} - findReport={findReport} /> {report ? ( <> diff --git a/superset-frontend/src/components/ReportModal/index.test.tsx b/superset-frontend/src/components/ReportModal/index.test.tsx index 44e3d0ef65c51..a38faed631ef4 100644 --- a/superset-frontend/src/components/ReportModal/index.test.tsx +++ b/superset-frontend/src/components/ReportModal/index.test.tsx @@ -33,16 +33,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', }, }, }; diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index 835cbb487d7e1..f1ad3a299e0c1 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -34,7 +34,10 @@ import Icons from 'src/components/Icons'; import withToasts from 'src/components/MessageToasts/withToasts'; import { CronError } from 'src/components/CronPicker'; import { RadioChangeEvent } from 'src/common/components'; +import { ReportObject, NOTIFICATION_FORMATS } from 'src/views/CRUD/alert/types'; import { ChartState } from 'src/explore/types'; +import { reportSelector } from 'src/views/CRUD/hooks'; +import { ReportType } from 'src/dashboard/util/constants'; import { StyledModal, StyledTopSection, @@ -52,37 +55,20 @@ 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: string }]; - report_format: string; - timezone: string; - type: string; - validator_config_json: {} | null; - validator_type: string; - working_timeout: number; - creation_method: string; - force_screenshot: boolean; - error?: string; -} +const errorMapping = { + 'Name must be unique': t( + 'A report with this name exists, please enter a new name.', + ), +}; interface ReportProps { addReport: (report?: ReportObject) => {}; onHide: () => void; onReportAdd: (report?: ReportObject) => {}; - showModal: boolean; + showModal?: boolean; userId: number; userEmail: string; dashboardId?: number; chart?: ChartState; - findReport: ReportObject; } interface ReportPayloadType { @@ -123,12 +109,6 @@ const TEXT_BASED_VISUALIZATION_TYPES = [ 'paired_ttest', ]; -const NOTIFICATION_FORMATS = { - TEXT: 'TEXT', - PNG: 'PNG', - CSV: 'CSV', -}; - const reportReducer = ( state: Partial | null, action: ReportActionType, @@ -169,7 +149,6 @@ const ReportModal: FunctionComponent = ({ chart, userId, userEmail, - findReport, }) => { const vizType = chart?.sliceFormData?.viz_type; const isChart = !!chart; @@ -186,7 +165,12 @@ const ReportModal: FunctionComponent = ({ const [cronError, setCronError] = useState(); const dispatch = useDispatch(); // Report fetch logic - const report = findReport; + 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(() => { @@ -300,6 +284,13 @@ const ReportModal: FunctionComponent = ({ ); + const renderErrorMessage = () => { + if (currentReport?.error) { + return errorMapping[currentReport.error] || currentReport?.error; + } + return currentReport?.error; + }; + return ( = ({ value: target.value, }), }} - errorMessage={currentReport?.error || ''} + errorMessage={renderErrorMessage() || ''} label="Report Name" data-test="report-name-test" /> diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index c87a6f60bd96b..572156135ff22 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -550,26 +550,13 @@ class Header extends React.PureComponent { {this.state.showingPropertiesModal && ( { - const { dashboardInfoChanged, dashboardTitleChanged } = - this.props; - dashboardInfoChanged({ - slug: updates.slug, - metadata: JSON.parse(updates.jsonMetadata), - }); - setColorSchemeAndUnsavedChanges(updates.colorScheme); - dashboardTitleChanged(updates.title); - if (updates.slug) { - window.history.pushState( - { event: 'dashboard_properties_changed' }, - '', - `/superset/dashboard/${updates.slug}/`, - ); - } - }} + onSubmit={handleOnPropertiesChange} + onlyApply /> )} diff --git a/superset-frontend/src/dashboard/util/constants.ts b/superset-frontend/src/dashboard/util/constants.ts index 640028eb4e947..ab2b2b8a3bcb5 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 702498d39bc38..4349a1dba12f7 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -49,11 +49,7 @@ const createProps = () => ({ }, chartStatus: 'rendered', }, -<<<<<<< HEAD slice: { -======= - slice: ({ ->>>>>>> fix(Explore): Remove changes to the properties on cancel (#17184) cache_timeout: null, changed_on: '2021-03-19T16:30:56.750230', changed_on_humanized: '7 days ago', @@ -90,11 +86,7 @@ const createProps = () => ({ slice_id: 318, slice_name: 'Age distribution of respondents', slice_url: '/superset/explore/?form_data=%7B%22slice_id%22%3A%20318%7D', -<<<<<<< HEAD } as unknown as Slice, -======= - } as unknown) as Slice, ->>>>>>> fix(Explore): Remove changes to the properties on cancel (#17184) slice_name: 'Age distribution of respondents', actions: { postChartFormData: () => null, diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 9e7f7c2a9c105..581150c5b75c9 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -25,13 +25,8 @@ import { } from 'src/components/MessageToasts/actions'; export const SET_REPORT = 'SET_REPORT'; -export function setReport(report) { - return { type: SET_REPORT, report }; -} - -export const DELETE_REPORT = 'DELETE_REPORT'; -export function deleteReport(report) { - return { type: DELETE_REPORT, report }; +export function setReport(report, resourceId, creationMethod) { + return { type: SET_REPORT, report, resourceId, creationMethod }; } export function fetchUISpecificReport({ @@ -64,7 +59,7 @@ export function fetchUISpecificReport({ endpoint: `/api/v1/report/?q=${queryParams}`, }) .then(({ json }) => { - dispatch(setReport(json)); + dispatch(setReport(json, resourceId, creationMethod)); }) .catch(() => dispatch( @@ -164,7 +159,7 @@ export function deleteActiveReport(report) { dispatch(addDangerToast(t('Your report could not be deleted'))); }) .finally(() => { - dispatch(deleteReport(report)); + dispatch(structureFetchAction); dispatch(addSuccessToast(t('Deleted: %s', report.name))); }); }; diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index 04aac19f9710e..18c05734bc729 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -18,49 +18,21 @@ */ /* eslint-disable camelcase */ // eslint-disable-next-line import/no-extraneous-dependencies -import { - SET_REPORT, - ADD_REPORT, - EDIT_REPORT, - DELETE_REPORT, -} from '../actions/reports'; +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, + }, }; }, @@ -113,34 +85,6 @@ export default function reportsReducer(state = {}, action) { }, }; }, - - [DELETE_REPORT]() { - const reportWithDashboard = !!action.report.dashboard; - - if (reportWithDashboard) { - const { dashboard } = action.report; - // making a shallow copy so as to not directly delete state - const { ...dashboardReports } = state.dashboards; - delete dashboardReports[dashboard]; - return { - ...state, - dashboards: { - dashboardReports, - }, - }; - } - - const { chart } = action.report; - // making a shallow copy so as to not directly delete state - const { ...chartReports } = state.charts; - delete chartReports[chart]; - return { - ...state, - charts: { - chartReports, - }, - }; - }, }; if (action.type in actionHandlers) { diff --git a/superset-frontend/src/types/Owner.ts b/superset-frontend/src/types/Owner.ts index b3645990e1b97..b7548ec629003 100644 --- a/superset-frontend/src/types/Owner.ts +++ b/superset-frontend/src/types/Owner.ts @@ -26,5 +26,5 @@ export default interface Owner { id: number; last_name: string; username: string; - email: string; + email?: string; } diff --git a/superset-frontend/src/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index 5a96493641bb2..1b6bcaae26c63 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -82,7 +82,7 @@ export type AlertObject = { sql?: string; timezone?: string; recipients?: Array; - report_format?: 'PNG' | 'CSV' | 'TEXT' | string; + report_format?: NOTIFICATION_FORMATS; type?: string; validator_config_json?: { op?: Operator; @@ -93,6 +93,33 @@ export type AlertObject = { 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: string }]; + report_format: string; + timezone: string; + type: string; + 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 2c9c2f9930e05..9cb7488a7411c 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -759,3 +759,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 {}; +}; From 9abc8038f02c63cfa8e61479873d3bbf01de3275 Mon Sep 17 00:00:00 2001 From: Arash Date: Wed, 9 Feb 2022 17:02:16 -0500 Subject: [PATCH 21/23] added testing --- .../spec/helpers/reducerIndex.ts | 2 + superset-frontend/src/SqlLab/reducers/user.js | 21 +++ .../HeaderReportDropdown/index.test.tsx | 153 ++++++++++++++++ .../index.tsx | 124 ++++++------- .../src/components/ReportModal/index.test.tsx | 71 ++++++++ .../components/Header/Header.test.tsx | 172 ------------------ .../src/dashboard/components/Header/index.jsx | 4 +- .../src/dashboard/reducers/types.ts | 10 + .../components/ExploreChartHeader/index.jsx | 4 +- 9 files changed, 324 insertions(+), 237 deletions(-) create mode 100644 superset-frontend/src/SqlLab/reducers/user.js create mode 100644 superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx rename superset-frontend/src/components/ReportModal/{HeaderReportActionsDropdown => HeaderReportDropdown}/index.tsx (70%) diff --git a/superset-frontend/spec/helpers/reducerIndex.ts b/superset-frontend/spec/helpers/reducerIndex.ts index 5073d9fd6894a..5415ba5171c74 100644 --- a/superset-frontend/spec/helpers/reducerIndex.ts +++ b/superset-frontend/spec/helpers/reducerIndex.ts @@ -29,6 +29,7 @@ import messageToasts from 'src/components/MessageToasts/reducers'; import saveModal from 'src/explore/reducers/saveModalReducer'; import explore from 'src/explore/reducers/exploreReducer'; import sqlLab from 'src/SqlLab/reducers/sqlLab'; +import user from 'src/SqlLab/reducers/user'; import localStorageUsageInKilobytes from 'src/SqlLab/reducers/localStorageUsage'; import reports from 'src/reports/reducers/reports'; @@ -54,6 +55,7 @@ export default { explore, sqlLab, localStorageUsageInKilobytes, + user, reports, common: () => common, }; diff --git a/superset-frontend/src/SqlLab/reducers/user.js b/superset-frontend/src/SqlLab/reducers/user.js new file mode 100644 index 0000000000000..33de2c9857c3e --- /dev/null +++ b/superset-frontend/src/SqlLab/reducers/user.js @@ -0,0 +1,21 @@ +/** + * 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. + */ +export default function user(state = {}) { + return state; +} 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 0000000000000..f9021053a02b5 --- /dev/null +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx @@ -0,0 +1,153 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React from 'react'; +import userEvent from '@testing-library/user-event'; +import { render, screen } 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 = { + 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 = { + 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(); + setup(mockedProps, stateWithUserAndReport); + expect(screen.getByRole('button')).toBeInTheDocument(); + }); + + it('renders the dropdown correctly', () => { + const mockedProps = createProps(); + 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(); + 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(); + 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(); + 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/HeaderReportActionsDropdown/index.tsx b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx similarity index 70% rename from superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx rename to superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx index e2f4e478e6042..b35159bbc7909 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx @@ -37,17 +37,19 @@ const deleteColor = (theme: SupersetTheme) => css` color: ${theme.colors.error.base}; `; -export default function HeaderReportActionsDropDown({ +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, -}: { - toggleActive: (data: AlertObject, checked: boolean) => void; - deleteActiveReport: (data: AlertObject) => void; - dashboardId?: number; - chart?: ChartState; -}) { +}: HeaderReportProps) { const dispatch = useDispatch(); const report = useSelector(state => { @@ -56,7 +58,6 @@ export default function HeaderReportActionsDropDown({ : ReportType.CHARTS; return reportSelector(state, resourceType, dashboardId || chart?.id); }); - const user: UserWithPermissionsAndRoles = useSelector< any, UserWithPermissionsAndRoles @@ -136,59 +137,60 @@ export default function HeaderReportActionsDropDown({ ); return ( - canAddReports() && ( - <> - setShowModal(false)} - userEmail={user.email} - dashboardId={dashboardId} - chart={chart} - /> - {report ? ( - <> - - triggerNode.closest('.action-button') - } + <> + {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)} > - - - - - {currentReportDeleting && ( - { - if (currentReportDeleting) { - handleReportDelete(currentReportDeleting); - } - }} - onHide={() => setCurrentReportDeleting(null)} - open - title={t('Delete Report?')} - /> - )} - - ) : ( - setShowModal(true)} - > - - - )} - - ) + + + )} + + )} + ); } diff --git a/superset-frontend/src/components/ReportModal/index.test.tsx b/superset-frontend/src/components/ReportModal/index.test.tsx index a38faed631ef4..c1a6b84451321 100644 --- a/superset-frontend/src/components/ReportModal/index.test.tsx +++ b/superset-frontend/src/components/ReportModal/index.test.tsx @@ -18,13 +18,19 @@ */ import 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 = { @@ -104,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/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index ea94aceead179..74c91ec690de6 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 '.'; @@ -323,171 +319,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 572156135ff22..55be323593263 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'; @@ -538,7 +538,7 @@ class Header extends React.PureComponent { )} - ; + roles: Record; + userId: number; + username: string; +}; export interface DashboardInfo { id: number; json_metadata: string; diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 9b809fa88cd4e..f0db61465a39e 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -28,7 +28,7 @@ import { t, } from '@superset-ui/core'; import { toggleActive, deleteActiveReport } from 'src/reports/actions/reports'; -import HeaderReportActionsDropdown from 'src/components/ReportModal/HeaderReportActionsDropdown'; +import HeaderReportDropdown from 'src/components/ReportModal/HeaderReportDropdown'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import EditableTitle from 'src/components/EditableTitle'; import AlteredSliceTag from 'src/components/AlteredSliceTag'; @@ -271,7 +271,7 @@ export class ExploreChartHeader extends React.PureComponent { isRunning={chartStatus === 'loading'} status={CHART_STATUS_MAP[chartStatus]} /> - Date: Thu, 10 Feb 2022 12:28:55 -0500 Subject: [PATCH 22/23] removed user reducer --- .../spec/helpers/reducerIndex.ts | 2 - superset-frontend/src/SqlLab/reducers/user.js | 21 ------ .../HeaderReportDropdown/index.test.tsx | 68 +++++++++++-------- .../HeaderReportDropdown/index.tsx | 3 +- .../src/components/ReportModal/index.test.tsx | 2 +- .../src/components/ReportModal/index.tsx | 7 +- .../components/Header/Header.test.tsx | 3 - .../ExploreChartHeader.test.tsx | 4 +- .../components/PropertiesModal/index.tsx | 2 +- .../src/views/CRUD/alert/types.ts | 2 +- 10 files changed, 51 insertions(+), 63 deletions(-) delete mode 100644 superset-frontend/src/SqlLab/reducers/user.js diff --git a/superset-frontend/spec/helpers/reducerIndex.ts b/superset-frontend/spec/helpers/reducerIndex.ts index 5415ba5171c74..5073d9fd6894a 100644 --- a/superset-frontend/spec/helpers/reducerIndex.ts +++ b/superset-frontend/spec/helpers/reducerIndex.ts @@ -29,7 +29,6 @@ import messageToasts from 'src/components/MessageToasts/reducers'; import saveModal from 'src/explore/reducers/saveModalReducer'; import explore from 'src/explore/reducers/exploreReducer'; import sqlLab from 'src/SqlLab/reducers/sqlLab'; -import user from 'src/SqlLab/reducers/user'; import localStorageUsageInKilobytes from 'src/SqlLab/reducers/localStorageUsage'; import reports from 'src/reports/reducers/reports'; @@ -55,7 +54,6 @@ export default { explore, sqlLab, localStorageUsageInKilobytes, - user, reports, common: () => common, }; diff --git a/superset-frontend/src/SqlLab/reducers/user.js b/superset-frontend/src/SqlLab/reducers/user.js deleted file mode 100644 index 33de2c9857c3e..0000000000000 --- a/superset-frontend/src/SqlLab/reducers/user.js +++ /dev/null @@ -1,21 +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. - */ -export default function user(state = {}) { - return state; -} diff --git a/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx index f9021053a02b5..18c27c2421433 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx @@ -16,9 +16,9 @@ * 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 { render, screen } from 'spec/helpers/testing-library'; +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 '.'; @@ -32,31 +32,35 @@ const createProps = () => ({ }); const stateWithOnlyUser = { - 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', + 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 = { - 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', + 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: { @@ -109,13 +113,17 @@ describe('Header Report Dropdown', () => { it('renders correctly', () => { const mockedProps = createProps(); - setup(mockedProps, stateWithUserAndReport); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); expect(screen.getByRole('button')).toBeInTheDocument(); }); it('renders the dropdown correctly', () => { const mockedProps = createProps(); - setup(mockedProps, stateWithUserAndReport); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); const emailReportModalButton = screen.getByRole('button'); userEvent.click(emailReportModalButton); expect(screen.getByText('Email reports active')).toBeInTheDocument(); @@ -125,7 +133,9 @@ describe('Header Report Dropdown', () => { it('opens an edit modal', () => { const mockedProps = createProps(); - setup(mockedProps, stateWithUserAndReport); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); const emailReportModalButton = screen.getByRole('button'); userEvent.click(emailReportModalButton); const editModal = screen.getByText('Edit email report'); @@ -135,7 +145,9 @@ describe('Header Report Dropdown', () => { it('opens a delete modal', () => { const mockedProps = createProps(); - setup(mockedProps, stateWithUserAndReport); + act(() => { + setup(mockedProps, stateWithUserAndReport); + }); const emailReportModalButton = screen.getByRole('button'); userEvent.click(emailReportModalButton); const deleteModal = screen.getByText('Delete email report'); @@ -145,7 +157,9 @@ describe('Header Report Dropdown', () => { it('renders a new report modal if there is no report', () => { const mockedProps = createProps(); - setup(mockedProps, stateWithOnlyUser); + 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 index b35159bbc7909..e9c5f697f8e6f 100644 --- a/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx +++ b/superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx @@ -82,6 +82,7 @@ export default function HeaderReportDropDown({ if (!isFeatureEnabled(FeatureFlag.ALERT_REPORTS)) { return false; } + if (!user?.userId) { // this is in the case that there is an anonymous user. return false; @@ -94,7 +95,6 @@ export default function HeaderReportDropDown({ ); return permissions[0].length > 0; }; - const shouldFetch = canAddReports() && !!((dashboardId && prevDashboard !== dashboardId) || chart?.id); @@ -135,7 +135,6 @@ export default function HeaderReportDropDown({ ); - return ( <> {canAddReports() && ( diff --git a/superset-frontend/src/components/ReportModal/index.test.tsx b/superset-frontend/src/components/ReportModal/index.test.tsx index c1a6b84451321..1656e220d67c7 100644 --- a/superset-frontend/src/components/ReportModal/index.test.tsx +++ b/superset-frontend/src/components/ReportModal/index.test.tsx @@ -16,7 +16,7 @@ * 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'; diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index f1ad3a299e0c1..d70da4e19d10f 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -22,7 +22,6 @@ import React, { useCallback, useReducer, Reducer, - FunctionComponent, } from 'react'; import { t, SupersetTheme } from '@superset-ui/core'; import { useDispatch, useSelector } from 'react-redux'; @@ -141,7 +140,7 @@ const reportReducer = ( } }; -const ReportModal: FunctionComponent = ({ +function ReportModal({ onReportAdd, onHide, showModal = false, @@ -149,7 +148,7 @@ const ReportModal: FunctionComponent = ({ chart, userId, userEmail, -}) => { +}: ReportProps) { const vizType = chart?.sliceFormData?.viz_type; const isChart = !!chart; const defaultNotificationFormat = @@ -379,6 +378,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 74c91ec690de6..e5851fb2d500b 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -108,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( diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx index 4349a1dba12f7..18fa715d5a74c 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx @@ -25,7 +25,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/PropertiesModal/index.tsx b/superset-frontend/src/explore/components/PropertiesModal/index.tsx index 022b74f02907d..b6a0418b61bc0 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/views/CRUD/alert/types.ts b/superset-frontend/src/views/CRUD/alert/types.ts index 1b6bcaae26c63..aeb1d3e0f57c3 100644 --- a/superset-frontend/src/views/CRUD/alert/types.ts +++ b/superset-frontend/src/views/CRUD/alert/types.ts @@ -59,7 +59,7 @@ export type Operator = '<' | '>' | '<=' | '>=' | '==' | '!=' | 'not null'; export type AlertObject = { active?: boolean; - creation_method: string; + creation_method?: string; chart?: MetaObject; changed_by?: user; changed_on_delta_humanized?: string; From 1e8b10fd629c3ac17c9556f875c64574545d2e62 Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 18 Feb 2022 11:11:59 -0500 Subject: [PATCH 23/23] elizabeth suggestions --- .../src/components/ReportModal/index.tsx | 11 ++--- .../src/reports/reducers/reports.js | 40 +++++-------------- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/superset-frontend/src/components/ReportModal/index.tsx b/superset-frontend/src/components/ReportModal/index.tsx index d70da4e19d10f..4b235c85dc75b 100644 --- a/superset-frontend/src/components/ReportModal/index.tsx +++ b/superset-frontend/src/components/ReportModal/index.tsx @@ -283,12 +283,9 @@ function ReportModal({ ); - const renderErrorMessage = () => { - if (currentReport?.error) { - return errorMapping[currentReport.error] || currentReport?.error; - } - return currentReport?.error; - }; + const renderErrorMessage = + currentReport?.error && + (errorMapping[currentReport.error] || currentReport?.error); return ( diff --git a/superset-frontend/src/reports/reducers/reports.js b/superset-frontend/src/reports/reducers/reports.js index 18c05734bc729..d011cb2dc6031 100644 --- a/superset-frontend/src/reports/reducers/reports.js +++ b/superset-frontend/src/reports/reducers/reports.js @@ -39,26 +39,14 @@ export default function reportsReducer(state = {}, action) { [ADD_REPORT]() { const { result, id } = action.json; const report = { ...result, id }; - if (result.dashboard) { - return { - ...state, - dashboards: { - ...state.dashboards, - [result.dashboard]: report, - }, - }; - } - if (result.chart) { - return { - ...state, - charts: { - ...state.chart, - [result.chart]: report, - }, - }; - } + const reportId = report.dashboard || report.chart; + return { ...state, + [report.creation_method]: { + ...state[report.creation_method], + [reportId]: report, + }, }; }, @@ -67,21 +55,13 @@ export default function reportsReducer(state = {}, action) { ...action.json.result, id: action.json.id, }; + const reportId = report.dashboard || report.chart; - if (action.json.result.dashboard) { - return { - ...state, - dashboards: { - ...state.dashboards, - [report.dashboard]: report, - }, - }; - } return { ...state, - charts: { - ...state.chart, - [report.chart]: report, + [report.creation_method]: { + ...state[report.creation_method], + [reportId]: report, }, }; },