Skip to content

Fixes #22588: Filter VLANs by Site Group scope when assigning to a prefix#22684

Merged
arthanson merged 11 commits into
netbox-community:mainfrom
ascjreddy:fix/22588-vlan-scope-site-group
Jul 22, 2026
Merged

Fixes #22588: Filter VLANs by Site Group scope when assigning to a prefix#22684
arthanson merged 11 commits into
netbox-community:mainfrom
ascjreddy:fix/22588-vlan-scope-site-group

Conversation

@ascjreddy

Copy link
Copy Markdown
Contributor

Closes: #22588

When a prefix's scope is set to a Site Group, the VLAN dropdown was showing all VLANs regardless of scope. This PR fixes that by:

  1. Adding a get_for_site_group() method to VLANQuerySet that filters VLANs to only those belonging to the VLAN Groups scoped to the specified Site Group (or its ancestors)
  2. Adding an available_at_site_group filter to VLANFilterSet that calls this method
  3. Updating PrefixForm.__init__ to swap the available_at_site query param for available_at_site_group when the prefix scope is a Site Group

@jeremystretch

Copy link
Copy Markdown
Member

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

@jeremystretch
jeremystretch requested review from a team and arthanson and removed request for a team July 15, 2026 12:15
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jeremystretch's task in 2m 21s —— View job


Review

  • Gather context
  • Read changed files
  • Analyze for concerns
  • Post review

Two concerns worth addressing:


1. get_for_site_group() omits global VLANs (netbox/ipam/querysets.py:293–306)

Every other get_for_* method includes globally-scoped VLANs (groups with no scope, and standalone VLANs with no group/site). get_for_site_group() does not:

# get_for_site() — includes global VLANs ✓
return self.filter(
    Q(group__in=VLANGroup.objects.filter(q)) |
    Q(site=site) |
    Q(group__scope_id__isnull=True, site__isnull=True) |  # Global group VLANs
    Q(group__isnull=True, site__isnull=True)              # Global VLANs
)

# get_for_site_group() — omits global VLANs ✗
return self.filter(
    Q(group__in=VLANGroup.objects.filter(q))
)

When a prefix is scoped to a site group, globally available VLANs (those not tied to any specific location scope) would not appear in the assignment dropdown. That seems unintentional. Fix this →


2. Test uses unsaved SiteGroup instance (netbox/ipam/tests/test_forms.py:34)

site_group = SiteGroup(name='Site Group 1', slug='site-group-1')  # pk=None, not saved
form = PrefixForm(data={
    'scope_type': ContentType.objects.get_for_model(SiteGroup).id,
    'scope': site_group,
})

The test works because it only checks widget attributes and never calls is_valid(), but a saved instance with a real pk would be the expected pattern for model form tests. If the scope field ever validates its value during __init__, this would silently fail.

@ascjreddy

Copy link
Copy Markdown
Contributor Author

Hi @arthanson — just wanted to do a friendly follow-up on this PR. The two concerns from the automated review have been addressed (global VLANs added to get_for_site_group() and the test updated to use a saved instance). All checks are passing. Let me know if there's anything else you'd like me to look at!

@arthanson arthanson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ascjreddy I'd request to just add a filterset test for the actual filtering.

The current Test does not exercise get_for_site_group() or the available_at_site_group filter at all. The bug-fixing logic is currently untested.

There's a ready-made pattern to mirror at ipam/tests/test_filtersets.py:2219:

def test_available_at_site(self):
site_id = Site.objects.first().pk
params = {'available_at_site': site_id}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 5) # 4 scoped + 1 global group + 1 global

Please add an analogous test_available_at_site_group that asserts a Site-Group-scoped VLAN is included while an out-of-scope VLAN is excluded

@ascjreddy

Copy link
Copy Markdown
Contributor Author

Hi @arthanson — added test_available_at_site_group mirroring test_available_at_site. All checks passing! Could you check once and let me know if any changes are needed. Thanks!!

@pheus
pheus requested a review from arthanson July 22, 2026 13:06

@arthanson arthanson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ascjreddy !

@arthanson
arthanson merged commit 3e3d36c into netbox-community:main Jul 22, 2026
18 checks passed
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.

Any vlan can be assigned to prefix if Scope Site Group is Selected for prefix

3 participants