chore: move SLACK_ENABLE_AVATARS from config to feature flag (apache#…

…30274)

RESOURCES/FEATURE_FLAGS.md

@@ -68,6 +68,13 @@ These features flags are **safe for production**. They have been tested and will
 - DISABLE_LEGACY_DATASOURCE_EDITOR
 
 ### Flags retained for runtime configuration
+
+Currently some of our feature flags act as dynamic configurations that can changed
+on the fly. This acts in contradiction with the typical ephemeral feature flag use case,
+where the flag is used to mature a feature, and eventually deprecated once the feature is
+solid. Eventually we'll likely refactor these under a more formal "dynamic configurations" managed
+independently. This new framework will also allow for non-boolean configurations.
+
 - ALERTS_ATTACH_REPORTS
 - ALLOW_ADHOC_SUBQUERY
 - DASHBOARD_RBAC [(docs)](https://superset.apache.org/docs/using-superset/creating-your-first-dashboard#manage-access-to-dashboards)
@@ -79,6 +86,7 @@ These features flags are **safe for production**. They have been tested and will
 - ESCAPE_MARKDOWN_HTML
 - LISTVIEWS_DEFAULT_CARD_VIEW
 - SCHEDULED_QUERIES [(docs)](https://superset.apache.org/docs/configuration/alerts-reports)
+- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information)
 - SQLLAB_BACKEND_PERSISTENCE
 - SQL_VALIDATORS_BY_ENGINE [(docs)](https://superset.apache.org/docs/configuration/sql-templating)
 - THUMBNAILS [(docs)](https://superset.apache.org/docs/configuration/cache)

UPDATING.md

@@ -60,6 +60,7 @@ assists people when migrating to a new version.
 - [29264](https://github.com/apache/superset/pull/29264) Slack has updated its file upload api, and we are now supporting this new api in Superset, although the Slack api is not backward compatible. The original Slack integration is deprecated and we will require a new Slack scope `channels:read` to be added to Slack workspaces in order to use this new api. In an upcoming release, we will make this new Slack scope mandatory and remove the old Slack functionality.
 - [29798](https://github.com/apache/superset/pull/29798) Since 3.1.0, the intial schedule for an alert or report was mistakenly offset by the specified timezone's relation to UTC. The initial schedule should now begin at the correct time.
 - [30021](https://github.com/apache/superset/pull/30021) The `dev` layer in our Dockerfile no long includes firefox binaries, only Chromium to reduce bloat/docker-build-time
+- [30274](https://github.com/apache/superset/pull/30274) Moved SLACK_ENABLE_AVATAR from config.py to the feature flag framework, please adapt your configs
 
 ### Potential Downtime
 

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

@@ -59,6 +59,7 @@ export enum FeatureFlag {
   Thumbnails = 'THUMBNAILS',
   UseAnalagousColors = 'USE_ANALAGOUS_COLORS',
   ForceSqlLabRunAsync = 'SQLLAB_FORCE_RUN_ASYNC',
+  SlackEnableAvatars = 'SLACK_ENABLE_AVATARS',
 }
 
 export type ScheduleQueriesProps = {

superset-frontend/src/components/FacePile/index.tsx

@@ -16,15 +16,15 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { useSelector } from 'react-redux';
 import {
   getCategoricalSchemeRegistry,
   styled,
+  isFeatureEnabled,
+  FeatureFlag,
   SupersetTheme,
 } from '@superset-ui/core';
 import { Tooltip } from 'src/components/Tooltip';
 import { Avatar } from 'src/components';
-import { RootState } from 'src/views/store';
 import { getRandomColor } from './utils';
 
 interface FacePileProps {
@@ -55,16 +55,13 @@ const StyledGroup = styled(Avatar.Group)`
 `;
 
 export default function FacePile({ users, maxCount = 4 }: FacePileProps) {
-  const enableAvatars = useSelector<RootState, boolean>(
-    state => state.common?.conf?.SLACK_ENABLE_AVATARS,
-  );
   return (
     <StyledGroup maxCount={maxCount}>
       {users.map(({ first_name, last_name, id }) => {
         const name = `${first_name} ${last_name}`;
         const uniqueKey = `${id}-${first_name}-${last_name}`;
         const color = getRandomColor(uniqueKey, colorList);
-        const avatarUrl = enableAvatars
+        const avatarUrl = isFeatureEnabled(FeatureFlag.SlackEnableAvatars)
           ? `/api/v1/user/${id}/avatar.png`
           : undefined;
         return (

superset/config.py

@@ -545,6 +545,10 @@ class D3TimeFormat(TypedDict, total=False):
     "SQLLAB_FORCE_RUN_ASYNC": False,
     # Set to True to to enable factory resent CLI command
     "ENABLE_FACTORY_RESET_COMMAND": False,
+    # Whether Superset should use Slack avatars for users.
+    # If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed
+    # domains in your TALISMAN_CONFIG
+    "SLACK_ENABLE_AVATARS": False,
 }
 
 # ------------------------------
@@ -1427,11 +1431,6 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 SLACK_API_TOKEN: Callable[[], str] | str | None = None
 SLACK_PROXY = None
 
-# Whether Superset should use Slack avatars for users.
-# If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed
-# domains in your TALISMAN_CONFIG
-SLACK_ENABLE_AVATARS = False
-
 # The webdriver to use for generating reports. Use one of the following
 # firefox
 #   Requires: geckodriver and firefox installations

superset/views/base.py

@@ -114,7 +114,6 @@
     "NATIVE_FILTER_DEFAULT_ROW_LIMIT",
     "PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET",
     "JWT_ACCESS_CSRF_COOKIE_NAME",
-    "SLACK_ENABLE_AVATARS",
     "SQLLAB_QUERY_RESULT_TIMEOUT",
 )
 

superset/views/users/api.py

@@ -19,7 +19,7 @@
 from flask_jwt_extended.exceptions import NoAuthorizationError
 from sqlalchemy.orm.exc import NoResultFound
 
-from superset import app
+from superset import app, is_feature_enabled
 from superset.daos.user import UserDAO
 from superset.utils.slack import get_user_avatar, SlackClientError
 from superset.views.base_api import BaseSupersetApi
@@ -143,11 +143,12 @@ def avatar(self, user_id: int) -> Response:
         # fetch from the one-to-one relationship
         if len(user.extra_attributes) > 0:
             avatar_url = user.extra_attributes[0].avatar_url
-
-        should_fetch_slack_avatar = app.config.get(
-            "SLACK_ENABLE_AVATARS"
-        ) and app.config.get("SLACK_API_TOKEN")
-        if not avatar_url and should_fetch_slack_avatar:
+        slack_token = app.config.get("SLACK_API_TOKEN")
+        if (
+            not avatar_url
+            and slack_token
+            and is_feature_enabled("SLACK_ENABLE_AVATARS")
+        ):
             try:
                 # Fetching the avatar url from slack
                 avatar_url = get_user_avatar(user.email)

tests/integration_tests/users/api_tests.py

@@ -22,7 +22,7 @@
 from superset import security_manager
 from superset.utils import json, slack  # noqa: F401
 from tests.integration_tests.base_tests import SupersetTestCase
-from tests.integration_tests.conftest import with_config
+from tests.integration_tests.conftest import with_config, with_feature_flags
 from tests.integration_tests.constants import ADMIN_USERNAME
 
 meUri = "/api/v1/me/"
@@ -81,7 +81,8 @@ def test_avatar_valid_user_no_avatar(self):
         response = self.client.get("/api/v1/user/1/avatar.png", follow_redirects=False)
         assert response.status_code == 204
 
-    @with_config({"SLACK_API_TOKEN": "dummy", "SLACK_ENABLE_AVATARS": True})
+    @with_config({"SLACK_API_TOKEN": "dummy"})
+    @with_feature_flags(SLACK_ENABLE_AVATARS=True)
     @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL)
     def test_avatar_with_valid_user(self, mock):
         self.login(ADMIN_USERNAME)