Opened 7 years ago
Closed 10 months ago
#28404 closed Bug (fixed)
Django admin empty_value/empty_value_display doesn't check for empty strings
Reported by: | Mark Koh | Owned by: | Alexander Lazarević |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | empty value display admin charfield |
Cc: | Sardorbek Imomaliev, Sarah Boyce | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | yes |
Description
According to the Django documentation, empty_value_display
should "display value for record’s fields that are empty (None, empty string, etc.)" However, currently this value is only returned when the field value is None and does not check for empty strings. This can be seen here - https://github.com/django/django/blob/5a52d932ef4da8228d82673314341c2c86602248/django/contrib/admin/utils.py#L420
Thus, if you have a CharField with a value of ''
(empty string), the empty_value_display
value will not appear, contrary to the documentation.
How to reproduce:
- Create a model with a
CharField(blank=True, null=True)
- Create a model_admin and add that field to the
list_display
list - Create two instances of the model, one with that field set to None and another with it set to
''
(empty string) - Open the admin panel and find those models, you will see that the instance with None shows
-
but the one with the empty string shows nothing.
Attachments (1)
Change History (30)
by , 7 years ago
Attachment: | empty_string bug.png added |
---|
comment:1 by , 7 years ago
Easy pickings: | unset |
---|
I'm not immediately convinced that the behavior should be changed instead of correcting the documentation to reflect the current behavior. Has the behavior changed since the documentation was introduced in 0207bdd2d4157c542c981264c86706b78ca246e9 or was the original documentation inaccurate?
comment:2 by , 7 years ago
The functionality hasn't changed since the documentation was updated, so it seems as though the original documentation was inaccurate. While I do have a PR in for the fix (WIP), I agree that it may make more sense to simply correct the documentation.
On the other hand, I do feel that this functionality would be useful as an additional overridable property for the ModelAdmin. Perhaps as an optional empty_string_display
property?
comment:3 by , 7 years ago
I've grown wearisome of all the ModelAdmin
hooks and complexity that they add. That's the only grounds on which I'd object to an additional property. If you have a compelling use case, I guess it's okay. I guess you're finding empty_value_display
useful?
comment:4 by , 7 years ago
I would definitely find it useful if it worked with empty strings as well as None
. Currently we have a field in our ModelAdmin that is a CharField
with blank=True
and null=False
and that field is also specified in the list_display_links
. Thus, when that field value is an empty string we are unable to click it (you can see this in the image attached to this ticket).
Seeing as the blank=True, null=False
method is the recommended method for supporting nullable CharFields I imagine that I'm not the only person whose seen this problem. I am open to either allowing empty_value_display
to acknowledge empty strings or to creating a new optional attribute for empty strings.
comment:5 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I don't know if adding a separate property will be simpler than modifying the existing empty_value_display
. I guess there could be use cases for a separate value for each case. You could write to the DevelopersMailingList for further feedback.
comment:6 by , 7 years ago
Patch needs improvement: | set |
---|
comment:7 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 7 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Version: | 1.11 → master |
comment:9 by , 7 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Hi, Nazarov Georgiy, you can't check "Ready for checkin" yourself, please see triaging-tickets
comment:10 by , 7 years ago
Patch needs improvement: | set |
---|
comment:11 by , 7 years ago
I haven't yet seen a reason not to re-use empty_value_display
here (rather than add a new admin option) but we need to consider when that is applied:
There are some cases where I really do want an empty string displayed (though not in a linked column). Maybe only use it only when it's an auto-linked field/column? Or maybe only use this behavior in display_for_field()? (So if you use a custom function/method, you can still have full control.) — Collin Anderson on Django Dev
comment:12 by , 5 years ago
Cc: | added |
---|
comment:13 by , 3 years ago
Confirming that this is definitely an issue with Django 1.11, and the docs for 1.11 still say this works for empty strings, which it does not: https://docs.djangoproject.com/en/1.11/ref/contrib/admin/#django.contrib.admin.ModelAdmin.empty_value_display. These docs should be updated to avoid confusion in the future.
comment:14 by , 3 years ago
Are there any updates on this?
I just stumbled on this issue when trying to use a "nullable" char field (blank=True, null=False as the docs suggest) in a changelist filter. The filter option for the empty string is displayed as a literal empty string, which results in the surrounding link element collapsing to height 0 and being virtually unusable since you can't click it. The link itself, once accessed via inspector, works fine.
Docs are also unchanged, so what is the verdict on this issue?
comment:15 by , 3 years ago
Given the db.models.Field
has an API that defines what empty values should be I think that admin logic should be changed to rely on it?
diff --git a/django/contrib/admin/templatetags/admin_list.py b/django/contrib/admin/templatetags/admin_list.py index 5865843dce..a570026b1e 100644 --- a/django/contrib/admin/templatetags/admin_list.py +++ b/django/contrib/admin/templatetags/admin_list.py @@ -227,7 +227,7 @@ def link_in_col(is_first, field_name, cl): else: if isinstance(f.remote_field, models.ManyToOneRel): field_val = getattr(result, f.name) - if field_val is None: + if field_val in f.empty_values: result_repr = empty_value_display else: result_repr = field_va
That would at least make the notion of empty coherent between models and the admin. It kind of blurs the line it terms of what change list filtering by empty value mean when there's multiple candidates though (e.g. CharField(blank=True, null=True)
). Kind of wish we had an __empty
lookup for this purpose that would be an alias for all allowed empty values by the field definition:
class EmptyModel(models.Model): text = TextField(blank=True, null=False) nullable_text = TextField(blank=True, null=True) integer = IntegerField(blank=True, null=True) array = ArrayField(TextField(), null=True) json = JSONField() EmptyModel.objects.filter( text__empty=True, # Q(text="") nullable_text__empty=True, # Q(nullable_text="") | Q(nullable_text=None) integer__empty=True, # Q(integer=None) array__empty=True, # Q(array=[]) | Q(array=None) json__empty=True, # Q(json=[]) | Q(json={}) | Q(json=None) )
If this existed the admin filter could for empty value could basically be __empty=True
instead of __isnull=True
.
I don't think we should change the list display behaviour without fixing the filter one as that would cause a large discrepancy and given the filter one is a bit tricky I'd suggest starting by adjusting the documentation.
comment:16 by , 19 months ago
Cc: | added |
---|
comment:17 by , 10 months ago
Owner: | changed from | to
---|
After watching how to perform the "vulture method" in (1) I found this ticket.
According to “Claiming” tickets' I reassign this ticket to myself, because "... If a ticket has been assigned for weeks or months without any activity, it’s probably safe to reassign it to yourself." I hope that's ok.
(1) https://fosstodon.org/@djangonaut@indieweb.social/111697455678403023
comment:19 by , 10 months ago
New PR https://github.com/django/django/pull/17699
I had to create a fresh PR: https://github.com/django/django/pull/17702
comment:21 by , 10 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:24 by , 10 months ago
I'm afraid my PR might not have been complete. I just tried something like this:
@admin.register(SomeModel) class SomeModelAdmin(admin.ModelAdmin): list_display = ( "name", "description", "my_description", ) empty_value_display = "-empty-" @admin.display(empty_value="-very empty-") def my_description(self, obj): return "" # obj.description
and it did not show the "-very empty-" string.
I guess this needs to be changed as well:
def display_for_value(value, empty_value_display, boolean=False): from django.contrib.admin.templatetags.admin_list import _boolean_icon if boolean: return _boolean_icon(value) elif value is None: <------------------------- return empty_value_display elif isinstance(value, bool): return str(value) ...
Can somebody please confirm this? Sorry for not seeing it earlier ...
comment:25 by , 10 months ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
Agreed, let's reopen.
comment:26 by , 10 months ago
Has patch: | set |
---|
Additional PR https://github.com/django/django/pull/17720
comment:27 by , 10 months ago
Status: | new → assigned |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:29 by , 10 months ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Empty strings with no empty_value