Code

Opened 3 years ago

Closed 11 months ago

Last modified 5 months ago

#14825 closed New feature (fixed)

LocaleMiddleware should store language preferences if possible

Reported by: vzima Owned by: vzima
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 vzima 3 years ago.
LocaleMiddleware patch
patch-14825.2.diff (1.1 KB) - added by vzima 3 years ago.
Better patch

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by vzima

LocaleMiddleware patch

comment:1 follow-up: Changed 3 years ago by rasca

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 3 years ago by vzima

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 3 years ago by ramiro

  • 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 3 years ago by vzima

Better patch

comment:4 Changed 3 years ago by vzima

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

comment:5 Changed 3 years ago by anonymous

  • Severity set to Normal
  • Type set to New feature

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

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

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

comment:7 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 13 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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 13 months ago by vzima

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to vzima
  • Patch needs improvement unset
  • Status changed from new to assigned

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 13 months ago by vzima

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 13 months ago by charettes

  • Triage Stage changed from Ready for checkin to Accepted
  • Version changed from 1.2 to master

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

comment:13 Changed 11 months ago by aaugustin

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 11 months ago by Claude Paroz <claude@…>

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

In 6de81d65f443a01961c23139ca5d7653ef012d35:

Fixed #14825 -- LocaleMiddleware keeps language

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

comment:15 Changed 5 months 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 5 months ago by akaariai

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.