[backport] Fix wrong handling moveTo parameter on role destroy.

taiga/projects/api.py

@@ -256,20 +256,14 @@ class RolesViewSet(ModelCrudViewSet):
     filter_backends = (filters.CanViewProjectFilterBackend,)
     filter_fields = ('project',)
 
-    @tx.atomic
-    def destroy(self, request, *args, **kwargs):
-        moveTo = self.request.QUERY_PARAMS.get('moveTo', None)
-        if moveTo is None:
-            return super().destroy(request, *args, **kwargs)
+    def pre_delete(self, obj):
+        move_to = self.request.QUERY_PARAMS.get('moveTo', None)
+        if move_to:
+            role_dest = get_object_or_404(self.model, project=obj.project, id=move_to)
+            qs = models.Membership.objects.filter(project_id=obj.project.pk, role=obj)
+            qs.update(role=role_dest)
 
-        obj = self.get_object_or_none()
-
-        moveItem = get_object_or_404(self.model, project=obj.project, id=moveTo)
-
-        self.check_permissions(request, 'destroy', obj)
-
-        models.Membership.objects.filter(project=obj.project, role=obj).update(role=moveItem)
-        return super().destroy(request, *args, **kwargs)
+        super().pre_delete(obj)
 
 
 # User Stories commin ViewSets
@@ -317,19 +311,19 @@ class PointsViewSet(ModelCrudViewSet, BulkUpdateOrderMixin):
 class MoveOnDestroyMixin(object):
     @tx.atomic
     def destroy(self, request, *args, **kwargs):
-        moveTo = self.request.QUERY_PARAMS.get('moveTo', None)
-        if moveTo is None:
+        move_to = self.request.QUERY_PARAMS.get('moveTo', None)
+        if move_to is None:
             return super().destroy(request, *args, **kwargs)
 
         obj = self.get_object_or_none()
 
-        moveItem = get_object_or_404(self.model, project=obj.project, id=moveTo)
+        move_item = get_object_or_404(self.model, project=obj.project, id=move_to)
 
         self.check_permissions(request, 'destroy', obj)
-        kwargs = {self.move_on_destroy_related_field: moveItem}
+        kwargs = {self.move_on_destroy_related_field: move_item}
         self.move_on_destroy_related_class.objects.filter(project=obj.project, **{self.move_on_destroy_related_field: obj}).update(**kwargs)
         if getattr(obj.project, self.move_on_destroy_project_default_field) == obj:
-            setattr(obj.project, self.move_on_destroy_project_default_field, moveItem)
+            setattr(obj.project, self.move_on_destroy_project_default_field, move_item)
             obj.project.save()
         return super().destroy(request, *args, **kwargs)
 

tests/integration/test_roles.py

@@ -0,0 +1,82 @@
+# Copyright (C) 2014 Andrey Antukh <niwi@niwi.be>
+# Copyright (C) 2014 Jesús Espino <jespinog@gmail.com>
+# Copyright (C) 2014 David Barragán <bameda@dbarragan.com>
+# Copyright (C) 2014 Anler Hernández <hello@anler.me>
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import pytest
+from unittest.mock import patch, Mock
+
+from django.apps import apps
+from django.core.urlresolvers import reverse
+
+from taiga.base.utils import json
+
+from taiga.users.models import Role
+from taiga.projects.models import Membership
+from taiga.projects.models import Project
+from taiga.projects.userstories.serializers import UserStorySerializer
+
+from .. import factories as f
+
+
+pytestmark = pytest.mark.django_db
+
+def test_destroy_role_and_reassign_members(client):
+    user1 = f.UserFactory.create()
+    user2 = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user1)
+    role1 = f.RoleFactory.create(project=project)
+    role2 = f.RoleFactory.create(project=project)
+    member = f.MembershipFactory.create(project=project, user=user1, role=role1)
+    member = f.MembershipFactory.create(project=project, user=user2, role=role2)
+
+    url = reverse("roles-detail", args=[role2.pk]) + "?moveTo={}".format(role1.pk)
+
+    client.login(user1)
+
+    response = client.delete(url)
+    assert response.status_code == 204
+
+    qs = Role.objects.filter(project=project)
+    assert qs.count() == 1
+
+    qs = Membership.objects.filter(project=project, role_id=role2.pk)
+    assert qs.count() == 0
+
+    qs = Membership.objects.filter(project=project, role_id=role1.pk)
+    assert qs.count() == 2
+
+def test_destroy_role_and_reassign_members_with_deleted_project(client):
+    """
+    Regression test, that fixes some 500 errors on production
+    """
+
+    user1 = f.UserFactory.create()
+    user2 = f.UserFactory.create()
+    project = f.ProjectFactory.create(owner=user1)
+    role1 = f.RoleFactory.create(project=project)
+    role2 = f.RoleFactory.create(project=project)
+    member = f.MembershipFactory.create(project=project, user=user1, role=role1)
+    member = f.MembershipFactory.create(project=project, user=user2, role=role2)
+
+    Project.objects.filter(pk=project.id).delete()
+
+    url = reverse("roles-detail", args=[role2.pk]) + "?moveTo={}".format(role1.pk)
+    client.login(user1)
+
+    response = client.delete(url)
+
+    # FIXME: really should return 403? I think it should be 404
+    assert response.status_code == 403, response.content