-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closes: #15239 - Allow adding/removing tagged VLANs in bulk editing of Interfaces #17524
base: develop
Are you sure you want to change the base?
Closes: #15239 - Allow adding/removing tagged VLANs in bulk editing of Interfaces #17524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with this @bctiemann, we just need to figure out a pattern for accommodating these non-model many-to-many fields generically.
# Add/remove tagged VLANs | ||
if form.cleaned_data.get('add_tagged_vlans', None): | ||
obj.tagged_vlans.add(*form.cleaned_data['add_tagged_vlans']) | ||
if form.cleaned_data.get('remove_tagged_vlans', None): | ||
obj.tagged_vlans.remove(*form.cleaned_data['remove_tagged_vlans']) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model-specific fields can't go into a generic view. We'll need to come up with a pattern for handling these generically, probably in a manner similar to how we handle many-to-many assignments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, how about this?
netbox/dcim/views.py
Outdated
# Add/remove tagged VLANs | ||
if form.cleaned_data.get('add_tagged_vlans', None): | ||
obj.tagged_vlans.add(*form.cleaned_data['add_tagged_vlans']) | ||
if form.cleaned_data.get('remove_tagged_vlans', None): | ||
obj.tagged_vlans.remove(*form.cleaned_data['remove_tagged_vlans']) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to validate the interface mode for these interfaces when bulk editing, to prevent #15924 and similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanSheps I would think this change is all we need, right? We don't need to throw an error if the mode is wrong, we can just silently skip interfaces that aren't in the right mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think for this it is a valid approach.
Closes: #15239
Adds "Add Tagged VLANs" and "Remove tagged VLANs" widgets to the bulk edit form of Interfaces, both when editing them standalone and when accessing them as components of a device. These fields operate in the same way as the existing "Add tags"/"Remove tags" and allow the user to add or remove the selected items to all selected objects regardless of their existing VLANs, rather than having to set all objects to the same VLAN settings in the edit operation.
Note that this PR also cleans up the
__init__
function ofInterfaceBulkEditForm
, removing the after-the-fact modification of the widgets to add query parameters, as these params can be set in the field definitions directly using the$device
token.