From 2157fe3f28eb73b0c77b32d90719995e83a8d3b1 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 16 Sep 2024 16:07:11 -0700 Subject: [PATCH] chore: move SLACK_ENABLE_AVATARS from config to feature flag (#30274) (cherry picked from commit f315a4f02c3ed3311d501602e525ea1d0c0fde8b) --- RESOURCES/FEATURE_FLAGS.md | 8 ++++++++ UPDATING.md | 1 + .../superset-ui-core/src/utils/featureFlags.ts | 1 + superset-frontend/src/components/FacePile/index.tsx | 9 +++------ superset/config.py | 9 ++++----- superset/views/base.py | 1 - superset/views/users/api.py | 13 +++++++------ tests/integration_tests/users/api_tests.py | 5 +++-- 8 files changed, 27 insertions(+), 20 deletions(-) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 2c4d4d0c3e0..8bc275d21ac 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/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/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) diff --git a/UPDATING.md b/UPDATING.md index 9fb5a67613e..544aee5bf66 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -58,6 +58,7 @@ assists people when migrating to a new version. backend, as well as the .json files used by the frontend. If you were doing anything before as part of your bundling to expose translation packages, it's probably not needed anymore. - [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. +- [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 diff --git a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts index 67f3785ab60..8801706c55f 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts +++ b/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 = { diff --git a/superset-frontend/src/components/FacePile/index.tsx b/superset-frontend/src/components/FacePile/index.tsx index 51e44d248e6..d67d9d8d368 100644 --- a/superset-frontend/src/components/FacePile/index.tsx +++ b/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( - state => state.common?.conf?.SLACK_ENABLE_AVATARS, - ); return ( {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 ( diff --git a/superset/config.py b/superset/config.py index 098fdb4a368..6206925260e 100644 --- a/superset/config.py +++ b/superset/config.py @@ -545,6 +545,10 @@ DEFAULT_FEATURE_FLAGS: dict[str, bool] = { "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, } # ------------------------------ @@ -1428,11 +1432,6 @@ EMAIL_REPORTS_CTA = "Explore in Superset" 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 diff --git a/superset/views/base.py b/superset/views/base.py index adfb8b3db57..33ba43b2f68 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -114,7 +114,6 @@ FRONTEND_CONF_KEYS = ( "NATIVE_FILTER_DEFAULT_ROW_LIMIT", "PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET", "JWT_ACCESS_CSRF_COOKIE_NAME", - "SLACK_ENABLE_AVATARS", "SQLLAB_QUERY_RESULT_TIMEOUT", ) diff --git a/superset/views/users/api.py b/superset/views/users/api.py index a7000b6b96c..82089fe84fe 100644 --- a/superset/views/users/api.py +++ b/superset/views/users/api.py @@ -19,7 +19,7 @@ from flask_appbuilder.api import expose, safe 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 @@ class UserRestApi(BaseSupersetApi): # 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) diff --git a/tests/integration_tests/users/api_tests.py b/tests/integration_tests/users/api_tests.py index 4153a5bd08f..7894b1856c1 100644 --- a/tests/integration_tests/users/api_tests.py +++ b/tests/integration_tests/users/api_tests.py @@ -22,7 +22,7 @@ from unittest.mock import patch 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 @@ class TestUserApi(SupersetTestCase): 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)