-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(code-review): Consolidate code review checks #105561
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
Changes from all commits
05b44fd
17b7e2a
66e863e
9a8da6d
6fd4aff
d46d0e1
1a69e22
3de8ad8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from sentry import quotas | ||
| from sentry.constants import DataCategory | ||
| from sentry.models.organizationcontributors import OrganizationContributors | ||
| from sentry.utils import metrics | ||
|
|
||
|
|
||
| def passes_code_review_billing_check( | ||
| organization_id: int, | ||
| integration_id: int, | ||
| external_identifier: str, | ||
| ) -> bool: | ||
| """ | ||
| Check if contributor exists, and if there's either a seat or quota available. | ||
| NOTE: We explicitly check billing as the source of truth because if the contributor exists, | ||
| then that means that they've opened a PR before, and either have a seat already OR it's their | ||
| "Free action." | ||
|
|
||
| Returns False if the billing check does not pass for code review feature. | ||
| """ | ||
| try: | ||
| contributor = OrganizationContributors.objects.get( | ||
| organization_id=organization_id, | ||
| integration_id=integration_id, | ||
| external_identifier=external_identifier, | ||
| ) | ||
| except OrganizationContributors.DoesNotExist: | ||
| metrics.incr( | ||
| "overwatch.code_review.contributor_not_found", | ||
| tags={"organization_id": organization_id}, | ||
| ) | ||
| return False | ||
|
|
||
| return quotas.backend.check_seer_quota( | ||
| org_id=organization_id, | ||
| data_category=DataCategory.SEER_USER, | ||
| seat_object=contributor, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Callable | ||
| from dataclasses import dataclass | ||
|
|
||
| from sentry import features | ||
| from sentry.constants import ENABLE_PR_REVIEW_TEST_GENERATION_DEFAULT, HIDE_AI_FEATURES_DEFAULT | ||
| from sentry.models.organization import Organization | ||
| from sentry.models.repository import Repository | ||
| from sentry.models.repositorysettings import CodeReviewSettings, RepositorySettings | ||
|
|
||
| DenialReason = str | None | ||
|
|
||
|
|
||
| @dataclass | ||
| class CodeReviewPreflightResult: | ||
| allowed: bool | ||
| settings: CodeReviewSettings | None = None | ||
| denial_reason: DenialReason = None | ||
|
|
||
|
|
||
| class CodeReviewPreflightService: | ||
| def __init__( | ||
| self, | ||
| organization: Organization, | ||
| repo: Repository, | ||
| integration_id: int | None = None, | ||
| pr_author_external_id: str | None = None, | ||
| ): | ||
| self.organization = organization | ||
| self.repo = repo | ||
| self.integration_id = integration_id | ||
| self.pr_author_external_id = pr_author_external_id | ||
|
|
||
| repo_settings = RepositorySettings.objects.filter(repository=repo).first() | ||
| self._repo_settings = repo_settings.get_code_review_settings() if repo_settings else None | ||
|
|
||
| def check(self) -> CodeReviewPreflightResult: | ||
| checks: list[Callable[[], DenialReason]] = [ | ||
| self._check_legal_ai_consent, | ||
| self._check_org_feature_enablement, | ||
| self._check_repo_feature_enablement, | ||
| self._check_billing, | ||
| ] | ||
|
|
||
| for check in checks: | ||
| denial_reason = check() | ||
| if denial_reason is not None: | ||
| return CodeReviewPreflightResult(allowed=False, denial_reason=denial_reason) | ||
|
|
||
| return CodeReviewPreflightResult(allowed=True, settings=self._repo_settings) | ||
|
|
||
| # ------------------------------------------------------------------------- | ||
| # Checks - each returns denial reason (str) or None if valid | ||
| # ------------------------------------------------------------------------- | ||
|
|
||
| def _check_legal_ai_consent(self) -> DenialReason: | ||
| has_gen_ai_flag = features.has("organizations:gen-ai-features", self.organization) | ||
| has_hidden_ai = self.organization.get_option( | ||
| "sentry:hide_ai_features", HIDE_AI_FEATURES_DEFAULT | ||
| ) | ||
|
|
||
| if not has_gen_ai_flag or has_hidden_ai: | ||
| return "org_legal_ai_consent_not_granted" | ||
| return None | ||
suejung-sentry marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def _check_org_feature_enablement(self) -> DenialReason: | ||
| # Seat-based orgs are always eligible | ||
| if self._is_seat_based_seer_plan_org(): | ||
| return None | ||
|
|
||
| # Beta orgs need the legacy toggle enabled | ||
| if self._is_code_review_beta_org(): | ||
| if self._has_legacy_toggle_enabled(): | ||
| return None | ||
| return "org_pr_review_legacy_toggle_disabled" | ||
|
|
||
| return "org_not_eligible_for_code_review" | ||
|
|
||
| def _check_repo_feature_enablement(self) -> DenialReason: | ||
| if self._is_seat_based_seer_plan_org(): | ||
| if self._repo_settings is None or not self._repo_settings.enabled: | ||
| return "repo_code_review_disabled" | ||
| return None | ||
| elif self._is_code_review_beta_org(): | ||
| # For beta orgs, all repos are considered enabled | ||
| return None | ||
| else: | ||
| return "repo_code_review_disabled" | ||
|
|
||
| def _check_billing(self) -> DenialReason: | ||
suejung-sentry marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # TODO: Once we're ready to actually gate billing (when it's time for GA), uncomment this | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that I updated this to always let the billing check go through for now since it's not turned on for the overwatch flow yet either (just logged here). This will be turned on with GA. |
||
| """ | ||
| if self.integration_id is None or self.pr_author_external_id is None: | ||
| return "billing_missing_contributor_info" | ||
|
|
||
| billing_ok = passes_code_review_billing_check( | ||
| organization_id=self.organization.id, | ||
| integration_id=self.integration_id, | ||
| external_identifier=self.pr_author_external_id, | ||
| ) | ||
| if not billing_ok: | ||
| return "billing_quota_exceeded" | ||
| """ | ||
|
|
||
| return None | ||
|
|
||
| # ------------------------------------------------------------------------- | ||
| # Org type helpers | ||
| # ------------------------------------------------------------------------- | ||
|
|
||
| def _is_seat_based_seer_plan_org(self) -> bool: | ||
| return features.has("organizations:seat-based-seer-enabled", self.organization) | ||
|
|
||
| def _is_code_review_beta_org(self) -> bool: | ||
| # TODO: Remove the has_legacy_opt_in check once the beta list is frozen | ||
| has_beta_flag = features.has("organizations:code-review-beta", self.organization) | ||
| has_legacy_opt_in = self.organization.get_option( | ||
| "sentry:enable_pr_review_test_generation", False | ||
| ) | ||
| return has_beta_flag or bool(has_legacy_opt_in) | ||
|
|
||
| def _has_legacy_toggle_enabled(self) -> bool: | ||
| return bool( | ||
| self.organization.get_option( | ||
| "sentry:enable_pr_review_test_generation", | ||
| ENABLE_PR_REVIEW_TEST_GENERATION_DEFAULT, | ||
| ) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,3 +201,27 @@ def transform_webhook_to_codegen_request( | |
| }, | ||
| }, | ||
| } | ||
|
|
||
|
|
||
| def get_pr_author_id(event: Mapping[str, Any]) -> str | None: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| """ | ||
| Extract the PR author's GitHub user ID from the webhook payload. | ||
| The user information can be found in different locations depending on the webhook type. | ||
| """ | ||
| # Check issue.user.id (for issue comments on PRs) | ||
| if (user_id := event.get("issue", {}).get("user", {}).get("id")) is not None: | ||
| return str(user_id) | ||
|
|
||
| # Check pull_request.user.id (for pull request events) | ||
| if (user_id := event.get("pull_request", {}).get("user", {}).get("id")) is not None: | ||
| return str(user_id) | ||
|
|
||
| # Check user.id (fallback for direct user events) | ||
| if (user_id := event.get("user", {}).get("id")) is not None: | ||
| return str(user_id) | ||
|
|
||
| # Check sender.id (for check_run events). Sender is the user who triggered the event | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll confirm with @ajay-sentry / Product who/what we want to increment for this webhook event. It's possible they would want this one to be "always free" |
||
| if (user_id := event.get("sender", {}).get("id")) is not None: | ||
suejung-sentry marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return str(user_id) | ||
|
|
||
| return None | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that when I moved this over I removed the tag for repository id as I thought it would add too much cardinality on the metric. Will connect with @ajay-sentry if that is going to be a problem..