Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First step to internationalization #5760

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Conversation

shipko
Copy link
Contributor

@shipko shipko commented Jan 16, 2022

Hi guys!

I want to add localization in project.
Why it need?
This will make it possible to translate the interface on other language. If company needs interface on other language - it can generate locale file.
On first step I decided prepare base.html file. If my pull request will be interesting for you, I continue work

@github-actions github-actions bot added the ui label Jan 16, 2022
@damiencarol
Copy link
Contributor

@shipko Don't you need a localization file to match the strings to localize?

@devGregA
Copy link
Contributor

Hi @shipko nice to meet you! I've always wanted to pursue multilingual support. Wouldn't it be better to start with the models rather than the templates?

@shipko
Copy link
Contributor Author

shipko commented Jan 17, 2022

@damiencarol on first step it's not require. Django will use words wrapped in translatization function.

If somebody wants to use other language can execute django-admin makemessages -l , replace LANGUAGE_CODE in settings.dist.py and translate language file

@shipko
Copy link
Contributor Author

shipko commented Jan 17, 2022

@devGregA of course. This not last commit. After that I will start edit models.py

@shipko
Copy link
Contributor Author

shipko commented Jan 18, 2022

@devGregA wrapped models.py to translatization form.

@damiencarol
Copy link
Contributor

wow. this will be a big DB migration. (every model that have some description)

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@shipko
Copy link
Contributor Author

shipko commented Jan 21, 2022

That's all @devGregA @damiencarol
I edited base.html and models.py
For the beginning I think that completely done first step for i18n

Also i notice that no required migration, because we changed 'verbose_name' and 'help_text' parameters.

Please approve my PR or write that I need to fix. Because I don't want to resolve merge conflicts in soon

@damiencarol
Copy link
Contributor

damiencarol commented Jan 21, 2022

@shipko could you add some languages files? I want to test this PR (adding a new language, changing the data, etc..)

@shipko
Copy link
Contributor Author

shipko commented Jan 23, 2022

@damiencarol of course!
This is test branch where I added some required files for testing.
This files changed only some elements in left bar menu. Another elements should be translated in /dojo/locale/ru/LC_MESSAGES/django.po file.

I want to notice, that this files from test branch will not be merged in dev branch.

@shipko
Copy link
Contributor Author

shipko commented Jan 30, 2022

@damiencarol you have news?

@damiencarol
Copy link
Contributor

I'm still testing. I need to catch-up with the team as it will impact the whole UI.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added apiv2 docker docs New Migration Adding a new migration file. Take care when merging. parser settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR labels Feb 10, 2022
@shipko
Copy link
Contributor Author

shipko commented Feb 13, 2022

@damiencarol I agree with @valentijnscholten and want to use _() construction. Because it's ~95% cases. For another cases we will be use another functions (ngettext(), gettext_lazy(), ...)

If you're ok about this, I won't change anything

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@damiencarol
Copy link
Contributor

@shipko ok.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@shipko
Copy link
Contributor Author

shipko commented Feb 19, 2022

@damiencarol @devGregA @valentijnscholten please leave feedback about this pr. I constantly resolve merge conflicts. It's boring

@valentijnscholten
Copy link
Member

valentijnscholten commented Feb 19, 2022

I understand. I have no experience with i18n, especially not in Django. I guess if this PR follows the docs it should be OK.
One thing I am wondering is how it would work if a string has variables/parameters/interpolation.
Something like the example in the docs:
image
But strangely enough I can't find how that would actually work in the .po file.

I've pinged the rest of the team to look at the PR, but I'm good with it.

@devGregA
Copy link
Contributor

@shipko looking at this now

@devGregA
Copy link
Contributor

@Maffooch would you also please review?

@damiencarol
Copy link
Contributor

It's good enough for me as a first step. I made a couple of tests by adding a new language. I will accept it before the end of the week if everything is ok.

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@valentijnscholten valentijnscholten added this to the 2.8.0 milestone Feb 25, 2022
Copy link
Contributor

@Maffooch Maffooch left a comment

Choose a reason for hiding this comment

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

I've done a quick performance test with this to be sure making translations via the templates would not be too impacting. Django's template loader can be super slow with certain operations.

I found that there really is no discernible difference, and that this is the standard way of doing django internalization. Approving for now

@valentijnscholten valentijnscholten removed this from the 2.8.0 milestone Mar 1, 2022
@shipko
Copy link
Contributor Author

shipko commented Mar 4, 2022

@Maffooch thanks!

@damiencarol @valentijnscholten @devGregA what about you?

@valentijnscholten valentijnscholten merged commit 6d2cb31 into DefectDojo:dev Mar 4, 2022
@shipko shipko deleted the translate branch April 29, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants