From aa448c1e66a6f1bc9513fe539bbcc4f13f5afc2d Mon Sep 17 00:00:00 2001 From: Tillman Elser Date: Mon, 12 May 2025 19:36:11 -0700 Subject: [PATCH] =?UTF-8?q?Revert=20"fix(issue=20summary):=20Remove=20org?= =?UTF-8?q?=20acknowledgement=20check=20for=20issue=20summar=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit e655f5635ffc1ec682a0850537b148ea59f062ed. --- src/sentry/seer/issue_summary.py | 4 ++ tests/sentry/seer/test_issue_summary.py | 60 ++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/sentry/seer/issue_summary.py b/src/sentry/seer/issue_summary.py index 4b4dc5af55f65f..d42cb4c9f9c0f3 100644 --- a/src/sentry/seer/issue_summary.py +++ b/src/sentry/seer/issue_summary.py @@ -21,6 +21,7 @@ from sentry.models.project import Project from sentry.seer.autofix import trigger_autofix from sentry.seer.models import SummarizeIssueResponse +from sentry.seer.seer_setup import get_seer_org_acknowledgement from sentry.seer.signed_seer_api import sign_with_seer_secret from sentry.tasks.base import instrumented_task from sentry.taskworker.config import TaskworkerConfig @@ -356,6 +357,9 @@ def get_issue_summary( if not features.has("organizations:gen-ai-features", group.organization, actor=user): return {"detail": "Feature flag not enabled"}, 400 + if not get_seer_org_acknowledgement(group.organization.id): + return {"detail": "AI Autofix has not been acknowledged by the organization."}, 403 + cache_key = f"ai-group-summary-v2:{group.id}" lock_key = f"ai-group-summary-v2-lock:{group.id}" lock_duration = 10 # How long the lock is held if acquired (seconds) diff --git a/tests/sentry/seer/test_issue_summary.py b/tests/sentry/seer/test_issue_summary.py index f5fbc6a35fa6b8..3a1a9b13484d7e 100644 --- a/tests/sentry/seer/test_issue_summary.py +++ b/tests/sentry/seer/test_issue_summary.py @@ -33,8 +33,12 @@ def tearDown(self): # Clear the cache after each test cache.delete(f"ai-group-summary-v2:{self.group.id}") + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") @patch("sentry.seer.issue_summary._call_seer") - def test_get_issue_summary_with_existing_summary(self, mock_call_seer): + def test_get_issue_summary_with_existing_summary( + self, mock_call_seer, mock_get_acknowledgement + ): + mock_get_acknowledgement.return_value = True existing_summary = { "group_id": str(self.group.id), "headline": "Existing headline", @@ -57,9 +61,12 @@ def test_get_issue_summary_with_existing_summary(self, mock_call_seer): assert status_code == 200 assert summary_data == convert_dict_key_case(existing_summary, snake_to_camel_case) mock_call_seer.assert_not_called() + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") @patch("sentry.seer.issue_summary._get_event") - def test_get_issue_summary_without_event(self, mock_get_event): + def test_get_issue_summary_without_event(self, mock_get_event, mock_get_acknowledgement): + mock_get_acknowledgement.return_value = True mock_get_event.return_value = [None, None] summary_data, status_code = get_issue_summary(self.group, self.user) @@ -67,13 +74,16 @@ def test_get_issue_summary_without_event(self, mock_get_event): assert status_code == 400 assert summary_data == {"detail": "Could not find an event for the issue"} assert cache.get(f"ai-group-summary-v2:{self.group.id}") is None + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") @patch("sentry.seer.issue_summary._get_trace_connected_issues") @patch("sentry.seer.issue_summary._call_seer") @patch("sentry.seer.issue_summary._get_event") def test_get_issue_summary_without_existing_summary( - self, mock_get_event, mock_call_seer, mock_get_connected_issues + self, mock_get_event, mock_call_seer, mock_get_connected_issues, mock_get_acknowledgement ): + mock_get_acknowledgement.return_value = True event = Mock( event_id="test_event_id", data="test_event_data", @@ -111,14 +121,31 @@ def test_get_issue_summary_without_existing_summary( [self.group, self.group], [serialized_event, serialized_event], ) + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) # Check if the cache was set correctly cached_summary = cache.get(f"ai-group-summary-v2:{self.group.id}") assert cached_summary == expected_response_summary + def test_get_issue_summary_without_ai_acknowledgement(self): + with patch( + "sentry.seer.issue_summary.get_seer_org_acknowledgement" + ) as mock_get_acknowledgement: + mock_get_acknowledgement.return_value = False + + summary_data, status_code = get_issue_summary(self.group, self.user) + + assert status_code == 403 + assert summary_data == { + "detail": "AI Autofix has not been acknowledged by the organization." + } + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) + @patch("sentry.seer.issue_summary.requests.post") @patch("sentry.seer.issue_summary._get_event") - def test_call_seer_integration(self, mock_get_event, mock_post): + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") + def test_call_seer_integration(self, mock_get_acknowledgement, mock_get_event, mock_post): + mock_get_acknowledgement.return_value = True event = Mock( event_id="test_event_id", data="test_event_data", @@ -152,11 +179,16 @@ def test_call_seer_integration(self, mock_get_event, mock_post): assert status_code == 200 assert summary_data == convert_dict_key_case(expected_response_summary, snake_to_camel_case) mock_post.assert_called_once() + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) assert cache.get(f"ai-group-summary-v2:{self.group.id}") == expected_response_summary @patch("sentry.seer.issue_summary.get_issue_summary") - def test_get_issue_summary_cache_write_read(self, mock_get_issue_summary): + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") + def test_get_issue_summary_cache_write_read( + self, mock_get_acknowledgement, mock_get_issue_summary + ): + mock_get_acknowledgement.return_value = True # First request to populate the cache mock_get_event = Mock() mock_call_seer = Mock() @@ -204,10 +236,15 @@ def test_get_issue_summary_cache_write_read(self, mock_get_issue_summary): # Verify that _get_event and _call_seer were not called due to cache hit mock_get_event.assert_not_called() mock_call_seer.assert_not_called() + mock_get_acknowledgement.assert_called_with(self.group.organization.id) @patch("sentry.seer.issue_summary._generate_summary") - def test_get_issue_summary_concurrent_wait_for_lock(self, mock_generate_summary): + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") + def test_get_issue_summary_concurrent_wait_for_lock( + self, mock_get_acknowledgement, mock_generate_summary + ): """Test that a second request waits for the lock and reads from cache.""" + mock_get_acknowledgement.return_value = True cache_key = f"ai-group-summary-v2:{self.group.id}" # Mock summary generation to take time and cache the result @@ -268,8 +305,12 @@ def target(req_id): assert cache.get(cache_key) == generated_summary @patch("sentry.seer.issue_summary._generate_summary") - def test_get_issue_summary_concurrent_force_event_id_bypasses_lock(self, mock_generate_summary): + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") + def test_get_issue_summary_concurrent_force_event_id_bypasses_lock( + self, mock_get_acknowledgement, mock_generate_summary + ): """Test that force_event_id bypasses lock waiting.""" + mock_get_acknowledgement.return_value = True # Mock summary generation forced_summary = {"headline": "Forced Summary", "event_id": "force_event"} mock_generate_summary.return_value = (forced_summary, 200) @@ -291,17 +332,21 @@ def test_get_issue_summary_concurrent_force_event_id_bypasses_lock(self, mock_ge # Ensure generation was called directly mock_generate_summary.assert_called_once() + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) @patch("sentry.seer.issue_summary.cache.get") @patch("sentry.seer.issue_summary._generate_summary") @patch("sentry.utils.locking.lock.Lock.blocking_acquire") + @patch("sentry.seer.issue_summary.get_seer_org_acknowledgement") def test_get_issue_summary_lock_timeout( self, + mock_get_acknowledgement, mock_blocking_acquire, mock_generate_summary_core, mock_cache_get, ): """Test that a timeout waiting for the lock returns 503.""" + mock_get_acknowledgement.return_value = True # Simulate lock acquisition always failing with the specific exception mock_blocking_acquire.side_effect = UnableToAcquireLock # Simulate cache miss even after timeout @@ -317,6 +362,7 @@ def test_get_issue_summary_lock_timeout( mock_generate_summary_core.assert_not_called() # Ensure cache was checked twice (once initially, once after lock failure) assert mock_cache_get.call_count == 2 + mock_get_acknowledgement.assert_called_once_with(self.group.organization.id) @patch("sentry.seer.issue_summary.Project.objects.filter") @patch("sentry.seer.issue_summary.eventstore.backend.get_events")