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

10348 add decimal custom field #10422

Merged
merged 24 commits into from
Sep 30, 2022
Merged

Conversation

arthanson
Copy link
Collaborator

Fixes: #10348

Adds custom decimal field type and associated tests.

@arthanson arthanson changed the title 10348 add decimal custom field DRAFT: 10348 add decimal custom field Sep 23, 2022
@jeremystretch jeremystretch added the beta Concerns a bug/feature in a beta release label Sep 26, 2022
@jeremystretch jeremystretch force-pushed the 10348-decimal-custom-field branch from 5a389cd to 818bbf0 Compare September 26, 2022 15:55
@jeremystretch jeremystretch force-pushed the 10348-decimal-custom-field branch from 818bbf0 to 91ec210 Compare September 26, 2022 15:56
@arthanson arthanson changed the title DRAFT: 10348 add decimal custom field 10348 add decimal custom field Sep 28, 2022
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Great work! The only issue I see at the moment is that decimal values are being stored as strings, rather than decimals/floats. Need to dig into this some more.

netbox/extras/models/customfields.py Outdated Show resolved Hide resolved
netbox/extras/models/customfields.py Outdated Show resolved Hide resolved
netbox/extras/tests/test_forms.py Show resolved Hide resolved
netbox/utilities/filters.py Outdated Show resolved Hide resolved
@jeremystretch
Copy link
Member

The only issue I see at the moment is that decimal values are being stored as strings, rather than decimals/floats.

Ok, tracked this down. The custom_field_data JSONField on each model is using DjangoJSONEncoder, which explicitly encodes decimals as strings.

Ideally, we should save these as raw JSON numbers. I'm not sure of the impact on precision, however it's probably acceptable.

If storing decimal values as JSON numbers is not tenable, an alternative would be to store them as strings but convert to decimals under CustomField.deserialize().

@jeremystretch
Copy link
Member

This should be about ready to go. We still need to resolve the validation issue with MultiValueDecimalFilter that @arthanson called out in ad6d4f8, but that should be addressed separately as it may have implications for other filters.

@jeremystretch jeremystretch merged commit af8bb0c into feature Sep 30, 2022
@jeremystretch jeremystretch deleted the 10348-decimal-custom-field branch September 30, 2022 20:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beta Concerns a bug/feature in a beta release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants