From 24b7751921668f2f933cdfb963ca26fdcd6de65b Mon Sep 17 00:00:00 2001 From: kk Date: Thu, 19 Dec 2024 15:33:12 +0700 Subject: [PATCH] [IMP] hr_attendance_location_ip_check: adopt hook pattern when check attendance ip address for better compatibility and scalability --- .../models/hr_attendance.py | 6 +- .../models/hr_employee.py | 88 +++--- .../tests/test_hr_attendance.py | 250 +++++++++++------- 3 files changed, 198 insertions(+), 146 deletions(-) diff --git a/hr_attendance_location_ip_check/models/hr_attendance.py b/hr_attendance_location_ip_check/models/hr_attendance.py index 66be8009..8aeb6520 100644 --- a/hr_attendance_location_ip_check/models/hr_attendance.py +++ b/hr_attendance_location_ip_check/models/hr_attendance.py @@ -14,7 +14,8 @@ def create(self, vals_list): for vals in vals_list: employee = self.env["hr.employee"].browse(vals.get("employee_id")) if employee.work_location_id: - self._validate_location_ip(employee) + action = "check_in" + employee._attendance_action_check(action) return super().create(vals_list) @@ -24,7 +25,8 @@ def write(self, vals): for attendance in self: if attendance.employee_id.work_location_id: action = "check_out" if "check_out" in vals else "check_in" - self._validate_location_ip(attendance.employee_id, action) + # Use the hook method + attendance.employee_id._attendance_action_check(action) return super().write(vals) def _validate_location_ip(self, employee, action="check_in"): diff --git a/hr_attendance_location_ip_check/models/hr_employee.py b/hr_attendance_location_ip_check/models/hr_employee.py index a4208287..4ba7244d 100644 --- a/hr_attendance_location_ip_check/models/hr_employee.py +++ b/hr_attendance_location_ip_check/models/hr_employee.py @@ -19,42 +19,22 @@ class HrEmployee(models.Model): groups="hr.group_hr_user", ) - @api.model - def fields_get(self, allfields=None, attributes=None): - res = super().fields_get(allfields, attributes) - if not self.env.user.has_group("hr.group_hr_manager"): - if "bypass_ip_check" in res: - res["bypass_ip_check"]["readonly"] = True - return res - - def write(self, vals): - if "bypass_ip_check" in vals and not self.env.user.has_group( - "hr.group_hr_manager" - ): - raise AccessError( - _("Only HR Managers can modify the IP check bypass setting.") - ) - return super().write(vals) - - def attendance_manual(self, next_action, entered_pin=None): - """Validate IP before processing manual attendance.""" + def _attendance_action_check(self, action_type): + """Hook method for attendance IP validation. + Called from hr.attendance during create/write operations. + + Args: + action_type: String indicating 'check_in' or 'check_out' + Returns: + True if validation passes + Raises: + ValidationError if IP check fails + """ self.ensure_one() - action_type = ( - "check_out" if self.attendance_state == "checked_in" else "check_in" - ) - self._validate_ip_address(action_type) - return super().attendance_manual(next_action, entered_pin) - def _get_ip_check_enabled(self): - """Get global IP check setting.""" - return const_eval( - self.env["ir.config_parameter"] - .sudo() - .get_param("hr_attendance.ip_check_enabled", "False") - ) + if not self._is_ip_check_required(): + return True - def _validate_ip_address(self, action_type): - """Validate current IP for attendance actions.""" remote_ip = self._get_remote_ip() if not remote_ip: raise ValidationError( @@ -67,14 +47,22 @@ def _validate_ip_address(self, action_type): _("IP %(ip)s not allowed for %(action)s") % {"ip": remote_ip, "action": action_type} ) + return True + + def _get_ip_check_enabled(self): + """Get global IP check setting.""" + return const_eval( + self.env["ir.config_parameter"] + .sudo() + .get_param("hr_attendance.ip_check_enabled", "False") + ) def _is_ip_check_required(self): - """Determine if IP check is required for this employee""" + """Determine if IP check is required for this employee.""" self.ensure_one() # Check if global IP check is enabled - global_check = self._get_ip_check_enabled() - if not global_check: + if not self._get_ip_check_enabled(): return False # Employee bypass takes precedence @@ -89,12 +77,9 @@ def _is_ip_check_required(self): return self.work_location_id.check_ip def _is_ip_allowed(self, ip_addr): - """Check if IP is allowed for this employee""" - if not self._is_ip_check_required(): - return True - + """Check if IP is allowed for this employee.""" if not ip_addr: - raise ValidationError(_("No IP address detected")) + return False try: ip = ipaddress.ip_address(ip_addr) @@ -135,4 +120,23 @@ def _get_remote_ip(self): return request.httprequest.remote_addr if request else None except Exception as e: _logger.error("Error getting IP: %s", str(e)) - raise ValidationError(_("Unable to determine IP address")) from e + return None + + @api.model + def fields_get(self, allfields=None, attributes=None): + """Restrict bypass_ip_check field modification.""" + res = super().fields_get(allfields, attributes) + if not self.env.user.has_group("hr.group_hr_manager"): + if "bypass_ip_check" in res: + res["bypass_ip_check"]["readonly"] = True + return res + + def write(self, vals): + """Restrict bypass_ip_check modification to HR managers.""" + if "bypass_ip_check" in vals and not self.env.user.has_group( + "hr.group_hr_manager" + ): + raise AccessError( + _("Only HR Managers can modify the IP check bypass setting.") + ) + return super().write(vals) diff --git a/hr_attendance_location_ip_check/tests/test_hr_attendance.py b/hr_attendance_location_ip_check/tests/test_hr_attendance.py index 8d6c25d3..e02cec1b 100644 --- a/hr_attendance_location_ip_check/tests/test_hr_attendance.py +++ b/hr_attendance_location_ip_check/tests/test_hr_attendance.py @@ -37,14 +37,7 @@ def setUpClass(cls): "login": "hr_manager@test.com", "email": "hr_manager@test.com", "groups_id": [ - ( - 6, - 0, - [ - cls.group_hr_manager.id, - cls.group_attendance_manager.id, - ], - ) + (6, 0, [cls.group_hr_manager.id, cls.group_attendance_manager.id]) ], }, } @@ -114,6 +107,16 @@ def setUpClass(cls): "odoo.addons.hr_attendance_location_ip_check.models.hr_employee" ) + def setUp(self): + super().setUp() + # Setup IP patcher that can be used across all tests + self.ip_patcher = patch(f"{self.patch_path}.HrEmployee._get_remote_ip") + self.mock_ip = self.ip_patcher.start() + + def tearDown(self): + self.ip_patcher.stop() + super().tearDown() + def _create_test_attendance(self, employee=None, check_in="2024-01-01 08:00:00"): """Helper method to create test attendance""" return self.env["hr.attendance"].create( @@ -123,12 +126,6 @@ def _create_test_attendance(self, employee=None, check_in="2024-01-01 08:00:00") } ) - def _patch_ip(self, ip_address): - """Helper method to create IP address patch""" - return patch( - f"{self.patch_path}.HrEmployee._get_remote_ip", return_value=ip_address - ) - def test_01_hr_user_access_rights(self): """Test HR User access rights for IP check features""" with self.with_user("hr_user@test.com"): @@ -200,45 +197,47 @@ def test_03_cidr_validation(self): self.assertTrue(cidr.exists()) def test_04_attendance_allowed_ip(self): - """Test attendance creation with allowed IP""" - with self._patch_ip("192.168.1.100"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + """Test attendance with allowed IP using the hook""" + self.mock_ip.return_value = "192.168.1.100" + + # Test the hook directly first + self.employee._attendance_action_check("check_in") + + # Then test full attendance flow + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) def test_05_attendance_blocked_ip(self): - """Test attendance creation with blocked IP""" - with self._patch_ip("10.0.0.1"): - with self.assertRaises(ValidationError): - self._create_test_attendance() + """Test attendance with blocked IP using the hook""" + self.mock_ip.return_value = "10.0.0.1" + + # Test both hook and full attendance flow + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") + + with self.assertRaises(ValidationError): + self._create_test_attendance() def test_06_ip_check_disabled(self): - """Test attendance when IP check is disabled for work location""" - # Disable IP check with manager rights + """Test attendance when IP check is disabled""" with self.with_user("hr_manager@test.com"): self.work_location.check_ip = False - with self._patch_ip("1.2.3.4"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "1.2.3.4" - attendance.write( - { - "check_out": "2024-01-01 17:00:00", - } - ) - check_out_str = "2024-01-01 17:00:00" - self.assertEqual( - attendance.check_out.strftime("%Y-%m-%d %H:%M:%S"), - check_out_str, - ) + # Test the hook + self.employee._attendance_action_check("check_in") - # Re-enable IP check with manager rights - with self.with_user("hr_manager@test.com"): - self.work_location.check_ip = True + # Test full attendance flow + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) - with self._patch_ip("1.2.3.4"): - with self.assertRaises(ValidationError): - self._create_test_attendance() + # Test modification + attendance.write({"check_out": "2024-01-01 17:00:00"}) + self.assertEqual( + attendance.check_out.strftime("%Y-%m-%d %H:%M:%S"), + "2024-01-01 17:00:00", + ) def test_07_multiple_cidrs(self): """Test with multiple CIDR ranges""" @@ -254,9 +253,14 @@ def test_07_multiple_cidrs(self): ) # Test IP from first range - with self._patch_ip("192.168.1.50"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "192.168.1.50" + + # Test hook directly + self.employee._attendance_action_check("check_in") + + # Test full attendance flow + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) def test_08_multi_company(self): """Test CIDR restrictions respect company boundaries""" @@ -301,14 +305,19 @@ def test_08_multi_company(self): ) # Test cross-company IP validation - with self._patch_ip("172.16.0.100"): - # Should work for company 2 employee - attendance = self._create_test_attendance(employee2) - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "172.16.0.100" - # Should fail for company 1 employee - with self.assertRaises(ValidationError): - self._create_test_attendance(self.employee) + # Test hook directly for both employees + employee2._attendance_action_check("check_in") + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") + + # Test full attendance flow + attendance = self._create_test_attendance(employee2) + self.assertTrue(attendance.exists()) + + with self.assertRaises(ValidationError): + self._create_test_attendance(self.employee) def test_09_inactive_cidr(self): """Test IP validation with inactive CIDR""" @@ -319,15 +328,15 @@ def test_09_inactive_cidr(self): ) cidrs.write({"active": False}) - # Should fail with all CIDRs inactive - with self._patch_ip("192.168.1.100"): - with self.assertRaises(ValidationError): - self._create_test_attendance() + self.mock_ip.return_value = "192.168.1.100" - # Should fail with inactive CIDR - with self._patch_ip("192.168.1.100"): - with self.assertRaises(ValidationError): - self._create_test_attendance() + # Test hook directly + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") + + # Test full attendance flow + with self.assertRaises(ValidationError): + self._create_test_attendance() def test_10_cidr_sequence(self): """Test CIDR sequence priorities""" @@ -341,38 +350,42 @@ def test_10_cidr_sequence(self): } ) - # Test IP in the more specific range - with self._patch_ip("172.16.0.50"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "172.16.0.50" + + # Test hook directly + self.employee._attendance_action_check("check_in") + + # Test full attendance flow + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) def test_11_attendance_modification(self): """Test IP validation on attendance modification""" # Create initial attendance with valid IP - with self._patch_ip("192.168.1.100"): - attendance = self._create_test_attendance() + self.mock_ip.return_value = "192.168.1.100" + attendance = self._create_test_attendance() # Test modification with invalid IP - with self._patch_ip("10.0.0.1"): - with self.assertRaises(ValidationError): - attendance.write( - { - "check_out": "2024-01-01 17:00:00", - } - ) - - # Test modification with valid IP - with self._patch_ip("192.168.1.100"): + self.mock_ip.return_value = "10.0.0.1" + with self.assertRaises(ValidationError): attendance.write( { "check_out": "2024-01-01 17:00:00", } ) - check_out_str = "2024-01-01 17:00:00" - self.assertEqual( - attendance.check_out.strftime("%Y-%m-%d %H:%M:%S"), - check_out_str, - ) + + # Test modification with valid IP + self.mock_ip.return_value = "192.168.1.100" + attendance.write( + { + "check_out": "2024-01-01 17:00:00", + } + ) + check_out_str = "2024-01-01 17:00:00" + self.assertEqual( + attendance.check_out.strftime("%Y-%m-%d %H:%M:%S"), + check_out_str, + ) def test_12_ip_edge_cases(self): """Test edge cases in IP validation""" @@ -385,16 +398,18 @@ def test_12_ip_edge_cases(self): ] for ip, expected in edge_cases: - with self._patch_ip(ip): - if expected == "error": - with self.assertRaises(ValidationError): - self._create_test_attendance() - elif expected: - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) - else: - with self.assertRaises(ValidationError): - self._create_test_attendance() + self.mock_ip.return_value = ip + + if expected == "error": + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") + elif expected: + self.employee._attendance_action_check("check_in") + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) + else: + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") def test_13_config_changes(self): """Test impact of configuration changes""" @@ -402,16 +417,20 @@ def test_13_config_changes(self): # Disable IP check globally self.env["ir.config_parameter"].sudo().set_param(param, "False") - with self._patch_ip("1.2.3.4"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "1.2.3.4" + # Should work when disabled + self.employee._attendance_action_check("check_in") + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) # Re-enable IP check self.env["ir.config_parameter"].sudo().set_param(param, "True") - with self._patch_ip("1.2.3.4"): - with self.assertRaises(ValidationError): - self._create_test_attendance() + # Should fail when re-enabled + with self.assertRaises(ValidationError): + self.employee._attendance_action_check("check_in") + with self.assertRaises(ValidationError): + self._create_test_attendance() def test_14_bypass_features(self): """Test bypass feature functionality""" @@ -419,10 +438,14 @@ def test_14_bypass_features(self): # Enable bypass self.employee.bypass_ip_check = True - # Should work with any IP when bypassed - with self._patch_ip("1.2.3.4"): - attendance = self._create_test_attendance() - self.assertTrue(attendance.exists()) + self.mock_ip.return_value = "1.2.3.4" + + # Test hook directly + self.employee._attendance_action_check("check_in") + + # Test full attendance flow + attendance = self._create_test_attendance() + self.assertTrue(attendance.exists()) def test_15_multi_user_scenarios(self): """Test various user scenarios""" @@ -444,3 +467,26 @@ def test_15_multi_user_scenarios(self): } ) self.assertTrue(cidr.exists()) + + def test_16_hook_inheritance(self): + """Test that the attendance action check hook works properly with inheritance""" + self.mock_ip.return_value = "192.168.1.100" + + # First test the hook directly + self.employee._attendance_action_check("check_in") + + # Then verify it works through attendance_manual + result = self.employee.attendance_manual({}) + self.assertTrue(result) # Verify we got a response + + # Verify the attendance data in the result + self.assertIn("action", result) + self.assertIn("attendance", result["action"]) + attendance_data = result["action"]["attendance"] + self.assertEqual(attendance_data["employee_id"][0], self.employee.id) + + # Verify check_out works through the hook too + checkout_result = self.employee.attendance_manual({}) + self.assertTrue(checkout_result) + self.assertIn("action", checkout_result) + self.assertIn("attendance", checkout_result["action"])