Refactor and simplify notification logic a bit (#1185)

Author: Swatinem

services/notification/__init__.py

@@ -162,6 +162,8 @@ def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]:
         if yaml_field is not None:
             for instance_type, instance_configs in yaml_field.items():
                 class_to_use = mapping.get(instance_type)
+                if not class_to_use:
+                    continue
                 for title, individual_config in instance_configs.items():
                     yield class_to_use(
                         repository=self.repository,
@@ -269,127 +271,107 @@ def notify(self, comparison: ComparisonProxy) -> list[IndividualResult]:
             ),
         )
 
-        results = []
-        status_or_checks_helper_text = {}
-        notifiers_to_notify = [
+        status_or_checks_notifiers, all_other_notifiers = split_notifiers(
             notifier
             for notifier in self.get_notifiers_instances()
             if notifier.is_enabled()
-        ]
-
-        status_or_checks_notifiers = [
-            notifier
-            for notifier in notifiers_to_notify
-            if notifier.notification_type in notification_type_status_or_checks
-        ]
-
-        all_other_notifiers = [
-            notifier
-            for notifier in notifiers_to_notify
-            if notifier.notification_type not in notification_type_status_or_checks
-        ]
+        )
 
-        status_or_checks_results = [
+        results = [
             self.notify_individual_notifier(notifier, comparison)
             for notifier in status_or_checks_notifiers
         ]
 
-        if status_or_checks_results and all_other_notifiers:
+        status_or_checks_helper_text = {}
+        if results and all_other_notifiers:
             # if the status/check fails, sometimes we want to add helper text to the message of the other notifications,
             # to better surface that the status/check failed.
             # so if there are status_and_checks_notifiers and all_other_notifiers, do the status_and_checks_notifiers first,
             # look at the results of the checks, if any failed AND they are the type we have helper text for,
             # add that text onto the other notifiers messages through status_or_checks_helper_text.
-            for result in status_or_checks_results:
-                notification_result = result["result"]
-                if (
-                    notification_result is not None
-                    and notification_result.data_sent is not None
-                ):
+            for _notifier, result in results:
+                if result is not None and result.data_sent is not None:
                     if (
-                        notification_result.data_sent.get("state")
-                        == StatusState.failure.value
-                    ) and notification_result.data_sent.get("included_helper_text"):
+                        result.data_sent.get("state") == StatusState.failure.value
+                    ) and result.data_sent.get("included_helper_text"):
                         status_or_checks_helper_text.update(
-                            notification_result.data_sent["included_helper_text"]
+                            result.data_sent["included_helper_text"]
                         )
-        results = results + status_or_checks_results
 
-        results = results + [
+        results.extend(
             self.notify_individual_notifier(
                 notifier,
                 comparison,
                 status_or_checks_helper_text=status_or_checks_helper_text,
             )
             for notifier in all_other_notifiers
+        )
+
+        return [
+            IndividualResult(
+                notifier=notifier.name, title=notifier.title, result=result
+            )
+            for notifier, result in results
         ]
-        return results
 
     def notify_individual_notifier(
         self,
         notifier: AbstractBaseNotifier,
         comparison: ComparisonProxy,
         status_or_checks_helper_text: Optional[dict[str, str]] = None,
-    ) -> IndividualResult:
+    ) -> tuple[AbstractBaseNotifier, NotificationResult | None]:
         commit = comparison.head.commit
         base_commit = comparison.project_coverage_base.commit
-        log.info(
-            "Attempting individual notification",
-            extra=dict(
-                commit=commit.commitid,
-                base_commit=(
-                    base_commit.commitid if base_commit is not None else "NO_BASE"
-                ),
-                repoid=commit.repoid,
-                notifier=notifier.name,
-                notifier_title=notifier.title,
-            ),
-        )
-        # individual_result.result is updated in case of success
-        individual_result = IndividualResult(
-            notifier=notifier.name, title=notifier.title, result=None
-        )
+        log_extra = {
+            "commit": commit.commitid,
+            "base_commit": base_commit.commitid
+            if base_commit is not None
+            else "NO_BASE",
+            "repoid": commit.repoid,
+            "notifier": notifier.name,
+            "notifier_title": notifier.title,
+        }
+
+        log.info("Attempting individual notification", extra=log_extra)
+        res: NotificationResult | None = None
         try:
             res = notifier.notify(
                 comparison, status_or_checks_helper_text=status_or_checks_helper_text
             )
-            individual_result["result"] = res
+            log_extra["result"] = res
 
+            # TODO: The `CommentNotifier` is the only one implementing this method,
+            # so maybe we should just move it there
             notifier.store_results(comparison, res)
-            log.info(
-                "Individual notification done",
-                extra=dict(
-                    individual_result=individual_result,
-                    commit=commit.commitid,
-                    base_commit=(
-                        base_commit.commitid if base_commit is not None else "NO_BASE"
-                    ),
-                    repoid=commit.repoid,
-                ),
-            )
-            return individual_result
+
+            log.info("Individual notification done", extra=log_extra)
+            return notifier, res
         except (CeleryError, SoftTimeLimitExceeded):
             raise
         except Exception:
-            log.exception(
-                "Individual notifier failed",
-                extra=dict(
-                    repoid=commit.repoid,
-                    commit=commit.commitid,
-                    individual_result=individual_result,
-                    base_commit=(
-                        base_commit.commitid if base_commit is not None else "NO_BASE"
-                    ),
-                ),
-            )
-            return individual_result
+            log.exception("Individual notifier failed", extra=log_extra)
+            return notifier, res
         finally:
-            if (
-                individual_result["result"] is None
-                or individual_result["result"].notification_attempted
-            ):
+            if res is None or res.notification_attempted:
                 # only running if there is no result (indicating some exception)
                 # or there was an actual attempt
                 create_or_update_commit_notification_from_notification_result(
-                    comparison, notifier, individual_result["result"]
+                    comparison, notifier, res
                 )
+
+
+def split_notifiers(
+    notifiers: Iterator[AbstractBaseNotifier],
+) -> tuple[list[AbstractBaseNotifier], list[AbstractBaseNotifier]]:
+    "Splits the input notifiers based on whether they are a status/check, or other notifier."
+
+    status_or_checks_notifiers = []
+    all_other_notifiers = []
+
+    for notifier in notifiers:
+        if notifier.notification_type in notification_type_status_or_checks:
+            status_or_checks_notifiers.append(notifier)
+        else:
+            all_other_notifiers.append(notifier)
+
+    return status_or_checks_notifiers, all_other_notifiers

services/notification/commit_notifications.py

@@ -4,7 +4,6 @@
 
 from database.enums import NotificationState
 from database.models import CommitNotification, Pull
-from helpers.metrics import metrics
 from services.comparison import ComparisonProxy
 from services.notification.notifiers.base import (
     AbstractBaseNotifier,
@@ -14,30 +13,11 @@
 log = logging.getLogger(__name__)
 
 
-def _get_notification_state_from_result(
-    notification_result: NotificationResult | None,
-) -> NotificationState:
-    """
-    Take notification result from notification service and convert to
-    the proper NotificationState enum
-    """
-    if notification_result is None:
-        return NotificationState.error
-
-    successful = notification_result.notification_successful
-
-    if successful:
-        return NotificationState.success
-    else:
-        return NotificationState.error
-
-
-@metrics.timer("internal.services.notification.store_notification_result")
 def create_or_update_commit_notification_from_notification_result(
     comparison: ComparisonProxy,
     notifier: AbstractBaseNotifier,
     notification_result: NotificationResult | None,
-) -> CommitNotification:
+) -> CommitNotification | None:
     """Saves a CommitNotification entry in the database.
     We save an entry in the following scenarios:
         - We save all notification attempts for commits that are part of a PullRequest
@@ -54,12 +34,12 @@ def create_or_update_commit_notification_from_notification_result(
         or notification_result.notification_successful == False
     )
     if not_pull and (not_head_commit or not_github_app_info or failed):
-        return
+        return None
 
     commit = pull.get_head_commit() if pull else comparison.head.commit
     if not commit:
         log.warning("Head commit not found for pull", extra=dict(pull=pull))
-        return
+        return None
 
     db_session: Session = commit.get_db_session()
 
@@ -72,7 +52,9 @@ def create_or_update_commit_notification_from_notification_result(
         .first()
     )
 
-    notification_state = _get_notification_state_from_result(notification_result)
+    notification_state = (
+        NotificationState.error if failed else NotificationState.success
+    )
     github_app_used = (
         notification_result.github_app_used if notification_result else None
     )

services/notification/notifiers/comment/__init__.py

@@ -42,7 +42,7 @@
 
 
 class CommentNotifier(MessageMixin, AbstractBaseNotifier):
-    notify_conditions: list[NotifyCondition] = [
+    notify_conditions: list[type[NotifyCondition]] = [
         ComparisonHasPull,
         PullRequestInProvider,
         PullHeadMatchesComparisonHead,

services/notification/notifiers/mixins/message/__init__.py

@@ -49,7 +49,6 @@ def create_message(
         pull = comparison.pull
 
         settings = yaml_settings
-
         current_yaml = self.current_yaml
 
         links = {
@@ -63,7 +62,7 @@ def create_message(
         }
 
         # bool: show complexity
-        if read_yaml_field(self.current_yaml, ("codecov", "ui", "hide_complexity")):
+        if read_yaml_field(current_yaml, ("codecov", "ui", "hide_complexity")):
             show_complexity = False
         else:
             show_complexity = bool(

services/notification/tests/unit/test_notification_service.py

@@ -610,11 +610,7 @@ def test_notify_individual_notifier_timeout(self, mocker, sample_comparison):
         res = notifications_service.notify_individual_notifier(
             notifier, sample_comparison
         )
-        assert res == {
-            "notifier": notifier.name,
-            "result": None,
-            "title": "fake_notifier",
-        }
+        assert res == (notifier, None)
 
     @pytest.mark.django_db
     def test_notify_individual_checks_project_notifier(
@@ -649,10 +645,9 @@ def test_notify_individual_checks_project_notifier(
             notifier, sample_comparison
         )
 
-        assert res == {
-            "notifier": "checks-project",
-            "title": "title",
-            "result": NotificationResult(
+        assert res == (
+            notifier,
+            NotificationResult(
                 notification_attempted=True,
                 notification_successful=True,
                 explanation=None,
@@ -668,7 +663,7 @@ def test_notify_individual_checks_project_notifier(
                 },
                 data_received=None,
             ),
-        }
+        )
 
     @pytest.mark.django_db
     def test_notify_individual_checks_patch_and_project_notifier_included_helper_text(
@@ -894,11 +889,8 @@ def test_notify_individual_notifier_timeout_notification_created(
         res = notifications_service.notify_individual_notifier(
             notifier, sample_comparison
         )
-        assert res == {
-            "notifier": notifier.name,
-            "result": None,
-            "title": "fake_notifier",
-        }
+        assert res == (notifier, None)
+
         dbsession.flush()
         pull = sample_comparison.enriched_pull.database_pull
         pull_commit_notifications = pull.get_head_commit_notifications()
@@ -930,11 +922,8 @@ def test_notify_individual_notifier_notification_created_then_updated(
         res = notifications_service.notify_individual_notifier(
             notifier, sample_comparison
         )
-        assert res == {
-            "notifier": notifier.name,
-            "result": None,
-            "title": "fake_notifier",
-        }
+        assert res == (notifier, None)
+
         dbsession.flush()
         pull = sample_comparison.enriched_pull.database_pull
         pull_commit_notifications = pull.get_head_commit_notifications()