Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15272 closed (fixed)

Context object name is translated string

Reported by: szczav Owned by: nobody
Component: Generic views Version: master
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: UI/UX:

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 4 years ago.
Patch and tests
patch2.diff (15.0 KB) - added by szczav 4 years ago.
fix, tests, docs

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by anonymous

  • Keywords blocker added
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

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

Changed 4 years ago by szczav

Patch and tests

comment:3 Changed 4 years ago by szczav

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

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 4 years ago by lrekucki (previous) (diff)

comment:5 Changed 4 years ago by lrekucki

  • Needs documentation set
  • Patch needs improvement set

Changed 4 years ago by szczav

fix, tests, docs

comment:6 Changed 4 years ago by 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.

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

  • Cc victor.andree@… added

comment:8 Changed 4 years ago by lrekucki

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

  • Needs documentation unset
  • Patch needs improvement unset

Forgot about the flags again.

comment:10 Changed 4 years ago by adamnelson

  • Triage Stage changed from Accepted to 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.

comment:11 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

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

  • milestone 1.3 deleted

Milestone 1.3 deleted

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