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.

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

Worker: Migrate to Plan / Tier Tables #1007

Merged
merged 20 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 3 additions & 1 deletion database/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ class Account(CodecovBaseModel, MixinBaseClassNoExternalID):
name = Column(types.String(100), nullable=False, unique=True)
is_active = Column(types.Boolean, nullable=False, default=True)
plan = Column(
types.String(50), nullable=False, default=PlanName.BASIC_PLAN_NAME.value
types.String(50),
nullable=False,
default=PlanName.BASIC_PLAN_NAME.value, # TODO: UPDATE WITH NEW FREE PLAN NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

)
plan_seat_count = Column(types.SmallInteger, nullable=False, default=1)
free_seat_count = Column(types.SmallInteger, nullable=False, default=0)
Expand Down
32 changes: 31 additions & 1 deletion database/tests/factories/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

import factory
from factory import Factory
from shared.plan.constants import PlanName
from shared.django_apps.codecov_auth.models import Plan, Tier
from shared.plan.constants import PlanName, TierName

from database import enums, models
from services.encryption import encryptor
Expand Down Expand Up @@ -298,3 +299,32 @@ class Meta:

key = ""
value = ""


class TierFactory(Factory):
class Meta:
model = Tier

tier_name = TierName.BASIC.value
bundle_analysis = False
test_analytics = False
flaky_test_detection = False
project_coverage = False
private_repo_support = False


class PlanFactory(Factory):
class Meta:
model = Plan

tier = factory.SubFactory(TierFactory)
base_unit_price = 0
benefits = factory.LazyFunction(lambda: ["Benefit 1", "Benefit 2", "Benefit 3"])
billing_rate = None
is_active = True
marketing_name = factory.Faker("catch_phrase")
max_seats = 1
monthly_uploads_limit = None
name = PlanName.BASIC_PLAN_NAME.value
paid_plan = False
stripe_id = None
2 changes: 1 addition & 1 deletion requirements.in
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
https://github.com/codecov/test-results-parser/archive/190bbc8a911099749928e13d5fe57f6027ca1e74.tar.gz#egg=test-results-parser
https://github.com/codecov/shared/archive/191837f5e35f5efc410e670aac7e50e0d09e43e1.tar.gz#egg=shared
https://github.com/codecov/shared/archive/52fdc2d14fa21d4c476d3264b021e42390b30c5f.tar.gz#egg=shared
https://github.com/codecov/timestring/archive/d37ceacc5954dff3b5bd2f887936a98a668dda42.tar.gz#egg=timestring
asgiref>=3.7.2
analytics-python==1.3.0b1
Expand Down
29 changes: 17 additions & 12 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# This file was autogenerated by uv via the following command:
# uv pip compile requirements.in -o requirements.txt
#
# This file is autogenerated by pip-compile with Python 3.12
# by the following command:
RulaKhaled marked this conversation as resolved.
Show resolved Hide resolved
#
# pip-compile requirements.in
#
amqp==5.2.0
# via kombu
analytics-python==1.3.0b1
Expand Down Expand Up @@ -73,7 +77,7 @@ codecov-ribs==0.1.18
# shared
colour==0.1.5
# via shared
coverage==7.5.0
coverage[toml]==7.5.0
# via
# -r requirements.in
# pytest-cov
Expand Down Expand Up @@ -111,7 +115,7 @@ filelock==3.12.4
# via virtualenv
freezegun==1.5.0
# via pytest-freezegun
google-api-core==2.23.0
google-api-core[grpc]==2.23.0
# via
# google-cloud-bigquery
# google-cloud-bigquery-storage
Expand Down Expand Up @@ -151,7 +155,7 @@ google-resumable-media==2.7.2
# via
# google-cloud-bigquery
# google-cloud-storage
googleapis-common-protos==1.66.0
googleapis-common-protos[grpc]==1.66.0
# via
# google-api-core
# grpc-google-iam-v1
Expand Down Expand Up @@ -363,19 +367,17 @@ requests==2.32.3
# stripe
respx==0.20.2
# via -r requirements.in
rfc3986==1.4.0
rfc3986[idna2008]==1.4.0
# via httpx
rsa==4.7.2
# via google-auth
s3transfer==0.10.1
# via boto3
sentry-sdk==2.13.0
sentry-sdk[celery]==2.13.0
# via
# -r requirements.in
# shared
setuptools==75.7.0
# via nodeenv
shared @ https://github.com/codecov/shared/archive/191837f5e35f5efc410e670aac7e50e0d09e43e1.tar.gz#egg=shared
shared @ https://github.com/codecov/shared/archive/52fdc2d14fa21d4c476d3264b021e42390b30c5f.tar.gz
# via -r requirements.in
six==1.16.0
# via
Expand Down Expand Up @@ -405,13 +407,13 @@ statsd==3.3.0
# via -r requirements.in
stripe==11.4.1
# via -r requirements.in
test-results-parser @ https://github.com/codecov/test-results-parser/archive/190bbc8a911099749928e13d5fe57f6027ca1e74.tar.gz#egg=test-results-parser
test-results-parser @ https://github.com/codecov/test-results-parser/archive/190bbc8a911099749928e13d5fe57f6027ca1e74.tar.gz
# via -r requirements.in
text-unidecode==1.3
# via faker
time-machine==2.14.1
# via -r requirements.in
timestring @ https://github.com/codecov/timestring/archive/d37ceacc5954dff3b5bd2f887936a98a668dda42.tar.gz#egg=timestring
timestring @ https://github.com/codecov/timestring/archive/d37ceacc5954dff3b5bd2f887936a98a668dda42.tar.gz
# via -r requirements.in
tqdm==4.66.1
# via openai
Expand Down Expand Up @@ -455,3 +457,6 @@ zstandard==0.23.0
# via
# -r requirements.in
# shared

# The following packages are considered to be unsafe in a requirements file:
# setuptools
23 changes: 16 additions & 7 deletions services/notification/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@

from celery.exceptions import CeleryError, SoftTimeLimitExceeded
from shared.config import get_config
from shared.django_apps.codecov_auth.models import Plan
from shared.helpers.yaml import default_if_true
from shared.plan.constants import TEAM_PLAN_REPRESENTATIONS
from shared.plan.constants import TierName
from shared.torngit.base import TorngitBaseAdapter
from shared.yaml import UserYaml

Expand Down Expand Up @@ -66,9 +67,13 @@ def __init__(
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
plan = Plan.objects.get(name=owner.plan)

if (
plan.tier.tier_name == TierName.TEAM.value
and status_type != StatusType.PATCH.value
):
return False

return True

Expand All @@ -81,9 +86,13 @@ def _should_use_checks_notifier(self, status_type: StatusType) -> bool:
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
plan = Plan.objects.get(name=owner.plan)

if (
plan.tier.tier_name == TierName.TEAM.value
and status_type != StatusType.PATCH.value
):
return False

app_installation_filter = filter(
lambda obj: (
Expand Down
77 changes: 64 additions & 13 deletions services/notification/tests/unit/test_notification_service.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import os
from asyncio import CancelledError
from asyncio import TimeoutError as AsyncioTimeoutError
from unittest.mock import patch

import mock
import pytest
from celery.exceptions import SoftTimeLimitExceeded
from shared.plan.constants import PlanName
from shared.plan.constants import PlanName, TierName
from shared.reports.resources import Report, ReportFile, ReportLine
from shared.torngit.status import Status
from shared.yaml import UserYaml
Expand All @@ -16,6 +17,7 @@
GithubAppInstallation,
)
from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory
from database.tests.factories.core import PlanFactory, TierFactory
from services.comparison import ComparisonProxy
from services.comparison.types import Comparison, EnrichedPull, FullCommit
from services.notification import NotificationService
Expand Down Expand Up @@ -103,9 +105,13 @@ def test_should_use_checks_notifier_yaml_field_false(self, dbsession):
),
],
)
@patch("services.notification.Plan.objects.get")
def test_should_use_checks_notifier_deprecated_flow(
self, repo_data, outcome, dbsession
self, plan_objects_get, repo_data, outcome, dbsession
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some questions, and see some problems with this:

  • first of all, you are creating the entity based on the Factory, so you might as well instert this into the database instead of using a mock for it.
  • but judging by the mock, this looks like you are using django .objects.get somewhere internally. This does not mix well with sqlalchemy (judging by both the dbsession fixture, and the .create test factory.

I believe that also in production, we should not mix these two ORMs, as that can potentially cause deadlocks.


repository = RepositoryFactory.create(**repo_data)
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == []
Expand All @@ -115,7 +121,12 @@ def test_should_use_checks_notifier_deprecated_flow(
== outcome
)

def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession):
@patch("services.notification.Plan.objects.get")
def test_should_use_checks_notifier_ghapp_all_repos_covered(
self, plan_objects_get, dbsession
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(owner__service="github")
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
Expand All @@ -133,9 +144,19 @@ def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession):
== True
)

def test_use_checks_notifier_for_team_plan(self, dbsession):
@patch("services.notification.Plan.objects.get")
def test_use_checks_notifier_for_team_plan(
self,
plan_objects_get,
dbsession,
):
tier = TierFactory.create(
tier_name=TierName.TEAM.value,
)
plan = PlanFactory.create(tier=tier, name=PlanName.TEAM_MONTHLY.value)
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.TEAM_MONTHLY.value
owner__service="github", owner__plan=plan.name
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
Expand All @@ -161,9 +182,16 @@ def test_use_checks_notifier_for_team_plan(self, dbsession):
== True
)

def test_use_status_notifier_for_team_plan(self, dbsession):
@patch("services.notification.Plan.objects.get")
def test_use_status_notifier_for_team_plan(self, plan_objects_get, dbsession):
tier = TierFactory.create(
tier_name=TierName.TEAM.value,
)
plan = PlanFactory.create(tier=tier, name=PlanName.TEAM_MONTHLY.value)
plan_objects_get.return_value = plan

repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.TEAM_MONTHLY.value
owner__service="github", owner__plan=plan.name
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
Expand All @@ -189,9 +217,15 @@ def test_use_status_notifier_for_team_plan(self, dbsession):
== True
)

def test_use_status_notifier_for_non_team_plan(self, dbsession):
@patch("services.notification.Plan.objects.get")
def test_use_status_notifier_for_non_team_plan(self, plan_objects_get, dbsession):
tier = TierFactory.create(
tier_name=TierName.PRO.value,
)
plan = PlanFactory.create(tier=tier, name=PlanName.CODECOV_PRO_MONTHLY.value)
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(
owner__service="github", owner__plan=PlanName.CODECOV_PRO_MONTHLY.value
owner__service="github", owner__plan=plan.name
)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
Expand Down Expand Up @@ -221,9 +255,12 @@ def test_use_status_notifier_for_non_team_plan(self, dbsession):
"gh_installation_name",
[GITHUB_APP_INSTALLATION_DEFAULT_NAME, "notifications-app"],
)
@patch("services.notification.Plan.objects.get")
def test_should_use_checks_notifier_ghapp_some_repos_covered(
self, dbsession, gh_installation_name
self, plan_objects_get, dbsession, gh_installation_name
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(owner__service="github")
other_repo_same_owner = RepositoryFactory.create(owner=repository.owner)
ghapp_installation = GithubAppInstallation(
Expand Down Expand Up @@ -281,9 +318,12 @@ def test_get_notifiers_instances_only_third_party(
assert instance.site_settings == ["slack.com"]
assert instance.current_yaml == current_yaml

@patch("services.notification.Plan.objects.get")
def test_get_notifiers_instances_checks(
self, dbsession, mock_configuration, mocker
self, plan_objects_get, dbsession, mock_configuration, mocker
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(
owner__integration_id=123,
owner__service="github",
Expand All @@ -310,9 +350,12 @@ def test_get_notifiers_instances_checks(
"codecov-slack-app",
]

@patch("services.notification.Plan.objects.get")
def test_get_notifiers_instances_slack_app_false(
self, dbsession, mock_configuration, mocker
self, plan_objects_get, dbsession, mock_configuration, mocker
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
mocker.patch("services.notification.get_config", return_value=False)
repository = RepositoryFactory.create(
owner__integration_id=123,
Expand Down Expand Up @@ -343,9 +386,17 @@ def test_get_notifiers_instances_slack_app_false(
"gh_installation_name",
[GITHUB_APP_INSTALLATION_DEFAULT_NAME, "notifications-app"],
)
@patch("services.notification.Plan.objects.get")
def test_get_notifiers_instances_checks_percentage_whitelist(
self, dbsession, mock_configuration, mocker, gh_installation_name
self,
plan_objects_get,
dbsession,
mock_configuration,
mocker,
gh_installation_name,
):
plan = PlanFactory.create()
plan_objects_get.return_value = plan
repository = RepositoryFactory.create(
owner__integration_id=123,
owner__service="github",
Expand Down
10 changes: 7 additions & 3 deletions services/test_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
from hashlib import sha256
from typing import Sequence

from shared.plan.constants import FREE_PLAN_REPRESENTATIONS, TEAM_PLAN_REPRESENTATIONS
from shared.django_apps.codecov_auth.models import Plan
from shared.plan.constants import TierName
from shared.yaml import UserYaml
from sqlalchemy import desc, distinct, func
from sqlalchemy.orm import joinedload
Expand Down Expand Up @@ -395,10 +396,13 @@ def get_test_summary_for_commit(


def not_private_and_free_or_team(repo: Repository):
plan = Plan.objects.get(name=repo.owner.plan)
return not (
repo.private
and repo.owner.plan
in {**FREE_PLAN_REPRESENTATIONS, **TEAM_PLAN_REPRESENTATIONS}
and (
plan.tier.tier_name != TierName.FREE.value
and plan.tier.tier_name != TierName.TEAM.value
)
)


Expand Down
Loading