-
Notifications
You must be signed in to change notification settings - Fork 14
Fix machine translation clean #876
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
base: develop
Are you sure you want to change the base?
Conversation
…activation checks
62ad22b
to
3378ee6
Compare
translation = ::DeepL.translate text, source_locale.to_s, target_locale.to_s | ||
|
||
Decidim::MachineTranslationSaveJob.perform_later( | ||
resource, | ||
field_name, | ||
target_locale, | ||
translation.text | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might consider adding a guard clause after the DeepL API call to prevent scheduling a job with empty/nil translation results:
translation = ::DeepL.translate text, source_locale.to_s, target_locale.to_s
return nil if translation.nil? || translation.text.blank?
Decidim::MachineTranslationSaveJob.perform_later(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great to include tests ! Personally, I think we're missing some test coverage here, especially around error scenarios. Since this class makes external API calls, it would be good to test what happens when:
- The translation comes back nil or empty
- We pass blank text
That would be a bonus (so if you can't do it, it's fine) to add some tests about:
- The job scheduling fails
- The DeepL API fails (auth errors, quota exceeded, etc.)
The current test only covers the happy path integration, which is great, but given the previous feedback about error handling, it would probably be worth adding some unit tests for the #translate
method to make sure errors are logged properly and don't break the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback It's great, I will add them !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had them. About The job scheduling
I think is almost test in the machine_translation_resource_job: https://github.com/decidim/decidim/blob/7d584a3cd73c93d8eb3bcdf8c0eb8e1b3562d9bf/decidim-core/spec/jobs/decidim/machine_translation_resource_job_spec.rb#L23
Do you think is still necessary here ? I'm not sure...
🎩 Description
Previously, we manually configured all DeepL integration details in our fork.
Now, that the base machine translation framework is integrated upstream in decidim/decidim,
This PR configures DeepL as the machine translation service using the new Decidim configuration system.
✅ Testing
SET UP
DEEPL_API_KEY="ta_clef_ici" bin/rails server
bundle exec sidekiq -q translations
TEST
fr
for exemple📌 Related Issues
[Link your PR to an issue
](feat: Add Machine Translation #785)
Tasks
📷 Screenshots
Please add screenshots of the changes you're proposing if related to the UI
Extra information