Code

Opened 7 years ago

Closed 21 months ago

Last modified 21 months ago

#5373 closed Bug (wontfix)

Field label for a ForeignKey not translated

Reported by: Szilveszter Farkas <szilveszter.farkas@…> Owned by: Fandekasp
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 7 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 mk 6 years ago.
5373.2.patch (1022 bytes) - added by mk 5 years ago.
Updated patch to current SVN trunk, added comments
5373.3.patch (1.0 KB) - added by datakid 4 years ago.
adapted patch to put import at the top of the file
5373.3.diff (1.2 KB) - added by datakid 4 years ago.
same again, 4 space indents
5373_regressiontests_admin_inlines.diff (1.9 KB) - added by datakid 4 years ago.
regression test to check admin_inlines
5373_related.py.diff (680 bytes) - added by datakid 4 years ago.
Patch changes related.py instead of init.py
5373.4.diff (1.1 KB) - added by datakid 4 years ago.
Patch, version 4
5373.5.diff (976 bytes) - added by datakid 4 years ago.
Proves that self.verbose_name is never none
5373.6.diff (1.5 KB) - added by Roald de Vries <roald@…> 3 years ago.
Improvement of prior solutions, but no final fix
0001-Regression-for-5373-with-tests.patch (6.8 KB) - added by Fandekasp 2 years ago.
patch udpated + test
0002-Fix-5373-test-at-model-level.patch (4.5 KB) - added by Fandekasp 2 years ago.
0003-Fix-5373-test-at-model-level.patch (5.0 KB) - added by Fandekasp 2 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 Changed 7 years ago by Szilveszter Farkas <szilveszter.farkas@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 7 years ago by adrian

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 7 years ago by adrian

Yes, some sample source code would be great!

Changed 7 years ago by szilveszter

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 follow-up: Changed 6 years ago by mk

  • Has patch set
  • Owner changed from nobody to mk
  • Status changed from new to 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.

Changed 6 years ago by mk

comment:5 Changed 6 years ago by mk

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.

Changed 5 years ago by mk

Updated patch to current SVN trunk, added comments

comment:6 Changed 5 years ago by garcia_marc

  • Owner changed from mk to garcia_marc
  • Status changed from assigned to new

Changed 4 years ago by datakid

adapted patch to put import at the top of the file

comment:7 Changed 4 years ago by datakid

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

Changed 4 years ago by datakid

same again, 4 space indents

comment:8 Changed 4 years ago by datakid

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.

Changed 4 years ago by datakid

regression test to check admin_inlines

comment:9 in reply to: ↑ 4 Changed 4 years ago by datakid

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?

Changed 4 years ago by datakid

Patch changes related.py instead of init.py

Changed 4 years ago by datakid

Patch, version 4

comment:10 Changed 4 years ago by datakid

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 Changed 4 years ago by datakid

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 Changed 4 years ago by datakid

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.

Changed 4 years ago by datakid

Proves that self.verbose_name is never none

comment:13 Changed 4 years ago by datakid

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 Changed 4 years ago by David Danier <david.danier@…>

  • Cc david.danier@… added

comment:15 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:16 Changed 3 years ago by theaspect@…

  • Easy pickings unset

subscribe on ticket

comment:17 Changed 3 years ago by anonymous

  • Easy pickings set
  • Version changed from SVN to 1.3

bug still active

Changed 3 years ago by Roald de Vries <roald@…>

Improvement of prior solutions, but no final fix

comment:18 Changed 3 years ago by Roald de Vries <roald@…>

  • Cc roald@… added
  • Easy pickings unset
  • Patch needs improvement set
  • Triage Stage changed from Accepted to 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 Changed 3 years ago by carljm

  • Triage Stage changed from Design decision needed to 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 Changed 2 years ago by Fandekasp

  • Cc lemaire.adrien@… added
  • Owner changed from garcia_marc to Fandekasp

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 Changed 2 years ago by Fandekasp

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

Changed 2 years ago by Fandekasp

patch udpated + test

comment:22 Changed 2 years ago by Fandekasp

  • 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 2 years ago by Fandekasp (previous) (diff)

comment:23 Changed 2 years ago by claudep

  • 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?

Changed 2 years ago by Fandekasp

comment:24 Changed 2 years ago by Fandekasp

  • 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 Changed 2 years ago by claudep

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

Changed 2 years ago by Fandekasp

comment:26 Changed 2 years ago by Fandekasp

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

comment:27 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to 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 Changed 2 years ago by Fandekasp

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 2 years ago by Fandekasp (previous) (diff)

comment:29 Changed 2 years ago by claudep

  • Triage Stage changed from Ready for checkin to 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 Changed 21 months ago by lrekucki

  • Patch needs improvement set
  • Resolution set to wontfix
  • Status changed from new to 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 Changed 21 months ago by datakid

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 Changed 21 months ago by datakid

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.