Skip to content

Commit d8d46a2

Browse files
fix(code-review): Consolidate code review checks (#105561)
Consolidate all the different checks for code review to one spot so we can use it in our new webhook flow. Note that I left the billing check off for now and will check with the team on how best to transition it on for GA Closes [CW-88](https://linear.app/getsentry/issue/CW-88/list-features-and-related-features-for-code-review)
1 parent 4b36fbf commit d8d46a2

File tree

15 files changed

+739
-53
lines changed

15 files changed

+739
-53
lines changed

fixtures/github.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3559,6 +3559,10 @@
35593559
"check_run": {
35603560
"external_id": "4663713",
35613561
"html_url": "https://github.com/test/repo/runs/4"
3562+
},
3563+
"sender": {
3564+
"id": 12345678,
3565+
"login": "test-user"
35623566
}
35633567
}"""
35643568

@@ -3569,5 +3573,9 @@
35693573
"id": 35129377,
35703574
"full_name": "getsentry/sentry",
35713575
"html_url": "https://github.com/getsentry/sentry"
3576+
},
3577+
"sender": {
3578+
"id": 12345678,
3579+
"login": "test-user"
35723580
}
35733581
}"""

src/sentry/integrations/github/webhook.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ def __call__(
8686
event: Mapping[str, Any],
8787
organization: Organization,
8888
repo: Repository,
89+
integration: RpcIntegration | None = None,
8990
**kwargs: Any,
9091
) -> None: ...
9192

src/sentry/models/repositorysettings.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
from dataclasses import dataclass
34
from enum import StrEnum
45

56
from django.contrib.postgres.fields.array import ArrayField
@@ -19,6 +20,14 @@ def as_choices(cls) -> tuple[tuple[str, str], ...]:
1920
return tuple((trigger.value, trigger.value) for trigger in cls)
2021

2122

23+
@dataclass
24+
class CodeReviewSettings:
25+
"""Settings for code review functionality on a repository."""
26+
27+
enabled: bool
28+
triggers: list[CodeReviewTrigger]
29+
30+
2231
@region_silo_model
2332
class RepositorySettings(Model):
2433
"""
@@ -41,3 +50,8 @@ class Meta:
4150
db_table = "sentry_repositorysettings"
4251

4352
__repr__ = sane_repr("repository_id", "enabled_code_review")
53+
54+
def get_code_review_settings(self) -> CodeReviewSettings:
55+
"""Return code review settings for this repository."""
56+
triggers = [CodeReviewTrigger(t) for t in self.code_review_triggers]
57+
return CodeReviewSettings(enabled=self.enabled_code_review, triggers=triggers)

src/sentry/overwatch/endpoints/overwatch_rpc.py

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,20 @@
1111
from rest_framework.request import Request
1212
from rest_framework.response import Response
1313

14-
from sentry import features, quotas
14+
from sentry import features
1515
from sentry.api.api_owners import ApiOwner
1616
from sentry.api.api_publish_status import ApiPublishStatus
1717
from sentry.api.authentication import AuthenticationSiloLimit, StandardAuthentication
1818
from sentry.api.base import Endpoint, region_silo_endpoint
19-
from sentry.constants import DEFAULT_CODE_REVIEW_TRIGGERS, DataCategory, ObjectStatus
19+
from sentry.constants import DEFAULT_CODE_REVIEW_TRIGGERS, ObjectStatus
2020
from sentry.integrations.services.integration import integration_service
2121
from sentry.models.organization import Organization
22-
from sentry.models.organizationcontributors import OrganizationContributors
2322
from sentry.models.repository import Repository
2423
from sentry.models.repositorysettings import RepositorySettings
2524
from sentry.prevent.models import PreventAIConfiguration
2625
from sentry.prevent.types.config import PREVENT_AI_CONFIG_DEFAULT, PREVENT_AI_CONFIG_DEFAULT_V1
26+
from sentry.seer.code_review.billing import passes_code_review_billing_check
2727
from sentry.silo.base import SiloMode
28-
from sentry.utils import metrics
2928
from sentry.utils.seer import can_use_prevent_ai_features
3029

3130
logger = logging.getLogger(__name__)
@@ -309,32 +308,11 @@ def _is_eligible_for_code_review(
309308
if not code_review_enabled:
310309
return False
311310

312-
# Check if contributor exists, and if there's either a seat or quota available.
313-
# NOTE: We explicitly check billing as the source of truth because if the contributor exists,
314-
# then that means that they've opened a PR before, and either have a seat already OR it's their
315-
# "Free action."
316-
try:
317-
contributor = OrganizationContributors.objects.get(
318-
organization_id=organization.id,
319-
integration_id=integration_id,
320-
external_identifier=external_identifier,
321-
)
322-
323-
if not quotas.backend.check_seer_quota(
324-
org_id=organization.id,
325-
data_category=DataCategory.SEER_USER,
326-
seat_object=contributor,
327-
):
328-
return False
329-
330-
except OrganizationContributors.DoesNotExist:
331-
metrics.incr(
332-
"overwatch.code_review.contributor_not_found",
333-
tags={"organization_id": organization.id, "repository_id": repository_id},
334-
)
335-
return False
336-
337-
return True
311+
return passes_code_review_billing_check(
312+
organization_id=organization.id,
313+
integration_id=integration_id,
314+
external_identifier=external_identifier,
315+
)
338316

339317

340318
@region_silo_endpoint
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
from __future__ import annotations
2+
3+
from sentry import quotas
4+
from sentry.constants import DataCategory
5+
from sentry.models.organizationcontributors import OrganizationContributors
6+
from sentry.utils import metrics
7+
8+
9+
def passes_code_review_billing_check(
10+
organization_id: int,
11+
integration_id: int,
12+
external_identifier: str,
13+
) -> bool:
14+
"""
15+
Check if contributor exists, and if there's either a seat or quota available.
16+
NOTE: We explicitly check billing as the source of truth because if the contributor exists,
17+
then that means that they've opened a PR before, and either have a seat already OR it's their
18+
"Free action."
19+
20+
Returns False if the billing check does not pass for code review feature.
21+
"""
22+
try:
23+
contributor = OrganizationContributors.objects.get(
24+
organization_id=organization_id,
25+
integration_id=integration_id,
26+
external_identifier=external_identifier,
27+
)
28+
except OrganizationContributors.DoesNotExist:
29+
metrics.incr(
30+
"overwatch.code_review.contributor_not_found",
31+
tags={"organization_id": organization_id},
32+
)
33+
return False
34+
35+
return quotas.backend.check_seer_quota(
36+
org_id=organization_id,
37+
data_category=DataCategory.SEER_USER,
38+
seat_object=contributor,
39+
)
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
from __future__ import annotations
2+
3+
from collections.abc import Callable
4+
from dataclasses import dataclass
5+
6+
from sentry import features
7+
from sentry.constants import ENABLE_PR_REVIEW_TEST_GENERATION_DEFAULT, HIDE_AI_FEATURES_DEFAULT
8+
from sentry.models.organization import Organization
9+
from sentry.models.repository import Repository
10+
from sentry.models.repositorysettings import CodeReviewSettings, RepositorySettings
11+
12+
DenialReason = str | None
13+
14+
15+
@dataclass
16+
class CodeReviewPreflightResult:
17+
allowed: bool
18+
settings: CodeReviewSettings | None = None
19+
denial_reason: DenialReason = None
20+
21+
22+
class CodeReviewPreflightService:
23+
def __init__(
24+
self,
25+
organization: Organization,
26+
repo: Repository,
27+
integration_id: int | None = None,
28+
pr_author_external_id: str | None = None,
29+
):
30+
self.organization = organization
31+
self.repo = repo
32+
self.integration_id = integration_id
33+
self.pr_author_external_id = pr_author_external_id
34+
35+
repo_settings = RepositorySettings.objects.filter(repository=repo).first()
36+
self._repo_settings = repo_settings.get_code_review_settings() if repo_settings else None
37+
38+
def check(self) -> CodeReviewPreflightResult:
39+
checks: list[Callable[[], DenialReason]] = [
40+
self._check_legal_ai_consent,
41+
self._check_org_feature_enablement,
42+
self._check_repo_feature_enablement,
43+
self._check_billing,
44+
]
45+
46+
for check in checks:
47+
denial_reason = check()
48+
if denial_reason is not None:
49+
return CodeReviewPreflightResult(allowed=False, denial_reason=denial_reason)
50+
51+
return CodeReviewPreflightResult(allowed=True, settings=self._repo_settings)
52+
53+
# -------------------------------------------------------------------------
54+
# Checks - each returns denial reason (str) or None if valid
55+
# -------------------------------------------------------------------------
56+
57+
def _check_legal_ai_consent(self) -> DenialReason:
58+
has_gen_ai_flag = features.has("organizations:gen-ai-features", self.organization)
59+
has_hidden_ai = self.organization.get_option(
60+
"sentry:hide_ai_features", HIDE_AI_FEATURES_DEFAULT
61+
)
62+
63+
if not has_gen_ai_flag or has_hidden_ai:
64+
return "org_legal_ai_consent_not_granted"
65+
return None
66+
67+
def _check_org_feature_enablement(self) -> DenialReason:
68+
# Seat-based orgs are always eligible
69+
if self._is_seat_based_seer_plan_org():
70+
return None
71+
72+
# Beta orgs need the legacy toggle enabled
73+
if self._is_code_review_beta_org():
74+
if self._has_legacy_toggle_enabled():
75+
return None
76+
return "org_pr_review_legacy_toggle_disabled"
77+
78+
return "org_not_eligible_for_code_review"
79+
80+
def _check_repo_feature_enablement(self) -> DenialReason:
81+
if self._is_seat_based_seer_plan_org():
82+
if self._repo_settings is None or not self._repo_settings.enabled:
83+
return "repo_code_review_disabled"
84+
return None
85+
elif self._is_code_review_beta_org():
86+
# For beta orgs, all repos are considered enabled
87+
return None
88+
else:
89+
return "repo_code_review_disabled"
90+
91+
def _check_billing(self) -> DenialReason:
92+
# TODO: Once we're ready to actually gate billing (when it's time for GA), uncomment this
93+
"""
94+
if self.integration_id is None or self.pr_author_external_id is None:
95+
return "billing_missing_contributor_info"
96+
97+
billing_ok = passes_code_review_billing_check(
98+
organization_id=self.organization.id,
99+
integration_id=self.integration_id,
100+
external_identifier=self.pr_author_external_id,
101+
)
102+
if not billing_ok:
103+
return "billing_quota_exceeded"
104+
"""
105+
106+
return None
107+
108+
# -------------------------------------------------------------------------
109+
# Org type helpers
110+
# -------------------------------------------------------------------------
111+
112+
def _is_seat_based_seer_plan_org(self) -> bool:
113+
return features.has("organizations:seat-based-seer-enabled", self.organization)
114+
115+
def _is_code_review_beta_org(self) -> bool:
116+
# TODO: Remove the has_legacy_opt_in check once the beta list is frozen
117+
has_beta_flag = features.has("organizations:code-review-beta", self.organization)
118+
has_legacy_opt_in = self.organization.get_option(
119+
"sentry:enable_pr_review_test_generation", False
120+
)
121+
return has_beta_flag or bool(has_legacy_opt_in)
122+
123+
def _has_legacy_toggle_enabled(self) -> bool:
124+
return bool(
125+
self.organization.get_option(
126+
"sentry:enable_pr_review_test_generation",
127+
ENABLE_PR_REVIEW_TEST_GENERATION_DEFAULT,
128+
)
129+
)

src/sentry/seer/code_review/utils.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,3 +201,27 @@ def transform_webhook_to_codegen_request(
201201
},
202202
},
203203
}
204+
205+
206+
def get_pr_author_id(event: Mapping[str, Any]) -> str | None:
207+
"""
208+
Extract the PR author's GitHub user ID from the webhook payload.
209+
The user information can be found in different locations depending on the webhook type.
210+
"""
211+
# Check issue.user.id (for issue comments on PRs)
212+
if (user_id := event.get("issue", {}).get("user", {}).get("id")) is not None:
213+
return str(user_id)
214+
215+
# Check pull_request.user.id (for pull request events)
216+
if (user_id := event.get("pull_request", {}).get("user", {}).get("id")) is not None:
217+
return str(user_id)
218+
219+
# Check user.id (fallback for direct user events)
220+
if (user_id := event.get("user", {}).get("id")) is not None:
221+
return str(user_id)
222+
223+
# Check sender.id (for check_run events). Sender is the user who triggered the event
224+
if (user_id := event.get("sender", {}).get("id")) is not None:
225+
return str(user_id)
226+
227+
return None

src/sentry/seer/code_review/webhooks/check_run.py

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
from sentry.models.organization import Organization
2222
from sentry.utils import metrics
2323

24-
from ..permissions import has_code_review_enabled
2524
from ..utils import SeerEndpoint, make_seer_request
2625

2726
logger = logging.getLogger(__name__)
@@ -30,7 +29,6 @@
3029
class ErrorStatus(enum.StrEnum):
3130
MISSING_ORGANIZATION = "missing_organization"
3231
MISSING_ACTION = "missing_action"
33-
CODE_REVIEW_NOT_ENABLED = "code_review_not_enabled"
3432
INVALID_PAYLOAD = "invalid_payload"
3533

3634

@@ -115,13 +113,6 @@ def handle_check_run_event(
115113
if action != GitHubCheckRunAction.REREQUESTED:
116114
return
117115

118-
if not has_code_review_enabled(organization):
119-
metrics.incr(
120-
f"{Metrics.ERROR.value}",
121-
tags={**tags, "error_status": ErrorStatus.CODE_REVIEW_NOT_ENABLED.value},
122-
)
123-
return
124-
125116
try:
126117
validated_event = _validate_github_check_run_event(event)
127118
except (ValidationError, ValueError):

0 commit comments

Comments
 (0)