From 822d0c3748c56c14b54054d09d68182c46f981af Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 7 Apr 2026 10:29:05 -0500 Subject: [PATCH] Sort registered actions and improve test coverage Sort action names alphabetically for stable display order. Add tests for cloning, empty registry, and models_csv output. --- netbox/users/forms/model_forms.py | 8 +++--- netbox/users/models/permissions.py | 9 ++----- netbox/utilities/tests/test_permissions.py | 30 ++++++++++++++++++---- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/netbox/users/forms/model_forms.py b/netbox/users/forms/model_forms.py index 9b72e5063..75ea16e32 100644 --- a/netbox/users/forms/model_forms.py +++ b/netbox/users/forms/model_forms.py @@ -389,14 +389,12 @@ class ObjectPermissionForm(forms.ModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # Build dynamic BooleanFields for registered actions (deduplicated by name) + # Build dynamic BooleanFields for registered actions (deduplicated, sorted by name) seen = set() - registered_action_names = [] for actions in registry['model_actions'].values(): for action in actions: - if action.name not in seen: - registered_action_names.append(action.name) - seen.add(action.name) + seen.add(action.name) + registered_action_names = sorted(seen) action_field_names = [] for action_name in registered_action_names: diff --git a/netbox/users/models/permissions.py b/netbox/users/models/permissions.py index b4c79ba55..4e86ed374 100644 --- a/netbox/users/models/permissions.py +++ b/netbox/users/models/permissions.py @@ -91,19 +91,14 @@ class ObjectPermission(CloningMixin, models.Model): """ enabled_actions = set(self.actions) - set(RESERVED_ACTIONS) - seen = [] - seen_set = set() action_models = {} for model_key, model_actions in registry['model_actions'].items(): for action in model_actions: - if action.name not in seen_set: - seen.append(action.name) - seen_set.add(action.name) action_models.setdefault(action.name, []).append(model_key) return [ - (action, action in enabled_actions, ', '.join(sorted(action_models[action]))) - for action in seen + (name, name in enabled_actions, ', '.join(sorted(action_models[name]))) + for name in sorted(action_models) ] def get_absolute_url(self): diff --git a/netbox/utilities/tests/test_permissions.py b/netbox/utilities/tests/test_permissions.py index 8d003d9f6..1594e8758 100644 --- a/netbox/utilities/tests/test_permissions.py +++ b/netbox/utilities/tests/test_permissions.py @@ -99,7 +99,6 @@ class RegisterModelActionsTest(TestCase): register_model_actions(Site, [ModelAction('')]) def test_no_duplicate_action_fields(self): - """Same action name registered for multiple models should produce only one form field.""" register_model_actions(Device, [ModelAction('render_config')]) register_model_actions(VirtualMachine, [ModelAction('render_config')]) @@ -119,7 +118,6 @@ class ObjectPermissionFormTest(TestCase): registry['model_actions'].update(self._original_actions) def test_shared_action_preselection(self): - """Editing a permission with render_config should pre-select the action_render_config checkbox.""" register_model_actions(Device, [ModelAction('render_config')]) register_model_actions(VirtualMachine, [ModelAction('render_config')]) @@ -136,13 +134,11 @@ class ObjectPermissionFormTest(TestCase): self.assertTrue(form.fields['action_render_config'].initial) - # Should not leak into the additional actions field self.assertEqual(form.initial['actions'], []) permission.delete() def test_clean_accepts_valid_registered_action(self): - """clean() should include checked registered action in saved actions.""" register_model_actions(Device, [ModelAction('render_config')]) device_ct = ObjectType.objects.get_for_model(Device) @@ -158,7 +154,6 @@ class ObjectPermissionFormTest(TestCase): self.assertIn('render_config', form.cleaned_data['actions']) def test_get_registered_actions(self): - """ObjectPermission.get_registered_actions() should return tuples of (name, is_enabled, models).""" register_model_actions(Device, [ModelAction('render_config')]) register_model_actions(VirtualMachine, [ModelAction('render_config')]) @@ -175,5 +170,30 @@ class ObjectPermissionFormTest(TestCase): action_name, is_enabled, models_csv = registered[0] self.assertEqual(action_name, 'render_config') self.assertTrue(is_enabled) + self.assertIn('dcim.device', models_csv) + self.assertIn('virtualization.virtualmachine', models_csv) permission.delete() + + def test_form_with_no_registered_actions(self): + device_ct = ObjectType.objects.get_for_model(Device) + form = ObjectPermissionForm(data={ + 'name': 'test perm', + 'object_types_0': [], + 'object_types_1': [device_ct.pk], + 'can_view': True, + 'actions': '', + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertIn('view', form.cleaned_data['actions']) + action_fields = [k for k in form.fields if k.startswith('action_')] + self.assertEqual(action_fields, []) + + def test_clone_preselects_registered_actions(self): + register_model_actions(Device, [ModelAction('render_config')]) + + form = ObjectPermissionForm(initial={ + 'actions': ['view', 'render_config'], + }) + self.assertTrue(form.fields['action_render_config'].initial) + self.assertNotIn('render_config', form.initial['actions'])