Field label for a ForeignKey not translated

[Szilveszter Farkas <szilveszter.farkas@…>]

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.

[Szilveszter Farkas <szilveszter.farkas@…>]

I forgot to add: the same problem applies for a newforms form created with form_for_model(ModelB).

[Adrian Holovaty]

Yes, some sample source code would be great!

[Szilveszter Farkas]

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).

[Matthias Kestenholz]

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.

[Matthias Kestenholz]

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.

[Matthias Kestenholz]

Updated patch to current SVN trunk, added comments

[Lachlan Musicman]

adapted patch to put import at the top of the file

[Lachlan Musicman]

5373.2.patch worked for me, I've made it more conventional and have added as 5373.3.diff

[Lachlan Musicman]

same again, 4 space indents

[Lachlan Musicman]

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.

[Lachlan Musicman]

regression test to check admin_inlines

[Lachlan Musicman]

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?

[Lachlan Musicman]

Patch changes related.py instead of init.py

[Lachlan Musicman]

Patch, version 4

[Lachlan Musicman]

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

[Lachlan Musicman]

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)

[Lachlan Musicman]

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.

[Lachlan Musicman]

Proves that self.verbose_name is never none

[Lachlan Musicman]

I should note that when I say "Proves that self.verbose_name is never none" I mean "when in set_attributes_from_rel context".

[theaspect@…]

subscribe on ticket

[anonymous]

bug still active

[Roald de Vries <roald@…>]

Improvement of prior solutions, but no final fix

[Roald de Vries <roald@…>]

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.

[Carl Meyer]

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.

[Adrien Lemaire]

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.

[Adrien Lemaire]

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.

[Adrien Lemaire]

patch udpated + test

[Adrien Lemaire]

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.

[Claude Paroz]

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?

[Adrien Lemaire]

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.

[Claude Paroz]

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).

[Adrien Lemaire]

I hope you'll be satisfied with this last one :)

[Claude Paroz]

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.

[Adrien Lemaire]

Not 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.

[Claude Paroz]

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.

[Łukasz Rekucki]

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.

[Lachlan Musicman]

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.

[Lachlan Musicman]

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.