#5373 closed Bug (wontfix)
Field label for a ForeignKey not translated
Reported by: | Owned by: | Adrien Lemaire | |
---|---|---|---|
Component: | Internationalization | Version: | 1.3 |
Severity: | Normal | Keywords: | i18n foreignkey field label |
Cc: | david.danier@…, roald@…, lemaire.adrien@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Situation: I have ModelA and ModelB. ModelB contains a ForeignKey(ModelA) attribute. Both ModelA and ModelB have lazy translated verbose_name and verbose_name_plural Meta attributes. These show up well in the admin interface, however on ModelB's change form, the ForeignKey's field has the label 'ModelA' (not translated).
Hopefully my description is understandable. I can of course provide source code, if necessary.
Attachments (13)
Change History (45)
comment:1 by , 17 years ago
comment:2 by , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 17 years ago
Attachment: | ticket5373.tar.bz2 added |
---|
The smallest proof of concept code I could come up with. When you open the site, the "Category" label is not translated (everything else is).
follow-up: 9 comment:4 by , 16 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Analysis:
The self.verbose_name of the Category ForeignKey field gets set in django/db/models/fields/init.py in set_attributes_from_name (around line 190)
Later, when set_attributes_from_rel from django/db/models/fields/related.py is run, self.verbose_name always has a value and therefore, the value if self.rel.to._meta.verbose_name is never taken into account.
The attached patch fixes this behavior by not assigning a value to self.verbose_name in set_attributes_from_name for RelatedField instances.
by , 16 years ago
Attachment: | 5373.patch added |
---|
comment:5 by , 16 years ago
I don't have time to work on this currently, but I think that the patch attached is correct.
If a Field is a RelatedField, Meta.verbose_name should get set by set_attributes_from_rel, not by set_attributes_from_name. This patch prevents an early assignment to verbose_name which leads to set_attributes_from_rel NOT using the value of verbose_name from the other class.
I hope this explanation is clear enough.
by , 15 years ago
Attachment: | 5373.2.patch added |
---|
Updated patch to current SVN trunk, added comments
comment:6 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
by , 14 years ago
Attachment: | 5373.3.patch added |
---|
adapted patch to put import at the top of the file
comment:7 by , 14 years ago
5373.2.patch worked for me, I've made it more conventional and have added as 5373.3.diff
comment:8 by , 14 years ago
After some testing last night, my diff fails because it's calling django.db.models.fields.related to early in the initialization. The solutions available therefore are:
- Move the import statement lower as per 5373.2.patch - this seems wrong - goes against style?
- Change the relevant line of the patch to something like this (seems clunky...):
if not isinstance(self, django.db.models.related.RelatedField):
- Come up with new solution.
by , 14 years ago
Attachment: | 5373_regressiontests_admin_inlines.diff added |
---|
regression test to check admin_inlines
comment:9 by , 14 years ago
Replying to mk:
The self.verbose_name of the Category ForeignKey field gets set in django/db/models/fields/init.py in set_attributes_from_name (around line 190)
Later, when set_attributes_from_rel from django/db/models/fields/related.py is run, self.verbose_name always has a value and therefore, the value if self.rel.to._meta.verbose_name is never taken into account.
The attached patch fixes this behavior by not assigning a value to self.verbose_name in set_attributes_from_name for RelatedField instances.
I've just attached a patch to tests/regressiontests/admin_inlines that checks for this problem.
Since the second patch submitted by mk works, but 1. hasn't been accepted yet, and 2. puts an import statement in the body of the code, I thought I'd look for another solution.
I'm not sure about the full implications, but presumably making sure that the verbose_name is set in django/db/models/fields/related.py would work by just removing the line "if self.verbose_name = None" at 114?
by , 14 years ago
Attachment: | 5373_related.py.diff added |
---|
Patch changes related.py instead of init.py
comment:10 by , 14 years ago
I've added a new patch, which works on related.py, not init.py. In particular, it will change the verbose_name should it have been set in init . Simply, it checks if self.verbose_name = self.name and self.verbose_name != self.rel.to._meta.verbose_name
comment:11 by , 14 years ago
During my testing, I've not actually managed to get to django/db/models/fields/related.py line 115.
This change was added here: [8132] in relation to #8011 which includes the line
"I'll hook up a quick hack to fix this and investigate a longer term solution when I get a chance."
Can someone confirm that they can get to line 115? ie getting True from (self.verbose_name is None)
comment:12 by , 14 years ago
Ticket Summary:
[8132] added a line to set_attributes_from_rel in /django/db/models/fields/related.py that has a condition that will never be true, due to self.verbose_name being set by set_attribute_from_name in /django/db/models/fields/init.py
set_attribute_from_name makes self.verbose_name a string.
set_attribute_from_rel makes self.verbose_name a django.utils.functional.proxy object (ie, a translatable string)
I have been told in IRC and private advice that I should change the admin form instead of the models code, but since the fix attached works _and_ the problem is as I've stated above (str obj v django.utils.functional.proxy obj) I don't believe that's a very good way of dealing with this problem.
comment:13 by , 14 years ago
I should note that when I say "Proves that self.verbose_name is never none" I mean "when in set_attributes_from_rel context".
comment:14 by , 14 years ago
Cc: | added |
---|
comment:15 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:18 by , 13 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
Patch needs improvement: | set |
Triage Stage: | Accepted → Design decision needed |
UI/UX: | unset |
The last patch passes the tests from this ticket, bu breaks other tests. In some of those cases, the tests specify wrong behavior, but in other cases the tested behavior seems to be what you want. A decision should be made what the actual desired behavior is.
comment:19 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
This doesn't look like DDN, just a matter of reviewing the patch and making some implementation decisions. The ticket is a bug that needs to be fixed.
comment:20 by , 13 years ago
Cc: | added |
---|---|
Owner: | changed from | to
I have the same problem in my project.
@garcia_marc, I can't see any activity from you for the last 3 years on this ticket, I reassign it to myself.
comment:21 by , 13 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | unset |
Problem solved with the patch (code has a bit evolved, so patch is broken). I struggled a big because of another bug related to my project.
Will now write a test to cover that case.
comment:22 by , 13 years ago
Needs tests: | unset |
---|
Done. Without patch, test will raise "AssertionError: u'Choice' != 'Choice Option'"
Also cleaned a bit regressiontests/forms/ and added regressiontests/forms/forms.py to avoid recreating several times the same Form over the tests.
comment:23 by , 13 years ago
Patch needs improvement: | set |
---|
Thanks for the good-looking patch. However, you solved the issue at the model level, and the tests are at the forms level. I think it would be better to test the verbose_name in the model level (model_regress). Do you agree?
by , 13 years ago
Attachment: | 0002-Fix-5373-test-at-model-level.patch added |
---|
comment:24 by , 13 years ago
Patch needs improvement: | unset |
---|
Done. You're right, testing from the models makes much more sense :) Got confused because the initial bug report was talking about the label for a form field.
Note that for the test assertEqual, I hardcoded 'Fancy Department Name' instead of a more generic Department._meta.verbose_name, because the test would pass if someone removed the Department Meta class.
comment:25 by , 13 years ago
Thanks. Now even if it might not be broken currently, I think it would still be worth to test when the ForeignKey definition has a verbose_name (should have priority over related model verbose_name).
by , 13 years ago
Attachment: | 0003-Fix-5373-test-at-model-level.patch added |
---|
comment:27 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Sure, thanks! I just think that the committed patch will have parentheses removed around verbose_name string, unless you had a compelling reason to add them.
comment:28 by , 13 years ago
No reason. I was thinking for a moment if it would be a good idea to use ugettext as _ for this verbose_name, but didn't find another test doing so.
comment:29 by , 13 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
There are several failures in the test suite with the patch applied. I realize now that until now, the foreign key label is not determined from the related model name, but from the current model attribute name. For example:
class Book(models.Model): ... class Article(models.Model): parent = ForeignKey(Book)
The label will be Parent, not Book. With the patch applied, the logic will change, which is clearly backward incompatible.
I wonder in fact if this ticket should not simply be a won't fix, in that in the case of the original poster, the solution is simply to add a verbose_name attribute to the ForeignKey field. Even if it appears non-DRY when the field and the related model name are identical, we cannot assume that simply because the FK field has no verbose_name and there is a verbose_name on the related model, the user intends to use the related model name as label.
comment:30 by , 12 years ago
Patch needs improvement: | set |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
FWIW, to make it a little more DRY in case when the attribute and the model are named alike:
class Citation(models.Model): book = ForeignKey(Book, verbose_name=Book._meta.verbose_name)
Django could handle this special-case in the code, but IMHO that's too much magic.
comment:31 by , 12 years ago
I've just noticed that a somewhat similar error is occurring re: verbose_name on Object being a FK in a model. ie, the "not translated"/Internationalisation here is secondary to the problem.
This should be documented - in the i18n docs and the FK docs I think.
comment:32 by , 12 years ago
I also note that
class Citation(models.Model): book = ForeignKey(Book, verbose_name=Book._meta.verbose_name_plural)
works but
class Citation(models.Model): book = ForeignKey(Book, verbose_name_plural=Book._meta.verbose_name_plural)
doesn't.
I forgot to add: the same problem applies for a newforms form created with form_for_model(ModelB).