Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#15272 closed (fixed)

Context object name is translated string

Reported by: szczav Owned by: nobody
Component: Generic views Version: dev
Severity: Keywords: generic views context verbose_name blocker
Cc: victor.andree@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm using latest trunk. While I was using generic views all was perfect until I made some translations. Then some of my views crashed with VariableDoesNotExist("Failed lookup for key... I found out that when there is no context_object_name defined in generic class view then default value of this variable is not a object name, but its translation. In my case model name is Company so default context_object_name should be "company" and so it is so until I make translations, then it is (in my case) "firma". Context variables can't be dependand on translations and should be lowercased model name. Now it's inconsistent and may cause crash after modifying translations.

It seems that fix is quite simple: in all methods get_context_object_name in ObjectMixins there is line with obj._meta.verbose_name.lower(). It should be replaced by obj._meta.object_name.lower().

Tomorrow I'll try to do patch & tests.

Attachments (2)

patch.diff (17.1 KB ) - added by szczav 13 years ago.
Patch and tests
patch2.diff (15.0 KB ) - added by szczav 13 years ago.
fix, tests, docs

Download all attachments as: .zip

Change History (14)

comment:1 by anonymous, 13 years ago

Keywords: blocker added
milestone: 1.3
Triage Stage: UnreviewedAccepted

This looks like it might be a release blocker.

Using object_name is a good suggestion, but it won't work, because we need a plural version too (for list views).

comment:2 by Russell Keith-Magee, 13 years ago

That last comment was me. Trac logged me out again...

by szczav, 13 years ago

Attachment: patch.diff added

Patch and tests

comment:3 by szczav, 13 years ago

Has patch: set

I think that using verbose name as context variables names is bad idea. Verbose name is used to provide translations, not to provide variables names. We can create some default translations using variables, but it shouldn't work in the other direction. I can even imagine case when somebody creates verbose name containing only national letters which now would be replaced by underscores so we would have context variables like 'a_' or '_'. I don't see better way to create names of context variables than adding postfix '_list' to object_name. This pattern is often used in Django. So model Article for detail list view will have article_list variable. It's easy to create, and guaranties safety.

There is also other inconsistency in generic views. Error messages for 404 aren't translated but object name passed to them is again verbose_name:

raise Http404("No %s available" % model._meta.verbose_name)

Translating only part of the string makes no sense. For developer such message is confusing, especially when he doesn't know language used in translation. This also should be model._meta.object_name.

I made patch with fixes & tests for booth cases: context variables and error messages.

comment:4 by Łukasz Rekucki, 13 years ago

This is a bit related to #14018. In both cases, it would be great to have a pluralized version of the model's name, but a one that's a valid variable name. Falling back to "%s_list" is a good idea. Adding name_plural to model's Meta would allow for optional nicer names. Either way, this also needs an update to the docs. Current docs don't even describe the current behaviour (maybe just remove it then, as undocumented ?).

As for error messages, it's a separate issue that should be dealt with a separate ticket. IMHO, they should be translated, so using verbose name is ok there.

Last edited 13 years ago by Łukasz Rekucki (previous) (diff)

comment:5 by Łukasz Rekucki, 13 years ago

Needs documentation: set
Patch needs improvement: set

by szczav, 13 years ago

Attachment: patch2.diff added

fix, tests, docs

comment:6 by szczav, 13 years ago

I've removed error message fixes (I'll create separate ticket on this) and added docs. Way how context variables are created is described in docs, so I've just rewritten it. By the way get_context_object_name() description for the SingleObjectMixin was completely wrong before.

Idea of #14018 is good but as I see its milestone is 2.0 so probably we have to stay with object_name concept.

comment:7 by Victor Andrée, 13 years ago

Cc: victor.andree@… added

comment:8 by Łukasz Rekucki, 13 years ago

Replying to szczav:

I've removed error message fixes (I'll create separate ticket on this) and added docs. Way how context variables are created is described in docs, so I've just rewritten it. By the way get_context_object_name() description for the SingleObjectMixin was completely wrong before.

Looks good to me, but didn't actually run it, so someone should take a look at it.


Idea of #14018 is good but as I see its milestone is 2.0 so probably we have to stay with object_name concept.

Not everything people set on Trac is accurate. IMHO, adding "name_plural" to Meta is fully backwards compatible. If it's actually worth the small improvements, is a different matter. But it's also out of scope for 1.3 as a new feature (could be added in 1.4). It definitely shouldn't block a release, so going with object_name is the right way here :)

comment:9 by Łukasz Rekucki, 13 years ago

Needs documentation: unset
Patch needs improvement: unset

Forgot about the flags again.

comment:10 by Adam Nelson, 13 years ago

Triage Stage: AcceptedReady for checkin

All tests pass for me and the patch looks consistent. I don't know what's up with the milestone though - up to somebody else.

comment:11 by Russell Keith-Magee, 13 years ago

Resolution: fixed
Status: newclosed

In [15531]:

Fixed #15272 -- Altered generic views to use the guaranteed untranslated object_name, rather than the possibly translated verbose_name(_plural) for default context objects. Thanks to szczav for the report and patch.

This is BACKWARDS INCOMPATIBLE for anyone relying on the default context object names for class-based Detail and List views. To migrate, either update your templates to use the new default names, or add a context_object_name argument to your generic views.

comment:12 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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