seunggabi commented on PR #44499:
URL: https://github.com/apache/superset/pull/44499#issuecomment-5770203435
Thanks for the second pass. Going through the five suggestions from run
#7ae183 (plus the leftovers from #b860d1):
**Untested inline error path (`downloadReason.tsx`)** — fixed in b8e3dcc24.
`downloadReason.test.tsx` now renders the dialog content, triggers `onOk` with
a blank reason and asserts the `role="alert"` message appears, so a broken ref
wiring fails the test instead of silently no-op'ing.
**Weak xlsx URL assertion / unguarded `mock.calls` destructures** — fixed in
b8e3dcc24. The xlsx test pins the exact URL with `toBe`, every `mock.calls`
destructure is preceded by `toHaveBeenCalledTimes(1)`, and `lastConfig()`
asserts `Modal.confirm` was called first.
**Stacked confirm + reason modals (SQL Lab `ResultSet`)** — fixed in
b8e3dcc24. The reason is now collected *before* the LIMIT confirm, so at most
one dialog is open at a time; the confirm's `onConfirm` only redirects.
**Rejected requests not written to the event log (`chart/data`,
`sqllab/export*`)** — correct observation, and intentional. `log_context` has
no `try/finally` around its `yield`, so *any* exception raised inside these
handlers skips `log_with_context`; that is the existing behaviour for every
other raised error on these endpoints (`SupersetSecurityException`,
`SqlResultExportCommand` failures, …). The audit goal of this flag is "who
downloaded what, and why"; a rejected request downloads nothing, and the
frontend always supplies a reason when the flag is on, so a 400 here only ever
comes from an API client that hasn't been updated. `@statsd_metrics` still
counts the error. I'd rather not add a per-endpoint `try/except` just to log
the rejection; if maintainers want rejected attempts in the action log, the
right fix is a `finally` in `log_context` itself, which is out of scope for
this PR.
**API response contract change** — the `response_400(message=...)` shape
only ever existed in an earlier commit of this PR, never on `master`, so no
external consumer is affected. The SIP-40 `{"errors": [...]}` envelope is the
standard for `SupersetErrorException`, and the 400 + `download_reason`
parameter are documented in the OpenAPI docstrings and `UPDATING.md`
(ef0a67cc1).
**Duplicated OpenAPI `download_reason` param** (filtered) — the parameter is
repeated in four operation docstrings on purpose; Flask-AppBuilder's OpenAPI
generation has no shared `components/parameters` hook, and each entry is six
lines of YAML.
--
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]
