You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/01/21 19:21:54 UTC
[GitHub] [superset] AAfghahi opened a new pull request #18131: refactoring reports
AAfghahi opened a new pull request #18131:
URL: https://github.com/apache/superset/pull/18131
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Just a draft.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803192529
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -149,59 +135,61 @@ export default function HeaderReportActionsDropDown({
);
return (
- canAddReports() && (
- <>
- <ReportModal
- userId={user.userId}
- showModal={showModal}
- onHide={() => setShowModal(false)}
- userEmail={user.email}
- dashboardId={dashboardId}
- chart={chart}
- />
- {report ? (
- <>
- <NoAnimationDropdown
- // ref={ref}
- overlay={menu()}
- trigger={['click']}
- getPopupContainer={(triggerNode: any) =>
- triggerNode.closest('.action-button')
- }
+ <>
+ {canAddReports() && (
+ <>
+ <ReportModal
+ userId={user.userId}
+ showModal={showModal}
+ onHide={() => setShowModal(false)}
+ userEmail={user.email}
+ dashboardId={dashboardId}
+ chart={chart}
+ />
+ {report ? (
+ <>
+ <NoAnimationDropdown
+ // ref={ref}
Review comment:
Which portion do you want me to remove?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806343673
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -147,61 +135,61 @@ export default function HeaderReportActionsDropDown({
</Menu.Item>
</Menu>
);
-
return (
- canAddReports() && (
- <>
- <ReportModal
- userId={user.userId}
- showModal={showModal}
- onHide={() => setShowModal(false)}
- userEmail={user.email}
- dashboardId={dashboardId}
- chart={chart}
- />
- {report ? (
- <>
- <NoAnimationDropdown
- // ref={ref}
- overlay={menu()}
- trigger={['click']}
- getPopupContainer={(triggerNode: any) =>
- triggerNode.closest('.action-button')
- }
+ <>
+ {canAddReports() && (
Review comment:
since this is a functional component we can probably make this a prop instead of a method. We can fix this on the merge to master PR.
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -307,6 +283,13 @@ const ReportModal: FunctionComponent<ReportProps> = ({
</>
);
+ const renderErrorMessage = () => {
+ if (currentReport?.error) {
+ return errorMapping[currentReport.error] || currentReport?.error;
+ }
+ return currentReport?.error;
Review comment:
if 287 is false, would this also be falsy/null/undefined? Should we just return nothing?
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -330,7 +313,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
value: target.value,
}),
}}
- errorMessage={currentReport?.error || ''}
+ errorMessage={renderErrorMessage() || ''}
Review comment:
another example where this could probably be a static property/const rather than method.
##########
File path: superset-frontend/src/reports/reducers/reports.js
##########
@@ -18,74 +18,33 @@
*/
/* eslint-disable camelcase */
// eslint-disable-next-line import/no-extraneous-dependencies
-import { report } from 'process';
-import {
- SET_REPORT,
- ADD_REPORT,
- EDIT_REPORT,
- DELETE_REPORT,
-} from '../actions/reports';
-
-/* -- Report schema --
-reports: {
- dashboards: {
- [dashboardId]: {...reportObject}
- },
- charts: {
- [chartId]: {...reportObject}
- },
-}
-*/
+import { SET_REPORT, ADD_REPORT, EDIT_REPORT } from '../actions/reports';
export default function reportsReducer(state = {}, action) {
const actionHandlers = {
[SET_REPORT]() {
- // Grabs the first report with a dashboard id that
- // matches the parameter report's dashboard_id
- const reportWithDashboard = action.report.result?.find(
- report => !!report.dashboard_id,
- );
- // Grabs the first report with a chart id that
- // matches the parameter report's chart.id
- const reportWithChart = action.report.result?.find(
- report => !!report.chart?.id,
- );
+ const { report, resourceId, creationMethod } = action;
+
+ const reportObject = report.result?.find(report => !!report[resourceId]);
- // This organizes report by its type, dashboard or chart
- // and indexes it by the dashboard/chart id
- if (reportWithDashboard) {
- return {
- ...state,
- dashboards: {
- ...state.dashboards,
- [reportWithDashboard.dashboard_id]: reportWithDashboard,
- },
- };
- }
- if (reportWithChart) {
- return {
- ...state,
- charts: {
- ...state.chart,
- [reportWithChart.chart.id]: reportWithChart,
- },
- };
- }
return {
...state,
+ [creationMethod]: {
+ ...state[creationMethod],
+ [resourceId]: reportObject,
+ },
Review comment:
this looks much better!
##########
File path: superset-frontend/src/reports/reducers/reports.js
##########
@@ -94,7 +53,7 @@ export default function reportsReducer(state = {}, action) {
...state,
charts: {
...state.chart,
- [report.id]: report,
+ [result.chart]: report,
Review comment:
can we do the same thing with these actions where we dry up some of the code to look up the chart or dashboard like above? Maybe in the next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803129855
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx
##########
@@ -0,0 +1,159 @@
+/**
+ * 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 from '.';
+
+let isFeatureEnabledMock: jest.MockInstance<boolean, [string]>;
+
+interface HeaderReportProps {
Review comment:
shouldn't we reference this in a types.tsx file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r791131913
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
ok done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1020528666
@AAfghahi Ephemeral environment spinning up at http://35.160.227.163:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r791094114
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
Why are you adding `string` here? Are there other values other than PNG/CSV/TEXT that are valid here? If you add `string` it means that `report_format` can have any value.
+1 on using an enum like @hughhhh said.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803192529
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -149,59 +135,61 @@ export default function HeaderReportActionsDropDown({
);
return (
- canAddReports() && (
- <>
- <ReportModal
- userId={user.userId}
- showModal={showModal}
- onHide={() => setShowModal(false)}
- userEmail={user.email}
- dashboardId={dashboardId}
- chart={chart}
- />
- {report ? (
- <>
- <NoAnimationDropdown
- // ref={ref}
- overlay={menu()}
- trigger={['click']}
- getPopupContainer={(triggerNode: any) =>
- triggerNode.closest('.action-button')
- }
+ <>
+ {canAddReports() && (
+ <>
+ <ReportModal
+ userId={user.userId}
+ showModal={showModal}
+ onHide={() => setShowModal(false)}
+ userEmail={user.email}
+ dashboardId={dashboardId}
+ chart={chart}
+ />
+ {report ? (
+ <>
+ <NoAnimationDropdown
+ // ref={ref}
Review comment:
Which portion do you want me to remove?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1020527480
/testenv up FEATURE_ALERT_REPORTS=True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r790987670
##########
File path: superset-frontend/src/dashboard/components/Header/index.jsx
##########
@@ -550,28 +550,13 @@ class Header extends React.PureComponent {
{this.state.showingPropertiesModal && (
<PropertiesModal
dashboardId={dashboardInfo.id}
+ dashboardInfo={dashboardInfo}
Review comment:
this is from: https://github.com/apache/superset/pull/17570/files it looks like it wasn't added correctly in a merge. I added it here to pass CI though I think it will be eliminated when the feature branch gets rebased and merged into master.
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
ok done
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
Ok done. Thank you for pointing this out, I put this in to fix a quick TS bug then got bogged down with other things and forgot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi merged pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi merged pull request #18131:
URL: https://github.com/apache/superset/pull/18131
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r810142403
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -307,6 +283,13 @@ const ReportModal: FunctionComponent<ReportProps> = ({
</>
);
+ const renderErrorMessage = () => {
+ if (currentReport?.error) {
+ return errorMapping[currentReport.error] || currentReport?.error;
+ }
+ return currentReport?.error;
Review comment:
change this to a static prop and I think it fixes it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r796013282
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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));
Review comment:
I played around with this, and it doesn't work very well with the new way that we structured the fetch action to get rid of the racing condition for hydrating a dashboard.
I was working through the logic for making the setReport action also consider a deleteAction, and then realized that I was just writing a delete action, so wrote it that way instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803129151
##########
File path: superset-frontend/src/dashboard/components/Header/Header.test.tsx
##########
@@ -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
Review comment:
remove this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803192289
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx
##########
@@ -0,0 +1,159 @@
+/**
+ * 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 from '.';
+
+let isFeatureEnabledMock: jest.MockInstance<boolean, [string]>;
+
+interface HeaderReportProps {
Review comment:
I exported it in the main component, I can create a type.ts file, but it would only hold this one interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803188758
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx
##########
@@ -0,0 +1,159 @@
+/**
+ * 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 from '.';
+
+let isFeatureEnabledMock: jest.MockInstance<boolean, [string]>;
+
+interface HeaderReportProps {
Review comment:
can we move it to the types file? i think it will be better served there vs. duplicating code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803129404
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,16 +82,44 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: NOTIFICATION_FORMATS;
type?: string;
validator_config_json?: {
op?: Operator;
threshold?: number;
};
validator_type?: string;
working_timeout?: number;
+ error?: string;
};
+export enum NOTIFICATION_FORMATS {
Review comment:
nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806345818
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -330,7 +313,7 @@ const ReportModal: FunctionComponent<ReportProps> = ({
value: target.value,
}),
}}
- errorMessage={currentReport?.error || ''}
+ errorMessage={renderErrorMessage() || ''}
Review comment:
another example where this could probably be a static property/const rather than method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803128828
##########
File path: superset-frontend/src/components/ReportModal/index.test.tsx
##########
@@ -107,4 +110,105 @@ 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);
+ });
+ });
+
+ // it('edits an existing email report', async () => {
Review comment:
Remove this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803220000
##########
File path: 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;
Review comment:
how does this relate to sql lab?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806343673
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -147,61 +135,61 @@ export default function HeaderReportActionsDropDown({
</Menu.Item>
</Menu>
);
-
return (
- canAddReports() && (
- <>
- <ReportModal
- userId={user.userId}
- showModal={showModal}
- onHide={() => setShowModal(false)}
- userEmail={user.email}
- dashboardId={dashboardId}
- chart={chart}
- />
- {report ? (
- <>
- <NoAnimationDropdown
- // ref={ref}
- overlay={menu()}
- trigger={['click']}
- getPopupContainer={(triggerNode: any) =>
- triggerNode.closest('.action-button')
- }
+ <>
+ {canAddReports() && (
Review comment:
since this is a functional component we can probably make this a prop instead of a method. We can fix this on the merge to master PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1039713076
@AAfghahi this looks great. I'm trying not to get greedy with too many changes at once. I think we can merge this and maybe clean up a little bit on the final merge. The only thing that seems like perhaps a mistake that we may want to address in this PR is the comment here: `return currentReport?.error;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r790987670
##########
File path: superset-frontend/src/dashboard/components/Header/index.jsx
##########
@@ -550,28 +550,13 @@ class Header extends React.PureComponent {
{this.state.showingPropertiesModal && (
<PropertiesModal
dashboardId={dashboardInfo.id}
+ dashboardInfo={dashboardInfo}
Review comment:
this is from: https://github.com/apache/superset/pull/17570/files it looks like it wasn't added correctly in a merge. I added it here to pass CI though I think it will be eliminated when the feature branch gets rebased and merged into master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r791131913
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
Ok done. Thank you for pointing this out, I put this in to fix a quick TS bug then got bogged down with other things and forgot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r793188038
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -122,16 +110,16 @@ export default function HeaderReportActionsDropDown({
}),
);
}
- }, [dashboardId]);
+ }, []);
const menu = () => (
<Menu selectable={false} css={{ width: '200px' }}>
<Menu.Item>
{t('Email reports active')}
<Switch
data-test="toggle-active"
- checked={report?.active}
- onClick={(checked: boolean) => toggleActiveKey(report, checked)}
+ checked={findReport?.active}
Review comment:
is there another var referring to report? `findReport` sounds more like an action than an object. Can we use `report` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803130374
##########
File path: superset-frontend/src/dashboard/components/Header/Header.test.tsx
##########
@@ -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
Review comment:
wait, this is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803132647
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.test.tsx
##########
@@ -0,0 +1,159 @@
+/**
+ * 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 from '.';
+
+let isFeatureEnabledMock: jest.MockInstance<boolean, [string]>;
+
+interface HeaderReportProps {
Review comment:
its invoked in the component, but not as an interface, so I made the interface here. Happy to move the entire thing to a new type folder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803220155
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -29,46 +30,42 @@ import ReportModal from 'src/components/ReportModal';
import { ChartState } from 'src/explore/types';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { fetchUISpecificReport } from 'src/reports/actions/reports';
+import { reportSelector } from 'src/views/CRUD/hooks';
+import { ReportType } from 'src/dashboard/util/constants';
const deleteColor = (theme: SupersetTheme) => css`
color: ${theme.colors.error.base};
`;
-export 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: AlertObject = useSelector<any, AlertObject>(state => {
- if (dashboardId) {
- return state.reports.dashboards?.[dashboardId];
- }
- if (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 = useSelector<any, AlertObject>(state => {
+ const resourceType = dashboardId
+ ? ReportType.DASHBOARDS
+ : ReportType.CHARTS;
+ return reportSelector(state, resourceType, dashboardId || chart?.id);
+ });
const user: UserWithPermissionsAndRoles = useSelector<
any,
UserWithPermissionsAndRoles
>(state => state.user || state.explore?.user);
Review comment:
actually, nm.. I guess user was added to sql lab
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1050216236
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1020528666
@AAfghahi Ephemeral environment spinning up at http://35.160.227.163:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r791037936
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -46,21 +47,17 @@ export default function HeaderReportActionsDropDown({
chart?: ChartState;
}) {
const dispatch = useDispatch();
- const report: AlertObject = useSelector<any, AlertObject>(state => {
+
+ const findReport = useSelector<any, AlertObject>(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;
Review comment:
If `report = findReport` just use `findReport` wherever you'd use `report`.
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
Why are you adding `string` here? Are there other values other than PNG/CSV/TEXT that are valid here? If you add `string` it means that `report_format` can have any value.
+1 on using an enum like @hughhhh said.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r793190999
##########
File path: superset-frontend/src/reports/actions/reports.js
##########
@@ -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));
Review comment:
were we going to fetch an empty report instead of delete from state manually?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r793185553
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -46,21 +47,16 @@ export default function HeaderReportActionsDropDown({
chart?: ChartState;
}) {
const dispatch = useDispatch();
- const report: AlertObject = useSelector<any, AlertObject>(state => {
+
+ const findReport = useSelector<any, AlertObject>(state => {
if (dashboardId) {
return state.reports.dashboards?.[dashboardId];
}
if (chart?.id) {
- return state.reports.charts?.[chart.id];
+ return state.reports.charts?.[chart?.id];
Review comment:
I don't think you need the optional chaining here, since you check for `chart` on line 55.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1020527480
/testenv up FEATURE_ALERT_REPORTS=True
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r790972810
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -46,21 +47,17 @@ export default function HeaderReportActionsDropDown({
chart?: ChartState;
}) {
const dispatch = useDispatch();
- const report: AlertObject = useSelector<any, AlertObject>(state => {
+
+ const findReport = useSelector<any, AlertObject>(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;
Review comment:
just set the `useSelector` code snippet as the report
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803129551
##########
File path: superset-frontend/src/components/ReportModal/index.test.tsx
##########
@@ -107,4 +110,105 @@ 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);
+ });
+ });
+
+ // it('edits an existing email report', async () => {
Review comment:
I think this a todo for the future though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803219636
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -29,46 +30,42 @@ import ReportModal from 'src/components/ReportModal';
import { ChartState } from 'src/explore/types';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { fetchUISpecificReport } from 'src/reports/actions/reports';
+import { reportSelector } from 'src/views/CRUD/hooks';
+import { ReportType } from 'src/dashboard/util/constants';
const deleteColor = (theme: SupersetTheme) => css`
color: ${theme.colors.error.base};
`;
-export 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: AlertObject = useSelector<any, AlertObject>(state => {
- if (dashboardId) {
- return state.reports.dashboards?.[dashboardId];
- }
- if (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 = useSelector<any, AlertObject>(state => {
+ const resourceType = dashboardId
+ ? ReportType.DASHBOARDS
+ : ReportType.CHARTS;
+ return reportSelector(state, resourceType, dashboardId || chart?.id);
+ });
const user: UserWithPermissionsAndRoles = useSelector<
any,
UserWithPermissionsAndRoles
>(state => state.user || state.explore?.user);
Review comment:
now that you added user to state, will this ever hit state.explore.user?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r803189033
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportDropdown/index.tsx
##########
@@ -149,59 +135,61 @@ export default function HeaderReportActionsDropDown({
);
return (
- canAddReports() && (
- <>
- <ReportModal
- userId={user.userId}
- showModal={showModal}
- onHide={() => setShowModal(false)}
- userEmail={user.email}
- dashboardId={dashboardId}
- chart={chart}
- />
- {report ? (
- <>
- <NoAnimationDropdown
- // ref={ref}
- overlay={menu()}
- trigger={['click']}
- getPopupContainer={(triggerNode: any) =>
- triggerNode.closest('.action-button')
- }
+ <>
+ {canAddReports() && (
+ <>
+ <ReportModal
+ userId={user.userId}
+ showModal={showModal}
+ onHide={() => setShowModal(false)}
+ userEmail={user.email}
+ dashboardId={dashboardId}
+ chart={chart}
+ />
+ {report ? (
+ <>
+ <NoAnimationDropdown
+ // ref={ref}
Review comment:
remove this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806345294
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -307,6 +283,13 @@ const ReportModal: FunctionComponent<ReportProps> = ({
</>
);
+ const renderErrorMessage = () => {
+ if (currentReport?.error) {
+ return errorMapping[currentReport.error] || currentReport?.error;
+ }
+ return currentReport?.error;
Review comment:
if 287 is false, would this also be falsy/null/undefined? Should we just return nothing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #18131:
URL: https://github.com/apache/superset/pull/18131#issuecomment-1039713076
@AAfghahi this looks great. I'm trying not to get greedy with too many changes at once. I think we can merge this and maybe clean up a little bit on the final merge. The only thing that seems like perhaps a mistake that we may want to address in this PR is the comment here: `return currentReport?.error;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r790977524
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -46,21 +47,17 @@ export default function HeaderReportActionsDropDown({
chart?: ChartState;
}) {
const dispatch = useDispatch();
- const report: AlertObject = useSelector<any, AlertObject>(state => {
+
+ const findReport = useSelector<any, AlertObject>(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;
Review comment:
I don't fully understand what you mean here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r790974382
##########
File path: superset-frontend/src/views/CRUD/alert/types.ts
##########
@@ -81,14 +82,15 @@ export type AlertObject = {
sql?: string;
timezone?: string;
recipients?: Array<Recipient>;
- report_format?: 'PNG' | 'CSV' | 'TEXT';
+ report_format?: 'PNG' | 'CSV' | 'TEXT' | string;
Review comment:
We should make `report_format` an ENUM with the following fields
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r791037936
##########
File path: superset-frontend/src/components/ReportModal/HeaderReportActionsDropdown/index.tsx
##########
@@ -46,21 +47,17 @@ export default function HeaderReportActionsDropDown({
chart?: ChartState;
}) {
const dispatch = useDispatch();
- const report: AlertObject = useSelector<any, AlertObject>(state => {
+
+ const findReport = useSelector<any, AlertObject>(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;
Review comment:
If `report = findReport` just use `findReport` wherever you'd use `report`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r793189883
##########
File path: superset-frontend/src/components/ReportModal/index.tsx
##########
@@ -191,13 +160,11 @@ const ReportModal: FunctionComponent<ReportProps> = ({
const [cronError, setCronError] = useState<CronError>();
const dispatch = useDispatch();
// Report fetch logic
- const reports = useSelector<any, AlertObject>(state => state.reports);
- const isEditMode = reports && Object.keys(reports).length;
+ const report = findReport;
Review comment:
instead of passing in the report, I would suggest fetching from Redux as much as possible. You can import the function that you pass to useSelector if you want to reuse the code.
```
const report = useSelector<any, AlertObject>(reportSelector)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r810142013
##########
File path: superset-frontend/src/reports/reducers/reports.js
##########
@@ -94,7 +53,7 @@ export default function reportsReducer(state = {}, action) {
...state,
charts: {
...state.chart,
- [report.id]: report,
+ [result.chart]: report,
Review comment:
yeah, I looked into it and it is easily done (so I just did)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806346370
##########
File path: superset-frontend/src/reports/reducers/reports.js
##########
@@ -18,74 +18,33 @@
*/
/* eslint-disable camelcase */
// eslint-disable-next-line import/no-extraneous-dependencies
-import { report } from 'process';
-import {
- SET_REPORT,
- ADD_REPORT,
- EDIT_REPORT,
- DELETE_REPORT,
-} from '../actions/reports';
-
-/* -- Report schema --
-reports: {
- dashboards: {
- [dashboardId]: {...reportObject}
- },
- charts: {
- [chartId]: {...reportObject}
- },
-}
-*/
+import { SET_REPORT, ADD_REPORT, EDIT_REPORT } from '../actions/reports';
export default function reportsReducer(state = {}, action) {
const actionHandlers = {
[SET_REPORT]() {
- // Grabs the first report with a dashboard id that
- // matches the parameter report's dashboard_id
- const reportWithDashboard = action.report.result?.find(
- report => !!report.dashboard_id,
- );
- // Grabs the first report with a chart id that
- // matches the parameter report's chart.id
- const reportWithChart = action.report.result?.find(
- report => !!report.chart?.id,
- );
+ const { report, resourceId, creationMethod } = action;
+
+ const reportObject = report.result?.find(report => !!report[resourceId]);
- // This organizes report by its type, dashboard or chart
- // and indexes it by the dashboard/chart id
- if (reportWithDashboard) {
- return {
- ...state,
- dashboards: {
- ...state.dashboards,
- [reportWithDashboard.dashboard_id]: reportWithDashboard,
- },
- };
- }
- if (reportWithChart) {
- return {
- ...state,
- charts: {
- ...state.chart,
- [reportWithChart.chart.id]: reportWithChart,
- },
- };
- }
return {
...state,
+ [creationMethod]: {
+ ...state[creationMethod],
+ [resourceId]: reportObject,
+ },
Review comment:
this looks much better!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a change in pull request #18131: refactor(reports): refactoring reports
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #18131:
URL: https://github.com/apache/superset/pull/18131#discussion_r806346778
##########
File path: superset-frontend/src/reports/reducers/reports.js
##########
@@ -94,7 +53,7 @@ export default function reportsReducer(state = {}, action) {
...state,
charts: {
...state.chart,
- [report.id]: report,
+ [result.chart]: report,
Review comment:
can we do the same thing with these actions where we dry up some of the code to look up the chart or dashboard like above? Maybe in the next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org