Fix proxmox_user idempotent behavior and add change detection (#156)

* Fix proxmox_user idempotent behavior and add change detection

  - Use individual user API endpoint to retrieve group membership data
  - Add proper change detection to prevent unnecessary updates
  - Simplify helper functions and inline parameter building
  - Fix groups comparison between API list and string formats
  - Remove redundant exit_json calls in main function
  - Add co-author attribution for substantial modifications

* Refactor author information and clean up code formatting in proxmox_user module

* Update copyright information and author list in proxmox_user module

* Add GitHub handle for Kevin Quick in proxmox_user module author list

* Add unit tests for proxmox_user module functionality

* Refactor test_proxmox_user.py for code formatting and cleanup

* Refactor test_proxmox_user.py: reorganize test structure, improve module argument handling, and enhance user update logic tests

Author: kevinquick

plugins/modules/proxmox_user.py

@@ -2,8 +2,10 @@
 # -*- coding: utf-8 -*-
 #
 # Copyright (c) 2025, Jeffrey van Pelt (@Thulium-Drake) <[email protected]>
+# Copyright (c) 2025, Kevin Quick <[email protected]>
 # GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
 # SPDX-FileCopyrightText: (c) 2025, Jeffrey van Pelt (@Thulium-Drake) <[email protected]>
+# SPDX-FileCopyrightText: (c) 2025, Kevin Quick <[email protected]>
 # SPDX-License-Identifier: GPL-3.0-or-later
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
@@ -13,7 +15,9 @@
 short_description: User management for Proxmox VE cluster
 description:
   - Create or delete a user for Proxmox VE clusters.
-author: "Jeffrey van Pelt (@Thulium-Drake) <[email protected]>"
+author:
+  - Jeffrey van Pelt (@Thulium-Drake)
+  - Kevin Quick (@kevinquick)
 version_added: "1.2.0"
 attributes:
   check_mode:
@@ -123,7 +127,8 @@
 """
 
 from ansible.module_utils.basic import AnsibleModule
-from ansible_collections.community.proxmox.plugins.module_utils.proxmox import (proxmox_auth_argument_spec, ProxmoxAnsible)
+from ansible_collections.community.proxmox.plugins.module_utils.proxmox import (
+    proxmox_auth_argument_spec, ProxmoxAnsible)
 
 
 class ProxmoxUserAnsible(ProxmoxAnsible):
@@ -132,18 +137,39 @@ def is_user_existing(self, userid):
         """Check whether user already exist
 
         :param userid: str - name of the user
-        :return: bool - is user exists?
+        :return: dict|bool - user data if exists, False otherwise
         """
         try:
-            users = self.proxmox_api.access.users.get()
-            for user in users:
-                if user['userid'] == userid:
-                    return user
-            return False
+            user_data = self.proxmox_api.access.users(userid).get()
+            return user_data
         except Exception as e:
-            self.module.fail_json(msg="Unable to retrieve users: {0}".format(e))
-
-    def create_update_user(self, userid, comment=None, email=None, enable=True, expire=0, firstname=None, groups=None, password=None, keys=None, lastname=None):
+            if "does not exist" in str(e).lower() or "not found" in str(e).lower():
+                return False
+            else:
+                self.module.fail_json(msg="Unable to retrieve user {0}: {1}".format(userid, e))
+
+    def _user_needs_update(self, existing_user, comment, email, enable, expire, firstname, lastname, groups, keys):
+        """Check if user needs updating by comparing current vs desired state"""
+        # Check standard fields
+        fields = [('comment', comment, ''), ('email', email, ''), ('enable', enable, 1),
+                  ('expire', expire, 0), ('firstname', firstname, ''),
+                  ('lastname', lastname, ''), ('keys', keys, '')]
+
+        for field, new_value, default in fields:
+            if new_value is not None and existing_user.get(field, default) != new_value:
+                return True
+
+        # Check groups (API returns list, we send comma-separated string)
+        if groups is not None:
+            existing_groups_str = ','.join(existing_user.get('groups', []))
+            if existing_groups_str != groups:
+                return True
+
+        return False
+
+    def create_update_user(self, userid, comment=None, email=None, enable=True, expire=0,
+                           firstname=None, groups=None, password=None, keys=None,
+                           lastname=None):
         """Create or update Proxmox VE user
 
         :param userid: str - name of the user
@@ -158,30 +184,33 @@ def create_update_user(self, userid, comment=None, email=None, enable=True, expi
         :param lastname: str, optional - Lastname of the user
         :return: None
         """
-
         # Translate input to make API happy
         enable = int(enable)
-        groups = ','.join(groups)
-
-        if self.is_user_existing(userid):
+        groups = ','.join(groups) if groups else None
+        existing_user = self.is_user_existing(userid)
+        if existing_user:
+            needs_update = self._user_needs_update(existing_user, comment, email, enable, expire,
+                                                   firstname, lastname, groups, keys)
+            if not needs_update and not password:
+                self.module.exit_json(changed=False, userid=userid, msg="User {0} already up to date".format(userid))
             if self.module.check_mode:
-                self.module.exit_json(changed=False, userid=userid, msg="Would update {0} (check mode)".format(userid))
-
-            # Update the user details
-            try:
-                self.proxmox_api.access.users(userid).put(comment=comment,
-                                                          email=email,
-                                                          enable=enable,
-                                                          expire=expire,
-                                                          firstname=firstname,
-                                                          groups=groups,
-                                                          keys=keys,
-                                                          lastname=lastname)
-
-                self.module.exit_json(changed=True, userid=userid, msg="User {0} updated".format(userid))
+                self.module.exit_json(changed=needs_update or bool(password), userid=userid,
+                                      msg="Would update {0} (check mode)".format(userid))
 
-            except Exception as e:
-                self.module.fail_json(changed=False, userid=userid, msg="Failed to update user with ID {0}: {1}".format(userid, e))
+            if needs_update:
+                try:
+                    # Build update parameters - only include non-None values
+                    update_params = {'enable': enable}
+                    for field, value in [('comment', comment), ('email', email), ('expire', expire),
+                                         ('firstname', firstname), ('lastname', lastname),
+                                         ('groups', groups), ('keys', keys)]:
+                        if value is not None:
+                            update_params[field] = value
+                    self.proxmox_api.access.users(userid).put(**update_params)
+                    self.module.exit_json(changed=True, userid=userid, msg="User {0} updated".format(userid))
+                except Exception as e:
+                    self.module.fail_json(changed=False, userid=userid,
+                                          msg="Failed to update user with ID {0}: {1}".format(userid, e))
 
             # We have no way of testing if the user's password needs to be changed
             # so, if it's provided we will update it anyway
@@ -190,10 +219,11 @@ def create_update_user(self, userid, comment=None, email=None, enable=True, expi
                     self.proxmox_api.access.password.put(userid=userid, password=password)
                     self.module.exit_json(changed=True, userid=userid, msg="User {0} updated".format(userid))
                 except Exception as e:
-                    self.module.fail_json(changed=False, userid=userid, msg="Failed to update user password for user ID {0}: {1}".format(userid, e))
+                    self.module.fail_json(changed=False, userid=userid,
+                                          msg="Failed to update user password for user ID {0}: {1}".format(userid, e))
 
         if self.module.check_mode:
-            self.module.exit_json(changed=True, userid=userid, msg="Would update user {0} (check mode)".format(userid))
+            self.module.exit_json(changed=True, userid=userid, msg="Would create user {0} (check mode)".format(userid))
 
         # if the user is new, post it to the API
         try:
@@ -221,7 +251,8 @@ def delete_user(self, userid):
             self.module.exit_json(changed=False, userid=userid, msg="User {0} doesn't exist".format(userid))
 
         if self.module.check_mode:
-            self.module.exit_json(changed=False, userid=userid, msg="Would deleted user with ID {0} (check mode)".format(userid))
+            self.module.exit_json(changed=False, userid=userid,
+                                  msg="Would deleted user with ID {0} (check mode)".format(userid))
 
         try:
             self.proxmox_api.access.users(userid).delete()
@@ -269,12 +300,15 @@ def main():
 
     proxmox = ProxmoxUserAnsible(module)
 
+    # Convert empty strings to None for proper comparison
+    for param in ['comment', 'email', 'firstname', 'lastname', 'keys']:
+        if locals()[param] == "":
+            locals()[param] = None
+
     if state == "present":
         proxmox.create_update_user(userid, comment, email, enable, expire, firstname, groups, password, keys, lastname)
-        module.exit_json(changed=True, userid=userid, msg="User {0} successfully created".format(userid))
     else:
         proxmox.delete_user(userid)
-        module.exit_json(changed=True, userid=userid, msg="User {0} successfully deleted".format(userid))
 
 
 if __name__ == "__main__":

tests/unit/plugins/modules/test_proxmox_user.py

@@ -0,0 +1,170 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (c) 2025, Kevin Quick <[email protected]>
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import sys
+from unittest.mock import MagicMock as MagicMike, patch
+
+import pytest
+from ansible_collections.community.internal_test_tools.tests.unit.plugins.modules.utils import (
+    AnsibleExitJson, AnsibleFailJson, set_module_args, ModuleTestCase)
+
+# Skip tests if proxmoxer is not available
+proxmoxer = pytest.importorskip('proxmoxer')
+
+# Handle different import paths for different test environments
+try:
+    from ansible_collections.community.proxmox.plugins.modules import proxmox_user
+    import ansible_collections.community.proxmox.plugins.module_utils.proxmox as proxmox_utils
+except ImportError:
+    sys.path.insert(0, 'plugins/modules')
+    import proxmox_user
+    sys.path.insert(0, 'plugins/module_utils')
+    import proxmox as proxmox_utils
+
+
+class TestProxmoxUserModule(ModuleTestCase):
+    """Test cases for proxmox_user module using ModuleTestCase pattern."""
+
+    # Common test data
+    BASIC_MODULE_ARGS = {
+        'api_host': 'test.proxmox.com',
+        'api_user': 'root@pam',
+        'api_password': 'secret',
+    }
+
+    SAMPLE_USER = {
+        'userid': 'testuser@pam',
+        'comment': 'Test User',
+        'email': '[email protected]',
+        'enable': 1,
+        'expire': 0,
+        'firstname': 'John',
+        'lastname': 'Doe',
+        'groups': ['admins'],
+        'keys': ''
+    }
+
+    def setUp(self):
+        super(TestProxmoxUserModule, self).setUp()
+        proxmox_utils.HAS_PROXMOXER = True
+        self.module = proxmox_user
+        self.connect_mock = patch("ansible_collections.community.proxmox.plugins.module_utils.proxmox.ProxmoxAnsible._connect")
+        self.connect_mock.start()
+
+    def tearDown(self):
+        self.connect_mock.stop()
+        super(TestProxmoxUserModule, self).tearDown()
+
+    def _create_module_args(self, **kwargs):
+        """Helper to create module arguments with defaults."""
+        args = self.BASIC_MODULE_ARGS.copy()
+        args.update(kwargs)
+        return args
+
+    def test_module_fail_when_required_args_missing(self):
+        """Test module fails with missing required arguments"""
+        with set_module_args({}):
+            with pytest.raises(AnsibleFailJson):
+                proxmox_user.main()
+
+    def test_user_creation_check_mode(self):
+        """Test user creation in check mode"""
+        module_args = self._create_module_args(userid='testuser@pam', comment='Test User', state='present', _ansible_check_mode=True)
+
+        with set_module_args(module_args):
+            with patch.object(proxmox_user.ProxmoxUserAnsible, 'is_user_existing', return_value=False):
+                with pytest.raises(AnsibleExitJson) as exc_info:
+                    proxmox_user.main()
+
+                result = exc_info.value.args[0]
+                assert result['changed'] is True
+                assert 'check mode' in result['msg']
+
+    def test_user_update_no_changes_needed(self):
+        """Test user update when no changes needed"""
+        module_args = self._create_module_args(userid='testuser@pam', comment='Test User', state='present')
+        existing_user = {
+            'userid': 'testuser@pam', 'comment': 'Test User', 'email': '', 'enable': 1, 'expire': 0,
+            'firstname': '', 'lastname': '', 'groups': [], 'keys': ''
+        }
+
+        with set_module_args(module_args):
+            mock_user_exists = patch.object(proxmox_user.ProxmoxUserAnsible, 'is_user_existing', return_value=existing_user)
+            mock_needs_update = patch.object(proxmox_user.ProxmoxUserAnsible, '_user_needs_update', return_value=False)
+
+            with mock_user_exists, mock_needs_update:
+                with pytest.raises(AnsibleExitJson) as exc_info:
+                    proxmox_user.main()
+
+                result = exc_info.value.args[0]
+                assert result['changed'] is False
+                assert 'already up to date' in result['msg']
+
+
+# Tests for internal methods and business logic
+class TestProxmoxUserInternals:
+    """Test internal methods and business logic of ProxmoxUserAnsible class."""
+
+    # Test data for internal method testing
+    SAMPLE_EXISTING_USER = {
+        'userid': 'testuser@pam',
+        'comment': 'Old comment',
+        'email': '[email protected]',
+        'enable': 1,
+        'expire': 0,
+        'firstname': 'John',
+        'lastname': 'Doe',
+        'groups': ['admins'],
+        'keys': ''
+    }
+
+    @pytest.fixture
+    def user_manager(self):
+        """Create a ProxmoxUserAnsible instance for internal testing."""
+        module = MagicMike()
+        module.check_mode = False
+        module.exit_json = MagicMike()
+        module.fail_json = MagicMike()
+
+        with patch.object(proxmox_utils.ProxmoxAnsible, '__init__', return_value=None):
+            manager = proxmox_user.ProxmoxUserAnsible(module)
+            manager.module = module
+            manager.proxmox_api = MagicMike()
+            return manager
+
+    def test_user_needs_update_logic(self, user_manager):
+        """Test the _user_needs_update comparison logic for various scenarios."""
+        existing_user = self.SAMPLE_EXISTING_USER.copy()
+
+        # Test case: No update needed - identical data
+        no_update_needed = user_manager._user_needs_update(
+            existing_user, 'Old comment', '[email protected]', 1, 0, 'John', 'Doe', 'admins', ''
+        )
+        assert no_update_needed is False
+
+        # Test case: Update needed - different comment
+        update_needed = user_manager._user_needs_update(
+            existing_user, 'New comment', '[email protected]', 1, 0, 'John', 'Doe', 'admins', ''
+        )
+        assert update_needed is True
+
+    def test_groups_format_handling(self, user_manager):
+        """Test groups comparison between API format (list) and module input format (string)."""
+        existing_user_with_groups = {'userid': 'testuser@pam', 'groups': ['admins', 'users']}
+
+        # Test case: Same groups in different formats - no update needed
+        same_groups = user_manager._user_needs_update(
+            existing_user_with_groups, None, None, 1, None, None, None, 'admins,users', None
+        )
+        assert same_groups is False
+
+        # Test case: Different groups - update needed
+        different_groups = user_manager._user_needs_update(
+            existing_user_with_groups, None, None, 1, None, None, None, 'admins', None
+        )
+        assert different_groups is True