Skip to content
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

Fixes #17954 - Handle CircuitTerminations in Cable Bulk Import #17995

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 50 additions & 7 deletions netbox/dcim/forms/bulk_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from dcim.choices import *
from dcim.constants import *
from dcim.models import *
from circuits.models import Circuit
from extras.models import ConfigTemplate
from ipam.models import VRF, IPAddress
from netbox.forms import NetBoxModelImportForm
Expand Down Expand Up @@ -1171,8 +1172,18 @@ class CableImportForm(NetBoxModelImportForm):
label=_('Side A device'),
queryset=Device.objects.all(),
to_field_name='name',
required=False,
conditional=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment re: conditional below

help_text=_('Device name')
)
side_a_circuit = CSVModelChoiceField(
label=_('Side A circuit'),
queryset=Circuit.objects.all(),
to_field_name='cid',
required=False,
conditional=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment re: conditional below

help_text=_('Circuit ID'),
)
side_a_type = CSVContentTypeField(
label=_('Side A type'),
queryset=ContentType.objects.all(),
Expand All @@ -1189,8 +1200,18 @@ class CableImportForm(NetBoxModelImportForm):
label=_('Side B device'),
queryset=Device.objects.all(),
to_field_name='name',
required=False,
conditional=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment re: conditional below

help_text=_('Device name')
)
side_b_circuit = CSVModelChoiceField(
label=_('Side A device'),
queryset=Circuit.objects.all(),
to_field_name='cid',
required=False,
conditional=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment re: conditional below

help_text=_('Circuit ID'),
)
side_b_type = CSVContentTypeField(
label=_('Side B type'),
queryset=ContentType.objects.all(),
Expand Down Expand Up @@ -1232,7 +1253,7 @@ class CableImportForm(NetBoxModelImportForm):
class Meta:
model = Cable
fields = [
'side_a_device', 'side_a_type', 'side_a_name', 'side_b_device', 'side_b_type', 'side_b_name', 'type',
'side_a_device', 'side_a_circuit', 'side_a_type', 'side_a_name', 'side_b_device', 'side_b_circuit', 'side_b_type', 'side_b_name', 'type',
'status', 'tenant', 'label', 'color', 'length', 'length_unit', 'description', 'comments', 'tags',
]

Expand All @@ -1245,18 +1266,40 @@ def _clean_side(self, side):
assert side in 'ab', f"Invalid side designation: {side}"

device = self.cleaned_data.get(f'side_{side}_device')
circuit = self.cleaned_data.get(f'side_{side}_circuit')
content_type = self.cleaned_data.get(f'side_{side}_type')
name = self.cleaned_data.get(f'side_{side}_name')
if not device or not content_type or not name:

if not (device or circuit) or not content_type or not name:
return None

if device and circuit:
raise forms.ValidationError(
_("Side {side_upper}: Both `device` and `circuit` cannot be specified at the same time").format(side_upper=side.upper())
)

model = content_type.model_class()
try:
if device.virtual_chassis and device.virtual_chassis.master == device and \
model.objects.filter(device=device, name=name).count() == 0:
termination_object = model.objects.get(device__in=device.virtual_chassis.members.all(), name=name)
else:
termination_object = model.objects.get(device=device, name=name)

# Should never happen as we return None above if we don't have a device or circuit
assert device or circuit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things we came up to in another PR, is asserts are not a guaranteed thing since it requires python to be compiled with debug=true.

I am actually going to defer to Jeremy on whether we want to stick with asserts or not.


if device:
if device.virtual_chassis and device.virtual_chassis.master == device and \
model.objects.filter(device=device, name=name).count() == 0:
termination_object = model.objects.get(device__in=device.virtual_chassis.members.all(), name=name)
else:
termination_object = model.objects.get(device=device, name=name)
self.fields[f'side_{side}_circuit'].required = False
elif circuit:
termination_object = model.objects.get(circuit=circuit, term_side=name.upper())
if termination_object.provider_network is not None:
raise forms.ValidationError(
_("Side {side_upper}: {circuit} {termination_object} is already connected to a Provider Network").format(
side_upper=side.upper(), circuit=circuit, termination_object=termination_object
)
)

if termination_object.cable is not None and termination_object.cable != self.instance:
raise forms.ValidationError(
_("Side {side_upper}: {device} {termination_object} is already connected").format(
Expand Down
2 changes: 2 additions & 0 deletions netbox/templates/generic/bulk_import.html
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ <h2 class="card-header">{% trans "Field Options" %}</h2>
<td>
{% if field.required %}
{% checkmark True true="Required" %}
{% elif field.conditional %}
<span class="badge bg-yellow text-yellow-fg" >{% trans "Conditional" %}</span>
Comment on lines +132 to +133
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment re: conditional below

{% else %}
{{ ''|placeholder }}
{% endif %}
Expand Down
5 changes: 5 additions & 0 deletions netbox/utilities/forms/fields/csv.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ class CSVModelChoiceField(forms.ModelChoiceField):
'invalid_choice': _('Object not found: %(value)s'),
}

def __init__(self, conditional=False, *args, **kwargs):
# Used to display tags for fields that are conditionally required
self.conditional = conditional
super().__init__(*args, **kwargs)

Comment on lines +60 to +64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not myself prepared to make a decision on whether this is something we should consider introducing.

That said, I think that if this is something we want to persue further, there should be a FR for it and a related PR as I am sure there is more then just this.

For example, it marks it as a conditionally required value but do we want to enrich that further by telling the end-user which fields are part of this conditional group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your input. I did consider enriching the frontend to indicate which fields are part of the conditional group when adding the conditional attribute. However, I felt that this might be out of scope for this particular issue. The main goal here was to provide a way to mark those fields as conditionally required in the frontend. without altering much of the existing functionality.

I'm open to exploring further enhancements to improve user guidance on conditional fields, possibly in a separate feature request if needed.

def to_python(self, value):
try:
return super().to_python(value)
Expand Down
Loading