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(alerts): Move action check to serializer and remove signals #79389

Merged
merged 2 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 0 additions & 28 deletions src/sentry/incidents/endpoints/organization_alert_rule_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
from sentry.models.rule import Rule, RuleSource
from sentry.models.team import Team
from sentry.sentry_apps.services.app import app_service
from sentry.signals import alert_rule_created
from sentry.snuba.dataset import Dataset
from sentry.snuba.models import SnubaQuery
from sentry.uptime.models import (
Expand Down Expand Up @@ -118,17 +117,9 @@ def create_metric_alert(
},
data=data,
)

if not serializer.is_valid():
raise ValidationError(serializer.errors)

# if there are no triggers, then the serializer will raise an error
for trigger in data["triggers"]:
if not trigger.get("actions", []):
raise ValidationError(
"Each trigger must have an associated action for this alert to fire."
)

trigger_sentry_app_action_creators_for_incidents(serializer.validated_data)
if get_slack_actions_with_async_lookups(organization, request.user, request.data):
# need to kick off an async job for Slack
Expand All @@ -143,25 +134,6 @@ def create_metric_alert(
return Response({"uuid": client.uuid}, status=202)
else:
alert_rule = serializer.save()
referrer = request.query_params.get("referrer")
session_id = request.query_params.get("sessionId")
duplicate_rule = request.query_params.get("duplicateRule")
wizard_v3 = request.query_params.get("wizardV3")
subscriptions = alert_rule.snuba_query.subscriptions.all()
for sub in subscriptions:
alert_rule_created.send_robust(
user=request.user,
project=sub.project,
rule_id=alert_rule.id,
rule_type="metric",
sender=self,
referrer=referrer,
session_id=session_id,
is_api_token=request.auth is not None,
duplicate_rule=duplicate_rule,
wizard_v3=wizard_v3,
query_type=self.get_query_type_description(data.get("queryType", None)),
)
return Response(serialize(alert_rule, request.user), status=status.HTTP_201_CREATED)

def get_query_type_description(self, value):
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/incidents/serializers/alert_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ def validate(self, data):
raise serializers.ValidationError(
"Must send 1 or 2 triggers - A critical trigger, and an optional warning trigger"
)
for trigger in triggers:
if not trigger.get("actions", []):
raise serializers.ValidationError(
"Each trigger must have an associated action for this alert to fire."
)

if query_type == SnubaQuery.Type.CRASH_RATE:
data["event_types"] = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -979,11 +979,14 @@ def test_delete_action(self):
serialized_alert_rule["triggers"][1]["actions"].pop()

with self.feature("organizations:incidents"):
resp = self.get_success_response(
self.organization.slug, alert_rule.id, **serialized_alert_rule
resp = self.get_error_response(
self.organization.slug, alert_rule.id, status_code=400, **serialized_alert_rule
)

assert len(resp.data["triggers"][1]["actions"]) == 0
assert resp.data == {
"nonFieldErrors": [
"Each trigger must have an associated action for this alert to fire."
]
}

def test_update_trigger_action_type(self):
self.create_member(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -924,12 +924,11 @@ def test_critical_trigger_no_action(self):
resp = self.get_error_response(
self.organization.slug, status_code=400, **rule_one_trigger_only_critical_no_action
)
assert resp.data == [
ErrorDetail(
string="Each trigger must have an associated action for this alert to fire.",
code="invalid",
)
]
assert resp.data == {
"nonFieldErrors": [
"Each trigger must have an associated action for this alert to fire."
ceorourke marked this conversation as resolved.
Show resolved Hide resolved
]
}

def test_invalid_projects(self):
with self.feature("organizations:incidents"):
Expand Down Expand Up @@ -1328,9 +1327,6 @@ def test_performance_alert(self, record_analytics):
with self.feature(["organizations:incidents", "organizations:performance-view"]):
resp = self.get_response(self.organization.slug, **valid_alert_rule)
assert resp.status_code == 201
assert self.analytics_called_with_args(
record_analytics, "alert.created", query_type="PERFORMANCE"
)


@freeze_time()
Expand Down
12 changes: 10 additions & 2 deletions tests/sentry/incidents/endpoints/test_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,14 @@ def test_boundary(self):
assert serializer.is_valid(), serializer.errors

def test_boundary_off_by_one(self):
actions = [
{
"type": "slack",
"targetIdentifier": "my-channel",
"targetType": "specific",
"integration": self.integration.id,
}
]
self.run_fail_validation_test(
{
"thresholdType": AlertRuleThresholdType.ABOVE.value,
Expand All @@ -436,7 +444,7 @@ def test_boundary_off_by_one(self):
{
"label": "critical",
"alertThreshold": 0,
"actions": [],
"actions": actions,
},
],
},
Expand All @@ -457,7 +465,7 @@ def test_boundary_off_by_one(self):
{
"label": "critical",
"alertThreshold": 2,
"actions": [],
"actions": actions,
},
],
},
Expand Down
Loading