Feat(Observations): Add automatic translations #481

santostiago · 2024-01-15T19:37:20Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
Check the commit's or even all commits' message styles matches our requested structure.
Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

Yes
No

Other information

santostiago · 2024-01-29T21:33:16Z

@tsubik I'll add the specs during the week, but the code should be close to what it is now.

tsubik

Cannot run bundle install. There is some problem with googleauth (1.9.0), not sure why. Quick fix is to just run bundle update google-cloud-translate.

tsubik

Looks really good. I still need to test it locally after setting up google cloud. For now, I'm adding a couple comments. Also, this needs rebasing (that would fix security workflow).

tsubik · 2024-01-30T15:45:31Z

app/models/observation.rb

@@ -362,4 +366,11 @@ def notify_users(users, mail_template)
 end
 end
 end
+
+ def force_translations
+ return unless PUBLISHED_STATES.include? validation_status


There is a helper function for this so you can write

return unless published?

tsubik · 2024-01-30T15:48:44Z

app/resources/v1/observation_resource.rb

 def set_modified
 user = context[:current_user]
 @model.modified_user_id = user.id
+ @model.admin_locale = user.locale


Hmm, do we need admin_locale? We can get modified_user.locale later in callback that invoking translation.

In fact, we have a lot of observations without modfied_user.

Maybe those are old ones or were imported at the beginning?

tsubik · 2024-01-30T15:50:34Z

app/services/translation_service.rb

+ )
+ end
+
+ def run(text, from, to)


We only have one other service, and it has call method, should we use the same name here?

tsubik · 2024-01-30T15:57:24Z

app/jobs/translation_job.rb

+
+ translated_fields.each do |field, translation|
+ translation.each do |locale, text|
+ I18n.locale = locale


I think it's better to use I18n.with_locale

You mean the block? Yeah, can be.

Yes, the block.

tsubik · 2024-01-30T16:00:25Z

app/jobs/translation_job.rb

+ I18n.available_locales.each do |locale|
+ next if locale == original_locale
+
+ translated_fields[field][locale] = translation_service.run(entity.send(field), original_locale, locale)


Only this line could be within block of I18n.with_locale(original_locale). Instead of changing locale globally couple lines above. I think that could be more clear what is going on here.

tsubik · 2024-01-30T16:04:09Z

db/migrate/20240128163512_add_automatic_translation_field_to_observation_translations.rb

@@ -0,0 +1,7 @@
+class AddAutomaticTranslationFieldToObservationTranslations < ActiveRecord::Migration[7.0]
+ def change
+ add_column :observation_translations, :details_translated_from, :string


All columns are translated at once, couldn't we have only one column like auto_translated_from?

I thought of it, but maybe we'd like to change this to translate just one attribute, and it would be nice to know which one.

Sure, I thought that could be the reason.

tsubik · 2024-01-30T16:19:42Z

app/admin/observation.rb

@@ -581,10 +586,16 @@ def permitted_params
 end

 f.inputs I18n.t("active_admin.shared.translated_fields") do
+ div do
+ link_to I18n.t("active_admin.observations_page.force_translations"), force_translations_admin_observation_path(resource), method: :put, class: 'button'


I think this won't work as expected. When user is in edit mode, then can make some changes and after clicking this button all changes are gone. I think checkbox "Force translation", but placed after "Translations" fields with good hint message explaining what will happen is more right in this place.

There is also problem that user using English in backend interface, but is changing something in French and wants to force translation from French not English.

Mmmm.. from the conversation with Sophie I understood she'd like to have a button to make the translation, instead of saving the whole form.

Right, but this could be bad UX for the user. I think this needs further discussion. Either adding some confirm dialog with warning that none of the changes from the form will be saved, or doing it differently. Also, there is no good way to pick the original language that will be base for translations. I think this should be selected somehow, like the button could be connected with currently selected language tab, and says for example "Force translations (from EN)".

tsubik · 2024-02-27T17:32:08Z

.env.sample

@@ -23,3 +23,6 @@ RESPONSIBLE_EMAIL=
 SEND_EMAILS_IN_DEV=false

 MAPBOX_API_KEY=
+
+GOOGLE_CLIENT_ID=


looks like only API_KEY is used, so this could be removed.

santostiago · 2024-03-23T14:25:01Z

@tsubik I added a new field on the edit form containing the language to translate from. When something is selected, when submitting the form, the translation will occur based on the selected language.
I also added a way to directly translate the field on the view page.

santostiago · 2024-03-23T14:25:50Z

It seems like all the tests are failing... But the branch was rebased from develop.
Any idea what this can be?

tsubik

I like the new way of forcing translations, but I have some errors with translating job. Not sure what is the problem (some invalid argument error coming from google translate), will check it again tomorrow (maybe something changed in gem version as I have to bundle update google-cloud-translate to even run this code). I left some other comments though.

tsubik · 2024-03-25T19:32:15Z

app/admin/observation.rb

@@ -581,10 +597,20 @@ def permitted_params
 end

 f.inputs I18n.t("active_admin.shared.translated_fields") do
+ div do


The input looks weird, but if you remove that div it looks a way better, not sure why that div is here?

tsubik · 2024-03-25T19:34:06Z

app/admin/observation.rb

@@ -581,10 +597,20 @@ def permitted_params
 end

 f.inputs I18n.t("active_admin.shared.translated_fields") do
+ div do
+ f.input :admin_locale, label: I18n.t("active_admin.observations_page.translate_from"),


I like it that it's only select input without the button to force translation.

I would hide it when observation is not published, as I see in that case force translation is not working. I would also add some hint message saying something about how it works.

Hint message like: "Set if you want to perform automatic translations of all texts from selected language"

tsubik · 2024-03-26T07:57:07Z

app/jobs/translation_job.rb

+ fields.each do |field|
+ translated_fields[field] = {}
+ I18n.available_locales.each do |locale|
+ next if locale == original_locale


Ok, I found what is the problem. It still tries to do translations like from "fr" to "fr" as when forcing translations original_locale is string and available_locales symbol, needs to be normalized.

tsubik · 2024-03-26T07:59:03Z

app/models/observation.rb

@@ -85,6 +87,7 @@ class Observation < ApplicationRecord
 ].freeze

 attr_accessor :user_type
+ attr_accessor :admin_locale


I would probably change that name to force_translations_from or similar.

tsubik

Just found some minor things, probably naming typos. Otherwise, force translations feature works great.

tsubik · 2024-04-08T17:51:55Z

app/admin/observation.rb

@@ -103,6 +103,20 @@ def permitted_params
 end
 end

+ member_action :force_translations do
+ translate_to = params[:translate_to] || I18n.locale


Hmm, shouldn't that be translate_from?

tsubik · 2024-04-08T17:52:13Z

app/admin/observation.rb

+ end
+
+ action_item :force_translations, only: :show do
+ dropdown_menu I18n.t("active_admin.observations_page.force_translations") do


tsubik · 2024-04-08T17:52:30Z

app/admin/observation.rb

+ action_item :force_translations, only: :show do
+ dropdown_menu I18n.t("active_admin.observations_page.force_translations") do
+ I18n.available_locales.each do |locale|
+ item locale, force_translations_admin_observation_path(observation, translate_to: locale)


same, should probably be translate_from.

tsubik

LGTM 👍

santostiago requested a review from tsubik January 29, 2024 21:31

santostiago self-assigned this Jan 29, 2024

tsubik reviewed Jan 30, 2024

View reviewed changes

santostiago force-pushed the feat-observations_translation branch 2 times, most recently from ed4cc21 to 5fc0a6f Compare February 2, 2024 14:05

santostiago marked this pull request as ready for review February 3, 2024 13:04

tsubik reviewed Feb 27, 2024

View reviewed changes

santostiago force-pushed the feat-observations_translation branch from 879fcc5 to edb7d21 Compare March 23, 2024 14:23

santostiago requested a review from tsubik March 23, 2024 14:23

tsubik requested changes Mar 25, 2024

View reviewed changes

tsubik reviewed Mar 26, 2024

View reviewed changes

santostiago force-pushed the feat-observations_translation branch 3 times, most recently from b0c233a to 96428c6 Compare April 5, 2024 16:50

santostiago requested a review from tsubik April 5, 2024 16:54

tsubik reviewed Apr 8, 2024

View reviewed changes

santostiago force-pushed the feat-observations_translation branch 4 times, most recently from 6409f7e to bab12ee Compare May 8, 2024 18:00

santostiago requested a review from tsubik May 8, 2024 18:02

feat(Observations): Add automatic translations

cb180c7

santostiago force-pushed the feat-observations_translation branch from bab12ee to cb180c7 Compare May 8, 2024 18:13

tsubik approved these changes May 9, 2024

View reviewed changes

santostiago merged commit e2050e8 into develop May 12, 2024
8 checks passed

santostiago deleted the feat-observations_translation branch June 3, 2024 19:47

Feat(Observations): Add automatic translations #481

Feat(Observations): Add automatic translations #481

Conversation

santostiago commented Jan 15, 2024

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

santostiago commented Jan 29, 2024

tsubik left a comment

Choose a reason for hiding this comment

tsubik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santostiago commented Mar 23, 2024

santostiago commented Mar 23, 2024 • edited Loading

tsubik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsubik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsubik left a comment

Choose a reason for hiding this comment

tsubik left a comment •

edited

Loading

santostiago commented Mar 23, 2024 •

edited

Loading

tsubik left a comment •

edited

Loading