Merge pull request #51 from rapidpro/removing_users

Don't delete users, just remove them from orgs

casepro/profiles/__init__.py

@@ -123,9 +123,16 @@ def _user_can_edit(user, org, other):
     return other_partner and user.can_manage(other_partner)  # manager can edit users in same partner org
 
 
-def _user_release(user):
-    user.is_active = False
-    user.save(update_fields=('is_active',))
+def _user_remove_from_org(user, org):
+    # remove user from all org groups
+    org.administrators.remove(user)
+    org.editors.remove(user)
+    org.viewers.remove(user)
+
+    # remove from any partner for this org
+    if user.profile.partner and user.profile.partner.org == org:
+        user.profile.partner = None
+        user.profile.save(update_fields=('partner',))
 
 
 def _user_unicode(user):
@@ -151,6 +158,6 @@ def _user_as_json(user):
 User.can_administer = _user_can_administer
 User.can_manage = _user_can_manage
 User.can_edit = _user_can_edit
-User.release = _user_release
+User.remove_from_org = _user_remove_from_org
 User.__unicode__ = _user_unicode
 User.as_json = _user_as_json

casepro/profiles/migrations/0004_fix_deleted_users.py

@@ -0,0 +1,46 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+def fix_deleted_users(apps, schema_editor):
+    """
+    Previously deleting users from orgs caused them to be set as inactive, which then causes difficulties if someone
+    tries to re-add that user to an org. This re-activates deleted users and instead removes them from org groups.
+    """
+    Org = apps.get_model('orgs', 'Org')
+    User = apps.get_model('auth', 'User')
+
+    all_orgs = Org.objects.all()
+    inactive_users = list(User.objects.filter(is_active=False))
+
+    for user in inactive_users:
+        # remove user from all org groups
+        for org in all_orgs:
+            org.administrators.remove(user)
+            org.editors.remove(user)
+            org.viewers.remove(user)
+
+        # remove from any partner for this org
+        if user.profile.partner:
+            user.profile.partner = None
+            user.profile.save(update_fields=('partner',))
+
+        # re-activate user
+        user.is_active = True
+        user.save(update_fields=('is_active',))
+
+    if inactive_users:
+        print("Fixed %d inactive users" % len(inactive_users))
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('profiles', '0003_username_max_length'),
+    ]
+
+    operations = [
+        migrations.RunPython(fix_deleted_users)
+    ]

casepro/profiles/tests.py

@@ -111,9 +111,19 @@ def test_can_edit(self):
         self.assertFalse(self.user2.can_edit(self.unicef, self.user3))
         self.assertFalse(self.user2.can_edit(self.unicef, self.user3))
 
-    def test_release(self):
-        self.user1.release()
-        self.assertFalse(self.user1.is_active)
+    def test_remove_from_org(self):
+        # try with org admin
+        self.admin.remove_from_org(self.unicef)
+
+        self.admin.refresh_from_db()
+        self.assertIsNone(self.unicef.get_user_org_group(self.admin))
+
+        # try with partner user
+        self.user1.remove_from_org(self.unicef)
+
+        self.user1.refresh_from_db()
+        self.assertIsNone(self.unicef.get_user_org_group(self.user1))
+        self.assertIsNone(self.user1.get_partner(self.unicef))
 
     def test_unicode(self):
         self.assertEqual(unicode(self.superuser), "root")
@@ -462,6 +472,40 @@ def test_self(self):
         self.assertFalse(user.profile.change_password)
         self.assertNotEqual(user.password, old_password_hash)
 
+    def test_delete(self):
+        # partner data analyst can't delete anyone
+        self.login(self.user2)
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.admin.pk]))
+        self.assertEqual(response.status_code, 302)
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.user1.pk]))
+        self.assertEqual(response.status_code, 302)
+
+        # partner manager can delete fellow partner org users but not org admins
+        self.login(self.user1)
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.user2.pk]))
+        self.assertEqual(response.status_code, 204)
+        self.assertIsNone(self.unicef.get_user_org_group(self.user2))
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.admin.pk]))
+        self.assertEqual(response.status_code, 302)
+
+        # admins can delete anyone in their org
+        self.login(self.admin)
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.user1.pk]))
+        self.assertEqual(response.status_code, 204)
+        self.assertIsNone(self.unicef.get_user_org_group(self.user1))
+
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.norbert.pk]))
+        self.assertEqual(response.status_code, 302)
+
+        # but not themselves
+        response = self.url_post('unicef', reverse('profiles.user_delete', args=[self.admin.pk]))
+        self.assertEqual(response.status_code, 302)
+
 
 class ForcePasswordChangeMiddlewareTest(BaseCasesTest):
     @override_settings(CELERY_ALWAYS_EAGER=True, CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, BROKER_BACKEND='memory')

casepro/profiles/views.py

@@ -229,11 +229,13 @@ class Delete(OrgPermsMixin, SmartDeleteView):
         cancel_url = '@profiles.user_list'
 
         def has_permission(self, request, *args, **kwargs):
-            return request.user.can_edit(request.org, self.get_object())
+            user = self.get_object()
+            return request.user.can_edit(request.org, user) and request.user != user
 
         def post(self, request, *args, **kwargs):
             user = self.get_object()
-            user.release()
+            user.remove_from_org(request.org)
+
             return HttpResponse(status=204)
 
     class List(OrgPermsMixin, UserFieldsMixin, SmartListView):