From bd03440ac8781eef409ea85a96c05d09eab004b1 Mon Sep 17 00:00:00 2001 From: Bart Skowron Date: Sat, 8 Aug 2026 00:05:16 -0400 Subject: [PATCH] feat(alerts-reports): add per-schedule toggle to include/exclude the Explore in Superset link (#42494) Co-authored-by: Claude Fable 5 --- .github/workflows/codeql-analysis.yml | 4 +- .github/workflows/generate-FOSSA-report.yml | 2 +- .github/workflows/license-check.yml | 2 +- .github/workflows/superset-docs-deploy.yml | 2 +- .../configuration/alerts-reports.mdx | 1 + .../features/alerts/AlertReportModal.test.tsx | 141 ++++++++++++ .../src/features/alerts/AlertReportModal.tsx | 18 ++ .../src/features/alerts/types.ts | 1 + superset/commands/report/execute.py | 11 +- ...4af6_add_include_cta_to_report_schedule.py | 54 +++++ superset/reports/api.py | 2 + superset/reports/models.py | 4 + superset/reports/notifications/base.py | 1 + superset/reports/notifications/email.py | 41 +++- superset/reports/notifications/slack_mixin.py | 18 +- superset/reports/schemas.py | 14 ++ tests/integration_tests/reports/api_tests.py | 9 + .../reports/notifications/email_tests.py | 200 ++++++++++++++++++ .../reports/notifications/slack_tests.py | 98 +++++++++ tests/unit_tests/reports/schemas_test.py | 49 +++++ 20 files changed, 653 insertions(+), 19 deletions(-) create mode 100644 superset/migrations/versions/2026-07-28_10-00_2d6ad72e4af6_add_include_cta_to_report_schedule.py diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index f408eb7e959..951272bb11f 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -64,7 +64,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@f205ea1c3313d32999d8d6a48b4f6530d4437b38 # v4.37.4 + uses: github/codeql-action/init@e4fba868fa4b1b91e1fdab776edc8cfbe6e9fb81 # v4.37.3 with: languages: ${{ matrix.language }} # If you wish to specify custom queries, you can do so here or in a config file. @@ -75,6 +75,6 @@ jobs: # queries: security-extended,security-and-quality - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@f205ea1c3313d32999d8d6a48b4f6530d4437b38 # v4.37.4 + uses: github/codeql-action/analyze@e4fba868fa4b1b91e1fdab776edc8cfbe6e9fb81 # v4.37.3 with: category: "/language:${{matrix.language}}" diff --git a/.github/workflows/generate-FOSSA-report.yml b/.github/workflows/generate-FOSSA-report.yml index 72ddb0fa09e..afd57875c91 100644 --- a/.github/workflows/generate-FOSSA-report.yml +++ b/.github/workflows/generate-FOSSA-report.yml @@ -37,7 +37,7 @@ jobs: persist-credentials: false submodules: recursive - name: Setup Java - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 + uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5.6.0 with: distribution: "temurin" java-version: "11" diff --git a/.github/workflows/license-check.yml b/.github/workflows/license-check.yml index 0e2e2dc6786..31056d01d45 100644 --- a/.github/workflows/license-check.yml +++ b/.github/workflows/license-check.yml @@ -23,7 +23,7 @@ jobs: persist-credentials: false submodules: recursive - name: Setup Java - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 + uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5.6.0 with: distribution: "temurin" java-version: "11" diff --git a/.github/workflows/superset-docs-deploy.yml b/.github/workflows/superset-docs-deploy.yml index 9d8a9dcbf55..b55f3bd66cc 100644 --- a/.github/workflows/superset-docs-deploy.yml +++ b/.github/workflows/superset-docs-deploy.yml @@ -76,7 +76,7 @@ jobs: node-version-file: "./docs/.nvmrc" - name: Setup Python uses: ./.github/actions/setup-backend/ - - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 + - uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5.6.0 with: distribution: "zulu" java-version: "21" diff --git a/docs/admin_docs/configuration/alerts-reports.mdx b/docs/admin_docs/configuration/alerts-reports.mdx index e2262bcc584..9aa6a40f303 100644 --- a/docs/admin_docs/configuration/alerts-reports.mdx +++ b/docs/admin_docs/configuration/alerts-reports.mdx @@ -28,6 +28,7 @@ Alerts and reports are disabled by default. To turn them on, you'll need to chan - Users can customize the email subject by including date code placeholders, which will automatically be replaced with the corresponding UTC date when the email is sent. To enable this functionality, activate the `"DATE_FORMAT_IN_EMAIL_SUBJECT"` [feature flag](/admin-docs/configuration/configuring-superset#feature-flags). This enables date formatting in email subjects, preventing all reporting emails from being grouped into the same thread (optional for the reporting feature). - Use date codes from [strftime.org](https://strftime.org/) to create the email subject. - If no date code is provided, the original string will be used as the email subject. +- Each alert/report has an "Include a link back to Superset" option (enabled by default) controlling whether the call-to-action link is included in email and Slack notifications. The link text in emails is configurable via `EMAIL_REPORTS_CTA`; the Slack message always uses "Explore in Superset". Uncheck the option when recipients should not receive a link to your Superset host, e.g. for external audiences. ##### Disable dry-run mode diff --git a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx index 342209cf0c3..d1e6402af3f 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.test.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.test.tsx @@ -88,6 +88,7 @@ const generateMockPayload = (dashboard = true) => { force_screenshot: true, grace_period: 14400, id: 1, + include_cta: true, last_eval_dttm: null, last_state: 'Not triggered', last_value: null, @@ -308,6 +309,9 @@ afterEach(() => { 'put-dashboard-payload', 'put-report-1', 'put-no-recipients', + 'put-include-cta', + 'get-report-cta-false', + 'get-report-cta-absent', 'tabs-99', ]) { try { @@ -336,6 +340,7 @@ const validAlert: AlertObject = { dashboard_id: 0, chart_id: 1, force_screenshot: false, + include_cta: true, last_state: 'Not triggered', name: 'Test Alert', editors: [mockEditorSubject], @@ -729,6 +734,90 @@ test('removes ignore cache checkbox when chart is selected', async () => { ).not.toBeInTheDocument(); }); +test('renders include link checkbox checked by default in create mode', async () => { + render(, { + useRedux: true, + }); + userEvent.click(screen.getByTestId('contents-panel')); + const checkbox = await screen.findByRole('checkbox', { + name: /include a link back to superset/i, + }); + expect(checkbox).toBeChecked(); +}); + +test('keeps include link checkbox when chart is selected', 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.getAllByText(/select chart/i)[0], + ); + expect( + screen.getByRole('checkbox', { + name: /include a link back to superset/i, + }), + ).toBeInTheDocument(); +}); + +test('hydrates include link checkbox from a resource with include_cta false', async () => { + fetchMock.get( + 'glob:*/api/v1/report/8', + { result: { ...generateMockPayload(true), id: 8, include_cta: false } }, + { name: 'get-report-cta-false' }, + ); + + render( + , + { useRedux: true }, + ); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); + expect( + screen.getByRole('checkbox', { + name: /include a link back to superset/i, + }), + ).not.toBeChecked(); + + fetchMock.removeRoute('get-report-cta-false'); +}); + +test('treats a resource without include_cta as checked', async () => { + const { include_cta: _include_cta, ...payloadWithoutCta } = + generateMockPayload(true); + fetchMock.get( + 'glob:*/api/v1/report/9', + { result: { ...payloadWithoutCta, id: 9 } }, + { name: 'get-report-cta-absent' }, + ); + + render( + , + { useRedux: true }, + ); + userEvent.click(screen.getByTestId('contents-panel')); + await screen.findByText(/test dashboard/i); + expect( + screen.getByRole('checkbox', { + name: /include a link back to superset/i, + }), + ).toBeChecked(); + + fetchMock.removeRoute('get-report-cta-absent'); +}); + test('open chart button opens explore with slice_id', async () => { // Render with an existing alert that has a chart selected render(, { @@ -1550,6 +1639,58 @@ test('submit includes conditionNotNull without threshold in alert payload', asyn fetchMock.removeRoute('put-condition'); }, 45000); +test('submit includes include_cta false after unchecking the checkbox', async () => { + // Mock payload returns id:1, so updateResource PUTs to /api/v1/report/1 + fetchMock.put( + 'glob:*/api/v1/report/1', + { id: 1, result: {} }, + { name: 'put-include-cta' }, + ); + + render(, { + useRedux: true, + }); + + // Wait for resource to load and all validation to pass + await waitFor( + () => { + expect( + screen.queryAllByRole('img', { name: /check-circle/i }), + ).toHaveLength(5); + }, + { timeout: 10000 }, + ); + + // Open the contents panel and uncheck the include link checkbox + userEvent.click(screen.getByTestId('contents-panel')); + const checkbox = await screen.findByRole('checkbox', { + name: /include a link back to superset/i, + }); + expect(checkbox).toBeChecked(); + userEvent.click(checkbox); + await waitFor(() => { + expect(checkbox).not.toBeChecked(); + }); + + // Wait for Save to be enabled and click + await waitFor(() => { + expect(screen.getByRole('button', { name: /save/i })).toBeEnabled(); + }); + userEvent.click(screen.getByRole('button', { name: /save/i })); + + // Verify the PUT payload + await waitFor(() => { + const calls = fetchMock.callHistory.calls('put-include-cta'); + expect(calls.length).toBeGreaterThan(0); + }); + + const calls = fetchMock.callHistory.calls('put-include-cta'); + const body = JSON.parse(calls[calls.length - 1].options.body as string); + expect(body.include_cta).toBe(false); + + fetchMock.removeRoute('put-include-cta'); +}, 45000); + test('edit mode submit uses PUT and excludes read-only fields', async () => { // Mock payload returns id:1, so updateResource PUTs to /api/v1/report/1 fetchMock.put( diff --git a/superset-frontend/src/features/alerts/AlertReportModal.tsx b/superset-frontend/src/features/alerts/AlertReportModal.tsx index 1f4968236e7..77b58f25a37 100644 --- a/superset-frontend/src/features/alerts/AlertReportModal.tsx +++ b/superset-frontend/src/features/alerts/AlertReportModal.tsx @@ -721,6 +721,7 @@ const AlertReportModal: FunctionComponent = ({ validator_config_json: {}, validator_type: '', force_screenshot: false, + include_cta: true, grace_period: undefined, retry_on_failure: false, retry_max_attempts: 3, @@ -963,6 +964,7 @@ const AlertReportModal: FunctionComponent = ({ ...currentAlert, type: isReport ? 'Report' : 'Alert', force_screenshot: shouldEnableForceScreenshot || forceScreenshot, + include_cta: currentAlert?.include_cta ?? true, validator_type: conditionNotNull ? 'not null' : 'operator', validator_config_json: conditionNotNull ? {} @@ -2611,6 +2613,22 @@ const AlertReportModal: FunctionComponent = ({ )} +
+ + updateAlertState('include_cta', e.target.checked) + } + > + {t('Include a link back to Superset')} + + +
), }, diff --git a/superset-frontend/src/features/alerts/types.ts b/superset-frontend/src/features/alerts/types.ts index f49890e075c..a9d854beb3a 100644 --- a/superset-frontend/src/features/alerts/types.ts +++ b/superset-frontend/src/features/alerts/types.ts @@ -135,6 +135,7 @@ export type AlertObject = { force_screenshot: boolean; grace_period?: number; id: number; + include_cta?: boolean; last_eval_dttm?: number; last_state?: | 'Success' diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 7efe14820e9..81789f2a7c9 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -1243,6 +1243,8 @@ class BaseReportState: error_text = None header_data = self._get_log_data() url = self._get_url(user_friendly=True) + # NULL (rows predating the include_cta column) is treated as True + include_cta = self._report_schedule.include_cta is not False if ( feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS") @@ -1278,6 +1280,7 @@ class BaseReportState: text=error_text, header_data=header_data, url=url, + include_cta=include_cta, ) if ( @@ -1310,6 +1313,7 @@ class BaseReportState: xlsx=xlsx_data, embedded_data=embedded_data, header_data=header_data, + include_cta=include_cta, ) def _send( @@ -1414,7 +1418,12 @@ class BaseReportState: self._execution_id, ) notification_content = NotificationContent( - name=sanitize_title(name), text=message, header_data=header_data, url=url + name=sanitize_title(name), + text=message, + header_data=header_data, + url=url, + # NULL (rows predating the include_cta column) is treated as True + include_cta=self._report_schedule.include_cta is not False, ) # filter recipients to recipients who are also editors diff --git a/superset/migrations/versions/2026-07-28_10-00_2d6ad72e4af6_add_include_cta_to_report_schedule.py b/superset/migrations/versions/2026-07-28_10-00_2d6ad72e4af6_add_include_cta_to_report_schedule.py new file mode 100644 index 00000000000..34f2bc512f3 --- /dev/null +++ b/superset/migrations/versions/2026-07-28_10-00_2d6ad72e4af6_add_include_cta_to_report_schedule.py @@ -0,0 +1,54 @@ +# 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_cta to report_schedule + +Adds a nullable ``include_cta`` column to the ``report_schedule`` table. It controls +whether the call-to-action link back to Superset (e.g. "Explore in Superset") is +included in the notifications delivered for that schedule. The column defaults to +true and NULL is treated as true, so existing schedules keep the link. + +Revision ID: 2d6ad72e4af6 +Revises: 1a27941d5352 +Create Date: 2026-07-28 10:00:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "2d6ad72e4af6" +down_revision = "b8d2f4a6c901" + + +def upgrade() -> None: + """Add the nullable ``include_cta`` column to ``report_schedule``.""" + add_columns( + "report_schedule", + sa.Column( + "include_cta", + sa.Boolean(), + nullable=True, + server_default=sa.true(), + ), + ) + + +def downgrade() -> None: + """Drop the ``include_cta`` column from ``report_schedule``.""" + drop_columns("report_schedule", "include_cta") diff --git a/superset/reports/api.py b/superset/reports/api.py index 4eb28964d35..61178e87aae 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -127,6 +127,7 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi): "extra", "force_screenshot", "grace_period", + "include_cta", "last_eval_dttm", "last_state", "last_value", @@ -204,6 +205,7 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi): "extra", "force_screenshot", "grace_period", + "include_cta", "log_retention", "name", "recipients", diff --git a/superset/reports/models.py b/superset/reports/models.py index 0e67e254664..1510ea63782 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -188,6 +188,10 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): email_subject = Column(String(255)) + # (Alerts/Reports) Include the call-to-action link back to Superset in + # notifications? NULL is treated as True. + include_cta = Column(Boolean, default=True, nullable=True) + def __repr__(self) -> str: return str(self.name) diff --git a/superset/reports/notifications/base.py b/superset/reports/notifications/base.py index 4a99c0ef33a..5dc2d44c09a 100644 --- a/superset/reports/notifications/base.py +++ b/superset/reports/notifications/base.py @@ -38,6 +38,7 @@ class NotificationContent: # Populated only when this is a per-retry or final-failure notification retry_attempt: Optional[int] = None retry_max_attempts: Optional[int] = None + include_cta: bool = True # include the call-to-action link back to Superset class BaseNotification: # pylint: disable=too-few-public-methods diff --git a/superset/reports/notifications/email.py b/superset/reports/notifications/email.py index e8515848851..5f1687e872f 100644 --- a/superset/reports/notifications/email.py +++ b/superset/reports/notifications/email.py @@ -148,22 +148,29 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met return parseaddr(current_app.config["SMTP_MAIL_FROM"])[1].split("@")[1] def _error_template(self, text: str) -> str: - call_to_action = self._get_call_to_action() # The error text is derived from exception messages that can embed # data-controlled content (e.g. crafted table/column names in a DB # error). Strip all HTML before interpolating it into the email body, # matching the sanitization applied to the normal content path. # pylint: disable=no-member safe_text = nh3.clean(text, tags=set(), attributes={}) + if self._content.include_cta: + return __( + """ +

Your report/alert was unable to be generated because of the following error: %(text)s

+

Please check your dashboard/chart for errors.

+

%(call_to_action)s

+ """, # noqa: E501 + text=safe_text, + url=self._content.url, + call_to_action=self._get_call_to_action(), + ) return __( """

Your report/alert was unable to be generated because of the following error: %(text)s

Please check your dashboard/chart for errors.

-

%(call_to_action)s

""", # noqa: E501 text=safe_text, - url=self._content.url, - call_to_action=call_to_action, ) def _retry_error_template(self, text: str) -> str: @@ -173,7 +180,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met retries_remaining = (max_attempts or 0) - (attempt or 0) # pylint: disable=no-member safe_text = nh3.clean(text, tags=set(), attributes={}) - call_to_action = self._get_call_to_action() + cta_tag = self._render_call_to_action_paragraph() return textwrap.dedent( f""" @@ -199,7 +206,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met

Retry attempt: {attempt} of {max_attempts}    Retries remaining: {retries_remaining}

Error details: {safe_text}

-

{call_to_action}

+ {cta_tag} """ @@ -209,7 +216,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met """HTML body for the final-failure email after all retries are exhausted.""" # pylint: disable=no-member safe_text = nh3.clean(text, tags=set(), attributes={}) - call_to_action = self._get_call_to_action() + cta_tag = self._render_call_to_action_paragraph() max_attempts = self._content.retry_max_attempts return textwrap.dedent( @@ -246,7 +253,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met
  • Try generating the report manually to troubleshoot
  • Contact support if the issue persists
    • -

    {call_to_action}

    + {cta_tag} """ @@ -312,7 +319,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met """ ) img_tag = "".join(img_tags) - call_to_action = self._get_call_to_action() + call_to_action_tag = self._render_call_to_action_tag() body = textwrap.dedent( f""" @@ -333,7 +340,7 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met
    {description}

    - {call_to_action}

    + {call_to_action_tag} {html_table} {img_tag} @@ -404,6 +411,20 @@ class EmailNotification(BaseNotification): # pylint: disable=too-few-public-met def _get_call_to_action(self) -> str: return __(current_app.config["EMAIL_REPORTS_CTA"]) + def _render_call_to_action_tag(self) -> str: + """Anchor markup for the call-to-action link, or "" when disabled.""" + if not self._content.include_cta: + return "" + call_to_action = self._get_call_to_action() + return f'{call_to_action}

    ' + + def _render_call_to_action_paragraph(self) -> str: + """CTA link wrapped in a paragraph, or "" when disabled.""" + if not self._content.include_cta: + return "" + call_to_action = self._get_call_to_action() + return f'

    {call_to_action}

    ' + def _get_to(self) -> str: return json.loads(self._recipient.recipient_config_json)["target"] diff --git a/superset/reports/notifications/slack_mixin.py b/superset/reports/notifications/slack_mixin.py index 3b696730657..f85924a2d71 100644 --- a/superset/reports/notifications/slack_mixin.py +++ b/superset/reports/notifications/slack_mixin.py @@ -31,18 +31,30 @@ class SlackMixin: content: NotificationContent, table: str = "", ) -> str: - return __( - """*%(name)s* + if content.include_cta: + return __( + """*%(name)s* %(description)s <%(url)s|Explore in Superset> +%(table)s +""", + name=content.name, + description=content.description or "", + url=content.url, + table=table, + ) + return __( + """*%(name)s* + +%(description)s + %(table)s """, name=content.name, description=content.description or "", - url=content.url, table=table, ) diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index ae38f447d67..071870d07ee 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -74,6 +74,10 @@ name_description = "The report schedule name." # :) description_description = "Use a nice description to give context to this Alert/Report" email_subject_description = "The report schedule subject line" +include_cta_description = ( + "Whether to include the call-to-action link back to Superset " + "(e.g. 'Explore in Superset') in the delivered notifications" +) context_markdown_description = "Markdown description" crontab_description = ( "A CRON expression." @@ -269,6 +273,11 @@ class ReportSchedulePostSchema(Schema): dump_default=None, ) force_screenshot = fields.Boolean(dump_default=False) + include_cta = fields.Boolean( + dump_default=True, + allow_none=True, + metadata={"description": include_cta_description}, + ) custom_width = fields.Integer( metadata={ "description": _("Custom width of the screenshot in pixels"), @@ -482,6 +491,11 @@ class ReportSchedulePutSchema(Schema): ) extra = fields.Dict(dump_default=None) force_screenshot = fields.Boolean(dump_default=False) + include_cta = fields.Boolean( + dump_default=True, + allow_none=True, + metadata={"description": include_cta_description}, + ) custom_width = fields.Integer( metadata={ diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 23585c1793f..9b6c629e58c 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -619,6 +619,7 @@ class TestReportSchedulesApi(SupersetTestCase): "working_timeout": 3600, "chart": chart.id, "database": example_db.id, + "include_cta": False, } uri = "api/v1/report/" rv = self.post_assert_metric(uri, report_schedule_data, "post") @@ -634,6 +635,7 @@ class TestReportSchedulesApi(SupersetTestCase): assert created_model.chart.id == report_schedule_data["chart"] assert created_model.database.id == report_schedule_data["database"] assert created_model.creation_method == report_schedule_data["creation_method"] + assert created_model.include_cta is False # Rollback changes db.session.delete(created_model) db.session.commit() @@ -1515,6 +1517,7 @@ class TestReportSchedulesApi(SupersetTestCase): ], "chart": chart.id, "database": example_db.id, + "include_cta": False, } uri = f"api/v1/report/{report_schedule.id}" @@ -1529,6 +1532,12 @@ class TestReportSchedulesApi(SupersetTestCase): assert updated_model.crontab == report_schedule_data["crontab"] assert updated_model.chart_id == report_schedule_data["chart"] assert updated_model.database_id == report_schedule_data["database"] + assert updated_model.include_cta is False + + rv = self.client.get(uri) + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert data["result"]["include_cta"] is False @pytest.mark.usefixtures("create_report_schedules") def test_update_report_schedule_clear_recipients(self): diff --git a/tests/unit_tests/reports/notifications/email_tests.py b/tests/unit_tests/reports/notifications/email_tests.py index 14f57ee1270..fef59a3b4a7 100644 --- a/tests/unit_tests/reports/notifications/email_tests.py +++ b/tests/unit_tests/reports/notifications/email_tests.py @@ -107,6 +107,206 @@ def test_error_template_sanitizes_html() -> None: assert "onerror=alert(1)" not in email_body +def test_cta_link_included_by_default() -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + content = NotificationContent( + name="test alert", + description="

    This is a test alert

    ", + url="http://example.com/superset/dashboard/1/", + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "editors": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "test-execution-id", + }, + ) + email_body = ( + EmailNotification( + recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content + ) + ._get_content() + .body + ) + assert ( + '' + "Explore in Superset" in email_body + ) + + +def test_cta_link_omitted_when_include_cta_is_false() -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + content = NotificationContent( + name="test alert", + description="

    This is a test alert

    ", + url="http://example.com/superset/dashboard/1/", + include_cta=False, + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "editors": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "test-execution-id", + }, + ) + email_body = ( + EmailNotification( + recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content + ) + ._get_content() + .body + ) + assert "Explore in Superset" not in email_body + assert "http://example.com/superset/dashboard/1/" not in email_body + assert "

    This is a test alert

    " in email_body + + +def test_error_template_cta_link_respects_include_cta() -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + content = NotificationContent( + name="test alert", + text="Report generation failed", + url="http://example.com/superset/dashboard/1/", + include_cta=False, + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "editors": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "test-execution-id", + }, + ) + email_body = ( + EmailNotification( + recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content + ) + ._get_content() + .body + ) + assert "Report generation failed" in email_body + assert "Explore in Superset" not in email_body + assert "http://example.com/superset/dashboard/1/" not in email_body + + +@pytest.mark.parametrize( + "include_cta", + [True, False], + ids=["with-cta", "without-cta"], +) +def test_retry_error_template_cta_link_respects_include_cta( + include_cta: bool, +) -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + content = NotificationContent( + name="test alert", + text="Report generation failed", + url="http://example.com/superset/dashboard/1/", + include_cta=include_cta, + retry_attempt=1, + retry_max_attempts=3, + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "editors": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "test-execution-id", + }, + ) + email_body = ( + EmailNotification( + recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content + ) + ._get_content() + .body + ) + assert "Retry in Progress" in email_body + if include_cta: + assert "Explore in Superset" in email_body + assert "http://example.com/superset/dashboard/1/" in email_body + else: + assert "Explore in Superset" not in email_body + assert "http://example.com/superset/dashboard/1/" not in email_body + + +@pytest.mark.parametrize( + "include_cta", + [True, False], + ids=["with-cta", "without-cta"], +) +def test_final_failure_template_cta_link_respects_include_cta( + include_cta: bool, +) -> None: + # `superset.models.helpers`, a dependency of following imports, + # requires app context + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.email import EmailNotification + + content = NotificationContent( + name="test alert", + text="Report generation failed", + url="http://example.com/superset/dashboard/1/", + include_cta=include_cta, + retry_max_attempts=3, + header_data={ + "notification_format": "PNG", + "notification_type": "Alert", + "editors": [1], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "test-execution-id", + }, + ) + email_body = ( + EmailNotification( + recipient=ReportRecipients(type=ReportRecipientType.EMAIL), content=content + ) + ._get_content() + .body + ) + assert "failed to generate after" in email_body + if include_cta: + assert "Explore in Superset" in email_body + assert "http://example.com/superset/dashboard/1/" in email_body + else: + assert "Explore in Superset" not in email_body + assert "http://example.com/superset/dashboard/1/" not in email_body + + @with_feature_flags(DATE_FORMAT_IN_EMAIL_SUBJECT=True) def test_email_subject_with_datetime() -> None: # `superset.models.helpers`, a dependency of following imports, diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index b129e9a8d24..8b976331765 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -550,6 +550,104 @@ def test_slack_mixin_get_body_truncates_large_table( assert "(table was truncated)" in body +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_slackv2_body_omits_cta_when_include_cta_is_false( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data, +) -> None: + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + + flask_global_mock.logs_context = {} + content = NotificationContent( + name="test alert", + header_data=mock_header_data, + description="desc", + url="http://example.com/superset/dashboard/1/", + include_cta=False, + ) + notification = SlackV2Notification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACKV2, + recipient_config_json='{"target": "some_channel"}', + ), + content=content, + ) + body = notification._get_body(content=content) + assert "Explore in Superset" not in body + assert "http://example.com/superset/dashboard/1/" not in body + assert "*test alert*" in body + assert "desc" in body + + +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.get_slack_client") +def test_slack_body_omits_cta_when_include_cta_is_false( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data, +) -> None: + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.slack import SlackNotification + + flask_global_mock.logs_context = {} + content = NotificationContent( + name="test alert", + header_data=mock_header_data, + description="desc", + url="http://example.com/superset/dashboard/1/", + include_cta=False, + ) + notification = SlackNotification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json='{"target": "some_channel"}', + ), + content=content, + ) + body = notification._get_body(content=content) + assert "Explore in Superset" not in body + assert "http://example.com/superset/dashboard/1/" not in body + assert "*test alert*" in body + assert "desc" in body + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_slack_mixin_truncated_body_omits_cta_when_include_cta_is_false( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data, +) -> None: + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + + flask_global_mock.logs_context = {} + # Create a large DataFrame that exceeds the 4000-char message limit + large_df = pd.DataFrame({"col_" + str(i): range(100) for i in range(10)}) + content = NotificationContent( + name="test", + header_data=mock_header_data, + embedded_data=large_df, + description="desc", + url="http://example.com/superset/dashboard/1/", + include_cta=False, + ) + notification = SlackV2Notification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACKV2, + recipient_config_json='{"target": "some_channel"}', + ), + content=content, + ) + body = notification._get_body(content=content) + assert "(table was truncated)" in body + assert "Explore in Superset" not in body + + # --------------------------------------------------------------------------- # Bulletproof v2 send-path coverage # diff --git a/tests/unit_tests/reports/schemas_test.py b/tests/unit_tests/reports/schemas_test.py index b84686a554d..9bcf4ce50b4 100644 --- a/tests/unit_tests/reports/schemas_test.py +++ b/tests/unit_tests/reports/schemas_test.py @@ -522,3 +522,52 @@ def test_put_schema_retry_max_attempts_out_of_range( with pytest.raises(ValidationError) as exc: schema.load({"retry_max_attempts": 11}) assert "retry_max_attempts" in exc.value.messages + + +@pytest.mark.parametrize( + "schema_class,payload_base", + [ + (ReportSchedulePostSchema, MINIMAL_POST_PAYLOAD), + (ReportSchedulePutSchema, {}), + ], + ids=["post", "put"], +) +def test_include_cta_round_trips( + mocker: MockerFixture, schema_class, payload_base +) -> None: + mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG) + schema = schema_class() + + result = schema.load({**payload_base, "include_cta": False}) + assert result["include_cta"] is False + + result = schema.load({**payload_base, "include_cta": True}) + assert result["include_cta"] is True + + # explicit null is accepted and round-trips as None (legacy NULL rows are + # treated as True at execution time) + result = schema.load({**payload_base, "include_cta": None}) + assert result["include_cta"] is None + + # omitted key is absent from the load result (the model default applies) + result = schema.load(payload_base) + assert "include_cta" not in result + + +@pytest.mark.parametrize( + "schema_class,payload_base", + [ + (ReportSchedulePostSchema, MINIMAL_POST_PAYLOAD), + (ReportSchedulePutSchema, {}), + ], + ids=["post", "put"], +) +def test_include_cta_rejects_non_boolean( + mocker: MockerFixture, schema_class, payload_base +) -> None: + mocker.patch("flask.current_app.config", CUSTOM_WIDTH_CONFIG) + schema = schema_class() + + with pytest.raises(ValidationError) as exc: + schema.load({**payload_base, "include_cta": "not-a-boolean"}) + assert "include_cta" in exc.value.messages