member_create accepts either the object's id or name, and have extra …

…validations.

Even though its documentation said it accepted the object's id or name, this
wasn't the case. This is fixed now.

I've also added validations to check that the group, the object, and
object_type exists and are valid.

Now we always save the object's id in our members table, even if someone
calls member_create passing its name as obj_id.

ckan/logic/action/create.py

@@ -423,19 +423,31 @@ def member_create(context, data_dict=None):
     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.'))
+
+    try:
+        obj_class_name = obj_type.title()
+        obj_class = getattr(model, obj_class_name)
+    except AttributeError:
+        raise ValidationError(_("%s isn't a valid object_type" % obj_type))
+
+    obj = obj_class.get(obj_id)
+    if not obj:
+        raise NotFound(_('%s was not found.' % obj_class_name))
 
     # 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 not member:
         member = model.Member(table_name = obj_type,
-                              table_id = obj_id,
+                              table_id = obj.id,
                               group_id = group.id,
                               state = 'active')
 

ckan/tests/logic/test_member.py

@@ -8,11 +8,11 @@ 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.pkgs = [model.Package.by_name('warandpeace'),
                     model.Package.by_name('annakarenina')]
 
@@ -21,8 +21,9 @@ def teardown_class(cls):
         model.repo.rebuild_db()
 
     def test_member_create(self):
-        res = self._member_create(self.pkgs[0].id, 'package', 'public')
-        assert 'capacity' in res and res['capacity'] == u'public'
+        self._member_create(self.pkgs[0].id, 'package', 'public')
+        res = self._member_list()
+        assert (self.pkgs[0].id, 'package', 'public') in res, res
 
     def test_member_create_should_update_member_if_it_already_exists(self):
         initial = self._member_create(self.pkgs[0].id, 'package', 'public')
@@ -31,54 +32,66 @@ def test_member_create_should_update_member_if_it_already_exists(self):
         assert initial['capacity'] == u'public'
         assert final['capacity'] == u'private'
 
-    def test_member_create_validates_if_user_is_authorized_to_update_group(self):
-        ctx, dd = self._build_context(self.pkgs[0].id, 'package', user='unauthorized-user')
+    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):
         group = model.Group.get(self.groupname)
-        by_id = self._member_create_in_group(self.pkgs[0].id, 'package', 'public', group.name)
-        by_name = self._member_create_in_group(self.pkgs[0].id, 'package', 'public', group.id)
-        assert by_id['id'] == by_name['id']
-
-    def test_member_create_requires_all_parameters_to_be_defined(self):
+        by_name = self._member_create_in_group(self.pkgs[0].id, 'package', 'public', group.name)
+        by_id = self._member_create_in_group(self.pkgs[0].id, 'package', 'public', group.id)
+        assert by_name['id'] == by_id['id']
+
+    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_some_of_the_parameters_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._member_create(self.pkgs[0].id, 'package', 'public')
         self._member_create(self.pkgs[1].id, 'package', 'public')
-        ctx, dd = self._build_context('', 'package')
-        res = logic.get_action('member_list')(ctx, dd)
+        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._member_create(self.username, 'user', 'admin')
-        ctx, dd = self._build_context('', 'user', 'admin')
-        res = logic.get_action('member_list')(ctx, dd)
-        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._member_create(self.username, 'user', 'admin')
-        ctx, dd = self._build_context(self.username, 'user', 'admin')
-        res = logic.get_action('member_list')(ctx, dd)
-        assert (self.username, 'user', 'Admin') in res, res
+        self._member_create(self.user.id, 'user', 'admin')
+        res = self._member_list('user', 'admin')
+        assert (self.user.id, 'user', 'Admin') in res, res
 
-        logic.get_action('member_delete')(ctx, dd)
+        self._member_delete(self.user.id, 'user')
 
-        res = logic.get_action('member_list')(ctx, dd)
-        assert (self.username, 'user', 'Admin') not in res, res
+        res = self._member_list('user', 'admin')
+        assert (self.user.id, 'user', 'Admin') not in res, res
 
     def _member_create(self, obj, obj_type, capacity):
         ctx, dd = self._build_context(obj, obj_type, capacity)
@@ -92,10 +105,18 @@ 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 _build_context(self, obj, obj_type, capacity='public', group_id=None, user=None):
         ctx = {'model': model,
                'session': model.Session,
-               'user': user or self.username}
+               'user': user or self.user.id}
         dd = {'id': group_id or self.groupname,
               'object': obj,
               'object_type': obj_type,