From 14ec6c0f44c4ad18a19e4db81de20ee647fc1047 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Norman=20J=C3=A4ckel?= <openslides@normanjaeckel.de>
Date: Mon, 6 Mar 2017 16:34:20 +0100
Subject: [PATCH] Improved autoupdate on permission change.

---
 .travis.yml                             |  2 +-
 CHANGELOG                               |  1 +
 openslides/agenda/apps.py               |  9 ++-
 openslides/assignments/apps.py          | 11 +++-
 openslides/assignments/signals.py       |  7 +-
 openslides/core/apps.py                 |  7 +-
 openslides/core/signals.py              | 11 ++--
 openslides/mediafiles/apps.py           | 11 +++-
 openslides/mediafiles/signals.py        |  9 ++-
 openslides/motions/apps.py              |  9 ++-
 openslides/motions/signals.py           | 13 ++--
 openslides/topics/apps.py               |  9 ++-
 openslides/users/apps.py                |  9 ++-
 openslides/users/signals.py             |  9 ++-
 openslides/users/views.py               | 85 ++++++++++++++-----------
 openslides/utils/autoupdate.py          | 29 ++++-----
 setup.cfg                               |  4 +-
 tests/integration/users/test_viewset.py | 47 ++++++++++++++
 18 files changed, 186 insertions(+), 96 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index d9c90f81c..472e31682 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -28,6 +28,6 @@ script:
   - coverage report --fail-under=44
 
   - DJANGO_SETTINGS_MODULE='tests.settings' coverage run ./manage.py test tests.integration
-  - coverage report --fail-under=73
+  - coverage report --fail-under=74
 
   - DJANGO_SETTINGS_MODULE='tests.old.settings' ./manage.py test tests.old
diff --git a/CHANGELOG b/CHANGELOG
index 6d1607f67..d18e67e07 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -87,6 +87,7 @@ Elections:
 
 Users:
 - Added new matrix-interface for managing groups and their permissions.
+- Added autoupdate on permission change (permission added).
 - Improved password reset view for administrators.
 - Changed field for initial password to an unchangeable field.
 - Added new field for participant number.
diff --git a/openslides/agenda/apps.py b/openslides/agenda/apps.py
index 8c94160b0..e0123cfe9 100644
--- a/openslides/agenda/apps.py
+++ b/openslides/agenda/apps.py
@@ -1,5 +1,7 @@
 from django.apps import AppConfig
 
+from ..utils.collection import Collection
+
 
 class AgendaAppConfig(AppConfig):
     name = 'openslides.agenda'
@@ -37,5 +39,8 @@ class AgendaAppConfig(AppConfig):
         router.register(self.get_model('Item').get_collection_string(), ItemViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
-        return [Collection(self.get_model('Item').get_collection_string())]
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
+        yield Collection(self.get_model('Item').get_collection_string())
diff --git a/openslides/assignments/apps.py b/openslides/assignments/apps.py
index 8dc515f69..16634c46e 100644
--- a/openslides/assignments/apps.py
+++ b/openslides/assignments/apps.py
@@ -1,5 +1,7 @@
 from django.apps import AppConfig
 
+from ..utils.collection import Collection
+
 
 class AssignmentsAppConfig(AppConfig):
     name = 'openslides.assignments'
@@ -26,15 +28,18 @@ class AssignmentsAppConfig(AppConfig):
         # Connect signals.
         permission_change.connect(
             get_permission_change_data,
-            dispatch_uid='assignment_get_permission_change_data')
+            dispatch_uid='assignments_get_permission_change_data')
 
         # Register viewsets.
         router.register(self.get_model('Assignment').get_collection_string(), AssignmentViewSet)
         router.register('assignments/poll', AssignmentPollViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
-        return [Collection(self.get_model('Assignment').get_collection_string())]
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
+        yield Collection(self.get_model('Assignment').get_collection_string())
 
     def get_angular_constants(self):
         assignment = self.get_model('Assignment')
diff --git a/openslides/assignments/signals.py b/openslides/assignments/signals.py
index 00b353e41..22182ba51 100644
--- a/openslides/assignments/signals.py
+++ b/openslides/assignments/signals.py
@@ -3,11 +3,10 @@ from django.apps import apps
 
 def get_permission_change_data(sender, permissions=None, **kwargs):
     """
-    Returns all necessary collections if a 'can_see' permission changes.
+    Yields all necessary collections if 'assignments.can_see' permission changes.
     """
     assignments_app = apps.get_app_config(app_label='assignments')
     for permission in permissions:
         # There could be only one 'assignment.can_see' and then we want to return data.
-        if permission.content_type.app_label == assignment_app.label and permission.codename == 'can_see':
-            return assignment_app.get_startup_elements()
-    return None
+        if permission.content_type.app_label == assignments_app.label and permission.codename == 'can_see':
+            yield from assignments_app.get_startup_elements()
diff --git a/openslides/core/apps.py b/openslides/core/apps.py
index 76e809f6e..389316682 100644
--- a/openslides/core/apps.py
+++ b/openslides/core/apps.py
@@ -1,6 +1,8 @@
 from django.apps import AppConfig
 from django.conf import settings
 
+from ..utils.collection import Collection
+
 
 class CoreAppConfig(AppConfig):
     name = 'openslides.core'
@@ -51,8 +53,11 @@ class CoreAppConfig(AppConfig):
         router.register(self.get_model('Countdown').get_collection_string(), CountdownViewSet)
 
     def get_startup_elements(self):
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
         from .config import config
-        from ..utils.collection import Collection
         for model in ('Projector', 'ChatMessage', 'Tag', 'ProjectorMessage', 'Countdown'):
             yield Collection(self.get_model(model).get_collection_string())
         yield Collection(config.get_collection_string())
diff --git a/openslides/core/signals.py b/openslides/core/signals.py
index 5a5019017..2d3f2f8e7 100644
--- a/openslides/core/signals.py
+++ b/openslides/core/signals.py
@@ -4,12 +4,15 @@ from django.contrib.contenttypes.models import ContentType
 from django.db.models import Q
 from django.dispatch import Signal
 
+from ..utils.collection import Collection
+
 # This signal is send when the migrate command is done. That means it is sent
 # after post_migrate sending and creating all Permission objects. Don't use it
 # for other things than dealing with Permission objects.
 post_permission_creation = Signal()
 
-# This signal is send, if a permission is changed.
+# This signal is sent if a permission is changed (e. g. a group gets a new
+# permission). Connected receivers may yield Collections.
 permission_change = Signal()
 
 
@@ -25,14 +28,12 @@ def delete_django_app_permissions(sender, **kwargs):
     Permission.objects.filter(content_type__in=contenttypes).delete()
 
 
-def get_permission_change_data(sender, permissions=None, **kwargs):
+def get_permission_change_data(sender, permissions, **kwargs):
     """
-    Returns all necessary collections if the coupled permission changes.
+    Yields all necessary collections if the respective permissions change.
     """
-    from ..utils.collection import Collection
     core_app = apps.get_app_config(app_label='core')
     for permission in permissions:
-        # There could be only one 'users.can_see' and then we want to return data.
         if permission.content_type.app_label == core_app.label:
             if permission.codename == 'can_see_projector':
                 yield Collection(core_app.get_model('Projector').get_collection_string())
diff --git a/openslides/mediafiles/apps.py b/openslides/mediafiles/apps.py
index f86fcf329..76ef8b5b6 100644
--- a/openslides/mediafiles/apps.py
+++ b/openslides/mediafiles/apps.py
@@ -1,5 +1,7 @@
 from django.apps import AppConfig
 
+from ..utils.collection import Collection
+
 
 class MediafilesAppConfig(AppConfig):
     name = 'openslides.mediafiles'
@@ -21,11 +23,14 @@ class MediafilesAppConfig(AppConfig):
         # Connect signals.
         permission_change.connect(
             get_permission_change_data,
-            dispatch_uid='mediafile_get_permission_change_data')
+            dispatch_uid='mediafiles_get_permission_change_data')
 
         # Register viewsets.
         router.register(self.get_model('Mediafile').get_collection_string(), MediafileViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
-        return [Collection(self.get_model('Mediafile').get_collection_string())]
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
+        yield Collection(self.get_model('Mediafile').get_collection_string())
diff --git a/openslides/mediafiles/signals.py b/openslides/mediafiles/signals.py
index 096e3ef6a..ad297ac19 100644
--- a/openslides/mediafiles/signals.py
+++ b/openslides/mediafiles/signals.py
@@ -3,11 +3,10 @@ from django.apps import apps
 
 def get_permission_change_data(sender, permissions=None, **kwargs):
     """
-    Returns all necessary collections if a 'can_see' permission changes.
+    Yields all necessary collections if 'mediafiles.can_see' permission changes.
     """
-    mediafile_app = apps.get_app_config(app_label='mediafiles')
+    mediafiles_app = apps.get_app_config(app_label='mediafiles')
     for permission in permissions:
         # There could be only one 'mediafiles.can_see' and then we want to return data.
-        if permission.content_type.app_label == mediafile_app.label and permission.codename == 'can_see':
-            return mediafile_app.get_startup_elements()
-    return None
+        if permission.content_type.app_label == mediafiles_app.label and permission.codename == 'can_see':
+            yield from mediafiles_app.get_startup_elements()
diff --git a/openslides/motions/apps.py b/openslides/motions/apps.py
index ce484caf8..5a037e16b 100644
--- a/openslides/motions/apps.py
+++ b/openslides/motions/apps.py
@@ -1,6 +1,8 @@
 from django.apps import AppConfig
 from django.db.models.signals import post_migrate
 
+from ..utils.collection import Collection
+
 
 class MotionsAppConfig(AppConfig):
     name = 'openslides.motions'
@@ -28,7 +30,7 @@ class MotionsAppConfig(AppConfig):
         post_migrate.connect(create_builtin_workflows, dispatch_uid='motion_create_builtin_workflows')
         permission_change.connect(
             get_permission_change_data,
-            dispatch_uid='motion_get_permission_change_data')
+            dispatch_uid='motions_get_permission_change_data')
 
         # Register viewsets.
         router.register(self.get_model('Category').get_collection_string(), CategoryViewSet)
@@ -40,6 +42,9 @@ class MotionsAppConfig(AppConfig):
         router.register('motions/motionpoll', MotionPollViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
         for model in ('Category', 'Motion', 'MotionBlock', 'Workflow', 'MotionChangeRecommendation'):
             yield Collection(self.get_model(model).get_collection_string())
diff --git a/openslides/motions/signals.py b/openslides/motions/signals.py
index 3c9a6c49d..64a0cdb9e 100644
--- a/openslides/motions/signals.py
+++ b/openslides/motions/signals.py
@@ -105,13 +105,12 @@ def create_builtin_workflows(sender, **kwargs):
     workflow_2.save()
 
 
-def get_permission_change_data(sender, permissions=None, **kwargs):
+def get_permission_change_data(sender, permissions, **kwargs):
     """
-    Returns all necessary collections if a 'can_see' permission changes.
+    Yields all necessary collections if 'motions.can_see' permission changes.
     """
-    motion_app = apps.get_app_config(app_label='motions')
+    motions_app = apps.get_app_config(app_label='motions')
     for permission in permissions:
-        # There could be only one 'motion.can_see' and then we want to return data.
-        if permission.content_type.app_label == motion_app.label and permission.codename == 'can_see':
-            return motion_app.get_startup_elements()
-    return None
+        # There could be only one 'motions.can_see' and then we want to return data.
+        if permission.content_type.app_label == motions_app.label and permission.codename == 'can_see':
+            yield from motions_app.get_startup_elements()
diff --git a/openslides/topics/apps.py b/openslides/topics/apps.py
index 49eba7af8..f7f246288 100644
--- a/openslides/topics/apps.py
+++ b/openslides/topics/apps.py
@@ -1,5 +1,7 @@
 from django.apps import AppConfig
 
+from ..utils.collection import Collection
+
 
 class TopicsAppConfig(AppConfig):
     name = 'openslides.topics'
@@ -20,5 +22,8 @@ class TopicsAppConfig(AppConfig):
         router.register(self.get_model('Topic').get_collection_string(), TopicViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
-        return [Collection(self.get_model('Topic').get_collection_string())]
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
+        yield Collection(self.get_model('Topic').get_collection_string())
diff --git a/openslides/users/apps.py b/openslides/users/apps.py
index d0f99424a..7bbd60678 100644
--- a/openslides/users/apps.py
+++ b/openslides/users/apps.py
@@ -1,5 +1,7 @@
 from django.apps import AppConfig
 
+from ..utils.collection import Collection
+
 
 class UsersAppConfig(AppConfig):
     name = 'openslides.users'
@@ -29,13 +31,16 @@ class UsersAppConfig(AppConfig):
             dispatch_uid='create_builtin_groups_and_admin')
         permission_change.connect(
             get_permission_change_data,
-            dispatch_uid='user_get_permission_change_data')
+            dispatch_uid='users_get_permission_change_data')
 
         # Register viewsets.
         router.register(self.get_model('User').get_collection_string(), UserViewSet)
         router.register(self.get_model('Group').get_collection_string(), GroupViewSet)
 
     def get_startup_elements(self):
-        from ..utils.collection import Collection
+        """
+        Yields all collections required on startup i. e. opening the websocket
+        connection.
+        """
         for model in ('User', 'Group'):
             yield Collection(self.get_model(model).get_collection_string())
diff --git a/openslides/users/signals.py b/openslides/users/signals.py
index 318308764..157473780 100644
--- a/openslides/users/signals.py
+++ b/openslides/users/signals.py
@@ -8,14 +8,13 @@ from .models import Group, User
 
 def get_permission_change_data(sender, permissions=None, **kwargs):
     """
-    Returns all necessary collections if a 'can_see' permission changes.
+    Yields all necessary collections if 'users.can_see' permission changes.
     """
-    user_app = apps.get_app_config(app_label='users')
+    users_app = apps.get_app_config(app_label='users')
     for permission in permissions:
         # There could be only one 'users.can_see' and then we want to return data.
-        if permission.content_type.app_label == user_app.label and permission.codename == 'can_see':
-            return user_app.get_startup_elements()
-    return None
+        if permission.content_type.app_label == users_app.label and permission.codename == 'can_see':
+            yield from users_app.get_startup_elements()
 
 
 def create_builtin_groups_and_admin(**kwargs):
diff --git a/openslides/users/views.py b/openslides/users/views.py
index 240be742b..191d4e413 100644
--- a/openslides/users/views.py
+++ b/openslides/users/views.py
@@ -8,8 +8,8 @@ from django.utils.translation import ugettext as _
 from ..core.config import config
 from ..core.signals import permission_change
 from ..utils.auth import anonymous_is_enabled, has_perm
-from ..utils.autoupdate import send_collections_to_users, inform_changed_data
-from ..utils.collection import CollectionElement
+from ..utils.autoupdate import inform_data_collection_element_list
+from ..utils.collection import CollectionElement, CollectionElementList
 from ..utils.rest_api import (
     ModelViewSet,
     Response,
@@ -161,6 +161,51 @@ class GroupViewSet(ModelViewSet):
             result = False
         return result
 
+    def update(self, request, *args, **kwargs):
+        """
+        Customized endpoint to update a group. Send the signal
+        'permission_change' if group permissions change.
+        """
+        group = self.get_object()
+
+        # Collect old and new (given) permissions to get the difference.
+        old_permissions = list(group.permissions.all())  # Force evaluation so the perms don't change anymore.
+        permission_names = request.data['permissions']
+        if isinstance(permission_names, str):
+            permission_names = [permission_names]
+        given_permissions = [
+            PermissionRelatedField(read_only=True).to_internal_value(data=perm) for perm in permission_names]
+
+        # Run super to update the group.
+        response = super().update(request, *args, **kwargs)
+
+        # Check status code and send 'permission_change' signal.
+        if response.status_code == 200:
+
+            def diff(full, part):
+                """
+                This helper function calculates the difference of two lists:
+                The result is a list of all elements of 'full' that are
+                not in 'part'.
+                """
+                part = set(part)
+                return [item for item in full if item not in part]
+
+            new_permissions = diff(given_permissions, old_permissions)
+
+            # Some permissions are added.
+            if len(new_permissions) > 0:
+                collection_elements = CollectionElementList()
+                signal_results = permission_change.send(None, permissions=new_permissions, action='added')
+                for receiver, signal_collections in signal_results:
+                    for collection in signal_collections:
+                        collection_elements.extend(collection.element_generator())
+                inform_data_collection_element_list(collection_elements)
+
+            # TODO: Some permissions are deleted.
+
+        return response
+
     def destroy(self, request, *args, **kwargs):
         """
         Protects builtin groups 'Default' (pk=1) from being deleted.
@@ -171,42 +216,6 @@ class GroupViewSet(ModelViewSet):
         self.perform_destroy(instance)
         return Response(status=status.HTTP_204_NO_CONTENT)
 
-    def update(self, request, *args, **kwargs):
-        # Collect old and new permissions to get the difference.
-        permission_names = request.data['permissions']
-        if isinstance(permission_names, str):
-            permission_names = [permission_names]
-
-        permissionSerializer = PermissionRelatedField(read_only=True)
-        given_permissions = [
-            permissionSerializer.to_internal_value(data=perm) for perm in permission_names]
-        group = Group.objects.get(pk=int(kwargs['pk']))
-        old_permissions = list(group.permissions.all())  # Force evaluation so the perms doesn't change.
-
-        response = super().update(request, *args, **kwargs)
-
-        def diff(first, second):
-            """
-            This helper function calculates the difference of two lists: first-second.
-            """
-            second = set(second)
-            return [item for item in first if item not in second]
-
-        if response.status_code == 200:
-            new_permissions = diff(given_permissions, old_permissions)
-
-            # some permissions are added
-            if(len(new_permissions) > 0):
-                signal_results = permission_change.send(None, action='added', permissions=new_permissions)
-                collections_to_send = []
-                for (receiver, signal_collections) in signal_results:
-                    if signal_collections:
-                        collections_to_send.extend(signal_collections)
-                #send_collections_to_users(collections_to_send, [user.id for user in group.user_set.all()])
-                # inform_changed_data(what?)
-
-        return response
-
 
 # Special API views
 
diff --git a/openslides/utils/autoupdate.py b/openslides/utils/autoupdate.py
index 55fab43eb..4218fb2ce 100644
--- a/openslides/utils/autoupdate.py
+++ b/openslides/utils/autoupdate.py
@@ -146,6 +146,8 @@ def send_data(message):
     Informs all site users and projector clients about changed data.
     """
     collection_elements = CollectionElementList.from_channels_message(message)
+
+    # Send data to site users.
     for user_id, channel_names in websocket_user_cache.get_all().items():
         if not user_id:
             # Anonymous user
@@ -187,21 +189,6 @@ def send_data(message):
                     {'text': json.dumps(output)})
 
 
-def send_collections_to_users(collections, users_ids):
-    for user_id, channel_names in websocket_user_cache.get_all().items():
-        if user_id in users_ids:
-            try:
-                user = user_to_collection_user(user_id)
-            except ObjectDoesNotExist:
-                # The user does not exist. Skip him/her.
-                continue
-            output = []
-            for collection in collections:
-                output.extend(collection.as_autoupdate_for_user(user))
-            for channel_name in channel_names:
-                send_or_wait(Channel(channel_name).send, {'text': json.dumps(output)})
-
-
 def inform_changed_data(instances, information=None):
     """
     Informs the autoupdate system and the caching system about the creation or
@@ -268,6 +255,18 @@ def inform_deleted_data(*args, information=None):
     transaction.on_commit(lambda: send_autoupdate(collection_elements))
 
 
+def inform_data_collection_element_list(collection_elements, information=None):
+    """
+    Informs the autoupdate system about some collection elements. This is
+    used just to send some data to all users.
+    """
+    # If currently there is an open database transaction, then the
+    # send_autoupdate function is only called, when the transaction is
+    # commited. If there is currently no transaction, then the function
+    # is called immediately.
+    transaction.on_commit(lambda: send_autoupdate(collection_elements))
+
+
 def send_autoupdate(collection_elements):
     """
     Helper function, that sends collection_elements through a channel to the
diff --git a/setup.cfg b/setup.cfg
index 39c01bcdc..8ad6eaacc 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,6 +1,8 @@
 [coverage:run]
 source = openslides
-omit = openslides/core/management/commands/getgeiss.py
+omit =
+    openslides/core/management/commands/*.py
+    openslides/users/management/commands/*.py
 
 [coverage:html]
 directory = personal_data/tmp/htmlcov
diff --git a/tests/integration/users/test_viewset.py b/tests/integration/users/test_viewset.py
index 06a2e94ea..859ab0b1a 100644
--- a/tests/integration/users/test_viewset.py
+++ b/tests/integration/users/test_viewset.py
@@ -430,6 +430,53 @@ class GroupUpdate(TestCase):
         self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertEqual(response.data, {'name': ['This field is required.']})
 
+    def test_update_via_put_with_new_permissions(self):
+        admin_client = APIClient()
+        admin_client.login(username='admin', password='admin')
+        group = Group.objects.create(name='group_name_inooThe3dii4mahWeeSe')
+        # This contains all permissions.
+        permissions = [
+            'agenda.can_be_speaker',
+            'agenda.can_manage',
+            'agenda.can_see',
+            'agenda.can_see_hidden_items',
+            'assignments.can_manage',
+            'assignments.can_nominate_other',
+            'assignments.can_nominate_self',
+            'assignments.can_see',
+            'core.can_manage_config',
+            'core.can_manage_projector',
+            'core.can_manage_tags',
+            'core.can_manage_chat',
+            'core.can_see_frontpage',
+            'core.can_see_projector',
+            'core.can_use_chat',
+            'mediafiles.can_manage',
+            'mediafiles.can_see',
+            'mediafiles.can_see_hidden',
+            'mediafiles.can_upload',
+            'motions.can_create',
+            'motions.can_manage',
+            'motions.can_see',
+            'motions.can_see_and_manage_comments',
+            'motions.can_support',
+            'users.can_manage',
+            'users.can_see_extra_data',
+            'users.can_see_name',
+        ]
+
+        response = admin_client.put(
+            reverse('group-detail', args=[group.pk]),
+            {'name': 'new_group_name_Chie6duwaepoo8aech7r',
+             'permissions': permissions},
+            format='json')
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        group = Group.objects.get(pk=group.pk)
+        for permission in permissions:
+            app_label, codename = permission.split('.')
+            self.assertTrue(group.permissions.get(content_type__app_label=app_label, codename=codename))
+
 
 class GroupDelete(TestCase):
     """