Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

11432 device field #11567

Merged
merged 16 commits into from
Apr 12, 2023
Merged

11432 device field #11567

merged 16 commits into from
Apr 12, 2023

Conversation

arthanson
Copy link
Collaborator

Fixes: #11432

Makes the device field for interface read-only if a patch request. The 'normal' way of doing this would be by override get_extra_kwargs, but that doesn't work if the field is explicitly declared (which this is). As read-only it is just ignored if the patch is done which is the DRF convention.

@jeremystretch
Copy link
Member

This may be better addressed in the model itself. I know the bug was opened specifically for the API, however it's something we should prevent via other means as well. Once an interface has been assigned a PK, its assigned device should not be allowed to change.

@kkthxbye-code
Copy link
Contributor

kkthxbye-code commented Jan 26, 2023

The intention was to match the UI, which only allows reassigning the device of InventoryItem, but not the other device components listed here:

'ConsolePort',
'ConsoleServerPort',
'DeviceBay',
'FrontPort',
'Interface',
'ModuleBay',
'PowerOutlet',
'PowerPort',
'RearPort',

@arthanson arthanson changed the title 11432 device field DRAFT: 11432 device field Mar 30, 2023
@arthanson arthanson changed the title DRAFT: 11432 device field 11432 device field Apr 3, 2023
@arthanson
Copy link
Collaborator Author

@jeremystretch should be ready now, tests fixed.

Comment on lines 34 to 58
class BaseReadonlyDeviceMixin:

def get_fields(self, *args, **kwargs):
fields = super().get_fields(*args, **kwargs)

# get_extra_kwargs doesn't work if field explicitly declared on serializer...
if (self.instance):
fields[self.get_field_name()].read_only = True

return fields


class ReadonlyDeviceMixin(BaseReadonlyDeviceMixin):

# can't be a field as mucks up serializer
def get_field_name(self):
return "device"


class ReadonlyDeviceTypeMixin(BaseReadonlyDeviceMixin):

def get_field_name(self):
return "device_type"


Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify this a bit by doing something like this? Also, I'd really like to avoid introducing a new public method on the serializer as it could result in confusion.

Suggested change
class BaseReadonlyDeviceMixin:
def get_fields(self, *args, **kwargs):
fields = super().get_fields(*args, **kwargs)
# get_extra_kwargs doesn't work if field explicitly declared on serializer...
if (self.instance):
fields[self.get_field_name()].read_only = True
return fields
class ReadonlyDeviceMixin(BaseReadonlyDeviceMixin):
# can't be a field as mucks up serializer
def get_field_name(self):
return "device"
class ReadonlyDeviceTypeMixin(BaseReadonlyDeviceMixin):
def get_field_name(self):
return "device_type"
class ReadonlyDeviceMixin:
_field_name = 'device'
def get_fields(self, *args, **kwargs):
fields = super().get_fields(*args, **kwargs)
if self.instance:
fields[self._field_name].read_only = True
return fields
class ReadonlyDeviceTypeMixin(ReadonlyDeviceMixin):
_field_name = 'device_type'

@jeremystretch
Copy link
Member

I think this still needs to be addressed in the model. With the field set to read-only, any attempt to change the parent device/device type of an existing component is silently ignored, because the field is ignored. However, any other valid attributes in the request will be applied successfully. I think we should instead return a validation error, so that the client is aware the entire proposed change did not succeed.

@arthanson
Copy link
Collaborator Author

@jeremystretch changed to model based validator, as discussed this will not be officially correct in the OpenAPI spec. Looked at doing it as read_only in the serializer and overriding validate_device, but that is never called if read_only is set. Can also be done fairly cleanly in the validate_device if no read_only is set, but that will only address it for the API call and you had mentioned wanting at model level so moved it there.

@jeremystretch jeremystretch merged commit 8de252e into develop Apr 12, 2023
@jeremystretch jeremystretch deleted the 11432-device-field branch April 12, 2023 14:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The device field on device-components (except InventoryItem) should not be writeable via. the API
3 participants