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

ref(rules): Add changes to rule edit confirmation notification #66847

Merged
merged 13 commits into from
Mar 14, 2024
12 changes: 11 additions & 1 deletion src/sentry/api/endpoints/project_rule_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
from sentry.models.team import Team
from sentry.models.user import User
from sentry.rules.actions import trigger_sentry_app_action_creators_for_issues
from sentry.rules.actions.utils import get_changed_data, get_updated_rule_data
from sentry.signals import alert_rule_edited
from sentry.tasks.integrations.slack import find_channel_id_for_rule

Expand Down Expand Up @@ -228,6 +229,13 @@ def put(self, request: Request, project, rule) -> Response:
- Filters - help control noise by triggering an alert only if the issue matches the specified criteria.
- Actions - specify what should happen when the trigger conditions are met and the filters match.
"""
rule_data_before = dict(rule.data)
if rule.environment_id:
rule_data_before["environment_id"] = rule.environment_id
if rule.owner:
rule_data_before["owner"] = rule.owner
rule_data_before["label"] = rule.label

serializer = DrfRuleSerializer(
context={"project": project, "organization": project.organization},
data=request.data,
Expand Down Expand Up @@ -367,7 +375,9 @@ def put(self, request: Request, project, rule) -> Response:
if features.has(
"organizations:rule-create-edit-confirm-notification", project.organization
):
send_confirmation_notification(rule=rule, new=False)
new_rule_data = get_updated_rule_data(rule)
changed_data = get_changed_data(rule, new_rule_data, rule_data_before)
send_confirmation_notification(rule=rule, new=False, changed=changed_data)
return Response(serialize(updated_rule, request.user))

return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
Expand Down
3 changes: 2 additions & 1 deletion src/sentry/api/endpoints/project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,14 @@
from sentry.utils.safe import safe_execute


def send_confirmation_notification(rule: Rule, new: bool):
def send_confirmation_notification(rule: Rule, new: bool, changed: dict | None = None):
for action in rule.data.get("actions", ()):
action_inst = instantiate_action(rule, action)
safe_execute(
action_inst.send_confirmation_notification,
rule=rule,
new=new,
changed=changed,
)


Expand Down
6 changes: 3 additions & 3 deletions src/sentry/api/serializers/models/rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from sentry.services.hybrid_cloud.user.service import user_service


def _generate_rule_label(project, rule, data):
def generate_rule_label(project, rule, data):
from sentry.rules import rules

rule_cls = rules.get(data["id"])
Expand Down Expand Up @@ -202,7 +202,7 @@ def get_attrs(self, item_list, user, **kwargs):
def serialize(self, obj, attrs, user, **kwargs) -> RuleSerializerResponse:
environment = attrs["environment"]
all_conditions = [
dict(list(o.items()) + [("name", _generate_rule_label(obj.project, obj, o))])
dict(list(o.items()) + [("name", generate_rule_label(obj.project, obj, o))])
for o in obj.data.get("conditions", [])
]

Expand All @@ -212,7 +212,7 @@ def serialize(self, obj, attrs, user, **kwargs) -> RuleSerializerResponse:
actions.append(
dict(
list(action.items())
+ [("name", _generate_rule_label(obj.project, obj, action))]
+ [("name", generate_rule_label(obj.project, obj, action))]
)
)
except serializers.ValidationError:
Expand Down
6 changes: 4 additions & 2 deletions src/sentry/integrations/slack/actions/notification.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,16 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None:
metrics.incr("notifications.sent", instance="slack.notification", skip_internal=False)
yield self.future(send_notification, key=key)

def send_confirmation_notification(self, rule: Rule, new: bool):
def send_confirmation_notification(
self, rule: Rule, new: bool, changed: dict[str, Any] | None = None
):
integration = self.get_integration()
if not integration:
# Integration removed, rule still active.
return

channel = self.get_option("channel_id")
blocks = SlackRuleSaveEditMessageBuilder(rule=rule, new=new).build()
blocks = SlackRuleSaveEditMessageBuilder(rule=rule, new=new, changed=changed).build()
payload = {
"text": blocks.get("text"),
"blocks": json.dumps(blocks.get("blocks")),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ def __init__(
self,
rule: Rule,
new: bool,
changed: dict | None = None,
) -> None:
super().__init__()
self.rule = rule
self.new = new
self.changed = changed

def linkify_rule(self):
org_slug = self.rule.project.organization.slug
Expand All @@ -40,13 +42,21 @@ def build(self) -> SlackBlock:
rule_url = self.linkify_rule()
project = self.rule.project.slug
if self.new:
rule_text = f"{rule_url} was created in the *{project}* project and will send notifications here."
rule_text = f"Alert rule {rule_url} was created in the *{project}* project and will send notifications here."
else:
rule_text = f"{rule_url} in the *{project}* project was updated."
rule_text = f"Alert rule {rule_url} in the *{project}* project was updated."
# TODO potentially use old name if it's changed?

# TODO add short summary of the trigger & filter conditions
blocks.append(self.get_markdown_block(rule_text))

if not self.new and self.changed:
changes_text = "*Changes*\n"
for changes in self.changed.values():
for change in changes:
changes_text += f"• {change}\n"

blocks.append(self.get_markdown_block(changes_text))

settings_link = self.get_settings_url()
blocks.append(self.get_context_block(settings_link))

Expand Down
3 changes: 2 additions & 1 deletion src/sentry/rules/actions/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import abc
import logging
from collections.abc import Generator
from typing import Any

from sentry.eventstore.models import GroupEvent
from sentry.models.rule import Rule
Expand Down Expand Up @@ -54,7 +55,7 @@ def after(
>>> print(future)
"""

def send_confirmation_notification(self, rule: Rule, new: bool):
def send_confirmation_notification(self, rule: Rule, new: bool, changed: dict[str, Any]):
"""
Send a notification confirming that a rule was created or edited
"""
Expand Down
114 changes: 114 additions & 0 deletions src/sentry/rules/actions/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
from collections import defaultdict
from typing import Any, DefaultDict

from sentry.api.serializers.models.rule import generate_rule_label
from sentry.models.environment import Environment
from sentry.models.rule import Rule


def get_updated_rule_data(rule: Rule) -> dict[str, Any]:
rule_data = dict(rule.data)
if rule.environment_id:
rule_data["environment_id"] = rule.environment_id
if rule.owner:
rule_data["owner"] = rule.owner
rule_data["label"] = rule.label
return rule_data


def check_value_changed(
present_state: dict[str, Any], prior_state: dict[str, Any], key: str, word: str
) -> str | None:
if present_state.get(key) != prior_state.get(key):
old_value = prior_state.get(key)
new_value = present_state.get(key)
return f"Changed {word} from *{old_value}* to *{new_value}*"
return None
Copy link
Member

Choose a reason for hiding this comment

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

small nit: can skip returning None

Copy link
Member Author

Choose a reason for hiding this comment

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

mypy wouldn't let me 😭

Copy link
Member

@schew2381 schew2381 Mar 13, 2024

Choose a reason for hiding this comment

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

apparently by design: python/mypy#7511 (comment)

There's a flag --no-warn-no-return to disable this but looks like we don't have it on



def generate_diff_labels(
present_state: dict[str, Any],
prior_state: dict[str, Any],
rule: Rule,
changed_data: DefaultDict[str, list[str]],
key: str,
statement: str,
) -> DefaultDict[str, list[str]]:
for data in prior_state.get(key, []):
if data not in present_state.get(key, []):
label = generate_rule_label(rule.project, rule, data)
changed_data[data["id"]].append(statement.format(label))

return changed_data


def get_changed_data(
rule: Rule, rule_data: dict[str, Any], rule_data_before: dict[str, Any]
) -> dict[str, Any]:
"""
Generate a list per type of issue alert rule data of what changes occurred on edit.
"""
changed_data: DefaultDict[str, list[str]] = defaultdict(list)
changed_data = generate_diff_labels(
rule_data_before, rule_data, rule, changed_data, "conditions", "Added condition '{}'"
)
changed_data = generate_diff_labels(
rule_data, rule_data_before, rule, changed_data, "conditions", "Removed condition '{}'"
)
changed_data = generate_diff_labels(
rule_data_before, rule_data, rule, changed_data, "actions", "Added action '{}'"
)
changed_data = generate_diff_labels(
rule_data, rule_data_before, rule, changed_data, "actions", "Removed action '{}'"
)

frequency_text = check_value_changed(rule_data, rule_data_before, "frequency", "frequency")
if frequency_text:
changed_data["changed_frequency"].append(frequency_text)

if rule_data.get("environment_id") and not rule_data_before.get("environment_id"):
environment = None
try:
environment = Environment.objects.get(id=rule_data.get("environment_id"))
except Environment.DoesNotExist:
pass

if environment:
changed_data["environment"].append(f"Added *{environment.name}* environment")

if rule_data_before.get("environment_id") and not rule_data.get("environment_id"):
environment = None
try:
environment = Environment.objects.get(id=rule_data.get("environment_id"))
except Environment.DoesNotExist:
pass
Comment on lines +79 to +84
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this is a typo -- the type checker is pointing this out as a bug when I enable model checking

this is always passing id=None here -- should this be using rule_data_before["environment_id"] instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix here - #73665


if environment:
changed_data["environment"].append(f"Removed *{environment.name}* environment")

label_text = check_value_changed(rule_data, rule_data_before, "label", "rule name")
if label_text:
changed_data["changed_label"].append(label_text)

action_match_text = check_value_changed(rule_data, rule_data_before, "action_match", "trigger")
if action_match_text:
changed_data["action_match"].append(action_match_text)

filter_match_text = check_value_changed(rule_data, rule_data_before, "filter_match", "filter")
if filter_match_text:
changed_data["filter_match"].append(filter_match_text)

if rule_data_before.get("owner") != rule_data.get("owner"):
old_owner = rule_data_before.get("owner")
old_actor = "Unassigned"
if old_owner:
old_actor = old_owner.resolve()

new_owner = rule_data.get("owner")
new_actor = "Unassigned"
if new_owner:
new_actor = new_owner.resolve()
owner_changed_text = f"Changed owner from *{old_actor}* to *{new_actor}*"
changed_data["owner"].append(owner_changed_text)

return changed_data
23 changes: 19 additions & 4 deletions tests/sentry/api/endpoints/test_project_rule_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ def test_slack_confirmation_notification_contents(self):
"channel": "#old_channel_name",
}
]
self.rule.update(data={"conditions": conditions, "actions": actions})
self.rule.update(data={"conditions": conditions, "actions": actions}, label="my rule")

actions[0]["channel"] = "#new_channel_name"
actions[0]["channel_id"] = "new_channel_id"
Expand Down Expand Up @@ -1054,13 +1054,18 @@ def test_slack_confirmation_notification_contents(self):
content_type="application/json",
body=json.dumps(payload),
)
staging_env = self.create_environment(
self.project, name="staging", organization=self.organization
)
payload = {
"name": "#new_channel_name",
"name": "new rule",
"actionMatch": "any",
"filterMatch": "any",
"actions": actions,
"conditions": conditions,
"frequency": 30,
"environment": staging_env.name,
"owner": get_actor_for_user(self.user).get_actor_identifier(),
}
response = self.get_success_response(
self.organization.slug, self.project.slug, self.rule.id, status_code=200, **payload
Expand All @@ -1069,12 +1074,22 @@ def test_slack_confirmation_notification_contents(self):
rule_label = response.data["name"]
assert response.data["actions"][0]["channel_id"] == "new_channel_id"
data = parse_qs(responses.calls[1].request.body)
message = f"<http://testserver/organizations/{self.organization.slug}/alerts/rules/{self.project.slug}/{rule_id}/details/|*{rule_label}*> in the *{self.project.slug}* project was updated."
message = f"Alert rule <http://testserver/organizations/{self.organization.slug}/alerts/rules/{self.project.slug}/{rule_id}/details/|*{rule_label}*> in the *{self.project.slug}* project was updated."
assert data["text"][0] == message
rendered_blocks = json.loads(data["blocks"][0])
assert rendered_blocks[0]["text"]["text"] == message
changes = "*Changes*\n"
changes += "• Added action 'Send a notification to the Awesome Team Slack workspace to new_channel_name (optionally, an ID: new_channel_id) and show tags [] in notification'\n"
changes += "• Removed action 'Send a notification to the Awesome Team Slack workspace to #old_channel_name (optionally, an ID: old_channel_id) and show tags [] in notification'\n"
changes += "• Changed frequency from *None* to *30*\n"
changes += f"• Added *{staging_env.name}* environment\n"
changes += "• Changed rule name from *my rule* to *new rule*\n"
changes += "• Changed trigger from *None* to *any*\n"
changes += "• Changed filter from *None* to *any*\n"
changes += f"• Changed owner from *Unassigned* to *{self.user.email}*\n"
assert rendered_blocks[1]["text"]["text"] == changes
assert (
rendered_blocks[1]["elements"][0]["text"]
rendered_blocks[2]["elements"][0]["text"]
== "<http://testserver/settings/account/notifications/alerts/|*Notification Settings*>"
)

Expand Down
2 changes: 1 addition & 1 deletion tests/sentry/api/endpoints/test_project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ def test_slack_confirmation_notification_contents(self):
rule_label = response.data["name"]
assert response.data["actions"][0]["channel_id"] == self.channel_id
data = parse_qs(responses.calls[1].request.body)
message = f"<http://testserver/organizations/{self.organization.slug}/alerts/rules/{self.project.slug}/{rule_id}/details/|*{rule_label}*> was created in the *{self.project.slug}* project and will send notifications here."
message = f"Alert rule <http://testserver/organizations/{self.organization.slug}/alerts/rules/{self.project.slug}/{rule_id}/details/|*{rule_label}*> was created in the *{self.project.slug}* project and will send notifications here."
assert data["text"][0] == message
rendered_blocks = json.loads(data["blocks"][0])
assert rendered_blocks[0]["text"]["text"] == message
Expand Down
Loading