Opened 6 years ago

Closed 4 years ago

Last modified 3 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: master
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 6 years ago.
LocaleMiddleware patch
patch-14825.2.diff (1.1 KB) - added by Vlastimil Zíma 6 years ago.
Better patch

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by Vlastimil Zíma

Attachment: patch-14825.diff added

LocaleMiddleware patch

comment:1 Changed 6 years ago by rasca

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.

comment:2 in reply to:  1 Changed 6 years ago by Vlastimil Zíma

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 Changed 6 years ago by Ramiro Morales

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)

Changed 6 years ago by Vlastimil Zíma

Attachment: patch-14825.2.diff added

Better patch

comment:4 Changed 6 years ago by Vlastimil Zíma

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

comment:5 Changed 6 years ago by anonymous

Severity: Normal
Type: New feature

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

comment:6 Changed 6 years ago by raymond.penners@…

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

comment:7 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by Vlastimil Zíma

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 Changed 4 years ago by Vlastimil Zíma

Triage Stage: AcceptedReady for checkin

comment:12 Changed 4 years ago by Simon Charette

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

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 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

In 6de81d65f443a01961c23139ca5d7653ef012d35:

Fixed #14825 -- LocaleMiddleware keeps language

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

comment:15 Changed 3 years ago by dave@…

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 Changed 3 years ago by Anssi Kääriäinen

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