Fixed #36539 -- Admin uses related field verbose_name attribute if available #19704
Conversation
There was a problem hiding this comment.
Thank you @leandrodesouzadev for starting this work! 🌟
We should definitely add more tests ensuring that fields with and without verbose name are treated correctly. Particularly the domain field that does not define a verbose_name should still show what was shown previously.
Does that make sense?
|
Also where are the screenshots that you mentioned you took by checking the item in the PR template list? |
I thought that since there aren't any changes related to the documentation, then I could just checked them. |
I see the confusion! If you are not uploading screenshots, you can leave it unchecked or delete them. Thank you for asking! |
2f65c26 to
76269b6
Compare
Just pushed a few changes regarding the tests for using with/without verbose_name |
There was a problem hiding this comment.
Thank you for working on this ticket @leandrodesouzadev ⭐️
This is definitely a necessary change and something that should be improved!
tests/admin_utils/tests.py
Outdated
| self.assertEqual(label_for_field("site__owner", Article), owner_verbose_name) | ||
| self.assertEqual( | ||
| label_for_field("site__domain", Article, return_attr=True), | ||
| ("Site domain", Site._meta.get_field("domain")), | ||
| (owner_verbose_name, domain_field), |
There was a problem hiding this comment.
It seems like a mistake 😁
index e2f55b80f3..8bffa26683 100644
--- a/tests/admin_utils/tests.py
+++ b/tests/admin_utils/tests.py
@@ -348,8 +348,8 @@ class UtilsTests(SimpleTestCase):
self.assertEqual(owner_field.verbose_name, owner_verbose_name)
self.assertEqual(label_for_field("site__owner", Article), owner_verbose_name)
self.assertEqual(
- label_for_field("site__domain", Article, return_attr=True),
- (owner_verbose_name, domain_field),
+ label_for_field("site__owner", Article, return_attr=True),
+ (owner_verbose_name, owner_field),
)
def test_label_for_field_failed_lookup(self):There was a problem hiding this comment.
Oops! Was caught by the copy-pasta police. Just pushed a fix
|
Is there any wacky tests? A test is failing without any change related to the tests/code path.
Don't really know what do next now. |
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| class Site(models.Model): | |||
| domain = models.CharField(max_length=100) | |||
| owner = models.CharField(verbose_name=_("Site owner")) | |||
There was a problem hiding this comment.
Is there any wacky tests? A test is failing without any change related to the tests/code path.
FAIL: test_logentry_change_message_formsets (admin_utils.test_logentry.LogEntryTests.test_logentry_change_message_formsets)
All messages for changed formsets are logged in a change message.Don't really know what do next now.
index 37d13d24bd..50023b5788 100644
--- a/tests/admin_utils/test_logentry.py
+++ b/tests/admin_utils/test_logentry.py
@@ -21,7 +21,7 @@ class LogEntryTests(TestCase):
cls.user = User.objects.create_superuser(
username="super", password="secret", email="super@example.com"
)
- cls.site = Site.objects.create(domain="example.org")
+ cls.site = Site.objects.create(domain="example.org", owner="Tom")
cls.a1 = Article.objects.create(
site=cls.site,
title="Title",
@@ -148,6 +148,7 @@ class LogEntryTests(TestCase):
)
post_data = {
"domain": "example.com", # domain changed
+ "owner": "Tom",
"admin_articles-TOTAL_FORMS": "5",
"admin_articles-INITIAL_FORMS": "2",
"admin_articles-MIN_NUM_FORMS": "0",Since a field was added to the Site model, that field also needs to be included when passing data to the form.
It seems like this issue is what's causing the problem!
There was a problem hiding this comment.
Wow, thanks for pointing out.
|
Great stuff, thanks for the help on my first PR @nessita and @Antoliny0919. |
Trac ticket number
ticket-36539
Branch description
Related name supported was added on 5.1 but related field verbose names weren't taken into consideration.
This PR added support for using the verbose_name attribute of the related field.
Checklist
mainbranch.