chore(config): Migrating ENABLE_JAVASCRIPT_CONTROLS from app config to a feature flag

[rusackas]

SUMMARY

This PR moves the boolean enablement of the JS controls (i.e. custom tooltips in the DeckGL viz) from app configuration to feature flags. Two main reasons:

1. To better support deployments who manage feature flags on the fly... this seems more fitting in that area of configuration
2. To move the feature (with its self-admitted risks, and unknown level of community adoption) to the Feature Flag lifecycle, wherein we may more systematically either secure the feature's development and enablement, or deprecate it and replace it with a newer feature (e.g. jinja/markdown tooltips).

This is considered a breaking change since certain deployments may rely on enabling/disabling this feature, and they may have to adapt their configuration to set it in a new place.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

n/a

TESTING INSTRUCTIONS

Turn on the flag, and see that it works! We can (and shall!) do this on an ephemeral environment on this PR.

ADDITIONAL INFORMATION

• Has associated issue: Fixes [2.0] change the ENABLE_JAVASCRIPT_CONTROLS from a config value to a feature flag #18909
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• [ x ] Removes existing feature or API well, kind of

[codecov]

Codecov Report

Merging #19113 (94e458c) into master (3a78165) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master   #19113   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files        1645     1645           
  Lines       63524    63522    -2     
  Branches     6462     6462           
=======================================
- Hits        42260    42259    -1     
+ Misses      19592    19590    -2     
- Partials     1672     1673    +1     

Flag	Coverage Δ
hive	52.62% <100.00%> (-0.01%)	⬇️
javascript	51.27% <0.00%> (+<0.01%)	⬆️
mysql	81.83% <100.00%> (-0.01%)	⬇️
postgres	81.88% <100.00%> (-0.01%)	⬇️
presto	52.47% <100.00%> (-0.01%)	⬇️
python	82.31% <100.00%> (-0.01%)	⬇️
sqlite	81.63% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ackages/superset-ui-core/src/utils/featureFlags.ts	100.00% <ø> (ø)
...reset-chart-deckgl/src/utilities/Shared_DeckGL.jsx	86.48% <0.00%> (+2.27%)	⬆️
superset/config.py	91.79% <ø> (-0.03%)	⬇️
superset/views/utils.py	80.93% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a78165...94e458c. Read the comment docs.

[rusackas]

Note that in testing, I was encountering potential bugs and usability issues with the JS controls features. However, I was encountering this in both app config and feature flag modes. The intent of this PR is to move the configuration, not to stop and address issues with the feature itself.

[villebro]

It's my understanding that the feature flag dict in config.py is meant for temporarily enabling features that aren't yet considered stable. However, anything that needs to be permanently customizable should be a global config flag. For this reason it can be argued that the ENABLE_TEMPLATE_PROCESSING flag should also be moved back to a global config flag. It would be good to hear other people's thoughts on this - @dpgaspar @john-bodley , any thoughts?

[rusackas]

It's my understanding that the feature flag dict in config.py is meant for temporarily enabling features that aren't yet considered stable. However, anything that needs to be permanently customizable should be a global config flag. For this reason it can be argued that the ENABLE_TEMPLATE_PROCESSING flag should also be moved back to a global config flag. It would be good to hear other people's thoughts on this - @dpgaspar @john-bodley , any thoughts?

Agreed, and a part of the motivation to move this back to a feature flag is to de-stabilize the feature. Due to potential bugs, UX difficulties, and security concerns, I think we can move to deprecate it and replace it with a safer/simpler feature in coming versions.

But I must confess this also selfishly provides our org with an easier means to tweak the setting easily with the way we manage deployments, while its deprecation is being considered

[suddjian]

Javascript controls are a feature, this is a flag for that feature, it's a feature flag. QED 😁