-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Slack Avatar integration #27849
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27849 +/- ##
==========================================
- Coverage 69.83% 59.98% -9.85%
==========================================
Files 1920 1921 +1
Lines 75242 75284 +42
Branches 8423 8423
==========================================
- Hits 52546 45160 -7386
- Misses 20635 28062 +7427
- Partials 2061 2062 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
77c5a6a
to
1162ff7
Compare
superset/views/users/api.py
Outdated
) | ||
user_attrs.avatar_url = avatar_url | ||
db.session.add(user_attrs) | ||
db.session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
4d5a9d0
to
9a412fc
Compare
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.
9a412fc
to
376ac64
Compare
@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. |
# under the License. | ||
"""empty message | ||
|
||
Revision ID: bbf146925528 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this migration do? See that upgrade and downgrade are just pass
# under the License. | ||
"""empty message | ||
|
||
Revision ID: 0dc386701747 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, what would this migration do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more about it here -> https://alembic.sqlalchemy.org/en/latest/branches.html
I've tried enabling this in 4.1.0rc2, but I get the same behaviour as this issue #28833 I think it is caused by the combination of websockets with this feature. |
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.Note that I'm also introducing the config
SLACK_ENABLE_AVATARS
which isFalse
by default, and added a note toUPDATING.md
for release managers to switch it on if desired.The approach here is centered around a new endpoint
/api/v1/user/{id}/avatar.png
, which fetches from a new column inUserAttribute (our one-to-one extension to FAB's User model) name
avatar_url
, and redirects (301) to the destination. When loading for the first time, if information is empty and Slack is configured, we call Slack for the avatar URL, which has a secret location, but serves the image from outside the authentication part of their API once known. If the feature is off or no avatar url is set for the user, we simply return a204
(no-content), preventing from logging errors or anything negative as this is expected.The
avatar_url
field could be used programmatically in environments to serve other urls, but at this time I did not implement a GUI for users to self-set their avatar for instance. I think in most cases people will want to integrate with some external service (MSFT Team, Discord, ...). The approach I took here could be expanded to work with Gravatar for instance.About Perf
This approach while unconventional (is it?) should perform very well. Say in an environment with 1000 users, the Slack API should trigger once-ish for each user in the database to fill the
avatar_url
for that user (1000 api hits). For each user navigating the website, a single hit to/api/v1/users/{some_user_id}/avatar.png
where the database is looked up is sufficient (up to 1M hits, but super low-key endpoint), after which the permanent redirect is cached client-side. The 204 should get cached like any other request following the configured cache-control in place.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF