codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3425021113
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
Review Comment:
**Suggestion:** Add full type hints to this new test method signature,
including an explicit `-> None` return annotation. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method and it has no parameter or return
type annotations. The custom rule requires new Python code to be fully typed,
so the omission is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d731e2d2bb864021ab3a669eb74efddd&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=d731e2d2bb864021ab3a669eb74efddd&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/integration_tests/dashboards/api_tests.py
**Line:** 3281:3281
**Comment:**
*Custom Rule: Add full type hints to this new test method signature,
including an explicit `-> None` return annotation.
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%2F41133&comment_hash=51132cce37e917ec842a806e5a0a6a53deac234d013e1017d99d87ceebe38043&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=51132cce37e917ec842a806e5a0a6a53deac234d013e1017d99d87ceebe38043&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_dashboard_filter_context.py:
##########
@@ -277,6 +279,51 @@ def
test_merge_extra_form_data_merges_custom_form_data_dicts() -> None:
assert merged["custom_form_data"] == {"groupby": ["col1"], "foo": "bar"}
+# --- apply_extra_form_data_to_query_context_json ---
+
+
+def test_apply_extra_form_data_noop_when_empty() -> None:
+ """An empty extra_form_data leaves the body untouched."""
+ original = {"queries": [{"filters": [{"col": "a", "op": "IN", "val":
["x"]}]}]}
+ json_body = deepcopy(original)
+ apply_extra_form_data_to_query_context_json(json_body, {})
+ assert json_body == original
+
+
+def test_apply_extra_form_data_appends_filters_with_is_extra() -> None:
Review Comment:
**Suggestion:** Add a short docstring to this newly added test function to
comply with the rule requiring inline documentation for new functions.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function in the modified file, and it does
not have a docstring immediately under the definition. That matches the custom
rule requiring new functions and classes to be documented inline.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b71915533ca841bfa51c761921e16a6b&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=b71915533ca841bfa51c761921e16a6b&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/charts/test_dashboard_filter_context.py
**Line:** 293:293
**Comment:**
*Custom Rule: Add a short docstring to this newly added test function
to comply with the rule requiring inline documentation for new functions.
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%2F41133&comment_hash=1e8e4f3d3cbc2f9030f219bac0bf050eef752cc8ffeb5668eab2022b6dab011e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=1e8e4f3d3cbc2f9030f219bac0bf050eef752cc8ffeb5668eab2022b6dab011e&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_missing_dashboard(self, mock_task):
Review Comment:
**Suggestion:** Add type annotations for both parameters and return value in
this new patched test method (including a concrete type for `mock_task` and `->
None`). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new test method introduces an untyped parameter (`mock_task`) and lacks
an explicit return annotation. That violates the rule requiring new or modified
Python functions to include type hints.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8e152fc2af034c0bb30bc84f1bb21cee&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=8e152fc2af034c0bb30bc84f1bb21cee&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/integration_tests/dashboards/api_tests.py
**Line:** 3294:3294
**Comment:**
*Custom Rule: Add type annotations for both parameters and return value
in this new patched test method (including a concrete type for `mock_task` and
`-> None`).
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%2F41133&comment_hash=c5fcad114473bb140457a4658cad811d36c2615896b359db2509a1cce80711e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=c5fcad114473bb140457a4658cad811d36c2615896b359db2509a1cce80711e0&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_missing_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 404 for an unknown dashboard."""
+ self.login(ADMIN_USERNAME)
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv = self.client.post("api/v1/dashboard/99999999/export_xlsx/")
+ assert rv.status_code == 404
+ mock_task.apply_async.assert_not_called()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_400_for_empty_dashboard(self, mock_task):
Review Comment:
**Suggestion:** Update this new method definition to include explicit
parameter and return type hints, following the fully typed new-code rule.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The method is newly added and its signature is untyped: the mocked argument
has no annotation and there is no `-> None` return type. This matches the
type-hint violation described by the rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=960ec881f3e94e5290e55a63cc33be5a&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=960ec881f3e94e5290e55a63cc33be5a&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/integration_tests/dashboards/api_tests.py
**Line:** 3306:3306
**Comment:**
*Custom Rule: Update this new method definition to include explicit
parameter and return type hints, following the fully typed new-code rule.
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%2F41133&comment_hash=9b83aba6517bbde14294f89347826342c025385392d0bd57ae76d14b0f046457&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=9b83aba6517bbde14294f89347826342c025385392d0bd57ae76d14b0f046457&reaction=dislike'>👎</a>
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -0,0 +1,212 @@
+# 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 __future__ import annotations
+
+import glob
+import os
+import tempfile
+from collections.abc import Iterator
+from contextlib import ExitStack
+from typing import Any
+from unittest import mock
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.utils import json
+
+MODULE = "superset.tasks.export_dashboard_excel"
+
+
+def _chart(chart_id: int, name: str, has_context: bool = True) ->
mock.MagicMock:
Review Comment:
**Suggestion:** Add a short docstring to this helper function describing
that it builds a mocked chart object for test setup. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function in a new test file, and it has no
docstring. That matches the custom rule requiring new functions and classes to
be documented inline.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2b9a4e57972543828a8365041b6c4598&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=2b9a4e57972543828a8365041b6c4598&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/tasks/test_export_dashboard_excel.py
**Line:** 35:35
**Comment:**
*Custom Rule: Add a short docstring to this helper function describing
that it builds a mocked chart object for test setup.
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%2F41133&comment_hash=0bcaacbee99bfa49716881be8cf6e8e72c7fc9f8ed86a4d5d1ec63952b6f0b04&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=0bcaacbee99bfa49716881be8cf6e8e72c7fc9f8ed86a4d5d1ec63952b6f0b04&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_missing_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 404 for an unknown dashboard."""
+ self.login(ADMIN_USERNAME)
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv = self.client.post("api/v1/dashboard/99999999/export_xlsx/")
+ assert rv.status_code == 404
+ mock_task.apply_async.assert_not_called()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_400_for_empty_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 400 for a dashboard with no
charts."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-empty", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 400
+ mock_task.apply_async.assert_not_called()
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_202_enqueues_task(self, mock_task):
Review Comment:
**Suggestion:** Add missing type hints to this new test method signature,
including the mocked argument type and an explicit `-> None` return type.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly introduced test method has an unannotated mocked parameter and no
return type annotation. The custom rule explicitly requires new Python code to
be fully typed, so this is a verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9dc8276f11a4169af66dd694c72241f&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=d9dc8276f11a4169af66dd694c72241f&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/integration_tests/dashboards/api_tests.py
**Line:** 3325:3325
**Comment:**
*Custom Rule: Add missing type hints to this new test method signature,
including the mocked argument type and an explicit `-> None` return type.
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%2F41133&comment_hash=76fa7ed99d1daa23edc8e498f8f1c948b670621af32a3de03f9174e3d088bb65&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=76fa7ed99d1daa23edc8e498f8f1c948b670621af32a3de03f9174e3d088bb65&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3277,6 +3278,91 @@ def _base_filter(query):
response_roles = [result["text"] for result in response["result"]]
assert response_roles == ["Alpha"]
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_missing_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 404 for an unknown dashboard."""
+ self.login(ADMIN_USERNAME)
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv = self.client.post("api/v1/dashboard/99999999/export_xlsx/")
+ assert rv.status_code == 404
+ mock_task.apply_async.assert_not_called()
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_400_for_empty_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 400 for a dashboard with no
charts."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-empty", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 400
+ mock_task.apply_async.assert_not_called()
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_202_enqueues_task(self, mock_task):
+ """Dashboard API: export_xlsx enqueues the task and returns 202 +
job_id."""
+ self.login(ADMIN_USERNAME)
+ dashboard =
db.session.query(Dashboard).filter_by(slug="world_health").first()
+ with patch.dict(
+ "flask.current_app.config",
+ {"EXCEL_EXPORT_S3_BUCKET": "exports"},
+ ):
+ rv = self.client.post(
+ f"api/v1/dashboard/{dashboard.id}/export_xlsx/",
+ json={"active_data_mask": {}},
+ )
+ assert rv.status_code == 202
+ body = json.loads(rv.data.decode("utf-8"))
+ job_id = body["job_id"]
+ assert job_id
+ mock_task.apply_async.assert_called_once()
+ _, kwargs = mock_task.apply_async.call_args
+ assert kwargs["task_id"] == job_id
+ assert kwargs["kwargs"]["dashboard_id"] == dashboard.id
+
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_inaccessible_dashboard(self, mock_task):
Review Comment:
**Suggestion:** Add explicit typing to this newly added test method
signature so parameters and return value are fully annotated. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new Python test method with no type hints for its parameter or
return value. Since the surrounding code is being changed and new Python code
should be fully typed, the suggestion identifies a real issue.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d13968bf454b4ff59d25ede59aa59562&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=d13968bf454b4ff59d25ede59aa59562&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/integration_tests/dashboards/api_tests.py
**Line:** 3347:3347
**Comment:**
*Custom Rule: Add explicit typing to this newly added test method
signature so parameters and return value are fully annotated.
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%2F41133&comment_hash=a13892732e1bca4233df4c3e8c54d98a16a4713a0d935a05088f5d9c1d869ce9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=a13892732e1bca4233df4c3e8c54d98a16a4713a0d935a05088f5d9c1d869ce9&reaction=dislike'>👎</a>
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -0,0 +1,212 @@
+# 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 __future__ import annotations
+
+import glob
+import os
+import tempfile
+from collections.abc import Iterator
+from contextlib import ExitStack
+from typing import Any
+from unittest import mock
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.utils import json
+
+MODULE = "superset.tasks.export_dashboard_excel"
+
+
+def _chart(chart_id: int, name: str, has_context: bool = True) ->
mock.MagicMock:
+ chart = mock.MagicMock()
+ chart.id = chart_id
+ chart.slice_name = name
+ chart.query_context = json.dumps({"queries": [{}]}) if has_context else
None
+ return chart
+
+
[email protected]
+def mocks() -> Iterator[dict[str, Any]]:
+ """Patch every external dependency of the task; keep the real xlsx
writer."""
+ with ExitStack() as stack:
+ # Use explicit MagicMock instances: patch() auto-creates async-flavored
+ # mocks for these targets (their real objects expose async members),
which
+ # would make calls like security_manager.get_user_by_id() return
coroutines.
+ patched = {
+ name: stack.enter_context(
+ mock.patch(f"{MODULE}.{name}", new=mock.MagicMock())
+ )
+ for name in (
+ "security_manager",
+ "db",
+ "get_charts_in_layout_order",
+ "get_dashboard_filter_context",
+ "ChartDataQueryContextSchema",
+ "ChartDataCommand",
+ "s3",
+ "email",
+ )
+ }
+ user = mock.MagicMock()
+ user.email = "[email protected]"
+ patched["security_manager"].get_user_by_id.return_value = user
+
+ dashboard = mock.MagicMock()
+ dashboard.id = 1
+ dashboard.dashboard_title = "Sales"
+ patched[
+ "db"
+
].session.query.return_value.filter_by.return_value.one_or_none.return_value =
( # noqa: E501
+ dashboard
+ )
+
+ patched["get_dashboard_filter_context"].return_value.extra_form_data =
{}
+ patched["s3"].generate_presigned_url.return_value =
"https://signed/file.xlsx";
+
+ patched["user"] = user
+ patched["dashboard"] = dashboard
+ yield patched
+
+
+def _run(job_id: str = "job-1") -> None:
+ from superset.tasks.export_dashboard_excel import export_dashboard_excel
+
+ export_dashboard_excel(
+ dashboard_id=1, user_id=2, active_data_mask={}, job_id=job_id
+ )
+
+
+def _no_temp_files_left(job_id: str) -> bool:
+ pattern = os.path.join(tempfile.gettempdir(), f"dash-export-{job_id}-*")
+ return glob.glob(pattern) == []
+
+
+def _read_sheets(path: str) -> dict[str, list[list[object]]]:
Review Comment:
**Suggestion:** Add a docstring describing that this helper loads workbook
sheets into a dictionary for assertion-friendly comparisons. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The helper is newly introduced and has no docstring. The custom rule
requires newly added Python functions to include docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d114b7bd304e42e886381fcb05cb5b11&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=d114b7bd304e42e886381fcb05cb5b11&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/tasks/test_export_dashboard_excel.py
**Line:** 99:99
**Comment:**
*Custom Rule: Add a docstring describing that this helper loads
workbook sheets into a dictionary for assertion-friendly comparisons.
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%2F41133&comment_hash=558d0ddaf06fbc25c307a6bc8cc5bc4fe2b39d7b4b966cbd9ffdc3393098d9de&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=558d0ddaf06fbc25c307a6bc8cc5bc4fe2b39d7b4b966cbd9ffdc3393098d9de&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/excel_streaming_tests.py:
##########
@@ -0,0 +1,170 @@
+# 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 __future__ import annotations
+
+from datetime import date, datetime
+from decimal import Decimal
+from pathlib import Path
+
+import pytest
+
+from superset.utils import excel_streaming
+from superset.utils.excel_streaming import (
+ _sanitize_cell,
+ sanitize_sheet_name,
+ StreamingXlsxWriter,
+)
+
+# --- sanitize_sheet_name ---
+
+
+def test_sheet_name_replaces_forbidden_chars() -> None:
+ assert sanitize_sheet_name("a/b:c*d?e[f]g\\h", set()) == "a_b_c_d_e_f_g_h"
+
+
+def test_sheet_name_truncated_to_31() -> None:
+ assert sanitize_sheet_name("x" * 40, set()) == "x" * 31
+
+
+def test_sheet_name_dedupes_case_insensitively() -> None:
+ used: set[str] = set()
+ assert sanitize_sheet_name("Sales", used) == "Sales"
+ assert sanitize_sheet_name("sales", used) == "sales~2"
+ assert sanitize_sheet_name("SALES", used) == "SALES~3"
+
+
+def test_sheet_name_dedupe_marker_respects_length_cap() -> None:
+ used: set[str] = set()
+ long_name = "y" * 31
+ assert sanitize_sheet_name(long_name, used) == long_name
+ assert sanitize_sheet_name(long_name, used) == "y" * 29 + "~2"
+
+
+def test_sheet_name_blank_falls_back() -> None:
+ assert sanitize_sheet_name(" ", set()) == "Sheet"
+
+
+def test_sheet_name_reserved_history_is_escaped() -> None:
+ assert sanitize_sheet_name("History", set()) == "History_"
+
+
+def test_sheet_name_strips_surrounding_apostrophes() -> None:
+ assert sanitize_sheet_name("'quoted'", set()) == "quoted"
+
+
+# --- _sanitize_cell ---
+
+
[email protected](
+ "value,expected",
+ [
+ (None, ""),
+ ("=SUM(A1)", "'=SUM(A1)"),
+ ("+1", "'+1"),
+ ("-1", "'-1"),
+ ("@handle", "'@handle"),
+ ("normal", "normal"),
+ (True, True),
+ (5, 5),
+ (1.5, 1.5),
+ (Decimal("2.5"), 2.5),
+ (datetime(2020, 1, 2, 3, 4, 5), "2020-01-02T03:04:05"),
+ (date(2020, 1, 2), "2020-01-02"),
+ ],
+)
+def test_sanitize_cell(value: object, expected: object) -> None:
+ assert _sanitize_cell(value) == expected
+
+
+def test_sanitize_cell_large_int_becomes_string() -> None:
+ assert _sanitize_cell(10**16) == str(10**16)
+
+
+def test_sanitize_cell_non_finite_floats_blanked() -> None:
+ assert _sanitize_cell(float("nan")) == ""
+ assert _sanitize_cell(float("inf")) == ""
+
+
+# --- StreamingXlsxWriter (round-trip via openpyxl) ---
+
+
+def _read_workbook(path: str) -> dict[str, list[list[object]]]:
Review Comment:
**Suggestion:** Add a docstring that explains the input and expected output
contract of this helper function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This helper is newly added and lacks a docstring in the final file.
The rule explicitly flags newly added Python functions without docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9d7cf8fbd8c04865afbb2af6bdb3e73b&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=9d7cf8fbd8c04865afbb2af6bdb3e73b&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/utils/excel_streaming_tests.py
**Line:** 105:105
**Comment:**
*Custom Rule: Add a docstring that explains the input and expected
output contract of this helper function.
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%2F41133&comment_hash=ef249c1dd3e101ef663a0245d390e8ae68c9e720eede26902057c90d2c49e0d7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=ef249c1dd3e101ef663a0245d390e8ae68c9e720eede26902057c90d2c49e0d7&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/excel_streaming_tests.py:
##########
@@ -0,0 +1,170 @@
+# 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 __future__ import annotations
+
+from datetime import date, datetime
+from decimal import Decimal
+from pathlib import Path
+
+import pytest
+
+from superset.utils import excel_streaming
+from superset.utils.excel_streaming import (
+ _sanitize_cell,
+ sanitize_sheet_name,
+ StreamingXlsxWriter,
+)
+
+# --- sanitize_sheet_name ---
+
+
+def test_sheet_name_replaces_forbidden_chars() -> None:
Review Comment:
**Suggestion:** Add a short docstring describing the behavior being
validated by this test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function and it has no docstring in the final
file.
The custom rule requires newly added Python functions and classes to include
docstrings, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3ce8384f3e684ae2a26a5ce2a4370d34&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=3ce8384f3e684ae2a26a5ce2a4370d34&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/utils/excel_streaming_tests.py
**Line:** 35:35
**Comment:**
*Custom Rule: Add a short docstring describing the behavior being
validated by this test.
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%2F41133&comment_hash=545097bd36a5eaebd4ed196b8495b4a8b6e5702f2e131f84c4de209bf921746a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=545097bd36a5eaebd4ed196b8495b4a8b6e5702f2e131f84c4de209bf921746a&reaction=dislike'>👎</a>
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -0,0 +1,212 @@
+# 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 __future__ import annotations
+
+import glob
+import os
+import tempfile
+from collections.abc import Iterator
+from contextlib import ExitStack
+from typing import Any
+from unittest import mock
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.utils import json
+
+MODULE = "superset.tasks.export_dashboard_excel"
+
+
+def _chart(chart_id: int, name: str, has_context: bool = True) ->
mock.MagicMock:
+ chart = mock.MagicMock()
+ chart.id = chart_id
+ chart.slice_name = name
+ chart.query_context = json.dumps({"queries": [{}]}) if has_context else
None
+ return chart
+
+
[email protected]
+def mocks() -> Iterator[dict[str, Any]]:
+ """Patch every external dependency of the task; keep the real xlsx
writer."""
+ with ExitStack() as stack:
+ # Use explicit MagicMock instances: patch() auto-creates async-flavored
+ # mocks for these targets (their real objects expose async members),
which
+ # would make calls like security_manager.get_user_by_id() return
coroutines.
+ patched = {
+ name: stack.enter_context(
+ mock.patch(f"{MODULE}.{name}", new=mock.MagicMock())
+ )
+ for name in (
+ "security_manager",
+ "db",
+ "get_charts_in_layout_order",
+ "get_dashboard_filter_context",
+ "ChartDataQueryContextSchema",
+ "ChartDataCommand",
+ "s3",
+ "email",
+ )
+ }
+ user = mock.MagicMock()
+ user.email = "[email protected]"
+ patched["security_manager"].get_user_by_id.return_value = user
+
+ dashboard = mock.MagicMock()
+ dashboard.id = 1
+ dashboard.dashboard_title = "Sales"
+ patched[
+ "db"
+
].session.query.return_value.filter_by.return_value.one_or_none.return_value =
( # noqa: E501
+ dashboard
+ )
+
+ patched["get_dashboard_filter_context"].return_value.extra_form_data =
{}
+ patched["s3"].generate_presigned_url.return_value =
"https://signed/file.xlsx";
+
+ patched["user"] = user
+ patched["dashboard"] = dashboard
+ yield patched
+
+
+def _run(job_id: str = "job-1") -> None:
Review Comment:
**Suggestion:** Add a docstring explaining that this helper imports and
executes the dashboard Excel export task with test defaults. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added helper function has no docstring in the final file. The
custom rule explicitly flags new Python functions without docstrings.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=929da8e65daf4e53bc5ca6af8794aac7&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=929da8e65daf4e53bc5ca6af8794aac7&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/tasks/test_export_dashboard_excel.py
**Line:** 86:86
**Comment:**
*Custom Rule: Add a docstring explaining that this helper imports and
executes the dashboard Excel export task with test defaults.
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%2F41133&comment_hash=123ff4352f664a71884b3dd0877469398b9986a91a3e0472e2222501672a8ac9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=123ff4352f664a71884b3dd0877469398b9986a91a3e0472e2222501672a8ac9&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/excel_streaming_tests.py:
##########
@@ -0,0 +1,170 @@
+# 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 __future__ import annotations
+
+from datetime import date, datetime
+from decimal import Decimal
+from pathlib import Path
+
+import pytest
+
+from superset.utils import excel_streaming
+from superset.utils.excel_streaming import (
+ _sanitize_cell,
+ sanitize_sheet_name,
+ StreamingXlsxWriter,
+)
+
+# --- sanitize_sheet_name ---
+
+
+def test_sheet_name_replaces_forbidden_chars() -> None:
+ assert sanitize_sheet_name("a/b:c*d?e[f]g\\h", set()) == "a_b_c_d_e_f_g_h"
+
+
+def test_sheet_name_truncated_to_31() -> None:
+ assert sanitize_sheet_name("x" * 40, set()) == "x" * 31
+
+
+def test_sheet_name_dedupes_case_insensitively() -> None:
+ used: set[str] = set()
+ assert sanitize_sheet_name("Sales", used) == "Sales"
+ assert sanitize_sheet_name("sales", used) == "sales~2"
+ assert sanitize_sheet_name("SALES", used) == "SALES~3"
+
+
+def test_sheet_name_dedupe_marker_respects_length_cap() -> None:
+ used: set[str] = set()
+ long_name = "y" * 31
+ assert sanitize_sheet_name(long_name, used) == long_name
+ assert sanitize_sheet_name(long_name, used) == "y" * 29 + "~2"
+
+
+def test_sheet_name_blank_falls_back() -> None:
+ assert sanitize_sheet_name(" ", set()) == "Sheet"
+
+
+def test_sheet_name_reserved_history_is_escaped() -> None:
+ assert sanitize_sheet_name("History", set()) == "History_"
+
+
+def test_sheet_name_strips_surrounding_apostrophes() -> None:
+ assert sanitize_sheet_name("'quoted'", set()) == "quoted"
+
+
+# --- _sanitize_cell ---
+
+
[email protected](
+ "value,expected",
+ [
+ (None, ""),
+ ("=SUM(A1)", "'=SUM(A1)"),
+ ("+1", "'+1"),
+ ("-1", "'-1"),
+ ("@handle", "'@handle"),
+ ("normal", "normal"),
+ (True, True),
+ (5, 5),
+ (1.5, 1.5),
+ (Decimal("2.5"), 2.5),
+ (datetime(2020, 1, 2, 3, 4, 5), "2020-01-02T03:04:05"),
+ (date(2020, 1, 2), "2020-01-02"),
+ ],
+)
+def test_sanitize_cell(value: object, expected: object) -> None:
Review Comment:
**Suggestion:** Add a concise docstring clarifying what sanitization
behavior this parametrized test covers. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new function is missing a docstring in the final code.
Since the file is newly added and the function is new, the docstring
requirement applies.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e52e996d11e64e5286efe7c278e71f3d&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=e52e996d11e64e5286efe7c278e71f3d&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/utils/excel_streaming_tests.py
**Line:** 89:89
**Comment:**
*Custom Rule: Add a concise docstring clarifying what sanitization
behavior this parametrized test covers.
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%2F41133&comment_hash=9cd0991efd42e9014915aa9a560ac435c91a1b145bed4b90ef6782fce4e2e0ce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=9cd0991efd42e9014915aa9a560ac435c91a1b145bed4b90ef6782fce4e2e0ce&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/excel_streaming_tests.py:
##########
@@ -0,0 +1,170 @@
+# 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 __future__ import annotations
+
+from datetime import date, datetime
+from decimal import Decimal
+from pathlib import Path
+
+import pytest
+
+from superset.utils import excel_streaming
+from superset.utils.excel_streaming import (
+ _sanitize_cell,
+ sanitize_sheet_name,
+ StreamingXlsxWriter,
+)
+
+# --- sanitize_sheet_name ---
+
+
+def test_sheet_name_replaces_forbidden_chars() -> None:
+ assert sanitize_sheet_name("a/b:c*d?e[f]g\\h", set()) == "a_b_c_d_e_f_g_h"
+
+
+def test_sheet_name_truncated_to_31() -> None:
+ assert sanitize_sheet_name("x" * 40, set()) == "x" * 31
+
+
+def test_sheet_name_dedupes_case_insensitively() -> None:
+ used: set[str] = set()
+ assert sanitize_sheet_name("Sales", used) == "Sales"
+ assert sanitize_sheet_name("sales", used) == "sales~2"
+ assert sanitize_sheet_name("SALES", used) == "SALES~3"
+
+
+def test_sheet_name_dedupe_marker_respects_length_cap() -> None:
+ used: set[str] = set()
+ long_name = "y" * 31
+ assert sanitize_sheet_name(long_name, used) == long_name
+ assert sanitize_sheet_name(long_name, used) == "y" * 29 + "~2"
+
+
+def test_sheet_name_blank_falls_back() -> None:
+ assert sanitize_sheet_name(" ", set()) == "Sheet"
+
+
+def test_sheet_name_reserved_history_is_escaped() -> None:
+ assert sanitize_sheet_name("History", set()) == "History_"
+
+
+def test_sheet_name_strips_surrounding_apostrophes() -> None:
+ assert sanitize_sheet_name("'quoted'", set()) == "quoted"
+
+
+# --- _sanitize_cell ---
+
+
[email protected](
+ "value,expected",
+ [
+ (None, ""),
+ ("=SUM(A1)", "'=SUM(A1)"),
+ ("+1", "'+1"),
+ ("-1", "'-1"),
+ ("@handle", "'@handle"),
+ ("normal", "normal"),
+ (True, True),
+ (5, 5),
+ (1.5, 1.5),
+ (Decimal("2.5"), 2.5),
+ (datetime(2020, 1, 2, 3, 4, 5), "2020-01-02T03:04:05"),
+ (date(2020, 1, 2), "2020-01-02"),
+ ],
+)
+def test_sanitize_cell(value: object, expected: object) -> None:
+ assert _sanitize_cell(value) == expected
+
+
+def test_sanitize_cell_large_int_becomes_string() -> None:
+ assert _sanitize_cell(10**16) == str(10**16)
+
+
+def test_sanitize_cell_non_finite_floats_blanked() -> None:
+ assert _sanitize_cell(float("nan")) == ""
+ assert _sanitize_cell(float("inf")) == ""
+
+
+# --- StreamingXlsxWriter (round-trip via openpyxl) ---
+
+
+def _read_workbook(path: str) -> dict[str, list[list[object]]]:
+ openpyxl = pytest.importorskip("openpyxl")
+ workbook = openpyxl.load_workbook(path, read_only=True)
+ sheets = {
+ ws.title: [list(row) for row in ws.iter_rows(values_only=True)]
+ for ws in workbook.worksheets
+ }
+ workbook.close()
+ return sheets
+
+
+def test_writer_writes_one_sheet_per_chart(tmp_path: Path) -> None:
Review Comment:
**Suggestion:** Add a docstring summarizing the workbook-writing scenario
and expected sheet-level outcome checked by this test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The test function is newly introduced and has no docstring.
That matches the custom rule for newly added Python functions.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4416e0acb8be43889bf4c4e5556f9e5c&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=4416e0acb8be43889bf4c4e5556f9e5c&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/utils/excel_streaming_tests.py
**Line:** 116:116
**Comment:**
*Custom Rule: Add a docstring summarizing the workbook-writing scenario
and expected sheet-level outcome checked by this test.
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%2F41133&comment_hash=625dcb9e95e2accf26b6c9b3301f9d9d0011dfe9713368861876d6f07489452e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=625dcb9e95e2accf26b6c9b3301f9d9d0011dfe9713368861876d6f07489452e&reaction=dislike'>👎</a>
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -0,0 +1,212 @@
+# 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 __future__ import annotations
+
+import glob
+import os
+import tempfile
+from collections.abc import Iterator
+from contextlib import ExitStack
+from typing import Any
+from unittest import mock
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.utils import json
+
+MODULE = "superset.tasks.export_dashboard_excel"
+
+
+def _chart(chart_id: int, name: str, has_context: bool = True) ->
mock.MagicMock:
+ chart = mock.MagicMock()
+ chart.id = chart_id
+ chart.slice_name = name
+ chart.query_context = json.dumps({"queries": [{}]}) if has_context else
None
+ return chart
+
+
[email protected]
+def mocks() -> Iterator[dict[str, Any]]:
+ """Patch every external dependency of the task; keep the real xlsx
writer."""
+ with ExitStack() as stack:
+ # Use explicit MagicMock instances: patch() auto-creates async-flavored
+ # mocks for these targets (their real objects expose async members),
which
+ # would make calls like security_manager.get_user_by_id() return
coroutines.
+ patched = {
+ name: stack.enter_context(
+ mock.patch(f"{MODULE}.{name}", new=mock.MagicMock())
+ )
+ for name in (
+ "security_manager",
+ "db",
+ "get_charts_in_layout_order",
+ "get_dashboard_filter_context",
+ "ChartDataQueryContextSchema",
+ "ChartDataCommand",
+ "s3",
+ "email",
+ )
+ }
+ user = mock.MagicMock()
+ user.email = "[email protected]"
+ patched["security_manager"].get_user_by_id.return_value = user
+
+ dashboard = mock.MagicMock()
+ dashboard.id = 1
+ dashboard.dashboard_title = "Sales"
+ patched[
+ "db"
+
].session.query.return_value.filter_by.return_value.one_or_none.return_value =
( # noqa: E501
+ dashboard
+ )
+
+ patched["get_dashboard_filter_context"].return_value.extra_form_data =
{}
+ patched["s3"].generate_presigned_url.return_value =
"https://signed/file.xlsx";
+
+ patched["user"] = user
+ patched["dashboard"] = dashboard
+ yield patched
+
+
+def _run(job_id: str = "job-1") -> None:
+ from superset.tasks.export_dashboard_excel import export_dashboard_excel
+
+ export_dashboard_excel(
+ dashboard_id=1, user_id=2, active_data_mask={}, job_id=job_id
+ )
+
+
+def _no_temp_files_left(job_id: str) -> bool:
+ pattern = os.path.join(tempfile.gettempdir(), f"dash-export-{job_id}-*")
+ return glob.glob(pattern) == []
+
+
+def _read_sheets(path: str) -> dict[str, list[list[object]]]:
+ openpyxl = pytest.importorskip("openpyxl")
+ workbook = openpyxl.load_workbook(path, read_only=True)
+ sheets = {
+ ws.title: [list(r) for r in ws.iter_rows(values_only=True)]
+ for ws in workbook.worksheets
+ }
+ workbook.close()
+ return sheets
+
+
+def test_happy_path_uploads_and_emails(mocks: dict[str, Any]) -> None:
Review Comment:
**Suggestion:** Add a test docstring summarizing the success scenario being
validated (upload and success email behavior). [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
Test functions are Python functions, and this newly added test has no
docstring. That satisfies the custom rule violation being flagged.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ad68df3f4fbd4889bdbb8e5a0388e1de&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=ad68df3f4fbd4889bdbb8e5a0388e1de&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/tasks/test_export_dashboard_excel.py
**Line:** 110:110
**Comment:**
*Custom Rule: Add a test docstring summarizing the success scenario
being validated (upload and success email behavior).
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%2F41133&comment_hash=d3a3f6233cf16362564d46e7482a60bf1df2daf3c0a9957f9b5b99a7f0e9f9f9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=d3a3f6233cf16362564d46e7482a60bf1df2daf3c0a9957f9b5b99a7f0e9f9f9&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/s3_tests.py:
##########
@@ -0,0 +1,71 @@
+# 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 __future__ import annotations
+
+from unittest.mock import MagicMock, patch
+
+from superset.utils import s3
+
+
+@patch("superset.utils.s3.boto3.client")
+@patch("superset.utils.s3.current_app")
+def test_upload_file_to_s3(mock_app: MagicMock, mock_client_fn: MagicMock) ->
None:
+ mock_app.config = {"EXCEL_EXPORT_S3_CLIENT_KWARGS": {}}
+ client = mock_client_fn.return_value
+
+ s3.upload_file_to_s3("exports/out.xlsx", "my-bucket", "exports/1/abc.xlsx")
+
+ mock_client_fn.assert_called_once_with("s3")
+ client.upload_file.assert_called_once_with(
+ "exports/out.xlsx", "my-bucket", "exports/1/abc.xlsx"
+ )
+
+
+@patch("superset.utils.s3.boto3.client")
+@patch("superset.utils.s3.current_app")
+def test_client_kwargs_passthrough(
+ mock_app: MagicMock, mock_client_fn: MagicMock
+) -> None:
+ mock_app.config = {
Review Comment:
**Suggestion:** Add an inline docstring to this newly added test function to
explain the client kwargs passthrough scenario it verifies. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function, and it does not include a docstring.
The custom rule explicitly requires new functions and classes to be documented
inline, so the suggestion correctly identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=04eee27369084ecbbab6e28a3f752cdf&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=04eee27369084ecbbab6e28a3f752cdf&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/utils/s3_tests.py
**Line:** 40:43
**Comment:**
*Custom Rule: Add an inline docstring to this newly added test function
to explain the client kwargs passthrough scenario it verifies.
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%2F41133&comment_hash=2e6b40179c6a39c4e0231e9afbf4cd515fc0ee6393e269d5744b160ad3b4ccd3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=2e6b40179c6a39c4e0231e9afbf4cd515fc0ee6393e269d5744b160ad3b4ccd3&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/s3_tests.py:
##########
@@ -0,0 +1,71 @@
+# 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 __future__ import annotations
+
+from unittest.mock import MagicMock, patch
+
+from superset.utils import s3
+
+
+@patch("superset.utils.s3.boto3.client")
+@patch("superset.utils.s3.current_app")
+def test_upload_file_to_s3(mock_app: MagicMock, mock_client_fn: MagicMock) ->
None:
+ mock_app.config = {"EXCEL_EXPORT_S3_CLIENT_KWARGS": {}}
+ client = mock_client_fn.return_value
+
+ s3.upload_file_to_s3("exports/out.xlsx", "my-bucket", "exports/1/abc.xlsx")
+
+ mock_client_fn.assert_called_once_with("s3")
+ client.upload_file.assert_called_once_with(
+ "exports/out.xlsx", "my-bucket", "exports/1/abc.xlsx"
+ )
+
+
+@patch("superset.utils.s3.boto3.client")
+@patch("superset.utils.s3.current_app")
+def test_client_kwargs_passthrough(
+ mock_app: MagicMock, mock_client_fn: MagicMock
+) -> None:
+ mock_app.config = {
+ "EXCEL_EXPORT_S3_CLIENT_KWARGS": {
+ "endpoint_url": "http://minio:9000";,
+ "region_name": "us-east-1",
+ }
+ }
+
+ s3.upload_file_to_s3("exports/out.xlsx", "my-bucket", "k")
+
+ mock_client_fn.assert_called_once_with(
+ "s3", endpoint_url="http://minio:9000";, region_name="us-east-1"
+ )
+
+
+@patch("superset.utils.s3.boto3.client")
+@patch("superset.utils.s3.current_app")
+def test_generate_presigned_url(mock_app: MagicMock, mock_client_fn:
MagicMock) -> None:
+ mock_app.config = {"EXCEL_EXPORT_S3_CLIENT_KWARGS": {}}
Review Comment:
**Suggestion:** Add a concise docstring at the beginning of this new test
function to describe the presigned URL generation expectation. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function, and it lacks a docstring. That
matches the custom rule requiring newly added functions and classes to include
docstrings, so the violation is real.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e6ab38c4d6a2422282b20ee49825deff&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=e6ab38c4d6a2422282b20ee49825deff&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/utils/s3_tests.py
**Line:** 59:60
**Comment:**
*Custom Rule: Add a concise docstring at the beginning of this new test
function to describe the presigned URL generation expectation.
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%2F41133&comment_hash=1b220b962d42fb592a6ff959838b9d4be8475e14fb6af08a9ef5f5ed97fb0ee6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=1b220b962d42fb592a6ff959838b9d4be8475e14fb6af08a9ef5f5ed97fb0ee6&reaction=dislike'>👎</a>
##########
tests/unit_tests/utils/excel_streaming_tests.py:
##########
@@ -0,0 +1,170 @@
+# 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 __future__ import annotations
+
+from datetime import date, datetime
+from decimal import Decimal
+from pathlib import Path
+
+import pytest
+
+from superset.utils import excel_streaming
+from superset.utils.excel_streaming import (
+ _sanitize_cell,
+ sanitize_sheet_name,
+ StreamingXlsxWriter,
+)
+
+# --- sanitize_sheet_name ---
+
+
+def test_sheet_name_replaces_forbidden_chars() -> None:
+ assert sanitize_sheet_name("a/b:c*d?e[f]g\\h", set()) == "a_b_c_d_e_f_g_h"
+
+
+def test_sheet_name_truncated_to_31() -> None:
+ assert sanitize_sheet_name("x" * 40, set()) == "x" * 31
+
+
+def test_sheet_name_dedupes_case_insensitively() -> None:
+ used: set[str] = set()
+ assert sanitize_sheet_name("Sales", used) == "Sales"
+ assert sanitize_sheet_name("sales", used) == "sales~2"
+ assert sanitize_sheet_name("SALES", used) == "SALES~3"
+
+
+def test_sheet_name_dedupe_marker_respects_length_cap() -> None:
+ used: set[str] = set()
+ long_name = "y" * 31
+ assert sanitize_sheet_name(long_name, used) == long_name
+ assert sanitize_sheet_name(long_name, used) == "y" * 29 + "~2"
+
+
+def test_sheet_name_blank_falls_back() -> None:
+ assert sanitize_sheet_name(" ", set()) == "Sheet"
+
+
+def test_sheet_name_reserved_history_is_escaped() -> None:
+ assert sanitize_sheet_name("History", set()) == "History_"
+
+
+def test_sheet_name_strips_surrounding_apostrophes() -> None:
+ assert sanitize_sheet_name("'quoted'", set()) == "quoted"
+
+
+# --- _sanitize_cell ---
+
+
[email protected](
+ "value,expected",
+ [
+ (None, ""),
+ ("=SUM(A1)", "'=SUM(A1)"),
+ ("+1", "'+1"),
+ ("-1", "'-1"),
+ ("@handle", "'@handle"),
+ ("normal", "normal"),
+ (True, True),
+ (5, 5),
+ (1.5, 1.5),
+ (Decimal("2.5"), 2.5),
+ (datetime(2020, 1, 2, 3, 4, 5), "2020-01-02T03:04:05"),
+ (date(2020, 1, 2), "2020-01-02"),
+ ],
+)
+def test_sanitize_cell(value: object, expected: object) -> None:
+ assert _sanitize_cell(value) == expected
+
+
+def test_sanitize_cell_large_int_becomes_string() -> None:
+ assert _sanitize_cell(10**16) == str(10**16)
+
+
+def test_sanitize_cell_non_finite_floats_blanked() -> None:
+ assert _sanitize_cell(float("nan")) == ""
+ assert _sanitize_cell(float("inf")) == ""
+
+
+# --- StreamingXlsxWriter (round-trip via openpyxl) ---
+
+
+def _read_workbook(path: str) -> dict[str, list[list[object]]]:
+ openpyxl = pytest.importorskip("openpyxl")
+ workbook = openpyxl.load_workbook(path, read_only=True)
+ sheets = {
+ ws.title: [list(row) for row in ws.iter_rows(values_only=True)]
+ for ws in workbook.worksheets
+ }
+ workbook.close()
+ return sheets
+
+
+def test_writer_writes_one_sheet_per_chart(tmp_path: Path) -> None:
+ path = str(tmp_path / "out.xlsx")
+ writer = StreamingXlsxWriter(path)
+ assert writer.add_sheet("10 - First", ["a", "b"], [[1, 2], [3, 4]]) == 2
+ assert writer.add_sheet("20 - Second", ["c"], [["x"]]) == 1
+ writer.close()
+
+ sheets = _read_workbook(path)
+ assert list(sheets.keys()) == ["10 - First", "20 - Second"]
+ assert sheets["10 - First"] == [["a", "b"], [1, 2], [3, 4]]
+ assert sheets["20 - Second"] == [["c"], ["x"]]
+
+
+def test_writer_quotes_formula_cells(tmp_path: Path) -> None:
+ path = str(tmp_path / "out.xlsx")
+ writer = StreamingXlsxWriter(path)
+ writer.add_sheet("data", ["col"], [["=cmd()"]])
+ writer.close()
+
+ sheets = _read_workbook(path)
+ assert sheets["data"][1][0] == "'=cmd()"
+
+
+def test_writer_caps_rows_per_sheet(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+ monkeypatch.setattr(excel_streaming, "MAX_DATA_ROWS_PER_SHEET", 2)
+ path = str(tmp_path / "out.xlsx")
+ writer = StreamingXlsxWriter(path)
+ written = writer.add_sheet("data", ["col"], [[i] for i in range(5)])
+ writer.close()
+
+ assert written == 2
+ sheets = _read_workbook(path)
+ # header + 2 data rows
+ assert len(sheets["data"]) == 3
+
+
+def test_writer_empty_workbook_is_valid(tmp_path: Path) -> None:
+ path = str(tmp_path / "out.xlsx")
+ writer = StreamingXlsxWriter(path)
+ writer.close()
+
+ sheets = _read_workbook(path)
+ assert list(sheets.keys()) == ["Export"]
+
+
+def test_writer_summary_sheet(tmp_path: Path) -> None:
Review Comment:
**Suggestion:** Add a brief docstring describing the summary-sheet behavior
being verified. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added function and the final file does not include a
docstring for it.
The custom rule requires docstrings on newly added Python functions and
classes.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c341c3ca34854a7fb2a4a992898bbfec&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=c341c3ca34854a7fb2a4a992898bbfec&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/utils/excel_streaming_tests.py
**Line:** 163:163
**Comment:**
*Custom Rule: Add a brief docstring describing the summary-sheet
behavior being verified.
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%2F41133&comment_hash=b250ca3e90deabebfb803837f5aeae97bda43b0bdc93003afe742178b087f812&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=b250ca3e90deabebfb803837f5aeae97bda43b0bdc93003afe742178b087f812&reaction=dislike'>👎</a>
##########
tests/unit_tests/tasks/test_export_dashboard_excel.py:
##########
@@ -0,0 +1,212 @@
+# 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 __future__ import annotations
+
+import glob
+import os
+import tempfile
+from collections.abc import Iterator
+from contextlib import ExitStack
+from typing import Any
+from unittest import mock
+
+import pytest
+from celery.exceptions import SoftTimeLimitExceeded
+
+from superset.utils import json
+
+MODULE = "superset.tasks.export_dashboard_excel"
+
+
+def _chart(chart_id: int, name: str, has_context: bool = True) ->
mock.MagicMock:
+ chart = mock.MagicMock()
+ chart.id = chart_id
+ chart.slice_name = name
+ chart.query_context = json.dumps({"queries": [{}]}) if has_context else
None
+ return chart
+
+
[email protected]
+def mocks() -> Iterator[dict[str, Any]]:
+ """Patch every external dependency of the task; keep the real xlsx
writer."""
+ with ExitStack() as stack:
+ # Use explicit MagicMock instances: patch() auto-creates async-flavored
+ # mocks for these targets (their real objects expose async members),
which
+ # would make calls like security_manager.get_user_by_id() return
coroutines.
+ patched = {
+ name: stack.enter_context(
+ mock.patch(f"{MODULE}.{name}", new=mock.MagicMock())
+ )
+ for name in (
+ "security_manager",
+ "db",
+ "get_charts_in_layout_order",
+ "get_dashboard_filter_context",
+ "ChartDataQueryContextSchema",
+ "ChartDataCommand",
+ "s3",
+ "email",
+ )
+ }
+ user = mock.MagicMock()
+ user.email = "[email protected]"
+ patched["security_manager"].get_user_by_id.return_value = user
+
+ dashboard = mock.MagicMock()
+ dashboard.id = 1
+ dashboard.dashboard_title = "Sales"
+ patched[
+ "db"
+
].session.query.return_value.filter_by.return_value.one_or_none.return_value =
( # noqa: E501
+ dashboard
+ )
+
+ patched["get_dashboard_filter_context"].return_value.extra_form_data =
{}
+ patched["s3"].generate_presigned_url.return_value =
"https://signed/file.xlsx";
+
+ patched["user"] = user
+ patched["dashboard"] = dashboard
+ yield patched
+
+
+def _run(job_id: str = "job-1") -> None:
+ from superset.tasks.export_dashboard_excel import export_dashboard_excel
+
+ export_dashboard_excel(
+ dashboard_id=1, user_id=2, active_data_mask={}, job_id=job_id
+ )
+
+
+def _no_temp_files_left(job_id: str) -> bool:
Review Comment:
**Suggestion:** Add a docstring clarifying that this function checks whether
temporary export files were cleaned up. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new helper function and it lacks a docstring. That is a direct
violation of the stated docstring rule for newly added Python functions.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f8119006c934fdb904620bf57cbf901&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=0f8119006c934fdb904620bf57cbf901&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/tasks/test_export_dashboard_excel.py
**Line:** 94:94
**Comment:**
*Custom Rule: Add a docstring clarifying that this function checks
whether temporary export files were cleaned up.
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%2F41133&comment_hash=b770ef73a238041bee199e6b445bc1114fda806587f4b949922b23d1d2e2026c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=b770ef73a238041bee199e6b445bc1114fda806587f4b949922b23d1d2e2026c&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]
