From e0bdd93b5e226d626163e1dc200304926b97ed68 Mon Sep 17 00:00:00 2001 From: Ian Ross Date: Tue, 30 Aug 2016 14:38:39 +0200 Subject: [PATCH] Fix organization users API issues - Fix #603: org/project user deletion - Fix #608: new org users as admins --- cadasta/organization/serializers.py | 37 +++++++++++-------- .../organization/tests/test_serializers.py | 21 ++++++----- .../tests/test_views_api_organizations.py | 6 ++- .../tests/test_views_api_projects.py | 2 + cadasta/organization/views/api.py | 14 +++---- 5 files changed, 46 insertions(+), 34 deletions(-) diff --git a/cadasta/organization/serializers.py b/cadasta/organization/serializers.py index 04b42fbd7..5a89752e0 100644 --- a/cadasta/organization/serializers.py +++ b/cadasta/organization/serializers.py @@ -122,17 +122,18 @@ def get_url(self, object): class EntityUserSerializer(serializers.Serializer): username = serializers.CharField() - role = serializers.CharField() def to_representation(self, instance): if isinstance(instance, User): rep = UserSerializer(instance).data - rep['role'] = self.get_role_json(instance) + rep[self.Meta.role_key] = self.get_role_json(instance) return rep def to_internal_value(self, data): - data['role'] = self.set_roles(data.get('role', None)) - return super(EntityUserSerializer, self).to_internal_value(data) + data[self.Meta.role_key] = self.set_roles( + data.get(self.Meta.role_key, None) + ) + return super().to_internal_value(data) def validate_username(self, value): error = "" @@ -153,8 +154,10 @@ def validate_username(self, value): def create(self, validated_data): obj = self.context[self.Meta.context_key] + role_value = validated_data[self.Meta.role_key] + create_kwargs = { - self.Meta.role_key: validated_data['role'], + self.Meta.role_key: role_value, self.Meta.context_key: obj, 'user': self.user, } @@ -169,8 +172,10 @@ def create(self, validated_data): def update(self, instance, validated_data): role = self.get_roles_object(instance) - if 'role' in validated_data: - setattr(role, self.Meta.role_key, validated_data['role']) + role_value = validated_data[self.Meta.role_key] + + if self.Meta.role_key in validated_data: + setattr(role, self.Meta.role_key, role_value) role.save() @@ -182,18 +187,18 @@ class Meta: role_model = OrganizationRole context_key = 'organization' role_key = 'admin' + admin = serializers.BooleanField() def get_roles_object(self, instance): - if not hasattr(self, 'role'): - self.role = OrganizationRole.objects.get( - user=instance, - organization=self.context['organization']) + self.role = OrganizationRole.objects.get( + user=instance, + organization=self.context['organization']) return self.role def get_role_json(self, instance): role = self.get_roles_object(instance) - return 'Admin' if role.admin else 'User' + return role.admin def set_roles(self, data): admin = False @@ -233,6 +238,7 @@ class Meta: role_model = ProjectRole context_key = 'project' role_key = 'role' + role = serializers.CharField() def validate_username(self, value): super(ProjectUserSerializer, self).validate_username(value) @@ -244,10 +250,9 @@ def validate_username(self, value): "organization").format(username=value)) def get_roles_object(self, instance): - if not hasattr(self, 'role'): - self.role = ProjectRole.objects.get( - user=instance, - project=self.context['project']) + self.role = ProjectRole.objects.get( + user=instance, + project=self.context['project']) return self.role diff --git a/cadasta/organization/tests/test_serializers.py b/cadasta/organization/tests/test_serializers.py index 8557f9782..ceeb21110 100644 --- a/cadasta/organization/tests/test_serializers.py +++ b/cadasta/organization/tests/test_serializers.py @@ -334,7 +334,7 @@ def test_to_represenation(self): assert serializer.data['username'] == user.username assert serializer.data['email'] == user.email - assert serializer.data['role'] == 'User' + assert serializer.data['admin'] is False def test_list_to_representation(self): users = UserFactory.create_batch(2) @@ -349,16 +349,17 @@ def test_list_to_representation(self): assert len(serializer.data) == 3 + print(serializer.data) for u in serializer.data: - if u['username'] is org_admin.username: - assert u['role'] == 'Admin' + if u['username'] == org_admin.username: + assert u['admin'] is True else: - assert u['role'] == 'User' + assert u['admin'] is False def test_set_roles_with_username(self): user = UserFactory.create() org = OrganizationFactory.create() - data = {'username': user.username, 'role': 'Admin'} + data = {'username': user.username, 'admin': True} serializer = serializers.OrganizationUserSerializer( data=data, context={ @@ -377,7 +378,7 @@ def test_set_roles_with_username(self): def test_set_roles_with_email(self): user = UserFactory.create() org = OrganizationFactory.create() - data = {'username': user.email, 'role': 'Admin'} + data = {'username': user.email, 'admin': True} serializer = serializers.OrganizationUserSerializer( data=data, context={ @@ -395,7 +396,7 @@ def test_set_roles_with_email(self): def test_set_roles_for_user_that_does_not_exist(self): org = OrganizationFactory.create() - data = {'username': 'some-user', 'role': 'Admin'} + data = {'username': 'some-user', 'admin': True} serializer = serializers.OrganizationUserSerializer( data=data, context={'organization': org} ) @@ -410,7 +411,7 @@ def test_set_roles_for_duplicate_username(self): org = OrganizationFactory.create() user1 = UserFactory.create(email='some-user@some.com') UserFactory.create(email='some-user@some.com') - data = {'username': user1.email, 'role': 'Admin'} + data = {'username': user1.email, 'admin': 'true'} serializer = serializers.OrganizationUserSerializer( data=data, context={'organization': org} ) @@ -427,7 +428,7 @@ def test_update_roles_for_user(self): org = OrganizationFactory.create(add_users=[user]) serializer = serializers.OrganizationUserSerializer( user, - data={'role': 'Admin'}, + data={'admin': 'True'}, partial=True, context={'organization': org} ) @@ -466,7 +467,7 @@ def test_list_to_representation(self): assert len(serializer.data) == 3 for u in serializer.data: - if u['username'] is prj_admin.username: + if u['username'] == prj_admin.username: assert u['role'] == 'PM' else: assert u['role'] == 'PU' diff --git a/cadasta/organization/tests/test_views_api_organizations.py b/cadasta/organization/tests/test_views_api_organizations.py index 07b7ec588..5df73d205 100644 --- a/cadasta/organization/tests/test_views_api_organizations.py +++ b/cadasta/organization/tests/test_views_api_organizations.py @@ -8,6 +8,7 @@ from tutelary.models import Policy, assign_user_policies from core.tests.base_test_case import UserTestCase +from accounts.models import User from accounts.tests.factories import UserFactory from .factories import OrganizationFactory, clause from ..models import Organization, OrganizationRole @@ -372,6 +373,8 @@ def test_add_user(self): org = self.create_normal_org() new_user = UserFactory.create() self._post(org, {'username': new_user.username}, status=201, count=3) + r = OrganizationRole.objects.get(organization=org, user=new_user) + assert not r.admin def test_add_user_with_unauthorized_user(self): org = self.create_normal_org() @@ -470,7 +473,7 @@ def test_get_user(self): def test_update_user(self): user = UserFactory.create() org = OrganizationFactory.create(add_users=[user]) - self._patch(org, user.username, data={'roles': {'admin': True}}, + self._patch(org, user.username, data={'admin': 'true'}, status=200, count=1) role = OrganizationRole.objects.get(organization=org, user=user) assert role.admin is True @@ -481,6 +484,7 @@ def test_remove_user(self): org = OrganizationFactory.create(add_users=[user, user_to_remove]) self._delete(org, user_to_remove.username, status=204, count=1) assert user_to_remove not in org.users.all() + assert user_to_remove in User.objects.all() def test_remove_with_unauthorized_user(self): user = UserFactory.create() diff --git a/cadasta/organization/tests/test_views_api_projects.py b/cadasta/organization/tests/test_views_api_projects.py index 551d30b01..b74816fe0 100644 --- a/cadasta/organization/tests/test_views_api_projects.py +++ b/cadasta/organization/tests/test_views_api_projects.py @@ -8,6 +8,7 @@ from tutelary.models import Policy, assign_user_policies from core.tests.base_test_case import UserTestCase +from accounts.models import User from accounts.tests.factories import UserFactory from .factories import OrganizationFactory, ProjectFactory, clause from ..models import Project, ProjectRole, OrganizationRole @@ -199,6 +200,7 @@ def _delete(self, org, project, user, auth=None, status=None, count=None): assert response.status_code == status if count is not None: assert project.users.count() == count + assert User.objects.get(username=user) in User.objects.all() def test_get_user(self): user = UserFactory.create() diff --git a/cadasta/organization/views/api.py b/cadasta/organization/views/api.py index 5a78b7c32..fa81dbc98 100644 --- a/cadasta/organization/views/api.py +++ b/cadasta/organization/views/api.py @@ -4,7 +4,7 @@ from accounts.models import User -from ..models import Organization +from ..models import Organization, OrganizationRole, ProjectRole from .. import serializers from . import mixins @@ -63,9 +63,9 @@ class OrganizationUsersDetail(APIPermissionRequiredMixin, permission_required = 'org.users.remove' def destroy(self, request, *args, **kwargs): - user = self.get_object() - role = self.org.users.get(id=user.id) - role.delete() + OrganizationRole.objects.get( + organization=self.org, user=self.get_object() + ).delete() return Response(status=status.HTTP_204_NO_CONTENT) @@ -195,8 +195,8 @@ class ProjectUsersDetail(APIPermissionRequiredMixin, } def destroy(self, request, *args, **kwargs): - user = self.get_object() - role = self.prj.users.get(id=user.id) - role.delete() + ProjectRole.objects.get( + project=self.prj, user=self.get_object() + ).delete() return Response(status=status.HTTP_204_NO_CONTENT)