codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3471239860
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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 parameter and return type hints to this new test method
to satisfy full typing requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added test method has no type hints on its mock parameter and no return
annotation. That is a valid match for the typing rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a63e171d4c704d7493ffd1decd1f969b&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=a63e171d4c704d7493ffd1decd1f969b&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:** 3349:3349
**Comment:**
*Custom Rule: Add parameter and return type hints to this new test
method to satisfy full 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%2F41133&comment_hash=04fa1f3588a10a3218b6ebef10ce37d63a8966e07690c02b828d263d7f70cb4a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=04fa1f3588a10a3218b6ebef10ce37d63a8966e07690c02b828d263d7f70cb4a&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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 hints for the mocked parameter and the return type
in this new test method signature. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method omits type hints for both the mocked parameter
and the return type. That matches the custom typing rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=70efde15a6b64bbd9b21cda40ecb63ae&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=70efde15a6b64bbd9b21cda40ecb63ae&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:** 3296:3296
**Comment:**
*Custom Rule: Add type hints for the mocked parameter and the return
type in this new test method signature.
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=46a33dee7f83590b3ad7cca3798bce62d3ee9df367eb69c06e962455c6a69227&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=46a33dee7f83590b3ad7cca3798bce62d3ee9df367eb69c06e962455c6a69227&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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 a return type annotation to this new test method so the
function signature is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method with no return type annotation. The
rule requires new Python functions/methods to be fully typed, including return
values, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=760b23d67c7a417eb52480f6c8e7806d&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=760b23d67c7a417eb52480f6c8e7806d&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:** 3283:3283
**Comment:**
*Custom Rule: Add a return type annotation to this new test method so
the function signature is 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%2F41133&comment_hash=2bc2ad8ebaa7ef8fca2595b2bbe165bdb4410ad81760fbc50de490cdf67c0499&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=2bc2ad8ebaa7ef8fca2595b2bbe165bdb4410ad81760fbc50de490cdf67c0499&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"
+ )
Review Comment:
**Suggestion:** Add a short docstring to this new test function describing
the behavior being validated. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function and it has no docstring. The
custom rule requires newly added functions and classes to be documented inline,
so the suggestion identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b101a14bac0e4220b318aaad92c45bc0&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=b101a14bac0e4220b318aaad92c45bc0&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:** 24:35
**Comment:**
*Custom Rule: Add a short docstring to this new test function
describing the behavior being validated.
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=ba9ad10009163c0636cac1d5ed2766d7490c20a69af6fc8fefde9c0bdbee7be4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=ba9ad10009163c0636cac1d5ed2766d7490c20a69af6fc8fefde9c0bdbee7be4&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"
+ )
Review Comment:
**Suggestion:** Add a docstring to this newly added test function to explain
the client kwargs passthrough scenario. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added function lacks a docstring. Since the rule explicitly
requires docstrings for newly added Python functions, the suggestion is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=63f4104d13344945b23d18f26d9c00d2&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=63f4104d13344945b23d18f26d9c00d2&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:** 38:54
**Comment:**
*Custom Rule: Add a docstring to this newly added test function to
explain the client kwargs passthrough scenario.
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=39db0a73a70fa77378d91656fe3c4eddc589a827538a6c6a4485de33a2444845&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=39db0a73a70fa77378d91656fe3c4eddc589a827538a6c6a4485de33a2444845&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": {}}
+ client = mock_client_fn.return_value
+ client.generate_presigned_url.return_value = "https://signed.example/abc";
+
+ url = s3.generate_presigned_url("my-bucket", "exports/1/abc.xlsx", 86400)
+
+ assert url == "https://signed.example/abc";
+ client.generate_presigned_url.assert_called_once_with(
+ "get_object",
+ Params={"Bucket": "my-bucket", "Key": "exports/1/abc.xlsx"},
+ ExpiresIn=86400,
Review Comment:
**Suggestion:** Include a concise docstring in this new test function
describing the presigned URL behavior under test. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new test function and it does not include a docstring. The custom
rule applies to newly added functions, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4492382985264300a79a893a3bf6d29a&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=4492382985264300a79a893a3bf6d29a&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:** 57:70
**Comment:**
*Custom Rule: Include a concise docstring in this new test function
describing the presigned URL behavior under 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=ee8524e7df9f0e2fef3ce07caccd5c30e0771315928ceb3ea72058a40644f3e3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=ee8524e7df9f0e2fef3ce07caccd5c30e0771315928ceb3ea72058a40644f3e3&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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 explicit type annotations for the mock parameter and
return value on this new test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The method signature is missing both parameter annotations and a return type
annotation. Since this is newly added Python code, it should be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=280aab7da6404d73a645a9a811612ba2&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=280aab7da6404d73a645a9a811612ba2&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:** 3327:3327
**Comment:**
*Custom Rule: Add explicit type annotations for the mock parameter and
return value on this new test method.
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=4ec798022bea27d63ff88e66a122d0a3d3c32a3564dc2fa4786c8910f36b5be0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=4ec798022bea27d63ff88e66a122d0a3d3c32a3564dc2fa4786c8910f36b5be0&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3279,6 +3280,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:** Add type hints for the mocked parameter and the return type
in this newly added test method. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another new test method without parameter type hints or a return
annotation. It violates the rule that newly added Python code should be fully
typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2d880386e7494258af4e8470c9cab605&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=2d880386e7494258af4e8470c9cab605&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:** 3308:3308
**Comment:**
*Custom Rule: Add type hints for the mocked parameter and the return
type in this newly added test method.
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=bbaae6a32283f2c96ca57cd676aa54fef54d4d03cbde8381a4eff3df409dc7c6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=bbaae6a32283f2c96ca57cd676aa54fef54d4d03cbde8381a4eff3df409dc7c6&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)
Review Comment:
**Suggestion:** Add a docstring to this test function so the intent of the
edge case is explicitly documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function without a docstring.
The rule explicitly flags new functions that lack docstrings, so this is a
verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7f5928ff118d440fb4384d4059331c64&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=7f5928ff118d440fb4384d4059331c64&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:** 93:94
**Comment:**
*Custom Rule: Add a docstring to this test function so the intent of
the edge case is explicitly documented.
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=16aaacfd5f7b1213b982cc73caa884309e528d0dda67d3006bf80ec465e81dfb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=16aaacfd5f7b1213b982cc73caa884309e528d0dda67d3006bf80ec465e81dfb&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()"
Review Comment:
**Suggestion:** Add a concise docstring to this test function to describe
the formula-escaping expectation. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added function and it lacks a docstring.
The custom rule applies to new Python functions, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bfbcc1f4052e45b482e851ece137f3f5&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=bfbcc1f4052e45b482e851ece137f3f5&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:** 129:136
**Comment:**
*Custom Rule: Add a concise docstring to this test function to describe
the formula-escaping 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=01b1606923772beb32f689f065fa461f026add6d16a81e0f9b1e85243ec6922e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=01b1606923772beb32f689f065fa461f026add6d16a81e0f9b1e85243ec6922e&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]
