From 9e909a926737cba165f1ba1b054b57f6a4747e67 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 21 May 2026 05:44:57 -0400 Subject: [PATCH] Address review issues 1 and 3: no-field guard and remove new_name shim - Add form error when all field checkboxes are unchecked on submit; previously fell back to renaming every declared field silently - Remove obj.new_name backward-compat assignment; no template or documented plugin API references it (all use obj.new_names now) - Update base test data to include field_names=['name'] so the guard does not fire in views-framework tests that don't specify fields Co-Authored-By: Claude Sonnet 4.6 --- netbox/netbox/views/generic/bulk_views.py | 69 +++++++++++------------ netbox/utilities/testing/views.py | 3 + 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/netbox/netbox/views/generic/bulk_views.py b/netbox/netbox/views/generic/bulk_views.py index 975212c57..4512a789a 100644 --- a/netbox/netbox/views/generic/bulk_views.py +++ b/netbox/netbox/views/generic/bulk_views.py @@ -936,8 +936,6 @@ class BulkRenameView(GetReturnURLMixin, BaseMultiObjectView): new_values[field] = current.replace(find, replace) obj.new_names = SimpleNamespace(**new_values) - # Backward compat: single-field callers still reference obj.new_name - obj.new_name = new_values.get(field_names[0], '') obj.has_changes = any( new_values[f] != (getattr(obj, f, '') or '') for f in field_names ) @@ -962,55 +960,56 @@ class BulkRenameView(GetReturnURLMixin, BaseMultiObjectView): form = self.form(request.POST, initial={'pk': pk_list}) if form.is_valid(): - # field_names checkboxes are rendered manually in the template and - # submitted directly, not via a form field, to avoid widget styling issues. submitted = [ f for f in request.POST.getlist('field_names') if f in self.rename_fields ] - if submitted: + if self.rename_fields and not submitted: + form.add_error(None, _("Select at least one field to rename.")) + elif submitted: field_names = submitted - try: - with transaction.atomic(using=router.db_for_write(self.queryset.model)): - renamed_pks = self._rename_objects(form, selected_objects, field_names) + if not form.errors: + try: + with transaction.atomic(using=router.db_for_write(self.queryset.model)): + renamed_pks = self._rename_objects(form, selected_objects, field_names) - if '_apply' in request.POST: - # For MPTT models, delay tree updates until all saves are complete - if issubclass(self.queryset.model, MPTTModel): - with self.queryset.model.objects.delay_mptt_updates(): + if '_apply' in request.POST: + # For MPTT models, delay tree updates until all saves are complete + if issubclass(self.queryset.model, MPTTModel): + with self.queryset.model.objects.delay_mptt_updates(): + for obj in selected_objects: + for field in field_names: + setattr(obj, field, getattr(obj.new_names, field)) + obj._changelog_message = form.cleaned_data.get('changelog_message', '') + obj.save() + else: for obj in selected_objects: for field in field_names: setattr(obj, field, getattr(obj.new_names, field)) obj._changelog_message = form.cleaned_data.get('changelog_message', '') obj.save() - else: - for obj in selected_objects: - for field in field_names: - setattr(obj, field, getattr(obj.new_names, field)) - obj._changelog_message = form.cleaned_data.get('changelog_message', '') - obj.save() - # Enforce constrained permissions - if self.queryset.filter(pk__in=renamed_pks).count() != len(selected_objects): - raise PermissionsViolation + # Enforce constrained permissions + if self.queryset.filter(pk__in=renamed_pks).count() != len(selected_objects): + raise PermissionsViolation - messages.success( - request, - _("Renamed {count} {object_type}").format( - count=len(selected_objects), - object_type=self.queryset.model._meta.verbose_name_plural + messages.success( + request, + _("Renamed {count} {object_type}").format( + count=len(selected_objects), + object_type=self.queryset.model._meta.verbose_name_plural + ) ) - ) - return redirect(self.get_return_url(request)) + return redirect(self.get_return_url(request)) - except IntegrityError as e: - messages.error(self.request, ", ".join(e.args)) - clear_events.send(sender=self) + except IntegrityError as e: + messages.error(self.request, ", ".join(e.args)) + clear_events.send(sender=self) - except (AbortRequest, PermissionsViolation) as e: - logger.debug(e.message) - form.add_error(None, e.message) - clear_events.send(sender=self) + except (AbortRequest, PermissionsViolation) as e: + logger.debug(e.message) + form.add_error(None, e.message) + clear_events.send(sender=self) else: form = self.form(initial={'pk': pk_list}) diff --git a/netbox/utilities/testing/views.py b/netbox/utilities/testing/views.py index 9d1296b5e..f0aa24598 100644 --- a/netbox/utilities/testing/views.py +++ b/netbox/utilities/testing/views.py @@ -1032,6 +1032,7 @@ class ViewTestCases: data = { 'pk': pk_list, '_apply': True, # Form button + 'field_names': ['name'], } data.update(self.rename_data) @@ -1059,6 +1060,7 @@ class ViewTestCases: 'pk': pk_list, '_apply': True, 'changelog_message': 'Bulk rename test message', + 'field_names': ['name'], } data.update(self.rename_data) @@ -1091,6 +1093,7 @@ class ViewTestCases: data = { 'pk': pk_list, '_apply': True, # Form button + 'field_names': ['name'], } data.update(self.rename_data)