From e3fd633c771a85b20e04bfe03aaceb0262540a23 Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini <99758426+giovanni-guidini@users.noreply.github.com> Date: Thu, 23 May 2024 12:07:58 +0200 Subject: [PATCH] refactor: Comment conditions (#457) --- services/notification/notifiers/base.py | 21 ++- .../notifiers/comment/__init__.py | 113 +++++--------- .../notifiers/comment/conditions.py | 147 ++++++++++++++++++ .../notifiers/tests/unit/test_comment.py | 20 +-- .../tests/unit/test_notification_result.py | 35 +++++ 5 files changed, 245 insertions(+), 91 deletions(-) create mode 100644 services/notification/notifiers/comment/conditions.py create mode 100644 services/notification/tests/unit/test_notification_result.py diff --git a/services/notification/notifiers/base.py b/services/notification/notifiers/base.py index 7e754e2c3..3b06d0599 100644 --- a/services/notification/notifiers/base.py +++ b/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): """ diff --git a/services/notification/notifiers/comment/__init__.py b/services/notification/notifiers/comment/__init__.py index d8755b93d..5f9c157ff 100644 --- a/services/notification/notifiers/comment/__init__.py +++ b/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, diff --git a/services/notification/notifiers/comment/conditions.py b/services/notification/notifiers/comment/conditions.py new file mode 100644 index 000000000..b60f0ffa8 --- /dev/null +++ b/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) diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index 7b83eed03..56bf5396e 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -2416,7 +2416,7 @@ async def test_send_actual_notification_once_deleted_comment( "response", "message" ) result = await notifier.send_actual_notification(data) - assert not result.notification_attempted + assert result.notification_attempted is False assert result.notification_successful is None assert result.explanation == "comment_deleted" assert result.data_sent == data @@ -2642,7 +2642,7 @@ async def test_notify_no_pull_request( ) result = await notifier.notify(sample_comparison_without_pull) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful == False assert result.explanation == "no_pull_request" assert result.data_sent is None assert result.data_received is None @@ -2663,7 +2663,7 @@ async def test_notify_pull_head_doesnt_match(self, dbsession, sample_comparison) ) result = await notifier.notify(sample_comparison) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful is False assert result.explanation == "pull_head_does_not_match" assert result.data_sent is None assert result.data_received is None @@ -2684,7 +2684,7 @@ async def test_notify_pull_request_not_in_provider( ) result = await notifier.notify(sample_comparison_database_pull_without_provider) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful is False assert result.explanation == "pull_request_not_in_provider" assert result.data_sent is None assert result.data_received is None @@ -2821,7 +2821,7 @@ async def test_notify_closed_pull_request(self, dbsession, sample_comparison): dbsession.refresh(sample_comparison.pull) result = await notifier.notify(sample_comparison) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful is False assert result.explanation == "pull_request_closed" assert result.data_sent is None assert result.data_received is None @@ -2847,7 +2847,7 @@ async def test_notify_unable_to_fetch_info( ) result = await notifier.notify(sample_comparison) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful is False assert result.explanation == "unable_build_message" assert result.data_sent is None assert result.data_received is None @@ -2867,7 +2867,7 @@ async def test_notify_not_enough_builds(self, dbsession, sample_comparison): ) result = await notifier.notify(sample_comparison) assert not result.notification_attempted - assert result.notification_successful is None + assert result.notification_successful is False assert result.explanation == "not_enough_builds" assert result.data_sent is None assert result.data_received is None @@ -3133,7 +3133,7 @@ async def test_notify_exact_same_report_diff_unrelated_report( ) res = await notifier.notify(sample_comparison_no_change) assert res.notification_attempted is False - assert res.notification_successful is None + assert res.notification_successful is False assert res.explanation == "changes_required" assert res.data_sent is None assert res.data_received is None @@ -3195,7 +3195,7 @@ async def test_notify_exact_same_report_diff_unrelated_report_deleting_comment( ) res = await notifier.notify(sample_comparison_no_change) assert res.notification_attempted is False - assert res.notification_successful is None + assert res.notification_successful is False assert res.explanation == "changes_required" assert res.data_sent is None assert res.data_received == {"deleted_comment": True} @@ -3260,7 +3260,7 @@ async def test_notify_exact_same_report_diff_unrelated_report_deleting_comment_c ) res = await notifier.notify(sample_comparison_no_change) assert res.notification_attempted is False - assert res.notification_successful is None + assert res.notification_successful is False assert res.explanation == "changes_required" assert res.data_sent is None assert res.data_received == {"deleted_comment": False} diff --git a/services/notification/tests/unit/test_notification_result.py b/services/notification/tests/unit/test_notification_result.py new file mode 100644 index 000000000..52b2ff60c --- /dev/null +++ b/services/notification/tests/unit/test_notification_result.py @@ -0,0 +1,35 @@ +from services.notification.notifiers.base import NotificationResult + + +def test_notification_result_or_operation(): + result_default = NotificationResult() + result_explanation = NotificationResult(explanation="some_explanation") + result_not_attempted = NotificationResult( + notification_attempted=False, + notification_successful=False, + explanation="dont_want", + ) + result_attempted = NotificationResult( + notification_attempted=True, + notification_successful=True, + explanation=None, + ) + result_some_data = NotificationResult( + data_sent={"comment": "hi"}, data_received={"response": "hi"} + ) + assert result_default.merge(result_explanation) == result_explanation + assert result_default.merge(result_not_attempted) == result_not_attempted + assert result_attempted.merge(result_some_data) == NotificationResult( + notification_attempted=True, + notification_successful=True, + explanation=None, + data_sent={"comment": "hi"}, + data_received={"response": "hi"}, + ) + assert result_not_attempted.merge(result_some_data) == NotificationResult( + notification_attempted=False, + notification_successful=False, + explanation="dont_want", + data_sent={"comment": "hi"}, + data_received={"response": "hi"}, + )