seunggabi commented on issue #44512: URL: https://github.com/apache/superset/issues/44512#issuecomment-5986891312
Thanks for the review and the +1 @sfirke! On flag vs. config: I agree this is closer to a long-lived deployment policy than a rollout toggle, so a config key is a reasonable fit. I went with a feature flag only because of the existing frontend plumbing: flags already reach the browser through the bootstrap payload with a typed `FeatureFlag` enum, and `GRANULAR_EXPORT_CONTROLS` is a precedent for gating export behaviour that way. Switching is cheap, though. A `REQUIRE_DOWNLOAD_REASON` config key would be added to `FRONTEND_CONF_KEYS` in `superset/views/base.py` (with a matching frontend type) and the backend would read `app.config` instead of `is_feature_enabled`. The behaviour is unchanged: default `False`, dialog + `400` only when on, Alerts & Reports supplying their own reason. @michael-s-molina if you prefer the config key, I'm happy to update #44499 and the SIP text accordingly. Otherwise I'll keep the flag as proposed. Either way works for me. -- 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]
