Skip to content
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

chore(slack): correctly type issue message builder #74876

Merged
merged 10 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ module = [
"sentry.integrations.slack.actions.form",
"sentry.integrations.slack.client",
"sentry.integrations.slack.integration",
"sentry.integrations.slack.message_builder.issues",
"sentry.integrations.slack.message_builder.notifications.digest",
"sentry.integrations.slack.message_builder.notifications.issues",
"sentry.integrations.slack.notifications",
Expand Down
16 changes: 7 additions & 9 deletions src/sentry/integrations/message_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,17 @@


def format_actor_options(
actors: Sequence[Team | RpcUser], use_block_kit: bool = False
actors: Sequence[Team | RpcUser], is_slack: bool = False
) -> Sequence[Mapping[str, str]]:
sort_func: Callable[[Mapping[str, str]], Any] = lambda actor: actor["text"]
if use_block_kit:
if is_slack:
sort_func = lambda actor: actor["text"]["text"]
return sorted((format_actor_option(actor, use_block_kit) for actor in actors), key=sort_func)
return sorted((format_actor_option(actor, is_slack) for actor in actors), key=sort_func)


def format_actor_option(actor: Team | RpcUser, use_block_kit: bool = False) -> Mapping[str, str]:
def format_actor_option(actor: Team | RpcUser, is_slack: bool = False) -> Mapping[str, str]:
if isinstance(actor, RpcUser):
if use_block_kit:
if is_slack:
return {
"text": {
"type": "plain_text",
Expand All @@ -40,8 +40,8 @@ def format_actor_option(actor: Team | RpcUser, use_block_kit: bool = False) -> M
}

return {"text": actor.get_display_name(), "value": f"user:{actor.id}"}
if isinstance(actor, Team):
if use_block_kit:
elif isinstance(actor, Team):
if is_slack:
return {
"text": {
"type": "plain_text",
Expand All @@ -51,8 +51,6 @@ def format_actor_option(actor: Team | RpcUser, use_block_kit: bool = False) -> M
}
return {"text": f"#{actor.slug}", "value": f"team:{actor.id}"}

raise NotImplementedError
Comment on lines 53 to -54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would probably change the ifs to elifs and else: raise AssertionError('unreachable')



def build_attachment_title(obj: Group | GroupEvent) -> str:
ev_metadata = obj.get_event_metadata()
Expand Down
119 changes: 44 additions & 75 deletions src/sentry/integrations/slack/message_builder/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
import logging
from collections.abc import Mapping, Sequence
from datetime import datetime
from typing import Any
from typing import Any, TypedDict

import orjson
from django.core.exceptions import ObjectDoesNotExist
from sentry_relay.processing import parse_release

from sentry import tagstore
from sentry.api.endpoints.group_details import get_group_global_count
from sentry.constants import LOG_LEVELS_MAP
from sentry.constants import LOG_LEVELS
from sentry.eventstore.models import GroupEvent
from sentry.identity.services.identity import RpcIdentity, identity_service
from sentry.integrations.message_builder import (
Expand Down Expand Up @@ -109,9 +109,9 @@ def build_assigned_text(identity: RpcIdentity, assignee: str) -> str | None:
except ObjectDoesNotExist:
return None

if actor.is_team:
if isinstance(assigned_actor, Team):
assignee_text = f"#{assigned_actor.slug}"
elif actor.is_user:
elif isinstance(assigned_actor, RpcUser):
assignee_identity = identity_service.get_identity(
filter={
"provider_id": identity.idp_id,
Expand Down Expand Up @@ -147,40 +147,20 @@ def build_action_text(identity: RpcIdentity, action: MessageAction) -> str | Non
return f"*Issue {status} by <@{identity.external_id}>*"


def build_tag_fields(
event_for_tags: Any, tags: set[str] | None = None
) -> Sequence[Mapping[str, str | bool]]:
fields = []
if tags:
event_tags = event_for_tags.tags if event_for_tags else []
for key, value in event_tags:
std_key = tagstore.backend.get_standardized_key(key)
if std_key not in tags:
continue

labeled_value = tagstore.backend.get_tag_value_label(key, value)
fields.append(
{
"title": std_key.encode("utf-8"),
"value": labeled_value.encode("utf-8"),
"short": True,
}
)
return fields


def format_release_tag(value: str, event: GroupEvent | Group):
def format_release_tag(value: str, event: GroupEvent | None) -> str:
"""Format the release tag using the short version and make it a link"""
if not event:
return ""

path = f"/releases/{value}/"
url = event.project.organization.absolute_url(path)
release_description = parse_release(value, json_loads=orjson.loads).get("description")
return f"<{url}|{release_description}>"


def get_tags(
group: Group,
event_for_tags: Any,
tags: set[str] | None = None,
event_for_tags: GroupEvent | None,
tags: set[str] | list[tuple[str]] | None = None,
) -> Sequence[Mapping[str, str | bool]]:
"""Get tag keys and values for block kit"""
fields = []
Expand Down Expand Up @@ -243,41 +223,30 @@ def get_context(group: Group) -> str:
return context_text.rstrip()


def get_option_groups_block_kit(group: Group) -> Sequence[Mapping[str, Any]]:
all_members = group.project.get_members_as_rpc_users()
members = list({m.id: m for m in all_members}.values())
teams = group.project.teams.all()

option_groups = []
if teams:
team_options = format_actor_options(teams, True)
option_groups.append(
{"label": {"type": "plain_text", "text": "Teams"}, "options": team_options}
)
class OptionGroup(TypedDict):
label: Mapping[str, str]
options: Sequence[Mapping[str, Any]]

if members:
member_options = format_actor_options(members, True)
option_groups.append(
{"label": {"type": "plain_text", "text": "People"}, "options": member_options}
)
return option_groups


def get_group_assignees(group: Group) -> Sequence[Mapping[str, Any]]:
"""Get teams and users that can be issue assignees for block kit"""
def get_option_groups(group: Group) -> Sequence[OptionGroup]:
all_members = group.project.get_members_as_rpc_users()
members = list({m.id: m for m in all_members}.values())
teams = group.project.teams.all()

option_groups = []
if teams:
for team in teams:
option_groups.append({"label": team.slug, "value": f"team:{team.id}"})
team_option_group: OptionGroup = {
"label": {"type": "plain_text", "text": "Teams"},
"options": format_actor_options(teams, True),
}
option_groups.append(team_option_group)

if members:
for member in members:
option_groups.append({"label": member.email, "value": f"user:{member.id}"})

member_option_group: OptionGroup = {
"label": {"type": "plain_text", "text": "People"},
"options": format_actor_options(members, True),
}
option_groups.append(member_option_group)
return option_groups


Expand All @@ -298,20 +267,23 @@ def get_suggested_assignees(
logger.info("Skipping suspect committers because release does not exist.")
except Exception:
logger.exception("Could not get suspect committers. Continuing execution.")

if suggested_assignees:
suggested_assignees = dedupe_suggested_assignees(suggested_assignees)
assignee_texts = []

for assignee in suggested_assignees:
# skip over any suggested assignees that are the current assignee of the issue, if there is any
if assignee.is_user and not (
isinstance(current_assignee, RpcUser) and assignee.id == current_assignee.id
):
assignee_as_user = assignee.resolve()
assignee_texts.append(assignee_as_user.get_display_name())
elif assignee.is_team and not (
if assignee.is_team and not (
isinstance(current_assignee, Team) and assignee.id == current_assignee.id
):
assignee_texts.append(f"#{assignee.slug}")
elif assignee.is_user and not (
isinstance(current_assignee, RpcUser) and assignee.id == current_assignee.id
):
assignee_as_user = assignee.resolve()
if isinstance(assignee_as_user, RpcUser):
assignee_texts.append(assignee_as_user.get_display_name())
return assignee_texts
return []

Expand Down Expand Up @@ -417,7 +389,7 @@ def _assign_button() -> MessageAction:
label="Select Assignee...",
type="select",
selected_options=format_actor_options([assignee], True) if assignee else [],
option_groups=get_option_groups_block_kit(group),
option_groups=get_option_groups(group),
)
return assign_button

Expand Down Expand Up @@ -477,10 +449,10 @@ def escape_text(self) -> bool:

def get_title_block(
self,
rule_id: int,
notification_uuid: str,
event_or_group: GroupEvent | Group,
has_action: bool,
rule_id: int | None = None,
notification_uuid: str | None = None,
) -> SlackBlock:
title_link = get_title_link(
self.group,
Expand All @@ -504,11 +476,7 @@ def get_title_block(
else ACTIONED_CATEGORY_TO_EMOJI.get(self.group.issue_category)
)
elif is_error_issue:
level_text = None
for k, v in LOG_LEVELS_MAP.items():
if self.group.level == v:
level_text = k

level_text = LOG_LEVELS[self.group.level]
title_emoji = LEVEL_TO_EMOJI.get(level_text)
else:
title_emoji = CATEGORY_TO_EMOJI.get(self.group.issue_category)
Expand Down Expand Up @@ -584,7 +552,8 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock:
# If an event is unspecified, use the tags of the latest event (if one exists).
event_for_tags = self.event or self.group.get_latest_event()

obj = self.event if self.event is not None else self.group
event_or_group: Group | GroupEvent = self.event if self.event is not None else self.group
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should need an annotation? mypy should be able to infer this union I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it gets angry if i take it away

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the error? -- does it narrow it to object (probably)? :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes


action_text = ""

if not self.issue_details or (self.recipient and self.recipient.is_team):
Expand All @@ -605,9 +574,9 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock:
action_text = get_action_text(self.actions, self.identity)
has_action = True

blocks = [self.get_title_block(rule_id, notification_uuid, obj, has_action)]
blocks = [self.get_title_block(event_or_group, has_action, rule_id, notification_uuid)]

if culprit_block := self.get_culprit_block(obj):
if culprit_block := self.get_culprit_block(event_or_group):
blocks.append(culprit_block)

# build up text block
Expand All @@ -620,7 +589,7 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock:
blocks.append(self.get_markdown_block(action_text))

# build tags block
tags = get_tags(self.group, event_for_tags, self.tags)
tags = get_tags(event_for_tags, self.tags)
if tags:
blocks.append(self.get_tags_block(tags))

Expand Down Expand Up @@ -687,7 +656,7 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock:

return self._build_blocks(
*blocks,
fallback_text=self.build_fallback_text(obj, project.slug),
block_id=orjson.dumps(block_id).decode(),
fallback_text=self.build_fallback_text(event_or_group, project.slug),
block_id=block_id,
skip_fallback=self.skip_fallback,
)
25 changes: 16 additions & 9 deletions src/sentry/integrations/slack/webhooks/options_load.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import re
from collections.abc import Mapping, Sequence
from typing import Any
from typing import Any, TypedDict

import orjson
from rest_framework import status
Expand All @@ -20,6 +20,11 @@
from ..utils import logger


class OptionGroup(TypedDict):
label: Mapping[str, str]
options: Sequence[Mapping[str, Any]]


@region_silo_endpoint
class SlackOptionsLoadEndpoint(Endpoint):
owner = ApiOwner.ECOSYSTEM
Expand Down Expand Up @@ -69,15 +74,17 @@ def get_filtered_option_groups(

option_groups = []
if filtered_teams:
team_options = format_actor_options(filtered_teams, True)
option_groups.append(
{"label": {"type": "plain_text", "text": "Teams"}, "options": team_options}
)
team_options_group: OptionGroup = {
"label": {"type": "plain_text", "text": "Teams"},
"options": format_actor_options(filtered_teams, True),
}
option_groups.append(team_options_group)
if filtered_members:
member_options = format_actor_options(filtered_members, True)
option_groups.append(
{"label": {"type": "plain_text", "text": "People"}, "options": member_options}
)
member_options_group: OptionGroup = {
"label": {"type": "plain_text", "text": "People"},
"options": format_actor_options(filtered_members, True),
}
option_groups.append(member_options_group)
return option_groups

# XXX(isabella): atm this endpoint is used only for the assignment dropdown on issue alerts
Expand Down
5 changes: 3 additions & 2 deletions src/sentry/notifications/utils/participants.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from django.db.models import Q

from sentry import features
from sentry.eventstore.models import GroupEvent
from sentry.integrations.types import ExternalProviders
from sentry.integrations.utils.providers import get_provider_enum_from_string
from sentry.models.commit import Commit
Expand Down Expand Up @@ -262,7 +263,7 @@ def get_owner_reason(
return None


def get_suspect_commit_users(project: Project, event: Event) -> list[RpcUser]:
def get_suspect_commit_users(project: Project, event: Event | GroupEvent) -> list[RpcUser]:
"""
Returns a list of users that are suspect committers for the given event.

Expand All @@ -285,7 +286,7 @@ def get_suspect_commit_users(project: Project, event: Event) -> list[RpcUser]:
return [committer for committer in suspect_committers if committer.id in in_project_user_ids]


def dedupe_suggested_assignees(suggested_assignees: Iterable[Actor]) -> Iterable[Actor]:
def dedupe_suggested_assignees(suggested_assignees: Iterable[Actor]) -> list[Actor]:
return list({assignee.id: assignee for assignee in suggested_assignees}.values())


Expand Down
2 changes: 1 addition & 1 deletion src/sentry/utils/committers.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ def get_event_file_committers(


def get_serialized_event_file_committers(
project: Project, event: Event, frame_limit: int = 25
project: Project, event: Event | GroupEvent, frame_limit: int = 25
) -> Sequence[AuthorCommitsSerialized]:

group_owners = GroupOwner.objects.filter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
old_get_tags = get_tags


def fake_get_tags(group, event_for_tags, tags):
return old_get_tags(group, event_for_tags, None)
def fake_get_tags(event_for_tags, tags):
return old_get_tags(event_for_tags, None)


class SlackIssueAlertNotificationTest(SlackActivityNotificationTest, PerformanceIssueTestCase):
Expand Down
Loading
Loading