Address review: tighten test assertions, fix RackTypeImportForm, style consistency
Per code review on the PR: - The POST half of both new regression tests asserted only that the field name appeared in response.data, which the pre-fix "cannot be blank" error also satisfies (same key, different error code). Tightened both to assert the DRF error code is 'required', so the tests actually fail on main. - RackTypeImportForm.form_factor (CSV bulk import) had the same required=False mismatch as the REST API fields this PR fixes, but worse: Django's BaseModelForm._get_validation_exclusions() skips full_clean() validation for a blank=False model field whose form field is required=False and empty, so a CSV row omitting form_factor silently saved a RackType with form_factor='' instead of erroring. Confirmed empirically. Removed required=False; added a regression test, and fixed the existing RackTypeTestCase.csv_data fixture in test_views.py, which was already omitting form_factor and passing only because nothing checked the result. (RackImportForm.form_factor is untouched -- Rack.form_factor is genuinely optional/nullable, unlike RackType's.) - Made RackTypeSerializer.form_factor's required=True explicit, matching L2VPNSerializer.type's style, instead of relying on ChoiceField's implicit default.
This commit is contained in:
parent
74a7e1d262
commit
885797a667
|
|
@ -88,7 +88,7 @@ class RackBaseSerializer(PrimaryModelSerializer):
|
|||
class RackTypeSerializer(RackBaseSerializer):
|
||||
# Unlike Rack.form_factor (optional & nullable), RackType.form_factor is required
|
||||
# (blank=False, no default), so override RackBaseSerializer's optional declaration.
|
||||
form_factor = ChoiceField(choices=RackFormFactorChoices)
|
||||
form_factor = ChoiceField(choices=RackFormFactorChoices, required=True)
|
||||
manufacturer = ManufacturerSerializer(nested=True)
|
||||
rack_count = serializers.IntegerField(read_only=True)
|
||||
|
||||
|
|
|
|||
|
|
@ -225,7 +225,6 @@ class RackTypeImportForm(PrimaryModelImportForm):
|
|||
form_factor = CSVChoiceField(
|
||||
label=_('Type'),
|
||||
choices=RackFormFactorChoices,
|
||||
required=False,
|
||||
help_text=_('Form factor')
|
||||
)
|
||||
starting_unit = forms.IntegerField(
|
||||
|
|
|
|||
|
|
@ -1226,7 +1226,7 @@ class RackTypeTestCase(APIViewTestCases.APIViewTestCase):
|
|||
}
|
||||
response = self.client.post(self._get_list_url(), data, format='json', **self.header)
|
||||
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertIn('form_factor', response.data)
|
||||
self.assertEqual(response.data['form_factor'][0].code, 'required')
|
||||
|
||||
|
||||
class RackTestCase(APIViewTestCases.APIViewTestCase):
|
||||
|
|
|
|||
|
|
@ -503,10 +503,10 @@ class RackTypeTestCase(ViewTestCases.PrimaryObjectViewTestCase):
|
|||
}
|
||||
|
||||
cls.csv_data = (
|
||||
"manufacturer,model,slug,width,u_height,weight,max_weight,weight_unit",
|
||||
"Manufacturer 1,RackType 4,rack-type-4,19,42,100,2000,kg",
|
||||
"Manufacturer 1,RackType 5,rack-type-5,19,42,100,2000,kg",
|
||||
"Manufacturer 1,RackType 6,rack-type-6,19,42,100,2000,kg",
|
||||
"manufacturer,model,slug,form_factor,width,u_height,weight,max_weight,weight_unit",
|
||||
f"Manufacturer 1,RackType 4,rack-type-4,{RackFormFactorChoices.TYPE_CABINET},19,42,100,2000,kg",
|
||||
f"Manufacturer 1,RackType 5,rack-type-5,{RackFormFactorChoices.TYPE_CABINET},19,42,100,2000,kg",
|
||||
f"Manufacturer 1,RackType 6,rack-type-6,{RackFormFactorChoices.TYPE_CABINET},19,42,100,2000,kg",
|
||||
)
|
||||
|
||||
cls.csv_update_data = (
|
||||
|
|
@ -531,6 +531,30 @@ class RackTypeTestCase(ViewTestCases.PrimaryObjectViewTestCase):
|
|||
'comments': 'New comments',
|
||||
}
|
||||
|
||||
def test_bulk_import_objects_without_form_factor(self):
|
||||
"""
|
||||
A CSV import row omitting form_factor must be rejected, not silently saved
|
||||
with form_factor=''.
|
||||
"""
|
||||
obj_perm = ObjectPermission(name='Test permission', actions=['add'])
|
||||
obj_perm.save()
|
||||
obj_perm.users.add(self.user)
|
||||
obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
|
||||
|
||||
initial_count = self._get_queryset().count()
|
||||
csv_data = (
|
||||
"manufacturer,model,slug,width,u_height,weight,max_weight,weight_unit",
|
||||
"Manufacturer 1,RackType Missing Form Factor,rack-type-missing-form-factor,19,42,100,2000,kg",
|
||||
)
|
||||
data = {
|
||||
'data': '\n'.join(csv_data),
|
||||
'format': ImportFormatChoices.CSV,
|
||||
'csv_delimiter': CSVDelimiterChoices.AUTO,
|
||||
}
|
||||
response = self.client.post(self._get_url('bulk_import'), data)
|
||||
self.assertHttpStatus(response, 200)
|
||||
self.assertEqual(self._get_queryset().count(), initial_count)
|
||||
|
||||
|
||||
class RackTestCase(ViewTestCases.PrimaryObjectViewTestCase):
|
||||
model = Rack
|
||||
|
|
|
|||
|
|
@ -619,7 +619,7 @@ class L2VPNTestCase(APIViewTestCases.APIViewTestCase):
|
|||
}
|
||||
response = self.client.post(self._get_list_url(), data, format='json', **self.header)
|
||||
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
|
||||
self.assertIn('type', response.data)
|
||||
self.assertEqual(response.data['type'][0].code, 'required')
|
||||
|
||||
|
||||
class L2VPNTerminationTestCase(APIViewTestCases.APIViewTestCase):
|
||||
|
|
|
|||
Loading…
Reference in New Issue