Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#14825 closed New feature (fixed)

LocaleMiddleware should store language preferences if possible

Reported by: Vlastimil Zíma Owned by: Vlastimil Zíma
Component: Internationalization Version: dev
Severity: Normal Keywords:
Cc: raymond.penners@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

After logout (or other action than flushes session) language preferences are forgotten. This leads to strange behaviour when page during which session was flushed (e.g. /adminsite/logout/) is translated but next page is in default language (or browser prefered language).

This is tricky especially when your system requires explicit logout to log in other user.

Solution is quite easy, LocaleMiddleware should store current language when it is not stored in session or language cookie.

Attachments (2)

patch-14825.diff (406 bytes ) - added by Vlastimil Zíma 13 years ago.
LocaleMiddleware patch
patch-14825.2.diff (1.1 KB ) - added by Vlastimil Zíma 13 years ago.
Better patch

Download all attachments as: .zip

Change History (18)

by Vlastimil Zíma, 13 years ago

Attachment: patch-14825.diff added

LocaleMiddleware patch

comment:1 by rasca, 13 years ago

Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedDesign decision needed

Marking this as Design Decision Needed, cause despite I understand your use case when the session is flushed one might argue that the chosen language is sensitive data that needs to be flushed.

Another solution to your problem would be to render the "logged out" template before flushing the session.

in reply to:  1 comment:2 by Vlastimil Zíma, 13 years ago

Replying to rasca:

Marking this as Design Decision Needed, cause despite I understand your use case when the session is flushed one might argue that the chosen language is sensitive data that needs to be flushed.

Another solution to your problem would be to render the "logged out" template before flushing the session.

So it does not have to be the same middleware.

I do not see the point in rendering "logged out" page, because if you refresh it (GET request) you get different result (with default language) and any other page where you go from there will also have default language.

Only reasonable thing, I can think of, is call set_language after logout so it sets language to default before rendering the "logged out" page.

comment:3 by Ramiro Morales, 13 years ago

Patch needs improvement: set

Patch should be generated from the root of the tree, it isn't, and it seems to be in reversed patch. Also, would be nice if it has some context (e.g. changing its format to unified)

by Vlastimil Zíma, 13 years ago

Attachment: patch-14825.2.diff added

Better patch

comment:4 by Vlastimil Zíma, 13 years ago

I added better patch, sorry for errors in first one.

comment:5 by anonymous, 13 years ago

Severity: Normal
Type: New feature

Marking as 'new feature' as it changes current behavior; please also consider updating relevant documentation.

comment:6 by raymond.penners@…, 13 years ago

Cc: raymond.penners@… added
Easy pickings: unset

comment:7 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

This is a reasonable short-term workaround. #15902 will certainly take more time.

The patch is a good starting point, but it needs tests. Also, please use in rather than has_key, and consider using setdefault where possible.

I view this as a bugfix more than a new feature, but a line in the release notes can't hurt.

comment:10 by Vlastimil Zíma, 11 years ago

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Vlastimil Zíma
Patch needs improvement: unset
Status: newassigned

I updated the patch and created a pull request https://github.com/django/django/pull/957

I also realised that this only affects sessions. LocaleMiddleware should not touch the language cookie in this case, because it is not changed by session flush.

comment:11 by Vlastimil Zíma, 11 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Simon Charette, 11 years ago

Triage Stage: Ready for checkinAccepted
Version: 1.2master

Please don't mark your own patch as RFC, someone else should review your work.

comment:13 by Aymeric Augustin, 11 years ago

This looks good to me. Unfortunately the patch no longer applies after the sprints we just had. Could you bring it up to date and mark it as RFC once that's done? (Since I reviewed an almost final version, as an exception, you can mark it as RFC by yourself.)

comment:14 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 6de81d65f443a01961c23139ca5d7653ef012d35:

Fixed #14825 -- LocaleMiddleware keeps language

  • LocaleMiddleware stores language into session if it is not present there.

comment:15 by dave@…, 10 years ago

This change has made multi-language testing much more difficult for authenticated Django applications.

Prior to this commit, my QA team could follow this simple process to switch languages during testing:

  1. Switch language in browser settings
  2. Refresh page
  3. Test

Now, they must do this:

  1. Switch language in browser settings.
  2. Clear session cookie
  3. Re-authenticate
  4. Navigate to page under test
  5. Test

This behavior seems ok for the average user (whose language doesn't change minute to minute), but for a tester, this really makes things slow. Even test automation suffers (e.g., Selenium).

Could we at least put this behavior being a setting? Something like setting.STORE_LANGUAGE_IN_SESSION = True|False (default True)

comment:16 by Anssi Kääriäinen, 10 years ago

There is a ticket about this regression, see #21473. The suggested solution is to not flush language from session on logout.

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