Opened 17 years ago

Closed 12 years ago

Last modified 12 years ago

#5373 closed Bug (wontfix)

Field label for a ForeignKey not translated

Reported by: Szilveszter Farkas <szilveszter.farkas@…> 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)

ticket5373.tar.bz2 (7.1 KB ) - added by Szilveszter Farkas 17 years ago.
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).
5373.patch (910 bytes ) - added by Matthias Kestenholz 16 years ago.
5373.2.patch (1022 bytes ) - added by Matthias Kestenholz 15 years ago.
Updated patch to current SVN trunk, added comments
5373.3.patch (1.0 KB ) - added by Lachlan Musicman 14 years ago.
adapted patch to put import at the top of the file
5373.3.diff (1.2 KB ) - added by Lachlan Musicman 14 years ago.
same again, 4 space indents
5373_regressiontests_admin_inlines.diff (1.9 KB ) - added by Lachlan Musicman 14 years ago.
regression test to check admin_inlines
5373_related.py.diff (680 bytes ) - added by Lachlan Musicman 14 years ago.
Patch changes related.py instead of init.py
5373.4.diff (1.1 KB ) - added by Lachlan Musicman 14 years ago.
Patch, version 4
5373.5.diff (976 bytes ) - added by Lachlan Musicman 14 years ago.
Proves that self.verbose_name is never none
5373.6.diff (1.5 KB ) - added by Roald de Vries <roald@…> 13 years ago.
Improvement of prior solutions, but no final fix
0001-Regression-for-5373-with-tests.patch (6.8 KB ) - added by Adrien Lemaire 13 years ago.
patch udpated + test
0002-Fix-5373-test-at-model-level.patch (4.5 KB ) - added by Adrien Lemaire 13 years ago.
0003-Fix-5373-test-at-model-level.patch (5.0 KB ) - added by Adrien Lemaire 13 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 by Szilveszter Farkas <szilveszter.farkas@…>, 17 years ago

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

comment:2 by Adrian Holovaty, 17 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Adrian Holovaty, 17 years ago

Yes, some sample source code would be great!

by Szilveszter Farkas, 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).

comment:4 by Matthias Kestenholz, 16 years ago

Has patch: set
Owner: changed from nobody to Matthias Kestenholz
Status: newassigned

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 Matthias Kestenholz, 16 years ago

Attachment: 5373.patch added

comment:5 by Matthias Kestenholz, 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 Matthias Kestenholz, 15 years ago

Attachment: 5373.2.patch added

Updated patch to current SVN trunk, added comments

comment:6 by Marc Garcia, 15 years ago

Owner: changed from Matthias Kestenholz to Marc Garcia
Status: assignednew

by Lachlan Musicman, 14 years ago

Attachment: 5373.3.patch added

adapted patch to put import at the top of the file

comment:7 by Lachlan Musicman, 14 years ago

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

by Lachlan Musicman, 14 years ago

Attachment: 5373.3.diff added

same again, 4 space indents

comment:8 by Lachlan Musicman, 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 Lachlan Musicman, 14 years ago

regression test to check admin_inlines

in reply to:  4 comment:9 by Lachlan Musicman, 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 Lachlan Musicman, 14 years ago

Attachment: 5373_related.py.diff added

Patch changes related.py instead of init.py

by Lachlan Musicman, 14 years ago

Attachment: 5373.4.diff added

Patch, version 4

comment:10 by Lachlan Musicman, 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 Lachlan Musicman, 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 Lachlan Musicman, 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.

by Lachlan Musicman, 14 years ago

Attachment: 5373.5.diff added

Proves that self.verbose_name is never none

comment:13 by Lachlan Musicman, 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 David Danier <david.danier@…>, 14 years ago

Cc: david.danier@… added

comment:15 by Gabriel Hurley, 14 years ago

Severity: Normal
Type: Bug

comment:16 by theaspect@…, 14 years ago

Easy pickings: unset

subscribe on ticket

comment:17 by anonymous, 14 years ago

Easy pickings: set
Version: SVN1.3

bug still active

by Roald de Vries <roald@…>, 13 years ago

Attachment: 5373.6.diff added

Improvement of prior solutions, but no final fix

comment:18 by Roald de Vries <roald@…>, 13 years ago

Cc: roald@… added
Easy pickings: unset
Patch needs improvement: set
Triage Stage: AcceptedDesign 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 Carl Meyer, 13 years ago

Triage Stage: Design decision neededAccepted

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 Adrien Lemaire, 13 years ago

Cc: lemaire.adrien@… added
Owner: changed from Marc Garcia to 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.

comment:21 by Adrien Lemaire, 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.

by Adrien Lemaire, 13 years ago

patch udpated + test

comment:22 by Adrien Lemaire, 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.

Last edited 13 years ago by Adrien Lemaire (previous) (diff)

comment:23 by Claude Paroz, 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 Adrien Lemaire, 13 years ago

comment:24 by Adrien Lemaire, 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 Claude Paroz, 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 Adrien Lemaire, 13 years ago

comment:26 by Adrien Lemaire, 13 years ago

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

comment:27 by Claude Paroz, 13 years ago

Triage Stage: AcceptedReady 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 Adrien Lemaire, 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.

Last edited 13 years ago by Adrien Lemaire (previous) (diff)

comment:29 by Claude Paroz, 13 years ago

Triage Stage: Ready for checkinAccepted

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 Łukasz Rekucki, 12 years ago

Patch needs improvement: set
Resolution: wontfix
Status: newclosed

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 Lachlan Musicman, 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 Lachlan Musicman, 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.

Note: See TracTickets for help on using tickets.
Back to Top