chore: fix (some) type errors in BA notify (#777)

Author: giovanni-guidini

services/activation.py

@@ -104,7 +104,7 @@ def activate_user(db_session, org_ownerid: int, user_ownerid: int) -> bool:
     return activation_success
 
 
-def schedule_new_user_activated_task(self, org_ownerid, user_ownerid):
+def schedule_new_user_activated_task(org_ownerid, user_ownerid):
     celery_app.send_task(
         new_user_activated_task_name,
         args=None,

services/bundle_analysis/notify/__init__.py

@@ -1,5 +1,5 @@
 import logging
-from typing import NamedTuple
+from typing import NamedTuple, Never
 
 import sentry_sdk
 from shared.yaml import UserYaml
@@ -35,9 +35,9 @@ class NotificationFullContext(NamedTuple):
 
 
 class BundleAnalysisNotifyReturn(NamedTuple):
-    notifications_configured: tuple[NotificationType]
-    notifications_attempted: tuple[NotificationType]
-    notifications_successful: tuple[NotificationType]
+    notifications_configured: tuple[NotificationType, ...]
+    notifications_attempted: tuple[NotificationType, ...] | tuple[Never]
+    notifications_successful: tuple[NotificationType, ...] | tuple[Never]
 
     def to_NotificationSuccess(self) -> NotificationSuccess:
         notification_configured_count = len(self.notifications_configured)
@@ -102,33 +102,33 @@ def create_context_for_notification(
             tuple[NotificationContextBuilder, MessageStrategyInterface],
         ] = {
             NotificationType.PR_COMMENT: (
-                BundleAnalysisPRCommentContextBuilder,
-                BundleAnalysisCommentMarkdownStrategy,
+                BundleAnalysisPRCommentContextBuilder(),
+                BundleAnalysisCommentMarkdownStrategy(),
             ),
             NotificationType.COMMIT_STATUS: (
-                CommitStatusNotificationContextBuilder,
-                CommitStatusMessageStrategy,
+                CommitStatusNotificationContextBuilder(),
+                CommitStatusMessageStrategy(),
             ),
             # The commit-check API is more powerful than COMMIT_STATUS
             # but we currently don't differentiate between them
             NotificationType.GITHUB_COMMIT_CHECK: (
-                CommitStatusNotificationContextBuilder,
-                CommitStatusMessageStrategy,
+                CommitStatusNotificationContextBuilder(),
+                CommitStatusMessageStrategy(),
             ),
         }
         notifier_strategy = notifier_lookup.get(notification_type)
         if notifier_strategy is None:
             msg = f"No context builder for {notification_type}. Skipping"
             log.error(msg)
             return None
-        builder_class, message_strategy_class = notifier_strategy
+        builder_instance, message_strategy_instance = notifier_strategy
         try:
-            builder = builder_class().initialize_from_context(
+            builder = builder_instance.initialize_from_context(
                 self.current_yaml, base_context
             )
             return NotificationFullContext(
                 builder.build_context().get_result(),
-                message_strategy_class(),
+                message_strategy_instance,
             )
         except NotificationContextBuildError as exp:
             log.warning(
@@ -142,8 +142,8 @@ def create_context_for_notification(
     def get_notification_contexts(
         self,
         base_context: BaseBundleAnalysisNotificationContext,
-        notification_types: list[NotificationType],
-    ) -> list[BaseBundleAnalysisNotificationContext]:
+        notification_types: tuple[NotificationType, ...],
+    ) -> list[NotificationFullContext]:
         previous_context = base_context
         specific_contexts = []
         for notification_type in notification_types:

services/bundle_analysis/notify/contexts/__init__.py

@@ -47,13 +47,13 @@ class NotificationContextField(Generic[T]):
     def __set_name__(self, owner, name) -> None:
         self._name = name
 
-    def __get__(self, instance: "NotificationContextField", owner) -> T:
+    def __get__(self, instance, owner) -> T:
         if self._name not in instance.__dict__:
             msg = f"Property {self._name} is not loaded. Make sure to build the context before using it."
             raise ContextNotLoadedError(msg)
         return instance.__dict__[self._name]
 
-    def __set__(self, instance: "NotificationContextField", value: T) -> None:
+    def __set__(self, instance, value: T) -> None:
         instance.__dict__[self._name] = value
 
 
@@ -84,13 +84,9 @@ def repository_service(self) -> TorngitBaseAdapter:
             installation_name_to_use=self.gh_app_installation_name,
         )
 
-    commit_report: CommitReport = NotificationContextField[CommitReport]()
-    bundle_analysis_report: BundleAnalysisReport = NotificationContextField[
-        BundleAnalysisReport
-    ]()
-    user_config: NotificationUserConfig = NotificationContextField[
-        NotificationUserConfig
-    ]()
+    commit_report = NotificationContextField[CommitReport]()
+    bundle_analysis_report = NotificationContextField[BundleAnalysisReport]()
+    user_config = NotificationContextField[NotificationUserConfig]()
 
 
 class NotificationContextBuildError(Exception):
@@ -109,7 +105,7 @@ class NotificationContextBuilder:
 
     current_yaml: UserYaml
     """ Used with `initialize_from_context` method. Declare the fields the class wants to copy over when initializing from another context."""
-    fields_of_interest: tuple[str] = (
+    fields_of_interest: tuple[str, ...] = (
         "commit_report",
         "bundle_analysis_report",
         "user_config",
@@ -188,14 +184,13 @@ def load_user_config(self) -> "NotificationContextBuilder":
         This allows all notifiers to access configuration for any notifier and already have the defaults
         """
         comment_config: bool | dict = self.current_yaml.read_yaml_field("comment")
-        if not comment_config:
-            required_changes = False
+        required_changes: bool | Literal["bundle_increase"]
+        if isinstance(comment_config, bool):
+            required_changes = comment_config
             required_changes_threshold = BundleThreshold("absolute", 0)
         else:
-            required_changes: bool | Literal["bundle_increase"] = comment_config.get(
-                "require_bundle_changes", False
-            )
-            required_changes_threshold: int | float = comment_config.get(
+            required_changes = comment_config.get("require_bundle_changes", False)
+            required_changes_threshold = comment_config.get(
                 "bundle_change_threshold",
                 BundleThreshold("absolute", 0),
             )

services/bundle_analysis/notify/contexts/comment.py

@@ -42,18 +42,14 @@ class BundleAnalysisPRCommentNotificationContext(BaseBundleAnalysisNotificationC
 
     notification_type = NotificationType.PR_COMMENT
 
-    pull: EnrichedPull = NotificationContextField[EnrichedPull]()
-    bundle_analysis_comparison: BundleAnalysisComparison = NotificationContextField[
-        BundleAnalysisComparison
-    ]()
-    commit_status_level: CommitStatusLevel = NotificationContextField[
-        CommitStatusLevel
-    ]()
-    should_use_upgrade_comment: bool = NotificationContextField[bool]()
+    pull = NotificationContextField[EnrichedPull]()
+    bundle_analysis_comparison = NotificationContextField[BundleAnalysisComparison]()
+    commit_status_level = NotificationContextField[CommitStatusLevel]()
+    should_use_upgrade_comment = NotificationContextField[bool]()
 
 
 class BundleAnalysisPRCommentContextBuilder(NotificationContextBuilder):
-    fields_of_interest: tuple[str] = (
+    fields_of_interest: tuple[str, ...] = (
         "commit_report",
         "bundle_analysis_report",
         "user_config",
@@ -176,8 +172,7 @@ def evaluate_should_use_upgrade_message(self) -> Self:
                 )
                 if successful_activation:
                     schedule_new_user_activated_task(
-                        activate_seat_info.owner_id,
-                        activate_seat_info.author_id,
+                        activate_seat_info.owner_id, activate_seat_info.author_id
                     )
                     self._notification_context.should_use_upgrade_comment = False
                 else:
@@ -215,4 +210,4 @@ def build_context(self) -> Self:
         )
 
     def get_result(self) -> BundleAnalysisPRCommentNotificationContext:
-        return self._notification_context
+        return self._notification_context  # type: ignore

services/bundle_analysis/notify/contexts/commit_status.py

@@ -39,16 +39,12 @@
 class CommitStatusNotificationContext(BaseBundleAnalysisNotificationContext):
     notification_type = NotificationType.COMMIT_STATUS
 
-    pull: EnrichedPull | None = NotificationContextField[EnrichedPull | None]()
-    bundle_analysis_comparison: BundleAnalysisComparison = NotificationContextField[
-        BundleAnalysisComparison
-    ]()
-    commit_status_level: CommitStatusLevel = NotificationContextField[
-        CommitStatusLevel
-    ]()
-    commit_status_url: str = NotificationContextField[str]()
-    cache_ttl: int = NotificationContextField[int]()
-    should_use_upgrade_comment: bool = NotificationContextField[bool]()
+    pull = NotificationContextField[EnrichedPull | None]()
+    bundle_analysis_comparison = NotificationContextField[BundleAnalysisComparison]()
+    commit_status_level = NotificationContextField[CommitStatusLevel]()
+    commit_status_url = NotificationContextField[str]()
+    cache_ttl = NotificationContextField[int]()
+    should_use_upgrade_comment = NotificationContextField[bool]()
 
     @property
     def base_commit(self) -> Commit:
@@ -58,7 +54,7 @@ def base_commit(self) -> Commit:
 
 
 class CommitStatusNotificationContextBuilder(NotificationContextBuilder):
-    fields_of_interest: tuple[str] = (
+    fields_of_interest: tuple[str, ...] = (
         "commit_report",
         "bundle_analysis_report",
         "user_config",

services/bundle_analysis/notify/contexts/tests/test_comment_context.py

@@ -1,6 +1,7 @@
 from unittest.mock import MagicMock
 
 import pytest
+from shared.config import PATCH_CENTRIC_DEFAULT_CONFIG
 from shared.yaml import UserYaml
 
 from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
@@ -145,7 +146,11 @@ def test_evaluate_changes_fail(self, config, total_size_delta, dbsession, mocker
     @pytest.mark.parametrize(
         "config, total_size_delta",
         [
-            pytest.param({}, 100, id="default_config"),
+            pytest.param(
+                PATCH_CENTRIC_DEFAULT_CONFIG,
+                100,
+                id="default_config",
+            ),
             pytest.param(
                 {"comment": {"require_bundle_changes": False}},
                 100,
@@ -281,7 +286,7 @@ def test_build_context(self, dbsession, mocker, mock_storage):
         enriched_pull = get_enriched_pull_setting_up_mocks(
             dbsession, mocker, (head_commit, base_commit)
         )
-        user_yaml = UserYaml.from_dict({})
+        user_yaml = UserYaml.from_dict(PATCH_CENTRIC_DEFAULT_CONFIG)
         builder = BundleAnalysisPRCommentContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
         )
@@ -304,7 +309,7 @@ def test_build_context(self, dbsession, mocker, mock_storage):
 
     def test_initialize_from_context(self, dbsession, mocker):
         head_commit, _ = get_commit_pair(dbsession)
-        user_yaml = UserYaml.from_dict({})
+        user_yaml = UserYaml.from_dict(PATCH_CENTRIC_DEFAULT_CONFIG)
         builder = BundleAnalysisPRCommentContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
         )

services/bundle_analysis/notify/contexts/tests/test_commit_status_context.py

@@ -1,6 +1,7 @@
 from unittest.mock import MagicMock
 
 import pytest
+from shared.config import PATCH_CENTRIC_DEFAULT_CONFIG
 from shared.validation.types import BundleThreshold
 from shared.yaml import UserYaml
 
@@ -143,42 +144,51 @@ async def test_load_bundle_comparison_when_pull_is_none(
         "yaml_dict, percent_change, absolute_change, expected",
         [
             pytest.param(
-                {},
+                PATCH_CENTRIC_DEFAULT_CONFIG,
                 1.0,
                 10000,
                 CommitStatusLevel.INFO,
                 id="default_config_within_5%_change",
             ),
             pytest.param(
-                {},
+                PATCH_CENTRIC_DEFAULT_CONFIG,
                 5.5,
                 10000,
                 CommitStatusLevel.WARNING,
                 id="default_config_outside_5%_change",
             ),
             pytest.param(
-                {"bundle_analysis": {"warning_threshold": 10000}},
+                {
+                    **PATCH_CENTRIC_DEFAULT_CONFIG,
+                    "bundle_analysis": {"warning_threshold": 10000},
+                },
                 1.0,
                 10001,
                 CommitStatusLevel.WARNING,
                 id="informational_outside_range",
             ),
             pytest.param(
-                {"bundle_analysis": {"warning_threshold": 10000}},
+                {
+                    **PATCH_CENTRIC_DEFAULT_CONFIG,
+                    "bundle_analysis": {"warning_threshold": 10000},
+                },
                 1.0,
                 10000,
                 CommitStatusLevel.INFO,
                 id="informational_within_range",
             ),
             pytest.param(
-                {"bundle_analysis": {"warning_threshold": 10000, "status": True}},
+                {
+                    **PATCH_CENTRIC_DEFAULT_CONFIG,
+                    "bundle_analysis": {"warning_threshold": 10000, "status": True},
+                },
                 1.0,
                 10001,
                 CommitStatusLevel.ERROR,
                 id="fail_outside_range_absolute",
             ),
             pytest.param(
-                {"bundle_analysis": {"status": True}},
+                {**PATCH_CENTRIC_DEFAULT_CONFIG, "bundle_analysis": {"status": True}},
                 5.1,
                 10000,
                 CommitStatusLevel.ERROR,
@@ -191,6 +201,7 @@ def test_load_commit_status_level(
     ):
         head_commit = CommitFactory()
         dbsession.add(head_commit)
+        yaml_dict.update(PATCH_CENTRIC_DEFAULT_CONFIG)
         user_yaml = UserYaml.from_dict(yaml_dict)
         builder = CommitStatusNotificationContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
@@ -220,7 +231,7 @@ def test_build_context(self, dbsession, mocker, mock_storage):
         enriched_pull = get_enriched_pull_setting_up_mocks(
             dbsession, mocker, (head_commit, base_commit)
         )
-        user_yaml = UserYaml.from_dict({})
+        user_yaml = UserYaml.from_dict(PATCH_CENTRIC_DEFAULT_CONFIG)
         builder = CommitStatusNotificationContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
         )
@@ -252,7 +263,7 @@ def test_build_context(self, dbsession, mocker, mock_storage):
 
     def test_initialize_from_context(self, dbsession, mocker):
         head_commit, _ = get_commit_pair(dbsession)
-        user_yaml = UserYaml.from_dict({})
+        user_yaml = UserYaml.from_dict(PATCH_CENTRIC_DEFAULT_CONFIG)
         builder = CommitStatusNotificationContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
         )
@@ -330,7 +341,15 @@ def test_evaluate_should_use_upgrade_message(
             return_value=auto_activate_succeeds,
         )
         head_commit, _ = get_commit_pair(dbsession)
-        user_yaml = UserYaml.from_dict({})
+        user_yaml = UserYaml.from_dict(
+            {
+                "comment": {
+                    "layout": "reach,diff,flags,tree,reach",
+                    "behavior": "default",
+                    "show_carryforward_flags": False,
+                }
+            }
+        )
         builder = CommitStatusNotificationContextBuilder().initialize(
             head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
         )