Skip to content

Bug 794059. Allow pages to be activated per locale.#459

Merged
pmclanahan merged 1 commit intomozilla:masterfrom
pmclanahan:bug-794059-activate-pages-per-locale
Nov 26, 2012
Merged

Bug 794059. Allow pages to be activated per locale.#459
pmclanahan merged 1 commit intomozilla:masterfrom
pmclanahan:bug-794059-activate-pages-per-locale

Conversation

@pmclanahan
Copy link
Copy Markdown
Contributor

Going to a page at a non en-US locale will now redirect
to en-US unless the corresponding .lang file for that
template and locale has ## active ## as its first
line. This will allow localizers to only publish a
localized page after it's confirmed to be fully and
correctly translated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this should get some caching or not. It should be pretty quick to check the first line of a file, but if you guys think that storing the results in memcache for a minute or so might help with load then it can be easily added.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IMO it's worth given how easy this would be to cache, although I'm curious why you'd only cache it for a minute. How often do these files change on production? Is memcache cleared when we push?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That was based on the value in our settings for DOTLANG_CACHE being 60. I think we should consider increasing that as well to at least the length of time between the cron runs.

@rik
Copy link
Copy Markdown
Contributor

rik commented Nov 14, 2012

Note: This PR should not be merged before making sure the appropriate .lang files have been updated with the ## active ## header.

@pmclanahan
Copy link
Copy Markdown
Contributor Author

Agreed. @rik any thoughts on the performance implications?

@ghost ghost assigned Osmose Nov 21, 2012
@rik
Copy link
Copy Markdown
Contributor

rik commented Nov 21, 2012

I'll let others review this :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a possible race condition between here and when you actually open the file below. You should probably use a try clause instead and catch the IOError

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but only if the file doesn't exist right? So we might get one more false negative before we see the locale activated. This doesn't seem like a big enough deal to worry with. This should almost never happen anyway as these files are automatically created.

@pmclanahan
Copy link
Copy Markdown
Contributor Author

@Osmose updated for your approval.

@Osmose
Copy link
Copy Markdown

Osmose commented Nov 21, 2012

Looks good, although some tests for the cache logic would be nice. :D

r+

@pascalchevrel
Copy link
Copy Markdown
Member

FYI, The .lang files for python templates are all updated with the ## active ## tag

@pmclanahan pmclanahan merged commit 35cefae into mozilla:master Nov 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants