From ab61316479d3c32e2db4efeb458f844a62766cdf Mon Sep 17 00:00:00 2001 From: Gguidini Date: Thu, 29 Aug 2024 11:38:26 +0200 Subject: [PATCH 1/2] feat: count seats for BA notify closes: https://github.com/codecov/engineering-team/issues/2254 Aligning with coverage and test analytics, PR comment for BA will take up seats now. --- services/activation.py | 22 +++++++ .../new_notify/contexts/comment.py | 44 ++++++++++--- .../contexts/tests/test_comment_context.py | 44 +++++++++++++ .../new_notify/messages/comment.py | 39 +++++++++++- .../bundle_analysis_notify/upgrade_comment.md | 8 +++ .../new_notify/messages/tests/test_comment.py | 62 +++++++++++++++++++ 6 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md diff --git a/services/activation.py b/services/activation.py index 0145d8d68..52e050453 100644 --- a/services/activation.py +++ b/services/activation.py @@ -1,8 +1,13 @@ import logging +from shared.celery_config import ( + activate_account_user_task_name, + new_user_activated_task_name, +) from sqlalchemy import func from sqlalchemy.sql import text +from app import celery_app from services.license import ( calculate_reason_for_not_being_valid, get_current_license, @@ -97,3 +102,20 @@ 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): + celery_app.send_task( + new_user_activated_task_name, + args=None, + kwargs=dict(org_ownerid=org_ownerid, user_ownerid=user_ownerid), + ) + # Activate the account user if it exists. + celery_app.send_task( + activate_account_user_task_name, + args=None, + kwargs=dict( + user_ownerid=user_ownerid, + org_ownerid=org_ownerid, + ), + ) diff --git a/services/bundle_analysis/new_notify/contexts/comment.py b/services/bundle_analysis/new_notify/contexts/comment.py index f191f1ae0..4925a8e52 100644 --- a/services/bundle_analysis/new_notify/contexts/comment.py +++ b/services/bundle_analysis/new_notify/contexts/comment.py @@ -1,4 +1,5 @@ import logging +from typing import Self import sentry_sdk from asgiref.sync import async_to_sync @@ -8,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, @@ -29,6 +31,7 @@ EnrichedPull, fetch_and_update_pull_request_information_from_commit, ) +from services.seats import ShouldActivateSeat, determine_seat_activation log = logging.getLogger(__name__) @@ -42,6 +45,7 @@ class BundleAnalysisPRCommentNotificationContext(BaseBundleAnalysisNotificationC bundle_analysis_comparison: BundleAnalysisComparison = NotificationContextField[ BundleAnalysisComparison ]() + should_use_upgrade_comment: bool class BundleAnalysisPRCommentContextBuilder(NotificationContextBuilder): @@ -64,7 +68,7 @@ def initialize( return self @sentry_sdk.trace - async def load_enriched_pull(self) -> "BundleAnalysisPRCommentContextBuilder": + async def load_enriched_pull(self) -> Self: """Loads the EnrichedPull into the NotificationContext. EnrichedPull includes updated info from the git provider and info saved in the database. Raises: @@ -87,7 +91,7 @@ async def load_enriched_pull(self) -> "BundleAnalysisPRCommentContextBuilder": return self @sentry_sdk.trace - def load_bundle_comparison(self) -> "BundleAnalysisPRCommentContextBuilder": + def load_bundle_comparison(self) -> Self: """Loads the BundleAnalysisComparison into the NotificationContext. BundleAnalysisComparison is the diff between 2 BundleAnalysisReports, respectively the one for the pull's base and one for the pull's head. @@ -112,7 +116,7 @@ def load_bundle_comparison(self) -> "BundleAnalysisPRCommentContextBuilder": "load_bundle_comparison", detail=exp.__class__.__name__ ) - def evaluate_has_enough_changes(self) -> "BundleAnalysisPRCommentContextBuilder": + def evaluate_has_enough_changes(self) -> Self: """Evaluates if the NotificationContext includes enough changes to send the notification. Configuration is done via UserYaml. If a comment was previously made for this PR the required changes are bypassed so that we @@ -155,12 +159,38 @@ def evaluate_has_enough_changes(self) -> "BundleAnalysisPRCommentContextBuilder" raise NotificationContextBuildError("evaluate_has_enough_changes") return self - def build_context(self) -> "BundleAnalysisPRCommentContextBuilder": + @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_enriched_pull)() - self.load_bundle_comparison() - self.evaluate_has_enough_changes() - return self + return ( + self.load_bundle_comparison() + .evaluate_has_enough_changes() + .evaluate_should_use_upgrade_message() + ) def get_result(self) -> BundleAnalysisPRCommentNotificationContext: return self._notification_context diff --git a/services/bundle_analysis/new_notify/contexts/tests/test_comment_context.py b/services/bundle_analysis/new_notify/contexts/tests/test_comment_context.py index 58c37d5d3..616a35d1f 100644 --- a/services/bundle_analysis/new_notify/contexts/tests/test_comment_context.py +++ b/services/bundle_analysis/new_notify/contexts/tests/test_comment_context.py @@ -19,6 +19,7 @@ from services.bundle_analysis.new_notify.contexts.comment import ( BundleAnalysisPRCommentContextBuilder, ) +from services.seats import SeatActivationInfo, ShouldActivateSeat class TestBundleAnalysisPRCommentNotificationContext: @@ -222,6 +223,49 @@ def test_evaluate_changes_comment_exists(self, dbsession): result = builder.evaluate_has_enough_changes() assert result == builder + @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.new_notify.contexts.comment.determine_seat_activation", + return_value=activation_result, + ) + mocker.patch( + "services.bundle_analysis.new_notify.contexts.comment.activate_user", + return_value=auto_activate_succeeds, + ) + mocker.patch( + "services.bundle_analysis.new_notify.contexts.comment.schedule_new_user_activated_task", + return_value=auto_activate_succeeds, + ) + head_commit, _ = get_commit_pair(dbsession) + user_yaml = UserYaml.from_dict({}) + builder = BundleAnalysisPRCommentContextBuilder().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 + def test_build_context(self, dbsession, mocker, mock_storage): head_commit, base_commit = get_commit_pair(dbsession) repository = head_commit.repository diff --git a/services/bundle_analysis/new_notify/messages/comment.py b/services/bundle_analysis/new_notify/messages/comment.py index 80359ee37..407aeb7b9 100644 --- a/services/bundle_analysis/new_notify/messages/comment.py +++ b/services/bundle_analysis/new_notify/messages/comment.py @@ -1,5 +1,5 @@ import logging -from typing import TypedDict +from typing import Literal, TypedDict import sentry_sdk from django.template import loader @@ -17,8 +17,9 @@ get_github_app_used, ) from services.bundle_analysis.new_notify.messages import MessageStrategyInterface +from services.license import requires_license from services.notification.notifiers.base import NotificationResult -from services.urls import get_bundle_analysis_pull_url +from services.urls import get_bundle_analysis_pull_url, get_members_url log = logging.getLogger(__name__) @@ -37,9 +38,26 @@ class BundleCommentTemplateContext(TypedDict): bundle_rows: list[BundleRow] +class UpgradeCommentTemplateContext(TypedDict): + author_username: str + codecov_instance: Literal["cloud"] | Literal["self_hosted"] + codecov_name: Literal["Codecov"] | Literal["your instance of Codecov"] + activation_link: str + + class BundleAnalysisCommentMarkdownStrategy(MessageStrategyInterface): + def build_message( + self, context: BundleAnalysisPRCommentNotificationContext + ) -> 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_message(self, context: BundleAnalysisPRCommentNotificationContext) -> str: + def build_default_message( + self, context: BundleAnalysisPRCommentNotificationContext + ) -> str: template = loader.get_template("bundle_analysis_notify/bundle_comment.md") total_size_delta = context.bundle_analysis_comparison.total_size_delta context = BundleCommentTemplateContext( @@ -50,6 +68,21 @@ def build_message(self, context: BundleAnalysisPRCommentNotificationContext) -> ) return template.render(context) + @sentry_sdk.trace + def build_upgrade_message( + self, context: BundleAnalysisPRCommentNotificationContext + ) -> str: + template = loader.get_template("bundle_analysis_notify/upgrade_comment.md") + context = UpgradeCommentTemplateContext( + activation_link=get_members_url(context.pull.database_pull), + codecov_instance="self_hosted" if requires_license() else "cloud", + codecov_name="your instance of Codecov" + if requires_license() + else "Codecov", + author_username=context.pull.provider_pull["author"].get("username"), + ) + return template.render(context) + @sentry_sdk.trace async def send_message( self, context: BundleAnalysisPRCommentNotificationContext, message: str diff --git a/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md b/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md new file mode 100644 index 000000000..1c5fc0b72 --- /dev/null +++ b/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md @@ -0,0 +1,8 @@ +The author of this PR, {{author_username}}, {% if codecov_instance == "cloud" %}is not an activated member of this organization on Codecov.{% else %}is not activated in your Codecov Self-Hosted installation.{% endif %} +Please [activate this user]({{activation_link}}) to display this PR comment. +Bundle data is still being uploaded to {{codecov_name}} for purposes of overall calculations. +{% if codecov_instance == "cloud" %} +Please don't hesitate to email us at support@codecov.io with any questions. +{% else %} +Please contact your Codecov On-Premises installation administrator with any questions. +{% endif %} \ No newline at end of file diff --git a/services/bundle_analysis/new_notify/messages/tests/test_comment.py b/services/bundle_analysis/new_notify/messages/tests/test_comment.py index 117a00362..3c703948c 100644 --- a/services/bundle_analysis/new_notify/messages/tests/test_comment.py +++ b/services/bundle_analysis/new_notify/messages/tests/test_comment.py @@ -1,3 +1,4 @@ +from textwrap import dedent from unittest.mock import AsyncMock, MagicMock import pytest @@ -8,6 +9,7 @@ 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.new_notify.conftest import ( get_commit_pair, get_enriched_pull_setting_up_mocks, @@ -300,3 +302,63 @@ async def test_send_message_fail(self, dbsession, mocker): notification_successful=False, explanation="TorngitClientError", ) + + +class TestUpgradeMessage: + def test_build_upgrade_message_cloud(self, dbsession, mocker): + head_commit, _ = get_commit_pair(dbsession) + context = BundleAnalysisPRCommentNotificationContext( + 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"}}, + ) + mocker.patch( + "services.bundle_analysis.new_notify.messages.comment.requires_license", + return_value=False, + ) + mocker.patch( + "services.bundle_analysis.new_notify.messages.comment.get_members_url", + return_value="http://members_url", + ) + strategy = BundleAnalysisCommentMarkdownStrategy() + result = strategy.build_message(context) + assert result == dedent("""\ + The author of this PR, PR_author_username, is not an activated member of this organization on Codecov. + Please [activate this user](http://members_url) to display this PR comment. + Bundle data is still being uploaded to Codecov for purposes of overall calculations. + + Please don't hesitate to email us at support@codecov.io with any questions. + """) + + def test_build_upgrade_message_self_hosted(self, dbsession, mocker): + head_commit, _ = get_commit_pair(dbsession) + context = BundleAnalysisPRCommentNotificationContext( + 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"}}, + ) + mocker.patch( + "services.bundle_analysis.new_notify.messages.comment.requires_license", + return_value=True, + ) + mocker.patch( + "services.bundle_analysis.new_notify.messages.comment.get_members_url", + return_value="http://members_url", + ) + strategy = BundleAnalysisCommentMarkdownStrategy() + result = strategy.build_message(context) + assert result == dedent("""\ + The author of this PR, PR_author_username, is not activated in your Codecov Self-Hosted installation. + Please [activate this user](http://members_url) to display this PR comment. + Bundle data is still being uploaded to your instance of Codecov for purposes of overall calculations. + + Please contact your Codecov On-Premises installation administrator with any questions. + """) From a607d0aa94150cfc03ba57a24d68ef9fe468596a Mon Sep 17 00:00:00 2001 From: Gguidini Date: Thu, 29 Aug 2024 14:02:40 +0200 Subject: [PATCH 2/2] address feedback: move more logic to template --- .../bundle_analysis/new_notify/messages/comment.py | 10 +++------- .../bundle_analysis_notify/upgrade_comment.md | 6 +++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/services/bundle_analysis/new_notify/messages/comment.py b/services/bundle_analysis/new_notify/messages/comment.py index 407aeb7b9..b5bc0f085 100644 --- a/services/bundle_analysis/new_notify/messages/comment.py +++ b/services/bundle_analysis/new_notify/messages/comment.py @@ -1,5 +1,5 @@ import logging -from typing import Literal, TypedDict +from typing import TypedDict import sentry_sdk from django.template import loader @@ -40,8 +40,7 @@ class BundleCommentTemplateContext(TypedDict): class UpgradeCommentTemplateContext(TypedDict): author_username: str - codecov_instance: Literal["cloud"] | Literal["self_hosted"] - codecov_name: Literal["Codecov"] | Literal["your instance of Codecov"] + is_saas: bool activation_link: str @@ -75,10 +74,7 @@ def build_upgrade_message( template = loader.get_template("bundle_analysis_notify/upgrade_comment.md") context = UpgradeCommentTemplateContext( activation_link=get_members_url(context.pull.database_pull), - codecov_instance="self_hosted" if requires_license() else "cloud", - codecov_name="your instance of Codecov" - if requires_license() - else "Codecov", + is_saas=not requires_license(), author_username=context.pull.provider_pull["author"].get("username"), ) return template.render(context) diff --git a/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md b/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md index 1c5fc0b72..bfb8aa52b 100644 --- a/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md +++ b/services/bundle_analysis/new_notify/messages/templates/bundle_analysis_notify/upgrade_comment.md @@ -1,7 +1,7 @@ -The author of this PR, {{author_username}}, {% if codecov_instance == "cloud" %}is not an activated member of this organization on Codecov.{% else %}is not activated in your Codecov Self-Hosted installation.{% endif %} +The author of this PR, {{author_username}}, {% if is_saas %}is not an activated member of this organization on Codecov.{% else %}is not activated in your Codecov Self-Hosted installation.{% endif %} Please [activate this user]({{activation_link}}) to display this PR comment. -Bundle data is still being uploaded to {{codecov_name}} for purposes of overall calculations. -{% if codecov_instance == "cloud" %} +Bundle data is still being uploaded to {{ is_saas|yesno:"Codecov,your instance of Codecov"}} for purposes of overall calculations. +{% if is_saas %} Please don't hesitate to email us at support@codecov.io with any questions. {% else %} Please contact your Codecov On-Premises installation administrator with any questions.