From b354a7e48dc9bd01c7520c10106c0655e7e5e338 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 27 Nov 2024 12:15:12 -0800 Subject: [PATCH 1/3] chore(alerts): Remove incident activity comments --- .../organization_incident_details.py | 3 - src/sentry/incidents/events.py | 9 -- src/sentry/incidents/logic.py | 31 ---- src/sentry/incidents/tasks.py | 102 ------------- src/sentry/testutils/fixtures.py | 6 - src/sentry/web/debug_urls.py | 2 - .../debug/debug_incident_activity_email.py | 28 ---- .../api/serializers/test_incident_activity.py | 37 +---- tests/sentry/incidents/test_logic.py | 93 +----------- tests/sentry/incidents/test_tasks.py | 136 +----------------- 10 files changed, 5 insertions(+), 442 deletions(-) delete mode 100644 src/sentry/web/frontend/debug/debug_incident_activity_email.py diff --git a/src/sentry/incidents/endpoints/organization_incident_details.py b/src/sentry/incidents/endpoints/organization_incident_details.py index 7edc9a96a8bd5c..20f217883b7d50 100644 --- a/src/sentry/incidents/endpoints/organization_incident_details.py +++ b/src/sentry/incidents/endpoints/organization_incident_details.py @@ -10,7 +10,6 @@ from sentry.incidents.endpoints.serializers.incident import DetailedIncidentSerializer from sentry.incidents.logic import update_incident_status from sentry.incidents.models.incident import IncidentStatus, IncidentStatusMethod -from sentry.users.services.user.serial import serialize_generic_user class IncidentSerializer(serializers.Serializer): @@ -56,8 +55,6 @@ def put(self, request: Request, organization, incident) -> Response: incident = update_incident_status( incident=incident, status=result["status"], - user=serialize_generic_user(request.user), - comment=result.get("comment"), status_method=IncidentStatusMethod.MANUAL, ) return Response( diff --git a/src/sentry/incidents/events.py b/src/sentry/incidents/events.py index 0c12286a8a7375..03a00e89049097 100644 --- a/src/sentry/incidents/events.py +++ b/src/sentry/incidents/events.py @@ -21,14 +21,5 @@ class IncidentStatusUpdatedEvent(BaseIncidentEvent): ) -class IncidentCommentCreatedEvent(BaseIncidentEvent): - type = "incident.comment" - attributes = BaseIncidentEvent.attributes + ( - analytics.Attribute("user_id", required=False), - analytics.Attribute("activity_id", required=False), - ) - - analytics.register(IncidentCreatedEvent) analytics.register(IncidentStatusUpdatedEvent) -analytics.register(IncidentCommentCreatedEvent) diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py index 2f75ab8add4654..5b68e8bcb2224d 100644 --- a/src/sentry/incidents/logic.py +++ b/src/sentry/incidents/logic.py @@ -199,8 +199,6 @@ def create_incident( def update_incident_status( incident: Incident, status: IncidentStatus, - user: RpcUser | None = None, - comment: str | None = None, status_method: IncidentStatusMethod = IncidentStatusMethod.RULE_TRIGGERED, date_closed: datetime | None = None, ) -> Incident: @@ -216,13 +214,9 @@ def update_incident_status( create_incident_activity( incident, IncidentActivityType.STATUS_CHANGE, - user=user, value=status.value, previous_value=incident.status, - comment=comment, ) - if user: - subscribe_to_incident(incident, user.id) prev_status = incident.status kwargs: dict[str, Any] = {"status": status.value, "status_method": status_method.value} @@ -260,12 +254,9 @@ def create_incident_activity( user: RpcUser | User | None = None, value: str | int | None = None, previous_value: str | int | None = None, - comment: str | None = None, mentioned_user_ids: Collection[int] = (), date_added: datetime | None = None, ) -> IncidentActivity: - if activity_type == IncidentActivityType.COMMENT and user: - subscribe_to_incident(incident, user.id) value = str(value) if value is not None else None previous_value = str(previous_value) if previous_value is not None else None kwargs = {} @@ -277,7 +268,6 @@ def create_incident_activity( user_id=user.id if user else None, value=value, previous_value=previous_value, - comment=comment, notification_uuid=uuid4(), **kwargs, ) @@ -295,22 +285,6 @@ def create_incident_activity( for mentioned_user_id in user_ids_to_subscribe ] ) - transaction.on_commit( - lambda: tasks.send_subscriber_notifications.apply_async( - kwargs={"activity_id": activity.id}, countdown=10 - ), - router.db_for_write(IncidentSubscription), - ) - if activity_type == IncidentActivityType.COMMENT: - analytics.record( - "incident.comment", - incident_id=incident.id, - organization_id=incident.organization_id, - incident_type=incident.type, - user_id=user.id if user else None, - activity_id=activity.id, - ) - return activity @@ -481,11 +455,6 @@ def get_incident_aggregates( return aggregated_result[0] -def subscribe_to_incident(incident: Incident, user_id: int) -> IncidentSubscription: - subscription, _ = IncidentSubscription.objects.get_or_create(incident=incident, user_id=user_id) - return subscription - - def unsubscribe_from_incident(incident: Incident, user_id: int) -> None: IncidentSubscription.objects.filter(incident=incident, user_id=user_id).delete() diff --git a/src/sentry/incidents/tasks.py b/src/sentry/incidents/tasks.py index 57c8876abe7a03..d6935626ac3cb1 100644 --- a/src/sentry/incidents/tasks.py +++ b/src/sentry/incidents/tasks.py @@ -2,21 +2,15 @@ import logging from typing import Any -from urllib.parse import urlencode -from django.urls import reverse - -from sentry.auth.access import from_user from sentry.incidents.models.alert_rule import ( AlertRuleStatus, AlertRuleTriggerAction, AlertRuleTriggerActionMethod, ) from sentry.incidents.models.incident import ( - INCIDENT_STATUS, Incident, IncidentActivity, - IncidentActivityType, IncidentStatus, IncidentStatusMethod, ) @@ -31,107 +25,11 @@ from sentry.snuba.models import QuerySubscription from sentry.snuba.query_subscriptions.consumer import register_subscriber from sentry.tasks.base import instrumented_task -from sentry.users.models.user import User -from sentry.users.services.user import RpcUser -from sentry.users.services.user.service import user_service from sentry.utils import metrics -from sentry.utils.email import MessageBuilder -from sentry.utils.http import absolute_uri logger = logging.getLogger(__name__) -@instrumented_task( - name="sentry.incidents.tasks.send_subscriber_notifications", - queue="incidents", - silo_mode=SiloMode.REGION, -) -def send_subscriber_notifications(activity_id: int) -> None: - from sentry.incidents.logic import get_incident_subscribers, unsubscribe_from_incident - - try: - activity = IncidentActivity.objects.select_related( - "incident", "incident__organization" - ).get(id=activity_id) - except IncidentActivity.DoesNotExist: - return - - if activity.user_id is None: - return - - activity_user = user_service.get_user(user_id=activity.user_id) - - # Only send notifications for specific activity types. - if activity.type not in ( - IncidentActivityType.COMMENT.value, - IncidentActivityType.STATUS_CHANGE.value, - ): - return - - # Check that the user still has access to at least one of the projects - # related to the incident. If not then unsubscribe them. - projects = list(activity.incident.projects.all()) - for subscriber in get_incident_subscribers(activity.incident): - subscriber_user = user_service.get_user(user_id=subscriber.user_id) - if subscriber_user is None: - continue - - access = from_user(subscriber_user, activity.incident.organization) - if not any(project for project in projects if access.has_project_access(project)): - unsubscribe_from_incident(activity.incident, subscriber_user.id) - elif subscriber_user.id != activity.user_id: - msg = generate_incident_activity_email(activity, subscriber_user, activity_user) - msg.send_async([subscriber_user.email]) - - -def generate_incident_activity_email( - activity: IncidentActivity, user: RpcUser | User, activity_user: RpcUser | User | None = None -) -> MessageBuilder: - incident = activity.incident - return MessageBuilder( - subject=f"Activity on Alert {incident.title} (#{incident.identifier})", - template="sentry/emails/incidents/activity.txt", - html_template="sentry/emails/incidents/activity.html", - type="incident.activity", - context=build_activity_context(activity, user, activity_user), - ) - - -def build_activity_context( - activity: IncidentActivity, user: RpcUser | User, activity_user: RpcUser | None = None -) -> dict[str, Any]: - if activity_user is None: - activity_user = user_service.get_user(user_id=activity.user_id) - - if activity.type == IncidentActivityType.COMMENT.value: - action = "left a comment" - else: - action = "changed status from {} to {}".format( - INCIDENT_STATUS[IncidentStatus(int(activity.previous_value))], - INCIDENT_STATUS[IncidentStatus(int(activity.value))], - ) - incident = activity.incident - - action = f"{action} on alert {incident.title} (#{incident.identifier})" - - return { - "user_name": activity_user.name if activity_user else "Sentry", - "action": action, - "link": absolute_uri( - reverse( - "sentry-metric-alert", - kwargs={ - "organization_slug": incident.organization.slug, - "incident_id": incident.identifier, - }, - ) - ) - + "?" - + urlencode({"referrer": "incident_activity_email"}), - "comment": activity.comment, - } - - @register_subscriber(SUBSCRIPTION_METRICS_LOGGER) def handle_subscription_metrics_logger( subscription_update: QuerySubscriptionUpdate, subscription: QuerySubscription diff --git a/src/sentry/testutils/fixtures.py b/src/sentry/testutils/fixtures.py index 26413e962a48c4..c66670255091ec 100644 --- a/src/sentry/testutils/fixtures.py +++ b/src/sentry/testutils/fixtures.py @@ -10,7 +10,6 @@ from sentry.eventstore.models import Event from sentry.incidents.models.alert_rule import AlertRule, AlertRuleMonitorTypeInt -from sentry.incidents.models.incident import IncidentActivityType from sentry.integrations.models.integration import Integration from sentry.integrations.models.organization_integration import OrganizationIntegration from sentry.models.activity import Activity @@ -374,11 +373,6 @@ def create_incident(self, organization=None, projects=None, *args, **kwargs): def create_incident_activity(self, *args, **kwargs): return Factories.create_incident_activity(*args, **kwargs) - def create_incident_comment(self, incident, *args, **kwargs): - return self.create_incident_activity( - incident, type=IncidentActivityType.COMMENT.value, *args, **kwargs - ) - def create_incident_trigger(self, incident, alert_rule_trigger, status): return Factories.create_incident_trigger(incident, alert_rule_trigger, status=status) diff --git a/src/sentry/web/debug_urls.py b/src/sentry/web/debug_urls.py index 200e13f9954eac..5afc8bcdb669a3 100644 --- a/src/sentry/web/debug_urls.py +++ b/src/sentry/web/debug_urls.py @@ -21,7 +21,6 @@ from sentry.web.frontend.debug.debug_error_embed import DebugErrorPageEmbedView from sentry.web.frontend.debug.debug_feedback_issue import DebugFeedbackIssueEmailView from sentry.web.frontend.debug.debug_generic_issue import DebugGenericIssueEmailView -from sentry.web.frontend.debug.debug_incident_activity_email import DebugIncidentActivityEmailView from sentry.web.frontend.debug.debug_incident_trigger_email import DebugIncidentTriggerEmailView from sentry.web.frontend.debug.debug_incident_trigger_email_activated_alert import ( DebugIncidentActivatedAlertTriggerEmailView, @@ -148,7 +147,6 @@ re_path( r"^debug/mail/sso-unlinked/no-password/$", DebugSsoUnlinkedNoPasswordEmailView.as_view() ), - re_path(r"^debug/mail/incident-activity/$", DebugIncidentActivityEmailView.as_view()), re_path(r"^debug/mail/incident-trigger/$", DebugIncidentTriggerEmailView.as_view()), re_path( r"^debug/mail/activated-incident-trigger/$", diff --git a/src/sentry/web/frontend/debug/debug_incident_activity_email.py b/src/sentry/web/frontend/debug/debug_incident_activity_email.py deleted file mode 100644 index 926376b9378c75..00000000000000 --- a/src/sentry/web/frontend/debug/debug_incident_activity_email.py +++ /dev/null @@ -1,28 +0,0 @@ -from django.http import HttpRequest, HttpResponse -from django.views.generic import View - -from sentry.incidents.models.incident import Incident, IncidentActivity, IncidentActivityType -from sentry.incidents.tasks import generate_incident_activity_email -from sentry.models.organization import Organization -from sentry.users.models.user import User - -from .mail import MailPreview - - -class DebugIncidentActivityEmailView(View): - def get(self, request: HttpRequest) -> HttpResponse: - organization = Organization(slug="myorg") - user = User(id=1235, name="Hello There") - incident = Incident( - id=2, identifier=123, organization=organization, title="Something broke" - ) - activity = IncidentActivity( - incident=incident, - user_id=user.id, - type=IncidentActivityType.COMMENT.value, - comment="hi", - ) - email = generate_incident_activity_email(activity, user) - return MailPreview( - html_template=email.html_template, text_template=email.template, context=email.context - ).render(request) diff --git a/tests/sentry/api/serializers/test_incident_activity.py b/tests/sentry/api/serializers/test_incident_activity.py index 10b81a64efe3a6..b8a4707cd69684 100644 --- a/tests/sentry/api/serializers/test_incident_activity.py +++ b/tests/sentry/api/serializers/test_incident_activity.py @@ -8,47 +8,21 @@ from sentry.incidents.models.incident import IncidentActivityType from sentry.testutils.cases import SnubaTestCase, TestCase from sentry.testutils.helpers.datetime import before_now, freeze_time -from sentry.users.services.user.service import user_service class IncidentActivitySerializerTest(TestCase, SnubaTestCase): def test_simple(self): activity = create_incident_activity( incident=self.create_incident(), - activity_type=IncidentActivityType.COMMENT, - user=self.user, - comment="hello", + activity_type=IncidentActivityType.CREATED, ) result = serialize(activity) assert result["id"] == str(activity.id) assert result["incidentIdentifier"] == str(activity.incident.identifier) - assert ( - result["user"] - == user_service.serialize_many(filter=dict(user_ids=[activity.user_id]))[0] - ) - assert result["type"] == activity.type - assert result["value"] is None - assert result["previousValue"] is None - assert result["comment"] == activity.comment - assert result["dateCreated"] == activity.date_added - - def test_no_user(self): - activity = create_incident_activity( - incident=self.create_incident(), - activity_type=IncidentActivityType.COMMENT, - user=None, - comment="hello", - ) - result = serialize(activity) - - assert result["id"] == str(activity.id) - assert result["incidentIdentifier"] == str(activity.incident.identifier) - assert result["user"] is None assert result["type"] == activity.type assert result["value"] is None assert result["previousValue"] is None - assert result["comment"] == activity.comment assert result["dateCreated"] == activity.date_added def test_event_stats(self): @@ -68,20 +42,13 @@ def test_event_stats(self): ) activity = create_incident_activity( incident=incident, - activity_type=IncidentActivityType.COMMENT, - user=self.user, - comment="hello", + activity_type=IncidentActivityType.CREATED, ) result = serialize(activity) assert result["id"] == str(activity.id) assert result["incidentIdentifier"] == str(activity.incident.identifier) - assert ( - result["user"] - == user_service.serialize_many(filter=dict(user_ids=[activity.user_id]))[0] - ) assert result["type"] == activity.type assert result["value"] is None assert result["previousValue"] is None - assert result["comment"] == activity.comment assert result["dateCreated"] == activity.date_added diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py index 09e8d999f50414..28b31579469cd6 100644 --- a/tests/sentry/incidents/test_logic.py +++ b/tests/sentry/incidents/test_logic.py @@ -21,11 +21,7 @@ from sentry.conf.server import SEER_ANOMALY_DETECTION_STORE_DATA_URL from sentry.constants import ObjectStatus from sentry.deletions.tasks.scheduled import run_scheduled_deletions -from sentry.incidents.events import ( - IncidentCommentCreatedEvent, - IncidentCreatedEvent, - IncidentStatusUpdatedEvent, -) +from sentry.incidents.events import IncidentCreatedEvent, IncidentStatusUpdatedEvent from sentry.incidents.logic import ( CRITICAL_TRIGGER_LABEL, DEFAULT_ALERT_RULE_RESOLUTION, @@ -348,19 +344,6 @@ def test_sessions(self): @freeze_time() class CreateIncidentActivityTest(TestCase, BaseIncidentsTest): - @pytest.fixture(autouse=True) - def _setup_patches(self): - with mock.patch( - "sentry.incidents.tasks.send_subscriber_notifications" - ) as self.send_subscriber_notifications: - with mock.patch("sentry.analytics.base.Analytics.record_event") as self.record_event: - yield - - def assert_notifications_sent(self, activity): - self.send_subscriber_notifications.apply_async.assert_called_once_with( - kwargs={"activity_id": activity.id}, countdown=10 - ) - def test_no_snapshot(self): incident = self.create_incident() self.record_event.reset_mock() @@ -376,82 +359,8 @@ def test_no_snapshot(self): assert activity.user_id == self.user.id assert activity.value == str(IncidentStatus.CLOSED.value) assert activity.previous_value == str(IncidentStatus.WARNING.value) - self.assert_notifications_sent(activity) assert not self.record_event.called - def test_comment(self): - incident = self.create_incident() - comment = "hello" - - assert not IncidentSubscription.objects.filter( - incident=incident, user_id=self.user.id - ).exists() - self.record_event.reset_mock() - activity = create_incident_activity( - incident, IncidentActivityType.COMMENT, user=self.user, comment=comment - ) - assert IncidentSubscription.objects.filter(incident=incident, user_id=self.user.id).exists() - - assert activity.incident == incident - assert activity.type == IncidentActivityType.COMMENT.value - assert activity.user_id == self.user.id - assert activity.comment == comment - assert activity.value is None - assert activity.previous_value is None - self.assert_notifications_sent(activity) - assert len(self.record_event.call_args_list) == 1 - event = self.record_event.call_args[0][0] - assert isinstance(event, IncidentCommentCreatedEvent) - assert event.data == { - "organization_id": str(self.organization.id), - "incident_id": str(incident.id), - "incident_type": str(incident.type), - "user_id": str(self.user.id), - "activity_id": str(activity.id), - } - - def test_mentioned_user_ids(self): - incident = self.create_incident() - mentioned_member = self.create_user() - subscribed_mentioned_member = self.create_user() - IncidentSubscription.objects.create( - incident=incident, user_id=subscribed_mentioned_member.id - ) - comment = f"hello **@{mentioned_member.username}** and **@{subscribed_mentioned_member.username}**" - - assert not IncidentSubscription.objects.filter( - incident=incident, user_id=mentioned_member.id - ).exists() - self.record_event.reset_mock() - activity = create_incident_activity( - incident, - IncidentActivityType.COMMENT, - user=self.user, - comment=comment, - mentioned_user_ids=[mentioned_member.id, subscribed_mentioned_member.id], - ) - assert IncidentSubscription.objects.filter( - incident=incident, user_id=mentioned_member.id - ).exists() - - assert activity.incident == incident - assert activity.type == IncidentActivityType.COMMENT.value - assert activity.user_id == self.user.id - assert activity.comment == comment - assert activity.value is None - assert activity.previous_value is None - self.assert_notifications_sent(activity) - assert len(self.record_event.call_args_list) == 1 - event = self.record_event.call_args[0][0] - assert isinstance(event, IncidentCommentCreatedEvent) - assert event.data == { - "organization_id": str(self.organization.id), - "incident_id": str(incident.id), - "incident_type": str(incident.type), - "user_id": str(self.user.id), - "activity_id": str(activity.id), - } - class GetIncidentSubscribersTest(TestCase, BaseIncidentsTest): def test_simple(self): diff --git a/tests/sentry/incidents/test_tasks.py b/tests/sentry/incidents/test_tasks.py index 6302f6596398f8..7ed97da9dc148a 100644 --- a/tests/sentry/incidents/test_tasks.py +++ b/tests/sentry/incidents/test_tasks.py @@ -4,7 +4,6 @@ from unittest.mock import Mock, call, patch import pytest -from django.urls import reverse from django.utils import timezone from sentry.incidents.logic import ( @@ -12,152 +11,21 @@ create_alert_rule_trigger, create_alert_rule_trigger_action, create_incident_activity, - subscribe_to_incident, ) from sentry.incidents.models.alert_rule import AlertRuleTriggerAction -from sentry.incidents.models.incident import ( - INCIDENT_STATUS, - IncidentActivityType, - IncidentStatus, - IncidentSubscription, -) -from sentry.incidents.tasks import ( - build_activity_context, - generate_incident_activity_email, - handle_subscription_metrics_logger, - handle_trigger_action, - send_subscriber_notifications, -) +from sentry.incidents.models.incident import IncidentActivityType, IncidentStatus +from sentry.incidents.tasks import handle_subscription_metrics_logger, handle_trigger_action from sentry.incidents.utils.constants import SUBSCRIPTION_METRICS_LOGGER from sentry.snuba.dataset import Dataset from sentry.snuba.models import SnubaQuery from sentry.snuba.subscriptions import create_snuba_query, create_snuba_subscription from sentry.testutils.cases import TestCase from sentry.testutils.helpers.alert_rule import TemporaryAlertRuleTriggerActionRegistry -from sentry.testutils.helpers.datetime import freeze_time from sentry.testutils.skips import requires_kafka, requires_snuba -from sentry.users.services.user.service import user_service -from sentry.utils.http import absolute_uri pytestmark = [pytest.mark.sentry_metrics, requires_snuba, requires_kafka] -class BaseIncidentActivityTest(TestCase): - @property - def incident(self): - return self.create_incident(title="hello") - - -class TestSendSubscriberNotifications(BaseIncidentActivityTest): - @pytest.fixture(autouse=True) - def _setup_send_async_patch(self): - with mock.patch("sentry.utils.email.MessageBuilder.send_async") as self.send_async: - yield - - def test_simple(self): - activity = create_incident_activity( - self.incident, IncidentActivityType.COMMENT, user=self.user, comment="hello" - ) - send_subscriber_notifications(activity.id) - # User shouldn't receive an email for their own activity - assert self.send_async.call_count == 0 - - self.send_async.reset_mock() - non_member_user = self.create_user(email="non_member@test.com") - subscribe_to_incident(activity.incident, non_member_user.id) - - member_user = self.create_user(email="member@test.com") - self.create_member([self.team], user=member_user, organization=self.organization) - subscribe_to_incident(activity.incident, member_user.id) - send_subscriber_notifications(activity.id) - self.send_async.assert_called_once_with([member_user.email]) - assert not IncidentSubscription.objects.filter( - incident=activity.incident, user_id=non_member_user.id - ).exists() - assert IncidentSubscription.objects.filter( - incident=activity.incident, user_id=member_user.id - ).exists() - - def test_invalid_types(self): - activity_type = IncidentActivityType.CREATED - activity = create_incident_activity(self.incident, activity_type) - send_subscriber_notifications(activity.id) - assert self.send_async.call_count == 0 - self.send_async.reset_mock() - - -class TestGenerateIncidentActivityEmail(BaseIncidentActivityTest): - @freeze_time() - def test_simple(self): - activity = create_incident_activity( - self.incident, IncidentActivityType.COMMENT, user=self.user, comment="hello" - ) - incident = activity.incident - recipient = self.create_user() - message = generate_incident_activity_email(activity, recipient) - assert message.subject == f"Activity on Alert {incident.title} (#{incident.identifier})" - assert message.type == "incident.activity" - assert message.context == build_activity_context(activity, recipient) - - -class TestBuildActivityContext(BaseIncidentActivityTest): - def run_test( - self, activity, expected_username, expected_action, expected_comment, expected_recipient - ): - incident = activity.incident - context = build_activity_context(activity, expected_recipient) - assert context["user_name"] == expected_username - assert ( - context["action"] - == f"{expected_action} on alert {activity.incident.title} (#{activity.incident.identifier})" - ) - assert ( - context["link"] - == absolute_uri( - reverse( - "sentry-metric-alert", - kwargs={ - "organization_slug": incident.organization.slug, - "incident_id": incident.identifier, - }, - ) - ) - + "?referrer=incident_activity_email" - ) - assert context["comment"] == expected_comment - - @freeze_time() - def test_simple(self): - activity = create_incident_activity( - self.incident, IncidentActivityType.COMMENT, user=self.user, comment="hello" - ) - recipient = self.create_user() - assert activity.user_id is not None - user = user_service.get_user(user_id=activity.user_id) - assert user is not None - self.run_test( - activity, - expected_username=user.name, - expected_action="left a comment", - expected_comment=activity.comment, - expected_recipient=recipient, - ) - activity.type = IncidentActivityType.STATUS_CHANGE - activity.value = str(IncidentStatus.CLOSED.value) - activity.previous_value = str(IncidentStatus.WARNING.value) - assert activity.user_id is not None - user = user_service.get_user(user_id=activity.user_id) - assert user is not None - self.run_test( - activity, - expected_username=user.name, - expected_action="changed status from %s to %s" - % (INCIDENT_STATUS[IncidentStatus.WARNING], INCIDENT_STATUS[IncidentStatus.CLOSED]), - expected_comment=activity.comment, - expected_recipient=recipient, - ) - - class HandleTriggerActionTest(TestCase): @pytest.fixture(autouse=True) def _setup_metric_patch(self): From f64a953b5e710227eba52dd9102614283f586328 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 27 Nov 2024 12:43:44 -0800 Subject: [PATCH 2/3] update tests, remove closing comment --- src/sentry/incidents/tasks.py | 1 - .../endpoints/serializers/test_incident.py | 10 ------- tests/sentry/incidents/test_logic.py | 29 ++----------------- 3 files changed, 2 insertions(+), 38 deletions(-) diff --git a/src/sentry/incidents/tasks.py b/src/sentry/incidents/tasks.py index d6935626ac3cb1..a8ca7387a08491 100644 --- a/src/sentry/incidents/tasks.py +++ b/src/sentry/incidents/tasks.py @@ -163,7 +163,6 @@ def auto_resolve_snapshot_incidents(alert_rule_id: int, **kwargs: Any) -> None: update_incident_status( incident, IncidentStatus.CLOSED, - comment="This alert has been auto-resolved because the rule that triggered it has been modified or deleted.", status_method=IncidentStatusMethod.RULE_UPDATED, ) diff --git a/tests/sentry/incidents/endpoints/serializers/test_incident.py b/tests/sentry/incidents/endpoints/serializers/test_incident.py index b99955e4660ae1..fad3ba967b7321 100644 --- a/tests/sentry/incidents/endpoints/serializers/test_incident.py +++ b/tests/sentry/incidents/endpoints/serializers/test_incident.py @@ -4,7 +4,6 @@ from sentry.api.serializers import serialize from sentry.incidents.endpoints.serializers.incident import DetailedIncidentSerializer -from sentry.incidents.logic import subscribe_to_incident from sentry.snuba.dataset import Dataset from sentry.testutils.cases import TestCase from sentry.testutils.helpers.datetime import freeze_time @@ -31,15 +30,6 @@ def test_simple(self): class DetailedIncidentSerializerTest(TestCase): - def test_subscribed(self): - incident = self.create_incident(date_started=timezone.now() - timedelta(minutes=5)) - serializer = DetailedIncidentSerializer() - result = serialize(incident, serializer=serializer, user=self.user) - assert not result["isSubscribed"] - subscribe_to_incident(incident, self.user.id) - result = serialize(incident, serializer=serializer, user=self.user) - assert result["isSubscribed"] - def test_error_alert_rule(self): query = "test query" incident = self.create_incident(query=query) diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py index 28b31579469cd6..5d6a2117555e85 100644 --- a/tests/sentry/incidents/test_logic.py +++ b/tests/sentry/incidents/test_logic.py @@ -48,10 +48,8 @@ get_alert_resolution, get_available_action_integrations_for_org, get_incident_aggregates, - get_incident_subscribers, get_triggers_for_alert_rule, snapshot_alert_rule, - subscribe_to_incident, translate_aggregate_field, update_alert_rule, update_alert_rule_trigger, @@ -76,7 +74,6 @@ IncidentProject, IncidentStatus, IncidentStatusMethod, - IncidentSubscription, IncidentTrigger, IncidentType, TriggerStatus, @@ -176,16 +173,12 @@ def test_status_already_set(self): ) assert incident.status == IncidentStatus.WARNING.value - def run_test( - self, incident, status, expected_date_closed, user=None, comment=None, date_closed=None - ): + def run_test(self, incident, status, expected_date_closed, user=None, date_closed=None): prev_status = incident.status self.record_event.reset_mock() update_incident_status( incident, status, - user=user, - comment=comment, status_method=IncidentStatusMethod.RULE_TRIGGERED, date_closed=date_closed, ) @@ -194,12 +187,8 @@ def run_test( assert incident.date_closed == expected_date_closed activity = self.get_most_recent_incident_activity(incident) assert activity.type == IncidentActivityType.STATUS_CHANGE.value - assert activity.user_id == (user.id if user else None) - if user: - assert IncidentSubscription.objects.filter(incident=incident, user_id=user.id).exists() assert activity.value == str(status.value) assert activity.previous_value == str(prev_status) - assert activity.comment == comment assert len(self.record_event.call_args_list) == 1 event = self.record_event.call_args[0][0] @@ -230,9 +219,7 @@ def test_closed_specify_date(self): def test_all_params(self): incident = self.create_incident() - self.run_test( - incident, IncidentStatus.CLOSED, timezone.now(), user=self.user, comment="lol" - ) + self.run_test(incident, IncidentStatus.CLOSED, timezone.now(), user=self.user) class BaseIncidentsValidation: @@ -346,28 +333,16 @@ def test_sessions(self): class CreateIncidentActivityTest(TestCase, BaseIncidentsTest): def test_no_snapshot(self): incident = self.create_incident() - self.record_event.reset_mock() activity = create_incident_activity( incident, IncidentActivityType.STATUS_CHANGE, - user=self.user, value=str(IncidentStatus.CLOSED.value), previous_value=str(IncidentStatus.WARNING.value), ) assert activity.incident == incident assert activity.type == IncidentActivityType.STATUS_CHANGE.value - assert activity.user_id == self.user.id assert activity.value == str(IncidentStatus.CLOSED.value) assert activity.previous_value == str(IncidentStatus.WARNING.value) - assert not self.record_event.called - - -class GetIncidentSubscribersTest(TestCase, BaseIncidentsTest): - def test_simple(self): - incident = self.create_incident() - assert list(get_incident_subscribers(incident)) == [] - subscription = subscribe_to_incident(incident, self.user.id) - assert list(get_incident_subscribers(incident)) == [subscription] class CreateAlertRuleTest(TestCase, BaseIncidentsTest): From 0e5dfe080f02a1adeadf02a2f603cf8ca522b66b Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Wed, 27 Nov 2024 13:03:55 -0800 Subject: [PATCH 3/3] remove test --- .../test_organization_incident_details.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/tests/sentry/incidents/endpoints/test_organization_incident_details.py b/tests/sentry/incidents/endpoints/test_organization_incident_details.py index 685d7059e1d8e9..42445edd26c516 100644 --- a/tests/sentry/incidents/endpoints/test_organization_incident_details.py +++ b/tests/sentry/incidents/endpoints/test_organization_incident_details.py @@ -1,7 +1,7 @@ from functools import cached_property from sentry.api.serializers import serialize -from sentry.incidents.models.incident import Incident, IncidentActivity, IncidentStatus +from sentry.incidents.models.incident import Incident, IncidentStatus from sentry.silo.base import SiloMode from sentry.testutils.abstract import Abstract from sentry.testutils.cases import APITestCase @@ -91,22 +91,6 @@ def test_cannot_open(self): assert resp.status_code == 400 assert resp.data.startswith("Status cannot be changed") - def test_comment(self): - incident = self.create_incident() - status = IncidentStatus.CLOSED.value - comment = "fixed" - with self.feature("organizations:incidents"): - self.get_success_response( - incident.organization.slug, incident.identifier, status=status, comment=comment - ) - - incident = Incident.objects.get(id=incident.id) - assert incident.status == status - activity = IncidentActivity.objects.filter(incident=incident).order_by("-id")[:1].get() - assert activity.value == str(status) - assert activity.comment == comment - assert activity.user_id == self.user.id - def test_invalid_status(self): incident = self.create_incident() with self.feature("organizations:incidents"):