-
Notifications
You must be signed in to change notification settings - Fork 252
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
Device modules UI #5724
Device modules UI #5724
Conversation
has_empty_module_bays = django_filters.BooleanFilter( | ||
method="filter_has_empty_module_bays", | ||
label="Has empty module bays", | ||
) | ||
module_bays = django_filters.ModelMultipleChoiceFilter( |
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.
Not NaturalKeyOrPKMultipleChoiceFilter
?
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.
Might be appropriate here? I was thinking the module bay "position" wouldn't be suitable for a natural key since in the common case they're going to be integers.
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.
I think that would be appropriate, seems like module_bays=lc0&module_bays=lc1
might be a plausible filter to use?
…e detail views, add module column to component list views, update destroy and bulk destroy warnings, various other fixes
@@ -18,21 +18,21 @@ | |||
{% render_table devicebay_table 'inc/table.html' %} | |||
<div class="panel-footer noprint"> | |||
{% if perms.dcim.change_devicebay %} | |||
<button type="submit" name="_rename" formaction="{% url 'dcim:devicebay_bulk_rename' %}?return_url={{ object.get_absolute_url }}%23tab_devicebays" class="btn btn-warning btn-xs"> | |||
<button type="submit" name="_rename" formaction="{% url 'dcim:devicebay_bulk_rename' %}?return_url={% url 'dcim:device_devicebays' pk=object.pk %}" class="btn btn-warning btn-xs"> |
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.
Unrelated bug fix
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.
Wow! Great stuff.
ModuleBayTemplateFactory.create_batch(30, using=db_name) | ||
ModuleBayTemplateFactory.create_batch(90, using=db_name) |
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.
That's a lot! Not necessarily opposed but curious why so many are needed?
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.
I think I needed this for the ModuleFilterSet.has_modules
test. I know it was one of the filter tests. All of the components are split between modules and devices, so it was around 15 for devices and 15 for modules before. If we only had 15 of these split up between 20 modules there's a chance we wouldn't have enough modules with multiple module bays or at least 2 modules with nested modules.
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.
Might be able to instead call ModuleBayTemplateFactory.create_batch()
several times with distinct parameters as an alternative (create_batch(10, has_parent_device=False)
or whatever). Not critical.
|
||
context = serializer.data | ||
context["use_new_ui"] = True | ||
context = {"use_new_ui": True} |
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.
Could probably yeet this.
@@ -191,4 +191,20 @@ | |||
</div> | |||
{% include 'inc/paginator.html' with paginator=controller_table.paginator page=controller_table.page %} | |||
{% endif %} | |||
{% if module_table %} |
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.
Should we similarly update the location
detail view to include a count of colocated modules?
@@ -97,6 +97,7 @@ table.attr-table td:nth-child(1) { | |||
} | |||
.table-headings th { | |||
background-color: #f5f5f5; | |||
white-space: nowrap; |
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.
This removes wrapping on all table headings throughout nautobot @lampwins
name = factory.LazyAttribute( | ||
lambda o: f"{o.provider.name} Network {faker.Faker().word(part_of_speech='noun')}"[:100] | ||
) | ||
name = factory.LazyAttributeSequence(lambda o, n: f"{o.provider.name} Network {n + 1}"[:100]) |
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.
Fixes random failures
@@ -108,7 +108,9 @@ def test_location(self): | |||
has_location=True, | |||
location=locations[1], | |||
) | |||
expected = self.queryset.filter(circuit_terminations__location__in=[locations[0].pk, locations[1].pk]) | |||
expected = self.queryset.filter( |
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.
Random flaky test failure fixes
@@ -121,7 +121,7 @@ class BulkRenameForm(forms.Form): | |||
""" | |||
|
|||
find = forms.CharField() | |||
replace = forms.CharField() | |||
replace = forms.CharField(required=False, strip=False) |
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.
This allows you to have a blank replacement string, or to have leading and trailing whitespace on your replacement string. Examples:
Interface1
Replace ^Interface
with blank string to change the name to just 1
Replace ^Interface
with Interface
to change the name to Interface 1
# Create random 5 char string to append to attribute, used for icontains partial lookup | ||
lookup = "".join(random.choices(string.ascii_lowercase, k=5)) # noqa: S311 # pseudo-random generator | ||
obj_field_value = getattr(obj, obj_field_name) | ||
# Generic test only supports CharField or TextFields, skip all other types |
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.
This test was randomly failing because 5 random characters wasn't enough to make the field unique enough. Now the test fills the entire field with random lowercase letters and then filters on field__icontains=new_field_value[1:].upper()
or field__iexact=new_field_value.upper()
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.
1 in 26^5 = one in 11881376 odds of non-uniqueness? Sheesh, you did have the worst luck. :-)
""" | ||
|
||
form_data = {} | ||
update_data = {} |
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.
Added a new attr to the edit view generic test for cases where the create
data and edit
data are not interchangeable. Similar but not identical to the api generic tests
@@ -780,6 +785,10 @@ def test_list_objects_anonymous(self): | |||
@override_settings(EXEMPT_VIEW_PERMISSIONS=["*"]) | |||
def test_list_objects_filtered(self): | |||
instance1, instance2 = self._get_queryset().all()[:2] | |||
if hasattr(self.model, "name") and instance1.name == instance2.name: |
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.
Fixing issues with random data generation sometimes creating two instances with the same name
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.
This an issue with models with non-globally-unique names? Might be better to fix in the factories, but not critical.
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.
I think the issue might actually be where we're creating data that looks realistic like Interface 1
- Interface 24
on more than one device.
@@ -1363,6 +1379,7 @@ class BulkRenameObjectsViewTestCase(ModelViewTestCase): | |||
"replace": "\\1X", # Append an X to the original value | |||
"use_regex": True, | |||
} | |||
rename_field = "name" # The model field to be changed |
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.
May be able to remove this when we add the name
field to ModuleBay
nautobot/core/testing/views.py
Outdated
def _edit_object_test_setup(self): | ||
test_instance = self._get_queryset().first() | ||
self.update_data = { | ||
"name": test_instance.name, | ||
"device_type": test_instance.device_type.pk, | ||
"label": "new test label", | ||
"description": "new test description", | ||
} | ||
|
||
@override_settings(EXEMPT_VIEW_PERMISSIONS=["*"]) | ||
def test_edit_object_with_constrained_permission(self): | ||
# Overload this test so that only the label and description fields are changed | ||
self._edit_object_test_setup() | ||
super().test_edit_object_with_constrained_permission() | ||
|
||
@override_settings(EXEMPT_VIEW_PERMISSIONS=["*"]) | ||
def test_edit_object_with_permission(self): | ||
# Overload this test so that only the label and description fields are changed | ||
self._edit_object_test_setup() | ||
super().test_edit_object_with_permission() | ||
|
||
class ModularDeviceComponentTemplateViewTestCase(DeviceComponentTemplateViewTestCase): | ||
def _edit_object_test_setup(self): | ||
test_instance = self._get_queryset().first() | ||
self.update_data = { | ||
"name": test_instance.name, | ||
"device_type": getattr(getattr(test_instance, "device_type", None), "pk", None), | ||
"module_type": getattr(getattr(test_instance, "module_type", None), "pk", None), | ||
"label": "new test label", | ||
"description": "new test description", | ||
} |
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.
With the creation of the component factories, this test was setting statically defined data on randomly generated instances. That doesn't work for cases like FrontPort
where the RearPort
has to belong to the same device/module. We should update this test to work like the API tests where we can supply specific field values to change instead of having to provide the entire set of form data.
factory.LazyAttribute(lambda o: get_random_software_version_for_device_type(o.device_type)), | ||
None, | ||
) | ||
software_version = factory.LazyAttribute(lambda o: get_random_software_version_for_device_type(o.device_type)) |
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.
DeviceType already has a 50% chance of having a software version
# Create random 5 char string to append to attribute, used for icontains partial lookup | ||
lookup = "".join(random.choices(string.ascii_lowercase, k=5)) # noqa: S311 # pseudo-random generator | ||
obj_field_value = getattr(obj, obj_field_name) | ||
# Generic test only supports CharField or TextFields, skip all other types |
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.
1 in 26^5 = one in 11881376 odds of non-uniqueness? Sheesh, you did have the worst luck. :-)
@@ -780,6 +785,10 @@ def test_list_objects_anonymous(self): | |||
@override_settings(EXEMPT_VIEW_PERMISSIONS=["*"]) | |||
def test_list_objects_filtered(self): | |||
instance1, instance2 = self._get_queryset().all()[:2] | |||
if hasattr(self.model, "name") and instance1.name == instance2.name: |
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.
This an issue with models with non-globally-unique names? Might be better to fix in the factories, but not critical.
# add parent object key to related object data | ||
rel_obj_data[obj._meta.verbose_name.replace(" ", "_")] = str(obj.pk) |
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.
Not entirely sure I follow this one, can you clarify the comment?
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.
Previously, the ComponentTemplateImportForm
base class took the parent object as the first argument in __init__
and added it to data
. I changed it so we're updating the form data before we instantiate the form.
nautobot/nautobot/dcim/forms.py
Lines 1403 to 1411 in bc22ef3
def __init__(self, device_type, data=None, *args, **kwargs): | |
# Must pass the parent DeviceType on form initialization | |
data.update( | |
{ | |
"device_type": device_type.pk, | |
} | |
) | |
super().__init__(data, *args, **kwargs) |
class Meta: | ||
# TODO: Ordering by parent_module.id is not correct but prevents an infinite loop | ||
ordering = ( | ||
"parent_device", | ||
"parent_module__id", | ||
"position", | ||
"_position", |
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.
Do we need CollateAsChar
here?
<i class="mdi mdi-plus-thick" aria-hidden="true" title="Install device"></i> | ||
</a> | ||
{% endif %} | ||
{% endif %} | ||
""" | ||
|
||
MODULE_BUTTONS = """ | ||
<a href="{% url 'dcim:module' pk=record.pk %}" class="btn btn-default btn-xs" title="Details"><i class="mdi mdi-information-outline" aria-hidden="true"></i></a> |
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.
do we need perms.dcim.view_module
here?
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.
I don't think these would be rendered if you don't have access to view modules (the table would be empty)
@@ -0,0 +1,5 @@ | |||
{% extends "generic/object_bulk_destroy.html" %} |
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.
nit: can we merge it with module_bulk_destroy.html
?
Closes #2101
What's Changed
Screenshots
List views
Module
ModuleType
ModuleBay
Modular device component list view
Device bulk add component
Module bulk add component
Create/Edit views
Module
ModuleType
Delete/Bulk Delete views
ModuleBay delete
ModuleBay bulk delete
Module delete
Module bulk delete
Detail views
Module
Module ModuleBay view tab
ModuleBay
ModuleType
ModuleType detail tab
ModuleType detail "add components" menu
Device Modules tab
DeviceType ModuleBays tab
DeviceType detail "add components" menu
TODO
position
similar to other componentsname