You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2024/04/02 04:35:08 UTC
[PR] feat: Slack Avatar integration [superset]
mistercrunch opened a new pull request, #27849:
URL: https://github.com/apache/superset/pull/27849
### SUMMARY
Our current Avatars are a little dry, using a limited variety of colors
and showing the user's initials. The app could use a little more
personality.
Now given we already have a slack integration and administrators can
configure their `SLACK_API_TOKEN`, I was thinking we can use this
integration to get Slack's avatars integrated in Superset. Given the
popularity of Slack and users familiarity with the avatars there,
I thought it'd be neat to bring a bit more color in Superset.
The approach here is centered around a new endpoint
`/api/v1/user/{id}/avatar.png`, which fetches from a new column in
UserAttribute (our one-to-one extension to FAB's User model) for
avatar_url, and redirects to the destination. When loading for the
first time, if information is empty and Slack is configured, we ask
Slack for the URL, which is secret, but serves the image from outside
the authentication part of their API.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562748940
##########
superset/models/user_attributes.py:
##########
@@ -39,6 +41,22 @@ class UserAttribute(Model, AuditMixinNullable):
user = relationship(
security_manager.user_model, backref="extra_attributes", foreign_keys=[user_id]
)
-
welcome_dashboard_id = Column(Integer, ForeignKey("dashboards.id"))
welcome_dashboard = relationship("Dashboard")
+ avatar_url = Column(String(100))
+
+
+class OwnershipMixin: # pylint: disable=too-few-public-methods
Review Comment:
I meant to add a note about this, it's likely to be useful in the future if/when we decide to incorporate more UserAttributes. Thought it may come handy. Maybe I add a `NOTE:`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1561501550
##########
superset/config.py:
##########
@@ -1328,6 +1328,8 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
# Slack API token for the superset reports, either string or callable
SLACK_API_TOKEN: Callable[[], str] | str | None = None
SLACK_PROXY = None
+# Whether Superset should use Slack avatars for users
+SLACK_ENABLE_AVATARS = True
Review Comment:
At one point we discussed and _might have completed_ moving all boolean configs to Feature Flags. If that's going to be a policy (which I don't think has _official_ consensus) then this should be too. CC @eschutho and @michael-s-molina in case they remember differently. I'm just a fan of consistency, in any case ;)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562493039
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
Review Comment:
we could just:
`@expose("/<user_id:int>/avatar.png", methods=("GET",))`
and then remove the `isdigit` check
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1564815827
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
Ok I made SLACK_ENABLE_AVATARS = False by default (at least for now), and commented the `TALISMAN_CONFIG` changes out, with a note about de-commenting the line if you set SLACK_ENABLE_AVATARS to True. Altered the UPDATING.md comment to reflect that.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1567835935
##########
superset/migrations/versions/2024-04-15_16-06_0dc386701747_.py:
##########
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""empty message
+
+Revision ID: 0dc386701747
Review Comment:
more about it here -> https://alembic.sqlalchemy.org/en/latest/branches.html
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562487735
##########
superset/utils/slack.py:
##########
@@ -0,0 +1,33 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from flask import current_app
+from slack_sdk import WebClient
+
+
+def get_slack_client() -> WebClient:
+ token: str = current_app.config["SLACK_API_TOKEN"]
+ if callable(token):
+ token = token()
+ return WebClient(token=token, proxy=current_app.config["SLACK_PROXY"])
+
+
+def get_user_avatar(email: str, client: WebClient = None) -> str:
+ client = client or get_slack_client()
+ response = client.users_lookupByEmail(email=email)
+ avatar_url = response.data.get("user").get("profile").get("image_192")
Review Comment:
nit: this could return `None` or raise
##########
superset/models/user_attributes.py:
##########
@@ -39,6 +41,22 @@ class UserAttribute(Model, AuditMixinNullable):
user = relationship(
security_manager.user_model, backref="extra_attributes", foreign_keys=[user_id]
)
-
welcome_dashboard_id = Column(Integer, ForeignKey("dashboards.id"))
welcome_dashboard = relationship("Dashboard")
+ avatar_url = Column(String(100))
+
+
+class OwnershipMixin: # pylint: disable=too-few-public-methods
Review Comment:
this is not used, delete?
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
Review Comment:
nit: we are avoiding to use SQLAlchemy queries at the API level, create a DAO for users?
or create a `find_user_by_id` on `SecurityManager`
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
+
+ # Saving the avatar url to the database
+ user_attrs = (
+ # already exists
+ db.session.query(UserAttribute)
+ .filter_by(user_id=user_id_as_int)
+ .first()
+ or
+ # create new
+ UserAttribute(user_id=user_id_as_int)
+ )
+ user_attrs.avatar_url = avatar_url
+ db.session.add(user_attrs)
+ db.session.commit()
Review Comment:
by the book, a GET should not change any state. What about creating a celery beat job that would collect all the existing user avatars?
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
Review Comment:
why are we allowing user_id to be a string and then checking `isdigit`?
we could just:
`@expose("/<user_id:int>/avatar.png", methods=("GET",))`
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
+
+ # Saving the avatar url to the database
+ user_attrs = (
+ # already exists
+ db.session.query(UserAttribute)
+ .filter_by(user_id=user_id_as_int)
+ .first()
+ or
+ # create new
+ UserAttribute(user_id=user_id_as_int)
+ )
+ user_attrs.avatar_url = avatar_url
+ db.session.add(user_attrs)
+ db.session.commit()
+
+ if avatar_url:
+ return redirect(avatar_url, code=301)
+
+ return Response(status=204)
+
+ except NoAuthorizationError:
Review Comment:
this is probably only possible on `get_user_avatar`, we could just:
``` python
try:
avatar_url = get_user_avatar(user.email)
except NoAuthorizationError:
return self.response_401()
```
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
Review Comment:
how about making `get_user_avatar` more generic? would be nice if people could use Gravatar or others also
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -62,3 +63,23 @@ def test_get_me_anonymous(self, mock_g):
mock_g.user = security_manager.get_anonymous_user
rv = self.client.get(meUri)
self.assertEqual(401, rv.status_code)
+
+
+class TestUserApi(SupersetTestCase):
+ AVATAR_URL = "https://example.com/avatar.png"
+
+ def test_avatar_with_invalid_user(self):
+ self.login(ADMIN_USERNAME)
+ response = self.client.get("/api/v1/user/NOT_A_USER/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+ response = self.client.get("/api/v1/user/999/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+
+ @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL)
+ def test_avatar_with_valid_user(self, mock):
+ self.login(ADMIN_USERNAME)
+
+ response = self.client.get("/api/v1/user/1/avatar.png")
+ mock.assert_called_once_with("admin@fab.org")
+ assert response.status_code == 301
+ assert response.headers["Location"] == self.AVATAR_URL
Review Comment:
I think we are currently preferring to use `unit_tests` to test API endpoints. for example if the endpoint just calls a `Command` we can easily mock it, and test all exceptions, validations and responses.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1566593892
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
+
+ # Saving the avatar url to the database
+ user_attrs = (
+ # already exists
+ db.session.query(UserAttribute)
+ .filter_by(user_id=user_id_as_int)
+ .first()
+ or
+ # create new
+ UserAttribute(user_id=user_id_as_int)
+ )
+ user_attrs.avatar_url = avatar_url
+ db.session.add(user_attrs)
+ db.session.commit()
Review Comment:
This is the only element I did not fully address because it's effectively the feature here. Note that mutations will be very very sparse here. Only if:
- feature is on (SLACK_ENABLE_AVATAR + SLACK_API_TOKEN)
- it's the **very first time** the avatar is requested for a given target user in the whole environment, updating the UserAttributes.avatar_url once with a short string
In all normal day-to-day
- upon a new browser requesting an avatar, the GET does as a normal get (a one cell database lookup)
- 301 gets cached, target image gets cached
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1547136207
##########
superset/dashboards/api.py:
##########
@@ -179,6 +179,9 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
"owners.id",
"owners.first_name",
"owners.last_name",
+ "owners.email",
+ "owners.id",
+ # "owners.user_attributes.avatar_url",
Review Comment:
@dpgaspar I think I might need your help to get this to work or to find some workaround. Here I need a handle on `avatar_url` which I stored in the `UserAttributes` model, which is one-to-one with FAB's `User` model, and looks like there's some sweet magic here that's a bit beyond me - at least for now.
I considered other approaches, like extending FAB's User model, but I don't think we have a handle on it in Superset RN. Other approach would be to support multiple dots here.
Curious on your thoughts
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1564797018
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
Think I should tie this to the feature flag? I could also add as a comment in `config.py` ("you'll want to make sure...") near the `SLACK_ENABLE_AVATARS`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1561788473
##########
superset/config.py:
##########
@@ -1328,6 +1328,8 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
# Slack API token for the superset reports, either string or callable
SLACK_API_TOKEN: Callable[[], str] | str | None = None
SLACK_PROXY = None
+# Whether Superset should use Slack avatars for users
+SLACK_ENABLE_AVATARS = True
Review Comment:
Yes there's a fine line between configuration setting and feature flag. Happy to move to FF if we think it belong there
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562902595
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
Review Comment:
I like the idea of adding a configuration hook here, though I'd have to define the interface prematurely (say guessing `def GET_USER_AVATAR_FUNC(user) -> avatar_url_str` ), and probably think of support for non `png` file types. I'd punt and say we shouldn't prematurely generalize the feature. Makes me think the main issue with the PR is the `.png` assumption. I have to test whether the jpg redirect happens to work or whether I can make it work.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1547131639
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -99,6 +99,7 @@ class CeleryConfig:
WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
SQLLAB_CTAS_NO_LIMIT = True
+SLACK_API_TOKEN = os.getenv("SLACK_API_TOKEN")
Review Comment:
It could be nice to have a gitignored `superset_config_local.py` (similar to `.env-local` above)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562752025
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
They shouldn't, especially as these are just default conf settings, and we don't store anything. Personally can't think of a rule anyone would be breaking by turning this on, plus it's off by default.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562908645
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -62,3 +63,23 @@ def test_get_me_anonymous(self, mock_g):
mock_g.user = security_manager.get_anonymous_user
rv = self.client.get(meUri)
self.assertEqual(401, rv.status_code)
+
+
+class TestUserApi(SupersetTestCase):
+ AVATAR_URL = "https://example.com/avatar.png"
+
+ def test_avatar_with_invalid_user(self):
+ self.login(ADMIN_USERNAME)
+ response = self.client.get("/api/v1/user/NOT_A_USER/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+ response = self.client.get("/api/v1/user/999/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+
+ @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL)
+ def test_avatar_with_valid_user(self, mock):
+ self.login(ADMIN_USERNAME)
+
+ response = self.client.get("/api/v1/user/1/avatar.png")
+ mock.assert_called_once_with("admin@fab.org")
+ assert response.status_code == 301
+ assert response.headers["Location"] == self.AVATAR_URL
Review Comment:
Here I just followed what was there before, but may be an opportunity to improve this, let me see.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1566588802
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
Made all defaults more conservative, `SLACK_ENABLE_AVATARS` is False by default and users have to alter their `TALISMAN_CONFIG` manually, resolving this thread.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27849:
URL: https://github.com/apache/superset/pull/27849#issuecomment-2031092195
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27849?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `53.19149%` with `22 lines` in your changes are missing coverage. Please review.
> Project coverage is 59.99%. Comparing base [(`0d0e47a`)](https://app.codecov.io/gh/apache/superset/commit/0d0e47acf796fe289e9c47c21506f427204d8f45?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`65f1461`)](https://app.codecov.io/gh/apache/superset/pull/27849?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/superset/pull/27849?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/users/api.py](https://app.codecov.io/gh/apache/superset/pull/27849?src=pr&el=tree&filepath=superset%2Fviews%2Fusers%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvdXNlcnMvYXBpLnB5) | 36.36% | [21 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27849?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/slack.py](https://app.codecov.io/gh/apache/superset/pull/27849?src=pr&el=tree&filepath=superset%2Futils%2Fslack.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2xhY2sucHk=) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27849?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #27849 +/- ##
==========================================
- Coverage 69.83% 59.99% -9.85%
==========================================
Files 1920 1921 +1
Lines 75242 75280 +38
Branches 8423 8423
==========================================
- Hits 52546 45163 -7383
- Misses 20635 28056 +7421
Partials 2061 2061
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.92% <44.68%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.62% <44.68%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.75% <53.19%> (-20.41%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/27849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.76% <53.19%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27849?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1561503023
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
I'm not sure anymore if the ASF will care about this or not.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1567637221
##########
superset/migrations/versions/2024-04-15_16-06_0dc386701747_.py:
##########
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""empty message
+
+Revision ID: 0dc386701747
Review Comment:
Same as above, what would this migration do?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "padbk (via GitHub)" <gi...@apache.org>.
padbk commented on PR #27849:
URL: https://github.com/apache/superset/pull/27849#issuecomment-2318012133
I've tried enabling this in 4.1.0rc2, but I get the same behaviour as this issue https://github.com/apache/superset/issues/28833 I think it is caused by the combination of websockets with this feature.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #27849:
URL: https://github.com/apache/superset/pull/27849
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1567636900
##########
superset/migrations/versions/2024-04-11_00-49_bbf146925528_.py:
##########
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""empty message
+
+Revision ID: bbf146925528
Review Comment:
What does this migration do? See that upgrade and downgrade are just pass
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1563072835
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
I think it's that when any user traffic hits a 3rd party service, they could get IPs/analytics from it, in which case they'll want to have a DPA in place. They might already have one with Slack, which would make this a moot point, but if not, we can probably get one one, if this is even a concern.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27849:
URL: https://github.com/apache/superset/pull/27849#issuecomment-2058064791
@dpgaspar I addressed pretty much all feedback. Personally prefer avoiding the CELERY_BEAT configuration, but open to consider that approach too if we decide it's preferable.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1561585283
##########
superset/config.py:
##########
@@ -1328,6 +1328,8 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
# Slack API token for the superset reports, either string or callable
SLACK_API_TOKEN: Callable[[], str] | str | None = None
SLACK_PROXY = None
+# Whether Superset should use Slack avatars for users
+SLACK_ENABLE_AVATARS = True
Review Comment:
We need to have a discussion about feature flags in a specific meeting. I was researching a little bit about feature flags best practices and [this](https://docs.getunleash.io/topics/feature-flags/short-lived-feature-flags) caught my attention. Specifically, the following:
> The most common use case for feature flags is to protect new functionality. That means that when the roll-out of new functionality is complete, the feature flag should be removed from the code and archived.
> Feature flags should not be used for static application configuration.
If we followed a similar model, we would never have the categorization we currently have in [FEATURE_FLAGS.md](https://github.com/apache/superset/blob/master/RESOURCES/FEATURE_FLAGS.md). We would just assume that all feature flags should be removed once the feature is complete and what we have as `STABLE` would actually be application configurations, and ideally [changeable at runtime](https://docs.getunleash.io/topics/feature-flags/runtime-control). What I like about this model is the clarity/simplicity.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562793384
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -62,3 +63,23 @@ def test_get_me_anonymous(self, mock_g):
mock_g.user = security_manager.get_anonymous_user
rv = self.client.get(meUri)
self.assertEqual(401, rv.status_code)
+
+
+class TestUserApi(SupersetTestCase):
+ AVATAR_URL = "https://example.com/avatar.png"
+
+ def test_avatar_with_invalid_user(self):
+ self.login(ADMIN_USERNAME)
+ response = self.client.get("/api/v1/user/NOT_A_USER/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+ response = self.client.get("/api/v1/user/999/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+
+ @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL)
+ def test_avatar_with_valid_user(self, mock):
+ self.login(ADMIN_USERNAME)
+
+ response = self.client.get("/api/v1/user/1/avatar.png")
+ mock.assert_called_once_with("admin@fab.org")
+ assert response.status_code == 301
+ assert response.headers["Location"] == self.AVATAR_URL
Review Comment:
Maybe I can just `git mv` the file
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1566593892
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
+
+ # Saving the avatar url to the database
+ user_attrs = (
+ # already exists
+ db.session.query(UserAttribute)
+ .filter_by(user_id=user_id_as_int)
+ .first()
+ or
+ # create new
+ UserAttribute(user_id=user_id_as_int)
+ )
+ user_attrs.avatar_url = avatar_url
+ db.session.add(user_attrs)
+ db.session.commit()
Review Comment:
This is the only element I did not fully address because it's effectively the feature here. Note that mutations will be very very sparse here. Mutation only happen if:
- feature is on (`SLACK_ENABLE_AVATAR` + `SLACK_API_TOKEN`)
- it's the **very first time** the avatar is requested for a given target user in the whole environment, doing a tiny update of UserAttributes.avatar_url once with a normal-sized string
In all normal day-to-day
- upon a new browser requesting an avatar, the GET does as a normal get (a one cell database lookup)
- 301 gets cached
- target image gets cached
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1566589124
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -62,3 +63,23 @@ def test_get_me_anonymous(self, mock_g):
mock_g.user = security_manager.get_anonymous_user
rv = self.client.get(meUri)
self.assertEqual(401, rv.status_code)
+
+
+class TestUserApi(SupersetTestCase):
+ AVATAR_URL = "https://example.com/avatar.png"
+
+ def test_avatar_with_invalid_user(self):
+ self.login(ADMIN_USERNAME)
+ response = self.client.get("/api/v1/user/NOT_A_USER/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+ response = self.client.get("/api/v1/user/999/avatar.png")
+ assert response.status_code == 404 # Assuming no user found leads to 404
+
+ @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL)
+ def test_avatar_with_valid_user(self, mock):
+ self.login(ADMIN_USERNAME)
+
+ response = self.client.get("/api/v1/user/1/avatar.png")
+ mock.assert_called_once_with("admin@fab.org")
+ assert response.status_code == 301
+ assert response.headers["Location"] == self.AVATAR_URL
Review Comment:
@dpgaspar I ended up creating a very simple DAO and writing basic unit tests for it, and expanded on the api tests here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1567835548
##########
superset/migrations/versions/2024-04-15_16-06_0dc386701747_.py:
##########
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""empty message
+
+Revision ID: 0dc386701747
Review Comment:
Hey, sorry about the confusion, but in alembic the migration "chain" can split and you end up with multiple `heads`, typically when you rebase and there's been some other migration. This migration merges the 2 heads. In theory it's possible to do the equivalent of a rebase, but that requires a bit of work. In this case I "git rebased" twice during the lifecycle of this PR and had to merge the heads 2 times, leaving this 2 empty-ish migrations.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1547130615
##########
.gitignore:
##########
@@ -110,5 +110,6 @@ release.json
messages.mo
docker/requirements-local.txt
+docker/.env-local
Review Comment:
Note: I was working with `docker-compose` and had to set my `SLACK_API_KEY` somewhere, and was afraid that I'd accidentally push it to the repo while working in `docker./env` so I added a new **gitignored** docker/.env-local
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1564797018
##########
superset/config.py:
##########
@@ -1453,6 +1455,7 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
"data:",
"https://apachesuperset.gateway.scarf.sh",
"https://static.scarf.sh/",
+ "https://avatars.slack-edge.com",
Review Comment:
Think I should tie this to the feature flag?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1562902595
##########
superset/views/users/api.py:
##########
@@ -93,3 +97,85 @@ def get_my_roles(self) -> Response:
return self.response_401()
user = bootstrap_user_data(g.user, include_perms=True)
return self.response(200, result=user)
+
+
+class UserRestApi(BaseSupersetApi):
+ """An API to get information about users"""
+
+ resource_name = "user"
+ openapi_spec_tag = "User"
+ openapi_spec_component_schemas = (UserResponseSchema,)
+
+ @expose("/<user_id>/avatar.png", methods=("GET",))
+ @safe
+ def avatar(self, user_id: str) -> Response:
+ """Get a redirect to the avatar's URL for the user with the given ID.
+ ---
+ get:
+ summary: Get the user avatar
+ description: >-
+ Gets the avatar URL for the user with the given ID, or returns a 401 error
+ if the user is unauthenticated.
+ parameters:
+ - in: path
+ name: user_id
+ required: true
+ description: The ID of the user
+ schema:
+ type: string
+ responses:
+ 301:
+ description: A redirect to the user's avatar URL
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ """
+ try:
+ avatar_url = None
+
+ if not user_id.isdigit():
+ return self.response_404()
+
+ user_id_as_int: int = int(user_id)
+
+ user = (
+ db.session.query(security_manager.user_model)
+ .filter_by(id=user_id_as_int)
+ .first()
+ )
+ if not user:
+ return self.response_404()
+
+ # 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:
+ # Fetching the avatar url from slack
+ avatar_url = get_user_avatar(user.email)
Review Comment:
I like the idea of adding a configuration hook here, though I'd have to define the interface prematurely (say guessing `def GET_USER_AVATAR_FUNC(user) -> str` ), and probably think of support for non `png` file types. I'd punt and say we shouldn't prematurely generalize the feature. Makes me think the main issue with the PR is the `.png` assumption. I have to test whether the jpg redirect happens to work or whether I can make it work.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1563018685
##########
superset/config.py:
##########
@@ -1328,6 +1328,8 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
# Slack API token for the superset reports, either string or callable
SLACK_API_TOKEN: Callable[[], str] | str | None = None
SLACK_PROXY = None
+# Whether Superset should use Slack avatars for users
+SLACK_ENABLE_AVATARS = True
Review Comment:
After town hall conversation, I think we want to rethink guidelines and categorization of FF. Outside the scope of this PR so I'll resolve this convo here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Slack Avatar integration [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27849:
URL: https://github.com/apache/superset/pull/27849#discussion_r1564818341
##########
superset/utils/slack.py:
##########
@@ -0,0 +1,33 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from flask import current_app
+from slack_sdk import WebClient
+
+
+def get_slack_client() -> WebClient:
+ token: str = current_app.config["SLACK_API_TOKEN"]
+ if callable(token):
+ token = token()
+ return WebClient(token=token, proxy=current_app.config["SLACK_PROXY"])
+
+
+def get_user_avatar(email: str, client: WebClient = None) -> str:
+ client = client or get_slack_client()
+ response = client.users_lookupByEmail(email=email)
+ avatar_url = response.data.get("user").get("profile").get("image_192")
Review Comment:
ok I improved the error handling here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org