From 58709a089e61629ffb120b5d6e3e7d15c9d019bf Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 16:27:47 -0700 Subject: [PATCH 1/2] feat(investigations): Register update_detector and say who can apply it Part of a series that adds follow-up actions to Seer investigations: when a run completes, Seer suggests one next step, and the user can apply a typed action, such as a monitor change, with one click. This registers the update_detector action kind. Reads say whether the viewer can apply a monitor change, using the detector endpoint's own plan and permission checks. Depends on https://github.com/getsentry/sentry/pull/128671 --- .../services/action_kinds/base.py | 3 + .../services/action_kinds/update_detector.py | 120 ++++++++++++- .../organization_seer_chat_suggestions.py | 4 +- .../organization_detector_details.py | 8 +- .../endpoints/validators/utils.py | 33 ++-- .../serializers/test_investigation.py | 70 +++++++- ...tion_investigation_action_request_apply.py | 170 ++++++++++++++++++ ...test_organization_investigation_details.py | 12 +- .../endpoints/validators/test_utils.py | 47 +++++ 9 files changed, 440 insertions(+), 27 deletions(-) create mode 100644 tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index 45578c1f6784..bc0b6de2911b 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -20,6 +20,9 @@ class CannotApplyReason(StrEnum): NOT_SUGGESTED = "not_suggested" UNSUPPORTED_ACTION = "unsupported_action" NO_ACCESS = "no_access" + NOT_TARGET_DETECTOR = "not_target_detector" + DETECTOR_NOT_FOUND = "detector_not_found" + PERMISSION_DENIED = "permission_denied" class ActionKind(abc.ABC): diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index e61123b11fe8..3c6e0f3bf96c 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -1,13 +1,20 @@ from __future__ import annotations +from collections import defaultdict +from collections.abc import Collection, Mapping, Sequence from dataclasses import dataclass from typing import Any, Literal, TypedDict, cast +from django.contrib.auth.models import AnonymousUser from rest_framework import serializers +from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.serializers import serialize +from sentry.auth.access import Access from sentry.db.models.fields.bounded import I32_MAX, I64_MAX from sentry.incidents.endpoints.serializers.query_subscription import SnubaQuerySerializer +from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.models.alert_rule import AlertRuleDetectionType from sentry.investigations.contracts import ( ActionPayloadSerializer, StrictCharField, @@ -15,11 +22,23 @@ StrictFloatField, StrictIntegerField, ) -from sentry.investigations.services.action_kinds.base import ActionKind +from sentry.investigations.models import ( + Investigation, + InvestigationActionRequest, + InvestigationOrchestrationRun, +) +from sentry.investigations.services.action_kinds.base import ActionKind, CannotApplyReason +from sentry.models.organization import Organization from sentry.snuba.models import SnubaQuery +from sentry.users.models.user import User +from sentry.users.services.user.model import RpcUser +from sentry.workflow_engine.endpoints.organization_detector_details import ( + check_metric_detector_allowed, +) from sentry.workflow_engine.endpoints.serializers.data_condition_serializer import ( DataConditionSerializer, ) +from sentry.workflow_engine.endpoints.validators.utils import can_edit_detector_with_access from sentry.workflow_engine.models import Detector from sentry.workflow_engine.types import DetectorPriorityLevel @@ -252,6 +271,105 @@ def update(self, changes: DetectorChanges) -> DetectorUpdate: return update +def is_dynamic_detector(detector: Detector) -> bool: + # Saving a dynamic monitor calls Seer's anomaly detection inline, so we leave those alone. + return detector.config.get("detection_type") == AlertRuleDetectionType.DYNAMIC + + +def target_detectors(organization_id: int, detector_ids: Collection[int]) -> dict[int, Detector]: + """The metric detectors with these ids that the detector API would find in this org.""" + + return { + detector.id: detector + for detector in Detector.objects.by_organization(organization_id) + .with_type_filters() + .filter(id__in=detector_ids, type=MetricIssue.slug) + .select_related("project", "workflow_condition_group") + } + + +def source_detector_id(source: Mapping[str, Any]) -> str | None: + """The monitor an investigation's run started from, if it started from one.""" + + detector_id = source.get("detectorId") + return str(detector_id) if detector_id is not None else None + + +def detector_cannot_apply_reason( + detector_id: str, + *, + source_detector_id: str | None, + detector: Detector | None, + organization: Organization, + access: Access, +) -> CannotApplyReason | None: + """Why the viewer can't change `detector_id`, or None if they can. `detector` is what + target_detectors found for that id. + """ + if detector_id != source_detector_id: + return CannotApplyReason.NOT_TARGET_DETECTOR + if detector is None: + return CannotApplyReason.DETECTOR_NOT_FOUND + try: + check_metric_detector_allowed(detector, organization) + except ResourceDoesNotExist: + return CannotApplyReason.DETECTOR_NOT_FOUND + if not can_edit_detector_with_access(detector, access): + return CannotApplyReason.PERMISSION_DENIED + if is_dynamic_detector(detector): + return CannotApplyReason.UNSUPPORTED_ACTION + return None + + +def _action_detector_id(action_request: InvestigationActionRequest) -> int: + # The payload was validated when it arrived, and advice never reaches the kind. + assert action_request.action_payload is not None + return int(action_request.action_payload["detectorId"]) + + class UpdateDetectorKind(ActionKind): kind = UPDATE_DETECTOR_ACTION payload_serializer = UpdateDetectorActionSerializer + + def cannot_apply_reasons( + self, + action_requests: Sequence[InvestigationActionRequest], + *, + investigations: dict[int, Investigation], + access: Access, + user: User | RpcUser | AnonymousUser, + ) -> dict[int, CannotApplyReason | None]: + source_detector_ids = { + investigation_id: source_detector_id(source) + for investigation_id, source in InvestigationOrchestrationRun.objects.filter( + investigation_id__in={item.investigation_id for item in action_requests} + ).values_list("investigation_id", "source") + } + detector_ids_by_org: dict[int, set[int]] = defaultdict(set) + for item in action_requests: + organization_id = investigations[item.investigation_id].organization_id + detector_ids_by_org[organization_id].add(_action_detector_id(item)) + detectors = { + (organization_id, detector_id): detector + for organization_id, detector_ids in detector_ids_by_org.items() + for detector_id, detector in target_detectors(organization_id, detector_ids).items() + } + + # Requests often share a detector, and the checks query. + detector_reasons: dict[tuple[int, str | None, int], CannotApplyReason | None] = {} + reasons: dict[int, CannotApplyReason | None] = {} + for item in action_requests: + detector_id = _action_detector_id(item) + investigation = investigations[item.investigation_id] + source_id = source_detector_ids.get(item.investigation_id) + key = (detector_id, source_id, investigation.organization_id) + if key not in detector_reasons: + detector_reasons[key] = detector_cannot_apply_reason( + str(detector_id), + source_detector_id=source_id, + detector=detectors.get((investigation.organization_id, detector_id)), + organization=investigation.organization, + access=access, + ) + reasons[item.id] = detector_reasons[key] + return reasons diff --git a/src/sentry/seer/endpoints/organization_seer_chat_suggestions.py b/src/sentry/seer/endpoints/organization_seer_chat_suggestions.py index 8e6f30f1f695..f5085f066bb5 100644 --- a/src/sentry/seer/endpoints/organization_seer_chat_suggestions.py +++ b/src/sentry/seer/endpoints/organization_seer_chat_suggestions.py @@ -32,7 +32,7 @@ from sentry.types.ratelimit import RateLimit, RateLimitCategory from sentry.utils import metrics from sentry.workflow_engine.endpoints.organization_detector_details import ( - _check_metric_detector_allowed, + check_metric_detector_allowed, ) from sentry.workflow_engine.endpoints.validators.utils import ( ORGANIZATION_WORKFLOW_WRITE_SCOPES, @@ -233,7 +233,7 @@ def _can_edit_node_type( return None try: - _check_metric_detector_allowed(detector, organization) + check_metric_detector_allowed(detector, organization) except ResourceDoesNotExist: return None if can_edit_detector(detector, request): diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_details.py b/src/sentry/workflow_engine/endpoints/organization_detector_details.py index 3c2b4d6a003e..94cb9b41a52e 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_details.py @@ -50,7 +50,7 @@ from sentry.workflow_engine.models import DataSource, Detector -def _check_metric_detector_allowed(detector: Detector, organization: Organization) -> None: +def check_metric_detector_allowed(detector: Detector, organization: Organization) -> None: """Raise ResourceDoesNotExist if this is a metric detector the org is not entitled to.""" if detector.type != MetricIssue.slug: return @@ -138,7 +138,7 @@ def get( """ Return details on an individual monitor """ - _check_metric_detector_allowed(detector, organization) + check_metric_detector_allowed(detector, organization) serialized_detector = serialize( detector, @@ -170,7 +170,7 @@ def put( """ Update an existing monitor """ - _check_metric_detector_allowed(detector, organization) + check_metric_detector_allowed(detector, organization) if not can_edit_detector(detector, request): if not is_system_created_detector(detector): @@ -209,7 +209,7 @@ def delete( """ Delete a monitor """ - # Intentionally no _check_metric_detector_allowed gate here: + # Intentionally no check_metric_detector_allowed gate here: # orgs should be able to delete detectors they can no longer use # (e.g. after a plan downgrade). if not can_delete_detector(detector, request): diff --git a/src/sentry/workflow_engine/endpoints/validators/utils.py b/src/sentry/workflow_engine/endpoints/validators/utils.py index dcf379bf6ede..09c916311121 100644 --- a/src/sentry/workflow_engine/endpoints/validators/utils.py +++ b/src/sentry/workflow_engine/endpoints/validators/utils.py @@ -13,6 +13,7 @@ from sentry import audit_log from sentry.api.permissions import enforce_scope +from sentry.auth.access import Access from sentry.issues import grouptype from sentry.models.organization import Organization from sentry.models.project import Project @@ -45,15 +46,6 @@ def is_system_created_detector(detector: Detector) -> bool: ) -def can_edit_system_created_detectors(request: Request, detector: Detector) -> bool: - if detector.type == IssueStreamGroupType.slug: - return False - - return request.access.has_any_project_scope( - detector.linked_project, SYSTEM_CREATED_DETECTOR_REQUIRED_SCOPES - ) - - def can_edit_user_created_detectors(request: Request, project: Project) -> bool: return request.access.has_any_project_scope(project, USER_CREATED_DETECTOR_REQUIRED_SCOPES) @@ -79,18 +71,29 @@ def can_edit_detectors(detectors: QuerySet[Detector], request: Request) -> bool: ) +def can_edit_detector_with_access(detector: Detector, access: Access) -> bool: + """Like can_edit_detector, for callers that have an Access instead of a request.""" + if is_system_created_detector(detector): + # Check this first, because the all-projects issue stream detector has no project. + if detector.type == IssueStreamGroupType.slug: + return False + if not access.has_any_project_scope( + detector.linked_project, SYSTEM_CREATED_DETECTOR_REQUIRED_SCOPES + ): + return False + + return access.has_any_project_scope( + detector.linked_project, USER_CREATED_DETECTOR_REQUIRED_SCOPES + ) + + def can_edit_detector(detector: Detector, request: Request) -> bool: """ Determine if the requesting user has access to detector edit. If the request does not have the "alerts:write" permission, then we must verify that the user is a team admin with "alerts:write" access to the project(s) in their request. """ - if is_system_created_detector(detector) and not can_edit_system_created_detectors( - request, detector - ): - return False - - return can_edit_user_created_detectors(request, detector.linked_project) + return can_edit_detector_with_access(detector, request.access) def can_delete_detectors(detectors: QuerySet[Detector], request: Request) -> bool: diff --git a/tests/sentry/investigations/endpoints/serializers/test_investigation.py b/tests/sentry/investigations/endpoints/serializers/test_investigation.py index 2117a7ed621d..80f624211af2 100644 --- a/tests/sentry/investigations/endpoints/serializers/test_investigation.py +++ b/tests/sentry/investigations/endpoints/serializers/test_investigation.py @@ -8,6 +8,7 @@ from sentry.api.serializers import serialize from sentry.auth.access import from_user +from sentry.constants import ObjectStatus from sentry.incidents.grouptype import MetricIssue from sentry.investigations.endpoints.serializers import ( InvestigationDetailsSerializer, @@ -23,6 +24,7 @@ ) from sentry.investigations.services.action_kinds import ACTION_KINDS from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.features import with_feature from tests.sentry.investigations.test_contracts import RecordingKind, fake_action, fake_action_kind @@ -343,8 +345,8 @@ def test_serializes_an_action_request(self) -> None: "state": "suggested", "reason": None, "action": self.action(), - "canApply": False, - "cannotApplyReason": "unsupported_action", + "canApply": True, + "cannotApplyReason": None, "appliedBy": None, "appliedDetails": None, "dateApplied": None, @@ -486,6 +488,13 @@ def serialize_for(self, user: Any) -> list[dict[str, Any]]: def cannot_apply_reason(self, user: Any = None) -> str | None: return self.serialize_for(user or self.user)[0]["cannotApplyReason"] + def test_an_editor_can_apply_a_suggestion(self) -> None: + self.add_request() + + (serialized,) = self.serialize_for(self.user) + assert serialized["action"] == self.action() + assert (serialized["canApply"], serialized["cannotApplyReason"]) == (True, None) + def test_archived_investigations_cannot_apply(self) -> None: self.add_request() self.investigation.update(status=InvestigationStatus.ARCHIVED) @@ -529,3 +538,60 @@ def test_nothing_can_apply_without_access(self) -> None: assert (serialized["canApply"], serialized["cannotApplyReason"]) == (False, "no_access") assert kind.checked == [] + + def test_dynamic_monitors_cannot_apply(self) -> None: + self.add_request() + self.detector.update(config={"detection_type": "dynamic"}) + + assert self.cannot_apply_reason() == "unsupported_action" + + def test_only_the_source_detector_can_be_changed(self) -> None: + other = self.create_detector(project=self.project, type=MetricIssue.slug) + self.add_request(action=self.action(detectorId=str(other.id))) + + assert self.cannot_apply_reason() == "not_target_detector" + + def test_missing_detectors_cannot_apply(self) -> None: + self.add_request() + self.detector.update(status=ObjectStatus.PENDING_DELETION) + + assert self.cannot_apply_reason() == "detector_not_found" + + def test_detectors_in_other_organizations_cannot_apply(self) -> None: + other_project = self.create_project(organization=self.create_organization()) + foreign = self.create_detector(project=other_project, type=MetricIssue.slug) + self.investigation.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(foreign.id)} + ) + self.add_request(action=self.action(detectorId=str(foreign.id))) + + assert self.cannot_apply_reason() == "detector_not_found" + + def test_users_who_cannot_edit_the_detector_cannot_apply(self) -> None: + self.organization.update_option("sentry:alerts_member_write", False) + member = self.create_user() + self.create_member(user=member, organization=self.organization, role="member") + self.add_request() + + assert self.cannot_apply_reason(member) == "permission_denied" + + @with_feature("organizations:team-roles") + def test_query_count_does_not_grow_with_suggestions(self) -> None: + self.organization.update_option("sentry:alerts_member_write", False) + member = self.create_user() + self.create_member(user=member, organization=self.organization, role="member") + team = self.create_team(organization=self.organization) + self.project.add_team(team) + self.create_team_membership(team=team, user=member, role="admin") + self.add_request() + with CaptureQueriesContext(connection) as one: + assert self.serialize_for(member)[0]["canApply"] is True + + self.add_request("second") + self.add_request("third") + for index in range(5): + self.add_request(f"applied-{index}", state=InvestigationActionRequestState.APPLIED) + with CaptureQueriesContext(connection) as many: + assert len(self.serialize_for(member)) == 8 + + assert len(many.captured_queries) == len(one.captured_queries) diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py new file mode 100644 index 000000000000..ce3ca7d501cb --- /dev/null +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_action_request_apply.py @@ -0,0 +1,170 @@ +from __future__ import annotations + +from collections.abc import Sequence +from datetime import timedelta +from typing import Any + +from django.urls import reverse + +from sentry.incidents.grouptype import MetricIssue +from sentry.incidents.utils.constants import INCIDENTS_SNUBA_SUBSCRIPTION_TYPE +from sentry.incidents.utils.subscription_limits import METRIC_SUBSCRIPTION_FEATURE_FLAGS +from sentry.investigations.models import ( + InvestigationActionRequest, +) +from sentry.seer.models.run import SeerRunMirrorStatus, SeerRunType +from sentry.snuba.dataset import Dataset +from sentry.snuba.models import ExtrapolationMode, SnubaQuery, SnubaQueryEventType +from sentry.snuba.subscriptions import create_snuba_query, create_snuba_subscription +from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers.features import with_feature +from sentry.testutils.skips import requires_kafka, requires_snuba +from sentry.workflow_engine.models import DataConditionGroup, Detector +from sentry.workflow_engine.models.data_condition import Condition +from sentry.workflow_engine.types import DetectorPriorityLevel + +pytestmark = [requires_snuba, requires_kafka] + +FEATURE = "organizations:investigations" + + +@with_feature({FEATURE: True, **METRIC_SUBSCRIPTION_FEATURE_FLAGS}) +class OrganizationInvestigationActionRequestApplyTest(APITestCase): + def setUp(self) -> None: + super().setUp() + self.login_as(self.user) + self.environment = self.create_environment(project=self.project, name="production") + self.detector = self.create_metric_detector() + self.investigation = self.create_investigation( + organization=self.organization, created_by=self.user, title="Breach" + ) + self.orchestration_run = self.create_investigation_orchestration_run( + investigation=self.investigation, + seer_run=self.create_seer_run( + organization=self.organization, + type=SeerRunType.INVESTIGATION, + seer_run_state_id=4815, + mirror_status=SeerRunMirrorStatus.LIVE, + ), + workflow_version=4, + phase="completed", + status="completed", + source={"type": "breached_metric", "detectorId": str(self.detector.id)}, + projection={"workflowVersion": 4, "report": {"revision": 0, "status": "completed"}}, + ) + + def create_metric_detector( + self, + *, + dataset: Dataset = Dataset.Events, + time_window: int = 300, + high: float = 500.0, + conditions: Sequence[tuple[Condition, float, int]] | None = None, + config: dict[str, Any] | None = None, + ) -> Detector: + eap = dataset == Dataset.EventsAnalyticsPlatform + with self.tasks(): + snuba_query = create_snuba_query( + query_type=SnubaQuery.Type.PERFORMANCE if eap else SnubaQuery.Type.ERROR, + dataset=dataset, + query="span.op:http.client" if eap else "level:error", + aggregate="count(span.duration)" if eap else "count()", + time_window=timedelta(seconds=time_window), + resolution=timedelta(minutes=1), + environment=self.environment, + event_types=[ + SnubaQueryEventType.EventType.TRACE_ITEM_SPAN + if eap + else SnubaQueryEventType.EventType.ERROR + ], + extrapolation_mode=(ExtrapolationMode.CLIENT_AND_SERVER_WEIGHTED if eap else None), + ) + subscription = create_snuba_subscription( + project=self.project, + subscription_type=INCIDENTS_SNUBA_SUBSCRIPTION_TYPE, + snuba_query=snuba_query, + ) + group = self.create_data_condition_group( + organization_id=self.organization.id, logic_type=DataConditionGroup.Type.ANY + ) + for condition_type, comparison, result in conditions or [ + (Condition.GREATER, high, DetectorPriorityLevel.HIGH), + (Condition.LESS_OR_EQUAL, high, DetectorPriorityLevel.OK), + ]: + self.create_data_condition( + condition_group=group, + type=condition_type, + comparison=comparison, + condition_result=result, + ) + detector = self.create_detector( + project=self.project, + type=MetricIssue.slug, + workflow_condition_group=group, + config=config or {"detection_type": "static"}, + ) + data_source = self.create_data_source( + organization=self.organization, source_id=subscription.id + ) + self.create_data_source_detector(data_source=data_source, detector=detector) + return detector + + def suggest( + self, + *, + before: dict[str, Any] | None = None, + changes: dict[str, Any] | None = None, + detector_id: int | None = None, + **values: Any, + ) -> InvestigationActionRequest: + values.setdefault("action_kind", "update_detector") + values.setdefault( + "action_payload", + { + "kind": "update_detector", + "detectorId": str(detector_id or self.detector.id), + "before": before or {"timeWindowSeconds": 300}, + "changes": changes or {"timeWindowSeconds": 900}, + }, + ) + return self.create_investigation_action_request( + investigation=self.investigation, key="widen-window", **values + ) + + def details_request(self) -> dict[str, Any]: + response = self.client.get( + reverse( + "sentry-api-0-organization-investigation-details", + kwargs={ + "organization_id_or_slug": self.organization.slug, + "investigation_id": self.investigation.id, + }, + ) + ) + assert response.status_code == 200 + [action_request] = response.data["actionRequests"] + return action_request + + def test_details_say_the_viewer_can_apply_a_change_to_the_monitor(self) -> None: + self.suggest( + before={"query": "level:error"}, changes={"query": "level:error !logger:health"} + ) + + serialized = self.details_request() + + assert (serialized["canApply"], serialized["cannotApplyReason"]) == (True, None) + + def test_a_dataset_the_plan_no_longer_allows_cannot_apply(self) -> None: + self.detector = self.create_metric_detector(dataset=Dataset.EventsAnalyticsPlatform) + self.orchestration_run.update( + source={"type": "breached_metric", "detectorId": str(self.detector.id)} + ) + self.suggest() + + with self.feature({"organizations:visibility-explore-view": False}): + serialized = self.details_request() + + assert (serialized["canApply"], serialized["cannotApplyReason"]) == ( + False, + "detector_not_found", + ) diff --git a/tests/sentry/investigations/endpoints/test_organization_investigation_details.py b/tests/sentry/investigations/endpoints/test_organization_investigation_details.py index 954410fd037a..eafeae97f402 100644 --- a/tests/sentry/investigations/endpoints/test_organization_investigation_details.py +++ b/tests/sentry/investigations/endpoints/test_organization_investigation_details.py @@ -303,11 +303,17 @@ def test_detail_includes_action_requests(self) -> None: [serialized] = response.data["actionRequests"] assert serialized["id"] == str(action_request.id) assert serialized["action"] == action_request.action_payload - assert (serialized["canApply"], serialized["cannotApplyReason"]) == ( - False, - "unsupported_action", + assert (serialized["canApply"], serialized["cannotApplyReason"]) == (True, None) + + updated = self.client.put( + url, + data={"investigationVersion": investigation.version, "title": "Renamed"}, + format="json", ) + assert updated.status_code == 200 + assert updated.data["actionRequests"][0]["canApply"] is True + def test_metadata_update_persists_and_stale_version_rolls_back(self) -> None: created = self.client.post( self.collection_url, diff --git a/tests/sentry/workflow_engine/endpoints/validators/test_utils.py b/tests/sentry/workflow_engine/endpoints/validators/test_utils.py index ffca7e5a3bd9..eb1d96cb726f 100644 --- a/tests/sentry/workflow_engine/endpoints/validators/test_utils.py +++ b/tests/sentry/workflow_engine/endpoints/validators/test_utils.py @@ -4,10 +4,14 @@ from rest_framework.serializers import ValidationError from sentry.auth.access import Access, NoAccess, SystemAccess, from_user +from sentry.grouping.grouptype import ErrorGroupType +from sentry.incidents.grouptype import MetricIssue from sentry.testutils.cases import TestCase from sentry.testutils.helpers.features import with_feature from sentry.workflow_engine.defaults.detectors import ensure_default_all_projects_detector from sentry.workflow_engine.endpoints.validators.utils import ( + can_edit_detector, + can_edit_detector_with_access, connect_detectors_to_workflows, validate_detectors_exist_and_have_permissions, validate_workflow_connections, @@ -172,3 +176,46 @@ def test_null_project_detector_permission_denied_when_no_access(self) -> None: with pytest.raises(PermissionDenied): validate_detectors_exist_and_have_permissions([detector.id], self.organization, request) + + +class TestCanEditDetectorWithAccess(TestCase): + def setUp(self) -> None: + super().setUp() + self.organization.update_option("sentry:alerts_member_write", False) + self.member = self.create_user() + self.create_member(user=self.member, organization=self.organization, role="member") + self.metric_detector = self.create_detector(project=self.project, type=MetricIssue.slug) + self.error_detector = self.create_detector(project=self.project, type=ErrorGroupType.slug) + + def test_user_created_detectors_need_alerts_write(self) -> None: + assert can_edit_detector_with_access( + self.metric_detector, from_user(self.user, self.organization) + ) + assert not can_edit_detector_with_access( + self.metric_detector, from_user(self.member, self.organization) + ) + assert not can_edit_detector_with_access(self.metric_detector, NoAccess()) + + def test_system_created_detectors_need_org_write(self) -> None: + self.organization.update_option("sentry:alerts_member_write", True) + member_access = from_user(self.member, self.organization) + + assert can_edit_detector_with_access(self.metric_detector, member_access) + assert not can_edit_detector_with_access(self.error_detector, member_access) + assert can_edit_detector_with_access( + self.error_detector, from_user(self.user, self.organization) + ) + + def test_the_all_projects_detector_cannot_be_edited(self) -> None: + detector = ensure_default_all_projects_detector(self.organization.id) + + assert detector.project_id is None + assert not can_edit_detector_with_access(detector, from_user(self.user, self.organization)) + + def test_request_variant_uses_the_request_access(self) -> None: + request: Request = self.make_request(user=self.member) # type: ignore[assignment] + request.access = from_user(self.member, self.organization) + assert not can_edit_detector(self.metric_detector, request) + + request.access = from_user(self.user, self.organization) + assert can_edit_detector(self.metric_detector, request) From 1d4f21807ae7cb062a346cf2d2b58ca67597617f Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 17:45:58 -0700 Subject: [PATCH 2/2] feat(investigations): Give dynamic monitors their own cannot-apply reason The UI told users Seer can't apply this kind of change for a dynamic monitor, which Seer does apply on static ones. A separate code lets it say why. --- .../investigations/services/action_kinds/base.py | 1 + .../services/action_kinds/update_detector.py | 2 +- .../endpoints/serializers/test_investigation.py | 11 ++++++++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/sentry/investigations/services/action_kinds/base.py b/src/sentry/investigations/services/action_kinds/base.py index bc0b6de2911b..55efc2c27e5a 100644 --- a/src/sentry/investigations/services/action_kinds/base.py +++ b/src/sentry/investigations/services/action_kinds/base.py @@ -23,6 +23,7 @@ class CannotApplyReason(StrEnum): NOT_TARGET_DETECTOR = "not_target_detector" DETECTOR_NOT_FOUND = "detector_not_found" PERMISSION_DENIED = "permission_denied" + DYNAMIC_DETECTOR = "dynamic_detector" class ActionKind(abc.ABC): diff --git a/src/sentry/investigations/services/action_kinds/update_detector.py b/src/sentry/investigations/services/action_kinds/update_detector.py index 3c6e0f3bf96c..555e105e6341 100644 --- a/src/sentry/investigations/services/action_kinds/update_detector.py +++ b/src/sentry/investigations/services/action_kinds/update_detector.py @@ -317,7 +317,7 @@ def detector_cannot_apply_reason( if not can_edit_detector_with_access(detector, access): return CannotApplyReason.PERMISSION_DENIED if is_dynamic_detector(detector): - return CannotApplyReason.UNSUPPORTED_ACTION + return CannotApplyReason.DYNAMIC_DETECTOR return None diff --git a/tests/sentry/investigations/endpoints/serializers/test_investigation.py b/tests/sentry/investigations/endpoints/serializers/test_investigation.py index 80f624211af2..6460155ccd98 100644 --- a/tests/sentry/investigations/endpoints/serializers/test_investigation.py +++ b/tests/sentry/investigations/endpoints/serializers/test_investigation.py @@ -543,7 +543,16 @@ def test_dynamic_monitors_cannot_apply(self) -> None: self.add_request() self.detector.update(config={"detection_type": "dynamic"}) - assert self.cannot_apply_reason() == "unsupported_action" + assert self.cannot_apply_reason() == "dynamic_detector" + + def test_each_suggestion_gets_the_reason_for_its_own_monitor(self) -> None: + other = self.create_detector(project=self.project, type=MetricIssue.slug) + self.add_request("own-monitor") + self.add_request("other-monitor", action=self.action(detectorId=str(other.id))) + + reasons = {row["key"]: row["cannotApplyReason"] for row in self.serialize_for(self.user)} + + assert reasons == {"own-monitor": None, "other-monitor": "not_target_detector"} def test_only_the_source_detector_can_be_changed(self) -> None: other = self.create_detector(project=self.project, type=MetricIssue.slug)