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 16536 - Fix filtering of device component by device role #16553

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Alef-Burzmali
Copy link
Contributor

@Alef-Burzmali Alef-Burzmali commented Jun 13, 2024

Fixes: #16536

Fix the filtering of device components by device role by aligning the name of the fields from the DeviceComponentFilterForm and the DeviceComponentFilterSet.

I saw two solutions to implement the fix:

  1. Rename the fields in the DeviceComponentFilterSet to device_role_id and device_role (that's the one I've implemented)
  2. Rename the field in the DeviceComponentFilterForm to role_id but create an exception for InventoryItemFilterSet and InventoryItemFilterForm, which already have a field named role for the InventoryItemRole

I've chosen solution 1 because:

  • We are not filtering according to the component's role but according to its parent device's role, so device_role makes more sense as role could be confused with the role of the component.
  • It avoids creating an exception for InventoryItemFilterSet, which would have its role field override the role field from DeviceComponentFilterSet, and would have needed a device_role field or equivalent to preserve the feature. The role filter of DeviceComponent would have a different meaning between an Interface or a InventoryItem, which would have been more confusing.
  • While the release note for 4.0 clearly states in breaking changes that the device_role field of devices has been removed after a depreciation period, there are no mentions of DeviceComponent in the notes or in Replaces device_role with role on device model #13342, so it may have been an undocumented or unintended change
  • There is no equivalent filtering on VMInterface, it's not possible to filter them by their VM's role at the moment, so the reason behind the renaming of "device_role" to "role" is not currently present for DeviceComponent. However, if this feature is added, a more neutral field name such as "parent_role" or "parent_object_role" could be used instead.
  • It's the least amount of changes required for this fix. Solution 2 would have required modifying InventoryItemFilterSet, InventoryItemFilterForm, DeviceComponentFilterForm and the fieldsets of all inheriting FilterForms.

However, solution 1 also changes the API to filter the device components by roles. This was an undocumented change in 4.0, so maybe that's acceptable.

Please let me know if you agree with that approach or if you prefer another solution.

Rename role and role_id fields to device_role and device_role_id in
DeviceComponentFilterSet
* Use device_role filter name for DeviceComponentFilterSetTests

* Add test_device_role test in InventoryItemTestCase
@Alef-Burzmali
Copy link
Contributor Author

Hello. Please let me know if that approach suits you or if you want another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filtering device components per device role fails
1 participant