-
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?
Changes from 23 commits
6573591
8146b2e
8fe89d5
2777556
bd20ad6
fd04a41
52471b2
5f51ad0
325fd85
0967f23
29ab8a9
e08cd19
3f14d90
754efe4
ab40270
5fe90cd
ee5104a
c2f4c39
9294acc
3378ee6
980210e
d88a281
9cfe340
74cd814
f5cad79
ba1c030
783aea0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
require "deepl" | ||
|
||
class DeeplTranslator | ||
attr_reader :resource, :field_name, :text, :target_locale, :source_locale | ||
|
||
def initialize(resource, field_name, text, target_locale, source_locale) | ||
@resource = resource | ||
@field_name = field_name | ||
@text = text | ||
@target_locale = target_locale | ||
@source_locale = source_locale | ||
end | ||
|
||
def translate | ||
return if text.blank? | ||
|
||
translation = ::DeepL.translate text, source_locale.to_s, target_locale.to_s | ||
|
||
Decidim::MachineTranslationSaveJob.perform_later( | ||
resource, | ||
field_name, | ||
target_locale, | ||
translation.text | ||
) | ||
rescue StandardError => e | ||
Rails.logger.error("[DeeplTranslator] #{e.class} - #{e.message}") | ||
nil | ||
end | ||
end |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
That would be a bonus (so if you can't do it, it's fine) to add some tests about:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I had them. About |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# frozen_string_literal: true | ||
|
||
require "spec_helper" | ||
require "deepl" | ||
|
||
module Decidim | ||
describe DeeplTranslator do | ||
let(:title) { { en: "New Title" } } | ||
let(:process) { build(:participatory_process, title:) } | ||
let(:target_locale) { "fr" } | ||
let(:source_locale) { "en" } | ||
let(:translation) { double("translation", text: "Nouveau Titre") } | ||
|
||
before do | ||
allow(Decidim).to receive(:machine_translation_service_klass).and_return(DeeplTranslator) | ||
allow(::DeepL).to receive(:translate).with(title[source_locale.to_sym], source_locale, target_locale).and_return(translation) | ||
end | ||
|
||
describe "When fields job is executed" do | ||
before do | ||
clear_enqueued_jobs | ||
end | ||
|
||
it "calls DeeplTranslator to create machine translations" do | ||
expect(DeeplTranslator).to receive(:new).with( | ||
process, | ||
"title", | ||
process["title"][source_locale], | ||
target_locale, | ||
source_locale | ||
).and_call_original | ||
|
||
process.save | ||
|
||
MachineTranslationFieldsJob.perform_now( | ||
process, | ||
"title", | ||
process["title"][source_locale], | ||
target_locale, | ||
source_locale | ||
) | ||
end | ||
end | ||
end | ||
end |
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: