Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 10 additions & 5 deletions backend/adapter_processor_v2/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,16 +294,17 @@ def destroy(
def partial_update(
self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any]
) -> Response:
# Store current shared users before update (for email notifications)
adapter = self.get_object()
current_shared_users = set(adapter.shared_users.all())
shared_users_updated = AdapterKeys.SHARED_USERS in request.data
current_shared_users = set()

if AdapterKeys.SHARED_USERS in request.data:
if shared_users_updated:
current_shared_users = set(adapter.shared_users.all())
# find the deleted users
shared_users = {
int(user_id) for user_id in request.data.get("shared_users", {})
}
current_users = {user.id for user in adapter.shared_users.all()}
current_users = {user.id for user in current_shared_users}
removed_users = current_users.difference(shared_users)

# if removed user use this adapter as default
Expand Down Expand Up @@ -345,7 +346,11 @@ def partial_update(
response = super().partial_update(request, *args, **kwargs)

# Send email notifications to newly shared users
if response.status_code == 200 and AdapterKeys.SHARED_USERS in request.data:
if (
response.status_code == 200
and shared_users_updated
and bool(notification_plugin)
):
try:
adapter.refresh_from_db()
new_shared_users = set(adapter.shared_users.all())
Expand Down
11 changes: 5 additions & 6 deletions backend/api_v2/api_deployment_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,17 +373,16 @@ def partial_update(self, request: Request, *args: Any, **kwargs: Any) -> Respons
"""Override partial_update to handle sharing notifications."""
# Get current instance and shared users
instance = self.get_object()
current_shared_users = set(instance.shared_users.all())
shared_users_updated = "shared_users" in request.data
current_shared_users = set()
if shared_users_updated:
current_shared_users = set(instance.shared_users.all())

# Perform the update
response = super().partial_update(request, *args, **kwargs)

# If successful and shared_users changed, send notifications
if (
response.status_code == 200
and "shared_users" in request.data
and notification_plugin
):
if response.status_code == 200 and shared_users_updated and notification_plugin:
try:
instance.refresh_from_db()
new_shared_users = set(instance.shared_users.all())
Expand Down
7 changes: 5 additions & 2 deletions backend/connector_v2/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,16 @@ def perform_destroy(self, instance: ConnectorInstance) -> None:
def partial_update(self, request: Request, *args: Any, **kwargs: Any) -> Response:
"""Override to handle sharing notifications."""
instance = self.get_object()
current_shared_users = set(instance.shared_users.all())
shared_users_updated = "shared_users" in request.data
current_shared_users = set()
if shared_users_updated:
current_shared_users = set(instance.shared_users.all())

response = super().partial_update(request, *args, **kwargs)

if (
response.status_code == 200
and "shared_users" in request.data
and shared_users_updated
and bool(notification_plugin)
):
try:
Expand Down
11 changes: 5 additions & 6 deletions backend/pipeline_v2/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,14 @@ def list_of_shared_users(self, request: Request, pk: str | None = None) -> Respo
def partial_update(self, request: Request, *args: Any, **kwargs: Any) -> Response:
"""Override to handle sharing notifications."""
instance = self.get_object()
current_shared_users = set(instance.shared_users.all())
shared_users_updated = "shared_users" in request.data
current_shared_users = set()
if shared_users_updated:
current_shared_users = set(instance.shared_users.all())

response = super().partial_update(request, *args, **kwargs)

if (
response.status_code == 200
and "shared_users" in request.data
and notification_plugin
):
if response.status_code == 200 and shared_users_updated and notification_plugin:
try:
instance.refresh_from_db()
new_shared_users = set(instance.shared_users.all())
Expand Down
8 changes: 5 additions & 3 deletions backend/prompt_studio/prompt_studio_core_v2/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,17 @@ def destroy(
def partial_update(
self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any]
) -> Response:
# Store current shared users before update for email notifications
custom_tool = self.get_object()
current_shared_users = set(custom_tool.shared_users.all())
shared_users_updated = "shared_users" in request.data
current_shared_users = set()
if shared_users_updated:
current_shared_users = set(custom_tool.shared_users.all())

# Perform the update
response = super().partial_update(request, *args, **kwargs)

# Send email notifications to newly shared users
if response.status_code == 200 and "shared_users" in request.data:
if response.status_code == 200 and shared_users_updated:
from plugins import get_plugin

notification_plugin = get_plugin("notification")
Expand Down
62 changes: 62 additions & 0 deletions backend/utils/tests/test_shared_user_query_optimizations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
from types import SimpleNamespace
from unittest.mock import MagicMock, patch

from rest_framework import status, viewsets
from rest_framework.response import Response

from adapter_processor_v2.views import AdapterInstanceViewSet
from workflow_manager.workflow_v2.views import WorkflowViewSet

Comment on lines +1 to +9
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Missing skip-path coverage for four changed views

The test file only exercises WorkflowViewSet and AdapterInstanceViewSet, but ConnectorInstanceViewSet, PipelineViewSet, PromptStudioCoreView, and APIDeploymentViewSet all received the same guard change. A regression in any of those four (e.g. accidentally removing the shared_users_updated guard) would go undetected. The skip-path test for WorkflowViewSet is a single-function template that could be copy-adapted for each missing view in a few lines.

Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/tests/test_shared_user_query_optimizations.py
Line: 1-9

Comment:
**Missing skip-path coverage for four changed views**

The test file only exercises `WorkflowViewSet` and `AdapterInstanceViewSet`, but `ConnectorInstanceViewSet`, `PipelineViewSet`, `PromptStudioCoreView`, and `APIDeploymentViewSet` all received the same guard change. A regression in any of those four (e.g. accidentally removing the `shared_users_updated` guard) would go undetected. The skip-path test for `WorkflowViewSet` is a single-function template that could be copy-adapted for each missing view in a few lines.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code


def test_workflow_partial_update_skips_shared_user_lookup_without_share_changes() -> None:
view = WorkflowViewSet()
workflow = MagicMock()
workflow.shared_users.all.side_effect = AssertionError(
"shared_users should not be queried for non-sharing updates"
)
request = SimpleNamespace(data={"workflow_name": "renamed"}, user=MagicMock())

with (
patch.object(WorkflowViewSet, "get_object", return_value=workflow),
patch.object(
viewsets.ModelViewSet,
"partial_update",
autospec=True,
return_value=Response(status=status.HTTP_200_OK),
),
):
response = view.partial_update(request)

assert response.status_code == status.HTTP_200_OK
workflow.shared_users.all.assert_not_called()


def test_adapter_partial_update_queries_shared_users_once_without_notification_plugin() -> None:
# When notification_plugin is absent (the common test-env case), the post-update
# comparison block is skipped entirely. Only one pre-update snapshot query should
# be made — the old code made two (snapshot + duplicate ID extraction).
view = AdapterInstanceViewSet()
shared_user_1 = SimpleNamespace(id=1)
shared_user_2 = SimpleNamespace(id=2)
adapter = MagicMock(adapter_type="LLM", adapter_name="Adapter", id=1)
adapter.shared_users.all.return_value = [shared_user_1, shared_user_2]
request = SimpleNamespace(
data={"shared_users": ["1", "2"]},
user=MagicMock(),
)

with (
patch.object(AdapterInstanceViewSet, "get_object", return_value=adapter),
patch.object(
viewsets.ModelViewSet,
"partial_update",
autospec=True,
return_value=Response(status=status.HTTP_200_OK),
),
patch("adapter_processor_v2.views.notification_plugin", None),
):
response = view.partial_update(request)

assert response.status_code == status.HTTP_200_OK
# Pre-update snapshot only — the duplicate ID-extraction query is gone.
assert adapter.shared_users.all.call_count == 1
9 changes: 6 additions & 3 deletions backend/workflow_manager/workflow_v2/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,17 +140,20 @@ def partial_update(self, request: Request, *args: Any, **kwargs: Any) -> Respons
"""Override partial_update to handle sharing notifications."""
# Get the workflow instance before update
workflow = self.get_object()
shared_users_updated = "shared_users" in request.data

# Store current shared users for comparison
current_shared_users = set(workflow.shared_users.all())
# Store current shared users for comparison only when sharing changes.
current_shared_users = set()
if shared_users_updated:
current_shared_users = set(workflow.shared_users.all())

# Perform the standard partial update
response = super().partial_update(request, *args, **kwargs)

# If update was successful and shared_users field was modified
if (
response.status_code == 200
and "shared_users" in request.data
and shared_users_updated
and bool(notification_plugin)
):
try:
Expand Down