From cc799966252983067898711973b1439e38e03e1e Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Mon, 21 Apr 2025 09:44:26 -0700 Subject: [PATCH] fix(feedback): fix environment id for shim to userreport --- .../feedback/usecases/save_feedback_event.py | 7 +- .../usecases/test_save_feedback_event.py | 122 ++++++++++++------ 2 files changed, 85 insertions(+), 44 deletions(-) diff --git a/src/sentry/feedback/usecases/save_feedback_event.py b/src/sentry/feedback/usecases/save_feedback_event.py index 281bc7f051aeec..7cd86ddae9b263 100644 --- a/src/sentry/feedback/usecases/save_feedback_event.py +++ b/src/sentry/feedback/usecases/save_feedback_event.py @@ -5,6 +5,7 @@ from sentry.feedback.usecases.create_feedback import FeedbackCreationSource, create_feedback_issue from sentry.ingest.userreport import save_userreport +from sentry.models.environment import Environment from sentry.models.project import Project from sentry.utils import metrics @@ -35,6 +36,10 @@ def save_feedback_event(event_data: Mapping[str, Any], project_id: int): if associated_event_id: project = Project.objects.get_from_cache(id=project_id) + environment = Environment.objects.get( + organization_id=project.organization_id, + name=fixed_event_data.get("environment", "production"), + ) timestamp = fixed_event_data["timestamp"] start_time = ( datetime.fromtimestamp(timestamp, tz=UTC) @@ -48,7 +53,7 @@ def save_feedback_event(event_data: Mapping[str, Any], project_id: int): "project_id": project_id, # XXX(aliu): including environment ensures the update_user_reports task # will not shim the report back to feedback. - "environment_id": fixed_event_data.get("environment", "production"), + "environment_id": environment.id, "name": feedback_context.get("name", ""), "email": feedback_context.get("contact_email", ""), "comments": feedback_context["message"], diff --git a/tests/sentry/feedback/usecases/test_save_feedback_event.py b/tests/sentry/feedback/usecases/test_save_feedback_event.py index 83698fcc98ac7e..e010635762fa5b 100644 --- a/tests/sentry/feedback/usecases/test_save_feedback_event.py +++ b/tests/sentry/feedback/usecases/test_save_feedback_event.py @@ -3,8 +3,12 @@ import pytest +from sentry.eventstore.models import Event from sentry.feedback.usecases.create_feedback import FeedbackCreationSource from sentry.feedback.usecases.save_feedback_event import save_feedback_event +from sentry.models.environment import Environment +from sentry.models.userreport import UserReport +from sentry.testutils.factories import Factories from sentry.testutils.pytest.fixtures import django_db_all from tests.sentry.feedback.usecases.test_create_feedback import mock_feedback_event @@ -15,16 +19,18 @@ def mock_create_feedback_issue(): yield m -@pytest.fixture -def mock_save_userreport(): - with mock.patch("sentry.feedback.usecases.save_feedback_event.save_userreport") as m: - yield m +def create_test_event(project_id: int, environment: Environment) -> Event: + return Factories.store_event( + { + "event_id": "ff1c2e3d4b5a6978899abbccddeeff00", + "environment": environment.name, + }, + project_id, + ) @django_db_all -def test_save_feedback_event_no_associated_error( - default_project, mock_create_feedback_issue, mock_save_userreport -): +def test_save_feedback_event_no_associated_event(default_project, mock_create_feedback_issue): event_data = mock_feedback_event(default_project.id) mock_create_feedback_issue.return_value = None @@ -33,7 +39,7 @@ def test_save_feedback_event_no_associated_error( mock_create_feedback_issue.assert_called_once_with( event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE ) - mock_save_userreport.assert_not_called() + assert UserReport.objects.count() == 0 @django_db_all @@ -41,15 +47,20 @@ def test_save_feedback_event_no_associated_error( "timestamp_format", ["number", "iso"], ) -def test_save_feedback_event_with_associated_error( - default_project, mock_create_feedback_issue, mock_save_userreport, timestamp_format +def test_save_feedback_event_with_associated_event( + default_project, mock_create_feedback_issue, timestamp_format ): + environment = Factories.create_environment(default_project, name="production") + assoc_event = create_test_event(default_project.id, environment) + event_data = mock_feedback_event(default_project.id) - event_data["contexts"]["feedback"]["associated_event_id"] = "abcd" * 8 - start_time = datetime.now(UTC) + event_data["contexts"]["feedback"]["associated_event_id"] = assoc_event.event_id event_data["timestamp"] = ( - start_time.timestamp() if timestamp_format == "number" else start_time.isoformat() + datetime.now(UTC).timestamp() + if timestamp_format == "number" + else datetime.now(UTC).isoformat() ) + event_data["environment"] = "production" mock_create_feedback_issue.return_value = event_data save_feedback_event(event_data, default_project.id) @@ -58,30 +69,59 @@ def test_save_feedback_event_with_associated_error( event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE ) + assert UserReport.objects.count() == 1 + report = UserReport.objects.get() + feedback_context = event_data["contexts"]["feedback"] - mock_save_userreport.assert_called_once_with( - default_project, - { - "event_id": "abcd" * 8, - "project_id": default_project.id, - "environment_id": event_data["environment"], - "name": feedback_context["name"], - "email": feedback_context["contact_email"], - "comments": feedback_context["message"], - }, - FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE, - start_time=start_time, - ) + assert report.event_id == assoc_event.event_id + assert report.project_id == default_project.id + assert report.group_id == assoc_event.group_id + assert report.environment_id == environment.id + assert report.name == feedback_context["name"] + assert report.email == feedback_context["contact_email"] + assert report.comments == feedback_context["message"] @django_db_all -def test_save_feedback_event_with_associated_error_and_missing_fields( - default_project, mock_create_feedback_issue, mock_save_userreport +def test_save_feedback_event_with_unprocessed_associated_event( + default_project, + mock_create_feedback_issue, ): + environment = Factories.create_environment(default_project, name="production") + event_data = mock_feedback_event(default_project.id) event_data["contexts"]["feedback"]["associated_event_id"] = "abcd" * 8 - start_time = datetime.now(UTC) - event_data["timestamp"] = start_time.isoformat() + event_data["timestamp"] = datetime.now(UTC).isoformat() + event_data["environment"] = "production" + mock_create_feedback_issue.return_value = event_data + + save_feedback_event(event_data, default_project.id) + + mock_create_feedback_issue.assert_called_once_with( + event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE + ) + + # UserReport is still created, with null group_id. + assert UserReport.objects.count() == 1 + report = UserReport.objects.get() + + feedback_context = event_data["contexts"]["feedback"] + assert report.event_id == "abcd" * 8 + assert report.project_id == default_project.id + assert report.group_id is None + assert report.environment_id == environment.id + assert report.name == feedback_context["name"] + assert report.email == feedback_context["contact_email"] + assert report.comments == feedback_context["message"] + + +@django_db_all +def test_save_feedback_event_with_missing_fields(default_project, mock_create_feedback_issue): + environment = Factories.create_environment(default_project, name="production") + + event_data = mock_feedback_event(default_project.id) + event_data["contexts"]["feedback"]["associated_event_id"] = "abcd" * 8 + event_data["timestamp"] = datetime.now(UTC).isoformat() # Remove name, email, environment del event_data["contexts"]["feedback"]["name"] @@ -96,17 +136,13 @@ def test_save_feedback_event_with_associated_error_and_missing_fields( event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE ) + assert UserReport.objects.count() == 1 + report = UserReport.objects.get() + feedback_context = event_data["contexts"]["feedback"] - mock_save_userreport.assert_called_once_with( - default_project, - { - "event_id": "abcd" * 8, - "project_id": default_project.id, - "environment_id": "production", - "name": "", - "email": "", - "comments": feedback_context["message"], - }, - FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE, - start_time=start_time, - ) + assert report.event_id == "abcd" * 8 + assert report.project_id == default_project.id + assert report.environment_id == environment.id + assert report.name == "" + assert report.email == "" + assert report.comments == feedback_context["message"]