Skip to content

chore(github): Add business+ plan check for multi org #91905

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

Merged
merged 15 commits into from
May 27, 2025
Merged

chore(github): Add business+ plan check for multi org #91905

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e043866
add gh mo to permanent ff list
Christinarlong May 19, 2025
7bd57c5
Merge branch 'master' into crl/gh-mo-business-gate
Christinarlong May 19, 2025
2ba5d3e
switch flag to integrations-multi-org
Christinarlong May 20, 2025
4e2d4cf
add ff check for business plans
Christinarlong May 20, 2025
604bcab
Merge branch 'master' into crl/gh-mo-business-gate
Christinarlong May 20, 2025
5acabb2
add permanent ff and feature check in be
Christinarlong May 21, 2025
bee5115
Merge branch 'master' into crl/gh-mo-business-gate
Christinarlong May 21, 2025
db11b20
weirdge test
Christinarlong May 21, 2025
e1a57cc
add ff to feature list
Christinarlong May 20, 2025
49e22aa
rename business plan to feature check
Christinarlong May 21, 2025
48b29d1
wording
Christinarlong May 21, 2025
510aa8a
add error if user tries to access multi org without valid feature
Christinarlong May 22, 2025
5ed8d97
add error if user tries to mo without ff
Christinarlong May 22, 2025
156fb07
add error if user tries to mo without ff
Christinarlong May 22, 2025
84a0414
remove user parameter in ff check
Christinarlong May 22, 2025
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
2 changes: 2 additions & 0 deletions src/sentry/features/permanent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ def register_permanent_features(manager: FeatureManager):
"organizations:seer-based-priority": False,
# Enable Vercel integration - there is a custom handler in getsentry
"organizations:integrations-vercel": True,
# Enable GitHub multi-org for users to connect many Sentry orgs to a single GitHub org.
"organizations:integrations-scm-multi-org": True,
# Enable issue view endpoints and UI
"organizations:issue-views": False,
}
Expand Down
25 changes: 23 additions & 2 deletions src/sentry/integrations/github/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,8 @@ class GitHubInstallationError(StrEnum):
INSTALLATION_EXISTS = "Github installed on another Sentry organization."
USER_MISMATCH = "Authenticated user is not the same as who installed the app."
MISSING_INTEGRATION = "Integration does not exist."
INVALID_INSTALLATION = "User does not have access to given installation"
INVALID_INSTALLATION = "User does not have access to given installation."
FEATURE_NOT_AVAILABLE = "Your organization does not have access to this feature."


def record_event(event: IntegrationPipelineViewType):
Expand Down Expand Up @@ -887,6 +888,15 @@ def _get_eligible_multi_org_installations(
class GithubOrganizationSelection(PipelineView):
def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase:
self.active_user_organization = determine_active_organization(request)
has_scm_multi_org = (
features.has(
"organizations:integrations-scm-multi-org",
organization=self.active_user_organization.organization,
actor=request.user,
Copy link
Member

Choose a reason for hiding this comment

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

qq: whats the purpose of passing user here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 I probably yoinked this from a previous feature flag checker, but looking further yeah we dont need to be passing the user/it wont be used.

)
if self.active_user_organization is not None
else False
)

if self.active_user_organization is None or not features.has(
"organizations:github-multi-org",
Expand Down Expand Up @@ -915,6 +925,14 @@ def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase
if chosen_installation_id == "-1":
return pipeline.next_step()

if not has_scm_multi_org:
lifecycle.record_failure(GitHubInstallationError.FEATURE_NOT_AVAILABLE)
return error(
request,
self.active_user_organization,
error_short=GitHubInstallationError.FEATURE_NOT_AVAILABLE,
)

# Verify that the given GH installation belongs to the person installing the pipeline
installation_ids = [
installation["installation_id"] for installation in installation_info
Expand All @@ -936,7 +954,10 @@ def dispatch(self, request: HttpRequest, pipeline: Pipeline) -> HttpResponseBase
return self.render_react_view(
request=request,
pipeline_name="githubInstallationSelect",
props={"installation_info": installation_info},
props={
"installation_info": installation_info,
Copy link
Member

Choose a reason for hiding this comment

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

if they don't have multi org, should we still be passing the full list of installations to the FE?

Copy link
Member

Choose a reason for hiding this comment

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

i think we should also error if somebody tries to pass a chosen installation id that's not -1 and they don't have multi org

Copy link
Contributor Author

@Christinarlong Christinarlong May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For the passing the full list comment, I think it's useful to keep in case we want to do the per gh org upsell in the dropdown in the UI.

"has_scm_multi_org": has_scm_multi_org,
},
)


Expand Down
1 change: 1 addition & 0 deletions tests/sentry/api/serializers/test_organization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ def test_simple(self):
"integrations-incident-management",
"integrations-issue-basic",
"integrations-issue-sync",
"integrations-scm-multi-org",
"integrations-stacktrace-link",
"integrations-ticket-rules",
"integrations-vercel",
Expand Down
131 changes: 129 additions & 2 deletions tests/sentry/integrations/github/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,7 @@ def test_get_account_id_backfill_missing(self):
integration = Integration.objects.get(id=integration_id)
assert integration.metadata["account_id"] == 60591805

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand Down Expand Up @@ -1315,12 +1316,13 @@ def test_github_installation_calls_ui(self, mock_render, mock_record):
mock_render.assert_called_with(
request=ANY,
pipeline_name="githubInstallationSelect",
props={"installation_info": installations},
props={"installation_info": installations, "has_scm_multi_org": True},
)

# SLO assertions
assert_success_metric(mock_record)

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand Down Expand Up @@ -1405,6 +1407,7 @@ def test_github_installation_stores_chosen_installation(self, mock_record):
# SLO assertions
assert_success_metric(mock_record)

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand Down Expand Up @@ -1449,7 +1452,7 @@ def test_github_installation_fails_on_invalid_installation(self, mock_record):

self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert (
b'{"success":false,"data":{"error":"User does not have access to given installation"}'
b'{"success":false,"data":{"error":"User does not have access to given installation."}'
in resp.content
)
assert (
Expand All @@ -1476,6 +1479,128 @@ def test_github_installation_fails_on_invalid_installation(self, mock_record):

assert_failure_metric(mock_record, GitHubInstallationError.INVALID_INSTALLATION)

@with_feature(
{"organizations:github-multi-org": True, "organizations:integrations-scm-multi-org": False}
)
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
@patch.object(PipelineView, "render_react_view", return_value=HttpResponse())
def test_github_installation_calls_ui_no_biz_plan(self, mock_render, mock_record):
self._setup_with_existing_installations()
installations = [
{
"installation_id": "1",
"github_account": "santry",
"avatar_url": "https://github.com/knobiknows/all-the-bufo/raw/main/all-the-bufo/bufo-pitchforks.png",
},
{
"installation_id": "2",
"github_account": "bufo-bot",
"avatar_url": "https://github.com/knobiknows/all-the-bufo/raw/main/all-the-bufo/bufo-pog.png",
},
{
"installation_id": "-1",
"github_account": "Integrate with a new GitHub organization",
"avatar_url": "",
},
]

resp = self.client.get(self.init_path)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
assert (
redirect.query
== f"client_id=github-client-id&state={self.pipeline.signature}&redirect_uri=http://testserver/extensions/github/setup/"
)
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode({"code": "12345678901234567890", "state": self.pipeline.signature}),
)
)
mock_render.assert_called_with(
request=ANY,
pipeline_name="githubInstallationSelect",
props={"installation_info": installations, "has_scm_multi_org": False},
)

# SLO assertions
assert_success_metric(mock_record)

@with_feature(
{"organizations:github-multi-org": True, "organizations:integrations-scm-multi-org": False}
)
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
@patch.object(PipelineView, "render_react_view", return_value=HttpResponse())
def test_errors_when_invalid_access_to_multi_org(self, mock_render, mock_record):
self._setup_with_existing_installations()
installations = [
{
"installation_id": "1",
"github_account": "santry",
"avatar_url": "https://github.com/knobiknows/all-the-bufo/raw/main/all-the-bufo/bufo-pitchforks.png",
},
{
"installation_id": "2",
"github_account": "bufo-bot",
"avatar_url": "https://github.com/knobiknows/all-the-bufo/raw/main/all-the-bufo/bufo-pog.png",
},
{
"installation_id": "-1",
"github_account": "Integrate with a new GitHub organization",
"avatar_url": "",
},
]

resp = self.client.get(self.init_path)
assert resp.status_code == 302
redirect = urlparse(resp["Location"])
assert redirect.scheme == "https"
assert redirect.netloc == "github.com"
assert redirect.path == "/login/oauth/authorize"
assert (
redirect.query
== f"client_id=github-client-id&state={self.pipeline.signature}&redirect_uri=http://testserver/extensions/github/setup/"
)
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode({"code": "12345678901234567890", "state": self.pipeline.signature}),
)
)
mock_render.assert_called_with(
request=ANY,
pipeline_name="githubInstallationSelect",
props={"installation_info": installations, "has_scm_multi_org": False},
)

# We rendered the GithubOrganizationSelection UI and the user chose to skip
resp = self.client.get(
"{}?{}".format(
self.setup_path,
urlencode(
{
"code": "12345678901234567890",
"state": self.pipeline.signature,
"chosen_installation_id": "12345",
}
),
)
)

self.assertTemplateUsed(resp, "sentry/integrations/github-integration-failed.html")
assert (
b'{"success":false,"data":{"error":"Your organization does not have access to this feature."}}'
in resp.content
)
assert b'window.opener.postMessage({"success":false' in resp.content
assert_failure_metric(mock_record, GitHubInstallationError.FEATURE_NOT_AVAILABLE)

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
Expand Down Expand Up @@ -1543,6 +1668,7 @@ def test_github_installation_skips_chosen_installation(self, mock_record):
# SLO assertions
assert_success_metric(mock_record)

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
def test_github_installation_gets_owner_orgs(self):
Expand All @@ -1554,6 +1680,7 @@ def test_github_installation_gets_owner_orgs(self):

assert owner_orgs == ["santry"]

@with_feature("organizations:integrations-scm-multi-org")
@with_feature("organizations:github-multi-org")
@responses.activate
def test_github_installation_filters_valid_installations(self):
Expand Down
Loading