gabotorresruiz commented on code in PR #43340:
URL: https://github.com/apache/superset/pull/43340#discussion_r3867274331
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -83,9 +89,28 @@ export const useDownloadMenuItems = (
canExportImage,
} = props;
- const { addDangerToast, addSuccessToast } = useToasts();
+ const { addDangerToast, addSuccessToast, addInfoToast } = useToasts();
const dataMask = useSelector((state: RootState) => state.dataMask);
- const SCREENSHOT_NODE_SELECTOR = ".dashboard";
+ // Embedded (iframe) sessions may have no email address, so they get
+ // delivery-neutral copy and a poll window that outlives the task budget.
+ const isEmbedded = isEmbeddedDashboard();
Review Comment:
Fixed in c75abca73a alongside the other identity comment: delivery copy and
the poll window now follow the requester identity, so sessions the backend
cannot email get the neutral copy and the 12 minute window. The task also
records a running status when a worker picks the job up, and the client
restarts its wait window on observing it, so queue delay is no longer counted
against the export and a queued guest export is not orphaned. Jest tests cover
both.
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -190,36 +215,37 @@ export const useDownloadMenuItems = (
download_url: downloadUrl,
message,
} = json as ExportStatusResponse;
- if (status === "ready") {
+ if (status === 'ready') {
if (downloadUrl) {
- window.location.href = downloadUrl;
+ redirect(downloadUrl);
}
- addSuccessToast(t("Your export is ready and downloading."));
+ addSuccessToast(t('Your export is ready and downloading.'));
return;
}
- if (status === "error") {
+ if (status === 'error') {
addDangerToast(
- message || t("Sorry, something went wrong. Try again later."),
+ message || t('Sorry, something went wrong. Try again later.'),
);
return;
}
- if (Date.now() - startedAt > EXPORT_STATUS_POLL_TIMEOUT_MS) {
+ if (Date.now() - startedAt > pollTimeoutMs) {
addDangerToast(
- t("Your export is taking longer than expected. Try again later."),
+ t('Your export is taking longer than expected. Try again later.'),
);
return;
}
+ addExportPendingToast();
Review Comment:
Kept as is deliberately: this mirrors useDownloadScreenshot's existing
repeating info toast with noDuplicate, and consistency between the two download
flows matters more to us than reducing the re announcements. Happy to revisit
both flows together in a follow up if the repetition proves to be a real
problem.
##########
superset/dashboards/api.py:
##########
@@ -1922,17 +1937,27 @@ def download_xlsx(self, job_id: uuid.UUID) ->
WerkzeugResponse:
description: The job_id from the export_xlsx response
responses:
302:
- description: Redirect to a pre-signed S3 download URL
+ description: Redirect to a signed storage download URL
410:
description: The link is unknown, expired, or the export failed
+ 501:
+ description: Excel export is not configured on this server
"""
resolved = resolve_download_link(job_id)
if resolved is None:
return self.response(410, message="This download link has
expired.")
bucket, key = resolved
- return redirect(
- s3.generate_presigned_url(bucket, key, PRESIGNED_URL_TTL_SECONDS)
+ storage_backend = current_app.config["EXPORT_STORAGE"].get("backend")
Review Comment:
Addressed in c75abca73a with a deliberately minimal shape: the link record
stores the dotted path of the uploading backend, and the download redirect
refuses to sign when the configured backend differs, returning the same 410 as
an expired link. Legacy records without the field keep signing with the
configured backend. Reconstructing the original backend felt like overreach,
since a replaced backend instance and its constructor kwargs no longer exist in
config; failing clearly beats redirecting to a URL for the wrong provider.
Integration tests cover the mismatch and the legacy record path.
##########
docs/docs/using-superset/exporting-dashboard-data.mdx:
##########
@@ -72,23 +91,25 @@ will not register.
## Configuration keys
-| Key | Default | Description
|
-| ------------------------------- | ---------------------- |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
-| `EXCEL_EXPORT_S3_BUCKET` | `None` | Destination
bucket. Required; `501` if unset.
|
-| `EXCEL_EXPORT_S3_KEY_PREFIX` | `"dashboard-exports/"` | Key prefix:
`{prefix}{dashboard_id}/{job_id}.xlsx`.
|
-| `EXCEL_EXPORT_LINK_TTL_SECONDS` | `86400` | Lifetime of the
pre-signed download URL (24h).
|
-| `EXCEL_EXPORT_S3_CLIENT_KWARGS` | `{}` | Extra kwargs for
`boto3.client("s3", ...)` — e.g. `region_name`, or `endpoint_url` for
MinIO/LocalStack.
|
-| `EXCEL_EXPORT_TABLE_VIZ_TYPES` | `None` | Viz types kept
tabular in **Export Images to Excel** mode; every other type is embedded as an
image. `None` uses the built-in default (`table`, `pivot_table`,
`pivot_table_v2`). |
-| `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER` | `None` | Optional
`Callable[[form_data_dict], dict \| None]` to build a query context for a chart
missing a saved one, tried before the built-in form-data rebuild. Point it at a
service that runs the chart's real frontend `buildQuery` to faithfully export
viz types the built-in rebuild can't handle. Must return `None` when it can't
build faithfully, so the export falls back. |
-
-Credentials and region resolve through the standard boto3 chain (environment
-variables, shared config, or instance role) unless overridden via
-`EXCEL_EXPORT_S3_CLIENT_KWARGS`. The worker needs `s3:PutObject` on the bucket.
+| Key | Default |
Description
|
+| -------------------------------------- | ---------------------- |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
+| `EXPORT_STORAGE["bucket"]` | unset |
Destination bucket. Required; `501` if unset.
|
+| `EXPORT_STORAGE["backend"]` | unset | Storage
backend instance: `S3ExportStorage()` (`superset.utils.s3`),
`GCSExportStorage()` (`superset.utils.gcs`), or a custom
`superset.utils.export_storage.ExportStorage` implementation. Required; `501`
if unset. |
Review Comment:
Fixed in c75abca73a: the Limitations section now documents the guest polling
flow (auto download, no email fallback, images unavailable), and SMTP is
documented as needed for email delivery only.
--
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]
