From b56f726dc2e88e114bc17078a09027d9071ddda0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Wed, 25 Apr 2018 12:55:36 +0100
Subject: [PATCH] Add 'other' attribute to GECOS fields to avoid
 inconsistencies with chfn

Fix unsupported chars checking on GECOS fields

Add unit test for new method 'user.chother'

Do make comparisons in a single line

Add 'other' as valid kwargs for 'user.add' method
---
 salt/modules/useradd.py            |  39 +++-
 salt/states/user.py                |  28 ++-
 tests/unit/modules/test_useradd.py | 445 +++++++++++++++++++++++++++++++++++++
 tests/unit/modules/useradd_test.py |  36 ++-
 4 files changed, 528 insertions(+), 20 deletions(-)
 create mode 100644 tests/unit/modules/test_useradd.py

diff --git a/salt/modules/useradd.py b/salt/modules/useradd.py
index 2fa3b2d7e2..9e71296d85 100644
--- a/salt/modules/useradd.py
+++ b/salt/modules/useradd.py
@@ -52,17 +52,18 @@ def _get_gecos(name):
     '''
     Retrieve GECOS field info and return it in dictionary form
     '''
-    gecos_field = pwd.getpwnam(_quote_username(name)).pw_gecos.split(',', 3)
+    gecos_field = pwd.getpwnam(_quote_username(name)).pw_gecos.split(',', 4)
     if not gecos_field:
         return {}
     else:
         # Assign empty strings for any unspecified trailing GECOS fields
-        while len(gecos_field) < 4:
+        while len(gecos_field) < 5:
             gecos_field.append('')
         return {'fullname': locales.sdecode(gecos_field[0]),
                 'roomnumber': locales.sdecode(gecos_field[1]),
                 'workphone': locales.sdecode(gecos_field[2]),
-                'homephone': locales.sdecode(gecos_field[3])}
+                'homephone': locales.sdecode(gecos_field[3]),
+                'other': locales.sdecode(gecos_field[4])}
 
 
 def _build_gecos(gecos_dict):
@@ -70,10 +71,11 @@ def _build_gecos(gecos_dict):
     Accepts a dictionary entry containing GECOS field names and their values,
     and returns a full GECOS comment string, to be used with usermod.
     '''
-    return u'{0},{1},{2},{3}'.format(gecos_dict.get('fullname', ''),
-                                    gecos_dict.get('roomnumber', ''),
-                                    gecos_dict.get('workphone', ''),
-                                    gecos_dict.get('homephone', '')).rstrip(',')
+    return '{0},{1},{2},{3},{4}'.format(gecos_dict.get('fullname', ''),
+                                        gecos_dict.get('roomnumber', ''),
+                                        gecos_dict.get('workphone', ''),
+                                        gecos_dict.get('homephone', ''),
+                                        gecos_dict.get('other', ''),).rstrip(',')
 
 
 def _update_gecos(name, key, value, root=None):
@@ -114,6 +116,7 @@ def add(name,
         roomnumber='',
         workphone='',
         homephone='',
+        other='',
         createhome=True,
         loginclass=None,
         root=None):
@@ -221,6 +224,8 @@ def add(name,
         chworkphone(name, workphone)
     if homephone:
         chhomephone(name, homephone)
+    if other:
+        chother(name, other)
     return True
 
 
@@ -491,6 +496,19 @@ def chhomephone(name, homephone):
     return _update_gecos(name, 'homephone', homephone)
 
 
+def chother(name, other):
+    '''
+    Change the user's other GECOS attribute
+
+    CLI Example:
+
+    .. code-block:: bash
+
+        salt '*' user.chother foobar
+    '''
+    return _update_gecos(name, 'other', other)
+
+
 def chloginclass(name, loginclass, root=None):
     '''
     Change the default login class of the user
@@ -572,9 +590,9 @@ def _format_info(data):
     Return user information in a pretty way
     '''
     # Put GECOS info into a list
-    gecos_field = data.pw_gecos.split(',', 3)
+    gecos_field = data.pw_gecos.split(',', 4)
     # Make sure our list has at least four elements
-    while len(gecos_field) < 4:
+    while len(gecos_field) < 5:
         gecos_field.append('')
 
     return {'gid': data.pw_gid,
@@ -587,7 +605,8 @@ def _format_info(data):
             'fullname': gecos_field[0],
             'roomnumber': gecos_field[1],
             'workphone': gecos_field[2],
-            'homephone': gecos_field[3]}
+            'homephone': gecos_field[3],
+            'other': gecos_field[4]}
 
 
 @decorators.which('id')
diff --git a/salt/states/user.py b/salt/states/user.py
index 48f0563ffb..4bc73e69ba 100644
--- a/salt/states/user.py
+++ b/salt/states/user.py
@@ -66,6 +66,7 @@ def _changes(name,
              roomnumber='',
              workphone='',
              homephone='',
+             other='',
              loginclass=None,
              date=0,
              mindays=0,
@@ -164,24 +165,26 @@ def _changes(name,
 
     # MacOS doesn't have full GECOS support, so check for the "ch" functions
     # and ignore these parameters if these functions do not exist.
-    if 'user.chroomnumber' in __salt__ \
-            and roomnumber is not None:
+    if 'user.chroomnumber' in __salt__ and roomnumber is not None:
         roomnumber = sdecode_if_string(roomnumber)
         lusr['roomnumber'] = sdecode_if_string(lusr['roomnumber'])
         if lusr['roomnumber'] != roomnumber:
             change['roomnumber'] = roomnumber
-    if 'user.chworkphone' in __salt__ \
-            and workphone is not None:
+    if 'user.chworkphone' in __salt__ and workphone is not None:
         workphone = sdecode_if_string(workphone)
         lusr['workphone'] = sdecode_if_string(lusr['workphone'])
         if lusr['workphone'] != workphone:
             change['workphone'] = workphone
-    if 'user.chhomephone' in __salt__ \
-            and homephone is not None:
+    if 'user.chhomephone' in __salt__ and homephone is not None:
         homephone = sdecode_if_string(homephone)
         lusr['homephone'] = sdecode_if_string(lusr['homephone'])
         if lusr['homephone'] != homephone:
             change['homephone'] = homephone
+    if 'user.chother' in __salt__ and other is not None:
+        other = sdecode_if_string(other)
+        lusr['other'] = sdecode_if_string(lusr['other'])
+        if lusr['other'] != other:
+            change['other'] = other
     # OpenBSD/FreeBSD login class
     if __grains__['kernel'] in ('OpenBSD', 'FreeBSD'):
         if loginclass:
@@ -211,6 +214,7 @@ def present(name,
             roomnumber=None,
             workphone=None,
             homephone=None,
+            other=None,
             loginclass=None,
             date=None,
             mindays=None,
@@ -330,7 +334,10 @@ def present(name,
 
     homephone
         The user's home phone number (not supported in MacOS)
-        If GECOS field contains more than 3 commas, this field will have the rest of 'em
+
+    other
+        The user's other attribute (not supported in MacOS)
+        If GECOS field contains more than 4 commas, this field will have the rest of 'em
 
     .. versionchanged:: 2014.7.0
        Shadow attribute support added.
@@ -403,6 +410,8 @@ def present(name,
         workphone = sdecode(workphone)
     if homephone is not None:
         homephone = sdecode(homephone)
+    if other is not None:
+        other = sdecode(other)
 
     ret = {'name': name,
            'changes': {},
@@ -411,7 +420,7 @@ def present(name,
 
     # the comma is used to separate field in GECOS, thus resulting into
     # salt adding the end of fullname each time this function is called
-    for gecos_field in ['fullname', 'roomnumber', 'workphone']:
+    for gecos_field in [fullname, roomnumber, workphone]:
         if isinstance(gecos_field, string_types) and ',' in gecos_field:
             ret['comment'] = "Unsupported char ',' in {0}".format(gecos_field)
             ret['result'] = False
@@ -468,6 +477,7 @@ def present(name,
                        roomnumber,
                        workphone,
                        homephone,
+                       other,
                        loginclass,
                        date,
                        mindays,
@@ -591,6 +601,7 @@ def present(name,
                            roomnumber,
                            workphone,
                            homephone,
+                           other,
                            loginclass,
                            date,
                            mindays,
@@ -636,6 +647,7 @@ def present(name,
                       'roomnumber': roomnumber,
                       'workphone': workphone,
                       'homephone': homephone,
+                      'other': other,
                       'createhome': createhome,
                       'loginclass': loginclass}
         else:
diff --git a/tests/unit/modules/test_useradd.py b/tests/unit/modules/test_useradd.py
new file mode 100644
index 0000000000..e79c78c663
--- /dev/null
+++ b/tests/unit/modules/test_useradd.py
@@ -0,0 +1,445 @@
+# -*- coding: utf-8 -*-
+'''
+    :codeauthor: :email:`Jayesh Kariya <jayeshk@saltstack.com>`
+'''
+
+# Import Python libs
+from __future__ import absolute_import, print_function, unicode_literals
+try:
+    import pwd
+    HAS_PWD = True
+except ImportError:
+    HAS_PWD = False
+
+# Import Salt Testing Libs
+from tests.support.mixins import LoaderModuleMockMixin
+from tests.support.unit import TestCase, skipIf
+from tests.support.mock import (
+    MagicMock,
+    patch,
+    NO_MOCK,
+    NO_MOCK_REASON
+)
+
+# Import Salt Libs
+import salt.modules.useradd as useradd
+from salt.exceptions import CommandExecutionError
+
+
+@skipIf(NO_MOCK, NO_MOCK_REASON)
+class UserAddTestCase(TestCase, LoaderModuleMockMixin):
+    '''
+    Test cases for salt.modules.useradd
+    '''
+    def setup_loader_modules(self):
+        return {useradd: {}}
+
+    @classmethod
+    def setUpClass(cls):
+        cls.mock_pwall = {'gid': 0,
+                          'groups': ['root'],
+                          'home': '/root',
+                          'name': 'root',
+                          'passwd': 'x',
+                          'shell': '/bin/bash',
+                          'uid': 0,
+                          'fullname': 'root',
+                          'roomnumber': '',
+                          'workphone': '',
+                          'homephone': '',
+                          'other': ''}
+
+    @classmethod
+    def tearDownClass(cls):
+        del cls.mock_pwall
+
+    # 'add' function tests: 1
+
+    def test_add(self):
+        '''
+        Test for adding a user
+        '''
+        with patch.dict(useradd.__grains__, {'kernel': 'OpenBSD'}):
+            mock_primary = MagicMock(return_value='Salt')
+            with patch.dict(useradd.__salt__,
+                            {'file.gid_to_group': mock_primary}):
+                mock = MagicMock(return_value={'retcode': 0})
+                with patch.dict(useradd.__salt__, {'cmd.run_all': mock}):
+                    self.assertTrue(useradd.add('Salt'))
+
+                mock = MagicMock(return_value={'retcode': 1})
+                with patch.dict(useradd.__salt__, {'cmd.run_all': mock}):
+                    self.assertFalse(useradd.add('Salt'))
+
+    # 'getent' function tests: 2
+
+    @skipIf(HAS_PWD is False, 'The pwd module is not available')
+    def test_getent(self):
+        '''
+        Test if user.getent already have a value
+        '''
+        with patch('salt.modules.useradd.__context__', MagicMock(return_value='Salt')):
+            self.assertTrue(useradd.getent())
+
+    @skipIf(HAS_PWD is False, 'The pwd module is not available')
+    def test_getent_user(self):
+        '''
+        Tests the return information on all users
+        '''
+        with patch('pwd.getpwall', MagicMock(return_value=[''])):
+            ret = [{'gid': 0,
+                    'groups': ['root'],
+                    'home': '/root',
+                    'name': 'root',
+                    'passwd': 'x',
+                    'shell': '/bin/bash',
+                    'uid': 0,
+                    'fullname': 'root',
+                    'roomnumber': '',
+                    'workphone': '',
+                    'homephone': '',
+                    'other': ''}]
+            with patch('salt.modules.useradd._format_info', MagicMock(return_value=self.mock_pwall)):
+                self.assertEqual(useradd.getent(), ret)
+
+    # 'chuid' function tests: 1
+
+    def test_chuid(self):
+        '''
+        Test if the uid of a user change
+        '''
+        mock = MagicMock(return_value={'uid': 11})
+        with patch.object(useradd, 'info', mock):
+            self.assertTrue(useradd.chuid('name', 11))
+
+        mock_run = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'uid': 11}, {'uid': 11}])
+            with patch.object(useradd, 'info', mock):
+                self.assertFalse(useradd.chuid('name', 22))
+
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'uid': 11}, {'uid': 22}])
+            with patch.object(useradd, 'info', mock):
+                self.assertTrue(useradd.chuid('name', 11))
+
+    # 'chgid' function tests: 1
+
+    def test_chgid(self):
+        '''
+        Test the default group of the user
+        '''
+        mock = MagicMock(return_value={'gid': 11})
+        with patch.object(useradd, 'info', mock):
+            self.assertTrue(useradd.chgid('name', 11))
+
+        mock_run = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'gid': 22}, {'gid': 22}])
+            with patch.object(useradd, 'info', mock):
+                self.assertFalse(useradd.chgid('name', 11))
+
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'gid': 11}, {'gid': 22}])
+            with patch.object(useradd, 'info', mock):
+                self.assertTrue(useradd.chgid('name', 11))
+
+    # 'chshell' function tests: 1
+
+    def test_chshell(self):
+        '''
+        Test the default shell of user
+        '''
+        mock = MagicMock(return_value={'shell': '/bin/bash'})
+        with patch.object(useradd, 'info', mock):
+            self.assertTrue(useradd.chshell('name', '/bin/bash'))
+
+        mock_run = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'shell': '/bin/bash'},
+                                          {'shell': '/bin/bash'}])
+            with patch.object(useradd, 'info', mock):
+                self.assertFalse(useradd.chshell('name', '/usr/bash'))
+
+        with patch.dict(useradd.__salt__, {'cmd.run': mock_run}):
+            mock = MagicMock(side_effect=[{'shell': '/bin/bash'},
+                                          {'shell': '/usr/bash'}])
+            with patch.object(useradd, 'info', mock):
+                self.assertTrue(useradd.chshell('name', '/bin/bash'))
+
+    # 'chhome' function tests: 1
+
+    def test_chhome(self):
+        '''
+        Test if home directory given is same as previous home directory
+        '''
+        mock = MagicMock(return_value={'home': '/root'})
+        with patch.object(useradd, 'info', mock):
+            self.assertTrue(useradd.chhome('name', '/root'))
+
+        mock = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+            mock = MagicMock(side_effect=[{'home': '/root'}, {'home': '/root'}])
+            with patch.object(useradd, 'info', mock):
+                self.assertFalse(useradd.chhome('name', '/user'))
+
+        mock = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+            mock = MagicMock(side_effect=[{'home': '/root'}, {'home': '/root'}])
+            with patch.object(useradd, 'info', mock):
+                self.assertTrue(useradd.chhome('name', '/root'))
+
+    # 'chgroups' function tests: 1
+
+    def test_chgroups(self):
+        '''
+        Test if user groups changed
+        '''
+        mock = MagicMock(return_value=['wheel', 'root'])
+        with patch.object(useradd, 'list_groups', mock):
+            self.assertTrue(useradd.chgroups('foo', 'wheel,root'))
+
+        mock = MagicMock(return_value=['wheel', 'root'])
+        with patch.object(useradd, 'list_groups', mock):
+            with patch.dict(useradd.__grains__, {'kernel': 'OpenBSD'}):
+                mock_runall = MagicMock(return_value={'retcode': False,
+                                                      'stderr': ''})
+                with patch.dict(useradd.__salt__, {'cmd.run_all': mock_runall}):
+                    self.assertTrue(useradd.chgroups('foo', 'wheel,test,root'))
+
+                mock_runall = MagicMock(return_value={'retcode': True,
+                                                      'stderr': ''})
+                with patch.dict(useradd.__salt__, {'cmd.run_all': mock_runall}):
+                    self.assertFalse(useradd.chgroups('foo', 'wheel,test,root'))
+
+    # 'chfullname' function tests: 1
+
+    def test_chfullname(self):
+        '''
+        Test if the user's Full Name is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chfullname('Salt', 'SaltStack'))
+
+        mock = MagicMock(return_value={'fullname': 'SaltStack'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chfullname('Salt', 'SaltStack'))
+
+        mock = MagicMock(return_value={'fullname': 'SaltStack'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'fullname': 'SaltStack2'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chfullname('Salt', 'SaltStack1'))
+
+        mock = MagicMock(return_value={'fullname': 'SaltStack2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'fullname': 'SaltStack2'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chfullname('Salt', 'SaltStack1'))
+
+    # 'chroomnumber' function tests: 1
+
+    def test_chroomnumber(self):
+        '''
+        Test if the user's Room Number is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chroomnumber('salt', 1))
+
+        mock = MagicMock(return_value={'roomnumber': '1'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chroomnumber('salt', 1))
+
+        mock = MagicMock(return_value={'roomnumber': '2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'roomnumber': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chroomnumber('salt', 1))
+
+        mock = MagicMock(return_value={'roomnumber': '3'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'roomnumber': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chroomnumber('salt', 1))
+
+    # 'chworkphone' function tests: 1
+
+    def test_chworkphone(self):
+        '''
+        Test if the user's Work Phone is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chworkphone('salt', 1))
+
+        mock = MagicMock(return_value={'workphone': '1'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chworkphone('salt', 1))
+
+        mock = MagicMock(return_value={'workphone': '2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'workphone': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chworkphone('salt', 1))
+
+        mock = MagicMock(return_value={'workphone': '3'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'workphone': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chworkphone('salt', 1))
+
+    # 'chhomephone' function tests: 1
+
+    def test_chhomephone(self):
+        '''
+        Test if the user's Home Phone is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chhomephone('salt', 1))
+
+        mock = MagicMock(return_value={'homephone': '1'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chhomephone('salt', 1))
+
+        mock = MagicMock(return_value={'homephone': '2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'homephone': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chhomephone('salt', 1))
+
+        mock = MagicMock(return_value={'homephone': '3'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'homephone': '3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chhomephone('salt', 1))
+
+    # 'chother' function tests: 1
+
+    def test_chother(self):
+        '''
+        Test if the user's other GECOS attribute is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chother('salt', 1))
+
+        mock = MagicMock(return_value={'other': 'foobar'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chother('salt', 'foobar'))
+
+        mock = MagicMock(return_value={'other': 'foobar2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'other': 'foobar3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chother('salt', 'foobar'))
+
+        mock = MagicMock(return_value={'other': 'foobar3'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'other': 'foobar3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chother('salt', 'foobar'))
+
+    # 'info' function tests: 1
+
+    @skipIf(HAS_PWD is False, 'The pwd module is not available')
+    def test_info(self):
+        '''
+        Test the user information
+        '''
+        self.assertEqual(useradd.info('username-that-doesnt-exist'), {})
+
+        mock = MagicMock(return_value=pwd.struct_passwd(('_TEST_GROUP',
+                                                         '*',
+                                                         83,
+                                                         83,
+                                                         'AMaViS Daemon',
+                                                         '/var/virusmails',
+                                                         '/usr/bin/false')))
+        with patch.object(pwd, 'getpwnam', mock):
+            self.assertEqual(useradd.info('username-that-doesnt-exist')['name'], '_TEST_GROUP')
+
+    # 'list_groups' function tests: 1
+
+    def test_list_groups(self):
+        '''
+        Test if it return a list of groups the named user belongs to
+        '''
+        with patch('salt.utils.user.get_group_list', MagicMock(return_value='Salt')):
+            self.assertEqual(useradd.list_groups('name'), 'Salt')
+
+    # 'list_users' function tests: 1
+
+    @skipIf(HAS_PWD is False, 'The pwd module is not available')
+    def test_list_users(self):
+        '''
+        Test if it returns a list of all users
+        '''
+        self.assertTrue(useradd.list_users())
+
+    # 'list_users' function tests: 1
+
+    def test_rename(self):
+        '''
+        Test if the username for a named user changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, 'info', mock):
+            self.assertRaises(CommandExecutionError, useradd.rename, 'salt', 1)
+
+        mock = MagicMock(return_value=True)
+        with patch.object(useradd, 'info', mock):
+            self.assertRaises(CommandExecutionError, useradd.rename, 'salt', 1)
+
+        mock = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+            mock = MagicMock(side_effect=[{'name': ''}, False,
+                                          {'name': 'salt'}])
+            with patch.object(useradd, 'info', mock):
+                self.assertTrue(useradd.rename('name', 'salt'))
+
+        mock = MagicMock(return_value=None)
+        with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+            mock = MagicMock(side_effect=[{'name': ''}, False, {'name': ''}])
+            with patch.object(useradd, 'info', mock):
+                self.assertFalse(useradd.rename('salt', 'salt'))
+
+    def test_build_gecos_field(self):
+        '''
+        Test if gecos fields are built correctly (removing trailing commas)
+        '''
+        test_gecos = {'fullname': 'Testing',
+                      'roomnumber': 1234,
+                      'workphone': 22222,
+                      'homephone': 99999}
+        expected_gecos_fields = 'Testing,1234,22222,99999'
+        self.assertEqual(useradd._build_gecos(test_gecos), expected_gecos_fields)
+        test_gecos.pop('roomnumber')
+        test_gecos.pop('workphone')
+        expected_gecos_fields = 'Testing,,,99999'
+        self.assertEqual(useradd._build_gecos(test_gecos), expected_gecos_fields)
+        test_gecos.pop('homephone')
+        expected_gecos_fields = 'Testing'
+        self.assertEqual(useradd._build_gecos(test_gecos), expected_gecos_fields)
diff --git a/tests/unit/modules/useradd_test.py b/tests/unit/modules/useradd_test.py
index 077e8f4239..dbe600f0c0 100644
--- a/tests/unit/modules/useradd_test.py
+++ b/tests/unit/modules/useradd_test.py
@@ -41,7 +41,8 @@ class UserAddTestCase(TestCase):
                   'fullname': 'root',
                   'roomnumber': '',
                   'workphone': '',
-                  'homephone': ''}
+                  'homephone': '',
+                  'other': ''}
 
     # 'add' function tests: 1
 
@@ -87,7 +88,8 @@ class UserAddTestCase(TestCase):
                   'fullname': 'root',
                   'roomnumber': '',
                   'workphone': '',
-                  'homephone': ''}]
+                  'homephone': '',
+                  'other': ''}]
         self.assertEqual(useradd.getent(), ret)
 
     # 'chuid' function tests: 1
@@ -320,6 +322,36 @@ class UserAddTestCase(TestCase):
                 with patch.object(useradd, 'info', mock):
                     self.assertFalse(useradd.chhomephone('salt', 1))
 
+    # 'chother' function tests: 1
+
+    def test_chother(self):
+        '''
+        Test if the user's other GECOS attribute is changed
+        '''
+        mock = MagicMock(return_value=False)
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertFalse(useradd.chother('salt', 1))
+
+        mock = MagicMock(return_value={'other': 'foobar'})
+        with patch.object(useradd, '_get_gecos', mock):
+            self.assertTrue(useradd.chother('salt', 'foobar'))
+
+        mock = MagicMock(return_value={'other': 'foobar2'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'other': 'foobar3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chother('salt', 'foobar'))
+
+        mock = MagicMock(return_value={'other': 'foobar3'})
+        with patch.object(useradd, '_get_gecos', mock):
+            mock = MagicMock(return_value=None)
+            with patch.dict(useradd.__salt__, {'cmd.run': mock}):
+                mock = MagicMock(return_value={'other': 'foobar3'})
+                with patch.object(useradd, 'info', mock):
+                    self.assertFalse(useradd.chother('salt', 'foobar'))
+
     # 'info' function tests: 1
 
     def test_info(self):
-- 
2.12.2


