From bbe16d8e9ffa3216dceef8b55bb7eaf7949d6643 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 11 May 2026 21:00:58 -0700 Subject: [PATCH] :bug: fix[gitlab]: add assignee sync diagnostics --- src/sentry/integrations/gitlab/issue_sync.py | 106 +++++++++--------- src/sentry/integrations/gitlab/webhooks.py | 3 + .../tasks/sync_assignee_outbound.py | 6 +- src/sentry/integrations/utils/sync.py | 42 +++++-- .../integrations/gitlab/test_integration.py | 24 +++- .../integrations/gitlab/test_webhook.py | 25 +++++ .../tasks/test_sync_assignee_outbound.py | 6 +- tests/sentry/integrations/utils/test_sync.py | 12 ++ tests/sentry/models/test_groupassignee.py | 7 +- 9 files changed, 165 insertions(+), 66 deletions(-) diff --git a/src/sentry/integrations/gitlab/issue_sync.py b/src/sentry/integrations/gitlab/issue_sync.py index 2ffb5f14770737..6d77e1d4b9b525 100644 --- a/src/sentry/integrations/gitlab/issue_sync.py +++ b/src/sentry/integrations/gitlab/issue_sync.py @@ -20,6 +20,11 @@ logger = logging.getLogger("sentry.integrations.gitlab.issue_sync") +def _add_lifecycle_extras(lifecycle: Any, extras: Mapping[str, Any]) -> None: + if lifecycle is not None and hasattr(lifecycle, "add_extras"): + lifecycle.add_extras(extras) + + class GitlabIssueSyncSpec(IssueSyncIntegration): comment_key = "sync_comments" outbound_assignee_key = "sync_forward_assignment" @@ -70,19 +75,23 @@ def sync_assignee_outbound( If assign=True, we're assigning the issue. Otherwise, deassign. """ client = self.get_client() + lifecycle = kwargs.get("lifecycle") project_id, issue_id = self.split_external_issue_key(external_issue.key) + log_context: dict[str, Any] = { + "provider": self.model.provider, + "integration_id": external_issue.integration_id, + "external_issue_key": external_issue.key, + "external_issue_id": external_issue.id, + "project_path": project_id, + "issue_iid": issue_id, + "assign": assign, + "user_id": user.id if user else None, + } + _add_lifecycle_extras(lifecycle, log_context) if not project_id or not issue_id: - logger.error( - "assignee-outbound.invalid-key", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "external_issue_key": external_issue.key, - "external_issue_id": external_issue.id, - }, - ) + logger.warning("assignee-outbound.invalid-key", extra=log_context) return gitlab_user_id = None @@ -98,58 +107,53 @@ def sync_assignee_outbound( organization=external_issue.organization, ).first() if not external_actor: - logger.info( - "assignee-outbound.external-actor-not-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "user_id": user.id, - }, - ) + logger.info("assignee-outbound.external-actor-not-found", extra=log_context) return + log_context.update( + { + "external_actor_id": external_actor.id, + "external_actor_name": external_actor.external_name, + "external_actor_external_id": external_actor.external_id, + } + ) + _add_lifecycle_extras(lifecycle, log_context) + # Strip the @ from the username stored in external_name gitlab_username = external_actor.external_name.lstrip("@") # Search for the GitLab user by username to get their user ID try: - users = client.search_users(gitlab_username) - if not users or len(users) == 0: - logger.warning( - "assignee-outbound.gitlab-user-not-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - }, - ) - return - - # Take the first matching user (exact username match) - gitlab_user = users[0] - gitlab_user_id = gitlab_user["id"] - - logger.info( - "assignee-outbound.gitlab-user-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - "gitlab_user_id": gitlab_user_id, - }, - ) + users = client.search_users(gitlab_username) or [] except ApiError as e: - logger.warning( - "assignee-outbound.gitlab-user-search-failed", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - "error": str(e), - }, - ) + log_context["error"] = str(e) + logger.warning("assignee-outbound.gitlab-user-search-failed", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) return + usernames = [ + found_user.get("username") for found_user in users[:5] if found_user.get("username") + ] + log_context.update( + { + "assignee_resolution_method": "username_search", + "gitlab_search_result_count": len(users), + "gitlab_search_usernames": usernames, + } + ) + + if not users: + logger.warning("assignee-outbound.gitlab-user-not-found", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) + return + + gitlab_user = users[0] + gitlab_user_id = gitlab_user["id"] + log_context["resolved_gitlab_user_id"] = gitlab_user_id + + logger.info("assignee-outbound.gitlab-user-found", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) + # Update GitLab issue assignees try: # URL-encode project_id since it's a path_with_namespace (e.g., "owner/repo") diff --git a/src/sentry/integrations/gitlab/webhooks.py b/src/sentry/integrations/gitlab/webhooks.py index ef34e32f00b656..b37e2edae476b7 100644 --- a/src/sentry/integrations/gitlab/webhooks.py +++ b/src/sentry/integrations/gitlab/webhooks.py @@ -244,6 +244,7 @@ def _handle_assignment( # GitLab supports multiple assignees, but Sentry currently only supports one # Take the first assignee from the current state first_assignee = assignees[0] + assignee_id = first_assignee.get("id") assignee_username = first_assignee.get("username") if not assignee_username: @@ -264,6 +265,7 @@ def _handle_assignment( external_user_name=assignee_name, external_issue_key=external_issue_key, assign=True, + external_user_id=assignee_id, ) logger.info( @@ -271,6 +273,7 @@ def _handle_assignment( extra={ "integration_id": integration.id, "external_issue_key": external_issue_key, + "assignee_id": assignee_id, "assignee_name": assignee_name, "total_assignees": len(assignees), }, diff --git a/src/sentry/integrations/tasks/sync_assignee_outbound.py b/src/sentry/integrations/tasks/sync_assignee_outbound.py index 82154b256c5286..4b3765781b7b14 100644 --- a/src/sentry/integrations/tasks/sync_assignee_outbound.py +++ b/src/sentry/integrations/tasks/sync_assignee_outbound.py @@ -97,7 +97,11 @@ def sync_assignee_outbound( # Assume unassign if None. user = user_service.get_user(user_id) if user_id else None installation.sync_assignee_outbound( - external_issue, user, assign=assign, assignment_source=parsed_assignment_source + external_issue, + user, + assign=assign, + assignment_source=parsed_assignment_source, + lifecycle=lifecycle, ) analytics.record( IntegrationIssueAssigneeSyncedEvent( diff --git a/src/sentry/integrations/utils/sync.py b/src/sentry/integrations/utils/sync.py index 2855825f10125c..5bd44383026f62 100644 --- a/src/sentry/integrations/utils/sync.py +++ b/src/sentry/integrations/utils/sync.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +from collections.abc import Iterable from enum import StrEnum from typing import TYPE_CHECKING @@ -74,8 +75,9 @@ def _get_affected_groups( def _handle_deassign( - groups: QuerySet[Group], integration: RpcIntegration | Integration -) -> QuerySet[Group]: + groups: Iterable[Group], integration: RpcIntegration | Integration +) -> list[Group]: + groups_deassigned: list[Group] = [] for group in groups: if not should_sync_assignee_inbound(group.organization, integration.provider): continue @@ -84,11 +86,12 @@ def _handle_deassign( group, assignment_source=AssignmentSource.from_integration(integration), ) - return groups + groups_deassigned.append(group) + return groups_deassigned def _handle_assign( - affected_groups: QuerySet[Group], + affected_groups: Iterable[Group], integration: RpcIntegration | Integration, users: list[RpcUser], ) -> list[Group]: @@ -137,45 +140,64 @@ def sync_group_assignee_inbound_by_external_actor( external_user_name: str, external_issue_key: str | None, assign: bool = True, + external_user_id: str | int | None = None, ) -> QuerySet[Group] | list[Group]: logger = logging.getLogger(f"sentry.integrations.{integration.provider}") with ProjectManagementEvent( action_type=ProjectManagementActionType.INBOUND_ASSIGNMENT_SYNC, integration=integration ).capture() as lifecycle: - affected_groups = _get_affected_groups(integration, external_issue_key) + affected_groups = list(_get_affected_groups(integration, external_issue_key)) + external_user_id_str = str(external_user_id) if external_user_id is not None else None log_context = { "integration_id": integration.id, "external_user_name": external_user_name, + "external_user_id": external_user_id_str, "issue_key": external_issue_key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": assign, + "affected_group_ids": [group.id for group in affected_groups], } + lifecycle.add_extras(log_context) if not affected_groups: logger.info("no-affected-groups", extra=log_context) return [] if not assign: - return _handle_deassign(affected_groups, integration) + groups_deassigned = _handle_deassign(affected_groups, integration) + log_context["unassigned_group_ids"] = [group.id for group in groups_deassigned] + lifecycle.add_extras(log_context) + return groups_deassigned - external_actors = ExternalActor.objects.filter( + base_external_actors = ExternalActor.objects.filter( provider=EXTERNAL_PROVIDERS_REVERSE[ExternalProviderEnum(integration.provider)].value, - external_name__iexact=external_user_name, integration_id=integration.id, user_id__isnull=False, - ).values_list("user_id", flat=True) + ) + + match_method = "external_name" + external_actors = base_external_actors.filter(external_name__iexact=external_user_name) + external_actor_user_ids = list(external_actors.values_list("user_id", flat=True)) user_ids = [ - external_actor for external_actor in external_actors if external_actor is not None + external_actor_user_id + for external_actor_user_id in external_actor_user_ids + if external_actor_user_id is not None ] + log_context["match_method"] = match_method + log_context["external_actor_count"] = len(user_ids) + log_context["matched_user_ids"] = user_ids log_context["user_ids"] = user_ids logger.info("sync_group_assignee_inbound_by_external_actor.user_ids", extra=log_context) + lifecycle.add_extras(log_context) users = user_service.get_many_by_id(ids=user_ids) groups_assigned = _handle_assign(affected_groups, integration, users) + log_context["assigned_group_ids"] = [group.id for group in groups_assigned] + lifecycle.add_extras(log_context) if len(groups_assigned) != len(affected_groups): log_context["groups_assigned_count"] = len(groups_assigned) diff --git a/tests/sentry/integrations/gitlab/test_integration.py b/tests/sentry/integrations/gitlab/test_integration.py index 528d9c0daea850..c457661199c560 100644 --- a/tests/sentry/integrations/gitlab/test_integration.py +++ b/tests/sentry/integrations/gitlab/test_integration.py @@ -178,7 +178,7 @@ def _setup_assignee_sync_test( self, user_email: str = "foo@example.com", external_name: str = "@gitlab_user", - external_id: str = "123", + external_id: str | None = None, issue_key: str = "example.gitlab.com/group-x:cool-group/sentry#45", create_external_user: bool = True, ) -> tuple: @@ -402,7 +402,7 @@ def test_sync_assignee_outbound_unassign(self) -> None: @responses.activate @with_feature("organizations:integrations-gitlab-project-management") def test_sync_assignee_outbound_no_external_actor(self) -> None: - """Test that sync fails gracefully when user has no GitLab account linked""" + """Test that sync no-ops when user has no GitLab account linked.""" user, installation, external_issue, _, _ = self._setup_assignee_sync_test( create_external_user=False ) @@ -493,6 +493,26 @@ def test_sync_assignee_outbound_user_not_found(self) -> None: assert len(responses.calls) == 1 assert "users?username=gitlab_user" in responses.calls[0].request.url + @responses.activate + @with_feature("organizations:integrations-gitlab-project-management") + def test_sync_assignee_outbound_user_search_failed(self) -> None: + user, installation, external_issue, _, _ = self._setup_assignee_sync_test() + + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/users?username=gitlab_user", + json={"message": "Internal server error"}, + status=500, + ) + + responses.calls.reset() + + with assume_test_silo_mode(SiloMode.CELL): + installation.sync_assignee_outbound(external_issue, user, assign=True) + + assert len(responses.calls) == 1 + assert "users?username=gitlab_user" in responses.calls[0].request.url + def test_create_comment(self) -> None: """Test creating a comment on a GitLab issue""" self.user.name = "Sentry Admin" diff --git a/tests/sentry/integrations/gitlab/test_webhook.py b/tests/sentry/integrations/gitlab/test_webhook.py index a8a28dc587570d..3eca27c07ae0b9 100644 --- a/tests/sentry/integrations/gitlab/test_webhook.py +++ b/tests/sentry/integrations/gitlab/test_webhook.py @@ -528,6 +528,31 @@ def test_issue_assigned(self, mock_sync: MagicMock) -> None: call_args[1]["external_issue_key"] == "example.gitlab.com/group-x:cool-group/sentry#23" ) assert call_args[1]["assign"] is True + assert call_args[1]["external_user_id"] == 1 + + @patch("sentry.integrations.gitlab.webhooks.sync_group_assignee_inbound_by_external_actor") + def test_issue_assigned_with_dotted_username(self, mock_sync: MagicMock) -> None: + event = orjson.loads(ISSUE_ASSIGNED_EVENT) + event["assignees"][0]["id"] = 123 + event["assignees"][0]["username"] = "first.last" + + response = self.client.post( + self.url, + data=orjson.dumps(event), + content_type="application/json", + HTTP_X_GITLAB_TOKEN=WEBHOOK_TOKEN, + HTTP_X_GITLAB_EVENT="Issue Hook", + ) + assert response.status_code == 204 + + assert mock_sync.called + call_args = mock_sync.call_args + assert call_args[1]["external_user_name"] == "@first.last" + assert call_args[1]["external_user_id"] == 123 + assert ( + call_args[1]["external_issue_key"] == "example.gitlab.com/group-x:cool-group/sentry#23" + ) + assert call_args[1]["assign"] is True @patch("sentry.integrations.gitlab.webhooks.sync_group_assignee_inbound_by_external_actor") def test_issue_unassigned(self, mock_sync: MagicMock) -> None: diff --git a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py index 31b216590d3d86..04f477adf32f60 100644 --- a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py +++ b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py @@ -55,7 +55,11 @@ def test_syncs_outbound_assignee( sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_called_once() mock_sync_assignee.assert_called_with( - self.external_issue, mock.ANY, assign=True, assignment_source=None + self.external_issue, + mock.ANY, + assign=True, + assignment_source=None, + lifecycle=mock.ANY, ) user_arg = mock_sync_assignee.call_args_list[0][0][1] diff --git a/tests/sentry/integrations/utils/test_sync.py b/tests/sentry/integrations/utils/test_sync.py index d8af02f4908d04..149906cec38230 100644 --- a/tests/sentry/integrations/utils/test_sync.py +++ b/tests/sentry/integrations/utils/test_sync.py @@ -465,10 +465,16 @@ def test_assign_with_no_external_actor_found(self, mock_record_halt: mock.MagicM extra={ "integration_id": self.example_integration.id, "external_user_name": "unknownuser", + "external_user_id": None, "issue_key": external_issue.key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": True, + "affected_group_ids": [self.group.id], + "match_method": "external_name", + "external_actor_count": 0, + "matched_user_ids": [], "user_ids": [], + "assigned_group_ids": [], "groups_assigned_count": 0, "affected_groups_count": 1, }, @@ -510,10 +516,16 @@ def test_assign_with_user_not_in_project(self, mock_record_halt: mock.MagicMock) extra={ "integration_id": self.example_integration.id, "external_user_name": "outsider", + "external_user_id": None, "issue_key": external_issue.key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": True, + "affected_group_ids": [self.group.id], + "match_method": "external_name", + "external_actor_count": 1, + "matched_user_ids": [other_user.id], "user_ids": [other_user.id], + "assigned_group_ids": [], "groups_assigned_count": 0, "affected_groups_count": 1, }, diff --git a/tests/sentry/models/test_groupassignee.py b/tests/sentry/models/test_groupassignee.py index 4d3f2a07240098..08abd9d9bbee45 100644 --- a/tests/sentry/models/test_groupassignee.py +++ b/tests/sentry/models/test_groupassignee.py @@ -169,6 +169,7 @@ def test_assignee_sync_outbound_assign( user_service.get_user(self.user.id), assign=True, assignment_source=None, + lifecycle=mock.ANY, ) assert GroupAssignee.objects.filter( @@ -286,7 +287,11 @@ def test_assignee_sync_outbound_unassign( with self.tasks(): GroupAssignee.objects.deassign(self.group, self.user) mock_sync_assignee_outbound.assert_called_with( - external_issue, None, assign=False, assignment_source=None + external_issue, + None, + assign=False, + assignment_source=None, + lifecycle=mock.ANY, ) assert not GroupAssignee.objects.filter(