More cleanup

Author: XanderVertegaal

backend/annotation/serializers.py

@@ -53,10 +53,7 @@ def get_removable(self, labeling: Labeling) -> bool:
         if user is None or user.is_anonymous:
             return False
 
-        if user.is_superuser:
-            return True
-
-        if user.has_perm("annotation.delete_any_labeling"):
+        if user.is_superuser or user.has_perm("annotation.delete_any_labeling"):
             return True
 
         if user.has_perm("annotation.delete_own_labeling"):

backend/annotation/urls.py

@@ -72,55 +72,70 @@ def create(self, request: Request) -> Response:
 
         selected_label_ids = {label["id"] for label in selected_labels}
 
+        try:
+            self._update_labelings(problem, user, selected_label_ids, remarks)
+        except PermissionError as e:
+            return Response({"detail": str(e)}, status=403)
+
+        return Response({"ok": True}, status=HTTP_200_OK)
+
+    def _update_labelings(
+        self,
+        problem: Problem,
+        user: User,
+        selected_label_ids: set[int],
+        remarks: str,
+    ) -> None:
+        """Update labelings for a problem based on selected labels."""
+
         with transaction.atomic():
-            # Get all active labelings for this problem
             active_labelings = Labeling.objects.filter(
                 problem=problem, removed_at__isnull=True
-            )
+            ).select_related("label", "attached_by")
 
-            # Determine which labels to remove and which to add
             current_label_ids = {labeling.label.pk for labeling in active_labelings}
             labels_to_remove = current_label_ids - selected_label_ids
             labels_to_add = selected_label_ids - current_label_ids
 
-            # Mark labels as removed
             for labeling in active_labelings:
                 if labeling.label.pk in labels_to_remove:
-                    # Check permission for removal
-                    if not self._can_remove_labeling(user, labeling):
-                        logger.warning(
-                            f"User {user.username} attempted to remove label {labeling.label.pk} "
-                            f"attached by {labeling.attached_by.username}"
-                        )
-                        return Response(
-                            {
-                                "error": "You can only remove labels you attached yourself."
-                            },
-                            status=403,
-                        )
-                    labeling.removed_at = timezone.now()
-                    labeling.removed_by = user
-                    if remarks:
-                        labeling.notes = remarks
-                    labeling.save()
-
-            # Add new labels
+                    self._remove_labeling(
+                        labeling=labeling,
+                        user=user,
+                        remarks=remarks,
+                    )
+
             for label_id in labels_to_add:
-                Labeling.objects.create(
-                    problem=problem,
+                self._create_labeling(
                     label_id=label_id,
-                    attached_by=user,
-                    notes=remarks,
+                    problem=problem,
+                    user=user,
+                    remarks=remarks,
                 )
 
-        return Response({"ok": True}, status=HTTP_200_OK)
-
-    def _can_remove_labeling(self, user: User, labeling: Labeling) -> bool:
-        """Check if user can remove a specific labeling."""
-        if user.is_superuser or user.has_perm("annotation.delete_any_labeling"):
-            return True
-
-        if user.has_perm("annotation.delete_own_labeling"):
-            return labeling.attached_by.pk == user.pk
-
-        return False
+    def _create_labeling(
+        self, label_id: int, problem: Problem, user: User, remarks: str
+    ) -> None:
+        """Create a new labeling."""
+
+        Labeling.objects.create(
+            problem=problem,
+            label_id=label_id,
+            attached_by=user,
+            notes=remarks,
+        )
+
+    def _remove_labeling(self, labeling: Labeling, user: User, remarks: str) -> None:
+        """Mark a labeling as removed."""
+
+        if not user.can_remove_labeling(labeling):
+            logger.warning(
+                f"User {user.username} attempted to remove label {labeling.label.pk} "
+                f"attached by {labeling.attached_by.username}"
+            )
+            raise PermissionError("You can only remove labels you attached yourself.")
+        labeling.removed_at = timezone.now()
+        labeling.removed_by = user
+        if remarks:
+            labeling.notes = remarks
+        labeling.save()

backend/annotation/views.py

@@ -219,8 +219,8 @@ def test_master_annotator_can_remove_others_labeling(
         assert labeling_by_annotator.removed_by == master_annotator
 
 
-class TestLabelingAddAndRemoveOperations:
-    """Tests for add/remove labeling operations."""
+class TestLabelingAddAndRemove:
+    """Tests for adding / removing labelings."""
 
     def test_adding_label_creates_labeling(
         self, api_client, annotator, sample_problem, sample_label
@@ -288,25 +288,3 @@ def test_keeping_existing_labels_unchanged(
         assert Labeling.objects.filter(
             problem=sample_problem, label=another_label, removed_at__isnull=True
         ).exists()
-
-
-class TestUnsupportedHttpMethods:
-    """Tests for unsupported HTTP methods."""
-
-    def test_put_not_allowed(self, api_client, master_annotator, sample_label):
-        """PUT method should not be allowed."""
-        api_client.force_authenticate(user=master_annotator)
-        response = api_client.put(f"/api/label/{sample_label.id}/", {}, format="json")
-        assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED
-
-    def test_patch_not_allowed(self, api_client, master_annotator, sample_label):
-        """PATCH method should not be allowed."""
-        api_client.force_authenticate(user=master_annotator)
-        response = api_client.patch(f"/api/label/{sample_label.id}/", {}, format="json")
-        assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED
-
-    def test_delete_not_allowed(self, api_client, master_annotator, sample_label):
-        """DELETE method should not be allowed."""
-        api_client.force_authenticate(user=master_annotator)
-        response = api_client.delete(f"/api/label/{sample_label.id}/")
-        assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED