From 7bea441680d6625409442d39018959434c85dec9 Mon Sep 17 00:00:00 2001 From: Euan Date: Mon, 24 Feb 2025 23:01:29 +0100 Subject: [PATCH 1/5] feat(reports) removing index column --- .../features/alerts/AlertReportModal.test.tsx | 13 +++++ .../src/features/alerts/AlertReportModal.tsx | 21 ++++++++ .../src/features/alerts/types.ts | 1 + superset/commands/report/execute.py | 3 ++ superset/commands/report/utils.py | 32 +++++++++++++ ...bc_add_include_index_to_report_schedule.py | 41 ++++++++++++++++ superset/reports/api.py | 2 + superset/reports/models.py | 3 ++ superset/reports/schemas.py | 16 ++++++- .../commands/report/test_report_utils.py | 48 +++++++++++++++++++ 10 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 superset/commands/report/utils.py create mode 100644 superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py create mode 100644 tests/unit_tests/commands/report/test_report_utils.py diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index f526b465c1edb..43a75c94860f4 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -438,6 +438,19 @@ test('renders tab selection when Dashboard is selected', async () => { expect(screen.getByText(/select tab/i)).toBeInTheDocument(); }); +test('properly renders include index checkbox', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test chart/i); + expect( + screen.getByRole('checkbox', { + name: /include index column/i, + }), + ).toBeInTheDocument(); +}); + test('changes to content options when chart is selected', async () => { render(, { useRedux: true, diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index f230a02b52fbc..b0504a5ecd2d5 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -442,6 +442,7 @@ const AlertReportModal: FunctionComponent = ({ DEFAULT_NOTIFICATION_FORMAT, ); const [forceScreenshot, setForceScreenshot] = useState(false); + const [includeIndex, setIncludeIndex] = useState(false); const [isScreenshot, setIsScreenshot] = useState(false); useEffect(() => { @@ -669,6 +670,7 @@ const AlertReportModal: FunctionComponent = ({ ...currentAlert, type: isReport ? 'Report' : 'Alert', force_screenshot: shouldEnableForceScreenshot || forceScreenshot, + include_index: includeIndex, validator_type: conditionNotNull ? 'not null' : 'operator', validator_config_json: conditionNotNull ? {} @@ -1119,6 +1121,10 @@ const AlertReportModal: FunctionComponent = ({ setForceScreenshot(event.target.checked); }; + const onIncludeIndexChange = (event: any) => { + setIncludeIndex(event.target.checked); + }; + // Make sure notification settings has the required info const checkNotificationSettings = () => { if (!notificationSettings.length) { @@ -1339,6 +1345,7 @@ const AlertReportModal: FunctionComponent = ({ setChartVizType((resource.chart as ChartObject).viz_type); } setForceScreenshot(resource.force_screenshot); + setIncludeIndex(resource.include_index || false); setCurrentAlert({ ...resource, @@ -1810,6 +1817,20 @@ const AlertReportModal: FunctionComponent = ({ )} + {isReport && + contentType === ContentType.Chart && + reportFormat === 'CSV' && ( +
+ + {t('Include index column')} + +
+ )} bytes: def _get_csv_data(self) -> bytes: url = self._get_url(result_format=ChartDataResultFormat.CSV) + if not self._report_schedule.include_index: + url = remove_post_processed(url) _, username = get_executor( executors=app.config["ALERT_REPORTS_EXECUTORS"], model=self._report_schedule, diff --git a/superset/commands/report/utils.py b/superset/commands/report/utils.py new file mode 100644 index 0000000000000..779ac8efb8297 --- /dev/null +++ b/superset/commands/report/utils.py @@ -0,0 +1,32 @@ +# 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. + + +def remove_post_processed(url: str) -> str: + """Remove the type=post_processed parameter from the URL query string. + Args: + url (str): The URL to process. + Returns: + str: The URL with the type=post_processed parameter removed.""" + if "?" not in url: + return url + base_url, query_string = url.split("?", 1) + params = query_string.split("&") + filtered_params = [param for param in params if param != "type=post_processed"] + filtered_query_string = "&".join(filtered_params) + filtered_url = f"{base_url}?{filtered_query_string}" + return filtered_url diff --git a/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py new file mode 100644 index 0000000000000..f181dce48c579 --- /dev/null +++ b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py @@ -0,0 +1,41 @@ +# 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. +"""add_include_index_to_report_schedule + +Revision ID: 3b86f563edbc +Revises: 74ad1125881c +Create Date: 2025-02-24 17:52:02.369467 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "3b86f563edbc" +down_revision = "74ad1125881c" + + +def upgrade(): + op.add_column( + "report_schedule", + sa.Column("include_index", sa.Boolean(), nullable=True), + ) + + +def downgrade(): + op.drop_column("report_schedule", "include_index") diff --git a/superset/reports/api.py b/superset/reports/api.py index 320eb97c05133..de6eb62c5c014 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -104,6 +104,7 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "extra", "force_screenshot", "grace_period", + "include_index", "last_eval_dttm", "last_state", "last_value", @@ -170,6 +171,7 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "extra", "force_screenshot", "grace_period", + "include_index", "log_retention", "name", "owners", diff --git a/superset/reports/models.py b/superset/reports/models.py index e4cdd7c9b4d0e..8aca855b98735 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -169,6 +169,9 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): # (Reports) When generating a screenshot, bypass the cache? force_screenshot = Column(Boolean, default=False) + # (Reports) Include index column in report + include_index = Column(Boolean, default=False) + custom_width = Column(Integer, nullable=True) custom_height = Column(Integer, nullable=True) diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 7078970b3815b..b397d54875cae 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -233,6 +233,13 @@ class ReportSchedulePostSchema(Schema): dump_default=None, ) force_screenshot = fields.Boolean(dump_default=False) + include_index = fields.Boolean( + metadata={ + "description": _("Include index column in report"), + "example": False, + }, + dump_default=False, + ) custom_width = fields.Integer( metadata={ "description": _("Custom width of the screenshot in pixels"), @@ -370,7 +377,14 @@ class ReportSchedulePutSchema(Schema): ) extra = fields.Dict(dump_default=None) force_screenshot = fields.Boolean(dump_default=False) - + include_index = fields.Boolean( + metadata={ + "description": _("Include index column in report"), + "example": False, + }, + dump_default=False, + required=False, + ) custom_width = fields.Integer( metadata={ "description": _("Custom width of the screenshot in pixels"), diff --git a/tests/unit_tests/commands/report/test_report_utils.py b/tests/unit_tests/commands/report/test_report_utils.py new file mode 100644 index 0000000000000..1313af047ca77 --- /dev/null +++ b/tests/unit_tests/commands/report/test_report_utils.py @@ -0,0 +1,48 @@ +# 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. + +from superset.commands.report.utils import remove_post_processed + + +def test_remove_post_processed(): + url = "https://superset.com/?param1=value1&type=post_processed¶m2=value2" + expected = "https://superset.com/?param1=value1¶m2=value2" + assert remove_post_processed(url) == expected + + +def test_retain_other_parameters(): + url = "https://superset.com/?param1=value1¶m2=value2" + expected = "https://superset.com/?param1=value1¶m2=value2" + assert remove_post_processed(url) == expected + + +def test_no_post_processed_present(): + url = "https://superset.com/?param1=value1¶m2=value2" + expected = "https://superset.com/?param1=value1¶m2=value2" + assert remove_post_processed(url) == expected + + +def test_empty_query_string(): + url = "https://superset.com/?" + expected = "https://superset.com/?" + assert remove_post_processed(url) == expected + + +def test_no_query_string(): + url = "https://superset.com" + expected = "https://superset.com" + assert remove_post_processed(url) == expected From a62c6b5a6ef4f2faa3985f0936254735a543ce74 Mon Sep 17 00:00:00 2001 From: Euan Date: Wed, 26 Feb 2025 10:23:33 +0100 Subject: [PATCH 2/5] setting the default csv index column behaviour to current behavior --- .../features/alerts/AlertReportModal.test.tsx | 38 ++++++++++++------- .../src/features/alerts/AlertReportModal.tsx | 15 ++++---- .../src/features/alerts/types.ts | 2 +- superset/commands/report/execute.py | 5 ++- ...bc_add_remove_index_to_report_schedule.py} | 4 +- superset/reports/api.py | 4 +- superset/reports/models.py | 4 +- superset/reports/schemas.py | 8 ++-- 8 files changed, 47 insertions(+), 33 deletions(-) rename superset/migrations/versions/{2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py => 2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py} (90%) diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index 43a75c94860f4..33ef28bbbd0a1 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -438,19 +438,6 @@ test('renders tab selection when Dashboard is selected', async () => { expect(screen.getByText(/select tab/i)).toBeInTheDocument(); }); -test('properly renders include index checkbox', async () => { - render(, { - useRedux: true, - }); - userEvent.click(screen.getByTestId('contents-panel')); - await screen.findByText(/test chart/i); - expect( - screen.getByRole('checkbox', { - name: /include index column/i, - }), - ).toBeInTheDocument(); -}); - test('changes to content options when chart is selected', async () => { render(, { useRedux: true, @@ -519,6 +506,31 @@ test('does not show screenshot width when csv is selected', async () => { expect(screen.queryByRole('spinbutton')).not.toBeInTheDocument(); }); +test('properly renders remove index checkbox for CSV reports', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); + const contentTypeSelector = screen.getByRole('combobox', { + name: /select content type/i, + }); + await comboboxSelect(contentTypeSelector, 'Chart', () => + screen.getByRole('combobox', { name: /chart/i }), + ); + const reportFormatSelector = screen.getByRole('combobox', { + name: /select format/i, + }); + await comboboxSelect( + reportFormatSelector, + 'CSV', + () => screen.getAllByText(/Send as CSV/i)[0], + ); + expect( + screen.getByRole('checkbox', { name: /remove index column/i }), + ).toBeInTheDocument(); +}); + // Schedule Section test('opens Schedule Section on click', async () => { render(, { diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index b0504a5ecd2d5..d89a2de8cda00 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -442,7 +442,7 @@ const AlertReportModal: FunctionComponent = ({ DEFAULT_NOTIFICATION_FORMAT, ); const [forceScreenshot, setForceScreenshot] = useState(false); - const [includeIndex, setIncludeIndex] = useState(false); + const [removeIndex, setRemoveIndex] = useState(false); const [isScreenshot, setIsScreenshot] = useState(false); useEffect(() => { @@ -670,7 +670,7 @@ const AlertReportModal: FunctionComponent = ({ ...currentAlert, type: isReport ? 'Report' : 'Alert', force_screenshot: shouldEnableForceScreenshot || forceScreenshot, - include_index: includeIndex, + remove_index: removeIndex, validator_type: conditionNotNull ? 'not null' : 'operator', validator_config_json: conditionNotNull ? {} @@ -1121,8 +1121,8 @@ const AlertReportModal: FunctionComponent = ({ setForceScreenshot(event.target.checked); }; - const onIncludeIndexChange = (event: any) => { - setIncludeIndex(event.target.checked); + const onRemoveIndexChange = (event: any) => { + setRemoveIndex(event.target.checked); }; // Make sure notification settings has the required info @@ -1345,7 +1345,6 @@ const AlertReportModal: FunctionComponent = ({ setChartVizType((resource.chart as ChartObject).viz_type); } setForceScreenshot(resource.force_screenshot); - setIncludeIndex(resource.include_index || false); setCurrentAlert({ ...resource, @@ -1824,10 +1823,10 @@ const AlertReportModal: FunctionComponent = ({ - {t('Include index column')} + {t('Remove index column')} )} diff --git a/superset-frontend/src/features/alerts/types.ts b/superset-frontend/src/features/alerts/types.ts index 5777197467267..bafb3a8e782bf 100644 --- a/superset-frontend/src/features/alerts/types.ts +++ b/superset-frontend/src/features/alerts/types.ts @@ -119,7 +119,7 @@ export type AlertObject = { error?: string; extra?: Extra; force_screenshot: boolean; - include_index?: boolean; + remove_index?: boolean; grace_period?: number; id: number; last_eval_dttm?: number; diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 6b0a880dd6d5f..4e69af0ba67f8 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -363,9 +363,12 @@ def _get_pdf(self) -> bytes: return pdf def _get_csv_data(self) -> bytes: + logger.info("Getting csv data for report %s", self._report_schedule.name) url = self._get_url(result_format=ChartDataResultFormat.CSV) - if not self._report_schedule.include_index: + logger.info("URL: %s", url) + if self._report_schedule.remove_index: url = remove_post_processed(url) + logger.info("URL after removing post processed: %s", url) _, username = get_executor( executors=app.config["ALERT_REPORTS_EXECUTORS"], model=self._report_schedule, diff --git a/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py similarity index 90% rename from superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py rename to superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py index f181dce48c579..3db5cb93e7470 100644 --- a/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_include_index_to_report_schedule.py +++ b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py @@ -33,9 +33,9 @@ def upgrade(): op.add_column( "report_schedule", - sa.Column("include_index", sa.Boolean(), nullable=True), + sa.Column("remove_index", sa.Boolean(), default=False), ) def downgrade(): - op.drop_column("report_schedule", "include_index") + op.drop_column("report_schedule", "remove_index") diff --git a/superset/reports/api.py b/superset/reports/api.py index de6eb62c5c014..1cdee1e5986f3 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -104,7 +104,6 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "extra", "force_screenshot", "grace_period", - "include_index", "last_eval_dttm", "last_state", "last_value", @@ -117,6 +116,7 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "recipients.id", "recipients.recipient_config_json", "recipients.type", + "remove_index", "report_format", "sql", "timezone", @@ -171,12 +171,12 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: "extra", "force_screenshot", "grace_period", - "include_index", "log_retention", "name", "owners", "recipients", "report_format", + "remove_index", "sql", "timezone", "type", diff --git a/superset/reports/models.py b/superset/reports/models.py index 8aca855b98735..cb51fc9a74e15 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -169,8 +169,8 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): # (Reports) When generating a screenshot, bypass the cache? force_screenshot = Column(Boolean, default=False) - # (Reports) Include index column in report - include_index = Column(Boolean, default=False) + # (Reports) Remove index column in report + remove_index = Column(Boolean, default=False) custom_width = Column(Integer, nullable=True) custom_height = Column(Integer, nullable=True) diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index b397d54875cae..574fe5bd1c1d4 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -233,9 +233,9 @@ class ReportSchedulePostSchema(Schema): dump_default=None, ) force_screenshot = fields.Boolean(dump_default=False) - include_index = fields.Boolean( + remove_index = fields.Boolean( metadata={ - "description": _("Include index column in report"), + "description": _("Remove index column in report"), "example": False, }, dump_default=False, @@ -377,9 +377,9 @@ class ReportSchedulePutSchema(Schema): ) extra = fields.Dict(dump_default=None) force_screenshot = fields.Boolean(dump_default=False) - include_index = fields.Boolean( + remove_index = fields.Boolean( metadata={ - "description": _("Include index column in report"), + "description": _("Remove index column in report"), "example": False, }, dump_default=False, From 2605a06d7f227545215f01aa34dcc60aca1a5a83 Mon Sep 17 00:00:00 2001 From: Euan Date: Wed, 26 Feb 2025 10:27:40 +0100 Subject: [PATCH 3/5] removing redundant logging --- superset/commands/report/execute.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 4e69af0ba67f8..f84ba5919cd76 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -363,12 +363,9 @@ def _get_pdf(self) -> bytes: return pdf def _get_csv_data(self) -> bytes: - logger.info("Getting csv data for report %s", self._report_schedule.name) url = self._get_url(result_format=ChartDataResultFormat.CSV) - logger.info("URL: %s", url) if self._report_schedule.remove_index: url = remove_post_processed(url) - logger.info("URL after removing post processed: %s", url) _, username = get_executor( executors=app.config["ALERT_REPORTS_EXECUTORS"], model=self._report_schedule, From d960fe76e3696bdf181ba2e3a2097028fdd2b9b6 Mon Sep 17 00:00:00 2001 From: Euan Date: Wed, 26 Feb 2025 10:31:31 +0100 Subject: [PATCH 4/5] updating migration title --- ...24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py index 3db5cb93e7470..270716ab42a32 100644 --- a/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py +++ b/superset/migrations/versions/2025-02-24_17-52_3b86f563edbc_add_remove_index_to_report_schedule.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""add_include_index_to_report_schedule +"""add_remove_index_to_report_schedule Revision ID: 3b86f563edbc Revises: 74ad1125881c From 29d18d433c53f26bdb727fe79c8edbedc984ee7f Mon Sep 17 00:00:00 2001 From: Euan Date: Wed, 26 Feb 2025 10:48:47 +0100 Subject: [PATCH 5/5] updated remove index test --- superset-frontend/src/features/alerts/AlertReportModal.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index 33ef28bbbd0a1..d015f21b9d7d2 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -507,7 +507,7 @@ test('does not show screenshot width when csv is selected', async () => { }); test('properly renders remove index checkbox for CSV reports', async () => { - render(, { + render(, { useRedux: true, }); userEvent.click(screen.getByTestId('contents-panel'));