[REVIEW] v1.4.0 Code Review #2046
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add status to interface and create interface_status data migration * add status StatusViewSetMixin to InterfaceViewSet * Update docs * Refactor Interface serializer to search for other status if active not found * create and implement versioned_serializer decorator * Update interface test case * Better status search handling in Interface serializer save method and BaseInterface save method * refactor Status get for Interface and VMInterface Serializers * Raise exception if interface status not found Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> Co-authored-by: Jathan McCollum <jathan@gmail.com>
* HTML dark mode toggle * Auto-detect system theme settings. Enable user override * Add new theme modal * Add dark mode support for GraphiQL page * Fix integration tests for theme * Add dark mode on admin page. Fix footer links
* Reject unrecognized filter parameters in UI and API (#1736) * Update example_plugin; improve filterset tests * Add view test coverage for filtering capabilities; fix various view filtering gaps/bugs * Add STRICT_FILTERING settings option * Revised approach using a custom metaclass rather than requiring inheritance from BaseFilterSet.Meta * Apply suggestions from code review Co-authored-by: Jathan McCollum <jathan@gmail.com> * Add a few more magic query parameters * Alternate non-metaclass approach to BaseFilterSet, add testing around STRICT_FILTERING option * Various point fixes * Add release-note for #1736 * Improved test coverage Co-authored-by: Jathan McCollum <jathan@gmail.com>
* Add parent field to Interface - rename @Property parent to parent_object * Add parent field to devices Filter, Forms and Table - Replace parent with parent_object in relevant locations * Replace parent with parent_object in relevant files in templates * Create InterfaceSerializerVersion13 serializer - Apply InterfaceSerializerVersion13 to InterfaceViewSet * Add interface parent and lag filter test case * Extend parent to VM - Add parent field to VMInterface, VMInterfaceForm, VMInterfaceFilterSet, VMInterfaceTable - Modify vm templates accordingly * Implement VMInterfaceSerializer for api_version 1.3 * Add VMinterface migration * Add bridge to field to the dcim.Interfac model * Modify relevant testcase * Add bridge to relevant files - Modify TestCases to account for bridge - Modify interface.html add bridge to table * Account for bridge in versioned interface and vminterface serializers * black format * fix dcim.tests.test_view error * Update nautobot/dcim/forms.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * group lag together with parent and bridge * Remove InterfaceCSV Form __init__ * add bridge to InterfaceTypeChoices * Fix serializer * Make black happy * Merge vminterface migrations and change parent field to parent interface in migrations * Rename all occurrence of parent_objects to parent * Rename all occurrence of parent(not previously parent_object) to parent_interface * Limit choices for parent, bridge, and LAG interfaces to the assigned device on InterfaceCSVForm * Add extra query_params field for bridge in InterfaceCreateForm * Add bridge restriction in Interface.clean() * Update nautobot/virtualization/views.py Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> * rename parent to parent_interface * Additional restriction to InterfaceBulkEditForm * Add query_params type bridge to Interfaceforms to limit choices to only bridge interfaces * Limit InterfaceFilterSet type bridge/lag * Update interface filter test case * Update nautobot/dcim/models/device_components.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/virtualization/templates/virtualization/vminterface.html Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/constants.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/templates/dcim/interface.html Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/tests/test_filters.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * set interface_parent to delete cascade and add extra warning on UI * Remove redundant `display_field="display"` * remove notimplemented parent check on CableTermination * parent property added back to interface * place paren_interface validations together in Interface.clean() method * minor changes: add bridge add_query_params where relevant * Add virtual_chasis query filter to InterfaceCreateForm * remove [] from parent_interface query_params dict * add VC master filter to InterfaceBulkEditForm * remove irrelevant code * Implement vminterface children interface and add table to UI * Fix InterfaceCSVForm test error * Rename Bridge Interface (ID) to bridge interface in InterfaceFilterSet * Update nautobot/dcim/forms.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/filters.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/forms.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * remove bridge restriction to only type bridge * squash migrations * Add child_interfaces_table to interface view * add ability to search for interfaces with devices belonging to same virtual_chassis remove redundant device.virtual_chassis filter in InterfaceCreateForm and InterfaceForm * Update nautobot/dcim/models/device_components.py Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> * rename BaseInterface bridge related name to bridged interface * reorder Interface csv_headers items * Apply suggestions from code review Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Code review refactoring. Sorry not sorry. - `InterfaceFilterSet.device_with_common_vc` was changed from `MultiValueUUIDFilter` to `UUIDFilter` because the underlying method has no valid use-case for multiple device IDs. Further, it was possible to provide `device_id` from two devices NOT in the same VC - `VirtualChassis.member_interfaces` property added to provide a queryset of `Interface` objects common to all VC member devices. `InterfaceFilterSet.device_with_common_vc` now calls this method, improving readability/maintainability - `Interface.clean()` and `VMInterface.clean()` have been unified under `BaseInterface.clean()` and this method has been refactored for optimization of related queries - Added a `VMInterface.site_id` attribute for simplifying "interface" lookups during `clean()` * Reorder csv_headers of parent_interface and bridge * Revert class inheritance ordering changed due to CableTermination * use device_with_common_vc for parent_interface and bridge filtering instead of device_id * Update nautobot/dcim/models/device_components.py Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> * Update nautobot/dcim/models/device_components.py Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> * Update nautobot/dcim/models/device_components.py Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> * Add test case for device_with_common_vc filter * Rename label Bridged Interface to Bridge Interface * Safer parent.virtual_chassis null check for Interfaces * replace .device with .parent since parent property returns self.device * Add test_case for InterfaceCSVForm * remove irrelevant config * revert moving Interface specific clean logic to BaseInterface * add interface API test case testing for vc or device * add available_on_device to vlanfilter set to filter for vlans ti specified device * merge next into dev * add merge * reorder new dcim and virtualization imports * add label to available_on_device * use UUIDFilter instead of ModelChoiceFilter for available_on_device field * add help text * rerun flake8 * Update nautobot/dcim/tests/test_api.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/tests/test_api.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/tests/test_forms.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/tests/test_forms.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * Update nautobot/dcim/tests/test_api.py Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> * exclude non-physical interface from parent filter in InterfaceCSVForm Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com> Co-authored-by: Bryan Culver <bryan.culver@networktocode.com> Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com> Co-authored-by: Jathan McCollum <jathan@gmail.com>
* Users can now toggle device full name and truncated name in the rack elevation view
* Truncating function is customizable in nautobot_config.py via defining UI_RACK_VIEW_TRUNCATE_FUNCTION
* Default behavior is to split on . and return the first item in the list
* "Save SVG" link presents the same view as what is currently displayed on screen
* Current preferred toggle state is preserved across tabs (requires refresh) and persists in-browser until local storage is cleared
* This presents a consistent behavior when browsing between multiple racks
Co-authored-by: Josh VanDeraa <jv@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
* Add parent interface and bridge to interface serializer v14 * refactor interface fields * refactor test
* Marked possible index opportunities so far * Added Meta.indexes and Meta.constraints as discussed * Finished up marking certain field as needs profiling and added new constraint and migration as discussed
bryanculver
force-pushed
the
next-code-review
branch
from
258b7c8
to
f9de4ab
Compare
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
| - DCIM: [#1729](https://github.com/nautobot/nautobot/issues/1729) | ||
| - Virtualization: [#1735](https://github.com/nautobot/nautobot/issues/1735) | ||
|
|
||
| The DCIM, Virtualization FilterSets have been updated with over 150 new filters, including hybrid filters that support filtering on both `pk` and `slug` (or `pk` and `name` where `slug` is not available). A new filter class `NaturalKeyOrPKMultipleChoiceFilter` was added to `nautobot.utilities.filters` to support filtering on multiple fields of a related object. See the [Best Practices](../development/best-practices/#mapping-model-fields-to-filters) documentation for more information. |
There was a problem hiding this comment.
Suggested change
| The DCIM, Virtualization FilterSets have been updated with over 150 new filters, including hybrid filters that support filtering on both `pk` and `slug` (or `pk` and `name` where `slug` is not available). A new filter class `NaturalKeyOrPKMultipleChoiceFilter` was added to `nautobot.utilities.filters` to support filtering on multiple fields of a related object. See the [Best Practices](../development/best-practices/#mapping-model-fields-to-filters) documentation for more information. | |
| The DCIM and Virtualization FilterSets have been updated with over 150 new filters, including hybrid filters that support filtering on both `pk` and `slug` (or `pk` and `name` where `slug` is not available). A new filter class `NaturalKeyOrPKMultipleChoiceFilter` was added to `nautobot.utilities.filters` to support filtering on multiple fields of a related object. See the [Best Practices](../development/best-practices/#mapping-model-fields-to-filters) documentation for more information. |
gsnider2195
reviewed
Aug 10, 2022
| A plugin may now define extra tabs which will be appended to the object view's list of tabs. | ||
|
|
||
| You can refer to the [plugin development guide](../plugins/development.md##adding-extra-tabs) on how to add tabs to existing object detail views. | ||
|
|
There was a problem hiding this comment.
This feature should probably be highlighted in the release overview
Suggested change
| #### Override Core and Plugin Views ([#1466](https://github.com/nautobot/nautobot/issues/1466)) | |
| Plugins can now override any of the core or plugin views by providing an `override_views` `dict` in a plugin's `views.py` file. For detailed information see the [plugin development guide](../plugins/development/#replacing-views) | |
There was a problem hiding this comment.
This isn't a hallmark feature that people should do very often. @lampwins Thoughts on including this in the release overview?
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
gsnider2195
reviewed
Aug 10, 2022
1 task
|
noticed some of the html tags don't have appropriate attribute values. such as alt text, label, ..etc. I would love to see if we use web accessibility checker such as Arc tool or scanner that can be incorporated with the ci/cd pipeline and give report on web accessibility compliant. |
HanlinMiao
reviewed
Aug 12, 2022
examples/example_plugin/example_plugin/templates/example_plugin/tab_circuit_detail.html
Show resolved
Hide resolved
examples/example_plugin/example_plugin/templates/example_plugin/tab_device_detail_1.html
Show resolved
Hide resolved
examples/example_plugin/example_plugin/templates/example_plugin/tab_device_detail_2.html
Show resolved
Hide resolved
| Namely, it: | ||
|
|
||
| - defines the `display` field which exposes a human friendly value for the given object. | ||
| - ensures that `id` field is always present on the serializer as well |
There was a problem hiding this comment.
Suggested change
| - ensures that `id` field is always present on the serializer as well | |
| - ensures that `id` field is always present on the serializer as well. |
Add a period for consistency sake
| @@ -115,6 +150,7 @@ def validate(self, data): | |||
| # Remove custom fields data and tags (if any) prior to model validation | |||
There was a problem hiding this comment.
Suggested change
| # Remove custom fields data and tags (if any) prior to model validation | |
| # Remove custom fields data, relationship data and tags (if any) prior to model validation |
Do we need to add notes as well? Or is that not supported in the API yet?
| @@ -0,0 +1,35 @@ | |||
| import django_filters | |||
|
|
|||
| from nautobot.dcim.models import Region, Site, Location | |||
There was a problem hiding this comment.
Suggested change
| from nautobot.dcim.models import Region, Site, Location | |
| from nautobot.dcim.models import Location, Region, Site |
This was referenced Aug 13, 2022
Merged
|
Feedback accounted for or migrated to new issues. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A LOT of changes have come in 1.4.0 with models, serializers, filters, etc.
Opening this PR as draft to provide a way to do collaborative code review of
nextwithout having to have huge CI churn that exists with merging intonextanddevelop.This is NOT meant to be the final PR, any changes we incorporate here we can merge into
nexton a semi-regular basis.CI is disabled to eliminate churn on every new merge to
next.Please leave comments as you see fit. If you would like to make a change to get folded into
next, either open a separate PR intonextor to this branch.TODO