diff --git a/taiga/projects/api.py b/taiga/projects/api.py index 1b18e4c3..914c4f77 100644 --- a/taiga/projects/api.py +++ b/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) diff --git a/tests/integration/test_roles.py b/tests/integration/test_roles.py new file mode 100644 index 00000000..e8bd89fa --- /dev/null +++ b/tests/integration/test_roles.py @@ -0,0 +1,82 @@ +# Copyright (C) 2014 Andrey Antukh +# Copyright (C) 2014 Jesús Espino +# Copyright (C) 2014 David Barragán +# Copyright (C) 2014 Anler Hernández +# 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 . + +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