From 40dbdb3b00b283367df3e398131eb6bfaf81a624 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Wed, 18 Aug 2021 13:09:32 -0400 Subject: [PATCH 1/7] don't maniuplate error message --- superset-frontend/src/reports/actions/reports.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index c7eb0c934561f..46c89344f0689 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -110,11 +110,12 @@ export const addReport = report => dispatch => { .catch(async e => { const parsedError = await getClientErrorObject(e); const errorMessage = parsedError.message; - const errorArr = Object.keys(errorMessage); - const error = errorMessage[errorArr[0]][0]; + // Do we need this? + // const errorArr = Object.keys(errorMessage); + // const error = errorMessage[errorArr[0]][0]; dispatch( addDangerToast( - t('An error occurred while editing this report: %s', error), + t('An error occurred while editing this report: %s', errorMessage), ), ); }); From f8d9614062438a78233e1a7d35e3cf980482c91e Mon Sep 17 00:00:00 2001 From: hughhhh Date: Wed, 18 Aug 2021 13:21:21 -0400 Subject: [PATCH 2/7] remove extra idx reference --- superset-frontend/src/reports/actions/reports.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index 46c89344f0689..adcb8056b0d2b 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -17,6 +17,7 @@ * under the License. */ /* eslint camelcase: 0 */ +import { ConsoleSqlOutlined } from '@ant-design/icons'; import { t, SupersetClient } from '@superset-ui/core'; import rison from 'rison'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; @@ -110,12 +111,11 @@ export const addReport = report => dispatch => { .catch(async e => { const parsedError = await getClientErrorObject(e); const errorMessage = parsedError.message; - // Do we need this? - // const errorArr = Object.keys(errorMessage); - // const error = errorMessage[errorArr[0]][0]; + const errorArr = Object.keys(errorMessage); + const error = errorMessage[errorArr[0]]; dispatch( addDangerToast( - t('An error occurred while editing this report: %s', errorMessage), + t('An error occurred while editing this report: %s', error), ), ); }); From 9d5fd18ea85037e219e10f782254738580807355 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Wed, 18 Aug 2021 13:28:05 -0400 Subject: [PATCH 3/7] u --- superset-frontend/src/reports/actions/reports.js | 1 - superset/reports/commands/base.py | 3 ++- superset/reports/commands/exceptions.py | 11 +++++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/reports/actions/reports.js b/superset-frontend/src/reports/actions/reports.js index adcb8056b0d2b..55cea9dbaa7c9 100644 --- a/superset-frontend/src/reports/actions/reports.js +++ b/superset-frontend/src/reports/actions/reports.js @@ -17,7 +17,6 @@ * under the License. */ /* eslint camelcase: 0 */ -import { ConsoleSqlOutlined } from '@ant-design/icons'; import { t, SupersetClient } from '@superset-ui/core'; import rison from 'rison'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index bb4064d22cde7..163928b20e16a 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -24,6 +24,7 @@ from superset.dashboards.dao import DashboardDAO from superset.reports.commands.exceptions import ( ChartNotFoundValidationError, + ChartNotSavedValidationError, DashboardNotFoundValidationError, ReportScheduleChartOrDashboardValidationError, ) @@ -60,4 +61,4 @@ def validate_chart_dashboard( exceptions.append(DashboardNotFoundValidationError()) self._properties["dashboard"] = dashboard elif not update: - exceptions.append(ReportScheduleChartOrDashboardValidationError()) + exceptions.append(ChartNotSavedValidationError()) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index dfe2402da0449..9080387e2991e 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -80,6 +80,17 @@ def __init__(self) -> None: super().__init__(_("Choose a chart or dashboard not both"), field_name="chart") +class ChartNotSavedValidationError(ValidationError): + """ + Marshmallow validation error for charts that haven't been saved yet + """ + + def __init__(self) -> None: + super().__init__( + _("Chart needs to be saved before creating schedule"), field_name="chart" + ) + + class ReportScheduleInvalidError(CommandInvalidError): message = _("Report Schedule parameters are invalid.") From 627079db13590c0c8a2c004348bfa12861f43efd Mon Sep 17 00:00:00 2001 From: hughhhh Date: Wed, 18 Aug 2021 13:31:45 -0400 Subject: [PATCH 4/7] change print --- superset/reports/commands/exceptions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index 9080387e2991e..dae314d9008c1 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -87,7 +87,8 @@ class ChartNotSavedValidationError(ValidationError): def __init__(self) -> None: super().__init__( - _("Chart needs to be saved before creating schedule"), field_name="chart" + _("Please save your chart first, then try creating a new email report."), + field_name="chart", ) From 9a659d72d3ca8c810bfe1ef1fbfa66b6ee87c1bc Mon Sep 17 00:00:00 2001 From: hughhhh Date: Thu, 19 Aug 2021 06:40:54 -0400 Subject: [PATCH 5/7] update with test --- superset/reports/commands/base.py | 5 +++- tests/integration_tests/reports/api_tests.py | 30 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index 163928b20e16a..a26924cf59dc1 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -60,5 +60,8 @@ def validate_chart_dashboard( if not dashboard: exceptions.append(DashboardNotFoundValidationError()) self._properties["dashboard"] = dashboard - elif not update: + elif dashboard_id is None and not chart_id: + # User hasn't saved the chart in explore yet exceptions.append(ChartNotSavedValidationError()) + elif not update: + exceptions.append(ReportScheduleChartOrDashboardValidationError()) diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 3c306490d1389..aabb2b242ee95 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -734,6 +734,36 @@ def test_create_report_schedule_schema(self): assert data["result"]["timezone"] == "America/Los_Angeles" assert rv.status_code == 201 + @pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_schedules" + ) + def test_unsaved_report_schedule_schema(self): + """ + ReportSchedule Api: Test create report schedule schema check + """ + self.login(username="admin") + chart = db.session.query(Slice).first() + dashboard = db.session.query(Dashboard).first() + example_db = get_example_database() + + # Check that a report does not have a database reference + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name3", + "description": "description", + "creation_method": ReportCreationMethodType.CHARTS, + "crontab": "0 9 * * *", + "chart": 0, + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + assert ( + data["message"]["chart"] + == "Please save your chart first, then try creating a new email report." + ) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_create_report_schedule_chart_dash_validation(self): """ From 82fe7c52429ad60d87c19d46cc538a0851c0aa86 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Thu, 19 Aug 2021 07:12:34 -0400 Subject: [PATCH 6/7] add case for dashboards --- superset/reports/commands/base.py | 14 +++++++--- superset/reports/commands/exceptions.py | 14 ++++++++++ tests/integration_tests/reports/api_tests.py | 27 +++++++++++++++++++- 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index a26924cf59dc1..1e2e288b985c9 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -22,10 +22,12 @@ from superset.charts.dao import ChartDAO from superset.commands.base import BaseCommand from superset.dashboards.dao import DashboardDAO +from superset.models.reports import ReportCreationMethodType from superset.reports.commands.exceptions import ( ChartNotFoundValidationError, ChartNotSavedValidationError, DashboardNotFoundValidationError, + DashboardNotSavedValidationError, ReportScheduleChartOrDashboardValidationError, ) @@ -48,6 +50,15 @@ def validate_chart_dashboard( """ Validate chart or dashboard relation """ chart_id = self._properties.get("chart") dashboard_id = self._properties.get("dashboard") + creation_method = self._properties.get("creation_method") + + if creation_method == ReportCreationMethodType.CHARTS and not chart_id: + # User has not saved chart yet in Explore view + exceptions.append(ChartNotSavedValidationError()) + + if creation_method == ReportCreationMethodType.DASHBOARDS and not dashboard_id: + exceptions.append(DashboardNotSavedValidationError()) + if chart_id and dashboard_id: exceptions.append(ReportScheduleChartOrDashboardValidationError()) if chart_id: @@ -60,8 +71,5 @@ def validate_chart_dashboard( if not dashboard: exceptions.append(DashboardNotFoundValidationError()) self._properties["dashboard"] = dashboard - elif dashboard_id is None and not chart_id: - # User hasn't saved the chart in explore yet - exceptions.append(ChartNotSavedValidationError()) elif not update: exceptions.append(ReportScheduleChartOrDashboardValidationError()) diff --git a/superset/reports/commands/exceptions.py b/superset/reports/commands/exceptions.py index dae314d9008c1..b7d7b433ba952 100644 --- a/superset/reports/commands/exceptions.py +++ b/superset/reports/commands/exceptions.py @@ -92,6 +92,20 @@ def __init__(self) -> None: ) +class DashboardNotSavedValidationError(ValidationError): + """ + Marshmallow validation error for dashboards that haven't been saved yet + """ + + def __init__(self) -> None: + super().__init__( + _( + "Please save your dashboard first, then try creating a new email report." + ), + field_name="dashboard", + ) + + class ReportScheduleInvalidError(CommandInvalidError): message = _("Report Schedule parameters are invalid.") diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index aabb2b242ee95..b2e895db882a5 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -739,7 +739,7 @@ def test_create_report_schedule_schema(self): ) def test_unsaved_report_schedule_schema(self): """ - ReportSchedule Api: Test create report schedule schema check + ReportSchedule Api: Test create report schedule with unsaved chart """ self.login(username="admin") chart = db.session.query(Slice).first() @@ -764,6 +764,31 @@ def test_unsaved_report_schedule_schema(self): == "Please save your chart first, then try creating a new email report." ) + @pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_schedules" + ) + def test_no_dashboard_report_schedule_schema(self): + """ + ReportSchedule Api: Test create report schedule with not dashboard id + """ + self.login(username="admin") + chart = db.session.query(Slice).first() + dashboard = db.session.query(Dashboard).first() + example_db = get_example_database() + + # Check that a report does not have a database reference + report_schedule_data = { + "type": ReportScheduleType.REPORT, + "name": "name3", + "description": "description", + "creation_method": ReportCreationMethodType.DASHBOARDS, + "crontab": "0 9 * * *", + } + uri = "api/v1/report/" + rv = self.client.post(uri, json=report_schedule_data) + data = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 422 + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_create_report_schedule_chart_dash_validation(self): """ From ae5de18740fd0680e64da8c523fef004fe2b1bcc Mon Sep 17 00:00:00 2001 From: hughhhh Date: Thu, 19 Aug 2021 07:50:30 -0400 Subject: [PATCH 7/7] fix test --- superset/reports/commands/base.py | 2 ++ tests/integration_tests/reports/api_tests.py | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/superset/reports/commands/base.py b/superset/reports/commands/base.py index 1e2e288b985c9..3582767ef65f2 100644 --- a/superset/reports/commands/base.py +++ b/superset/reports/commands/base.py @@ -55,9 +55,11 @@ def validate_chart_dashboard( if creation_method == ReportCreationMethodType.CHARTS and not chart_id: # User has not saved chart yet in Explore view exceptions.append(ChartNotSavedValidationError()) + return if creation_method == ReportCreationMethodType.DASHBOARDS and not dashboard_id: exceptions.append(DashboardNotSavedValidationError()) + return if chart_id and dashboard_id: exceptions.append(ReportScheduleChartOrDashboardValidationError()) diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index b2e895db882a5..55fce65162ea9 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -746,7 +746,6 @@ def test_unsaved_report_schedule_schema(self): dashboard = db.session.query(Dashboard).first() example_db = get_example_database() - # Check that a report does not have a database reference report_schedule_data = { "type": ReportScheduleType.REPORT, "name": "name3", @@ -775,8 +774,6 @@ def test_no_dashboard_report_schedule_schema(self): chart = db.session.query(Slice).first() dashboard = db.session.query(Dashboard).first() example_db = get_example_database() - - # Check that a report does not have a database reference report_schedule_data = { "type": ReportScheduleType.REPORT, "name": "name3", @@ -788,6 +785,10 @@ def test_no_dashboard_report_schedule_schema(self): rv = self.client.post(uri, json=report_schedule_data) data = json.loads(rv.data.decode("utf-8")) assert rv.status_code == 422 + assert ( + data["message"]["dashboard"] + == "Please save your dashboard first, then try creating a new email report." + ) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_create_report_schedule_chart_dash_validation(self):