codeant-ai-for-open-source[bot] commented on code in PR #42494:
URL: https://github.com/apache/superset/pull/42494#discussion_r3661947584
##########
superset/reports/schemas.py:
##########
@@ -269,6 +273,10 @@ class ReportSchedulePostSchema(Schema):
dump_default=None,
)
force_screenshot = fields.Boolean(dump_default=False)
+ include_cta = fields.Boolean(
+ dump_default=True,
+ metadata={"description": include_cta_description},
+ )
Review Comment:
**Suggestion:** `include_cta` is backed by a nullable database column and
execution explicitly treats `NULL` as equivalent to `True`, but both API
schemas reject an explicit JSON `null` because `allow_none=True` is omitted.
Clients cannot reset or preserve the legacy state by sending `null`, and such
requests fail validation at the API boundary. Allow null for this field or
normalize null to true before schema validation. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Schedule API rejects nullable legacy-state values.
- ⚠️ Clients cannot explicitly reset CTA state.
- ⚠️ Full schedule round-trips may fail on null values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The database model defines `ReportSchedule.include_cta` as nullable in
`superset/reports/models.py:171-173`, and execution explicitly treats only
`False` as
disabled at `superset/commands/report/execute.py:971-972`.
2. Both API schemas define the field without `allow_none=True`: creation uses
`superset/reports/schemas.py:276-279`, and updates use
`superset/reports/schemas.py:442-445`.
3. Send a report-schedule create or update request through the schemas
registered by
`superset/reports/api.py:211-213` with JSON `"include_cta": null`;
Marshmallow rejects the
null Boolean during deserialization before the create/update command runs.
4. This prevents clients from explicitly preserving or resetting legacy
nullable rows to
the execution-equivalent state represented by `NULL`; accepting null or
normalizing it to
true would align the API contract with the model and execution logic.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31ea7c6dacec489eb6fde8455e8bbca4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=31ea7c6dacec489eb6fde8455e8bbca4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 276:279
**Comment:**
*Api Mismatch: `include_cta` is backed by a nullable database column
and execution explicitly treats `NULL` as equivalent to `True`, but both API
schemas reject an explicit JSON `null` because `allow_none=True` is omitted.
Clients cannot reset or preserve the legacy state by sending `null`, and such
requests fail validation at the API boundary. Allow null for this field or
normalize null to true before schema validation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42494&comment_hash=d32abdcc49a42263a79f5025f5509bbc0053ddd4d195b83d0b4d70411146f3e6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42494&comment_hash=d32abdcc49a42263a79f5025f5509bbc0053ddd4d195b83d0b4d70411146f3e6&reaction=dislike'>👎</a>
##########
superset/reports/notifications/slack_mixin.py:
##########
@@ -31,18 +31,30 @@ def _message_template(
content: NotificationContent,
table: str = "",
) -> str:
- return __(
- """*%(name)s*
+ if content.include_cta:
+ return __(
+ """*%(name)s*
%(description)s
<%(url)s|Explore in Superset>
+%(table)s
+""",
Review Comment:
**Suggestion:** The CTA condition only affects the normal message template,
while `_get_body` returns `_error_template` before reaching this branch
whenever `content.text` is set. Consequently Slack and SlackV2 error
notifications never include the CTA, even when `include_cta` is true by
default, and the new per-schedule setting is not honored uniformly across
notification states. Pass the content or CTA flag into the error template and
render the link conditionally there as well. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Slack failure notifications omit configured CTAs.
- ❌ SlackV2 failure notifications omit configured CTAs.
- ⚠️ Error-state CTA behavior differs from email.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A failed report or alert creates `NotificationContent` with `text` and the
schedule-derived CTA flag in `superset/commands/report/execute.py:963-1009`;
`send_error()` also sets the flag at
`superset/commands/report/execute.py:1114-1133`.
2. Slack v1 calls `self._get_body(content=self._content)` from
`superset/reports/notifications/slack.py:49-55`, and SlackV2 does the same
from
`superset/reports/notifications/slackv2.py:42-49`.
3. At `superset/reports/notifications/slack_mixin.py:75-79`, any non-empty
`content.text`
returns `_error_template()` immediately, so execution never reaches
`_message_template()`
or its new `content.include_cta` branch at lines 34-59.
4. `_error_template()` at
`superset/reports/notifications/slack_mixin.py:61-73` accepts
only name, description, and error text and contains no URL placeholder.
Consequently,
error notifications omit the CTA even when `include_cta` is true, and the
flag cannot
control error-message CTA output uniformly.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3db4a38c70354fd29e3647106fab7c01&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3db4a38c70354fd29e3647106fab7c01&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/slack_mixin.py
**Line:** 34:43
**Comment:**
*Incomplete Implementation: The CTA condition only affects the normal
message template, while `_get_body` returns `_error_template` before reaching
this branch whenever `content.text` is set. Consequently Slack and SlackV2
error notifications never include the CTA, even when `include_cta` is true by
default, and the new per-schedule setting is not honored uniformly across
notification states. Pass the content or CTA flag into the error template and
render the link conditionally there as well.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42494&comment_hash=6127b1a4d1711a2a5f8c5a922a4624ccb3dae18fc5ebbbbcf795d18121f447eb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42494&comment_hash=6127b1a4d1711a2a5f8c5a922a4624ccb3dae18fc5ebbbbcf795d18121f447eb&reaction=dislike'>👎</a>
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
