Skip to content

Commit

Permalink
chore: add events for migration runs (#28637)
Browse files Browse the repository at this point in the history
  • Loading branch information
zlwaterfield authored Feb 13, 2025
1 parent ef17551 commit 8d61a41
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 5 deletions.
10 changes: 10 additions & 0 deletions posthog/api/organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from posthog.rbac.migrations.rbac_feature_flag_migration import rbac_feature_flag_role_access_migration
from sentry_sdk import capture_exception
from drf_spectacular.utils import extend_schema
from posthog.event_usage import report_organization_action


class PremiumMultiorganizationPermissions(permissions.BasePermission):
Expand Down Expand Up @@ -277,9 +278,18 @@ def migrate_access_control(self, request: Request, **kwargs) -> Response:
self.check_object_permissions(request, organization)

try:
user = cast(User, request.user)
report_organization_action(organization, "rbac_team_migration_started", {"user": user.distinct_id})

rbac_team_access_control_migration(organization.id)
rbac_feature_flag_role_access_migration(organization.id)

report_organization_action(organization, "rbac_team_migration_completed", {"user": user.distinct_id})

except Exception as e:
report_organization_action(
organization, "rbac_team_migration_failed", {"user": user.distinct_id, "error": str(e)}
)
capture_exception(e)
return Response({"status": False, "error": "An internal error has occurred."}, status=500)

Expand Down
76 changes: 71 additions & 5 deletions posthog/api/test/test_organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,8 @@ def setUp(self):
organization_member=self.admin_user.organization_memberships.first(),
)

def test_migrate_feature_flags_rbac_as_admin(self):
@patch("posthog.api.organization.report_organization_action")
def test_migrate_feature_flags_rbac_as_admin(self, mock_report_action):
self.client.force_login(self.admin_user)

# Create a test feature flag
Expand All @@ -348,7 +349,16 @@ def test_migrate_feature_flags_rbac_as_admin(self):
self.assertEqual(access_control.resource, "feature_flag")
self.assertEqual(access_control.resource_id, str(feature_flag.id))

def test_migrate_feature_flags_rbac_with_org_view_only(self):
# Verify reporting calls
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_completed", {"user": self.admin_user.distinct_id}
)

@patch("posthog.api.organization.report_organization_action")
def test_migrate_feature_flags_rbac_with_org_view_only(self, mock_report_action):
self.client.force_login(self.admin_user)

# Create organization-wide view-only access
Expand Down Expand Up @@ -386,7 +396,16 @@ def test_migrate_feature_flags_rbac_with_org_view_only(self):
)
self.assertEqual(editor_access.count(), 3)

def test_migrate_feature_flags_rbac_with_specific_role_access(self):
# Add verification of reporting calls at the end
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_completed", {"user": self.admin_user.distinct_id}
)

@patch("posthog.api.organization.report_organization_action")
def test_migrate_feature_flags_rbac_with_specific_role_access(self, mock_report_action):
self.client.force_login(self.admin_user)

# Create a test feature flag
Expand All @@ -413,7 +432,16 @@ def test_migrate_feature_flags_rbac_with_specific_role_access(self):
)
self.assertEqual(access_control.access_level, "editor")

def test_migrate_team_rbac_as_admin(self):
# Add verification of reporting calls at the end
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_completed", {"user": self.admin_user.distinct_id}
)

@patch("posthog.api.organization.report_organization_action")
def test_migrate_team_rbac_as_admin(self, mock_report_action):
# Create a new team with access control enabled
team_with_access_control = Team.objects.create(
organization=self.organization, name="Team with Access Control", access_control=True
Expand Down Expand Up @@ -501,6 +529,14 @@ def test_migrate_team_rbac_as_admin(self):
team_with_access_control.refresh_from_db()
self.assertFalse(team_with_access_control.access_control)

# Add verification of reporting calls at the end
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_completed", {"user": self.admin_user.distinct_id}
)

def test_migrate_team_rbac_as_member_without_permissions(self):
self.member_user = self._create_user("[email protected]")
self.client.force_login(self.member_user)
Expand All @@ -517,7 +553,8 @@ def test_migrate_team_rbac_wrong_organization(self):
response = self.client.post(f"/api/organizations/{other_org.id}/migrate_access_control/")
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

def test_migrate_both_feature_flags_and_team_rbac(self):
@patch("posthog.api.organization.report_organization_action")
def test_migrate_both_feature_flags_and_team_rbac(self, mock_report_action):
"""Test that both feature flag and team RBAC migrations can be performed in a single call."""
# Create a new team with access control enabled
team_with_access_control = Team.objects.create(
Expand Down Expand Up @@ -604,3 +641,32 @@ def test_migrate_both_feature_flags_and_team_rbac(self):
# Verify total number of access controls
# 2 feature flags + 2 team access controls (base + member)
self.assertEqual(AccessControl.objects.count(), 4)

# Add verification of reporting calls at the end
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_completed", {"user": self.admin_user.distinct_id}
)

@patch("posthog.api.organization.report_organization_action")
def test_migrate_team_rbac_fails_with_error(self, mock_report_action):
"""Test that errors during migration are properly handled and reported."""
self.client.force_login(self.admin_user)

with patch("posthog.api.organization.rbac_team_access_control_migration", side_effect=Exception("Test error")):
response = self.client.post(f"/api/organizations/{self.organization.id}/migrate_access_control/")

self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR)
self.assertEqual(response.json(), {"status": False, "error": "An internal error has occurred."})

# Verify error was reported
mock_report_action.assert_any_call(
self.organization, "rbac_team_migration_started", {"user": self.admin_user.distinct_id}
)
mock_report_action.assert_any_call(
self.organization,
"rbac_team_migration_failed",
{"user": self.admin_user.distinct_id, "error": "Test error"},
)

0 comments on commit 8d61a41

Please sign in to comment.