Add optional Front Image, Rear Image, and Image Count columns to Device Type and Module Type tables#22706
Add optional Front Image, Rear Image, and Image Count columns to Device Type and Module Type tables#22706TallesH13 wants to merge 2 commits into
Conversation
…odule Type tables Closes netbox-community#22030
…de indentação de comentários Co-authored-by: Christiane Giestas <christianegiestas@gmail.com>
|
@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration. |
|
Claude finished @jeremystretch's task in 4m 12s —— View job Review
Three issues worth addressing: 1. Missing translation wrapper in
|
arthanson
left a comment
There was a problem hiding this comment.
Thanks @TallesH13 a couple issues to clean up.
Please add some tests: The PR adds a filter, filter form fields, and table columns with zero test coverage:
- has_images filter tests for both models, mirroring test_has_front_image / test_has_rear_image at netbox/dcim/tests/test_filtersets.py:1520-1529.
- A table-render test exercising image_count when the column is selected (this is the path with the most logic).
Column-visibility detection in get_table() is incomplete? To replicate:
- Add a front and rear image and image attachment to a device type.
- Go to configure table and add these columns. (note the display the checkmarks and count)
- In the configure table dropdown, save this configuration.
- Go to configure table and remove these fields.
- I the Configure table dropdown load your saved configuration.
- Note the columns show but they appear with a dash (no checkmark or count)
Table.configure() resolves visible columns from more sources (netbox/netbox/tables/tables.py:189-200 and mixins.py:54):
- settings.DEFAULT_USER_PREFERENCES
- ?include_columns=image_count (query param)
- ?tableconfig_id=.
In all three paths the image_count column is displayed but the queryset isn't annotated.
| ) | ||
| has_images = django_filters.BooleanFilter( | ||
| method='_has_images', | ||
| label=('Has images'), |
There was a problem hiding this comment.
Need _( internationalization here
| filterset_form = forms.DeviceTypeFilterForm | ||
| table = tables.DeviceTypeTable | ||
|
|
||
| def get_table(self, data, request, bulk_actions=True): |
There was a problem hiding this comment.
This is duplicated below, can we extract this out into a common place, maybe an ImageCountAnnotationMixin to keep it DRY
| *.egg-info/ | ||
|
|
||
| # Ruff's locally generated cache | ||
| .ruff_cache No newline at end of file |
There was a problem hiding this comment.
This is out of scope for the PR, can we remove this
Closes: #22030
What this PR does
Adds three optional columns to the Device Type list view (Front Image, Rear Image and Images as the attached images counter) and one optional column to the Module Type list view (Images column, as the attached images counter)
Changes
front_image,rear_imageandimage_countcolumns to the Device Type table andimage_countfor Module Type tableTesting
Verified manually and via local Cypress tests that:
Screenshots
Co-authored by: Christiane Giestas christianegiestas@gmail.com