fix(pr): PR comments + tests

leedongwei · leedongwei · commit 497a19fcfb88 · 2025-07-17T11:39:22.000-07:00
diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_index.py b/src/sentry/workflow_engine/endpoints/organization_detector_index.py
@@ -1,3 +1,4 @@
+from collections.abc import Iterable, Sequence
 from functools import partial
 
 from django.db.models import Count, Q
@@ -16,6 +17,7 @@
 from sentry.api.event_search import SearchConfig, SearchFilter, SearchKey, default_config
 from sentry.api.event_search import parse_search_query as base_parse_search_query
 from sentry.api.exceptions import ResourceDoesNotExist
+from sentry.api.issue_search import convert_actor_or_none_value
 from sentry.api.paginator import OffsetPaginator
 from sentry.api.serializers import serialize
 from sentry.apidocs.constants import (
@@ -29,7 +31,9 @@
 from sentry.issues import grouptype
 from sentry.models.organization import Organization
 from sentry.models.project import Project
-from sentry.search.utils import parse_actor_or_none_value
+from sentry.models.team import Team
+from sentry.users.models.user import User
+from sentry.users.services.user import RpcUser
 from sentry.workflow_engine.endpoints.serializers import DetectorSerializer
 from sentry.workflow_engine.endpoints.utils.filters import apply_filter
 from sentry.workflow_engine.endpoints.utils.sortby import SortByParam
@@ -47,31 +51,25 @@
 parse_detector_query = partial(base_parse_search_query, config=detector_search_config)
 
 
-def convert_assignee_value(value: str, projects: list[Project], user) -> Q:
+def convert_assignee_values(value: Iterable[str], projects: Sequence[Project], user: User) -> Q:
     """
     Convert an assignee search value to a Django Q object for filtering detectors.
     """
-    if value == "me":
-        # Handle "me" keyword to search for current user
-        return Q(owner_user_id=user.id)
-    elif value == "unassigned":
-        # Handle "unassigned" keyword
-        return Q(owner_user_id__isnull=True, owner_team__isnull=True)
-    else:
-        # Parse actor (user or team) and create appropriate Q object
-        actor = parse_actor_or_none_value(projects, value, user)
-        if actor is None:
-            # If we can't parse the actor, return a query that matches nothing
-            return Q(pk__isnull=True)
-        elif hasattr(actor, "id") and hasattr(actor, "_meta") and actor._meta.model_name == "user":
-            # It's a user
-            return Q(owner_user_id=actor.id)
-        elif hasattr(actor, "id") and hasattr(actor, "_meta") and actor._meta.model_name == "team":
-            # It's a team
-            return Q(owner_team_id=actor.id)
+    actors_or_none: list[RpcUser | Team | None] = convert_actor_or_none_value(
+        value, projects, user, None
+    )
+    assignee_query = Q()
+    for actor in actors_or_none:
+        if isinstance(actor, (User, RpcUser)):
+            assignee_query |= Q(owner_user_id=actor.id)
+        elif isinstance(actor, Team):
+            assignee_query |= Q(owner_team_id=actor.id)
+        elif actor is None:
+            assignee_query |= Q(owner_team_id__isnull=True, owner_user_id__isnull=True)
         else:
             # Unknown actor type, return a query that matches nothing
-            return Q(pk__isnull=True)
+            assignee_query |= Q(pk__isnull=True)
+    return assignee_query
 
 
 # Maps API field name to database field name, with synthetic aggregate fields keeping
@@ -164,17 +162,13 @@ def get(self, request: Request, organization: Organization) -> Response:
                     case SearchFilter(key=SearchKey("type"), operator=("=" | "IN" | "!=")):
                         queryset = apply_filter(queryset, filter, "type")
                     case SearchFilter(key=SearchKey("assignee"), operator=("=" | "IN" | "!=")):
-                        # Handle assignee filtering with support for users, teams, "me", and "unassigned"
-                        if isinstance(filter.value.value, list):
-                            # Handle multiple values (IN operator)
-                            assignee_q = Q()
-                            for value in filter.value.value:
-                                assignee_q |= convert_assignee_value(value, projects, request.user)
-                        else:
-                            # Handle single value
-                            assignee_q = convert_assignee_value(
-                                filter.value.value, projects, request.user
-                            )
+                        # Filter values can be emails, team slugs, "me", "my_teams", "none"
+                        values = (
+                            filter.value.value
+                            if isinstance(filter.value.value, list)
+                            else [filter.value.value]
+                        )
+                        assignee_q = convert_assignee_values(values, projects, request.user)
 
                         if filter.operator == "!=":
                             queryset = queryset.exclude(assignee_q)
diff --git a/static/app/views/detectors/components/detectorSearch.tsx b/static/app/views/detectors/components/detectorSearch.tsx
@@ -1,94 +1,16 @@
 import {SearchQueryBuilder} from 'sentry/components/searchQueryBuilder';
 import {t} from 'sentry/locale';
-import type {Tag, TagCollection} from 'sentry/types/group';
-import type {FieldDefinition} from 'sentry/utils/fields';
-import {FieldKey, FieldKind} from 'sentry/utils/fields';
-import useAssignedSearchValues from 'sentry/utils/membersAndTeams/useAssignedSearchValues';
+import {getFieldDefinition} from 'sentry/utils/fields';
 import {useLocation} from 'sentry/utils/useLocation';
 import {useNavigate} from 'sentry/utils/useNavigate';
-import {DETECTOR_FILTER_KEYS} from 'sentry/views/detectors/constants';
-
-function getDetectorFilterKeyDefinition(filterKey: string): FieldDefinition | null {
-  if (DETECTOR_FILTER_KEYS.hasOwnProperty(filterKey) && DETECTOR_FILTER_KEYS[filterKey]) {
-    const {description, valueType, keywords, values} = DETECTOR_FILTER_KEYS[filterKey];
-
-    return {
-      kind: FieldKind.FIELD,
-      desc: description,
-      valueType,
-      keywords,
-      values,
-    };
-  }
-
-  return null;
-}
+import {useDetectorFilterKeys} from 'sentry/views/detectors/utils/useDetectorFilterKeys';
 
 export function DetectorSearch() {
   const location = useLocation();
   const navigate = useNavigate();
-  const assignedValues = useAssignedSearchValues();
+  const filterKeys = useDetectorFilterKeys();
   const query = typeof location.query.query === 'string' ? location.query.query : '';
 
-  const filterKeys: TagCollection = Object.fromEntries(
-    Object.keys(DETECTOR_FILTER_KEYS).map(key => {
-      const {values} = DETECTOR_FILTER_KEYS[key] ?? {};
-
-      // Special handling for assignee field to provide user/team values
-      // See getFeedbackFilterKeys in static/app/components/feedback/feedbackSearch.tsx
-      if (key === FieldKey.ASSIGNED) {
-        return [
-          key,
-          {
-            key,
-            name: key,
-            predefined: true,
-            values: assignedValues,
-          },
-        ];
-      }
-
-      return [
-        key,
-        {
-          key,
-          name: key,
-          predefined: values !== undefined,
-          values,
-        },
-      ];
-    })
-  );
-
-  const getTagValues = (tag: Tag, _query: string): Promise<string[]> => {
-    // For assignee field, return the assigned values filtered by query
-    if (tag.key === 'assignee') {
-      const allAssigneeValues: string[] = [];
-      assignedValues.forEach(group => {
-        if (Array.isArray(group.children)) {
-          group.children.forEach(child => {
-            if (typeof child.value === 'string') {
-              allAssigneeValues.push(child.value);
-            }
-          });
-        }
-      });
-
-      // Filter by query if provided
-      if (_query) {
-        return Promise.resolve(
-          allAssigneeValues.filter(value =>
-            value.toLowerCase().includes(_query.toLowerCase())
-          )
-        );
-      }
-      return Promise.resolve(allAssigneeValues);
-    }
-
-    // For other fields, return empty array (no dynamic values)
-    return Promise.resolve([]);
-  };
-
   return (
     <SearchQueryBuilder
       initialQuery={query}
@@ -103,9 +25,9 @@ export function DetectorSearch() {
         });
       }}
       filterKeys={filterKeys}
-      getTagValues={getTagValues}
+      getTagValues={() => Promise.resolve([])}
       searchSource="detectors-list"
-      fieldDefinitionGetter={getDetectorFilterKeyDefinition}
+      fieldDefinitionGetter={getFieldDefinition}
       disallowUnsupportedFilters
       disallowWildcard
       disallowLogicalOperators
diff --git a/static/app/views/detectors/constants.tsx b/static/app/views/detectors/constants.tsx
@@ -37,7 +37,7 @@ export const DETECTOR_FILTER_KEYS: Record<
     keywords: ['type'],
   },
   assignee: {
-    description: 'User or team assigned to the detector',
+    description: 'User or team assigned to the monitor',
     valueType: FieldValueType.STRING,
     keywords: ['assigned', 'owner'],
   },
diff --git a/static/app/views/detectors/utils/useDetectorFilterKeys.tsx b/static/app/views/detectors/utils/useDetectorFilterKeys.tsx
@@ -0,0 +1,40 @@
+import {useMemo} from 'react';
+
+import type {TagCollection} from 'sentry/types/group';
+import useAssignedSearchValues from 'sentry/utils/membersAndTeams/useAssignedSearchValues';
+import {DETECTOR_FILTER_KEYS} from 'sentry/views/detectors/constants';
+
+export function useDetectorFilterKeys(): TagCollection {
+  const assignedValues = useAssignedSearchValues();
+
+  return useMemo(() => {
+    return Object.fromEntries(
+      Object.keys(DETECTOR_FILTER_KEYS).map(key => {
+        const {values} = DETECTOR_FILTER_KEYS[key] ?? {};
+
+        // Special handling for assignee field to provide user/team values
+        if (key === 'assignee') {
+          return [
+            key,
+            {
+              key,
+              name: key,
+              predefined: true,
+              values: assignedValues,
+            },
+          ];
+        }
+
+        return [
+          key,
+          {
+            key,
+            name: key,
+            predefined: values !== undefined,
+            values,
+          },
+        ];
+      })
+    );
+  }, [assignedValues]);
+}
diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py
@@ -1,12 +1,14 @@
 from unittest import mock
 
+from django.db.models import Q
 from rest_framework.exceptions import ErrorDetail
 
 from sentry.api.serializers import serialize
 from sentry.grouping.grouptype import ErrorGroupType
 from sentry.incidents.grouptype import MetricIssue
 from sentry.incidents.models.alert_rule import AlertRuleDetectionType
 from sentry.models.environment import Environment
+from sentry.search.utils import _HACKY_INVALID_USER
 from sentry.snuba.dataset import Dataset
 from sentry.snuba.models import (
     QuerySubscription,
@@ -19,6 +21,7 @@
 from sentry.testutils.silo import region_silo_test
 from sentry.uptime.grouptype import UptimeDomainCheckFailure
 from sentry.uptime.types import DATA_SOURCE_UPTIME_SUBSCRIPTION
+from sentry.workflow_engine.endpoints.organization_detector_index import convert_assignee_values
 from sentry.workflow_engine.models import DataCondition, DataConditionGroup, DataSource, Detector
 from sentry.workflow_engine.models.data_condition import Condition
 from sentry.workflow_engine.registry import data_source_type_registry
@@ -543,21 +546,68 @@ def test_invalid_owner(self):
         )
         assert "owner" in response.data
 
-    def test_owner_not_in_organization(self):
-        # Create a user in another organization
-        other_org = self.create_organization()
-        other_user = self.create_user()
-        self.create_member(organization=other_org, user=other_user)
 
-        # Test with owner not in current organization
-        data_with_invalid_owner = {
-            **self.valid_data,
-            "owner": other_user.get_actor_identifier(),
-        }
+@region_silo_test
+class ConvertAssigneeValuesTest(APITestCase):
+    """Test the convert_assignee_values function"""
 
-        response = self.get_error_response(
-            self.organization.slug,
-            **data_with_invalid_owner,
-            status_code=400,
-        )
-        assert "owner" in response.data
+    def setUp(self):
+        super().setUp()
+        self.user = self.create_user()
+        self.team = self.create_team(organization=self.organization)
+        self.other_user = self.create_user()
+        self.create_member(organization=self.organization, user=self.other_user)
+        self.projects = [self.project]
+
+    def test_convert_assignee_values_user_email(self):
+        result = convert_assignee_values([self.user.email], self.projects, self.user)
+        expected = Q(owner_user_id=self.user.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_user_username(self):
+        result = convert_assignee_values([self.user.username], self.projects, self.user)
+        expected = Q(owner_user_id=self.user.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_team_slug(self):
+        result = convert_assignee_values([f"#{self.team.slug}"], self.projects, self.user)
+        expected = Q(owner_team_id=self.team.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_me(self):
+        result = convert_assignee_values(["me"], self.projects, self.user)
+        expected = Q(owner_user_id=self.user.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_none(self):
+        result = convert_assignee_values(["none"], self.projects, self.user)
+        expected = Q(owner_team_id__isnull=True, owner_user_id__isnull=True)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_multiple(self):
+        result = convert_assignee_values(
+            [str(self.user.email), f"#{self.team.slug}"], self.projects, self.user
+        )
+        expected = Q(owner_user_id=self.user.id) | Q(owner_team_id=self.team.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_mixed(self):
+        result = convert_assignee_values(
+            ["me", "none", f"#{self.team.slug}"], self.projects, self.user
+        )
+        expected = (
+            Q(owner_user_id=self.user.id)
+            | Q(owner_team_id__isnull=True, owner_user_id__isnull=True)
+            | Q(owner_team_id=self.team.id)
+        )
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_invalid(self):
+        result = convert_assignee_values(["999999"], self.projects, self.user)
+        expected = Q(owner_user_id=_HACKY_INVALID_USER.id)
+        self.assertEqual(str(result), str(expected))
+
+    def test_convert_assignee_values_empty(self):
+        result = convert_assignee_values([], self.projects, self.user)
+        expected = Q()
+        self.assertEqual(str(result), str(expected))
