feat(investigations): Register update_detector and say who can apply it

src/sentry/investigations/services/action_kinds/base.py

@@ -20,6 +20,10 @@ 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"
+    DYNAMIC_DETECTOR = "dynamic_detector"
 
 
 class ActionKind(abc.ABC):

src/sentry/investigations/services/action_kinds/update_detector.py

@@ -1,25 +1,44 @@
 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,
     StrictContractSerializer,
     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.DYNAMIC_DETECTOR
+    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

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):

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):

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:

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,69 @@ 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() == "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)
+        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)