seunggabi commented on code in PR #44499:
URL: https://github.com/apache/superset/pull/44499#discussion_r4070449686
##########
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:
Added in 416bff6: `test_export_csv_requires_download_reason` (flag on: 400
with the SIP-40 envelope and the command never runs; with `?download_reason=`
the export proceeds) and `test_export_csv_without_reason_when_flag_off` in
`tests/unit_tests/sqllab/api_test.py`.
##########
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:
Added in 416bff6: `test_export_streaming_requires_download_reason` posts
without a reason (400, command not constructed) and then with `download_reason`
as a form field (200, streaming command runs).
##########
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:
The fast path is `!isDownloadReasonRequired() && !confirmLimit`, so when
`confirmLimit` is true the handler does not return early; it falls through to
`event.preventDefault()` before `showConfirm`. The native navigation is only
kept when neither the reason dialog nor the LIMIT confirm is needed, which is
exactly the case where the old code also let the anchor navigate.
--
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]
