#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)
Change History (14)
comment:1 by , 14 years ago
Keywords: | blocker added |
---|---|
milestone: | → 1.3 |
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 14 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 , 14 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.
comment:5 by , 14 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:6 by , 14 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 , 14 years ago
Cc: | added |
---|
comment:8 by , 14 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 , 14 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
Forgot about the flags again.
comment:10 by , 14 years ago
Triage Stage: | Accepted → Ready 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.
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).