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.

Sign up for GitHub

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

Jump to bottom

17096 Do not omit "False" values from the request URL for cloned fields #17113

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

17096 Do not omit "False" values from the request URL for cloned fields #17113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dea3ff
Do not omit "False" values from the request URL for cloned fields
peteeckel Aug 9, 2024
e8a5fa3
Limit the fix to custom field data
peteeckel Aug 11, 2024
ce7343c
Properly handle "False" as a text parameter when initialising NetBoxM…
peteeckel Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions netbox/netbox/forms/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json

from django import forms
from django.forms.fields import BooleanField, NullBooleanField
from django.contrib.contenttypes.models import ContentType
from django.db.models import Q
from django.utils.translation import gettext_lazy as _
Expand Down Expand Up @@ -31,6 +32,15 @@ class NetBoxModelForm(CheckLastUpdatedMixin, CustomFieldsMixin, TagsMixin, forms
"""
fieldsets = ()

def __init__(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

While the logic makes sense, I don't think NetBoxModelForm is the best place to address this. We want to ensure that this behavior is consistent for all forms across the application, and many forms don't inherit from this class.

Maybe normalize_querydict() would be a more appropriate place for this logic. We lose access to the individual form fields but it's probably reasonably safe to assume the string "False" implies the boolean value False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's probably reasonably safe to assume the string "False" implies the boolean value False.

While this may be true in most cases I would not implement code that relies on this assumption. It's just that, an assumption, after all, and errors resulting from applications actually relying on "False" being a string will be pretty hard to debug. That kind of reasoning is exactly where the referenced issue came from, and the solution of just assuming "False" will always refer to a boolean value will only hide the issue better.

The question is: Are there any forms not derived from NetBoxModelForm that are used in cloning objects?

super().__init__(*args, **kwargs)

for key, value in self.initial.items():
if key not in self.fields:
continue
if isinstance(self.fields[key], (BooleanField, NullBooleanField)) and self.initial[key] == "False":
self.initial[key] = False

def _get_content_type(self):
return ContentType.objects.get_for_model(self._meta.model)

Expand Down
2 changes: 1 addition & 1 deletion netbox/utilities/querydict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def prepare_cloned_fields(instance):
for key, value in attrs.items():
if type(value) in (list, tuple):
params.extend([(key, v) for v in value])
elif value is not False and value is not None:
elif value is not None:
params.append((key, value))
else:
params.append((key, ''))
Expand Down