-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore(cells) Switch ReactView and GenericReactView to control silo #104552
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 3 commits
0a77403
321dad6
16dc706
6af7895
25b7459
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 |
|---|---|---|
|
|
@@ -1089,7 +1089,7 @@ | |
| re_path( | ||
| r"^(?P<organization_slug>[^/]+)/projects/(?P<project_id_or_slug>[^/]+)/events/(?P<client_event_id>[^/]+)/$", | ||
| ProjectEventRedirect.as_view(), | ||
| name="sentry-project-event-redirect", | ||
| name="sentry-organization-project-event-redirect", | ||
|
Member
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. We had two routes with this name 🫠 |
||
| ), | ||
| re_path( | ||
| r"^(?P<organization_slug>[^/]+)/api-keys/$", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,12 @@ | |
| from sentry.silo.base import SiloMode | ||
| from sentry.testutils.cases import APITestCase, TwoFactorAPITestCase | ||
| from sentry.testutils.hybrid_cloud import HybridCloudTestMixin | ||
| from sentry.testutils.silo import assume_test_silo_mode, create_test_regions, region_silo_test | ||
| from sentry.testutils.silo import ( | ||
| assume_test_silo_mode, | ||
| control_silo_test, | ||
| create_test_regions, | ||
| region_silo_test, | ||
| ) | ||
| from sentry.users.models.authenticator import Authenticator | ||
| from sentry.utils.slug import ORG_SLUG_PATTERN | ||
|
|
||
|
|
@@ -328,7 +333,7 @@ def test_streamline_only_is_true(self) -> None: | |
|
|
||
|
|
||
| @region_silo_test(regions=create_test_regions("de", "us")) | ||
| class OrganizationsCreateInRegionTest(OrganizationIndexTest, HybridCloudTestMixin): | ||
| class OrganizationsCreateInRegionTest(OrganizationIndexTest): | ||
| method = "post" | ||
|
|
||
| @override_settings(SENTRY_MONOLITH_REGION="us", SENTRY_REGION="de") | ||
|
|
@@ -348,7 +353,9 @@ def test_success(self) -> None: | |
| assert mapping.region_name == "de" | ||
|
|
||
|
|
||
| @control_silo_test | ||
| class OrganizationIndex2faTest(TwoFactorAPITestCase): | ||
| # This is the HTML view, not an API endpoint | ||
| endpoint = "sentry-organization-home" | ||
|
|
||
| def setUp(self) -> None: | ||
|
|
@@ -365,8 +372,7 @@ def test_preexisting_members_must_enable_2fa(self) -> None: | |
| self.login_as(self.no_2fa_user) | ||
| self.assert_redirected_to_2fa() | ||
|
|
||
| with assume_test_silo_mode(SiloMode.CONTROL): | ||
| TotpInterface().enroll(self.no_2fa_user) | ||
| TotpInterface().enroll(self.no_2fa_user) | ||
| self.get_success_response(self.org_2fa.slug) | ||
|
|
||
| def test_new_member_must_enable_2fa(self) -> None: | ||
|
|
@@ -396,7 +402,9 @@ def test_superuser_can_access_org_home(self) -> None: | |
| self.get_success_response(self.org_2fa.slug) | ||
|
|
||
|
|
||
| @control_silo_test | ||
| class OrganizationIndexMemberLimitTest(APITestCase): | ||
| # This is a react view, not an API endpoint | ||
| endpoint = "sentry-organization-index" | ||
|
|
||
| def setup_user(self, is_superuser=False): | ||
|
|
@@ -417,4 +425,5 @@ def test_member_limit_redirect(self) -> None: | |
|
|
||
| def test_member_limit_superuser_no_redirect(self) -> None: | ||
| self.setup_user(is_superuser=True) | ||
| self.get_success_response(self.organization.slug, status_code=200) | ||
| response = self.get_success_response(self.organization.slug, status_code=200) | ||
| assert response.headers["Content-Type"] == "text/html" | ||
|
Contributor
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. Content-Type assertion may fail due to charsetLow Severity The assertion |
||
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.
Elif breaks view_class extraction from partial-wrapped functions
Low Severity
The change from
iftoelifon line 99 alters the control flow logic. Previously, after unwrapping afunctools.partial, the code would also check if the unwrapped function has aview_classattribute (for class-based views). Withelif, iffuncis a partial, it gets unwrapped but then theview_classcheck is skipped entirely. This means if anas_view()result is wrapped in a partial, theview_classwon't be extracted, potentially causing the URL pattern to be incorrectly processed or skipped. While no partials are currently used in URL patterns, this is a semantic change to the logic.