fix(severity): Prevent skipping severity score when equal to zero (#56225)

This fixes a potential bug in assigning severity scores to new groups wherein the score wouldn't be assigned if it was equal to `0`. (Falsiness strikes again!) Since scores are currently handled as strings, this is only a hypothetical for now, but worth fixing in case we do ever treat score as a float instead.

It also adds tests for this case and applies a fix I noticed while writing said tests to an existing test. (It now checks for the `severity` key actually being _missing_ from group metadata rather than `severity` being set to `None`.)

Author: lobsterkatie

src/sentry/event_manager.py

@@ -1624,9 +1624,10 @@ def _save_aggregate(
 
                 group = _create_group(project, event, **kwargs)
 
-                if features.has(
-                    "projects:first-event-severity-calculation", event.project
-                ) and not group.data.get("metadata", {}).get("severity"):
+                if (
+                    features.has("projects:first-event-severity-calculation", event.project)
+                    and group.data.get("metadata", {}).get("severity") is None
+                ):
                     logger.error(
                         "Group created without severity score",
                         extra={
@@ -1843,7 +1844,7 @@ def _create_group(project: Project, event: Event, **kwargs: Any) -> Group:
     group_data = kwargs.pop("data", {})
     if features.has("projects:first-event-severity-calculation", event.project):
         severity = _get_severity_score(event)
-        if severity:
+        if severity is not None:  # Severity can be 0
             group_data.setdefault("metadata", {})
             group_data["metadata"]["severity"] = severity
 
@@ -2506,9 +2507,10 @@ def _save_grouphash_and_group(
             group = _create_group(project, event, **group_kwargs)
             group_hash.update(group=group)
 
-            if features.has(
-                "projects:first-event-severity-calculation", event.project
-            ) and not group.data.get("metadata", {}).get("severity"):
+            if (
+                features.has("projects:first-event-severity-calculation", event.project)
+                and group.data.get("metadata", {}).get("severity") is None
+            ):
                 logger.error(
                     "Group created without severity score",
                     extra={

tests/sentry/event_manager/test_event_manager.py

@@ -2639,7 +2639,37 @@ def test_severity_score_flag_off(self, mock_get_severity_score: MagicMock):
             event = manager.save(self.project.id)
 
             mock_get_severity_score.assert_not_called()
-            assert event.group and event.group.get_event_metadata().get("severity") is None
+            assert event.group and "severity" not in event.group.get_event_metadata()
+
+    @patch("sentry.event_manager._get_severity_score", return_value=None)
+    def test_no_severity_score_assigned_when_value_is_None(
+        self, mock_get_severity_score: MagicMock
+    ):
+        with self.feature({"projects:first-event-severity-calculation": True}):
+            manager = EventManager(
+                make_event(
+                    exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]}
+                )
+            )
+            event = manager.save(self.project.id)
+
+            mock_get_severity_score.assert_called()
+            assert event.group and "severity" not in event.group.get_event_metadata()
+
+    @patch("sentry.event_manager._get_severity_score", return_value=0)
+    def test_severity_score_still_assigned_when_value_is_zero(
+        self, mock_get_severity_score: MagicMock
+    ):
+        with self.feature({"projects:first-event-severity-calculation": True}):
+            manager = EventManager(
+                make_event(
+                    exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]}
+                )
+            )
+            event = manager.save(self.project.id)
+
+            mock_get_severity_score.assert_called()
+            assert event.group and event.group.get_event_metadata().get("severity") == 0
 
 
 class AutoAssociateCommitTest(TestCase, EventManagerTestMixin):
@@ -3717,6 +3747,29 @@ def test_logs_error_on_no_severity_score_when_enabled(
                 },
             )
 
+    @patch("sentry.event_manager.logger.error")
+    @patch("sentry.event_manager._get_severity_score", return_value=0)
+    def test_no_error_logged_on_zero_severity_score_when_enabled(
+        self,
+        mock_get_severity_score: MagicMock,
+        mock_logger_error: MagicMock,
+    ):
+        with self.feature({"projects:first-event-severity-calculation": True}):
+            event = _get_event_instance(
+                make_event(
+                    exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]}
+                ),
+                self.project.id,
+            )
+
+            group, _ = _save_grouphash_and_group(self.project, event, "dogs are great")
+
+            logger_messages = [call.args[0] for call in mock_logger_error.mock_calls]
+
+            mock_get_severity_score.assert_called()
+            assert group.data["metadata"]["severity"] == 0
+            assert "Group created without severity score" not in logger_messages
+
     @patch("sentry.event_manager.logger.error")
     @patch("sentry.event_manager._get_severity_score", return_value=None)
     def test_no_error_logged_on_no_severity_score_when_disabled(
@@ -3768,6 +3821,30 @@ def test_logs_error_on_no_severity_score_when_enabled(
                 },
             )
 
+    @patch("sentry.event_manager._save_aggregate", wraps=_save_aggregate)
+    @patch("sentry.event_manager.logger.error")
+    @patch("sentry.event_manager._get_severity_score", return_value=0)
+    def test_no_error_logged_on_zero_severity_score_when_enabled(
+        self,
+        mock_get_severity_score: MagicMock,
+        mock_logger_error: MagicMock,
+        mock_save_aggregate: MagicMock,
+    ):
+        with self.feature({"projects:first-event-severity-calculation": True}):
+            manager = EventManager(
+                make_event(
+                    exception={"values": [{"type": "NopeError", "value": "Nopey McNopeface"}]}
+                )
+            )
+            event = manager.save(self.project.id)
+
+            logger_messages = [call.args[0] for call in mock_logger_error.mock_calls]
+
+            mock_save_aggregate.assert_called()
+            mock_get_severity_score.assert_called()
+            assert event.group and event.group.data["metadata"]["severity"] == 0
+            assert "Group created without severity score" not in logger_messages
+
     @patch("sentry.event_manager._save_aggregate", wraps=_save_aggregate)
     @patch("sentry.event_manager.logger.error")
     @patch("sentry.event_manager._get_severity_score", return_value=None)
@@ -3789,5 +3866,4 @@ def test_no_error_logged_on_no_severity_score_when_disabled(
 
             mock_save_aggregate.assert_called()
             mock_get_severity_score.assert_not_called()
-
             assert "Group created without severity score" not in logger_messages