Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Logic to remove checks from team plan customer #463

Merged
merged 6 commits into from
May 23, 2024
Merged

Logic to remove checks from team plan customer #463

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
76c902d
Logic to remove checks from team plan customer
adrian-codecov May 22, 2024
d6791e8
refactored status + checks notifications for team plan case
adrian-codecov May 22, 2024
c5adbb8
encapsulated logic for readability
adrian-codecov May 23, 2024
972c3c9
Merge branch 'main' of github.com:codecov/worker into 1303-no-checks-…
adrian-codecov May 23, 2024
1673479
rework logic for cleanliness
adrian-codecov May 23, 2024
e8ba0de
Mkerge branch 'main' of github.com:codecov/worker into 1303-no-checks…
adrian-codecov May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 57 additions & 29 deletions services/notification/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@
from celery.exceptions import CeleryError, SoftTimeLimitExceeded
from shared.config import get_config
from shared.helpers.yaml import default_if_true
from shared.plan.constants import TEAM_PLAN_REPRESENTATIONS
from shared.yaml import UserYaml

from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME, Owner
from helpers.metrics import metrics
from services.comparison import ComparisonProxy
from services.decoration import Decoration
Expand All @@ -24,6 +25,7 @@
create_or_update_commit_notification_from_notification_result,
)
from services.notification.notifiers import (
StatusType,
get_all_notifier_classes_mapping,
get_pull_request_notifiers,
get_status_notifier_class,
Expand Down Expand Up @@ -55,15 +57,28 @@
self.decoration_type = decoration_type
self.gh_installation_name_to_use = gh_installation_name_to_use

def _should_use_checks_notifier(self) -> bool:
def _should_use_status_notifier(self, status_type: StatusType) -> bool:
owner: Owner = self.repository.owner

if owner.plan in TEAM_PLAN_REPRESENTATIONS:
if status_type != StatusType.PATCH.value:
return False

return True

def _should_use_checks_notifier(self, status_type: StatusType) -> bool:
checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",))
if checks_yaml_field is False:
return False

owner = self.repository.owner
owner: Owner = self.repository.owner
if owner.service not in ["github", "github_enterprise"]:
return False

if owner.plan in TEAM_PLAN_REPRESENTATIONS:
if status_type != StatusType.PATCH.value:
return False

app_installation_filter = filter(
lambda obj: (
obj.name == self.gh_installation_name_to_use and obj.is_configured()
Expand Down Expand Up @@ -101,39 +116,52 @@

current_flags = [rf.flag_name for rf in self.repository.flags if not rf.deleted]
for key, title, status_config in self.get_statuses(current_flags):
status_notifier_class = get_status_notifier_class(key, "status")
if self._should_use_checks_notifier():
checks_notifier = get_status_notifier_class(key, "checks")
yield ChecksWithFallback(
checks_notifier=checks_notifier(
repository=self.repository,
title=title,
notifier_yaml_settings=status_config,
notifier_site_settings={},
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
gh_installation_name_to_use=self.gh_installation_name_to_use,
),
status_notifier=status_notifier_class(
should_use_status_notifiers = self._should_use_status_notifier(
status_type=key
)
should_use_checks_notifier = self._should_use_checks_notifier(
status_type=key
)

# Introduced to gate team plan not having statuses nor checks based on conditionals.
if not should_use_status_notifiers and not should_use_checks_notifier:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering if this is meant to skip the comment and slack notifiers, if not, or if they will be skipped anyways, then we can remove this if statement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have the comment, clarifying about the slack one, but how would removing this help?

To clarify the logic here, everything remains the same, the only change introduced by this ticket is "if you're in a team plan, and you have a "changes" or "project" status checks, we don't show them, we only show "patch" specific comments" (the ticket was originally "skip everything" but its now "skip some things". There was a fn _should_use_checks_notifier that has some logic in it already that decides whether you show checks or not - I added a small case inside to do the team plan changes. We also always sent statuses, which changes with this ticket, so I added a fn _should_use_status_notifier which just adds the team plan related logic to it, otherwise returns true.

So I don't see a way to get rid of this if ^ here.

continue

Check warning on line 128 in services/notification/__init__.py

View check run for this annotation

Codecov Public QA / codecov/patch

services/notification/__init__.py#L128 

Added line #L128 was not covered by tests

Check warning on line 128 in services/notification/__init__.py

View check run for this annotation

Codecov - QA / codecov/patch

services/notification/__init__.py#L128 

Added line #L128 was not covered by tests

Check warning on line 128 in services/notification/__init__.py

View check run for this annotation

Codecov Notifications / codecov/patch

services/notification/__init__.py#L128 

Added line #L128 was not covered by tests

# We always send statuses, so there's currently no case of checks without status.
if should_use_status_notifiers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for readability maybe the logic following here should be encapsulated in a function

status_notifier_class = get_status_notifier_class(key, "status")
if should_use_checks_notifier:
checks_notifier = get_status_notifier_class(key, "checks")
yield ChecksWithFallback(
checks_notifier=checks_notifier(
repository=self.repository,
title=title,
notifier_yaml_settings=status_config,
notifier_site_settings={},
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
gh_installation_name_to_use=self.gh_installation_name_to_use,
),
status_notifier=status_notifier_class(
repository=self.repository,
title=title,
notifier_yaml_settings=status_config,
notifier_site_settings={},
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
gh_installation_name_to_use=self.gh_installation_name_to_use,
),
)
else:
yield status_notifier_class(
repository=self.repository,
title=title,
notifier_yaml_settings=status_config,
notifier_site_settings={},
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
gh_installation_name_to_use=self.gh_installation_name_to_use,
),
)
else:
yield status_notifier_class(
repository=self.repository,
title=title,
notifier_yaml_settings=status_config,
notifier_site_settings={},
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
gh_installation_name_to_use=self.gh_installation_name_to_use,
)
)

# yield notifier if slack_app field is True, nonexistent, or a non-empty dict
slack_app_yaml_field = read_yaml_field(self.current_yaml, ("slack_app",), True)
Expand Down
19 changes: 13 additions & 6 deletions services/notification/notifiers/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from enum import Enum
from typing import Dict, List, Type

from services.notification.notifiers.base import AbstractBaseNotifier
Expand Down Expand Up @@ -29,20 +30,26 @@ def get_all_notifier_classes_mapping() -> Dict[str, Type[AbstractBaseNotifier]]:
}


class StatusType(Enum):
PATCH = "patch"
PROJECT = "project"
CHANGES = "changes"


def get_status_notifier_class(
status_type: str, class_type: str = "status"
) -> Type[AbstractBaseNotifier]:
if status_type == "patch" and class_type == "checks":
if status_type == StatusType.PATCH.value and class_type == "checks":
return PatchChecksNotifier
if status_type == "project" and class_type == "checks":
if status_type == StatusType.PROJECT.value and class_type == "checks":
return ProjectChecksNotifier
if status_type == "changes" and class_type == "checks":
if status_type == StatusType.CHANGES.value and class_type == "checks":
return ChangesChecksNotifier
if status_type == "patch" and class_type == "status":
if status_type == StatusType.PATCH.value and class_type == "status":
return PatchStatusNotifier
if status_type == "project" and class_type == "status":
if status_type == StatusType.PROJECT.value and class_type == "status":
return ProjectStatusNotifier
if status_type == "changes" and class_type == "status":
if status_type == StatusType.CHANGES.value and class_type == "status":
return ChangesStatusNotifier


Expand Down
111 changes: 106 additions & 5 deletions services/notification/tests/unit/test_notification_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import mock
import pytest
from celery.exceptions import SoftTimeLimitExceeded
from shared.plan.constants import PlanName
from shared.reports.resources import Report, ReportFile, ReportLine
from shared.yaml import UserYaml

Expand All @@ -17,6 +18,7 @@
from services.comparison import ComparisonProxy
from services.comparison.types import Comparison, EnrichedPull, FullCommit
from services.notification import NotificationService
from services.notification.notifiers import StatusType
from services.notification.notifiers.base import NotificationResult
from services.notification.notifiers.checks import ProjectChecksNotifier
from services.notification.notifiers.checks.checks_with_fallback import (
Expand Down Expand Up @@ -58,7 +60,10 @@ def test_should_use_checks_notifier_yaml_field_false(self, dbsession):
repository = RepositoryFactory.create()
current_yaml = {"github_checks": False}
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == False
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== False
)

@pytest.mark.parametrize(
"repo_data,outcome",
Expand Down Expand Up @@ -104,7 +109,10 @@ def test_should_use_checks_notifier_deprecated_flow(
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == []
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == outcome
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== outcome
)

def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession):
repository = RepositoryFactory.create(owner__service="github")
Expand All @@ -119,7 +127,94 @@ def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession):
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == True
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== True
)

def test_use_checks_notifier_for_team_plan(self, dbsession):
repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.TEAM_MONTHLY.value
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
installation_id=456789,
owner=repository.owner,
repository_service_ids=None,
)
dbsession.add(ghapp_installation)
dbsession.flush()
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== False
)
assert (
service._should_use_checks_notifier(status_type=StatusType.CHANGES.value)
== False
)
assert (
service._should_use_checks_notifier(status_type=StatusType.PATCH.value)
== True
)

def test_use_status_notifier_for_team_plan(self, dbsession):
repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.TEAM_MONTHLY.value
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
installation_id=456789,
owner=repository.owner,
repository_service_ids=None,
)
dbsession.add(ghapp_installation)
dbsession.flush()
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert (
service._should_use_status_notifier(status_type=StatusType.PROJECT.value)
== False
)
assert (
service._should_use_checks_notifier(status_type=StatusType.CHANGES.value)
== False
)
assert (
service._should_use_checks_notifier(status_type=StatusType.PATCH.value)
== True
)

def test_use_status_notifier_for_non_team_plan(self, dbsession):
repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.CODECOV_PRO_MONTHLY.value
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
installation_id=456789,
owner=repository.owner,
repository_service_ids=None,
)
dbsession.add(ghapp_installation)
dbsession.flush()
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert (
service._should_use_status_notifier(status_type=StatusType.PROJECT.value)
== True
)
assert (
service._should_use_checks_notifier(status_type=StatusType.CHANGES.value)
== True
)
assert (
service._should_use_checks_notifier(status_type=StatusType.PATCH.value)
== True
)

@pytest.mark.parametrize(
"gh_installation_name",
Expand All @@ -145,9 +240,15 @@ def test_should_use_checks_notifier_ghapp_some_repos_covered(
service = NotificationService(
repository, current_yaml, gh_installation_name_to_use=gh_installation_name
)
assert service._should_use_checks_notifier() == True
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== True
)
service = NotificationService(other_repo_same_owner, current_yaml)
assert service._should_use_checks_notifier() == False
assert (
service._should_use_checks_notifier(status_type=StatusType.PROJECT.value)
== False
)

def test_get_notifiers_instances_only_third_party(
self, dbsession, mock_configuration
Expand Down
Loading