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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add Google Translate as MT provider #2637

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Add Google Translate as MT provider #2637

merged 1 commit into from
Feb 26, 2024

Conversation

MizukiTemma
Copy link
Member

@MizukiTemma MizukiTemma commented Feb 5, 2024 •
edited

Short description

This PR adds Google Translate as an additional machine translation provider.

Currently Google Translate is available only for the languages that are not supported by DeepL. Possibility to choose a provider per language will be introduced in #2586 .

I suggest to add tests separately as #2641 so this PR will not be extremely large after reviews 馃槄

Proposed changes

  • Add Google Translate in the same way with DeepL
  • Extend settings and integreat-cms.ini for Google Authentication
  • The hix check and budget control are there too.

Side effects

  • So far as I understood, the credential must be stored somewhere and accessed by the path given in integreat-cms.ini. I'm afraid this might bring security concerns.
  • Move check_usage and translatable_attributes to the class MachineTranslationAPIClient to avoid duplicates.

Resolved issues

Fixes: #1436

Pull Request Review Guidelines

@MizukiTemma MizukiTemma requested a review from a team as a code owner February 5, 2024 16:01
@codeclimate CodeClimate
Copy link

codeclimate bot commented Feb 5, 2024 •
edited

Code Climate has analyzed commit f57e8b2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 48.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 81.5% (-0.5% change).

View more on Code Climate.

@MizukiTemma MizukiTemma mentioned this pull request Feb 6, 2024
Closed
@MizukiTemma MizukiTemma force-pushed the google_translate branch 2 times, most recently from 5881b0d to 11ac7ca Compare February 9, 2024 09:38
@MizukiTemma MizukiTemma mentioned this pull request Feb 12, 2024
Merged
@MizukiTemma MizukiTemma added the 鈥硷笍 prio: high Needs to be resolved ASAP. label Feb 20, 2024
@MizukiTemma
Copy link
Member Author

MizukiTemma commented Feb 20, 2024 •
edited

Oh wait, renaming is needed for the command reset_deepl_budget too. I'll take care of it tomorrow 馃憖

@MizukiTemma MizukiTemma mentioned this pull request Feb 21, 2024
Open
62 tasks
david-venhoff
david-venhoff approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Awesome, I did some testing and found no problems 馃帀

So far as I understood, the credential must be stored somewhere and accessed by the path given in integreat-cms.ini. I'm afraid this might bring security concerns.

It seems like there is the option to do it without a credentials file: https://googleapis.dev/python/google-api-core/latest/auth.html#explicit-credentials

integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/core/settings.py Outdated Show resolved Hide resolved
example-configs/integreat-cms.ini Outdated Show resolved Hide resolved
integreat_cms/cms/models/regions/region.py Outdated Show resolved Hide resolved
integreat_cms/core/utils/machine_translation_api_client.py Outdated Show resolved Hide resolved
integreat_cms/deepl_api/deepl_api_client.py Outdated Show resolved Hide resolved
integreat_cms/google_translate_api/apps.py Outdated Show resolved Hide resolved
integreat_cms/google_translate_api/google_translate_api_client.py Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
@MizukiTemma
Copy link
Member Author

I'll squash the commits and resolve conflicts tomorrow.

PeterNerlich
PeterNerlich reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@PeterNerlich PeterNerlich left a comment •
edited

Choose a reason for hiding this comment

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

Didn't figure out how to inject the credentials but seeing that David already approved, I hope me testing the functionality is not essential.

I found mostly just a few typos, but have a look at my guess about cutting out the credentials file. Maybe it turns out to be right.

Also, we talk about Google Translate in the documentation and the comments, but it's actually called Google Cloud Translate. While I think it makes sense to keep that name facing the user, but I wonder whether it would be less confusing if we called it by the products actual name in the comments and docstrings.

example-configs/integreat-cms.ini Outdated Show resolved Hide resolved
integreat_cms/cms/models/regions/region.py Outdated Show resolved Hide resolved
integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/core/settings.py Outdated Show resolved Hide resolved
integreat_cms/core/utils/machine_translation_api_client.py Show resolved Hide resolved
integreat_cms/google_translate_api/google_translate_api_client.py Outdated Show resolved Hide resolved
integreat_cms/google_translate_api/apps.py Show resolved Hide resolved
@MizukiTemma MizukiTemma force-pushed the google_translate branch 3 times, most recently from de9ad66 to 7fa6893 Compare February 22, 2024 14:17
seluianova
seluianova approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

Thanks for all the fixes, I think we're all set!

@MizukiTemma MizukiTemma force-pushed the google_translate branch 3 times, most recently from 48637dd to bd42edc Compare February 22, 2024 19:19
@MizukiTemma @seluianova
@david-venhoff @PeterNerlich
Add google translate as MT provider
f57e8b2 
Co-authored-by: Tory <115008338+seluianova@users.noreply.github.com>
Co-authored-by: David Venhoff <david.venhoff@tuerantuer.org>
Co-authored-by: Peter Nerlich <PeterNerlich@users.noreply.github.com>
@MizukiTemma MizukiTemma merged commit 315da5f into develop Feb 26, 2024
5 checks passed
@MizukiTemma MizukiTemma deleted the google_translate branch February 26, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterNerlich PeterNerlich PeterNerlich left review comments

@david-venhoff david-venhoff david-venhoff approved these changes

@seluianova seluianova seluianova approved these changes

Assignees
No one assigned
Labels
鈥硷笍 prio: high Needs to be resolved ASAP.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Google Translate as additional MT provider
4 participants
@MizukiTemma @PeterNerlich @david-venhoff @seluianova