Skip to content

Conversation

@timabbott
Copy link
Member

@timabbott timabbott commented Mar 31, 2016

I'm posting this to enable getting help from others on actually displaying the translations people have been doing on Transifex in Zulip. On this branch you should need to do the following:

  • run ./manage.py compilemessages to build the .mo files.
  • Do something to get Django to actually display something translated. As one can see in the code in this branch, I've been trying to hardcode russian and get Django to translate the "USERS" heading, without success thus far (though I've clearly made some progress since e.g. the middleware changes are necessary).

The Django here are fairly useful: https://docs.djangoproject.com/en/1.8/topics/i18n/translation/

@smarx
Copy link

smarx commented Mar 31, 2016

Automated message from Dropbox CLA bot

@timabbott, it looks like you've already signed the Dropbox CLA. Thanks!

@gsw945
Copy link

gsw945 commented Apr 14, 2016

How is the speed of progress of the zulip's i18n?

@timabbott
Copy link
Member Author

Well the translations themselves are going well; I haven't had a chance to spend more time debugging the Django translations system since 2 weeks ago. So help debugging this would still be appreciated; I suspect this branch is just missing some piece of configuration.

@timabbott timabbott added this to the 2016 roadmap milestone Apr 29, 2016
@umairwaheed
Copy link
Member

@timabbott I was able to run the translations without any problem on my jinja2 branch. This is what I did:

  • Copied the .po file against de from this branch to my branch.
  • Ran python manage.py compilemessages
  • Added django.middleware.locale.LocaleMiddleware middleware.
  • Tested the application by sending the Accept-Language=de header in the request. I used requests library. If I set the LANGUAGE_CODE='de' that also works.

@timabbott
Copy link
Member Author

That's great! Sounds like this will be easy once we get the jinja2 piece merged.

@umairwaheed
Copy link
Member

Let me give a shot to Django templates as well may be I can find something.

@umairwaheed
Copy link
Member

so it's working with django templates as well. I think the problem might be with i18n urls. Can I try on your branch?

@timabbott
Copy link
Member Author

timabbott commented Apr 29, 2016

Yeah, definitely! If you want to build on top of my branch, you can use tools/reset-to-pull-request or tools/fetch-rebase-pull-request to check out my code, and then push an updated version to a branch in your repository when you have something working (or open a new PR if that's easier).

@umairwaheed
Copy link
Member

Cool. I have done it manually by doing git remote add timabbot git@github.com:timabbott/zulip.git and then starting a new branch from your branch. Will the effect be same?

@timabbott
Copy link
Member Author

timabbott commented Apr 29, 2016

yep! Those tools are just a nice way to avoid having to add another user's repo to access the branches in pull requests.

@umairwaheed
Copy link
Member

got it. 👍

@umairwaheed
Copy link
Member

@timabbott, I have got it working in https://github.com/umairwaheed/zulip/tree/translations. The only problem is that tests are failing because of the language prefix in the url.

@umairwaheed
Copy link
Member

we can either update the tests to use i18n urls or we can disable i18n in testing.

@timabbott
Copy link
Member Author

Maybe check using ./tools/test-backend --nonfatal-errors to see how many failures there will be? Curious how big of an update to the tests this would be to change it

@umairwaheed
Copy link
Member

There are a lot of failures, wc -l gives 159 occurrences of Traceback word.

@timabbott
Copy link
Member Author

OK. Probably best to disable i18n in testing (probably a small tweak to test_settings.py) and we can open an issue enabling it in testing later.

@umairwaheed
Copy link
Member

Yep, works now!

@timabbott
Copy link
Member Author

Awesome! I'll clean this up (drop the reverted commits, etc.), add some docs, and update the PR.

@umairwaheed
Copy link
Member

Cool.

@timabbott timabbott force-pushed the translations branch 2 times, most recently from ac25297 to 7b148b5 Compare April 29, 2016 18:45
@umairwaheed
Copy link
Member

umairwaheed commented Apr 29, 2016

@timabbott there is a little problem. If the user is not logged in he is redirected to http://localhost:9991/en/login?next=/en/ which gives 404. It seems that '/' is missing after 'login', this works http://localhost:9991/en/login/?next=/en/ though.

@timabbott
Copy link
Member Author

timabbott commented Apr 29, 2016

Interesting, the issue is probably in zulip_login_required in zerver/decorator.py (we just started forking that and the few functions its calls from what's built-in in Django recently). Can you investigate?

@umairwaheed
Copy link
Member

OK, checking it.

@timabbott
Copy link
Member Author

timabbott commented Apr 29, 2016

I guess the things we need to do to finish this PR are:

  • Fix the /login bug
  • document this in the Zulip features docs
  • add at least one test for the i18n support; might make a new test file zerver/tests/test_i18n.py for this. We can use with setting() to enable i18n for just those tests
  • Add tooling to generate the .mo files as part of the production release static asset build process (tools/build-release-tarball) and document that.

@umairwaheed
Copy link
Member

The login bug is fixed :). Just need to append '/' to HOME_NOT_LOGGED_IN setting in zproject/settings.py

@umairwaheed
Copy link
Member

  • document this in the Zulip features docs
  • add at least one test for the i18n support; might make a new test file zerver/tests/test_i18n.py for this. We can use with setting() to enable i18n for just those tests
  • Add tooling to generate the .mo files as part of the static asset build process and document that.

Sure. Let me know if I can work on any of these.

(Pushed the fix for login bug to my translations branch)

@timabbott
Copy link
Member Author

I think that should be the same; the Travis CI code just runs the scripts under tools/travis; first e.g. the setup-backend script to install dependencies and then the backend script. So it should basically just run test-backend.

@umairwaheed
Copy link
Member

Checking it.

@umairwaheed
Copy link
Member

One question -- should we be using i18n_patterns in any other urls.py blocks? I think maybe not in zproject/urls.py but we should be using it for the other apps (corporate/, analytics/, and zilencer/)?

There is a limitation we can only use i18n_patterns in the root url conf. Yeah, I'll add i18n urls for other apps as well.

@umairwaheed
Copy link
Member

@timabbott there are few problems I am seeing with this branch. Some static files are not being loaded correctly because the url starts with static rather than /static as a result the effective url becomes /en/static. There is a problem with logout as well.

@umairwaheed
Copy link
Member

@timabbott, it seems that URL regexes are compiled only once and as a result either i18n tests or regular tests fail depending on the value of USE_I18N at the time of compilation i.e. if we run i18n tests first then regular tests fail and if we run regular tests first then i18n tests fail. In isolation both i18n and regular tests pass. I haven't yet been able to find the code which is responsible for this behaviour.

@timabbott
Copy link
Member Author

Hmm. I wonder if we can do something where we leave the /en/ off the urls.py for the versions of pages in English, so we don't need to break the tests (and existing URLs for English speakers) to have i18n enabled?

Would it work to use a similar trick to v1_api_and_json_patterns to put those URL blocks in both a normal patterns and an i18n_patterns ?

@umairwaheed
Copy link
Member

I think we can just use the Accept-Language header and the cookie for now. That way we won't have to use the i18n urls.

@umairwaheed
Copy link
Member

umairwaheed commented May 5, 2016

@timabbott, now i18n works through session, cookie and http header.

@umairwaheed
Copy link
Member

I have used your technique to support i18n urls, the backend tests work, the frontend tests fail and the issue with static urls still a problem.

@umairwaheed
Copy link
Member

umairwaheed commented May 5, 2016

@timabbott this is done. Now we support i18n urls using your technique. All the tests pass as well.
branch: https://github.com/umairwaheed/zulip/tree/translations

@timabbott
Copy link
Member Author

@umairwaheed awesome! What do I need to do to test this manually? Actually maybe it's worth adding documentation on how to do that in a new section on "testing translations" in http://zulip.readthedocs.io/en/latest/translating.html ?

@umairwaheed
Copy link
Member

The simplest way to test is to use i18n urls.

Then you can also test by sending Accept-Lanuage=de or Accept-Language=sr header in the HTTP request to /login/. This one you can test using requests or urllib in Python CLI.

@umairwaheed
Copy link
Member

Actually maybe it's worth adding documentation on how to do that in a new section on "testing translations" in http://zulip.readthedocs.io/en/latest/translating.html ?

Sure, doing that now.

@umairwaheed
Copy link
Member

@timabbott, I have updated the documentation.

@timabbott timabbott force-pushed the translations branch 4 times, most recently from cbe773d to 460ed75 Compare May 8, 2016 00:04
@timabbott
Copy link
Member Author

timabbott commented May 18, 2016

@umairwaheed I merged half of this branch; I'd like to merge the rest but I've been having issues getting the Transifex API to actually download the latest versions of the .po files so we can merge an up-to-date version of the strings. I've invited you to have Transifex access so you can investigate; what I was trying to do is use the Transifex API with tx pull -a as our main mechanism for updating the .po files, but it wasn't working.

I think to finish this project, we should have scripts or otherwise a clear process to do the common workflow elements of managing the translations:

  • Using (a wrapper around) tx pull to fetch the latest .po files from Transifex (I imagine we'll use this just before each release)
  • Using (a wrapper around) tx push to upload the new strings to Transifex for translating (I imagine we'll do this periodically between releases, and definitely when we have a release candidate prepared, to give translators time to translate the new strings that will be added in the release).

Maybe that's it, but if there are any additional cases, we should have a clear plan for how to handle them.

@umairwaheed
Copy link
Member

Cool, @timabbott, I am checking the Transifex API now.

@umairwaheed
Copy link
Member

umairwaheed commented May 18, 2016

@timabbott, it turns out that there was a typo in .tx/config, I have pushed the fix to umairwaheed/zulip@976f7a1, now tx pull -a works correctly.

This supports i18n using all of the following:
- I18N urls
- Session
- Cookie
- HTTP header
@timabbott
Copy link
Member Author

Oh, that was easy, thanks for tracking that down @umairwaheed!

I merged everything but the last two commits (which I squashed into one), and tested it out with a few languages the only problem is now that I've cleaned up the duplicate copies of various languages in Transifex, the Chinese translations don't work; any idea what the issue is?

@umairwaheed
Copy link
Member

@timabbott, I think this is the reason, apparently, Django uses non-standard language codes for Chinese language.

@umairwaheed
Copy link
Member

@timabbott, fixed in umairwaheed/zulip@6405a4e.

@timabbott
Copy link
Member Author

Awesome, thanks for tracking that down and finding the clean solution to script this. Merged, thanks for all your work on this @umairwaheed !

@timabbott timabbott closed this May 19, 2016
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.

4 participants