Support to add/remove multi users for "group add/remove user"

Similar delete commands in OSC, we can also support add/remove
multi users for one specified group, this review implement it.

Change-Id: I8ccf99d4ee83a18778fa3ff5c0a42bc7c6ff21fb
Implements: bp support-multi-add-remove

Author: Huanxuan Ao

doc/source/command-objects/group.rst

@@ -16,7 +16,7 @@ Add user to group
         [--group-domain <group-domain>]
         [--user-domain <user-domain>]
         <group>
-        <user>
+        <user> [<user> ...]
 
 .. option:: --group-domain <group-domain>
 
@@ -38,7 +38,8 @@ Add user to group
 
 .. describe:: <user>
 
-    User to add to <group> (name or ID)
+    User(s) to add to <group> (name or ID)
+    (repeat option to add multiple users)
 
 group contains user
 -------------------
@@ -172,7 +173,7 @@ Remove user from group
         [--group-domain <group-domain>]
         [--user-domain <user-domain>]
         <group>
-        <user>
+        <user> [<user> ...]
 
 .. option:: --group-domain <group-domain>
 
@@ -194,7 +195,8 @@ Remove user from group
 
 .. describe:: <user>
 
-    User to remove from <group> (name or ID)
+    User(s) to remove from <group> (name or ID)
+    (repeat option to remove multiple users)
 
 group set
 ---------

openstackclient/identity/v3/group.py

@@ -44,7 +44,9 @@ def get_parser(self, prog_name):
         parser.add_argument(
             'user',
             metavar='<user>',
-            help=_('User to add to <group> (name or ID)'),
+            nargs='+',
+            help=_('User(s) to add to <group> (name or ID) '
+                   '(repeat option to add multiple users)'),
         )
         common.add_group_domain_option_to_parser(parser)
         common.add_user_domain_option_to_parser(parser)
@@ -53,20 +55,32 @@ def get_parser(self, prog_name):
     def take_action(self, parsed_args):
         identity_client = self.app.client_manager.identity
 
-        user_id = common.find_user(identity_client,
-                                   parsed_args.user,
-                                   parsed_args.user_domain).id
         group_id = common.find_group(identity_client,
                                      parsed_args.group,
                                      parsed_args.group_domain).id
 
-        try:
-            identity_client.users.add_to_group(user_id, group_id)
-        except Exception as e:
-            msg = _("%(user)s not added to group %(group)s: %(e)s") % {
-                'user': parsed_args.user,
+        result = 0
+        for i in parsed_args.user:
+            try:
+                user_id = common.find_user(identity_client,
+                                           i,
+                                           parsed_args.user_domain).id
+                identity_client.users.add_to_group(user_id, group_id)
+            except Exception as e:
+                result += 1
+                msg = _("%(user)s not added to group %(group)s: %(e)s") % {
+                    'user': i,
+                    'group': parsed_args.group,
+                    'e': e,
+                }
+                LOG.error(msg)
+        if result > 0:
+            total = len(parsed_args.user)
+            msg = (_("%(result)s of %(total)s users not added to group "
+                   "%(group)s.")) % {
+                'result': result,
+                'total': total,
                 'group': parsed_args.group,
-                'e': e,
             }
             raise exceptions.CommandError(msg)
 
@@ -286,7 +300,9 @@ def get_parser(self, prog_name):
         parser.add_argument(
             'user',
             metavar='<user>',
-            help=_('User to remove from <group> (name or ID)'),
+            nargs='+',
+            help=_('User(s) to remove from <group> (name or ID) '
+                   '(repeat option to remove multiple users)'),
         )
         common.add_group_domain_option_to_parser(parser)
         common.add_user_domain_option_to_parser(parser)
@@ -295,20 +311,32 @@ def get_parser(self, prog_name):
     def take_action(self, parsed_args):
         identity_client = self.app.client_manager.identity
 
-        user_id = common.find_user(identity_client,
-                                   parsed_args.user,
-                                   parsed_args.user_domain).id
         group_id = common.find_group(identity_client,
                                      parsed_args.group,
                                      parsed_args.group_domain).id
 
-        try:
-            identity_client.users.remove_from_group(user_id, group_id)
-        except Exception as e:
-            msg = _("%(user)s not removed from group %(group)s: %(e)s") % {
-                'user': parsed_args.user,
+        result = 0
+        for i in parsed_args.user:
+            try:
+                user_id = common.find_user(identity_client,
+                                           i,
+                                           parsed_args.user_domain).id
+                identity_client.users.remove_from_group(user_id, group_id)
+            except Exception as e:
+                result += 1
+                msg = _("%(user)s not removed from group %(group)s: %(e)s") % {
+                    'user': i,
+                    'group': parsed_args.group,
+                    'e': e,
+                }
+                LOG.error(msg)
+        if result > 0:
+            total = len(parsed_args.user)
+            msg = (_("%(result)s of %(total)s users not removed from group "
+                   "%(group)s.")) % {
+                'result': result,
+                'total': total,
                 'group': parsed_args.group,
-                'e': e,
             }
             raise exceptions.CommandError(msg)
 

openstackclient/tests/unit/identity/v3/fakes.py

@@ -770,6 +770,44 @@ def create_one_user(attrs=None):
                                   loaded=True)
         return user
 
+    @staticmethod
+    def create_users(attrs=None, count=2):
+        """Create multiple fake users.
+
+        :param Dictionary attrs:
+            A dictionary with all attributes
+        :param int count:
+            The number of users to fake
+        :return:
+            A list of FakeResource objects faking the users
+        """
+        users = []
+        for i in range(0, count):
+            user = FakeUser.create_one_user(attrs)
+            users.append(user)
+
+        return users
+
+    @staticmethod
+    def get_users(users=None, count=2):
+        """Get an iterable MagicMock object with a list of faked users.
+
+        If users list is provided, then initialize the Mock object with
+        the list. Otherwise create one.
+
+        :param List users:
+            A list of FakeResource objects faking users
+        :param Integer count:
+            The number of users to be faked
+        :return
+            An iterable Mock object with side_effect set to a list of faked
+            users
+        """
+        if users is None:
+            users = FakeUser.create_users(count)
+
+        return mock.Mock(side_effect=users)
+
 
 class FakeGroup(object):
     """Fake one or more group."""

openstackclient/tests/unit/identity/v3/test_group.py

@@ -42,47 +42,78 @@ def setUp(self):
 
 class TestGroupAddUser(TestGroup):
 
-    group = identity_fakes.FakeGroup.create_one_group()
-    user = identity_fakes.FakeUser.create_one_user()
+    _group = identity_fakes.FakeGroup.create_one_group()
+    users = identity_fakes.FakeUser.create_users(count=2)
 
     def setUp(self):
         super(TestGroupAddUser, self).setUp()
 
-        self.groups_mock.get.return_value = self.group
-        self.users_mock.get.return_value = self.user
+        self.groups_mock.get.return_value = self._group
+        self.users_mock.get = (
+            identity_fakes.FakeUser.get_users(self.users))
         self.users_mock.add_to_group.return_value = None
 
         self.cmd = group.AddUserToGroup(self.app, None)
 
     def test_group_add_user(self):
         arglist = [
-            self.group.name,
-            self.user.name,
+            self._group.name,
+            self.users[0].name,
         ]
         verifylist = [
-            ('group', self.group.name),
-            ('user', self.user.name),
+            ('group', self._group.name),
+            ('user', [self.users[0].name]),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         result = self.cmd.take_action(parsed_args)
         self.users_mock.add_to_group.assert_called_once_with(
-            self.user.id, self.group.id)
+            self.users[0].id, self._group.id)
         self.assertIsNone(result)
 
-    def test_group_add_user_with_error(self):
-        self.users_mock.add_to_group.side_effect = exceptions.CommandError()
+    def test_group_add_multi_users(self):
         arglist = [
-            self.group.name,
-            self.user.name,
+            self._group.name,
+            self.users[0].name,
+            self.users[1].name,
         ]
         verifylist = [
-            ('group', self.group.name),
-            ('user', self.user.name),
+            ('group', self._group.name),
+            ('user', [self.users[0].name, self.users[1].name]),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
+
+        result = self.cmd.take_action(parsed_args)
+        calls = [call(self.users[0].id, self._group.id),
+                 call(self.users[1].id, self._group.id)]
+        self.users_mock.add_to_group.assert_has_calls(calls)
+        self.assertIsNone(result)
+
+    @mock.patch.object(group.LOG, 'error')
+    def test_group_add_user_with_error(self, mock_error):
+        self.users_mock.add_to_group.side_effect = [
+            exceptions.CommandError(), None]
+        arglist = [
+            self._group.name,
+            self.users[0].name,
+            self.users[1].name,
+        ]
+        verifylist = [
+            ('group', self._group.name),
+            ('user', [self.users[0].name, self.users[1].name]),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        try:
+            self.cmd.take_action(parsed_args)
+            self.fail('CommandError should be raised.')
+        except exceptions.CommandError as e:
+            msg = "1 of 2 users not added to group %s." % self._group.name
+            self.assertEqual(msg, str(e))
+        msg = ("%(user)s not added to group %(group)s: ") % {
+            'user': self.users[0].name,
+            'group': self._group.name,
+        }
+        mock_error.assert_called_once_with(msg)
 
 
 class TestGroupCheckUser(TestGroup):
@@ -463,48 +494,78 @@ def test_group_list_long(self):
 
 class TestGroupRemoveUser(TestGroup):
 
-    group = identity_fakes.FakeGroup.create_one_group()
-    user = identity_fakes.FakeUser.create_one_user()
+    _group = identity_fakes.FakeGroup.create_one_group()
+    users = identity_fakes.FakeUser.create_users(count=2)
 
     def setUp(self):
         super(TestGroupRemoveUser, self).setUp()
 
-        self.groups_mock.get.return_value = self.group
-        self.users_mock.get.return_value = self.user
+        self.groups_mock.get.return_value = self._group
+        self.users_mock.get = (
+            identity_fakes.FakeUser.get_users(self.users))
         self.users_mock.remove_from_group.return_value = None
 
         self.cmd = group.RemoveUserFromGroup(self.app, None)
 
     def test_group_remove_user(self):
         arglist = [
-            self.group.id,
-            self.user.id,
+            self._group.id,
+            self.users[0].id,
         ]
         verifylist = [
-            ('group', self.group.id),
-            ('user', self.user.id),
+            ('group', self._group.id),
+            ('user', [self.users[0].id]),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         result = self.cmd.take_action(parsed_args)
         self.users_mock.remove_from_group.assert_called_once_with(
-            self.user.id, self.group.id)
+            self.users[0].id, self._group.id)
         self.assertIsNone(result)
 
-    def test_group_remove_user_with_error(self):
-        self.users_mock.remove_from_group.side_effect = (
-            exceptions.CommandError())
+    def test_group_remove_multi_users(self):
         arglist = [
-            self.group.id,
-            self.user.id,
+            self._group.name,
+            self.users[0].name,
+            self.users[1].name,
         ]
         verifylist = [
-            ('group', self.group.id),
-            ('user', self.user.id),
+            ('group', self._group.name),
+            ('user', [self.users[0].name, self.users[1].name]),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertRaises(exceptions.CommandError,
-                          self.cmd.take_action, parsed_args)
+
+        result = self.cmd.take_action(parsed_args)
+        calls = [call(self.users[0].id, self._group.id),
+                 call(self.users[1].id, self._group.id)]
+        self.users_mock.remove_from_group.assert_has_calls(calls)
+        self.assertIsNone(result)
+
+    @mock.patch.object(group.LOG, 'error')
+    def test_group_remove_user_with_error(self, mock_error):
+        self.users_mock.remove_from_group.side_effect = [
+            exceptions.CommandError(), None]
+        arglist = [
+            self._group.id,
+            self.users[0].id,
+            self.users[1].id,
+        ]
+        verifylist = [
+            ('group', self._group.id),
+            ('user', [self.users[0].id, self.users[1].id]),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        try:
+            self.cmd.take_action(parsed_args)
+            self.fail('CommandError should be raised.')
+        except exceptions.CommandError as e:
+            msg = "1 of 2 users not removed from group %s." % self._group.id
+            self.assertEqual(msg, str(e))
+        msg = ("%(user)s not removed from group %(group)s: ") % {
+            'user': self.users[0].id,
+            'group': self._group.id,
+        }
+        mock_error.assert_called_once_with(msg)
 
 
 class TestGroupSet(TestGroup):

releasenotes/notes/bp-support-multi-add-remove-9516f72cfacea11a.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    Add support to add/remove multi users by ``group add/remove user`` command.
+    [Blueprint  :oscbp:`support-multi-add-remove`]