Skip to content

Commit

Permalink
feat: upgrade message in BA commit status (#700)
Browse files Browse the repository at this point in the history
  • Loading branch information
giovanni-guidini committed Sep 11, 2024
1 parent ab427f8 commit dcea64a
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 10 deletions.
3 changes: 2 additions & 1 deletion services/bundle_analysis/notify/contexts/comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class BundleAnalysisPRCommentNotificationContext(BaseBundleAnalysisNotificationC
commit_status_level: CommitStatusLevel = NotificationContextField[
CommitStatusLevel
]()
should_use_upgrade_comment: bool
should_use_upgrade_comment: bool = NotificationContextField[bool]()


class BundleAnalysisPRCommentContextBuilder(NotificationContextBuilder):
Expand All @@ -59,6 +59,7 @@ class BundleAnalysisPRCommentContextBuilder(NotificationContextBuilder):
"user_config",
"pull",
"bundle_analysis_comparison",
"should_use_upgrade_comment",
)

def initialize(
Expand Down
45 changes: 38 additions & 7 deletions services/bundle_analysis/notify/contexts/commit_status.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from typing import Self

import sentry_sdk
from asgiref.sync import async_to_sync
from shared.bundle_analysis import (
Expand All @@ -7,6 +9,7 @@
from shared.yaml import UserYaml

from database.models.core import Commit
from services.activation import activate_user, schedule_new_user_activated_task
from services.bundle_analysis.comparison import ComparisonLoader
from services.bundle_analysis.exceptions import (
MissingBaseCommit,
Expand All @@ -29,6 +32,7 @@
EnrichedPull,
fetch_and_update_pull_request_information_from_commit,
)
from services.seats import ShouldActivateSeat, determine_seat_activation
from services.urls import get_bundle_analysis_pull_url, get_commit_url


Expand All @@ -44,6 +48,7 @@ class CommitStatusNotificationContext(BaseBundleAnalysisNotificationContext):
]()
commit_status_url: str = NotificationContextField[str]()
cache_ttl: int = NotificationContextField[int]()
should_use_upgrade_comment: bool = NotificationContextField[bool]()

@property
def base_commit(self) -> Commit:
Expand All @@ -59,11 +64,12 @@ class CommitStatusNotificationContextBuilder(NotificationContextBuilder):
"user_config",
"pull",
"bundle_analysis_comparison",
"should_use_upgrade_comment",
)

def initialize(
self, commit: Commit, current_yaml: UserYaml, gh_app_installation_name: str
) -> "CommitStatusNotificationContextBuilder":
) -> Self:
self.current_yaml = current_yaml
self._notification_context = CommitStatusNotificationContext(
commit=commit,
Expand All @@ -74,7 +80,7 @@ def initialize(
@sentry_sdk.trace
async def load_optional_enriched_pull(
self,
) -> "CommitStatusNotificationContextBuilder":
) -> Self:
"""Loads an optional EnrichedPull into the NotificationContext.
EnrichedPull includes updated info from the git provider and info saved in the database.
If the value is None it's because the commit is not in a Pull Request
Expand All @@ -94,7 +100,7 @@ async def load_optional_enriched_pull(
@sentry_sdk.trace
def load_bundle_comparison(
self,
) -> "CommitStatusNotificationContextBuilder":
) -> Self:
"""Loads the BundleAnalysisComparison into the NotificationContext.
BundleAnalysisComparison is the diff between 2 BundleAnalysisReports.
IF pull is not None, comparison is pull's BASE vs HEAD
Expand Down Expand Up @@ -127,7 +133,7 @@ def load_bundle_comparison(
"load_bundle_comparison", detail=exp.__class__.__name__
)

def load_commit_status_level(self) -> "CommitStatusNotificationContextBuilder":
def load_commit_status_level(self) -> Self:
bundle_analysis_comparison = (
self._notification_context.bundle_analysis_comparison
)
Expand All @@ -143,7 +149,7 @@ def load_commit_status_level(self) -> "CommitStatusNotificationContextBuilder":
self._notification_context.commit_status_level = CommitStatusLevel.ERROR
return self

def load_commit_status_url(self) -> "CommitStatusNotificationContextBuilder":
def load_commit_status_url(self) -> Self:
if self._notification_context.pull:
self._notification_context.commit_status_url = get_bundle_analysis_pull_url(
self._notification_context.pull.database_pull
Expand All @@ -154,20 +160,45 @@ def load_commit_status_url(self) -> "CommitStatusNotificationContextBuilder":
)
return self

def load_cache_ttl(self) -> "CommitStatusNotificationContextBuilder":
def load_cache_ttl(self) -> Self:
self._notification_context.cache_ttl = int(
# using `get_config` instead of `current_yaml` because
# `current_yaml` does not include the install configuration
get_config("setup", "cache", "send_status_notification", default=600)
) # 10 min default
return self

def build_context(self) -> "CommitStatusNotificationContextBuilder":
@sentry_sdk.trace
def evaluate_should_use_upgrade_message(self) -> Self:
activate_seat_info = determine_seat_activation(self._notification_context.pull)
match activate_seat_info.should_activate_seat:
case ShouldActivateSeat.AUTO_ACTIVATE:
successful_activation = activate_user(
db_session=self._notification_context.commit.get_db_session(),
org_ownerid=activate_seat_info.owner_id,
user_ownerid=activate_seat_info.author_id,
)
if successful_activation:
schedule_new_user_activated_task(
activate_seat_info.owner_id,
activate_seat_info.author_id,
)
self._notification_context.should_use_upgrade_comment = False
else:
self._notification_context.should_use_upgrade_comment = True
case ShouldActivateSeat.MANUAL_ACTIVATE:
self._notification_context.should_use_upgrade_comment = True
case ShouldActivateSeat.NO_ACTIVATE:
self._notification_context.should_use_upgrade_comment = False
return self

def build_context(self) -> Self:
super().build_context()
async_to_sync(self.load_optional_enriched_pull)()
return (
self.load_bundle_comparison()
.load_commit_status_level()
.evaluate_should_use_upgrade_message()
.load_commit_status_url()
.load_cache_ttl()
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
CommitStatusNotificationContextBuilder,
)
from services.bundle_analysis.notify.types import NotificationUserConfig
from services.seats import SeatActivationInfo, ShouldActivateSeat


class TestBundleAnalysisPRCommentNotificationContext:
Expand Down Expand Up @@ -287,3 +288,46 @@ def test_initialize_from_context(self, dbsession, mocker):
assert context.bundle_analysis_report == other_context.bundle_analysis_report
assert context.pull == other_context.pull
assert other_context.bundle_analysis_comparison == fake_comparison

@pytest.mark.parametrize(
"activation_result, auto_activate_succeeds, expected",
[
(ShouldActivateSeat.AUTO_ACTIVATE, True, False),
(ShouldActivateSeat.AUTO_ACTIVATE, False, True),
(ShouldActivateSeat.MANUAL_ACTIVATE, False, True),
(ShouldActivateSeat.NO_ACTIVATE, False, False),
],
)
def test_evaluate_should_use_upgrade_message(
self, activation_result, dbsession, auto_activate_succeeds, expected, mocker
):
activation_result = SeatActivationInfo(
should_activate_seat=activation_result,
owner_id=1,
author_id=10,
reason="mocked",
)
mocker.patch(
"services.bundle_analysis.notify.contexts.commit_status.determine_seat_activation",
return_value=activation_result,
)
mocker.patch(
"services.bundle_analysis.notify.contexts.commit_status.activate_user",
return_value=auto_activate_succeeds,
)
mocker.patch(
"services.bundle_analysis.notify.contexts.commit_status.schedule_new_user_activated_task",
return_value=auto_activate_succeeds,
)
head_commit, _ = get_commit_pair(dbsession)
user_yaml = UserYaml.from_dict({})
builder = CommitStatusNotificationContextBuilder().initialize(
head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
)
mock_pull = MagicMock(
name="fake_pull",
database_pull=MagicMock(bundle_analysis_commentid=12345, id=12),
)
builder._notification_context.pull = mock_pull
builder.evaluate_should_use_upgrade_message()
assert builder._notification_context.should_use_upgrade_comment == expected
17 changes: 16 additions & 1 deletion services/bundle_analysis/notify/messages/commit_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,16 @@ class BundleCommentTemplateContext(TypedDict):


class CommitStatusMessageStrategy(MessageStrategyInterface):
@sentry_sdk.trace
def build_message(self, context: CommitStatusNotificationContext) -> str | bytes:
if context.should_use_upgrade_comment:
return self.build_upgrade_message(context)
else:
return self.build_default_message(context)

@sentry_sdk.trace
def build_default_message(
self, context: CommitStatusNotificationContext
) -> str | bytes:
template = loader.get_template(
"bundle_analysis_notify/commit_status_summary.md"
)
Expand Down Expand Up @@ -61,6 +69,13 @@ def build_message(self, context: CommitStatusNotificationContext) -> str | bytes
)
return template.render(context)

@sentry_sdk.trace
def build_upgrade_message(self, context: CommitStatusNotificationContext) -> str:
author_username = context.pull.provider_pull["author"].get("username")
return (
f"Please activate user {author_username} to display a detailed status check"
)

def _cache_key(self, context: CommitStatusNotificationContext) -> str:
return make_hash_sha256(
dict(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from unittest.mock import AsyncMock
from unittest.mock import AsyncMock, MagicMock

import pytest
from asgiref.sync import async_to_sync
Expand All @@ -7,19 +7,22 @@
from shared.yaml import UserYaml

from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
from database.tests.factories.core import PullFactory
from services.bundle_analysis.notify.conftest import (
get_commit_pair,
get_enriched_pull_setting_up_mocks,
get_report_pair,
save_mock_bundle_analysis_report,
)
from services.bundle_analysis.notify.contexts.commit_status import (
CommitStatusNotificationContext,
CommitStatusNotificationContextBuilder,
)
from services.bundle_analysis.notify.messages.commit_status import (
CommitStatusMessageStrategy,
)
from services.notification.notifiers.base import NotificationResult
from services.seats import SeatActivationInfo, ShouldActivateSeat


class FakeRedis(object):
Expand Down Expand Up @@ -181,6 +184,12 @@ def _setup_send_message_tests(
"services.bundle_analysis.notify.contexts.commit_status.fetch_and_update_pull_request_information_from_commit",
return_value=None,
)
mocker.patch(
"services.bundle_analysis.notify.contexts.commit_status.determine_seat_activation",
return_value=SeatActivationInfo(
should_activate_seat=ShouldActivateSeat.NO_ACTIVATE
),
)
user_yaml = UserYaml.from_dict({})
builder = CommitStatusNotificationContextBuilder().initialize(
head_commit, user_yaml, GITHUB_APP_INSTALLATION_DEFAULT_NAME
Expand Down Expand Up @@ -264,3 +273,22 @@ def test_skip_payload_unchanged(self, dbsession, mocker, mock_storage, mock_cach
notification_successful=False,
explanation="payload_unchanged",
)


class TestCommitStatusUpgradeMessage:
def test_build_upgrade_message(self, dbsession):
head_commit, _ = get_commit_pair(dbsession)
context = CommitStatusNotificationContext(
head_commit, GITHUB_APP_INSTALLATION_DEFAULT_NAME
)
context.should_use_upgrade_comment = True
context.pull = MagicMock(
name="fake_pull",
database_pull=PullFactory(),
provider_pull={"author": {"username": "PR_author_username"}},
)
message = CommitStatusMessageStrategy().build_message(context)
assert (
message
== "Please activate user PR_author_username to display a detailed status check"
)

0 comments on commit dcea64a

Please sign in to comment.