-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve Slack integration to allow notifying private, public, and group channels (fixes #8569) #9046
Conversation
Contextual Security AnalysisAs DryRun Security performs checks, we’ll summarize them here. You can always dive into the detailed results in the section below for checks.
Chat with your AI-powered Security Buddy by typing Install and configure more repositories at DryRun Security |
dojo/db_migrations/0194_remove_usercontactinfo_slack_user_id.py
Outdated
Show resolved
Hide resolved
1773f30
to
f88d1e7
Compare
Hi @tomaszn, we reviewed this PR but weren't able to replicate the problems mentioned. Could you please provide additional details on the issues you experienced? |
@devGregA, sure. The simplest use case is setting up notifications for |
model_name="usercontactinfo", | ||
name="slack_user_id", | ||
), | ||
] |
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 about
migrations.RemoveField(
model_name="usercontactinfo",
name="slack_username",
),
migrations.RenameField(
model_name="usercontactinfo",
old_name="slack_user_id",
new_name="slack_username",
),
Looks easier compare to RunPython
https://docs.djangoproject.com/en/5.0/ref/migration-operations/#renamefield
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.
Indeed, thanks. And then AlterField
needs to be moved to the end, to set up verbose_name
etc.
Moreover, I should set max_length=81
(#
plus 80 characters of the channel name).
Hi @tomaszn , I tested creating and linking the Slack integration and I was able to post two different kinds of notifications:
I was able to define the Slack Channel for posts on the Slack-app side, when I created and installed it to my workplace. I updated the documentation with what I found: 750e548 Currently, Dojo does not support any customized or filtered notifications, or posts to multiple slack channels. Is that what you're intending to do by changing the notification targets from Slack Usernames to Slack Channels? If you wanted to add that feature, I think it would be more appropriate to add a Slack Channel mapping field on a Product, rather than on each user's profile which is where the Slack email address currently exists. |
Hello @paulOsinski, thank you for your input here, and for the update to the documentation.
Correct. And by creating "virtual" users and assigning them Slack channels I can customize notifications. Several channels can get notifications about events in independently configured sets of products.
In my case, I need several Slack channels to be informed about various events in the products. So a simple 1:1 mapping would not be enough. I acknowledge that the current solution is not perfect and serves more as a workaround. A fully-featured notification manager capable of handling mappings and preferences is something we should aim for in the future. Please bear in mind that this changeset was prepared in a feature freeze situation. The goal had to be obtained with a minimal changes not to disrupt work of others :) |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Close as stale |
This change enables users to set private, public, or group channels as the target for Slack notifications.
It achieves this by removing the
slack_user_id
intermediary field, which was filled with a response after querying the Slack API with the user's email, and which limited the notification targets to users' private Slack channels. Before removal, the value is copied intoslack_username
field, overwriting the email address, which is no longer needed (and no longer works).Detailed explanations are in #8569.
Test results
slack_username
is set to a channel names, with and without a "#" prefix.Documentation
No changes in the documentation are needed.
However, I noticed that the Slack administration page looks now differently, so I updated the relevant screen shot in the "Notifications" section.
Checklist
This checklist is for your information.