-
Notifications
You must be signed in to change notification settings - Fork 617
UN-2022 [FEAT] Add co-owner management for Adapters, API Deployments, Connectors, Pipelines, Workflows, Prompt Studio #1797
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
base: main
Are you sure you want to change the base?
Changes from all commits
5c5e583
4ef01eb
d2c6d37
7e5ef07
656b6ae
d579c8a
795f0a8
91ec5d2
91b52dc
a6e99b1
cf46432
085f7e1
87c9942
ea9b3eb
69a2952
c60cd4d
66ea6df
69c1933
6e17faa
4a81ed7
50fa507
9049c54
b2c8ab6
9a9eab6
e250f94
70366e2
b296cd7
874078e
4bfb980
e3797fb
5bc5b60
2c2786b
99b951c
4a91422
8a05862
a3a44fc
4ba9bcf
5095376
cc94ec6
331be44
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,24 @@ | ||
| # Generated by Django 4.2.1 on 2026-02-17 08:24 | ||
|
|
||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ("adapter_processor_v2", "0003_mark_deprecated_adapters"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="adapterinstance", | ||
| name="co_owners", | ||
| field=models.ManyToManyField( | ||
| blank=True, | ||
| help_text="Users with full ownership privileges", | ||
| related_name="co_owned_adapters", | ||
| to=settings.AUTH_USER_MODEL, | ||
| ), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| from django.db import migrations | ||
|
|
||
|
|
||
| def backfill_creator_to_co_owners(apps, schema_editor): | ||
| adapter_instance_model = apps.get_model("adapter_processor_v2", "AdapterInstance") | ||
| for adapter in adapter_instance_model.objects.filter(created_by__isnull=False): | ||
| if not adapter.co_owners.filter(id=adapter.created_by_id).exists(): | ||
| adapter.co_owners.add(adapter.created_by) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("adapter_processor_v2", "0004_adapterinstance_co_owners"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython( | ||
| backfill_creator_to_co_owners, | ||
| reverse_code=migrations.RunPython.noop, | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||||||||||||||||||||||||||
| from configuration.models import Configuration | ||||||||||||||||||||||||||||||
| from django.db.models import F, OuterRef, QuerySet, Subquery | ||||||||||||||||||||||||||||||
| from django.http import HttpResponse | ||||||||||||||||||||||||||||||
| from permissions.co_owner_views import CoOwnerManagementMixin | ||||||||||||||||||||||||||||||
| from permissions.permission import IsOwner, IsOwnerOrSharedUserOrSharedToOrg | ||||||||||||||||||||||||||||||
| from plugins import get_plugin | ||||||||||||||||||||||||||||||
| from prompt_studio.prompt_studio_registry_v2.models import PromptStudioRegistry | ||||||||||||||||||||||||||||||
|
|
@@ -17,7 +18,6 @@ | |||||||||||||||||||||||||||||
| from tool_instance_v2.models import ToolInstance | ||||||||||||||||||||||||||||||
| from utils.enums import CeleryTaskState | ||||||||||||||||||||||||||||||
| from utils.hubspot_notify import notify_hubspot_event | ||||||||||||||||||||||||||||||
| from utils.pagination import CustomPagination | ||||||||||||||||||||||||||||||
| from workflow_manager.workflow_v2.dto import ExecutionResponse | ||||||||||||||||||||||||||||||
| from workflow_manager.workflow_v2.models.execution import WorkflowExecution | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
@@ -245,11 +245,22 @@ def get( | |||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| class APIDeploymentViewSet(viewsets.ModelViewSet): | ||||||||||||||||||||||||||||||
| pagination_class = CustomPagination | ||||||||||||||||||||||||||||||
| class APIDeploymentViewSet(CoOwnerManagementMixin, viewsets.ModelViewSet): | ||||||||||||||||||||||||||||||
| notification_resource_name_field = "display_name" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def get_notification_resource_type(self, resource: Any) -> str | None: | ||||||||||||||||||||||||||||||
| from plugins.notification.constants import ResourceType | ||||||||||||||||||||||||||||||
|
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. @kirtimanmishrazipstack is this plugin part of OSS?
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. @johnyrahul The notification plugin is part of OSS — it's in |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return ResourceType.API_DEPLOYMENT.value # type: ignore | ||||||||||||||||||||||||||||||
|
Comment on lines
+251
to
+254
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. Unguarded import will crash on OSS deployments
Suggested change
The same unguarded import pattern is also present in:
Prompt To Fix With AIThis is a comment left during a code review.
Path: backend/api_v2/api_deployment_views.py
Line: 251-254
Comment:
**Unguarded import will crash on OSS deployments**
`get_notification_resource_type` calls `from plugins.notification.constants import ResourceType` without a try/except, so on OSS deployments where the notification plugin is not installed this will raise an `ImportError` whenever `add_co_owner` or `remove_co_owner` is triggered. Compare this to `AdapterInstanceViewSet.get_notification_resource_type` in `adapter_processor_v2/views.py` which correctly wraps the import in a try/except and returns `None`:
```suggestion
def get_notification_resource_type(self, resource: Any) -> str | None:
try:
from plugins.notification.constants import ResourceType
return ResourceType.API_DEPLOYMENT.value # type: ignore
except ImportError:
logger.debug(
"Notification plugin not available, skipping resource type lookup"
)
return None
```
The same unguarded import pattern is also present in:
- `connector_v2/views.py` (`ConnectorInstanceViewSet.get_notification_resource_type`, line ~41)
- `pipeline_v2/views.py` (`PipelineViewSet.get_notification_resource_type`, line ~40)
- `workflow_manager/workflow_v2/views.py` (`WorkflowViewSet.get_notification_resource_type`, line ~77)
- `prompt_studio/prompt_studio_core_v2/views.py` (`PromptStudioCoreView.get_notification_resource_type`, line ~96)
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def get_permissions(self) -> list[Any]: | ||||||||||||||||||||||||||||||
| if self.action in ["destroy", "partial_update", "update"]: | ||||||||||||||||||||||||||||||
| if self.action in [ | ||||||||||||||||||||||||||||||
| "destroy", | ||||||||||||||||||||||||||||||
| "partial_update", | ||||||||||||||||||||||||||||||
| "update", | ||||||||||||||||||||||||||||||
| "add_co_owner", | ||||||||||||||||||||||||||||||
| "remove_co_owner", | ||||||||||||||||||||||||||||||
| ]: | ||||||||||||||||||||||||||||||
| return [IsOwner()] | ||||||||||||||||||||||||||||||
| return [IsOwnerOrSharedUserOrSharedToOrg()] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
@@ -277,7 +288,7 @@ def get_queryset(self) -> QuerySet | None: | |||||||||||||||||||||||||||||
| if search: | ||||||||||||||||||||||||||||||
| queryset = queryset.filter(display_name__icontains=search) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return queryset | ||||||||||||||||||||||||||||||
| return queryset.prefetch_related("co_owners") | ||||||||||||||||||||||||||||||
|
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. @kirtimanmishrazipstack what exactly is this prefetch_related?
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. @johnyrahul The prefetch_related("co_owners") is a Django optimization — it pre-fetches the co_owners M2M relation in a single query instead of hitting the DB for each object when co-owners are accessed. This is correct and a standard Django best practice to avoid N+1 queries. Link here |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def get_serializer_class(self) -> serializers.Serializer: | ||||||||||||||||||||||||||||||
| if self.action in ["list"]: | ||||||||||||||||||||||||||||||
|
|
@@ -345,7 +356,9 @@ def by_prompt_studio_tool(self, request: Request) -> Response: | |||||||||||||||||||||||||||||
| workflow_id__in=workflow_ids, created_by=request.user | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| serializer = APIDeploymentListSerializer(deployments, many=True) | ||||||||||||||||||||||||||||||
| serializer = APIDeploymentListSerializer( | ||||||||||||||||||||||||||||||
| deployments, many=True, context={"request": request} | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| return Response(serializer.data, status=status.HTTP_200_OK) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| except PromptStudioRegistry.DoesNotExist: | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # Generated by Django 4.2.1 on 2026-02-17 08:24 | ||
|
|
||
| from django.conf import settings | ||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ("api_v2", "0003_add_organization_rate_limit"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="apideployment", | ||
| name="co_owners", | ||
| field=models.ManyToManyField( | ||
| blank=True, | ||
| help_text="Users with full ownership privileges", | ||
| related_name="co_owned_api_deployments", | ||
| to=settings.AUTH_USER_MODEL, | ||
| ), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| from django.db import migrations | ||
|
|
||
|
|
||
| def backfill_creator_to_co_owners(apps, schema_editor): | ||
| api_deployment_model = apps.get_model("api_v2", "APIDeployment") | ||
| for deployment in api_deployment_model.objects.filter(created_by__isnull=False): | ||
| if not deployment.co_owners.filter(id=deployment.created_by_id).exists(): | ||
| deployment.co_owners.add(deployment.created_by) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("api_v2", "0004_apideployment_co_owners"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython( | ||
| backfill_creator_to_co_owners, | ||
| reverse_code=migrations.RunPython.noop, | ||
| ), | ||
| ] |
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.
why does it require to have post/delete operation cant it use existing patch operation? @kirtimanmishrazipstack
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.
@johnyrahul co_owners is a separate concept from shared_users, and the dedicated endpoints provide better validation (e.g., preventing removal of the last owner)
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.
@kirtimanmishrazipstack one clarification: Then why don't we expand the shared users with WRITE/Updated permission instaed of new co-owners entity.?
cc: @hari-kuriakose