users: Minor refactoring

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>

actions/users

@@ -264,11 +264,12 @@ def subcommand_set_user_password(arguments):
 
 
 def get_user_groups(username):
-    """Returns only the supplementary groups of the given user and doesn't include
+    """Returns only the supplementary groups of the given user.
-    the 'users' primary group"""
+
+    Exclude the 'users' primary group from the returned list."""
     process = _run(
         ['ldapid', username], stdout=subprocess.PIPE, check=False)
-    output = process.stdout.decode('utf-8').strip()
+    output = process.stdout.decode().strip()
     if output:
         groups_part = output.split(' ')[2]
         groups = groups_part.split('=')[1]
@@ -276,13 +277,13 @@ def get_user_groups(username):
                        for user in re.findall('\(.*?\)', groups)]
         group_names.remove('users')
         return group_names
+
     return []
 
 
 def subcommand_get_user_groups(arguments):
     """Return list of a given user's groups."""
-    groups = [group
+    groups = get_user_groups(arguments.username)
-              for group in get_user_groups(arguments.username)]
     if groups:
         print(*groups, sep='\n')
 
@@ -327,8 +328,7 @@ def subcommand_add_user_to_group(arguments):
 
 def remove_user_from_group(username, groupname):
     """Remove an LDAP user from an LDAP group."""
-    _run(
+    _run(['ldapdeleteuserfromgroup', username, groupname])
-        ['ldapdeleteuserfromgroup', username, groupname])
 
 
 def subcommand_remove_user_from_group(arguments):

plinth/modules/deluge/__init__.py

@@ -82,7 +82,7 @@ def setup(helper, old_version=None):
             disable=disable)
     helper.call('post', service.notify_enabled, None, True)
     helper.call('post', add_shortcut)
-    add_group("bittorrent")
+    add_group('bittorrent')
 
 
 def add_shortcut():

plinth/modules/users/__init__.py

@@ -87,8 +87,10 @@ def _diagnose_ldap_entry(search_item):
 
 
 def add_group(group):
+    """Add an LDAP group."""
     actions.superuser_run('users', options=['create-group', group])
 
 
 def remove_group(group):
+    """Remove an LDAP group."""
     actions.superuser_run('users', options=['remove-group', group])

plinth/modules/users/tests/test_actions.py

@@ -96,6 +96,7 @@ class TestActions(unittest.TestCase):
                 self.delete_user(user)
             except Exception:
                 pass
+
         for group in self.groups:
             self.delete_group(group)
 
@@ -137,7 +138,7 @@ class TestActions(unittest.TestCase):
         """Return the list of groups for a user."""
         process = self.call_action(
             ['get-user-groups', username], stdout=subprocess.PIPE)
-        return process.stdout.split()
+        return process.stdout.decode().split()
 
     def create_group(self, groupname=None):
         groupname = groupname or random_string()
@@ -192,11 +193,9 @@ class TestActions(unittest.TestCase):
 
         new_groups = self.get_user_groups(new_username)
         old_users_groups = self.get_user_groups(old_username)
-        self.assertFalse(len(old_users_groups))
+        self.assertFalse(old_users_groups)  # empty
         self.assertEqual(old_groups, new_groups)
 
-        self.assertFalse(self.get_user_groups(old_username))  # is empty
-
         with self.assertRaises(subprocess.CalledProcessError):
             self.rename_user(old_username)
 
@@ -214,8 +213,6 @@ class TestActions(unittest.TestCase):
     def test_delete_user(self):
         """Test to check whether LDAP users can be deleted"""
         username, password = self.create_user(groups=[random_string()])
-        groups_before = self.get_user_groups(username)
-        self.assertTrue(groups_before)
         self.delete_user(username)
         groups_after = self.get_user_groups(username)
         self.assertFalse(groups_after)  # User gets removed from all groups
@@ -256,6 +253,7 @@ class TestActions(unittest.TestCase):
     def test_user_group_interactions(self):
         group1 = random_string()
         user1, _ = self.create_user(groups=[group1])
+        self.assertEqual([group1], self.get_user_groups(user1))
 
         # add-user-to-group is not idempotent
         with self.assertRaises(subprocess.CalledProcessError):
@@ -273,8 +271,7 @@ class TestActions(unittest.TestCase):
 
         # The expected groups got created and the user is part of them.
         expected_groups = [group1, group2, group3]
-        actual_groups = [g.decode() for g in self.get_user_groups(user1)]
+        self.assertEqual(expected_groups, self.get_user_groups(user1))
-        self.assertEqual(expected_groups, actual_groups)
 
         # Remove user from group
         group_to_remove_from = random.choice(expected_groups)
@@ -283,8 +280,7 @@ class TestActions(unittest.TestCase):
 
         # User is no longer in the group that they're removed from
         expected_groups.remove(group_to_remove_from)
-        actual_groups = [g.decode() for g in self.get_user_groups(user1)]
+        self.assertEqual(expected_groups, self.get_user_groups(user1))
-        self.assertEqual(expected_groups, actual_groups)
 
         # User cannot be removed from a group that they're not part of
         random_group = random_string()