#902 Added editUser api call
Merged 4 years ago by tkopecek. Opened 5 years ago by breilly.
breilly/koji useredit-862  into  master

file modified
+17
@@ -2029,6 +2029,23 @@ 

      session.disableUser(username)

  

  

+ def handle_edit_user(goptions, session, args):

+     "[admin] Alter user information"

+     usage = _("usage: %prog edit-user name [options]")

+     usage += _("\n(Specify the --help global option for a list of other help options)")

+     parser = OptionParser(usage=usage)

+     parser.add_option("--rename", help=_("Rename the user"))

+     parser.add_option("--krb", help=_("Change kerberos principal of the user"))

+     (options, args) = parser.parse_args(args)

+     if len(args) < 1:

+         parser.error(_("You must specify the username of the user to edit"))

+     elif len(args) > 1:

+         parser.error(_("This command only accepts one argument (username)"))

+     activate_session(session, goptions)

+     user = args[0]

+     session.editUser(user, options.rename, options.krb)

+ 

+ 

  def handle_list_signed(goptions, session, args):

      "[admin] List signed copies of rpms"

      usage = _("usage: %prog list-signed [options]")

file modified
+37
@@ -3701,6 +3701,42 @@ 

          user['krb_principals'] = list_user_krb_principals(user['id'])

      return user

  

+ def edit_user(userInfo, name=None, krb_principal=None):

+     """Edit information for an existing user.

+ 

+     userInfo specifies the user to edit

+     fields changes are provided as keyword arguments:

+         name: rename the user

+         krb_principal: change user's kerberos principal

+     """

+ 

+     context.session.assertPerm('admin')

+     _edit_user(userInfo, name=name, krb_principal=krb_principal)

+ 

+ 

+ def _edit_user(userInfo, name=None, krb_principal=None):

+     """Edit information for an existing user."""

+     user = get_user(userInfo, strict=True)

+     if name and user['name'] != name:

+         # attempt to update user name

+         values = {

+             'name': name,

+             'userID': user['id']

+             }

+         q = """SELECT id FROM users WHERE name=%(name)s"""

+         id = _singleValue(q, values, strict=False)

+         if id is not None:

+             # new name is taken

+             raise koji.GenericError("Name %s already taken by user %s" % (name, id))

+         update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i'])

+         update.set(name=name)

+         update.execute()

+     if krb_principal and user['krb_principal'] != krb_principal:

+         # attempt to update kerberos principal

+         update = UpdateProcessor('users', values={'userID': user['id']}, clauses=['id = %(userID)i'])

+         update.set(krb_principal=krb_principal)

+         update.execute()

+ 

  

  def list_user_krb_principals(user_info=None):

      """Return kerberos principal list of a user.
@@ -10942,6 +10978,7 @@ 

          return pkgs.get(pkg_id, None)

  

      getUser = staticmethod(get_user)

+     editUser = staticmethod(edit_user)

  

      def grantPermission(self, userinfo, permission, create=False):

          """Grant a permission to a user"""

@@ -26,6 +26,7 @@ 

          edit-tag                  Alter tag information

          edit-tag-inheritance      Edit tag inheritance

          edit-target               Set the name, build_tag, and/or dest_tag of an existing build target to new values

+         edit-user                 Alter user information

          enable-host               Mark one or more hosts as enabled

          enable-user               Enable logins by a user

          free-task                 Free a task

@@ -26,6 +26,7 @@ 

          edit-tag                  Alter tag information

          edit-tag-inheritance      Edit tag inheritance

          edit-target               Set the name, build_tag, and/or dest_tag of an existing build target to new values

+         edit-user                 Alter user information

          enable-host               Mark one or more hosts as enabled

          enable-user               Enable logins by a user

          free-task                 Free a task

@@ -0,0 +1,114 @@ 

+ from __future__ import absolute_import

+ import mock

+ import os

+ import six

+ import sys

+ import unittest

+ 

+ 

+ from koji_cli.commands import handle_edit_user

+ 

+ progname = os.path.basename(sys.argv[0]) or 'koji'

+ 

+ 

+ class TestEditUser(unittest.TestCase):

+     # Show long diffs in error output...

+     maxDiff = None

+ 

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     @mock.patch('koji_cli.commands.activate_session')

+     def test_handle_edit_user(self, activate_session_mock, stdout):

+         user = 'user'

+         rename = 'user2'

+         krb_principal = 'krb'

+         args = [user]

+         args.append('--rename=' + rename)

+         args.append('--krb=' + krb_principal)

+         options = mock.MagicMock()

+ 

+         # Mock out the xmlrpc server

+         session = mock.MagicMock()

+ 

+         # Run it and check immediate output

+         # args: user --rename=user --krb=krb

+         # expected: success

+         rv = handle_edit_user(options, session, args)

+         actual = stdout.getvalue()

+         expected = ''

+         self.assertMultiLineEqual(actual, expected)

+         # Finally, assert that things were called as we expected.

+         activate_session_mock.assert_called_once_with(session, options)

+         session.editUser.assert_called_once_with(user, rename, krb_principal)

+         self.assertEqual(rv, None)

+ 

+         stdout.seek(0)

+         stdout.truncate()

+         session.reset_mock()

+         activate_session_mock.reset_mock()

+ 

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     @mock.patch('sys.stderr', new_callable=six.StringIO)

+     @mock.patch('koji_cli.commands.activate_session')

+     def test_handle_edit_user_help(self, activate_session_mock, stderr, stdout):

+         args = ['--help']

+         options = mock.MagicMock()

+ 

+         # Mock out the xmlrpc server

+         session = mock.MagicMock()

+ 

+         # Run it and check immediate output

+         # args: --help

+         # expected: failed, help info shows

+         with self.assertRaises(SystemExit) as cm:

+             handle_edit_user(options, session, args)

+         actual_stdout = stdout.getvalue()

+         actual_stderr = stderr.getvalue()

+         expected_stdout = """Usage: %s edit-user name [options]

+ (Specify the --help global option for a list of other help options)

+ 

+ Options:

+   -h, --help       show this help message and exit

+   --rename=RENAME  Rename the user

+   --krb=KRB        Change kerberos principal of the user

+ """ % progname

+         expected_stderr = ''

+         self.assertMultiLineEqual(actual_stdout, expected_stdout)

+         self.assertMultiLineEqual(actual_stderr, expected_stderr)

+         # Finally, assert that things were called as we expected.

+         activate_session_mock.assert_not_called()

+         session.editUser.assert_not_called()

+         self.assertEqual(cm.exception.code, 0)

+ 

+     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     @mock.patch('sys.stderr', new_callable=six.StringIO)

+     @mock.patch('koji_cli.commands.activate_session')

+     def test_handle_edit_user_no_arg(self, activate_session_mock, stderr, stdout):

+         args = []

+         options = mock.MagicMock()

+ 

+         # Mock out the xmlrpc server

+         session = mock.MagicMock()

+ 

+         # Run it and check immediate output

+         # args: --help

+         # expected: failed, help info shows

+         with self.assertRaises(SystemExit) as cm:

+             handle_edit_user(options, session, args)

+         actual_stdout = stdout.getvalue()

+         actual_stderr = stderr.getvalue()

+         expected_stdout = ''

+         expected_stderr = """Usage: %(progname)s edit-user name [options]

+ (Specify the --help global option for a list of other help options)

+ 

+ %(progname)s: error: You must specify the username of the user to edit

+ """ % {'progname': progname}

+         self.assertMultiLineEqual(actual_stdout, expected_stdout)

+         self.assertMultiLineEqual(actual_stderr, expected_stderr)

+         # Finally, assert that things were called as we expected.

+         activate_session_mock.assert_not_called()

+         session.editUser.assert_not_called()

+         self.assertEqual(cm.exception.code, 2)

+ 

+ 

+ if __name__ == '__main__':

+     unittest.main()

@@ -0,0 +1,60 @@ 

+ import mock

+ import unittest

+ 

+ import koji

+ import kojihub

+ 

+ UP = kojihub.UpdateProcessor

+ 

+ class TestEditUser(unittest.TestCase):

+ 

+     def getUpdate(self, *args, **kwargs):

+         update = UP(*args, **kwargs)

+         update.execute = mock.MagicMock()

+         self.updates.append(update)

+         return update

+ 

+     def setUp(self):

+         self.updates = []

+         self._singleValue = mock.patch('kojihub._singleValue').start()

+         self.get_user = mock.patch('kojihub.get_user').start()

+         self.context = mock.patch('kojihub.context').start()

+         self.UpdateProcessor = mock.patch('kojihub.UpdateProcessor',

+                                           side_effect=self.getUpdate).start()

+         # It seems MagicMock will not automatically handle attributes that

+         # start with "assert"

+         self.context.session.assertLogin = mock.MagicMock()

+ 

+     def tearDown(self):

+         mock.patch.stopall()

+ 

+     def test_edit(self):

+         self.get_user.return_value = {'id': 333,

+                                      'name': 'user',

+                                      'krb_principal': 'krb'}

+         self._singleValue.return_value = None

+ 

+         kojihub._edit_user('user', name='newuser', krb_principal='krb')

+         # check the update

+         self.assertEqual(len(self.updates), 1)

+         update = self.updates[0]

+         self.assertEqual(update.table, 'users')

+         self.assertEqual(update.data, {'name': 'newuser'})

+         self.assertEqual(update.values, {'userID': 333})

+         self.assertEqual(update.clauses, ['id = %(userID)i'])

+ 

+         kojihub._edit_user('user', name='user', krb_principal='newkrb')

+         # check the insert/update

+         self.assertEqual(len(self.updates), 2)

+         update = self.updates[1]

+         self.assertEqual(update.table, 'users')

+         self.assertEqual(update.data, {'krb_principal': 'newkrb'})

+         self.assertEqual(update.values, {'userID': 333})

+         self.assertEqual(update.clauses, ['id = %(userID)i'])

+ 

+         self._singleValue.reset_mock()

+         self._singleValue.return_value = 2

+         with self.assertRaises(koji.GenericError) as cm:

+             kojihub._edit_user('user', name='newuser')

+         self._singleValue.assert_called_once()

+         self.assertEqual(cm.exception.args[0], 'Name newuser already taken by user 2')

As 'status' is handled by enable/disable user and user_perms are handled elsewhere, only included renaming and krb_principal here.

Can we get some tests?

@mikem What is your opinion on future use of _dml? Are we going to use Insert/UpdateProcessor where possible, or is it still ok to use _dml for such simple queries?

1 new commit added

  • Added edit_user unit tests
5 years ago

2 new commits added

  • Added edit_user unit tests
  • Added editUser api call
5 years ago

@mikem What is your opinion on future use of _dml? Are we going to use Insert/UpdateProcessor where possible, or is it still ok to use _dml for such simple queries?

Yeah, don't use _dml() in new code if at all possible. There may be exceptions, but this case is clearly a job for UpdateProcessor.

rebased onto a83c1d6df50168e5e86d3efe57c2bc68f916746a

5 years ago

:thumbsup: you can also use QueryProcessor here, but it is a simple one.

Metadata Update from @breilly:
- Pull-request tagged with: testing-ready

4 years ago

I think it's better to use obvious arguments than **kwargs

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

4 years ago

Agreed, that kwargs should be replaced with explicit options.

rebased onto 3bf9726

4 years ago

Commit be159d7 fixes this pull-request

Pull-Request has been merged by tkopecek

4 years ago