address review comment

move logic to get success value for bundle_analysis_notify return to the return type and create a proper Enum for the success values

services/bundle_analysis/new_notify/__init__.py

@@ -21,7 +21,10 @@
 from services.bundle_analysis.new_notify.messages.comment import (
     BundleAnalysisCommentMarkdownStrategy,
 )
-from services.bundle_analysis.new_notify.types import NotificationType
+from services.bundle_analysis.new_notify.types import (
+    NotificationSuccess,
+    NotificationType,
+)
 
 log = logging.getLogger(__name__)
 
@@ -35,6 +38,15 @@ class BundleAnalysisNotifyReturn(NamedTuple):
     notifications_configured: tuple[NotificationType]
     notifications_successful: tuple[NotificationType]
 
+    def to_NotificationSuccess(self) -> NotificationSuccess:
+        notification_configured_count = len(self.notifications_configured)
+        notifications_successful_count = len(self.notifications_successful)
+        if notification_configured_count == 0:
+            return NotificationSuccess.NOTHING_TO_NOTIFY
+        if notification_configured_count == notifications_successful_count:
+            return NotificationSuccess.FULL_SUCCESS
+        return NotificationSuccess.PARTIAL_SUCCESS
+
 
 class BundleAnalysisNotifyService:
     def __init__(

services/bundle_analysis/new_notify/tests/test_notify_service.py

@@ -9,6 +9,7 @@
     BundleAnalysisNotifyReturn,
     BundleAnalysisNotifyService,
     NotificationFullContext,
+    NotificationSuccess,
 )
 from services.bundle_analysis.new_notify.conftest import (
     get_commit_pair,
@@ -208,3 +209,25 @@ def test_notify(
         )
         assert len(result.notifications_configured) == expected_configured_count
         assert len(result.notifications_successful) == expected_success_count
+
+    @pytest.mark.parametrize(
+        "result, success_value",
+        [
+            (BundleAnalysisNotifyReturn([], []), NotificationSuccess.NOTHING_TO_NOTIFY),
+            (
+                BundleAnalysisNotifyReturn(
+                    [NotificationType.COMMIT_STATUS], [NotificationType.COMMIT_STATUS]
+                ),
+                NotificationSuccess.FULL_SUCCESS,
+            ),
+            (
+                BundleAnalysisNotifyReturn(
+                    [NotificationType.COMMIT_STATUS, NotificationType.PR_COMMENT],
+                    [NotificationType.COMMIT_STATUS],
+                ),
+                NotificationSuccess.PARTIAL_SUCCESS,
+            ),
+        ],
+    )
+    def test_to_NotificationSuccess(self, result, success_value):
+        assert result.to_NotificationSuccess() == success_value

services/bundle_analysis/new_notify/types.py

@@ -7,3 +7,9 @@ class NotificationType(Enum):
     # See docs on the difference between COMMIT_STATUS and GITHUB_COMMIT_CHECK
     # https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks#types-of-status-checks-on-github
     GITHUB_COMMIT_CHECK = "github_commit_check"
+
+
+class NotificationSuccess(Enum):
+    NOTHING_TO_NOTIFY = "nothing_to_notify"
+    FULL_SUCCESS = "full_success"
+    PARTIAL_SUCCESS = "partial_success"

tasks/bundle_analysis_notify.py

@@ -63,15 +63,6 @@ def run_impl(
         except LockRetry as retry:
             self.retry(max_retries=5, countdown=retry.countdown)
 
-    def get_success_value(
-        self, configured_notifications_count: int, successful_notifications_count: int
-    ) -> str:
-        if configured_notifications_count == 0:
-            return "nothing_to_notify"
-        if configured_notifications_count == successful_notifications_count:
-            return "full_success"
-        return "partial_success"
-
     def process_impl_within_lock(
         self,
         *,
@@ -98,8 +89,6 @@ def process_impl_within_lock(
         assert commit, "commit not found"
 
         notify = True
-        configured_notifications_count = 0
-        successful_notifications_count = 0
 
         # these are the task results from prior processor tasks in the chain
         # (they get accumulated as we execute each task in succession)
@@ -117,8 +106,6 @@ def process_impl_within_lock(
                 commit, commit_yaml, gh_app_installation_name=installation_name_to_use
             )
             result = notifier.notify()
-            configured_notifications_count = len(result.notifications_configured)
-            successful_notifications_count = len(result.notifications_successful)
 
         log.info(
             "Finished bundle analysis notify",
@@ -133,9 +120,7 @@ def process_impl_within_lock(
 
         return {
             "notify_attempted": notify,
-            "notify_succeeded": self.get_success_value(
-                configured_notifications_count, successful_notifications_count
-            ),
+            "notify_succeeded": result.to_NotificationSuccess(),
         }
 
 

tasks/tests/unit/test_bundle_analysis_notify_task.py

@@ -1,29 +1,10 @@
-import pytest
-
 from database.tests.factories import CommitFactory
 from services.bundle_analysis.new_notify import BundleAnalysisNotifyReturn
-from services.bundle_analysis.new_notify.types import NotificationType
-from tasks.bundle_analysis_notify import BundleAnalysisNotifyTask
-
-
-@pytest.mark.parametrize(
-    "configured_notifications_count, successful_notifications_count, expected",
-    [
-        (0, 0, "nothing_to_notify"),
-        (2, 2, "full_success"),
-        (2, 1, "partial_success"),
-    ],
+from services.bundle_analysis.new_notify.types import (
+    NotificationSuccess,
+    NotificationType,
 )
-def test_bundle_analysis_notify_task_get_success(
-    configured_notifications_count, successful_notifications_count, expected
-):
-    task = BundleAnalysisNotifyTask()
-    assert (
-        task.get_success_value(
-            configured_notifications_count, successful_notifications_count
-        )
-        == expected
-    )
+from tasks.bundle_analysis_notify import BundleAnalysisNotifyTask
 
 
 def test_bundle_analysis_notify_task(
@@ -55,5 +36,5 @@ def test_bundle_analysis_notify_task(
     )
     assert result == {
         "notify_attempted": True,
-        "notify_succeeded": "full_success",
+        "notify_succeeded": NotificationSuccess.FULL_SUCCESS,
     }