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):
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
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
4 changes: 2 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,14 @@ 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]):
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 label, changes in self.changed.items():
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -7,6 +7,7 @@
from sentry.eventstore.models import GroupEvent
from sentry.models.rule import Rule
from sentry.rules.base import CallbackFuture, EventState, RuleBase
from typing import Any

logger = logging.getLogger("sentry.rules")

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
116 changes: 116 additions & 0 deletions src/sentry/rules/actions/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
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(
rule_data: dict[str, Any], rule_data_before: dict[str, Any], key: str, word: str
) -> str:
if rule_data.get(key) != rule_data_before.get(key):
old_value = rule_data_before.get(key)
new_value = rule_data.get(key)
return f"Changed {word} from *{old_value}* to *{new_value}*"
return ""
ceorourke marked this conversation as resolved.
Show resolved Hide resolved


def check_added_or_removed(
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
rule_data_after: dict[str, Any],
rule_data_before: dict[str, Any],
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
rule: Rule,
changed_data: DefaultDict[str, list[str]],
key: str,
rule_section_type: str,
added: bool,
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
) -> DefaultDict[str, list[str]]:
verb = "Added" if added else "Removed"
for data in rule_data_before.get(key, []):
if data not in rule_data_after.get(key, []):
label = generate_rule_label(rule.project, rule, data)
changed_data[data["id"]].append(f"{verb} {rule_section_type} '{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(list)
changed_data = check_added_or_removed(
rule_data_before, rule_data, rule, changed_data, "conditions", "condition", added=True
)
changed_data = check_added_or_removed(
rule_data, rule_data_before, rule, changed_data, "conditions", "condition", added=False
)
changed_data = check_added_or_removed(
rule_data_before, rule_data, rule, changed_data, "actions", "action", added=True
)
changed_data = check_added_or_removed(
rule_data, rule_data_before, rule, changed_data, "actions", "action", added=False
)

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
Loading