fix(feedback): fix typing for shims from v2 to v1 (#89949)

Follow-up on #89685.

Fixes
[SENTRY-3SEX](https://sentry.sentry.io/issues/6551468609/events/9a88f59af64d4f86bddf42060cdff0d9/)
Fixes
[SENTRY-3SEZ](https://sentry.sentry.io/issues/6551532041/events/bd020dada1a74f1ba96faa07c33df30c/)
Fixes
[SENTRY-3SEY](https://sentry.sentry.io/issues/6551484362/events/42136852328a4acdb6f21922bd6425af/)

Only the message/comments field is required, so we need to handle
missing name or email. Also event timestamp is a ISO str.

Also adds better typing to `save_userreport` and cleans up the event
formatting code in `create_feedback` a bit. Functionality is the same,
except now we'll always include the `user.email` context/tag, defaulting
to empty str `""`

Author: aliu39

src/sentry/feedback/lib/types.py

@@ -0,0 +1,15 @@
+from typing import NotRequired, TypedDict
+
+
+class UserReportDict(TypedDict):
+    """Use for weak type checking of user report data. Keys correspond to fields of the UserReport model."""
+
+    event_id: str
+    comments: str
+    # required for the model, but functions usually infer this from an explicit Project argument.
+    project_id: NotRequired[int]
+    name: NotRequired[str]
+    email: NotRequired[str]
+    environment_id: NotRequired[int]  # defaults to "production".
+    group_id: NotRequired[int]
+    level: NotRequired[str]  # defaults to "info".

src/sentry/feedback/usecases/create_feedback.py

@@ -4,14 +4,15 @@
 import random
 from datetime import UTC, datetime
 from enum import Enum
-from typing import Any, TypedDict
+from typing import Any
 from uuid import uuid4
 
 import jsonschema
 
 from sentry import features, options
 from sentry.constants import DataCategory
 from sentry.eventstore.models import Event, GroupEvent
+from sentry.feedback.lib.types import UserReportDict
 from sentry.feedback.usecases.spam_detection import is_spam, spam_detection_enabled
 from sentry.issues.grouptype import FeedbackGroup
 from sentry.issues.issue_occurrence import IssueEvidence, IssueOccurrence
@@ -100,8 +101,10 @@ def fix_for_issue_platform(event_data: dict[str, Any]) -> dict[str, Any]:
     """
     The issue platform has slightly different requirements than ingest for event schema,
     so we need to massage the data a bit.
+    * event["timestamp"] is converted to a UTC ISO string.
     * event["tags"] is coerced to a dict.
-    * If event["user"]["email"] is missing we try to set using the feedback context.
+    * If user or replay context is missing we try to set it using the feedback context.
+    * level defaults to "info" and environment defaults to "production".
 
     Returns:
         A dict[str, Any] conforming to sentry.issues.json_schemas.EVENT_PAYLOAD_SCHEMA.
@@ -135,22 +138,19 @@ def fix_for_issue_platform(event_data: dict[str, Any]) -> dict[str, Any]:
     ret_event["request"] = event_data.get("request", {})
 
     ret_event["user"] = event_data.get("user", {})
+    if "name" in ret_event["user"]:
+        del ret_event["user"]["name"]
 
-    if event_data.get("dist") is not None:
-        del event_data["dist"]
-    if event_data.get("user", {}).get("name") is not None:
-        del event_data["user"]["name"]
-    if event_data.get("user", {}).get("isStaff") is not None:
-        del event_data["user"]["isStaff"]
+    if "isStaff" in ret_event["user"]:
+        del ret_event["user"]["isStaff"]
 
-    if event_data.get("user", {}).get("id") is not None:
-        event_data["user"]["id"] = str(event_data["user"]["id"])
+    if "id" in ret_event["user"]:
+        ret_event["user"]["id"] = str(ret_event["user"]["id"])
 
     # If no user email was provided specify the contact-email as the user-email.
     feedback_obj = event_data.get("contexts", {}).get("feedback", {})
-    contact_email = feedback_obj.get("contact_email")
-    if not ret_event["user"].get("email", ""):
-        ret_event["user"]["email"] = contact_email
+    if "email" not in ret_event["user"]:
+        ret_event["user"]["email"] = feedback_obj.get("contact_email", "")
 
     # Force `tags` to be a dict if it's initially a list,
     # since we can't guarantee its type here.
@@ -451,16 +451,8 @@ def auto_ignore_spam_feedbacks(project, issue_fingerprint):
 ###########
 
 
-class UserReportShimDict(TypedDict):
-    name: str
-    email: str
-    comments: str
-    event_id: str
-    level: str
-
-
 def shim_to_feedback(
-    report: UserReportShimDict,
+    report: UserReportDict,
     event: Event | GroupEvent,
     project: Project,
     source: FeedbackCreationSource,
@@ -491,7 +483,7 @@ def shim_to_feedback(
             "contexts": {
                 "feedback": {
                     "name": report.get("name", ""),
-                    "contact_email": report["email"],
+                    "contact_email": report.get("email", ""),
                     "message": report["comments"],
                 },
             },

src/sentry/feedback/usecases/save_feedback_event.py

@@ -32,8 +32,15 @@ def save_feedback_event(event_data: Mapping[str, Any], project_id: int):
         # Shim to UserReport
         feedback_context = fixed_event_data["contexts"]["feedback"]
         associated_event_id = feedback_context.get("associated_event_id")
+
         if associated_event_id:
             project = Project.objects.get_from_cache(id=project_id)
+            timestamp = fixed_event_data["timestamp"]
+            start_time = (
+                datetime.fromtimestamp(timestamp, tz=UTC)
+                if isinstance(timestamp, float)
+                else datetime.fromisoformat(timestamp)
+            )
             save_userreport(
                 project,
                 {
@@ -42,12 +49,12 @@ def save_feedback_event(event_data: Mapping[str, Any], project_id: int):
                     # 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"),
-                    "name": feedback_context["name"],
-                    "email": feedback_context["contact_email"],
+                    "name": feedback_context.get("name", ""),
+                    "email": feedback_context.get("contact_email", ""),
                     "comments": feedback_context["message"],
                 },
                 FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE,
-                start_time=datetime.fromtimestamp(fixed_event_data["timestamp"], UTC),
+                start_time=start_time,
             )
             metrics.incr("feedback.shim_to_userreport.success")
 

src/sentry/ingest/userreport.py

@@ -10,6 +10,7 @@
 from sentry import eventstore, options
 from sentry.constants import DataCategory
 from sentry.eventstore.models import Event, GroupEvent
+from sentry.feedback.lib.types import UserReportDict
 from sentry.feedback.usecases.create_feedback import (
     UNREAL_FEEDBACK_UNATTENDED_MESSAGE,
     FeedbackCreationSource,
@@ -34,7 +35,7 @@ class Conflict(Exception):
 
 def save_userreport(
     project: Project,
-    report,
+    report: UserReportDict,
     source: FeedbackCreationSource,
     start_time: datetime | None = None,
 ) -> UserReport | None:
@@ -100,7 +101,8 @@ def save_userreport(
                 raise Conflict("Feedback for this event cannot be modified.")
 
             report["environment_id"] = event.get_environment().id
-            report["group_id"] = event.group_id
+            if event.group_id:
+                report["group_id"] = event.group_id
 
         # Save the report.
         try:
@@ -126,7 +128,7 @@ def save_userreport(
 
             existing_report.update(
                 name=report.get("name", ""),
-                email=report["email"],
+                email=report.get("email", ""),
                 comments=report["comments"],
             )
             report_instance = existing_report

tests/sentry/api/endpoints/test_organization_user_reports.py

@@ -1,6 +1,7 @@
 from datetime import UTC, datetime, timedelta
 from unittest.mock import patch
 
+from sentry.feedback.lib.types import UserReportDict
 from sentry.feedback.usecases.create_feedback import FeedbackCreationSource
 from sentry.ingest.userreport import save_userreport
 from sentry.models.group import GroupStatus
@@ -132,11 +133,12 @@ def test_with_event_user(self):
         )
 
         # Simulate how ingest saves reports to get event_user connections
-        report_data = {
+        report_data: UserReportDict = {
             "event_id": event.event_id,
             "name": "",
             "email": "",
             "comments": "It broke",
+            "project_id": self.project_1.id,
         }
         save_userreport(
             self.project_1, report_data, FeedbackCreationSource.USER_REPORT_DJANGO_ENDPOINT

tests/sentry/feedback/usecases/test_save_feedback_event.py

@@ -37,11 +37,19 @@ def test_save_feedback_event_no_associated_error(
 
 
 @django_db_all
+@pytest.mark.parametrize(
+    "timestamp_format",
+    ["number", "iso"],
+)
 def test_save_feedback_event_with_associated_error(
-    default_project, mock_create_feedback_issue, mock_save_userreport
+    default_project, mock_create_feedback_issue, mock_save_userreport, timestamp_format
 ):
     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.timestamp() if timestamp_format == "number" else start_time.isoformat()
+    )
     mock_create_feedback_issue.return_value = event_data
 
     save_feedback_event(event_data, default_project.id)
@@ -62,5 +70,43 @@ def test_save_feedback_event_with_associated_error(
             "comments": feedback_context["message"],
         },
         FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE,
-        start_time=datetime.fromtimestamp(event_data["timestamp"], UTC),
+        start_time=start_time,
+    )
+
+
+@django_db_all
+def test_save_feedback_event_with_associated_error_and_missing_fields(
+    default_project, mock_create_feedback_issue, mock_save_userreport
+):
+    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()
+
+    # Remove name, email, environment
+    del event_data["contexts"]["feedback"]["name"]
+    del event_data["contexts"]["feedback"]["contact_email"]
+    del event_data["environment"]
+
+    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
+    )
+
+    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,
     )

tests/sentry/ingest/test_userreport.py

@@ -2,6 +2,7 @@
 
 from django.utils import timezone
 
+from sentry.feedback.lib.types import UserReportDict
 from sentry.feedback.usecases.create_feedback import (
     UNREAL_FEEDBACK_UNATTENDED_MESSAGE,
     FeedbackCreationSource,
@@ -73,7 +74,7 @@ def test_save_user_report_returns_instance(default_project, monkeypatch):
     )
 
     # Test data
-    report = {
+    report: UserReportDict = {
         "event_id": "123456",
         "name": "Test User",
         "email": "[email protected]",
@@ -91,7 +92,7 @@ def test_save_user_report_returns_instance(default_project, monkeypatch):
 @django_db_all
 def test_save_user_report_denylist(default_project, monkeypatch):
     monkeypatch.setattr("sentry.ingest.userreport.is_in_feedback_denylist", lambda org: True)
-    report = {
+    report: UserReportDict = {
         "event_id": "123456",
         "name": "Test User",
         "email": "[email protected]",
@@ -116,7 +117,7 @@ def test_save_user_report_filters_large_message(default_project, monkeypatch):
     if not max_length:
         assert False, "Missing max_length for UserReport comments field!"
 
-    report = {
+    report: UserReportDict = {
         "event_id": "123456",
         "name": "Test User",
         "email": "[email protected]",
@@ -145,7 +146,7 @@ def test_save_user_report_shims_if_event_found(default_project, monkeypatch):
     mock_shim_to_feedback = Mock()
     monkeypatch.setattr("sentry.ingest.userreport.shim_to_feedback", mock_shim_to_feedback)
 
-    report = {
+    report: UserReportDict = {
         "event_id": event.event_id,
         "name": "Test User",
         "email": "[email protected]",
@@ -176,7 +177,7 @@ def test_save_user_report_does_not_shim_if_event_found_but_source_is_new_feedbac
     mock_shim_to_feedback = Mock()
     monkeypatch.setattr("sentry.ingest.userreport.shim_to_feedback", mock_shim_to_feedback)
 
-    report = {
+    report: UserReportDict = {
         "event_id": event.event_id,
         "name": "Test User",
         "email": "[email protected]",