Change member_create and member_delete to accept object's id or name, and throw better errors

ckan/logic/__init__.py

@@ -439,6 +439,17 @@ def get_converter(converter):
         raise UnknownConverter('Converter `%s` does not exist' % converter)
 
 
+def model_name_to_class(model_module, model_name):
+    '''Return the class in model_module that has the same name as the received string.
+
+    Raises AttributeError if there's no model in model_module named model_name.
+    '''
+    try:
+        model_class_name = model_name.title()
+        return getattr(model_module, model_class_name)
+    except AttributeError:
+        raise ValidationError("%s isn't a valid model" % model_class_name)
+
 def _import_module_functions(module_path):
     '''Import a module and get the functions and return them in a dict'''
     functions_dict = {}

ckan/logic/action/create.py

@@ -417,28 +417,35 @@ def member_create(context, data_dict=None):
     if 'message' in context:
         rev.message = context['message']
     else:
-        rev.message = _(u'REST API: Create member object %s') % data_dict.get("name", "")
+        rev.message = _(u'REST API: Create member object %s') % data_dict.get('name', '')
 
-    group = model.Group.get(data_dict.get('id', ''))
-    obj_id, obj_type, capacity = _get_or_bust(data_dict, ['object', 'object_type', 'capacity'])
+    group_id, obj_id, obj_type, capacity = _get_or_bust(data_dict, ['id', 'object', 'object_type', 'capacity'])
+
+    group = model.Group.get(group_id)
+    if not group:
+        raise NotFound('Group was not found.')
+
+    obj_class = ckan.logic.model_name_to_class(model, obj_type)
+    obj = obj_class.get(obj_id)
+    if not obj:
+        raise NotFound('%s was not found.' % obj_type.title())
 
     # User must be able to update the group to add a member to it
     _check_access('group_update', context, data_dict)
 
     # Look up existing, in case it exists
     member = model.Session.query(model.Member).\
             filter(model.Member.table_name == obj_type).\
-            filter(model.Member.table_id == obj_id).\
+            filter(model.Member.table_id == obj.id).\
             filter(model.Member.group_id == group.id).\
-            filter(model.Member.state == "active").first()
-    if member:
-        member.capacity = capacity
-    else:
+            filter(model.Member.state == 'active').first()
+    if not member:
         member = model.Member(table_name = obj_type,
-                              table_id = obj_id,
+                              table_id = obj.id,
                               group_id = group.id,
-                              state = 'active',
-                              capacity=capacity)
+                              state = 'active')
+
+    member.capacity = capacity
 
     model.Session.add(member)
     model.repo.commit()

ckan/logic/action/delete.py

@@ -166,7 +166,7 @@ def member_delete(context, data_dict=None):
 
     :param id: the id of the group
     :type id: string
-    :param object: the id of the object to be removed
+    :param object: the id or name of the object to be removed
     :type object: string
     :param object_type: the type of the object to be removed, e.g. ``package``
         or ``user``
@@ -175,17 +175,25 @@ def member_delete(context, data_dict=None):
     '''
     model = context['model']
 
-    group = model.Group.get(_get_or_bust(data_dict, 'id'))
-    obj_id, obj_type = _get_or_bust(data_dict, ['object', 'object_type'])
+    group_id, obj_id, obj_type = _get_or_bust(data_dict, ['id', 'object', 'object_type'])
+
+    group = model.Group.get(group_id)
+    if not group:
+        raise NotFound('Group was not found.')
+
+    obj_class = ckan.logic.model_name_to_class(model, obj_type)
+    obj = obj_class.get(obj_id)
+    if not obj:
+        raise NotFound('%s was not found.' % obj_type.title())
 
     # User must be able to update the group to remove a member from it
     _check_access('group_update', context, data_dict)
 
     member = model.Session.query(model.Member).\
             filter(model.Member.table_name == obj_type).\
-            filter(model.Member.table_id == obj_id).\
+            filter(model.Member.table_id == obj.id).\
             filter(model.Member.group_id == group.id).\
-            filter(model.Member.state    == "active").first()
+            filter(model.Member.state    == 'active').first()
     if member:
         rev = model.repo.new_revision()
         rev.author = context.get('user')

ckan/tests/logic/test_init.py

@@ -0,0 +1,10 @@
+import nose.tools as tools
+
+import ckan.model as model
+import ckan.logic as logic
+
+
+class TestMemberLogic(object):
+    def test_model_name_to_class(self):
+        assert logic.model_name_to_class(model, 'package') == model.Package
+        tools.assert_raises(logic.ValidationError, logic.model_name_to_class, model, 'inexistent_model_name')

ckan/tests/logic/test_member.py

@@ -8,66 +8,166 @@ class TestMemberLogic(object):
 
     @classmethod
     def setup_class(cls):
-        cls.username = 'testsysadmin'
-        cls.groupname = 'david'
-
         model.repo.new_revision()
         create_test_data.CreateTestData.create()
+        cls.user = model.User.get('testsysadmin')
+        cls.group = model.Group.get('david')
         cls.pkgs = [model.Package.by_name('warandpeace'),
                     model.Package.by_name('annakarenina')]
 
     @classmethod
     def teardown_class(cls):
         model.repo.rebuild_db()
 
-    def _build_context(self, obj, obj_type, capacity='public'):
-        ctx = {'model': model,
-               'session': model.Session,
-               'user': self.username}
-        dd = {'id': self.groupname,
-              'object': obj,
-              'object_type': obj_type,
-              'capacity': capacity}
-        return ctx, dd
+    def test_member_create(self):
+        self._member_create(self.pkgs[0].id, 'package', 'public')
+        res = self._member_list()
+        assert (self.pkgs[0].id, 'package', 'public') in res, res
 
-    def _add_member(self, obj, obj_type, capacity):
-        ctx, dd = self._build_context(obj, obj_type, capacity)
-        return logic.get_action('member_create')(ctx, dd)
+    def test_member_create_should_update_member_if_it_already_exists(self):
+        initial = self._member_create(self.pkgs[0].id, 'package', 'public')
+        final = self._member_create(self.pkgs[0].id, 'package', 'private')
+        assert initial['id'] == final['id'], [initial, final]
+        assert initial['capacity'] == u'public'
+        assert final['capacity'] == u'private'
+
+    def test_member_create_raises_if_user_is_unauthorized_to_update_group(self):
+        ctx, dd = self._build_context(self.pkgs[0].id, 'package',
+                                      user='unauthorized_user')
+        assert_raises(logic.NotAuthorized,
+                      logic.get_action('member_create'), ctx, dd)
+
+    def test_member_create_accepts_group_name_or_id(self):
+        by_name = self._member_create_in_group(self.pkgs[0].id, 'package',
+                                               'public', self.group.name)
+        by_id = self._member_create_in_group(self.pkgs[0].id, 'package',
+                                             'public', self.group.id)
+        assert by_name['id'] == by_id['id']
 
-    def test_member_add(self):
-        res = self._add_member(self.pkgs[0].id, 'package', 'public')
-        assert 'capacity' in res and res['capacity'] == u'public'
+    def test_member_create_accepts_object_name_or_id(self):
+        test_cases = ((self.pkgs[0], 'package', 'public'),
+                      (self.user, 'user', 'admin'))
+        for case in test_cases:
+            obj = case[0]
+            by_name = self._member_create(obj.name, case[1], case[2])
+            by_id = self._member_create(obj.id, case[1], case[2])
+            assert by_name['id'] == by_id['id']
+
+    def test_member_create_raises_if_any_required_parameter_isnt_defined(self):
+        ctx, dd = self._build_context(self.pkgs[0].id, 'package')
+        for key in dd.keys():
+            new_dd = dd.copy()
+            del new_dd[key]
+            assert_raises(logic.ValidationError,
+                          logic.get_action('member_create'), ctx, new_dd)
+
+    def test_member_create_raises_if_group_wasnt_found(self):
+        assert_raises(logic.NotFound,
+                      self._member_create_in_group,
+                      self.pkgs[0].id, 'package', 'public', 'inexistent_group')
+
+    def test_member_create_raises_if_object_wasnt_found(self):
+        assert_raises(logic.NotFound,
+                      self._member_create,
+                      'inexistent_package', 'package', 'public')
+
+    def test_member_create_raises_if_object_type_is_invalid(self):
+        assert_raises(logic.ValidationError,
+                      self._member_create,
+                      'obj_id', 'invalid_obj_type', 'public')
 
     def test_member_list(self):
-        self._add_member(self.pkgs[0].id, 'package', 'public')
-        self._add_member(self.pkgs[1].id, 'package', 'public')
-        ctx, dd = self._build_context('', 'package')
-        res = logic.get_action('member_list')(ctx, dd)
-        assert len(res) == 2, res
+        self._member_create(self.pkgs[0].id, 'package', 'public')
+        self._member_create(self.pkgs[1].id, 'package', 'public')
+        res = self._member_list('package')
         assert (self.pkgs[0].id, 'package', 'public') in res
         assert (self.pkgs[1].id, 'package', 'public') in res
 
-        ctx, dd = self._build_context('', 'user', 'admin')
-        res = logic.get_action('member_list')(ctx, dd)
+        res = self._member_list('user', 'admin')
         assert len(res) == 0, res
 
-        ctx, dd = self._build_context('', 'user', 'admin')
-        dd['id'] = u'foo'
-        assert_raises(logic.NotFound, logic.get_action('member_list'), ctx, dd)
+        assert_raises(logic.NotFound,
+                      self._member_list, 'user', 'admin', 'inexistent_group')
 
-        self._add_member(self.username, 'user', 'admin')
-        ctx, dd = self._build_context('', 'user', 'admin')
-        res = logic.get_action('member_list')(ctx, dd)
-        assert len(res) == 1, res
-        assert (self.username, 'user', 'Admin') in res
+        self._member_create(self.user.id, 'user', 'admin')
+        res = self._member_list('user', 'admin')
+        assert (self.user.id, 'user', 'Admin') in res, res
 
-    def test_member_delete(self):
-        self._add_member(self.username, 'user', 'admin')
-        ctx, dd = self._build_context(self.username, 'user', 'admin')
-        res = logic.get_action('member_list')(ctx, dd)
-        assert len(res) == 1, res
+    def test_member_create_accepts_group_name_or_id(self):
+        for group_key in [self.group.id, self.group.name]:
+            self._member_create(self.user.id, 'user', 'admin')
 
-        logic.get_action('member_delete')(ctx, dd)
+            self._member_delete_in_group(self.user.id, 'user', group_key)
 
-        res = logic.get_action('member_list')(ctx, dd)
-        assert len(res) == 0, res
+            res = self._member_list('user', 'admin')
+            assert (self.user.id, 'user', 'Admin') not in res, res
+
+    def test_member_delete_accepts_object_name_or_id(self):
+        for key in [self.user.id, self.user.name]:
+            self._member_create(key, 'user', 'admin')
+
+            self._member_delete(key, 'user')
+
+            res = self._member_list('user', 'admin')
+            assert (self.user.id, 'user', 'Admin') not in res, res
+
+    def test_member_delete_raises_if_user_is_unauthorized_to_update_group(self):
+        ctx, dd = self._build_context(self.pkgs[0].id, 'package',
+                                      user='unauthorized_user')
+        assert_raises(logic.NotAuthorized,
+                      logic.get_action('member_delete'), ctx, dd)
+
+    def test_member_delete_raises_if_any_required_parameter_isnt_defined(self):
+        ctx, dd = self._build_context(self.pkgs[0].id, 'package')
+        for key in ['id', 'object', 'object_type']:
+            new_dd = dd.copy()
+            del new_dd[key]
+            assert_raises(logic.ValidationError,
+                          logic.get_action('member_delete'), ctx, new_dd)
+
+    def test_member_delete_raises_if_group_wasnt_found(self):
+        assert_raises(logic.NotFound,
+                      self._member_delete_in_group,
+                      self.pkgs[0].id, 'package', 'inexistent_group')
+
+    def test_member_delete_raises_if_object_wasnt_found(self):
+        assert_raises(logic.NotFound,
+                      self._member_delete, 'unexistent_package', 'package')
+
+    def test_member_delete_raises_if_object_type_is_invalid(self):
+        assert_raises(logic.ValidationError,
+                      self._member_delete, 'obj_id', 'invalid_obj_type')
+
+    def _member_create(self, obj, obj_type, capacity):
+        ctx, dd = self._build_context(obj, obj_type, capacity)
+        return logic.get_action('member_create')(ctx, dd)
+
+    def _member_create_in_group(self, obj, obj_type, capacity, group_id):
+        ctx, dd = self._build_context(obj, obj_type, capacity, group_id)
+        return logic.get_action('member_create')(ctx, dd)
+
+    def _member_create_as_user(self, obj, obj_type, capacity, user):
+        ctx, dd = self._build_context(obj, obj_type, capacity, user=user)
+        return logic.get_action('member_create')(ctx, dd)
+
+    def _member_list(self, obj_type=None, capacity=None, group_id=None):
+        ctx, dd = self._build_context(None, obj_type, capacity, group_id)
+        return logic.get_action('member_list')(ctx, dd)
+
+    def _member_delete(self, obj, obj_type):
+        ctx, dd = self._build_context(obj, obj_type)
+        return logic.get_action('member_delete')(ctx, dd)
+
+    def _member_delete_in_group(self, obj, obj_type, group_id):
+        ctx, dd = self._build_context(obj, obj_type, group_id=group_id)
+        return logic.get_action('member_delete')(ctx, dd)
+
+    def _build_context(self, obj, obj_type, capacity='public', group_id=None, user=None):
+        ctx = {'model': model,
+               'session': model.Session,
+               'user': user or self.user.id}
+        dd = {'id': group_id or self.group.name,
+              'object': obj,
+              'object_type': obj_type,
+              'capacity': capacity}
+        return ctx, dd