Skip to content

Commit 17e5184

Browse files
authored
Fixes #20759: Group object types by app in permission form (#20931)
* Fixes #20759: Group object types by app in permission form Modified the ObjectPermissionForm to use optgroups for organizing object types by application. This shortens the display names (e.g., "permission" instead of "Authentication and Authorization | permission") while maintaining clear organization through visual grouping. Changes: - Updated get_object_types_choices() to return nested optgroup structure - Enhanced AvailableOptions and SelectedOptions widgets to handle optgroups - Modified TypeScript moveOptions to preserve optgroup structure - Added hover text showing full model names - Styled optgroups with bold, padded labels * Address PR feedback
1 parent e1548bb commit 17e5184

File tree

8 files changed

+200
-36
lines changed

8 files changed

+200
-36
lines changed

netbox/project-static/dist/netbox.css

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

netbox/project-static/dist/netbox.js

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

netbox/project-static/dist/netbox.js.map

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

netbox/project-static/src/buttons/moveOptions.ts

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,50 @@
11
import { getElements } from '../util';
22

33
/**
4-
* Move selected options from one select element to another.
4+
* Move selected options from one select element to another, preserving optgroup structure.
55
*
66
* @param source Select Element
77
* @param target Select Element
88
*/
99
function moveOption(source: HTMLSelectElement, target: HTMLSelectElement): void {
1010
for (const option of Array.from(source.options)) {
1111
if (option.selected) {
12-
target.appendChild(option.cloneNode(true));
12+
// Check if option is inside an optgroup
13+
const parentOptgroup = option.parentElement as HTMLElement;
14+
15+
if (parentOptgroup.tagName === 'OPTGROUP') {
16+
// Find or create matching optgroup in target
17+
const groupLabel = parentOptgroup.getAttribute('label');
18+
let targetOptgroup = Array.from(target.children).find(
19+
child => child.tagName === 'OPTGROUP' && child.getAttribute('label') === groupLabel,
20+
) as HTMLOptGroupElement;
21+
22+
if (!targetOptgroup) {
23+
// Create new optgroup in target
24+
targetOptgroup = document.createElement('optgroup');
25+
targetOptgroup.setAttribute('label', groupLabel!);
26+
target.appendChild(targetOptgroup);
27+
}
28+
29+
// Move option to target optgroup
30+
targetOptgroup.appendChild(option.cloneNode(true));
31+
} else {
32+
// Option is not in an optgroup, append directly
33+
target.appendChild(option.cloneNode(true));
34+
}
35+
1336
option.remove();
37+
38+
// Clean up empty optgroups in source
39+
if (parentOptgroup.tagName === 'OPTGROUP' && parentOptgroup.children.length === 0) {
40+
parentOptgroup.remove();
41+
}
1442
}
1543
}
1644
}
1745

1846
/**
19-
* Move selected options of a select element up in order.
47+
* Move selected options of a select element up in order, respecting optgroup boundaries.
2048
*
2149
* Adapted from:
2250
* @see https://www.tomred.net/css-html-js/reorder-option-elements-of-an-html-select.html
@@ -27,14 +55,21 @@ function moveOptionUp(element: HTMLSelectElement): void {
2755
for (let i = 1; i < options.length; i++) {
2856
const option = options[i];
2957
if (option.selected) {
30-
element.removeChild(option);
31-
element.insertBefore(option, element.options[i - 1]);
58+
const parent = option.parentElement as HTMLElement;
59+
const previousOption = element.options[i - 1];
60+
const previousParent = previousOption.parentElement as HTMLElement;
61+
62+
// Only move if previous option is in the same parent (optgroup or select)
63+
if (parent === previousParent) {
64+
parent.removeChild(option);
65+
parent.insertBefore(option, previousOption);
66+
}
3267
}
3368
}
3469
}
3570

3671
/**
37-
* Move selected options of a select element down in order.
72+
* Move selected options of a select element down in order, respecting optgroup boundaries.
3873
*
3974
* Adapted from:
4075
* @see https://www.tomred.net/css-html-js/reorder-option-elements-of-an-html-select.html
@@ -43,12 +78,18 @@ function moveOptionUp(element: HTMLSelectElement): void {
4378
function moveOptionDown(element: HTMLSelectElement): void {
4479
const options = Array.from(element.options);
4580
for (let i = options.length - 2; i >= 0; i--) {
46-
let option = options[i];
81+
const option = options[i];
4782
if (option.selected) {
48-
let next = element.options[i + 1];
49-
option = element.removeChild(option);
50-
next = element.replaceChild(option, next);
51-
element.insertBefore(next, option);
83+
const parent = option.parentElement as HTMLElement;
84+
const nextOption = element.options[i + 1];
85+
const nextParent = nextOption.parentElement as HTMLElement;
86+
87+
// Only move if next option is in the same parent (optgroup or select)
88+
if (parent === nextParent) {
89+
const optionClone = parent.removeChild(option);
90+
const nextClone = parent.replaceChild(optionClone, nextOption);
91+
parent.insertBefore(nextClone, optionClone);
92+
}
5293
}
5394
}
5495
}

netbox/project-static/styles/transitional/_forms.scss

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,17 @@ form.object-edit {
3232
border: 1px solid $red;
3333
}
3434
}
35+
36+
// Make optgroup labels sticky when scrolling through select elements
37+
select[multiple] {
38+
optgroup {
39+
position: sticky;
40+
top: 0;
41+
background-color: var(--bs-body-bg);
42+
font-style: normal;
43+
font-weight: bold;
44+
}
45+
option {
46+
padding-left: 0.5rem;
47+
}
48+
}

netbox/users/forms/model_forms.py

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import json
2+
from collections import defaultdict
23

34
from django import forms
5+
from django.apps import apps
46
from django.conf import settings
57
from django.contrib.auth import password_validation
68
from django.contrib.postgres.forms import SimpleArrayField
@@ -21,6 +23,7 @@
2123
DynamicModelMultipleChoiceField,
2224
JSONField,
2325
)
26+
from utilities.string import title
2427
from utilities.forms.rendering import FieldSet
2528
from utilities.forms.widgets import DateTimePicker, SplitMultiSelectWidget
2629
from utilities.permissions import qs_filter_from_constraints
@@ -283,10 +286,24 @@ def save(self, *args, **kwargs):
283286

284287

285288
def get_object_types_choices():
286-
return [
287-
(ot.pk, str(ot))
288-
for ot in ObjectType.objects.filter(OBJECTPERMISSION_OBJECT_TYPES).order_by('app_label', 'model')
289-
]
289+
"""
290+
Generate choices for object types grouped by app label using optgroups.
291+
Returns nested structure: [(app_label, [(id, model_name), ...]), ...]
292+
"""
293+
app_label_map = {
294+
app_config.label: app_config.verbose_name
295+
for app_config in apps.get_app_configs()
296+
}
297+
choices_by_app = defaultdict(list)
298+
299+
for ot in ObjectType.objects.filter(OBJECTPERMISSION_OBJECT_TYPES).order_by('app_label', 'model'):
300+
app_label = app_label_map.get(ot.app_label, ot.app_label)
301+
302+
model_class = ot.model_class()
303+
model_name = model_class._meta.verbose_name if model_class else ot.model
304+
choices_by_app[app_label].append((ot.pk, title(model_name)))
305+
306+
return list(choices_by_app.items())
290307

291308

292309
class ObjectPermissionForm(forms.ModelForm):

netbox/utilities/forms/widgets/select.py

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,46 @@ class SelectWithPK(forms.Select):
6666
option_template_name = 'widgets/select_option_with_pk.html'
6767

6868

69-
class AvailableOptions(forms.SelectMultiple):
69+
class SelectMultipleBase(forms.SelectMultiple):
7070
"""
71-
Renders a <select multiple=true> including only choices that have been selected. (For unbound fields, this list
72-
will be empty.) Employed by SplitMultiSelectWidget.
71+
Base class for select widgets that filter choices based on selected values.
72+
Subclasses should set `include_selected` to control filtering behavior.
7373
"""
74+
include_selected = False
75+
7476
def optgroups(self, name, value, attrs=None):
75-
self.choices = [
76-
choice for choice in self.choices if str(choice[0]) not in value
77-
]
77+
filtered_choices = []
78+
include_selected = self.include_selected
79+
80+
for choice in self.choices:
81+
if isinstance(choice[1], (list, tuple)): # optgroup
82+
group_label, group_choices = choice
83+
filtered_group = [
84+
c for c in group_choices if (str(c[0]) in value) == include_selected
85+
]
86+
87+
if filtered_group: # Only include optgroup if it has choices left
88+
filtered_choices.append((group_label, filtered_group))
89+
else: # option, e.g. flat choice
90+
if (str(choice[0]) in value) == include_selected:
91+
filtered_choices.append(choice)
92+
93+
self.choices = filtered_choices
7894
value = [] # Clear selected choices
7995
return super().optgroups(name, value, attrs)
8096

97+
def create_option(self, name, value, label, selected, index, subindex=None, attrs=None):
98+
option = super().create_option(name, value, label, selected, index, subindex, attrs)
99+
option['attrs']['title'] = label # Add title attribute to show full text on hover
100+
return option
101+
102+
103+
class AvailableOptions(SelectMultipleBase):
104+
"""
105+
Renders a <select multiple=true> including only choices that have been selected. (For unbound fields, this list
106+
will be empty.) Employed by SplitMultiSelectWidget.
107+
"""
108+
81109
def get_context(self, name, value, attrs):
82110
context = super().get_context(name, value, attrs)
83111

@@ -87,17 +115,12 @@ def get_context(self, name, value, attrs):
87115
return context
88116

89117

90-
class SelectedOptions(forms.SelectMultiple):
118+
class SelectedOptions(SelectMultipleBase):
91119
"""
92120
Renders a <select multiple=true> including only choices that have _not_ been selected. (For unbound fields, this
93121
will include _all_ choices.) Employed by SplitMultiSelectWidget.
94122
"""
95-
def optgroups(self, name, value, attrs=None):
96-
self.choices = [
97-
choice for choice in self.choices if str(choice[0]) in value
98-
]
99-
value = [] # Clear selected choices
100-
return super().optgroups(name, value, attrs)
123+
include_selected = True
101124

102125

103126
class SplitMultiSelectWidget(forms.MultiWidget):

netbox/utilities/tests/test_forms.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
from utilities.forms.fields.csv import CSVSelectWidget
88
from utilities.forms.forms import BulkRenameForm
99
from utilities.forms.utils import get_field_value, expand_alphanumeric_pattern, expand_ipaddress_pattern
10+
from utilities.forms.widgets.select import AvailableOptions, SelectedOptions
1011

1112

1213
class ExpandIPAddress(TestCase):
@@ -481,3 +482,71 @@ def test_valid_value_not_omitted(self):
481482
widget = CSVSelectWidget()
482483
data = {'test_field': 'valid_value'}
483484
self.assertFalse(widget.value_omitted_from_data(data, {}, 'test_field'))
485+
486+
487+
class SelectMultipleWidgetTest(TestCase):
488+
"""
489+
Validate filtering behavior of AvailableOptions and SelectedOptions widgets.
490+
"""
491+
492+
def test_available_options_flat_choices(self):
493+
"""AvailableOptions should exclude selected values from flat choices"""
494+
widget = AvailableOptions(choices=[
495+
(1, 'Option 1'),
496+
(2, 'Option 2'),
497+
(3, 'Option 3'),
498+
])
499+
widget.optgroups('test', ['2'], None)
500+
501+
self.assertEqual(len(widget.choices), 2)
502+
self.assertEqual(widget.choices[0], (1, 'Option 1'))
503+
self.assertEqual(widget.choices[1], (3, 'Option 3'))
504+
505+
def test_available_options_optgroups(self):
506+
"""AvailableOptions should exclude selected values from optgroups"""
507+
widget = AvailableOptions(choices=[
508+
('Group A', [(1, 'Option 1'), (2, 'Option 2')]),
509+
('Group B', [(3, 'Option 3'), (4, 'Option 4')]),
510+
])
511+
512+
# Select options 2 and 3
513+
widget.optgroups('test', ['2', '3'], None)
514+
515+
# Should have 2 groups with filtered choices
516+
self.assertEqual(len(widget.choices), 2)
517+
self.assertEqual(widget.choices[0][0], 'Group A')
518+
self.assertEqual(widget.choices[0][1], [(1, 'Option 1')])
519+
self.assertEqual(widget.choices[1][0], 'Group B')
520+
self.assertEqual(widget.choices[1][1], [(4, 'Option 4')])
521+
522+
def test_selected_options_flat_choices(self):
523+
"""SelectedOptions should include only selected values from flat choices"""
524+
widget = SelectedOptions(choices=[
525+
(1, 'Option 1'),
526+
(2, 'Option 2'),
527+
(3, 'Option 3'),
528+
])
529+
530+
# Select option 2
531+
widget.optgroups('test', ['2'], None)
532+
533+
# Should only have option 2
534+
self.assertEqual(len(widget.choices), 1)
535+
self.assertEqual(widget.choices[0], (2, 'Option 2'))
536+
537+
def test_selected_options_optgroups(self):
538+
"""SelectedOptions should include only selected values from optgroups"""
539+
widget = SelectedOptions(choices=[
540+
('Group A', [(1, 'Option 1'), (2, 'Option 2')]),
541+
('Group B', [(3, 'Option 3'), (4, 'Option 4')]),
542+
])
543+
544+
# Select options 2 and 3
545+
widget.optgroups('test', ['2', '3'], None)
546+
547+
# Should have 2 groups with only selected choices
548+
self.assertEqual(len(widget.choices), 2)
549+
self.assertEqual(widget.choices[0][0], 'Group A')
550+
self.assertEqual(widget.choices[0][1], [(2, 'Option 2')])
551+
self.assertEqual(widget.choices[1][0], 'Group B')
552+
self.assertEqual(widget.choices[1][1], [(3, 'Option 3')])

0 commit comments

Comments
 (0)