bito-code-review[bot] commented on code in PR #44499:
URL: https://github.com/apache/superset/pull/44499#discussion_r4070451169
##########
superset/sqllab/api.py:
##########
@@ -332,6 +341,7 @@ def export_csv(self, client_id: str) -> CsvResponse:
"GRANULAR_EXPORT_CONTROLS"
) and not security_manager.can_access("can_export_data", "Superset"):
return self.response_403()
+ check_download_reason() # 400 via the SupersetErrorException handler
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate as it addresses a gap in test coverage for the
newly introduced download gate. Adding these test cases ensures that the
`check_download_reason()` logic and the subsequent 400 error propagation are
correctly exercised under both flag-enabled and flag-disabled conditions,
preventing potential regressions.
**superset/sqllab/api.py**
```
) and not security_manager.can_access("can_export_data", "Superset"):
return self.response_403()
check_download_reason() # 400 via the SupersetErrorException handler
```
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -369,31 +374,59 @@ const ResultSet = ({
schema: query?.schema,
};
- const handleDownloadCsv = (event: React.MouseEvent<HTMLElement>) => {
- logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
-
- if (limitingFactor === LimitingFactor.Dropdown && limit === rowsCount)
{
- event.preventDefault();
+ const handleDownloadCsv = async (
+ event: React.MouseEvent<HTMLElement>,
+ ) => {
+ const confirmLimit =
+ limitingFactor === LimitingFactor.Dropdown && limit === rowsCount;
+ if (!isDownloadReasonRequired() && !confirmLimit) {
+ // Flag off: keep the anchor's native navigation (cmd/ctrl-click,
+ // "open in new tab", ...) exactly as before.
+ logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
+ return;
+ }
+ // Take over the navigation so the reason / confirm dialog runs first.
+ event.preventDefault();
Review Comment:
<!-- Bito Reply -->
The suggestion is correct. Moving `event.preventDefault()` before the early
return ensures that the navigation is intercepted even when the limit
confirmation dialog is triggered, preventing the browser from simultaneously
initiating the native download of the CSV URL.
**superset-frontend/src/SqlLab/components/ResultSet/index.tsx**
```
const confirmLimit =
limitingFactor === LimitingFactor.Dropdown && limit === rowsCount;
event.preventDefault();
if (!isDownloadReasonRequired() && !confirmLimit) {
// Flag off: keep the anchor's native navigation (cmd/ctrl-click,
// "open in new tab", ...) exactly as before.
logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
return;
}
```
##########
superset/sqllab/api.py:
##########
@@ -410,6 +429,7 @@ def export_streaming_csv(self) -> Response:
"GRANULAR_EXPORT_CONTROLS"
) and not security_manager.can_access("can_export_data", "Superset"):
return self.response_403()
+ check_download_reason() # 400 via the SupersetErrorException handler
Review Comment:
<!-- Bito Reply -->
The suggestion provided by Bito is valid and appropriate. It correctly
identifies a gap in test coverage for the new `check_download_reason()` call in
`export_streaming_csv`, which is necessary to ensure the 400 error path is
exercised when the `REQUIRE_DOWNLOAD_REASON` flag is enabled. Adding these
tests will improve the robustness of the streaming export functionality.
--
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]
