feat(notifications platform): Send workflow and deploy Slack notifications#25286
Conversation
ceorourke
force-pushed
the
ceorourke/API-1807
branch
from
dc97e8c to
a2b3775
Compare
| ]: | ||
| short_id = context["group"].qualified_short_id | ||
| title = context["activity_name"] | ||
| text = context["text_description"] |
There was a problem hiding this comment.
I'm not the happiest with how this looks in the message - the short ID isn't a link and it's redundant since I have the short ID as a link in the footer. If I modify it I'll need custom logic for each type of notification to build the sentence though and I'm not sure it's worth it. We can make this better later on so that we can get something out.
ceorourke
changed the title
mgaeta
left a comment
mgaeta
left a comment
There was a problem hiding this comment.
I left some suggested refactors
| def build_notification_attachment(notification): | ||
| links = notification.get_dm_links() | ||
| notification_type = notification.__class__.__name__ | ||
| footer = build_notification_footer(links, notification_type) |
There was a problem hiding this comment.
I think you should change build_notification_footer to take the ActivityNotification object and internally calculate links.
| referrer = re.sub("Notification$", "Slack", notification_type) | ||
| settings_url = urljoin(links["settings_url"], "?referrer=" + referrer) | ||
|
|
||
| if notification_type == "ReleaseActivityNotification": |
There was a problem hiding this comment.
Comparing against class name can be flakey because the names can change. You should instead compare against the value returned by .get_category().
| "link_names": 1, | ||
| "attachments": json.dumps(attachment), | ||
| } | ||
| client = SlackClient() |
There was a problem hiding this comment.
Move client = SlackClient() out of the for loop.
| for external_actor in external_actors: | ||
| attachment = [build_notification_attachment(notification)] | ||
| payload = { | ||
| "token": external_actor.integration.metadata["access_token"], |
There was a problem hiding this comment.
All Slack external actors will have an integrations, but we should be defensive and check to make sure it exists.
| return user_context | ||
|
|
||
| def get_title(self) -> str: | ||
| return str(self.get_context()["activity_name"]) |
There was a problem hiding this comment.
return self.get_activity_name()
| return str(self.get_context()["activity_name"]) | ||
|
|
||
| def get_dm_text(self) -> str: | ||
| return str(self.get_context()["text_description"]) |
There was a problem hiding this comment.
This looks likely to break because not every ActivityNotification has text_description in context. It might make more sense to add dm_description to .get_context() and use that field in build_attachment.
2 participants
The notifications platform project allows users to receive workflow, project alert, and deploy notifications via Slack. This PR builds the payload to send Slack notifications for the following activities: