Browse Source

feat(bug reports): add env filtering (#56980)

Michelle Zhang 1 year ago
parent
commit
26f46b9699

+ 4 - 0
fixtures/backup/model_dependencies/detailed.json

@@ -1,6 +1,10 @@
 {
   "feedback.feedback": {
     "foreign_keys": {
+      "environment": {
+        "kind": "FlexibleForeignKey",
+        "model": "sentry.environment"
+      },
       "organization_id": {
         "kind": "ImplicitForeignKey",
         "model": "sentry.organization"

+ 1 - 0
fixtures/backup/model_dependencies/flat.json

@@ -1,5 +1,6 @@
 {
   "feedback.feedback": [
+    "sentry.environment",
     "sentry.organization",
     "sentry.project"
   ],

+ 2 - 2
fixtures/backup/model_dependencies/sorted.json

@@ -58,7 +58,6 @@
   "nodestore.node",
   "replays.replayrecordingsegment",
   "social_auth.usersocialauth",
-  "feedback.feedback",
   "hybridcloud.organizationslugreservationreplica",
   "sentry.discoversavedqueryproject",
   "sentry.processingissue",
@@ -195,6 +194,7 @@
   "sentry.teamkeytransaction",
   "sentry.monitorenvironment",
   "sentry.releasethreshold",
+  "feedback.feedback",
   "hybridcloud.apikeyreplica",
   "sentry.monitorcheckin",
   "sentry.alertruleexcludedprojects",
@@ -217,4 +217,4 @@
   "sentry.incidentsubscription",
   "sentry.incidenttrigger",
   "sentry.monitorincident"
-]
+]

+ 1 - 1
migrations_lockfile.txt

@@ -5,7 +5,7 @@ ahead of you.
 To resolve this, rebase against latest master and regenerate your migration. This file
 will then be regenerated, and you should be able to merge without conflicts.
 
-feedback: 0002_feedback_add_org_id_and_rename_event_id
+feedback: 0003_feedback_add_env
 nodestore: 0002_nodestore_no_dictfield
 replays: 0003_add_size_to_recording_segment
 sentry: 0571_add_hybrid_cloud_foreign_key_to_slug_reservation

+ 13 - 3
src/sentry/feedback/endpoints/feedback_ingest.py

@@ -22,7 +22,7 @@ from sentry.api.bases.project import ProjectPermission
 from sentry.api.exceptions import ResourceDoesNotExist
 from sentry.constants import ObjectStatus
 from sentry.feedback.models import Feedback
-from sentry.models import Organization, ProjectKey
+from sentry.models import Environment, Organization, ProjectKey
 from sentry.models.project import Project
 from sentry.utils.sdk import bind_organization_context, configure_scope
 
@@ -36,7 +36,7 @@ class FeedbackValidator(serializers.Serializer):
 
     # optional fields
     release = serializers.CharField(required=False)
-    environment = serializers.CharField(required=False)
+    environment = serializers.CharField(required=False, allow_null=True, default="production")
     dist = serializers.CharField(required=False)
     event_id = serializers.CharField(required=False)
     request = serializers.JSONField(required=False)
@@ -46,6 +46,11 @@ class FeedbackValidator(serializers.Serializer):
     BrowserContext = serializers.JSONField(required=False)
     DeviceContext = serializers.JSONField(required=False)
 
+    def validate_environment(self, value):
+        if not Environment.is_valid_name(value):
+            raise serializers.ValidationError("Invalid value for environment")
+        return value
+
     def validate(self, data):
         try:
             ret: Dict[str, Any] = {}
@@ -54,7 +59,6 @@ class FeedbackValidator(serializers.Serializer):
                 "platform": data["platform"],
                 "sdk": data["sdk"],
                 "release": data.get("release"),
-                "environment": data.get("environment"),
                 "request": data.get("request"),
                 "user": data.get("user"),
                 "tags": data.get("tags"),
@@ -70,6 +74,7 @@ class FeedbackValidator(serializers.Serializer):
             ret["replay_id"] = data["feedback"].get("replay_id")
             ret["project_id"] = self.context["project"].id
             ret["organization_id"] = self.context["organization"].id
+            ret["environment"] = data.get("environment")
             return ret
         except KeyError:
             raise serializers.ValidationError("Input has wrong field name or type")
@@ -157,5 +162,10 @@ class FeedbackIngestEndpoint(Endpoint):
             return self.respond(feedback_validator.errors, status=400)
 
         result = feedback_validator.validated_data
+
+        env = Environment.objects.get_or_create(
+            name=result["environment"], organization_id=organization.id
+        )[0]
+        result["environment"] = env
         Feedback.objects.create(**result)
         return self.respond(status=201)

+ 5 - 0
src/sentry/feedback/endpoints/organization_feedback_index.py

@@ -41,6 +41,11 @@ class OrganizationFeedbackIndexEndpoint(OrganizationEndpoint):
                 date_added__range=(filter_params["start"], filter_params["end"])
             )
 
+        if "environment" in filter_params:
+            feedback_list = feedback_list.filter(
+                environment__in=[env.id for env in filter_params["environment_objects"]]
+            )
+
         return self.paginate(
             request=request,
             queryset=feedback_list,

+ 34 - 0
src/sentry/feedback/migrations/0003_feedback_add_env.py

@@ -0,0 +1,34 @@
+# Generated by Django 3.2.20 on 2023-09-26 21:34
+from django.db import migrations
+
+import sentry.db.models.fields.foreignkey
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0571_add_hybrid_cloud_foreign_key_to_slug_reservation"),
+        ("feedback", "0002_feedback_add_org_id_and_rename_event_id"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="feedback",
+            name="environment",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                null=True, to="sentry.Environment"
+            ),
+        ),
+    ]

+ 2 - 0
src/sentry/feedback/models.py

@@ -4,6 +4,7 @@ from django.utils import timezone
 from sentry.backup.scopes import RelocationScope
 from sentry.db.models import BoundedBigIntegerField, Model, region_silo_only_model, sane_repr
 from sentry.db.models.fields import UUIDField
+from sentry.db.models.fields.foreignkey import FlexibleForeignKey
 
 
 @region_silo_only_model
@@ -17,6 +18,7 @@ class Feedback(Model):
     feedback_id = UUIDField(unique=True)
     date_added = models.DateTimeField(default=timezone.now)
     organization_id = BoundedBigIntegerField(db_index=True)
+    environment = FlexibleForeignKey("sentry.Environment", null=True)
 
     # This "data" field is the data coming from the Sentry event and includes things like contexts
     # As we develop the product more, we will add more specific columns and rely on this JSON field less and less

+ 1 - 1
src/sentry/feedback/serializers.py

@@ -40,7 +40,7 @@ class FeedbackSerializer(Serializer):
             "dist": obj.data.get("dist"),
             "sdk": obj.data.get("sdk"),
             "contact_email": obj.data.get("feedback").get("contact_email"),
-            "environment": obj.data.get("environment"),
+            "environment": obj.environment.name,
             "feedback_id": obj.feedback_id,
             "message": obj.message,
             "platform": obj.data.get("platform"),

+ 26 - 1
tests/sentry/feedback/test_feedback_ingest.py

@@ -55,7 +55,7 @@ class FeedbackIngestTest(MonitorIngestTestCase):
             # Feedback object is what was posted
             feedback = feedback_list[0]
             assert feedback.data["dist"] == "abc123"
-            assert feedback.data["environment"] == "production"
+            assert feedback.environment.name == "production"
             assert feedback.data["sdk"]["name"] == "sentry.javascript.react"
             assert feedback.data["feedback"]["contact_email"] == "colton.allen@sentry.io"
             assert (
@@ -194,3 +194,28 @@ class FeedbackIngestTest(MonitorIngestTestCase):
                 **self.dsn_auth_headers,
             )
             assert response.status_code == 201, response.content
+
+    def test_env(self):
+        # No environment name in input should default the field to "production"
+        test_data_missing_optional_fields = {
+            "feedback": {
+                "contact_email": "colton.allen@sentry.io",
+                "message": "I really like this user-feedback feature!",
+                "url": "https://docs.sentry.io/platforms/javascript/",
+            },
+            "platform": "javascript",
+            "sdk": {"name": "sentry.javascript.react", "version": "6.18.1"},
+            "timestamp": 1234456,
+        }
+
+        with self.feature({"organizations:user-feedback-ingest": True}):
+            path = reverse(self.endpoint)
+            response = self.client.post(
+                path,
+                data=test_data_missing_optional_fields,
+                **self.dsn_auth_headers,
+            )
+            assert response.status_code == 201, response.content
+            feedback_list = Feedback.objects.all()
+            feedback = feedback_list[0]
+            assert feedback.environment.name == "production"

Some files were not shown because too many files changed in this diff