Merge pull request #22 from aarcro/master

[#20] Don't interfere with other groups

README.md

@@ -25,6 +25,7 @@ This packages uses tox to run tests on multiple evironments, please run ```pytho
 [sdee](https://github.com/sdee)  
 [reduxionist](https://github.com/reduxionist)  
 [myonov](https://github.com/myonov)
+[aarcro](https://github.com/aarcro)
 
 ## Help
 

rolepermissions/roles.py

@@ -48,14 +48,20 @@ def get_name(cls):
 
     @classmethod
     def assign_role_to_user(cls, user):
-        old_groups = user.groups.all()
+        """Deletes all of user's previous roles, and removes all permissions
+        mentioned in their available_permissions property.
 
-        if old_groups:
-            old_group = old_groups[0] # assumes a user has only one group
+        :returns: :py:class:`django.contrib.auth.models.Group` The group for the
+            new role.
+        """
+
+        old_groups = user.groups.filter(name__in=registered_roles.keys())
+
+        for old_group in old_groups:  # Normally there is only one, but remove all other role groups
             role = RolesManager.retrieve_role(old_group.name)
             permissions_to_remove = Permission.objects.filter(codename__in=role.permission_names_list()).all()
             user.user_permissions.remove(*permissions_to_remove)
-            user.groups.clear()
+        user.groups.remove(*old_groups)
 
         group, created = Group.objects.get_or_create(name=cls.get_name())
         user.groups.add(group)
@@ -81,18 +87,18 @@ def get_default_true_permissions(cls):
     @classmethod
     def get_or_create_permissions(cls, permission_names):
         user_ct = ContentType.objects.get_for_model(get_user_model())
-        permissions = list(Permission.objects.filter(content_type=user_ct, codename__in=permission_names).all())
+        permissions = list(Permission.objects.filter(
+            content_type=user_ct, codename__in=permission_names).all())
 
         if len(permissions) != len(permission_names):
             for permission_name in permission_names:
-                permission, created = Permission.objects.get_or_create(content_type=user_ct, codename=permission_name)
+                permission, created = Permission.objects.get_or_create(
+                    content_type=user_ct, codename=permission_name)
                 if created:
                     permissions.append(permission)
 
         return permissions
 
-
     @classmethod
     def get_default(cls, permission_name):
         return cls.available_permissions[permission_name]
-

rolepermissions/tests/test_roles.py

@@ -1,6 +1,7 @@
 
 from django.test import TestCase
 from django.contrib.auth import get_user_model
+from django.contrib.auth.models import Group
 
 from model_mommy import mommy
 
@@ -13,12 +14,14 @@ class RolRole1(AbstractUserRole):
         'permission2': True,
     }
 
+
 class RolRole2(AbstractUserRole):
     available_permissions = {
         'permission3': True,
         'permission4': False,
     }
 
+
 class RolRole3(AbstractUserRole):
     role_name = 'new_name'
     available_permissions = {
@@ -94,15 +97,33 @@ def test_change_user_role(self):
 
         self.assertEquals(user_role.name, 'rol_role1')
 
-        user_role = RolRole2.assign_role_to_user(user)
+        new_user_role = RolRole2.assign_role_to_user(user)
+
+        self.assertEquals(new_user_role.name, 'rol_role2')
+        self.assertIn(new_user_role, user.groups.all())
+        self.assertNotIn(user_role, user.groups.all())
+
+    def test_dont_remove_other_groups(self):
+        user = mommy.make(get_user_model())
+        other_group = mommy.make(Group)
+        user.groups.add(other_group)
+
+        user_role = RolRole1.assign_role_to_user(user)
+
+        self.assertEquals(user_role.name, 'rol_role1')
+
+        new_user_role = RolRole2.assign_role_to_user(user)
 
-        self.assertEquals(user_role.name, 'rol_role2')
+        self.assertEquals(new_user_role.name, 'rol_role2')
+        self.assertIn(new_user_role, user.groups.all())
+        self.assertIn(other_group, user.groups.all())
+        self.assertNotIn(user_role, user.groups.all())
 
     def test_delete_old_permissions_on_role_change(self):
         user = mommy.make(get_user_model())
 
         RolRole1().assign_role_to_user(user)
-        
+
         permissions = user.user_permissions.all()
 
         permission_names = [n.codename for n in permissions]
@@ -123,7 +144,6 @@ def test_delete_old_permissions_on_role_change(self):
         self.assertNotIn('permission4', permission_names)
         self.assertEquals(len(permissions), 1)
 
-
     def test_permission_names_list(self):
         self.assertIn('permission1', RolRole1.permission_names_list())
         self.assertIn('permission2', RolRole1.permission_names_list())