fix:Report validation when saving with a duplicate name

Author: kcagran

apps/dashboard_reports/serializers.py

@@ -318,6 +318,20 @@ class Meta:
             },
         }
 
+    def validate(self, attrs: dict[str, Any]) -> dict[str, Any]:
+        """Ensure the user doesn't already have a filter set with this name."""
+        request = self.context.get("request")
+        user = getattr(request, "user", None)
+        if user is None:
+            return attrs
+        name = attrs.get("name", self.instance.name if self.instance is not None else None)
+        queryset = FilterSet.objects.filter(user=user, name=name)
+        if self.instance is not None:
+            queryset = queryset.exclude(pk=self.instance.pk)
+        if queryset.exists():
+            raise serializers.ValidationError({"name": "Report name already exists. Please choose a different name."})
+        return attrs
+
 
 class DashboardTelemetrySerializer(serializers.ModelSerializer):
     """Serializer for DashboardTelemetry"""

apps/dashboard_reports/viewsets/filter_sets.py

@@ -2,10 +2,11 @@
 from typing import Any
 
 from ansible_base.rest_pagination import DefaultPaginator
-from django.db import transaction
+from django.db import IntegrityError, transaction
 from django.db.models import QuerySet
 from drf_spectacular.utils import extend_schema, extend_schema_view
 from rest_framework import status
+from rest_framework.exceptions import ValidationError
 from rest_framework.mixins import CreateModelMixin, DestroyModelMixin, ListModelMixin, UpdateModelMixin
 from rest_framework.permissions import IsAuthenticated
 from rest_framework.request import Request
@@ -73,47 +74,32 @@ def get_queryset(self) -> QuerySet[FilterSet]:
         """Return filter sets belonging to the currently authenticated user."""
         return FilterSet.objects.filter(user=self.request.user)
 
+    def _clear_other_defaults(self, exclude_pk: int | None = None) -> None:
+        """Clear is_default on the user's other filter sets, optionally excluding one by pk."""
+        queryset = FilterSet.objects.filter(user=self.request.user, is_default=True)
+        if exclude_pk is not None:
+            queryset = queryset.exclude(pk=exclude_pk)
+        queryset.update(is_default=False)
+
     def perform_create(self, serializer: FilterSetSerializer) -> None:
         """Create a filter set, clearing any existing default if the new one is marked default."""
-        with transaction.atomic():
-            if serializer.validated_data.get("is_default", False):
-                FilterSet.objects.filter(user=self.request.user, is_default=True).update(is_default=False)
-            serializer.save(user=self.request.user)
-
-    def update(self, request: Request, *args: Any, **kwargs: Any) -> Response:
-        """
-        Updates the filter set. (PUT method)
-        """
-        instance = self.get_object()
-        serializer = self.get_serializer(instance, data=request.data, partial=False)
-        serializer.is_valid(raise_exception=True)
-        with transaction.atomic():
-            validated = serializer.validated_data
-            if validated.get("is_default", False):
-                FilterSet.objects.filter(user=self.request.user, is_default=True).exclude(pk=instance.pk).update(
-                    is_default=False
-                )
-            serializer.save(user=self.request.user)
-
-        return Response(serializer.data, status=status.HTTP_200_OK)
-
-    def partial_update(self, request: Request, *args: Any, **kwargs: Any) -> Response:
-        """
-        Partially updates the filter set. (PATCH method)
-        """
-        instance = self.get_object()
-        serializer = self.get_serializer(instance, data=request.data, partial=True)
-        serializer.is_valid(raise_exception=True)
-
-        with transaction.atomic():
-            validated = serializer.validated_data
-            if validated.get("is_default", False):
-                FilterSet.objects.filter(user=self.request.user, is_default=True).exclude(pk=instance.pk).update(
-                    is_default=False
-                )
-            serializer.save()
-
-        return Response(serializer.data, status=status.HTTP_200_OK)
+        try:
+            with transaction.atomic():
+                if serializer.validated_data.get("is_default", False):
+                    self._clear_other_defaults()
+                serializer.save(user=self.request.user)
+        except IntegrityError as exc:
+            raise ValidationError({"is_default": "Unable to set this filter set as default."}) from exc
+
+    def perform_update(self, serializer: FilterSetSerializer) -> None:
+        """Update a filter set, clearing any existing default if the new one is marked default."""
+        try:
+            with transaction.atomic():
+                if serializer.validated_data.get("is_default", False):
+                    self._clear_other_defaults(exclude_pk=serializer.instance.pk)
+                serializer.save(user=self.request.user)
+        except IntegrityError as exc:
+            raise ValidationError({"is_default": "Unable to set this filter set as default."}) from exc
 
     def destroy(self, request: Request, *args: Any, **kwargs: Any) -> Response:
         """

tests/unit/dashboard_reports/test_filter_sets.py

@@ -10,6 +10,8 @@
 - URL routing and router registration
 """
 
+from types import SimpleNamespace
+
 import pytest
 from django.test import TestCase
 from django.urls import resolve, reverse
@@ -76,7 +78,10 @@ def test_id_is_read_only(self) -> None:
         assert FilterSetSerializer().fields["id"].read_only is True
 
     def test_valid_payload_is_accepted(self) -> None:
-        serializer = FilterSetSerializer(data={"name": "Work Filters", "filters": _FILTERS_A, "is_default": False})
+        serializer = FilterSetSerializer(
+            data={"name": "Work Filters", "filters": _FILTERS_A, "is_default": False},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
         assert serializer.is_valid(), serializer.errors
 
     def test_name_is_required(self) -> None:
@@ -90,7 +95,10 @@ def test_filters_is_required(self) -> None:
         assert "filters" in serializer.errors
 
     def test_is_default_defaults_to_false_when_omitted(self) -> None:
-        serializer = FilterSetSerializer(data={"name": "Work Filters", "filters": _FILTERS_A})
+        serializer = FilterSetSerializer(
+            data={"name": "Work Filters", "filters": _FILTERS_A},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
         assert serializer.is_valid(), serializer.errors
         # When omitted, is_default is absent from validated_data (DRF applies the
         # default=False at save time, not in validated_data). Either absent or False is correct.
@@ -105,7 +113,37 @@ def test_maps_values_correctly(self) -> None:
 
     def test_filters_accepts_arbitrary_json(self) -> None:
         complex_filters = {"organization_id": [1, 2, 3], "date_range": "custom", "labels": [10, 20]}
-        serializer = FilterSetSerializer(data={"name": "Complex", "filters": complex_filters, "is_default": False})
+        serializer = FilterSetSerializer(
+            data={"name": "Complex", "filters": complex_filters, "is_default": False},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
+        assert serializer.is_valid(), serializer.errors
+
+    def test_validate_rejects_duplicate_name_for_same_user(self) -> None:
+        _create_filter_set(self.user, name="Existing")
+        serializer = FilterSetSerializer(
+            data={"name": "Existing", "filters": _FILTERS_A, "is_default": False},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
+        assert not serializer.is_valid()
+        assert "name" in serializer.errors
+
+    def test_validate_allows_same_name_for_different_user(self) -> None:
+        other_user = _create_user("other")
+        _create_filter_set(other_user, name="Shared Name")
+        serializer = FilterSetSerializer(
+            data={"name": "Shared Name", "filters": _FILTERS_A, "is_default": False},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
+        assert serializer.is_valid(), serializer.errors
+
+    def test_validate_allows_updating_instance_without_changing_name(self) -> None:
+        instance = _create_filter_set(self.user, name="Unchanged")
+        serializer = FilterSetSerializer(
+            instance,
+            data={"name": "Unchanged", "filters": _FILTERS_B, "is_default": False},
+            context={"request": SimpleNamespace(user=self.user)},
+        )
         assert serializer.is_valid(), serializer.errors
 
 
@@ -354,6 +392,22 @@ def test_create_without_filters_returns_400(self) -> None:
         response = self.client.post(url, {"name": "No Filters", "is_default": False}, format="json")
         assert response.status_code == status.HTTP_400_BAD_REQUEST
 
+    def test_create_with_duplicate_name_for_same_user_returns_400(self) -> None:
+        _create_filter_set(self.user, name="Duplicate")
+        url = reverse("v1:filter_sets-list")
+        response = self.client.post(url, {"name": "Duplicate", "filters": _FILTERS_A, "is_default": False}, format="json")
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert "name" in response.data
+
+    def test_create_with_duplicate_name_for_different_user_returns_201(self) -> None:
+        other_user = _create_user("other")
+        _create_filter_set(other_user, name="Shared Name")
+        url = reverse("v1:filter_sets-list")
+        response = self.client.post(
+            url, {"name": "Shared Name", "filters": _FILTERS_A, "is_default": False}, format="json"
+        )
+        assert response.status_code == status.HTTP_201_CREATED
+
 
 # =============================================================================
 # PUT /filter-sets/{id} — full update
@@ -423,6 +477,22 @@ def test_update_does_not_change_owner(self) -> None:
         self.filter_set.refresh_from_db()
         assert self.filter_set.user == self.user
 
+    def test_update_rejects_rename_to_existing_name(self) -> None:
+        """PUT that renames to a name already used by another of the user's filter sets must fail."""
+        _create_filter_set(self.user, name="Taken")
+        url = reverse("v1:filter_sets-detail", kwargs={"pk": self.filter_set.pk})
+        response = self.client.put(url, {"name": "Taken", "filters": _FILTERS_A, "is_default": False}, format="json")
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert "name" in response.data
+
+    def test_update_allows_keeping_its_own_name(self) -> None:
+        """PUT that keeps the same name on the same instance must not be treated as a duplicate."""
+        url = reverse("v1:filter_sets-detail", kwargs={"pk": self.filter_set.pk})
+        response = self.client.put(
+            url, {"name": "Original", "filters": _FILTERS_B, "is_default": False}, format="json"
+        )
+        assert response.status_code == status.HTTP_200_OK
+
 
 # =============================================================================
 # DELETE /filter-sets/{id} — destroy