Skip to content

Fix #22671 : Enable multi-selection for Site Groups and other scopes in VLAN Groups filter#22708

Open
LujainYasser1 wants to merge 3 commits into
netbox-community:mainfrom
LujainYasser1:fix-vlan-group-site-multiselect-22695
Open

Fix #22671 : Enable multi-selection for Site Groups and other scopes in VLAN Groups filter#22708
LujainYasser1 wants to merge 3 commits into
netbox-community:mainfrom
LujainYasser1:fix-vlan-group-site-multiselect-22695

Conversation

@LujainYasser1

Copy link
Copy Markdown

Closes: #22671

Description

The filter form for VLAN Groups sends arrays of IDs for site groups and other scopes, but VLANGroupFilterSet was implementing them using NumberFilter. This caused the backend to drop multiple selections or fail to process them properly.

I replaced NumberFilter with MultiValueNumberFilter across all scope fields in VLANGroupFilterSet (including site_group, region, site, location, rack_group, rack, cluster_group, and cluster). I also refactored the filter_scope method to use scope_id__in=value so it can cleanly evaluate multiple lookups at once.

Checklist

  • My code follows the code style of this project (self-documenting, no unnecessary comments).
  • Syntax checking passed successfully.

@jeremystretch

Copy link
Copy Markdown
Member

@LujainYasser1 could you please drop a comment on #22671 so the issue can be assigned to you? Thanks.

@jeremystretch
jeremystretch requested review from a team and pheus and removed request for a team July 20, 2026 12:09
@pheus

pheus commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pheus's task in 1m 21s —— View job


Review

  • Read diff and changed files
  • Analyze for issues of concern
  • Post review findings

1. Tests don't verify multi-value filtering

The existing scope filter tests (test_region, test_site_group, test_site, etc. in VLANGroupTestCase, lines 1781–1807) only test single-value filtering:

def test_region(self):
    params = {'region': Region.objects.first().pk}
    self.assertEqual(self.filterset(params, self.queryset).qs.count(), 1)

Since the entire purpose of this PR is to enable multi-value filtering, at minimum each test should also verify that passing a list of IDs returns multiple results (similar to how test_tenant works at line 1811). Without this, the fix could regress undetected.

2. Redundant isinstance guard

MultiValueNumberFilter inherits from django_filters.MultipleChoiceFilter, which always delivers a list to the method handler. The guard added in filter_scope (lines 993–994) is therefore never needed and adds noise:

if not isinstance(value, (list, tuple, set)):
    value = [value]

3. Missing newline at end of file

The diff removes the trailing newline from filtersets.py. This causes a minor style inconsistency that could show up in future diffs.

@pheus pheus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for opening this PR!

Could you please take a look at Claude's review comments and either address the points it raised or reply with a short explanation where you think a comment doesn’t apply? That context would be really helpful for the review.
Finally, please address the tests and lint failures. Thanks!

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.

Selecting multiple Site Groups in the VLAN Groups filter does not display the full output.

3 participants