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

Fixed #28404 -- Made displaying values in admin respect Field's empty_values. #17702

Conversation

e11bits
Copy link
Contributor

@e11bits e11bits commented Jan 6, 2024

This is a PR to pickup ticket 28404 again. Prior PRs are #9391 #8776 and #9390 .

This is my first PR for Django, addressing not showing the empty_display_value if value is the empty string and not None in the admin app. Please advise as you see fit.

Recreated the PR from a former PR (that I closed and thought could easily reopen again).

I now check for value in field.empty_values instead only None and "", which should cover all the "empty" cases.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@e11bits Thanks for this patch 👍

tests/admin_utils/tests.py Outdated Show resolved Hide resolved
tests/admin_utils/tests.py Outdated Show resolved Hide resolved
tests/admin_utils/tests.py Outdated Show resolved Hide resolved
e11bits added a commit to e11bits/django that referenced this pull request Jan 8, 2024
`test_null_display_for_field` is replaced by

  - `test_empty_value_display_for_field`
  - `test_empty_value_display_choices`
  - `test_empty_value_display_booleanfield`

PR recommendation by @felixxm
django#17702 (comment)
e11bits added a commit to e11bits/django that referenced this pull request Jan 8, 2024
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from 7627fb0 to ce1c14c Compare January 8, 2024 06:57
@felixxm
Copy link
Member

felixxm commented Jan 8, 2024

@e11bits Please also check Carlton's comment from the previous attempt as it's not addressed here.

e11bits added a commit to e11bits/django that referenced this pull request Jan 8, 2024
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch 2 times, most recently from 6017785 to a330fbb Compare January 8, 2024 09:00
e11bits added a commit to e11bits/django that referenced this pull request Jan 8, 2024
@e11bits
Copy link
Contributor Author

e11bits commented Jan 8, 2024

I added a new test test_result_list_empty_changelist_string_value lacking a better name.

@e11bits
Copy link
Contributor Author

e11bits commented Jan 8, 2024

Two tests failed because of "TimeoutError: SMTP server started, but not responding within allotted time. This might happen if the system is too busy. Try increasing the ready_timeout parameter."

I guess this just temporary? Can I trigger the test run again (without doing another commit)?

e11bits added a commit to e11bits/django that referenced this pull request Jan 8, 2024
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from a330fbb to c36dc38 Compare January 8, 2024 20:59
@e11bits e11bits changed the title Fixed empty_value_display for fields with empty string values Fixed #28404 -- empty_value_display for fields with empty string values Jan 8, 2024
@felixxm
Copy link
Member

felixxm commented Jan 9, 2024

I merged the first commit, 1b0a899. Please rebase.

e11bits added a commit to e11bits/django that referenced this pull request Jan 9, 2024
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from c36dc38 to dd2eca2 Compare January 9, 2024 10:52
tests/admin_widgets/tests.py Outdated Show resolved Hide resolved
tests/admin_widgets/tests.py Outdated Show resolved Hide resolved
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from dd2eca2 to a5a9a59 Compare January 9, 2024 11:04
e11bits added a commit to e11bits/django that referenced this pull request Jan 9, 2024
e11bits added a commit to e11bits/django that referenced this pull request Jan 9, 2024
@e11bits e11bits force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from a5a9a59 to 63b0773 Compare January 9, 2024 11:08
@e11bits
Copy link
Contributor Author

e11bits commented Jan 9, 2024

Updated the PR as suggested.

@felixxm felixxm changed the title Fixed #28404 -- empty_value_display for fields with empty string values Fixed #28404 -- Made displaying values in admin respect Field's empty_values. Jan 10, 2024
@felixxm felixxm force-pushed the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch from 63b0773 to 9b02ad9 Compare January 10, 2024 07:43
@felixxm
Copy link
Member

felixxm commented Jan 10, 2024

@e11bits Thanks 👍

@felixxm felixxm added the selenium Apply to have Selenium tests run on a PR label Jan 10, 2024
@felixxm felixxm merged commit 9b02ad9 into django:main Jan 10, 2024
37 checks passed
@e11bits e11bits deleted the bugfix/28404_showing_empty_value_for_empty_strings_in_django_admin branch January 11, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
selenium Apply to have Selenium tests run on a PR
Projects
None yet
2 participants