Skip to content
This repository has been archived by the owner on Aug 1, 2019. It is now read-only.

Use the interface language to generate the document list #73

Merged
merged 6 commits into from
Jan 5, 2016

Conversation

tsauerwein
Copy link
Member

tsauerwein added 2 commits December 22, 2015 16:58
So that the interface language can be read on the server-side.
@asaunier
Copy link
Member

Looks good to me.

For anonymous users, using a cookie to store the lang pref looks relevent.
For authenticated users, I wonder if it should not be stored in the user's data (https://github.com/c2corg/v6_api/blob/master/c2corg_api/models/user.py#L39-L45) and returned in the authData since it doesn't really make sense to lose the pref every time the browser is closed or when the user uses another device. We could keep the PR as is but that could be a significant improvement, don't you think?

@tsauerwein
Copy link
Member Author

We could keep the PR as is but that could be a significant improvement, don't you think?

Yep!

@asaunier
Copy link
Member

I'm OK for merging then (unless you want to change the cookie expiration time first).
Please add an issue about the lang prefs for auth users.

@tsauerwein
Copy link
Member Author

Ok, I set the expire date to 1 year. Note, that I also pushed a commit removing the method app.utils.getBestLangIndex.

@asaunier
Copy link
Member

I also pushed a commit removing the method app.utils.getBestLangIndex.

OK for you @gberaudo ?

@mfournier
Copy link
Contributor

Isn't it possible to avoid using cookies for anonymous users ? Would this be just for a single URL or for every URL served by the UI app ? I'm worried as this makes these elements hostile to caching.

An alternative behaviour I can imagine would be to use the browser's language to display (or redirect to) the page in the correct language, and use the url to define the language used (/fr/ or /?lang=fr).

Or is this purely in the javascript part ? I'm basically reacting to this as I noticed the self.request.cookies.get statement in the python part, which typically wouldn't work if varnish were to rip out cookies before passing the request to backend servers.

@gberaudo
Copy link
Contributor

Unless there is a fondamental issue with caching, I am OK for merging.

console.log('Failed to save interface language', e);
}
}
// store the interface language as cookie, so that it is available on the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obsolete comment, it is now 1 year.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing.

@tsauerwein
Copy link
Member Author

Isn't it possible to avoid using cookies for anonymous users ? Would this be just for a single URL or for every URL served by the UI app ? I'm worried as this makes these elements hostile to caching.

An alternative behaviour I can imagine would be to use the browser's language to display (or redirect to) the page in the correct language, and use the url to define the language used (/fr/ or /?lang=fr).

Or is this purely in the javascript part ? I'm basically reacting to this as I noticed the self.request.cookies.get statement in the python part, which typically wouldn't work if varnish were to rip out cookies before passing the request to backend servers.

As I understand it, by default Varnish resets the cookie because cookies often contain authentication information like the session id. In our case, we are not storing the authentication token in a cookie. We are just using the cookie to communicate the interface language to the server. Like you say, an alternative would be to make it an URL parameter. But if we configure Varnish properly (extract the language from the cookie and make it part of the cache key), we shouldn't have a problem with it. What do you think?

tsauerwein pushed a commit that referenced this pull request Jan 5, 2016
Use the interface language to generate the document list
@tsauerwein tsauerwein merged commit d2b1b58 into master Jan 5, 2016
@tsauerwein tsauerwein deleted the list-best-lang branch January 5, 2016 08:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants