[#1038] Setting of parent group in the form is now done via group['gr…

…oups'] rather than custom 'parent'

Removed explicit creation of Members in the create/update logic functions - these were a hangover. Instead, the form just needs to ensure the parent group gets added to the group['groups'] and it will get set in the model_save as normal.

Group.get_group no longer used anywhere (and the caching looked ropey anyway) now that the previous bit is removed.

Added schema validator to ensure that no loops form in the hierarchy.

Downwards CTE needed ordering as found it also couldn't be relied upon to be in order it recursed, probably due to the join.

Removed duplicate 'groups' key in the group schema.

ckan/controllers/organization.py

@@ -26,8 +26,10 @@ def _db_to_form_schema(self, group_type=None):
         into a format suitable for the form (optional)'''
         pass
 
-    def _setup_template_variables(self, context, data_dict, group_type=None):
-        pass
+    # This is commented so that the Group controller method runs instead,
+    # allowing a group plugins to setup template variables.
+    #def _setup_template_variables(self, context, data_dict, group_type=None):
+    #    pass
 
     def _new_template(self, group_type):
         return 'organization/new.html'

ckan/lib/dictization/model_save.py

@@ -342,7 +342,11 @@ def group_member_save(context, group_dict, member_table_name):
     entities = {}
     Member = model.Member
 
-    ModelClass = getattr(model, member_table_name[:-1].capitalize())
+    classname = member_table_name[:-1].capitalize()
+    if classname == 'Organization':
+        # Organizations use the model.Group class
+        classname = 'Group'
+    ModelClass = getattr(model, classname)
 
     for entity_dict in entity_list:
         name_or_id = entity_dict.get('id') or entity_dict.get('name')

ckan/logic/action/create.py

@@ -15,6 +15,7 @@
 import ckan.lib.dictization.model_dictize as model_dictize
 import ckan.lib.dictization.model_save as model_save
 import ckan.lib.navl.dictization_functions
+import ckan.lib.navl.validators as validators
 
 from ckan.common import _
 
@@ -472,7 +473,6 @@ def _group_or_org_create(context, data_dict, is_org=False):
     model = context['model']
     user = context['user']
     session = context['session']
-    parent = context.get('parent', None)
     data_dict['is_organization'] = is_org
 
 
@@ -512,14 +512,6 @@ def _group_or_org_create(context, data_dict, is_org=False):
 
     group = model_save.group_dict_save(data, context)
 
-    if parent:
-        parent_group = model.Group.get( parent )
-        if parent_group:
-            member = model.Member(group=parent_group, table_id=group.id, table_name='group')
-            session.add(member)
-            log.debug('Group %s is made child of group %s',
-                      group.name, parent_group.name)
-
     if user:
         admins = [model.User.by_name(user.decode('utf8'))]
     else:

ckan/logic/action/update.py

@@ -408,7 +408,6 @@ def _group_or_org_update(context, data_dict, is_org=False):
     user = context['user']
     session = context['session']
     id = _get_or_bust(data_dict, 'id')
-    parent = context.get('parent', None)
 
     group = model.Group.get(id)
     context["group"] = group
@@ -464,23 +463,6 @@ def _group_or_org_update(context, data_dict, is_org=False):
         context['prevent_packages_update'] = True
     group = model_save.group_dict_save(data, context)
 
-    if parent:
-        parent_group = model.Group.get( parent )
-        if parent_group and not parent_group in group.get_groups(group.type):
-            # Delete all of this groups memberships
-            current = session.query(model.Member).\
-               filter(model.Member.table_id == group.id).\
-               filter(model.Member.table_name == "group").all()
-            if current:
-                log.debug('Parents of group %s deleted: %r', group.name,
-                          [membership.group.name for membership in current])
-            for c in current:
-                session.delete(c)
-            member = model.Member(group=parent_group, table_id=group.id, table_name='group')
-            session.add(member)
-            log.debug('Group %s is made child of group %s',
-                      group.name, parent_group.name)
-
     if is_org:
         plugin_type = plugins.IOrganizationController
     else:

ckan/logic/schema.py

@@ -50,6 +50,7 @@
                                    url_validator,
                                    datasets_with_no_organization_cannot_be_private,
                                    list_of_strings,
+                                   no_loops_in_hierarchy,
                                    )
 from ckan.logic.converters import (convert_user_name_or_id_to_id,
                                    convert_package_name_or_id_to_id,
@@ -274,19 +275,14 @@ def default_group_schema():
             "title":[ignore_missing, unicode],
             "name":[ignore_missing, unicode],
             "__extras": [ignore]
-        },
-         'groups': {
-            "name": [not_empty, unicode],
-            "capacity": [ignore_missing],
-            "__extras": [ignore]
         },
         'users': {
             "name": [not_empty, unicode],
             "capacity": [ignore_missing],
             "__extras": [ignore]
         },
         'groups': {
-            "name": [not_empty, unicode],
+            "name": [not_empty, no_loops_in_hierarchy, unicode],
             "capacity": [ignore_missing],
             "__extras": [ignore]
         }

ckan/logic/validators.py

@@ -649,3 +649,22 @@ def list_of_strings(key, data, errors, context):
         if not isinstance(x, basestring):
             raise Invalid('%s: %s' % (_('Not a string'), x))
 
+def no_loops_in_hierarchy(key, data, errors, context):
+    '''Checks that the parent groups specified in the data would not cause
+    a loop in the group hierarchy, and therefore cause the recursion up/down
+    the hierarchy to get into an infinite loop.
+    '''
+    if not 'id' in data:
+        # Must be a new group - has no children, so no chance of loops
+        return
+    group = context['model'].Group.get(data['id'])
+    allowable_parents = group.\
+                        groups_allowed_to_be_its_parent(type=group.type)
+    for parent in data['groups']:
+        parent_name = parent['name']
+        # a blank name signifies top level, which is always allowed
+        if parent_name and context['model'].Group.get(parent_name) \
+                not in allowable_parents:
+            raise Invalid(_('This parent would create a loop in the '
+                            'hierarchy'))
+

ckan/model/group.py

@@ -193,11 +193,11 @@ def get_children_groups(self, type='group'):
                 for id_, name, title in results]
 
     def get_children_group_hierarchy(self, type='group'):
-        '''Returns the groups in all levels underneath this group in the 
+        '''Returns the groups in all levels underneath this group in the
         hierarchy. The ordering is such that children always come after their
         parent.
 
-        :rtype: a list of tuples, each one a Group and the ID its their parent
+        :rtype: a list of tuples, each one a Group and the ID of its parent
         group.
 
         e.g. >>> dept-health.get_children_group_hierarchy()
@@ -211,7 +211,7 @@ def get_children_group_hierarchy(self, type='group'):
         return results
 
     def get_parent_group_hierarchy(self, type='group'):
-        '''Returns this group's parent, parent's parent, parent's parent's 
+        '''Returns this group's parent, parent's parent, parent's parent's
         parent etc.. Sorted with the top level parent first.'''
         return meta.Session.query(Group).\
             from_statement(HIERARCHY_UPWARDS_CTE).\
@@ -231,6 +231,22 @@ def get_top_level_groups(cls, type='group'):
             filter(Group.type == type).\
             order_by(Group.title).all()
 
+    def groups_allowed_to_be_its_parent(self, type='group'):
+        '''Returns a list of the groups (of the specified type) which are
+        allowed to be this group's parent. It excludes ones which would
+        create a loop in the hierarchy, causing the recursive CTE to
+        be in an infinite loop.
+
+        :returns: A list of group objects ordered by group title
+
+        '''
+        all_groups = self.all(group_type=type)
+        excluded_groups = set(group.name for group, id_ in
+                              self.get_children_group_hierarchy(type=type))
+        excluded_groups.add(self.name)
+        return [group for group in all_groups
+                if group.name not in excluded_groups]
+
     def packages(self, with_private=False, limit=None,
             return_query=False, context=None):
         '''Return this group's active and pending packages.
@@ -319,28 +335,6 @@ def add_package_by_name(self, package_name):
                             table_name='package')
             meta.Session.add(member)
 
-    def get_groups(self, group_type=None, capacity=None):
-        """ Get all groups that this group is within (i.e. this group's child
-        groups)
-
-        """
-        import ckan.model as model
-        # DR: Why is this cached? Surely the members can change in the
-        #     lifetime of this Group?
-        if '_groups' not in self.__dict__:
-            self._groups = meta.Session.query(model.Group).\
-                join(model.Member, model.Member.group_id == model.Group.id and
-                     model.Member.table_name == 'group').\
-                filter(model.Member.state == 'active').\
-                filter(model.Member.table_id == self.id).all()
-
-        groups = self._groups
-        if group_type:
-            groups = [g for g in groups if g.type == group_type]
-        if capacity:
-            groups = [g for g in groups if g.capacity == capacity]
-        return groups
-
     @property
     def all_related_revisions(self):
         '''Returns chronological list of all object revisions related to
@@ -368,7 +362,6 @@ def all_related_revisions(self):
     def __repr__(self):
         return '<Group %s>' % self.name
 
-
 meta.mapper(Group, group_table,
             extension=[vdm.sqlalchemy.Revisioner(group_revision_table), ], )
 
@@ -393,20 +386,21 @@ def __repr__(self):
 MemberRevision.related_packages = lambda self: [self.continuity.package]
 
 
-HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child AS
+HIERARCHY_DOWNWARDS_CTE = """WITH RECURSIVE child(depth) AS
 (
     -- non-recursive term
-    SELECT * FROM member
+    SELECT 0, * FROM member
     WHERE table_id = :id AND table_name = 'group' AND state = 'active'
     UNION ALL
     -- recursive term
-    SELECT m.* FROM member AS m, child AS c
+    SELECT c.depth + 1, m.* FROM member AS m, child AS c
     WHERE m.table_id = c.group_id AND m.table_name = 'group'
           AND m.state = 'active'
 )
-SELECT G.*, child.table_id as parent_id FROM child
+SELECT G.*, child.depth, child.table_id as parent_id FROM child
     INNER JOIN public.group G ON G.id = child.group_id
-    WHERE G.type = :type AND G.state='active';"""
+    WHERE G.type = :type AND G.state='active'
+    ORDER BY child.depth ASC;"""
 
 HIERARCHY_UPWARDS_CTE = """WITH RECURSIVE parenttree(depth) AS (
     -- non-recursive term

ckan/tests/models/test_group.py

@@ -165,6 +165,15 @@ def test_get_top_level_groups(self):
                                        get_top_level_groups(type=group_type)),
                      ['cabinet-office', 'department-of-health'])
 
+    def test_groups_allowed_to_be_its_parent(self):
+        groups = model.Group.by_name(u'national-health-service').\
+            groups_allowed_to_be_its_parent(type=group_type)
+        names = names_from_groups(groups)
+        assert_in('department-of-health', names)
+        assert_in('cabinet-office', names)
+        assert_not_in('natonal-health-service', names)
+        assert_not_in('nhs-wirral-ccg', names)
+
 class TestGroupRevisions:
     @classmethod
     def setup_class(self):