Skip to content
This repository was archived by the owner on Oct 30, 2018. It is now read-only.

[fix bug 1129659] Display banner if user is authenticated.#128

Closed
glogiotatidis wants to merge 1 commit intomozilla:masterfrom
glogiotatidis:urloggedin
Closed

[fix bug 1129659] Display banner if user is authenticated.#128
glogiotatidis wants to merge 1 commit intomozilla:masterfrom
glogiotatidis:urloggedin

Conversation

@glogiotatidis
Copy link
Contributor

No description provided.

@glogiotatidis
Copy link
Contributor Author

@jgmize r?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should add a never_cache decorator here since we we merged #127

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will cause the Vary: Cookie header to be set, which should effectively invalidate the cache without the @never_cache decorator, but I don't think the decorator hurts either.

@glogiotatidis
Copy link
Contributor Author

rebased! @jgmize ping?

@jgmize
Copy link
Contributor

jgmize commented Feb 24, 2015

Sorry @glogiotatidis, this apparently needs a re-rebase

@glogiotatidis
Copy link
Contributor Author

this needs the css to be re-generated. if you don't mind review the code and I can rebase before merging.

@glogiotatidis
Copy link
Contributor Author

Let's merge this before Ben files another bug that all locales are shown :)

@jgmize
Copy link
Contributor

jgmize commented Mar 2, 2015

Actually @glogiotatidis, I'm beginning to question the value of showing all locales to authenticated users in prod: given that we needed to set up a separate environment for unauthenticated users to see all locales anyway in #153 (comment), is this even a use case that we still need to support?

@bensternthal
Copy link
Contributor

I was trying to think of a use case for supporting this. More specifically if within the normal course content creation if a user would ever need to see all locales. I can not think of one.

If you can't either than I think l10n.masterfirefoxos.com is enough.

@jgmize
Copy link
Contributor

jgmize commented Mar 2, 2015

Sorry @glogiotatidis this was nice work and would have been a good resolution of this bug, but in response to @bensternthal's comment above I'm going to go ahead and close this and WONTFIX the bug, and submit a separate PR to eliminate showing all locales to authenticated users in favor of using https://l10n.masterfirefoxos.com exclusively for previewing locales and versions.

@jgmize jgmize closed this Mar 2, 2015
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.

3 participants