Beta/legacy orgs now denied when contributor info missing

High Severity

Moving the beta/legacy org exemption after the contributor lookup introduces a regression. Previously, beta/legacy orgs bypassed all billing checks (returning None immediately). Now they'll be denied with BILLING_MISSING_CONTRIBUTOR_INFO if integration_id or pr_author_external_id is None, or with ORG_CONTRIBUTOR_NOT_FOUND if no contributor record exists. The intent was only to add the excluded-author check for all org types, but the reordering also removed the exemption from unrelated failure modes that beta/legacy orgs previously never hit.

Also curious about the reordering here @srest2021, should we have just put the new condition you added as the first check?

The thing is we need the integration id to get the contributor, which has the login. I can't see any solution other than just letting authors in beta orgs bypass the check :(

When is integration id missing?

It doesn't look like we have any instances of this happening during the last month https://app.datadoghq.com/dashboard/6x2-jgc-kiz/code-review?fromUser=true&refresh_mode=paused&from_ts=1770429006956&to_ts=1772848206956&live=false&tile_focus=6133593483676046

@cursor help me by clarifying what's this about

Based on my analysis of the code and PR discussion, here's what's happening:

The Issue

Original behavior (before this PR):
The _check_billing() method had beta/legacy orgs exit early at the very beginning:

def _check_billing(self):
    # Beta/legacy exemption was FIRST
    if beta_or_legacy_org:
        return None  # Skip all billing checks
    
    # These checks only ran for seat-based orgs
    if integration_id is None or pr_author_external_id is None:
        return BILLING_MISSING_CONTRIBUTOR_INFO
    
    contributor = get_contributor(...)  # Might not exist

Current behavior (after this PR):
The beta/legacy exemption was moved to AFTER the contributor lookup:

def _check_billing(self):
    # Now checks integration_id FIRST for all orgs
    if integration_id is None or pr_author_external_id is None:
        return BILLING_MISSING_CONTRIBUTOR_INFO  # ❌ Beta orgs now hit this
    
    contributor = get_contributor(...)  # ❌ Beta orgs now hit DoesNotExist
    
    if contributor.alias in excluded_logins:  # The new feature
        return PR_AUTHOR_EXCLUDED
    
    # Beta/legacy exemption now happens AFTER contributor lookup
    if beta_or_legacy_org:
        return None

Why It Was Reordered

The PR author (@srest2021) needs the contributor.alias to check if the PR author is excluded (e.g., dependabot). To get the contributor, they need integration_id and pr_author_external_id. So they had to move the beta/legacy exemption after the contributor lookup.

The Risk

Beta/legacy orgs will now be denied code review if:

1. integration_id is missing → Returns BILLING_MISSING_CONTRIBUTOR_INFO
2. pr_author_external_id is missing → Returns BILLING_MISSING_CONTRIBUTOR_INFO
3. No contributor record exists → Returns ORG_CONTRIBUTOR_NOT_FOUND

Previously, beta/legacy orgs bypassed these checks entirely.

Why It Might Be Fine

According to the Datadog dashboard that @srest2021 shared, there haven't been any instances of missing integration_id in the last month. If these fields are always present in practice, the regression won't manifest.

Recommendation

Since there's no evidence of missing data in production, the current implementation is probably fine. However, to be defensive, you could consider adding an early exemption for beta/legacy orgs that don't have the required contributor info, essentially treating them like excluded authors.

In the caller of CodeReviewPreflightService, we return early if integration is None, and we can get the PR author's external id from multiple spots in the webhook. If I had to pick a condition to be worried about, it would be a missing contributor row.

LGTM as is then

In theory the organization contributors always get created in here when they create new PRs, so in theory we should have rows for all expected OrganizationContributors. So should be good to go

In theory the organization contributors always get created in here when they create new PRs, so in theory we should have rows for all expected OrganizationContributors. So should be good to go