Resolve module_bay_types by name deterministically, not via blind filter
ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a global type and a manufacturer-scoped type can legally share the same name. The manufacturer-or-null queryset scoping added for ModuleBayTemplateImportForm.module_bay_types left both rows in the filtered queryset in that case, and ModelMultipleChoiceField's default name-based lookup silently attached both instead of just the one referenced -- confirmed by reproducing it directly against the form. Add clean_module_bay_types() to resolve each submitted name explicitly, preferring a manufacturer-specific match over a global one, and raising a clear error for an unresolvable name instead of silently under- or over-matching. Also factor clean_device_type/clean_module_type's duplicated scoping logic into a shared helper.
This commit is contained in:
parent
157a30ecd7
commit
508e2eaba2
|
|
@ -227,24 +227,49 @@ class ModuleBayTemplateImportForm(forms.ModelForm):
|
||||||
'device_type', 'module_type', 'name', 'label', 'position', 'description', 'module_bay_types',
|
'device_type', 'module_type', 'name', 'label', 'position', 'description', 'module_bay_types',
|
||||||
]
|
]
|
||||||
|
|
||||||
|
def _scope_module_bay_types(self, manufacturer):
|
||||||
|
module_bay_types = self.fields['module_bay_types']
|
||||||
|
module_bay_types.queryset = module_bay_types.queryset.filter(
|
||||||
|
Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer)
|
||||||
|
)
|
||||||
|
|
||||||
def clean_device_type(self):
|
def clean_device_type(self):
|
||||||
if device_type := self.cleaned_data['device_type']:
|
if device_type := self.cleaned_data['device_type']:
|
||||||
module_bay_types = self.fields['module_bay_types']
|
self._scope_module_bay_types(device_type.manufacturer)
|
||||||
module_bay_types.queryset = module_bay_types.queryset.filter(
|
|
||||||
Q(manufacturer__isnull=True) | Q(manufacturer=device_type.manufacturer)
|
|
||||||
)
|
|
||||||
|
|
||||||
return device_type
|
return device_type
|
||||||
|
|
||||||
def clean_module_type(self):
|
def clean_module_type(self):
|
||||||
if module_type := self.cleaned_data['module_type']:
|
if module_type := self.cleaned_data['module_type']:
|
||||||
module_bay_types = self.fields['module_bay_types']
|
self._scope_module_bay_types(module_type.manufacturer)
|
||||||
module_bay_types.queryset = module_bay_types.queryset.filter(
|
|
||||||
Q(manufacturer__isnull=True) | Q(manufacturer=module_type.manufacturer)
|
|
||||||
)
|
|
||||||
|
|
||||||
return module_type
|
return module_type
|
||||||
|
|
||||||
|
def clean_module_bay_types(self):
|
||||||
|
"""
|
||||||
|
Resolve each submitted name against the scoped queryset, preferring a manufacturer-
|
||||||
|
specific match over a global one when both exist. ModuleBayType's unique constraint
|
||||||
|
is on (manufacturer, name), not name alone, so a name can legitimately collide between
|
||||||
|
a global type and one scoped to this template's manufacturer; ModelMultipleChoiceField's
|
||||||
|
default name-based lookup would otherwise silently attach both.
|
||||||
|
"""
|
||||||
|
names = self.data.get('module_bay_types') or []
|
||||||
|
if not isinstance(names, (list, tuple)):
|
||||||
|
raise forms.ValidationError(_("Module bay types must be a list."))
|
||||||
|
|
||||||
|
queryset = self.fields['module_bay_types'].queryset
|
||||||
|
resolved = []
|
||||||
|
for name in names:
|
||||||
|
candidates = list(queryset.filter(name=name))
|
||||||
|
if not candidates:
|
||||||
|
raise forms.ValidationError(
|
||||||
|
_("Module bay type not found: {name}").format(name=name)
|
||||||
|
)
|
||||||
|
match = next((c for c in candidates if c.manufacturer_id is not None), candidates[0])
|
||||||
|
resolved.append(match)
|
||||||
|
|
||||||
|
return resolved
|
||||||
|
|
||||||
|
|
||||||
class DeviceBayTemplateImportForm(forms.ModelForm):
|
class DeviceBayTemplateImportForm(forms.ModelForm):
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -229,6 +229,53 @@ class ModuleTypeFormTestCase(TestCase):
|
||||||
self.assertEqual(module_type.attribute_data, {'media': ['copper', 'qsfp28']})
|
self.assertEqual(module_type.attribute_data, {'media': ['copper', 'qsfp28']})
|
||||||
|
|
||||||
|
|
||||||
|
class ModuleBayTemplateImportFormTestCase(TestCase):
|
||||||
|
|
||||||
|
def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self):
|
||||||
|
"""
|
||||||
|
ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a
|
||||||
|
global type and a manufacturer-scoped type can legally share the same name. Referencing
|
||||||
|
that name by import should resolve to the manufacturer-specific match only, not attach
|
||||||
|
both.
|
||||||
|
"""
|
||||||
|
manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
|
||||||
|
global_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-global')
|
||||||
|
scoped_type = ModuleBayType.objects.create(
|
||||||
|
name='SFP28', slug='sfp28-scoped', manufacturer=manufacturer,
|
||||||
|
)
|
||||||
|
device_type = DeviceType.objects.create(
|
||||||
|
manufacturer=manufacturer, model='Device Type 1', slug='device-type-1',
|
||||||
|
)
|
||||||
|
|
||||||
|
form = ModuleBayTemplateImportForm({
|
||||||
|
'device_type': device_type.pk,
|
||||||
|
'name': 'Module Bay 1',
|
||||||
|
'module_bay_types': ['SFP28'],
|
||||||
|
})
|
||||||
|
self.assertTrue(form.is_valid(), form.errors)
|
||||||
|
|
||||||
|
module_bay_template = form.save()
|
||||||
|
self.assertEqual(
|
||||||
|
list(module_bay_template.module_bay_types.all()), [scoped_type],
|
||||||
|
)
|
||||||
|
self.assertNotIn(global_type, module_bay_template.module_bay_types.all())
|
||||||
|
|
||||||
|
def test_module_bay_types_unknown_name_raises_error(self):
|
||||||
|
device_type = DeviceType.objects.create(
|
||||||
|
manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'),
|
||||||
|
model='Device Type 1',
|
||||||
|
slug='device-type-1',
|
||||||
|
)
|
||||||
|
|
||||||
|
form = ModuleBayTemplateImportForm({
|
||||||
|
'device_type': device_type.pk,
|
||||||
|
'name': 'Module Bay 1',
|
||||||
|
'module_bay_types': ['Nonexistent'],
|
||||||
|
})
|
||||||
|
self.assertFalse(form.is_valid())
|
||||||
|
self.assertIn('module_bay_types', form.errors)
|
||||||
|
|
||||||
|
|
||||||
class ModuleFormTestCase(TestCase):
|
class ModuleFormTestCase(TestCase):
|
||||||
|
|
||||||
@classmethod
|
@classmethod
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue