From de88cd86d5c651548658c2176ebfd9f2c24ee193 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 11 Mar 2024 18:43:31 -0700 Subject: [PATCH 01/13] feat(rules): Show what changed in edited rule confirmation notification --- .../api/endpoints/project_rule_details.py | 57 ++++++++++++++++++- src/sentry/api/endpoints/project_rules.py | 3 +- src/sentry/api/serializers/models/rule.py | 6 +- src/sentry/conf/server.py | 2 +- .../slack/actions/notification.py | 4 +- .../notifications/rule_save_edit.py | 35 +++++++++++- src/sentry/rules/actions/base.py | 3 +- 7 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/sentry/api/endpoints/project_rule_details.py b/src/sentry/api/endpoints/project_rule_details.py index 8584be8c13ec9c..a5cf38a218aee4 100644 --- a/src/sentry/api/endpoints/project_rule_details.py +++ b/src/sentry/api/endpoints/project_rule_details.py @@ -14,7 +14,7 @@ from sentry.api.endpoints.project_rules import find_duplicate_rule, send_confirmation_notification from sentry.api.fields.actor import ActorField from sentry.api.serializers import serialize -from sentry.api.serializers.models.rule import RuleSerializer +from sentry.api.serializers.models.rule import RuleSerializer, generate_rule_label from sentry.api.serializers.rest_framework.rule import RuleNodeField from sentry.api.serializers.rest_framework.rule import RuleSerializer as DrfRuleSerializer from sentry.apidocs.constants import ( @@ -228,6 +228,11 @@ 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) + rule_env_before = None + if rule.environment_id: + rule_env_before = rule.environment_id.copy() + rule_label_before = rule.label serializer = DrfRuleSerializer( context={"project": project, "organization": project.organization}, data=request.data, @@ -367,7 +372,55 @@ 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) + # find what changed between rule_data_before and rule.data and store + changed_data = { + "new_conditions": [], + "removed_conditions": [], + "new_actions": [], + "removed_actions": [], + "new_environments": [], + "removed_environments": [], + "changed_label": "", + } + # TODO decide if I want to bucket these by type (action/condition/etc) + # if not these dict keys can just be "new", "removed", and "changed_label" + + # also TODO move this into a function + for condition in rule.data.get("conditions", []): + if condition not in rule_data_before["conditions"]: + label = generate_rule_label(rule.project, rule, condition) + changed_data["new_conditions"].append(label) + + for condition in rule_data_before["conditions"]: + if condition not in rule.data["conditions"]: + label = generate_rule_label(rule.project, rule, condition) + changed_data["removed_conditions"].append(label) + + for action in rule.data.get("actions", []): + if action not in rule_data_before["actions"]: + label = generate_rule_label(rule.project, rule, action) + changed_data["new_actions"].append(label) + + for action in rule_data_before["actions"]: + if action not in rule.data.get("actions", []): + label = generate_rule_label(rule.project, rule, action) + changed_data["removed_actions"].append(label) + + if rule.environment_id and not rule_env_before: + # convert to environment label/name + changed_data["new_environments"].append(rule.environment_id) + + if rule_env_before and not rule.environment_id: + # convert to environment label/name + changed_data["removed_environments"].append(rule_env_before) + + if rule_label_before != rule.label: + changed_data["changed_label"] = f"Rule name changed from {rule_label_before} to {rule.label}" + + # TODO add changed action interval + + + 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) diff --git a/src/sentry/api/endpoints/project_rules.py b/src/sentry/api/endpoints/project_rules.py index f1f70caf0ebf22..627000b7397acc 100644 --- a/src/sentry/api/endpoints/project_rules.py +++ b/src/sentry/api/endpoints/project_rules.py @@ -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): 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, ) diff --git a/src/sentry/api/serializers/models/rule.py b/src/sentry/api/serializers/models/rule.py index 26e0d24640670e..d764a485f2002a 100644 --- a/src/sentry/api/serializers/models/rule.py +++ b/src/sentry/api/serializers/models/rule.py @@ -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"]) @@ -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", []) ] @@ -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: diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index f25aebca1f31a1..63e8dff53b252f 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1839,7 +1839,7 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: # Enable version 2 of reprocessing (completely distinct from v1) "organizations:reprocessing-v2": False, # Enable post create/edit rule confirmation notifications - "organizations:rule-create-edit-confirm-notification": False, + "organizations:rule-create-edit-confirm-notification": True, # Enable team member role provisioning through scim "organizations:scim-team-roles": False, # Enable detecting SDK crashes during event processing diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index 7a34b4bb66017d..6317ad42b764e6 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -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): 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")), diff --git a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py index d48f905519974d..467a67919c754e 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py +++ b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py @@ -13,10 +13,12 @@ def __init__( self, rule: Rule, new: bool, + changed: dict, ) -> None: super().__init__() self.rule = rule self.new = new + self.changed = changed def linkify_rule(self): org_slug = self.rule.project.organization.slug @@ -44,9 +46,40 @@ def build(self) -> SlackBlock: else: rule_text = f"{rule_url} in the *{project}* project was updated." - # TODO add short summary of the trigger & filter conditions blocks.append(self.get_markdown_block(rule_text)) + if not self.new and self.changed: + new = [] + removed = [] + fields = [] + for label, changes in self.changed.items(): + if label.startswith("new"): + for change in changes: + new.append(change) + elif label.startswith("removed"): + for change in changes: + removed.append(change) + elif label == "changed_label": + if changes: + print("what the heck") + print(changes) + blocks.append(self.get_markdown_block(changes)) + + + if new: + new_text = f"*Added*\n" + for new_change in new: + new_text += f"•{new_change}\n" + fields.append(self.make_field(new_text)) + + if removed: + removed_text = "*Removed*\n" + for removed_change in removed: + removed_text += f"•{removed_change}\n" + fields.append(self.make_field(removed_text)) + + blocks.append(self.get_section_fields_block(fields)) + settings_link = self.get_settings_url() blocks.append(self.get_context_block(settings_link)) diff --git a/src/sentry/rules/actions/base.py b/src/sentry/rules/actions/base.py index da2ca4ea4591ba..d922918019d08b 100644 --- a/src/sentry/rules/actions/base.py +++ b/src/sentry/rules/actions/base.py @@ -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") @@ -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 """ From d8fd5b6c302579dd18cb26ae789b8c23da2d0cb8 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Mar 2024 13:34:09 -0700 Subject: [PATCH 02/13] make it look less shitty --- .../api/endpoints/project_rule_details.py | 62 +++-------------- .../notifications/rule_save_edit.py | 37 ++-------- src/sentry/rules/actions/utils.py | 68 +++++++++++++++++++ 3 files changed, 85 insertions(+), 82 deletions(-) create mode 100644 src/sentry/rules/actions/utils.py diff --git a/src/sentry/api/endpoints/project_rule_details.py b/src/sentry/api/endpoints/project_rule_details.py index a5cf38a218aee4..3a990f25d28e95 100644 --- a/src/sentry/api/endpoints/project_rule_details.py +++ b/src/sentry/api/endpoints/project_rule_details.py @@ -14,7 +14,7 @@ from sentry.api.endpoints.project_rules import find_duplicate_rule, send_confirmation_notification from sentry.api.fields.actor import ActorField from sentry.api.serializers import serialize -from sentry.api.serializers.models.rule import RuleSerializer, generate_rule_label +from sentry.api.serializers.models.rule import RuleSerializer from sentry.api.serializers.rest_framework.rule import RuleNodeField from sentry.api.serializers.rest_framework.rule import RuleSerializer as DrfRuleSerializer from sentry.apidocs.constants import ( @@ -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 from sentry.signals import alert_rule_edited from sentry.tasks.integrations.slack import find_channel_id_for_rule @@ -229,10 +230,10 @@ def put(self, request: Request, project, rule) -> Response: - Actions - specify what should happen when the trigger conditions are met and the filters match. """ rule_data_before = dict(rule.data) - rule_env_before = None if rule.environment_id: - rule_env_before = rule.environment_id.copy() - rule_label_before = rule.label + rule_data_before["environment_id"] = rule.environment_id + rule_data_before["label"] = rule.label + serializer = DrfRuleSerializer( context={"project": project, "organization": project.organization}, data=request.data, @@ -372,54 +373,11 @@ def put(self, request: Request, project, rule) -> Response: if features.has( "organizations:rule-create-edit-confirm-notification", project.organization ): - # find what changed between rule_data_before and rule.data and store - changed_data = { - "new_conditions": [], - "removed_conditions": [], - "new_actions": [], - "removed_actions": [], - "new_environments": [], - "removed_environments": [], - "changed_label": "", - } - # TODO decide if I want to bucket these by type (action/condition/etc) - # if not these dict keys can just be "new", "removed", and "changed_label" - - # also TODO move this into a function - for condition in rule.data.get("conditions", []): - if condition not in rule_data_before["conditions"]: - label = generate_rule_label(rule.project, rule, condition) - changed_data["new_conditions"].append(label) - - for condition in rule_data_before["conditions"]: - if condition not in rule.data["conditions"]: - label = generate_rule_label(rule.project, rule, condition) - changed_data["removed_conditions"].append(label) - - for action in rule.data.get("actions", []): - if action not in rule_data_before["actions"]: - label = generate_rule_label(rule.project, rule, action) - changed_data["new_actions"].append(label) - - for action in rule_data_before["actions"]: - if action not in rule.data.get("actions", []): - label = generate_rule_label(rule.project, rule, action) - changed_data["removed_actions"].append(label) - - if rule.environment_id and not rule_env_before: - # convert to environment label/name - changed_data["new_environments"].append(rule.environment_id) - - if rule_env_before and not rule.environment_id: - # convert to environment label/name - changed_data["removed_environments"].append(rule_env_before) - - if rule_label_before != rule.label: - changed_data["changed_label"] = f"Rule name changed from {rule_label_before} to {rule.label}" - - # TODO add changed action interval - - + rule_data = dict(rule.data) + if rule.environment_id: + rule_data["environment_id"] = rule.environment_id + rule_data["label"] = rule.label + changed_data = get_changed_data(rule, rule_data, rule_data_before) send_confirmation_notification(rule=rule, new=False, changed=changed_data) return Response(serialize(updated_rule, request.user)) diff --git a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py index 467a67919c754e..7f5ee3fd51eedc 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py +++ b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py @@ -42,43 +42,20 @@ 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? blocks.append(self.get_markdown_block(rule_text)) if not self.new and self.changed: - new = [] - removed = [] - fields = [] + changes_text = "*Changes*\n" for label, changes in self.changed.items(): - if label.startswith("new"): - for change in changes: - new.append(change) - elif label.startswith("removed"): - for change in changes: - removed.append(change) - elif label == "changed_label": - if changes: - print("what the heck") - print(changes) - blocks.append(self.get_markdown_block(changes)) + for change in changes: + changes_text += f"• {change}\n" - - if new: - new_text = f"*Added*\n" - for new_change in new: - new_text += f"•{new_change}\n" - fields.append(self.make_field(new_text)) - - if removed: - removed_text = "*Removed*\n" - for removed_change in removed: - removed_text += f"•{removed_change}\n" - fields.append(self.make_field(removed_text)) - - blocks.append(self.get_section_fields_block(fields)) + blocks.append(self.get_markdown_block(changes_text)) settings_link = self.get_settings_url() blocks.append(self.get_context_block(settings_link)) diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py new file mode 100644 index 00000000000000..1e398e7b86124a --- /dev/null +++ b/src/sentry/rules/actions/utils.py @@ -0,0 +1,68 @@ +from collections import defaultdict +from typing import Any + +from sentry.api.serializers.models.rule import generate_rule_label +from sentry.models.environment import Environment +from sentry.models.rule import Rule + + +def get_changed_data( + rule: Rule, rule_data: dict[str, list[Any]], rule_data_before: dict[str, list[Any]] +) -> dict[str, list[Any]]: + changed_data = defaultdict(list) + + for condition in rule_data.get("conditions", []): + if condition not in rule_data_before["conditions"]: + label = generate_rule_label(rule_data.get("project"), rule, condition) + changed_data[condition["id"]].append(f"Added condition '{label}'") + + for condition in rule_data_before["conditions"]: + if condition not in rule_data["conditions"]: + label = generate_rule_label(rule.project, rule, condition) + changed_data[condition["id"]].append(f"Removed condition '{label}'") + + for action in rule_data.get("actions", []): + if action not in rule_data_before["actions"]: + label = generate_rule_label(rule.project, rule, action) + changed_data[condition["id"]].append(f"Added action '{label}'") + + for action in rule_data_before["actions"]: + if action not in rule_data.get("actions", []): + label = generate_rule_label(rule.project, rule, action) + changed_data[condition["id"]].append(f"Added action '{label}'") + + if rule_data.get("frequency") != rule_data_before.get("frequency"): + old_frequency = rule_data_before.get("frequency") + new_frequency = rule_data.get("frequency") + changed_data["changed_frequency"].append( + f"Changed frequency from *{old_frequency}* to *{new_frequency}*" + ) + + if rule_data.get("environment_id") and not rule_data_before.get("environment_id"): + 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"): + try: + environment = Environment.objects.get(id=rule_data.get("environment_id")) + except Environment.DoesNotExist: + pass + + if environment: + changed_data["environment"].append(f"Removed *{environment.name}* environment") + + if rule_data_before.get("label") != rule_data.get("label"): + old_label = rule_data_before.get("label") + new_label = rule_data.get("label") + changed_data["changed_label"].append( + f"Changed rule name from *{old_label}* to *{new_label}*" + ) + + # TODO add actionMatch, filterMatch, and owner changes + + return changed_data From 065b2f206a0ea47ff2f0cf4711de476ac284d914 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Mar 2024 14:49:10 -0700 Subject: [PATCH 03/13] add action match, filter match, and owner changes --- .../api/endpoints/project_rule_details.py | 9 +++++++ src/sentry/rules/actions/utils.py | 25 ++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/endpoints/project_rule_details.py b/src/sentry/api/endpoints/project_rule_details.py index 3a990f25d28e95..789cc7ae63a74b 100644 --- a/src/sentry/api/endpoints/project_rule_details.py +++ b/src/sentry/api/endpoints/project_rule_details.py @@ -232,7 +232,11 @@ def put(self, request: Request, project, rule) -> Response: 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 + rule_data_before["action_match"] = rule.data.get("action_match") + rule_data_before["filter_match"] = rule.data.get("filter_match") serializer = DrfRuleSerializer( context={"project": project, "organization": project.organization}, @@ -376,7 +380,12 @@ def put(self, request: Request, project, rule) -> Response: 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 + rule_data["action_match"] = rule.data.get("action_match") + rule_data["filter_match"] = rule.data.get("filter_match") + changed_data = get_changed_data(rule, rule_data, rule_data_before) send_confirmation_notification(rule=rule, new=False, changed=changed_data) return Response(serialize(updated_rule, request.user)) diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index 1e398e7b86124a..5d1f5025890d4f 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -63,6 +63,29 @@ def get_changed_data( f"Changed rule name from *{old_label}* to *{new_label}*" ) - # TODO add actionMatch, filterMatch, and owner changes + if rule_data_before.get("action_match") != rule_data.get("action_match"): + old_action_match = rule_data_before.get("action_match") + new_action_match = rule_data.get("action_match") + action_match_text = f"Changed trigger from *{old_action_match}* to *{new_action_match}*" + changed_data["action_match"].append(action_match_text) + + if rule_data_before.get("filter_match") != rule_data.get("filter_match"): + old_action_match = rule_data_before.get("filter_match") + new_action_match = rule_data.get("filter_match") + action_match_text = f"Changed filter from *{old_action_match}* to *{new_action_match}*" + changed_data["filter_match"].append(action_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 From 67e4ede88fb0908c848abe2abbf08577c80ad987 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Mar 2024 16:27:03 -0700 Subject: [PATCH 04/13] small fixes, update test --- .../api/endpoints/project_rule_details.py | 1 - .../notifications/rule_save_edit.py | 2 +- src/sentry/rules/actions/utils.py | 17 +++++++++----- .../endpoints/test_project_rule_details.py | 23 +++++++++++++++---- 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/sentry/api/endpoints/project_rule_details.py b/src/sentry/api/endpoints/project_rule_details.py index 789cc7ae63a74b..01355c77e31098 100644 --- a/src/sentry/api/endpoints/project_rule_details.py +++ b/src/sentry/api/endpoints/project_rule_details.py @@ -385,7 +385,6 @@ def put(self, request: Request, project, rule) -> Response: rule_data["label"] = rule.label rule_data["action_match"] = rule.data.get("action_match") rule_data["filter_match"] = rule.data.get("filter_match") - changed_data = get_changed_data(rule, rule_data, rule_data_before) send_confirmation_notification(rule=rule, new=False, changed=changed_data) return Response(serialize(updated_rule, request.user)) diff --git a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py index 7f5ee3fd51eedc..6fef3136f7a899 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py +++ b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py @@ -13,7 +13,7 @@ def __init__( self, rule: Rule, new: bool, - changed: dict, + changed: dict | None = None, ) -> None: super().__init__() self.rule = rule diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index 5d1f5025890d4f..d65b417043df3d 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -9,27 +9,30 @@ def get_changed_data( rule: Rule, rule_data: dict[str, list[Any]], rule_data_before: dict[str, list[Any]] ) -> dict[str, list[Any]]: + """ + Generate a list per type of issue alert rule data of what changes occurred on edit. + """ changed_data = defaultdict(list) for condition in rule_data.get("conditions", []): - if condition not in rule_data_before["conditions"]: + if condition not in rule_data_before.get("conditions", []): label = generate_rule_label(rule_data.get("project"), rule, condition) changed_data[condition["id"]].append(f"Added condition '{label}'") - for condition in rule_data_before["conditions"]: - if condition not in rule_data["conditions"]: + for condition in rule_data_before.get("conditions", []): + if condition not in rule_data.get("conditions", []): label = generate_rule_label(rule.project, rule, condition) changed_data[condition["id"]].append(f"Removed condition '{label}'") for action in rule_data.get("actions", []): - if action not in rule_data_before["actions"]: + if action not in rule_data_before.get("actions", []): label = generate_rule_label(rule.project, rule, action) changed_data[condition["id"]].append(f"Added action '{label}'") - for action in rule_data_before["actions"]: + for action in rule_data_before.get("actions", []): if action not in rule_data.get("actions", []): label = generate_rule_label(rule.project, rule, action) - changed_data[condition["id"]].append(f"Added action '{label}'") + changed_data[condition["id"]].append(f"Removed action '{label}'") if rule_data.get("frequency") != rule_data_before.get("frequency"): old_frequency = rule_data_before.get("frequency") @@ -39,6 +42,7 @@ def get_changed_data( ) 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: @@ -48,6 +52,7 @@ def get_changed_data( 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: diff --git a/tests/sentry/api/endpoints/test_project_rule_details.py b/tests/sentry/api/endpoints/test_project_rule_details.py index da1e3eac944433..5af1f58a9e402a 100644 --- a/tests/sentry/api/endpoints/test_project_rule_details.py +++ b/tests/sentry/api/endpoints/test_project_rule_details.py @@ -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" @@ -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 @@ -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" in the *{self.project.slug}* project was updated." + message = f"Alert rule 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"] == "" ) From ffbbec94d2fcdfb60230b7a292b41a2fcbf4b6b5 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Mar 2024 16:28:23 -0700 Subject: [PATCH 05/13] turn flag off --- src/sentry/conf/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 63e8dff53b252f..f25aebca1f31a1 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1839,7 +1839,7 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]: # Enable version 2 of reprocessing (completely distinct from v1) "organizations:reprocessing-v2": False, # Enable post create/edit rule confirmation notifications - "organizations:rule-create-edit-confirm-notification": True, + "organizations:rule-create-edit-confirm-notification": False, # Enable team member role provisioning through scim "organizations:scim-team-roles": False, # Enable detecting SDK crashes during event processing From dd731e26237088bb3967464b88a2a0b7a1f3b2f6 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Mar 2024 16:55:56 -0700 Subject: [PATCH 06/13] gh r u ok From 4ae40b8675a290972be6af7676553928465c2a78 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 12:46:09 -0700 Subject: [PATCH 07/13] condense --- .../api/endpoints/project_rule_details.py | 15 +-- .../slack/actions/notification.py | 2 +- src/sentry/rules/actions/utils.py | 112 +++++++++++------- 3 files changed, 70 insertions(+), 59 deletions(-) diff --git a/src/sentry/api/endpoints/project_rule_details.py b/src/sentry/api/endpoints/project_rule_details.py index 01355c77e31098..6c37a6d8eb270e 100644 --- a/src/sentry/api/endpoints/project_rule_details.py +++ b/src/sentry/api/endpoints/project_rule_details.py @@ -42,7 +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 +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 @@ -235,8 +235,6 @@ def put(self, request: Request, project, rule) -> Response: if rule.owner: rule_data_before["owner"] = rule.owner rule_data_before["label"] = rule.label - rule_data_before["action_match"] = rule.data.get("action_match") - rule_data_before["filter_match"] = rule.data.get("filter_match") serializer = DrfRuleSerializer( context={"project": project, "organization": project.organization}, @@ -377,15 +375,8 @@ def put(self, request: Request, project, rule) -> Response: if features.has( "organizations:rule-create-edit-confirm-notification", project.organization ): - 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 - rule_data["action_match"] = rule.data.get("action_match") - rule_data["filter_match"] = rule.data.get("filter_match") - changed_data = get_changed_data(rule, rule_data, rule_data_before) + 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)) diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index 6317ad42b764e6..1838241be4d786 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -139,7 +139,7 @@ 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, changed): + 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. diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index d65b417043df3d..fed3e1f053f3d2 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -1,45 +1,72 @@ from collections import defaultdict -from typing import Any +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 "" + + +def check_added_or_removed( + rule_data_after: dict[str, Any], + rule_data_before: dict[str, Any], + rule: Rule, + changed_data: DefaultDict[str, list[str]], + key: str, + rule_section_type: str, + added: bool, +) -> 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, list[Any]], rule_data_before: dict[str, list[Any]] -) -> dict[str, list[Any]]: + 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) - - for condition in rule_data.get("conditions", []): - if condition not in rule_data_before.get("conditions", []): - label = generate_rule_label(rule_data.get("project"), rule, condition) - changed_data[condition["id"]].append(f"Added condition '{label}'") - - for condition in rule_data_before.get("conditions", []): - if condition not in rule_data.get("conditions", []): - label = generate_rule_label(rule.project, rule, condition) - changed_data[condition["id"]].append(f"Removed condition '{label}'") - - for action in rule_data.get("actions", []): - if action not in rule_data_before.get("actions", []): - label = generate_rule_label(rule.project, rule, action) - changed_data[condition["id"]].append(f"Added action '{label}'") - - for action in rule_data_before.get("actions", []): - if action not in rule_data.get("actions", []): - label = generate_rule_label(rule.project, rule, action) - changed_data[condition["id"]].append(f"Removed action '{label}'") - - if rule_data.get("frequency") != rule_data_before.get("frequency"): - old_frequency = rule_data_before.get("frequency") - new_frequency = rule_data.get("frequency") - changed_data["changed_frequency"].append( - f"Changed frequency from *{old_frequency}* to *{new_frequency}*" - ) + 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 @@ -61,24 +88,17 @@ def get_changed_data( if environment: changed_data["environment"].append(f"Removed *{environment.name}* environment") - if rule_data_before.get("label") != rule_data.get("label"): - old_label = rule_data_before.get("label") - new_label = rule_data.get("label") - changed_data["changed_label"].append( - f"Changed rule name from *{old_label}* to *{new_label}*" - ) - - if rule_data_before.get("action_match") != rule_data.get("action_match"): - old_action_match = rule_data_before.get("action_match") - new_action_match = rule_data.get("action_match") - action_match_text = f"Changed trigger from *{old_action_match}* to *{new_action_match}*" + 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) - if rule_data_before.get("filter_match") != rule_data.get("filter_match"): - old_action_match = rule_data_before.get("filter_match") - new_action_match = rule_data.get("filter_match") - action_match_text = f"Changed filter from *{old_action_match}* to *{new_action_match}*" - changed_data["filter_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") From eba8d41376f964bc54a13962ac3158844ec30c99 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Wed, 13 Mar 2024 19:47:42 +0000 Subject: [PATCH 08/13] :hammer_and_wrench: apply pre-commit fixes --- src/sentry/rules/actions/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/rules/actions/base.py b/src/sentry/rules/actions/base.py index d922918019d08b..a65cd7f6aa7805 100644 --- a/src/sentry/rules/actions/base.py +++ b/src/sentry/rules/actions/base.py @@ -3,11 +3,11 @@ 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 from sentry.rules.base import CallbackFuture, EventState, RuleBase -from typing import Any logger = logging.getLogger("sentry.rules") From 42173854030e3598a9d68c5a9773ba78580e8ef7 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 12:56:14 -0700 Subject: [PATCH 09/13] make changes optional --- src/sentry/integrations/slack/actions/notification.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index 1838241be4d786..8b1c46da027ab6 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -139,7 +139,9 @@ 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, changed: dict[str, Any]): + 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. From 52feefd6c291277d5583d8ecbd9ef6d25ea8882b Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 13:06:50 -0700 Subject: [PATCH 10/13] typing --- src/sentry/api/endpoints/project_rules.py | 2 +- src/sentry/rules/actions/utils.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/api/endpoints/project_rules.py b/src/sentry/api/endpoints/project_rules.py index 627000b7397acc..a556ab6128b295 100644 --- a/src/sentry/api/endpoints/project_rules.py +++ b/src/sentry/api/endpoints/project_rules.py @@ -38,7 +38,7 @@ from sentry.utils.safe import safe_execute -def send_confirmation_notification(rule: Rule, new: bool, changed): +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( diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index fed3e1f053f3d2..4177de262bfaba 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -50,7 +50,7 @@ def get_changed_data( """ Generate a list per type of issue alert rule data of what changes occurred on edit. """ - changed_data = defaultdict(list) + changed_data: DefaultDict[str, list[str]] = defaultdict(list) changed_data = check_added_or_removed( rule_data_before, rule_data, rule, changed_data, "conditions", "condition", added=True ) From 50fea34704950c1d6e91164fcb8bea10324b9736 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 13:41:28 -0700 Subject: [PATCH 11/13] update test --- tests/sentry/api/endpoints/test_project_rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/api/endpoints/test_project_rules.py b/tests/sentry/api/endpoints/test_project_rules.py index ef975ca1d5fbf4..30f2dbec06da31 100644 --- a/tests/sentry/api/endpoints/test_project_rules.py +++ b/tests/sentry/api/endpoints/test_project_rules.py @@ -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" was created in the *{self.project.slug}* project and will send notifications here." + message = f"Alert rule 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 From 68d71e62c4ebc19fe86d659962c3a2fc11faf1d2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 14:11:37 -0700 Subject: [PATCH 12/13] pr suggestions --- .../notifications/rule_save_edit.py | 2 +- src/sentry/rules/actions/utils.py | 43 +++++++++---------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py index 6fef3136f7a899..fccdfc681ffe10 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py +++ b/src/sentry/integrations/slack/message_builder/notifications/rule_save_edit.py @@ -51,7 +51,7 @@ def build(self) -> SlackBlock: if not self.new and self.changed: changes_text = "*Changes*\n" - for label, changes in self.changed.items(): + for changes in self.changed.values(): for change in changes: changes_text += f"• {change}\n" diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index 4177de262bfaba..f4eafe8cf80861 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -17,29 +17,26 @@ def get_updated_rule_data(rule: Rule) -> dict[str, Any]: 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) + 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 "" -def check_added_or_removed( - rule_data_after: dict[str, Any], - rule_data_before: dict[str, Any], +def generate_diff_labels( + present_state: dict[str, Any], + prior_state: dict[str, Any], rule: Rule, changed_data: DefaultDict[str, list[str]], key: str, - rule_section_type: str, - added: bool, + statement: str, ) -> 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, []): + 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(f"{verb} {rule_section_type} '{label}'") + changed_data[data["id"]].append(statement.format(label)) return changed_data @@ -51,17 +48,17 @@ def get_changed_data( 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 = check_added_or_removed( - rule_data_before, rule_data, rule, changed_data, "conditions", "condition", added=True + changed_data = generate_diff_labels( + rule_data_before, rule_data, rule, changed_data, "conditions", "Added condition '{}'" ) - changed_data = check_added_or_removed( - rule_data, rule_data_before, rule, changed_data, "conditions", "condition", added=False + changed_data = generate_diff_labels( + rule_data, rule_data_before, rule, changed_data, "conditions", "Removed condition '{}'" ) - changed_data = check_added_or_removed( - rule_data_before, rule_data, rule, changed_data, "actions", "action", added=True + changed_data = generate_diff_labels( + rule_data_before, rule_data, rule, changed_data, "actions", "Added action '{}'" ) - changed_data = check_added_or_removed( - rule_data, rule_data_before, rule, changed_data, "actions", "action", added=False + 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") From f7b5d7568516539be8bd468ecf6901e7163456e3 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 13 Mar 2024 14:16:42 -0700 Subject: [PATCH 13/13] return none --- src/sentry/rules/actions/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/rules/actions/utils.py b/src/sentry/rules/actions/utils.py index f4eafe8cf80861..3013516343e4a8 100644 --- a/src/sentry/rules/actions/utils.py +++ b/src/sentry/rules/actions/utils.py @@ -23,6 +23,7 @@ def check_value_changed( old_value = prior_state.get(key) new_value = present_state.get(key) return f"Changed {word} from *{old_value}* to *{new_value}*" + return None def generate_diff_labels(