codeant-ai-for-open-source[bot] commented on code in PR #40885:
URL: https://github.com/apache/superset/pull/40885#discussion_r3570896929
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -144,3 +149,95 @@ def test_email_subject_with_datetime() -> None:
subject = notification._get_subject()
assert datetime_pattern not in subject
assert frozen_now.strftime(datetime_pattern) in subject
+
+
+def _make_notification(xlsx: bytes) -> "EmailNotification":
+ """Build an email notification for attachment tests."""
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+ from superset.reports.notifications.email import EmailNotification
+
+ recipient = ReportRecipients(type=ReportRecipientType.EMAIL)
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable to
comply with the type-hint requirement for relevant variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new local variable is introduced without a type annotation in a Python
file covered by the type-hint rule. It can be annotated, so the suggestion
matches the rule violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf3f5bec103f46259073de9b3a6a378d&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=bf3f5bec103f46259073de9b3a6a378d&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:** tests/unit_tests/reports/notifications/email_tests.py
**Line:** 160:160
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable
to comply with the type-hint requirement for relevant variables.
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%2F40885&comment_hash=51d3042e9df0276c253b86a6a8874db03b59f57e179687bc9907d0bb1fadd0b6&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=51d3042e9df0276c253b86a6a8874db03b59f57e179687bc9907d0bb1fadd0b6&reaction=dislike'>๐</a>
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -144,3 +149,95 @@ def test_email_subject_with_datetime() -> None:
subject = notification._get_subject()
assert datetime_pattern not in subject
assert frozen_now.strftime(datetime_pattern) in subject
+
+
+def _make_notification(xlsx: bytes) -> "EmailNotification":
+ """Build an email notification for attachment tests."""
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+ from superset.reports.notifications.email import EmailNotification
+
+ recipient = ReportRecipients(type=ReportRecipientType.EMAIL)
+ content = NotificationContent(
+ name="test report",
+ text=None,
+ xlsx=xlsx,
+ header_data={
+ "notification_format": "XLSX",
+ "notification_type": "Report",
+ "editors": [1],
+ "notification_source": None,
+ "chart_id": None,
+ "dashboard_id": None,
+ "slack_channels": None,
+ "execution_id": "test-execution-id",
+ },
+ )
+ return EmailNotification(recipient=recipient, content=content)
+
+
+def test_get_xlsx_attachment_extension_for_non_zip_content() -> None:
+ """Non-ZIP responses retain the XLSX extension."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+
+ assert _get_xlsx_attachment_extension(b"xlsx-response") == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_xlsx_content() -> None:
+ """An OOXML workbook is identified as XLSX."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils import excel
+
+ xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False)
+
+ assert _get_xlsx_attachment_extension(xlsx) == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_invalid_xlsx_zip_content() -> None:
+ """A ZIP with invalid XLSX entries is not identified as a report
archive."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils.core import create_zip
+
+ archive = create_zip({"query_1.xlsx": b"xlsx-response"}).getvalue()
Review Comment:
**Suggestion:** Annotate this archive variable with its concrete type to
satisfy the mandatory typing rule for new variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The variable is newly introduced in Python code and lacks a type annotation
even though it can be annotated, so this is a real violation of the type-hint
rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40d2863f115a42f5b16e55e9f998dce7&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=40d2863f115a42f5b16e55e9f998dce7&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:** tests/unit_tests/reports/notifications/email_tests.py
**Line:** 207:207
**Comment:**
*Custom Rule: Annotate this archive variable with its concrete type to
satisfy the mandatory typing rule for new variables.
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%2F40885&comment_hash=098113fcac91bf177a62f6f163df430d9a797325ac859db25d6e48a2fb410e32&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=098113fcac91bf177a62f6f163df430d9a797325ac859db25d6e48a2fb410e32&reaction=dislike'>๐</a>
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -144,3 +149,95 @@ def test_email_subject_with_datetime() -> None:
subject = notification._get_subject()
assert datetime_pattern not in subject
assert frozen_now.strftime(datetime_pattern) in subject
+
+
+def _make_notification(xlsx: bytes) -> "EmailNotification":
+ """Build an email notification for attachment tests."""
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+ from superset.reports.notifications.email import EmailNotification
+
+ recipient = ReportRecipients(type=ReportRecipientType.EMAIL)
+ content = NotificationContent(
+ name="test report",
+ text=None,
+ xlsx=xlsx,
+ header_data={
+ "notification_format": "XLSX",
+ "notification_type": "Report",
+ "editors": [1],
+ "notification_source": None,
+ "chart_id": None,
+ "dashboard_id": None,
+ "slack_channels": None,
+ "execution_id": "test-execution-id",
+ },
+ )
+ return EmailNotification(recipient=recipient, content=content)
+
+
+def test_get_xlsx_attachment_extension_for_non_zip_content() -> None:
+ """Non-ZIP responses retain the XLSX extension."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+
+ assert _get_xlsx_attachment_extension(b"xlsx-response") == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_xlsx_content() -> None:
+ """An OOXML workbook is identified as XLSX."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils import excel
+
+ xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False)
+
+ assert _get_xlsx_attachment_extension(xlsx) == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_invalid_xlsx_zip_content() -> None:
+ """A ZIP with invalid XLSX entries is not identified as a report
archive."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils.core import create_zip
+
+ archive = create_zip({"query_1.xlsx": b"xlsx-response"}).getvalue()
+
+ assert _get_xlsx_attachment_extension(archive) == "xlsx"
+
+
[email protected](
+ ("server_pagination", "expected_extension"),
+ [
+ (False, "xlsx"),
+ (True, "zip"),
+ ],
+)
+def test_xlsx_report_attachment_extension(
+ server_pagination: bool,
+ expected_extension: str,
+) -> None:
+ """Server-paginated XLSX reports should be attached as ZIP archives."""
+ from superset.utils import excel
+ from superset.utils.core import create_zip
+
+ xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False)
Review Comment:
**Suggestion:** Add an explicit type annotation to this variable assignment
so the new test code remains fully typed. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This added local variable is unannotated and clearly annotatable, so it
violates the Python type-hint requirement for new code.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=302ad9d342a6436f988ad3d65bee69f9&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=302ad9d342a6436f988ad3d65bee69f9&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:** tests/unit_tests/reports/notifications/email_tests.py
**Line:** 227:227
**Comment:**
*Custom Rule: Add an explicit type annotation to this variable
assignment so the new test code remains fully typed.
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%2F40885&comment_hash=e8a73fb6a4a723b31d638abedafbfaafef720c1adabfc89bdbf0ff86ba0872d7&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=e8a73fb6a4a723b31d638abedafbfaafef720c1adabfc89bdbf0ff86ba0872d7&reaction=dislike'>๐</a>
##########
tests/unit_tests/reports/notifications/email_tests.py:
##########
@@ -144,3 +149,95 @@ def test_email_subject_with_datetime() -> None:
subject = notification._get_subject()
assert datetime_pattern not in subject
assert frozen_now.strftime(datetime_pattern) in subject
+
+
+def _make_notification(xlsx: bytes) -> "EmailNotification":
+ """Build an email notification for attachment tests."""
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+ from superset.reports.notifications.base import NotificationContent
+ from superset.reports.notifications.email import EmailNotification
+
+ recipient = ReportRecipients(type=ReportRecipientType.EMAIL)
+ content = NotificationContent(
+ name="test report",
+ text=None,
+ xlsx=xlsx,
+ header_data={
+ "notification_format": "XLSX",
+ "notification_type": "Report",
+ "editors": [1],
+ "notification_source": None,
+ "chart_id": None,
+ "dashboard_id": None,
+ "slack_channels": None,
+ "execution_id": "test-execution-id",
+ },
+ )
+ return EmailNotification(recipient=recipient, content=content)
+
+
+def test_get_xlsx_attachment_extension_for_non_zip_content() -> None:
+ """Non-ZIP responses retain the XLSX extension."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+
+ assert _get_xlsx_attachment_extension(b"xlsx-response") == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_xlsx_content() -> None:
+ """An OOXML workbook is identified as XLSX."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils import excel
+
+ xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False)
+
+ assert _get_xlsx_attachment_extension(xlsx) == "xlsx"
+
+
+def test_get_xlsx_attachment_extension_for_invalid_xlsx_zip_content() -> None:
+ """A ZIP with invalid XLSX entries is not identified as a report
archive."""
+ from superset.reports.notifications.email import (
+ _get_xlsx_attachment_extension,
+ )
+ from superset.utils.core import create_zip
+
+ archive = create_zip({"query_1.xlsx": b"xlsx-response"}).getvalue()
+
+ assert _get_xlsx_attachment_extension(archive) == "xlsx"
+
+
[email protected](
+ ("server_pagination", "expected_extension"),
+ [
+ (False, "xlsx"),
+ (True, "zip"),
+ ],
+)
+def test_xlsx_report_attachment_extension(
+ server_pagination: bool,
+ expected_extension: str,
+) -> None:
+ """Server-paginated XLSX reports should be attached as ZIP archives."""
+ from superset.utils import excel
+ from superset.utils.core import create_zip
+
+ xlsx = excel.df_to_excel(pd.DataFrame({"value": [1, 2]}), index=False)
+ attachment = (
+ create_zip(
+ {
+ "query_1.xlsx": xlsx,
+ "query_2.xlsx": xlsx,
+ }
+ ).getvalue()
+ if server_pagination
+ else xlsx
+ )
+
+ email_content = _make_notification(xlsx=attachment)._get_content()
Review Comment:
**Suggestion:** Add a type annotation for this computed content variable to
keep the new code compliant with typing requirements. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new local variable is not type-annotated and could be annotated, so it
fits the custom Python type-hint requirement.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=03f11caa5582419689a52a4ff9a6432c&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=03f11caa5582419689a52a4ff9a6432c&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:** tests/unit_tests/reports/notifications/email_tests.py
**Line:** 239:239
**Comment:**
*Custom Rule: Add a type annotation for this computed content variable
to keep the new code compliant with typing requirements.
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%2F40885&comment_hash=e6783b321470036fc4dab5ade85a74fc190ae41787fb04bf0ad45386bd07ffee&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40885&comment_hash=e6783b321470036fc4dab5ade85a74fc190ae41787fb04bf0ad45386bd07ffee&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]
