diff --git a/services/notification/__init__.py b/services/notification/__init__.py index 3f22c7a4c..853ec3d00 100644 --- a/services/notification/__init__.py +++ b/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 diff --git a/services/notification/commit_notifications.py b/services/notification/commit_notifications.py index e05eeb98e..75efc3adc 100644 --- a/services/notification/commit_notifications.py +++ b/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 ) diff --git a/services/notification/notifiers/comment/__init__.py b/services/notification/notifiers/comment/__init__.py index 66870f1ee..636678636 100644 --- a/services/notification/notifiers/comment/__init__.py +++ b/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, diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 693d83406..106d88c3a 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/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( diff --git a/services/notification/tests/unit/test_notification_service.py b/services/notification/tests/unit/test_notification_service.py index 0e06465bf..ceaa1c9e5 100644 --- a/services/notification/tests/unit/test_notification_service.py +++ b/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()