Merge branch '1505-invite-to-organization-causes-error'

ckan/logic/auth/create.py

@@ -121,8 +121,8 @@ def user_create(context, data_dict=None):
             'create users')}
     return {'success': True}
 
-def user_invite(context, data_dict=None):
-    context['id'] = context.get('group_id')
+def user_invite(context, data_dict):
+    data_dict['id'] = data_dict['group_id']
     return group_member_create(context, data_dict)
 
 def _check_group_auth(context, data_dict):

ckan/new_tests/factories.py

@@ -152,6 +152,9 @@ class Group(factory.Factory):
     title = factory.LazyAttribute(_generate_group_title)
     description = 'A test description for this test group.'
 
+    user = factory.LazyAttribute(lambda _:
+                                 helpers.call_action('get_site_user'))
+
     @classmethod
     def _build(cls, target_class, *args, **kwargs):
         raise NotImplementedError(".build() isn't supported in CKAN")
@@ -160,12 +163,13 @@ def _build(cls, target_class, *args, **kwargs):
     def _create(cls, target_class, *args, **kwargs):
         if args:
             assert False, "Positional args aren't supported, use keyword args."
-        assert 'user' in kwargs, ('The Group factory requires an extra '
-                                  'user=user_dict keyword argument (the user '
-                                  'who will create the group)')
-        user_dict = kwargs.pop('user')
-        context = {'user': user_dict['name']}
-        group_dict = helpers.call_action('group_create', context=context,
+
+        context = {
+            'user': kwargs.pop('user')['name']
+        }
+
+        group_dict = helpers.call_action('group_create',
+                                         context=context,
                                          **kwargs)
         return group_dict
 

ckan/new_tests/helpers.py

@@ -114,13 +114,9 @@ def call_auth(auth_name, context, **kwargs):
     :rtype: dict
 
     '''
-    import ckan.logic.auth.update
-
     assert 'user' in context, ('Test methods must put a user name in the '
                                'context dict')
     assert 'model' in context, ('Test methods must put a model in the '
                                 'context dict')
 
-    # FIXME: Do we want to go through check_access() here?
-    auth_function = ckan.logic.auth.update.__getattribute__(auth_name)
-    return auth_function(context=context, data_dict=kwargs)
+    return logic.check_access(auth_name, context, data_dict=kwargs)

ckan/new_tests/logic/action/test_create.py

@@ -0,0 +1,89 @@
+'''Unit tests for ckan/logic/auth/create.py.
+
+'''
+
+import mock
+import nose.tools
+
+import ckan.new_tests.helpers as helpers
+import ckan.new_tests.factories as factories
+import ckan.model
+import ckan.logic
+
+
+class TestUserInvite(object):
+
+    def setup(self):
+        helpers.reset_db()
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    def test_user_invite(self, _):
+        invited_user = self._invite_user_to_group()
+
+        assert invited_user is not None
+        assert invited_user.is_pending()
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    def test_user_invite_creates_user_with_valid_username(self, _):
+        email = 'user$%+abc@email.com'
+        invited_user = self._invite_user_to_group(email)
+
+        assert invited_user.name.startswith('user---abc'), invited_user
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    def test_user_invite_assigns_user_to_group_in_expected_role(self, _):
+        role = 'admin'
+        invited_user = self._invite_user_to_group(role=role)
+
+        group_ids = invited_user.get_group_ids(capacity=role)
+        assert len(group_ids) == 1, group_ids
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    def test_user_invite_sends_invite(self, send_invite):
+        invited_user = self._invite_user_to_group()
+
+        assert send_invite.called
+        assert send_invite.call_args[0][0].id == invited_user.id
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    @mock.patch('random.SystemRandom')
+    def test_user_invite_works_even_if_username_already_exists(self, rand, _):
+        rand.return_value.random.side_effect = [1000, 1000, 1000, 2000,
+                                                3000, 4000, 5000]
+
+        for _ in range(3):
+            invited_user = self._invite_user_to_group(email='same@email.com')
+            assert invited_user is not None, invited_user
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    @nose.tools.raises(ckan.logic.ValidationError)
+    def test_user_invite_requires_email(self, _):
+        self._invite_user_to_group(email=None)
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    @nose.tools.raises(ckan.logic.ValidationError)
+    def test_user_invite_requires_role(self, _):
+        self._invite_user_to_group(role=None)
+
+    @mock.patch('ckan.lib.mailer.send_invite')
+    @nose.tools.raises(ckan.logic.ValidationError)
+    def test_user_invite_requires_group_id(self, _):
+        self._invite_user_to_group(group={'id': None})
+
+    def _invite_user_to_group(self, email='user@email.com',
+                              group=None, role='member'):
+        user = factories.User()
+        group = group or factories.Group(user=user)
+
+        context = {
+            'user': user['name']
+        }
+        params = {
+            'email': email,
+            'group_id': group['id'],
+            'role': role
+        }
+
+        result = helpers.call_action('user_invite', context, **params)
+
+        return ckan.model.User.get(result['id'])

ckan/new_tests/logic/auth/test_create.py

@@ -0,0 +1,35 @@
+'''Unit tests for ckan/logic/auth/create.py.
+
+'''
+
+import mock
+import nose
+
+import ckan.new_tests.helpers as helpers
+import ckan.new_tests.factories as factories
+
+logic = helpers.logic
+
+
+class TestCreate(object):
+
+    def setup(self):
+        helpers.reset_db()
+
+    @mock.patch('ckan.logic.auth.create.group_member_create')
+    def test_user_invite_delegates_correctly_to_group_member_create(self, gmc):
+        user = factories.User()
+        context = {
+            'user': user['name'],
+            'model': None,
+            'auth_user_obj': user
+        }
+        data_dict = {'group_id': 42}
+
+        gmc.return_value = {'success': False}
+        nose.tools.assert_raises(logic.NotAuthorized, helpers.call_auth,
+                                 'user_invite', context=context, **data_dict)
+
+        gmc.return_value = {'success': True}
+        result = helpers.call_auth('user_invite', context=context, **data_dict)
+        assert result is True

ckan/new_tests/logic/auth/test_update.py

@@ -2,9 +2,11 @@
 
 '''
 import mock
+import nose
 
 import ckan.new_tests.helpers as helpers
 import ckan.new_tests.factories as factories
+import ckan.logic as logic
 
 
 class TestUpdate(object):
@@ -32,13 +34,9 @@ def test_user_update_visitor_cannot_update_user(self):
             'id': fred.id,
             'name': 'updated_user_name',
         }
-        result = helpers.call_auth('user_update', context=context, **params)
 
-        assert result['success'] is False
-        # FIXME: This is a terrible error message, containing both 127.0.0.1
-        # and Fred's user id (not his name).
-        assert result['msg'] == ('User 127.0.0.1 not authorized to edit user '
-                                 'fred_user_id')
+        nose.tools.assert_raises(logic.NotAuthorized, helpers.call_auth,
+                                 'user_update', context=context, **params)
 
     ## START-AFTER
 
@@ -69,14 +67,11 @@ def test_user_update_user_cannot_update_another_user(self):
             'id': fred.id,
             'name': 'updated_user_name',
         }
-        result = helpers.call_auth('user_update', context=context, **params)
 
         # 3. Make assertions about the return value and/or side-effects.
 
-        assert result['success'] is False
-        # FIXME: This error message should contain Fred's user name not his id.
-        assert result['msg'] == ('User bob not authorized to edit user '
-                                 'fred_user_id')
+        nose.tools.assert_raises(logic.NotAuthorized, helpers.call_auth,
+                                 'user_update', context=context, **params)
 
         # 4. Do nothing else!
 
@@ -107,9 +102,9 @@ def test_user_update_user_can_update_herself(self):
             'id': fred.id,
             'name': 'updated_user_name',
         }
-        result = helpers.call_auth('user_update', context=context, **params)
 
-        assert result['success'] is True
+        result = helpers.call_auth('user_update', context=context, **params)
+        assert result is True
 
     def test_user_update_with_no_user_in_context(self):
 
@@ -132,10 +127,8 @@ def test_user_update_with_no_user_in_context(self):
             'id': mock_user.id,
             'name': 'updated_user_name',
         }
-        result = helpers.call_auth('user_update', context=context, **params)
 
-        assert result['success'] is False
-        # FIXME: Be nice if this error message was a complete sentence.
-        assert result['msg'] == 'Have to be logged in to edit user'
+        nose.tools.assert_raises(logic.NotAuthorized, helpers.call_auth,
+                                 'user_update', context=context, **params)
 
     # TODO: Tests for user_update's reset_key behavior.

ckan/tests/logic/test_action.py

@@ -552,75 +552,6 @@ def test_12_user_update_errors(self):
             for expected_message in test_call['messages']:
                 assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]])
 
-    @mock.patch('ckan.lib.mailer.send_invite')
-    def test_user_invite(self, send_invite):
-        email_username = 'invited_user$ckan'
-        email = '%s@email.com' % email_username
-        organization_name = 'an_organization'
-        CreateTestData.create_groups([{'name': organization_name}])
-        role = 'member'
-        organization = model.Group.get(organization_name)
-        params = {'email': email,
-                  'group_id': organization.id,
-                  'role': role}
-        postparams = '%s=1' % json.dumps(params)
-        extra_environ = {'Authorization': str(self.sysadmin_user.apikey)}
-
-        res = self.app.post('/api/action/user_invite', params=postparams,
-                            extra_environ=extra_environ)
-
-        res_obj = json.loads(res.body)
-        user = model.User.get(res_obj['result']['id'])
-        assert res_obj['success'] is True, res_obj
-        assert user.email == email, (user.email, email)
-        assert user.is_pending(), user
-        expected_username = email_username.replace('$', '-')
-        assert user.name.startswith(expected_username), (user.name,
-                                                         expected_username)
-        group_ids = user.get_group_ids(capacity=role)
-        assert organization.id in group_ids, (group_ids, organization.id)
-        assert send_invite.called
-        assert send_invite.call_args[0][0].id == res_obj['result']['id']
-
-    @mock.patch('ckan.lib.mailer.mail_user')
-    def test_user_invite_without_email_raises_error(self, mail_user):
-        user_dict = {}
-        postparams = '%s=1' % json.dumps(user_dict)
-        extra_environ = {'Authorization': str(self.sysadmin_user.apikey)}
-
-        res = self.app.post('/api/action/user_invite', params=postparams,
-                            extra_environ=extra_environ,
-                            status=StatusCodes.STATUS_409_CONFLICT)
-
-        res_obj = json.loads(res.body)
-        assert res_obj['success'] is False, res_obj
-        assert 'email' in res_obj['error'], res_obj
-
-    @mock.patch('random.SystemRandom')
-    def test_user_invite_should_work_even_if_tried_username_already_exists(self, system_random_mock):
-        patcher = mock.patch('ckan.lib.mailer.mail_user')
-        patcher.start()
-        email = 'invited_user@email.com'
-        organization_name = 'an_organization'
-        CreateTestData.create_groups([{'name': organization_name}])
-        role = 'member'
-        organization = model.Group.get(organization_name)
-        params = {'email': email,
-                  'group_id': organization.id,
-                  'role': role}
-        postparams = '%s=1' % json.dumps(params)
-        extra_environ = {'Authorization': str(self.sysadmin_user.apikey)}
-
-        system_random_mock.return_value.random.side_effect = [1000, 1000, 2000, 3000]
-
-        for _ in range(2):
-            res = self.app.post('/api/action/user_invite', params=postparams,
-                                extra_environ=extra_environ)
-
-            res_obj = json.loads(res.body)
-            assert res_obj['success'] is True, res_obj
-        patcher.stop()
-
     def test_user_delete(self):
         name = 'normal_user'
         CreateTestData.create_user(name)

ckan/tests/logic/test_auth.py

@@ -61,16 +61,6 @@ def create_user(cls, name):
 
 
 class TestAuthUsers(TestAuth):
-    @mock.patch('ckan.logic.auth.create.group_member_create')
-    def test_invite_user_prepares_context_and_delegates_to_group_member_create(self, group_member_create):
-        context = {'group_id': 42}
-        group_member_create_context = context
-        group_member_create_context['id'] = context['group_id']
-
-        new_authz.is_authorized_boolean('user_invite', context)
-
-        group_member_create.assert_called(group_member_create_context, None)
-
     def test_only_sysadmins_can_delete_users(self):
         username = 'username'
         user = {'id': username}