refactor: Comment conditions (#457)

services/notification/notifiers/base.py

@@ -12,12 +12,25 @@
 
 @dataclass
 class NotificationResult(object):
-    notification_attempted: bool
-    notification_successful: bool
-    explanation: str
-    data_sent: Mapping[str, Any]
+    notification_attempted: bool = False
+    notification_successful: bool = False
+    explanation: str = None
+    data_sent: Mapping[str, Any] = None
     data_received: Mapping[str, Any] = None
 
+    def merge(self, other: "NotificationResult") -> "NotificationResult":
+        ans = NotificationResult()
+        ans.notification_attempted = bool(
+            self.notification_attempted or other.notification_attempted
+        )
+        ans.notification_successful = bool(
+            self.notification_successful or other.notification_successful
+        )
+        ans.explanation = self.explanation or other.explanation
+        ans.data_sent = self.data_sent or other.data_sent
+        ans.data_received = self.data_received or other.data_received
+        return ans
+
 
 class AbstractBaseNotifier(object):
     """

services/notification/notifiers/comment/__init__.py

@@ -11,12 +11,22 @@
 from database.enums import Notification
 from helpers.metrics import metrics
 from services.billing import BillingPlan
+from services.comparison import ComparisonProxy
 from services.comparison.types import Comparison
 from services.license import requires_license
 from services.notification.notifiers.base import (
     AbstractBaseNotifier,
     NotificationResult,
 )
+from services.notification.notifiers.comment.conditions import (
+    ComparisonHasPull,
+    HasEnoughBuilds,
+    HasEnoughRequiredChanges,
+    NotifyCondition,
+    PullHeadMatchesComparisonHead,
+    PullRequestInProvider,
+    PullRequestOpen,
+)
 from services.notification.notifiers.mixins.message import MessageMixin
 from services.repository import get_repo_provider_service
 from services.urls import append_tracking_params_to_urls, get_members_url, get_plan_url
@@ -25,6 +35,15 @@
 
 
 class CommentNotifier(MessageMixin, AbstractBaseNotifier):
+    notify_conditions: List[NotifyCondition] = [
+        ComparisonHasPull,
+        PullRequestInProvider,
+        PullRequestOpen,
+        PullHeadMatchesComparisonHead,
+        HasEnoughBuilds,
+        HasEnoughRequiredChanges,
+    ]
+
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self._repository_service = None
@@ -69,88 +88,29 @@ async def has_enough_changes(self, comparison):
             return True
         return False
 
-    async def notify(self, comparison: Comparison, **extra_data) -> NotificationResult:
-        if comparison.pull is None:
-            return NotificationResult(
-                notification_attempted=False,
-                notification_successful=None,
-                explanation="no_pull_request",
-                data_sent=None,
-                data_received=None,
-            )
-        if (
-            comparison.enriched_pull is None
-            or comparison.enriched_pull.provider_pull is None
-        ):
-            return NotificationResult(
-                notification_attempted=False,
-                notification_successful=None,
-                explanation="pull_request_not_in_provider",
-                data_sent=None,
-                data_received=None,
+    async def notify(
+        self, comparison: ComparisonProxy, **extra_data
+    ) -> NotificationResult:
+        for condition in self.notify_conditions:
+            condition_result = (
+                await condition.check_condition(notifier=self, comparison=comparison)
+                if condition.is_async_condition
+                else condition.check_condition(notifier=self, comparison=comparison)
             )
-        if comparison.pull.state != "open":
-            return NotificationResult(
-                notification_attempted=False,
-                notification_successful=None,
-                explanation="pull_request_closed",
-                data_sent=None,
-                data_received=None,
-            )
-        if comparison.pull.head != comparison.head.commit.commitid:
-            return NotificationResult(
-                notification_attempted=False,
-                notification_successful=None,
-                explanation="pull_head_does_not_match",
-                data_sent=None,
-                data_received=None,
-            )
-        if self.notifier_yaml_settings.get("after_n_builds") is not None:
-            n_builds_present = len(comparison.head.report.sessions)
-            if n_builds_present < self.notifier_yaml_settings.get("after_n_builds"):
-                return NotificationResult(
+            if condition_result == False:
+                side_effect_result = (
+                    condition.on_failure_side_effect(self, comparison)
+                    if condition.is_async_condition is False
+                    else (await condition.on_failure_side_effect(self, comparison))
+                )
+                default_result = NotificationResult(
                     notification_attempted=False,
-                    notification_successful=None,
-                    explanation="not_enough_builds",
+                    explanation=condition.failure_explanation,
                     data_sent=None,
                     data_received=None,
                 )
+                return default_result.merge(side_effect_result)
         pull = comparison.pull
-        if self.notifier_yaml_settings.get("require_changes"):
-            if not (await self.has_enough_changes(comparison)):
-                data_received = None
-                if pull.commentid is not None:
-                    log.info(
-                        "Deleting comment because there are not enough changes according to YAML",
-                        extra=dict(
-                            repoid=pull.repoid,
-                            pullid=pull.pullid,
-                            commentid=pull.commentid,
-                        ),
-                    )
-                    try:
-                        await self.repository_service.delete_comment(
-                            pull.pullid, pull.commentid
-                        )
-                        data_received = {"deleted_comment": True}
-                    except TorngitClientError:
-                        log.warning(
-                            "Comment could not be deleted due to client permissions",
-                            exc_info=True,
-                            extra=dict(
-                                repoid=pull.repoid,
-                                pullid=pull.pullid,
-                                commentid=pull.commentid,
-                            ),
-                        )
-                        data_received = {"deleted_comment": False}
-                return NotificationResult(
-                    notification_attempted=False,
-                    notification_successful=None,
-                    explanation="changes_required",
-                    data_sent=None,
-                    data_received=data_received,
-                )
         try:
             with metrics.timer(
                 "worker.services.notifications.notifiers.comment.build_message"
@@ -167,7 +127,6 @@ async def notify(self, comparison: Comparison, **extra_data) -> NotificationResu
             )
             return NotificationResult(
                 notification_attempted=False,
-                notification_successful=None,
                 explanation="unable_build_message",
                 data_sent=None,
                 data_received=None,

services/notification/notifiers/comment/conditions.py

@@ -0,0 +1,147 @@
+import logging
+from abc import ABC, abstractmethod
+
+from shared.torngit.exceptions import (
+    TorngitClientError,
+)
+
+from services.comparison import ComparisonProxy
+from services.notification.notifiers.base import (
+    AbstractBaseNotifier,
+    NotificationResult,
+)
+
+log = logging.getLogger(__name__)
+
+
+class NotifyCondition(ABC):
+    """Abstract class that defines the basis of a NotifyCondition.
+
+    NotifyCondition specifies the conditions that need to be met in order for a notification to be sent
+    from Codecov to a git provider.
+    NotifyCondition can have a side effect that is called when the condition fails.
+    """
+
+    is_async_condition: bool = False
+    failure_explanation: str
+
+    @abstractmethod
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return True
+
+    def on_failure_side_effect(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> NotificationResult:
+        return NotificationResult()
+
+
+class AsyncNotifyCondition(NotifyCondition):
+    """Async version of NotifyCondition"""
+
+    is_async_condition: bool = True
+
+    @abstractmethod
+    async def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return True
+
+    async def on_failure_side_effect(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> NotificationResult:
+        return NotificationResult()
+
+
+class ComparisonHasPull(NotifyCondition):
+    failure_explanation = "no_pull_request"
+
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return comparison.pull is not None
+
+
+class PullRequestInProvider(NotifyCondition):
+    failure_explanation = "pull_request_not_in_provider"
+
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return (
+            comparison.enriched_pull is not None
+            and comparison.enriched_pull.provider_pull is not None
+        )
+
+
+class PullRequestOpen(NotifyCondition):
+    failure_explanation = "pull_request_closed"
+
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return comparison.pull.state == "open"
+
+
+class PullHeadMatchesComparisonHead(NotifyCondition):
+    failure_explanation = "pull_head_does_not_match"
+
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        return comparison.pull.head == comparison.head.commit.commitid
+
+
+class HasEnoughBuilds(NotifyCondition):
+    failure_explanation = "not_enough_builds"
+
+    def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        expected_builds = notifier.notifier_yaml_settings.get("after_n_builds", 0)
+        present_builds = len(comparison.head.report.sessions)
+        return present_builds >= expected_builds
+
+
+class HasEnoughRequiredChanges(AsyncNotifyCondition):
+    failure_explanation = "changes_required"
+
+    async def check_condition(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> bool:
+        requires_changes = notifier.notifier_yaml_settings.get("require_changes", False)
+        return (requires_changes == False) or await notifier.has_enough_changes(
+            comparison
+        )
+
+    async def on_failure_side_effect(
+        notifier: AbstractBaseNotifier, comparison: ComparisonProxy
+    ) -> NotificationResult:
+        pull = comparison.pull
+        data_received = None
+        extra_log_info = dict(
+            repoid=pull.repoid,
+            pullid=pull.pullid,
+            commentid=pull.commentid,
+        )
+        if pull.commentid is not None:
+            # Just porting logic as-is, but not sure if it's the best
+            # TODO: codecov/engineering-team#1761
+            log.info(
+                "Deleting comment because there are not enough changes according to YAML",
+                extra=extra_log_info,
+            )
+            try:
+                await notifier.repository_service.delete_comment(
+                    pull.pullid, pull.commentid
+                )
+                data_received = {"deleted_comment": True}
+            except TorngitClientError:
+                log.warning(
+                    "Comment could not be deleted due to client permissions",
+                    exc_info=True,
+                    extra=extra_log_info,
+                )
+                data_received = {"deleted_comment": False}
+        return NotificationResult(data_received=data_received)