From 28b54cf75adfdbcc809a39a66059be5e948fa725 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Wed, 10 Apr 2013 22:39:25 -0300 Subject: [PATCH] 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 | 16 ++++++- ckan/tests/logic/test_member.py | 79 +++++++++++++++++++++------------ 2 files changed, 64 insertions(+), 31 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 6dd883bd07a..8c937a36b48 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -423,6 +423,18 @@ 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) @@ -430,12 +442,12 @@ def member_create(context, data_dict=None): # 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') diff --git a/ckan/tests/logic/test_member.py b/ckan/tests/logic/test_member.py index 43f8f2ca26f..69e405159d7 100644 --- a/ckan/tests/logic/test_member.py +++ b/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,