From d09a47a023c7208d318f37ca7f2cee16fccc7482 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 29 Jan 2023 16:27:53 +0100
Subject: [PATCH 1/5] Move remote translations concern methods to the model

This way it'll be easier to change the code.
---
 .../concerns/remotely_translatable.rb         | 20 +----------------
 app/models/remote_translation.rb              | 22 +++++++++++++++++++
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/app/controllers/concerns/remotely_translatable.rb b/app/controllers/concerns/remotely_translatable.rb
index 7508a29f3..2524566fc 100644
--- a/app/controllers/concerns/remotely_translatable.rb
+++ b/app/controllers/concerns/remotely_translatable.rb
@@ -4,25 +4,7 @@ module RemotelyTranslatable
     def detect_remote_translations(*args)
       return [] unless Setting["feature.remote_translations"].present? && api_key_has_been_set_in_secrets?
 
-      resources_groups(*args).flatten.select { |resource| translation_empty?(resource) }.map do |resource|
-        remote_translation_for(resource)
-      end
-    end
-
-    def remote_translation_for(resource)
-      { "remote_translatable_id" => resource.id.to_s,
-        "remote_translatable_type" => resource.class.to_s,
-        "locale" => I18n.locale }
-    end
-
-    def translation_empty?(resource)
-      resource.class.translates? && resource.translations.where(locale: I18n.locale).empty?
-    end
-
-    def resources_groups(*args)
-      feeds = args.find { |arg| arg&.first.class == Widget::Feed } || []
-
-      args.compact - [feeds] + feeds.map(&:items)
+      RemoteTranslation.remote_translations_for(*args)
     end
 
     def api_key_has_been_set_in_secrets?
diff --git a/app/models/remote_translation.rb b/app/models/remote_translation.rb
index 63b4cfc92..6eec7ca0a 100644
--- a/app/models/remote_translation.rb
+++ b/app/models/remote_translation.rb
@@ -19,6 +19,28 @@ class RemoteTranslation < ApplicationRecord
           error_message: nil).any?
   end
 
+  def self.remote_translations_for(*args)
+    resources_groups(*args).flatten.select { |resource| translation_empty?(resource) }.map do |resource|
+      remote_translation_for(resource)
+    end
+  end
+
+  def self.resources_groups(*args)
+    feeds = args.find { |arg| arg&.first.class == Widget::Feed } || []
+
+    args.compact - [feeds] + feeds.map(&:items)
+  end
+
+  def self.remote_translation_for(resource)
+    { "remote_translatable_id" => resource.id.to_s,
+      "remote_translatable_type" => resource.class.to_s,
+      "locale" => I18n.locale }
+  end
+
+  def self.translation_empty?(resource)
+    resource.class.translates? && resource.translations.where(locale: I18n.locale).empty?
+  end
+
   def already_translated_resource
     if remote_translatable&.translations&.where(locale: locale).present?
       errors.add(:locale, :already_translated)

From 26cc75a891224fe878f3b8e223d48e80b98f135e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 29 Jan 2023 16:47:52 +0100
Subject: [PATCH 2/5] Move remote translations controller methods to the model

Now that all the code related to this model is in the same place,
changing it will be easier.
---
 .../remote_translations_controller.rb         | 25 +++++--------------
 app/models/remote_translation.rb              |  6 +++++
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/app/controllers/remote_translations_controller.rb b/app/controllers/remote_translations_controller.rb
index b1207157c..7ff8c9602 100644
--- a/app/controllers/remote_translations_controller.rb
+++ b/app/controllers/remote_translations_controller.rb
@@ -2,34 +2,21 @@ class RemoteTranslationsController < ApplicationController
   skip_authorization_check
   respond_to :html, :js
 
-  before_action :set_remote_translations, only: :create
-
   def create
-    @remote_translations.each do |remote_translation|
-      RemoteTranslation.create!(remote_translation) unless translations_enqueued?(remote_translation)
-    end
+    RemoteTranslation.create_all(remote_translations_params)
+
     redirect_to request.referer, notice: t("remote_translations.create.enqueue_remote_translation")
   end
 
   private
 
     def remote_translations_params
-      params.permit(allowed_params)
-    end
-
-    def allowed_params
-      [:remote_translations]
-    end
-
-    def set_remote_translations
-      remote_translations = remote_translations_params["remote_translations"]
-      decoded_remote_translations = ActiveSupport::JSON.decode(remote_translations)
-      @remote_translations = decoded_remote_translations.map do |remote_translation|
-        remote_translation.slice("remote_translatable_id", "remote_translatable_type", "locale")
+      ActiveSupport::JSON.decode(params["remote_translations"]).map do |remote_translation_params|
+        remote_translation_params.slice(*allowed_params)
       end
     end
 
-    def translations_enqueued?(remote_translation)
-      RemoteTranslation.remote_translation_enqueued?(remote_translation)
+    def allowed_params
+      ["remote_translatable_id", "remote_translatable_type", "locale"]
     end
 end
diff --git a/app/models/remote_translation.rb b/app/models/remote_translation.rb
index 6eec7ca0a..97d53216c 100644
--- a/app/models/remote_translation.rb
+++ b/app/models/remote_translation.rb
@@ -41,6 +41,12 @@ class RemoteTranslation < ApplicationRecord
     resource.class.translates? && resource.translations.where(locale: I18n.locale).empty?
   end
 
+  def self.create_all(remote_translations_params)
+    remote_translations_params.each do |remote_translation_params|
+      create!(remote_translation_params) unless remote_translation_enqueued?(remote_translation_params)
+    end
+  end
+
   def already_translated_resource
     if remote_translatable&.translations&.where(locale: locale).present?
       errors.add(:locale, :already_translated)

From 2f0327acf866079f3682a4cd29a7a519d0e16e60 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 29 Jan 2023 16:30:48 +0100
Subject: [PATCH 3/5] Use remote translations objects instead of hashes

This way we can simplify the code dealing with the translatable
association.
---
 .../remote_translations_button_component.rb   |  4 +--
 .../concerns/remotely_translatable.rb         |  2 +-
 app/models/remote_translation.rb              | 29 +++++++------------
 ...mote_translations_button_component_spec.rb |  2 +-
 4 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/app/components/layout/remote_translations_button_component.rb b/app/components/layout/remote_translations_button_component.rb
index ebae39d23..44e3a380f 100644
--- a/app/components/layout/remote_translations_button_component.rb
+++ b/app/components/layout/remote_translations_button_component.rb
@@ -13,8 +13,6 @@ class Layout::RemoteTranslationsButtonComponent < ApplicationComponent
   private
 
     def display_remote_translation_button?
-      remote_translations.none? do |remote_translation|
-        RemoteTranslation.remote_translation_enqueued?(remote_translation)
-      end
+      remote_translations.none?(&:enqueued?)
     end
 end
diff --git a/app/controllers/concerns/remotely_translatable.rb b/app/controllers/concerns/remotely_translatable.rb
index 2524566fc..9c61b1ef2 100644
--- a/app/controllers/concerns/remotely_translatable.rb
+++ b/app/controllers/concerns/remotely_translatable.rb
@@ -4,7 +4,7 @@ module RemotelyTranslatable
     def detect_remote_translations(*args)
       return [] unless Setting["feature.remote_translations"].present? && api_key_has_been_set_in_secrets?
 
-      RemoteTranslation.remote_translations_for(*args)
+      RemoteTranslation.for(*args)
     end
 
     def api_key_has_been_set_in_secrets?
diff --git a/app/models/remote_translation.rb b/app/models/remote_translation.rb
index 97d53216c..b186a9d3a 100644
--- a/app/models/remote_translation.rb
+++ b/app/models/remote_translation.rb
@@ -12,16 +12,9 @@ class RemoteTranslation < ApplicationRecord
     RemoteTranslations::Caller.new(self).delay.call
   end
 
-  def self.remote_translation_enqueued?(remote_translation)
-    where(remote_translatable_id: remote_translation["remote_translatable_id"],
-          remote_translatable_type: remote_translation["remote_translatable_type"],
-          locale: remote_translation["locale"],
-          error_message: nil).any?
-  end
-
-  def self.remote_translations_for(*args)
+  def self.for(*args)
     resources_groups(*args).flatten.select { |resource| translation_empty?(resource) }.map do |resource|
-      remote_translation_for(resource)
+      new(remote_translatable: resource, locale: I18n.locale)
     end
   end
 
@@ -31,20 +24,14 @@ class RemoteTranslation < ApplicationRecord
     args.compact - [feeds] + feeds.map(&:items)
   end
 
-  def self.remote_translation_for(resource)
-    { "remote_translatable_id" => resource.id.to_s,
-      "remote_translatable_type" => resource.class.to_s,
-      "locale" => I18n.locale }
-  end
-
   def self.translation_empty?(resource)
     resource.class.translates? && resource.translations.where(locale: I18n.locale).empty?
   end
 
   def self.create_all(remote_translations_params)
-    remote_translations_params.each do |remote_translation_params|
-      create!(remote_translation_params) unless remote_translation_enqueued?(remote_translation_params)
-    end
+    remote_translations_params.map do |remote_translation_params|
+      new(remote_translation_params)
+    end.reject(&:enqueued?).each(&:save!)
   end
 
   def already_translated_resource
@@ -52,4 +39,10 @@ class RemoteTranslation < ApplicationRecord
       errors.add(:locale, :already_translated)
     end
   end
+
+  def enqueued?
+    self.class.where(remote_translatable: remote_translatable,
+                     locale: locale,
+                     error_message: nil).any?
+  end
 end
diff --git a/spec/components/layout/remote_translations_button_component_spec.rb b/spec/components/layout/remote_translations_button_component_spec.rb
index f60544500..a427b4a64 100644
--- a/spec/components/layout/remote_translations_button_component_spec.rb
+++ b/spec/components/layout/remote_translations_button_component_spec.rb
@@ -1,7 +1,7 @@
 require "rails_helper"
 
 describe Layout::RemoteTranslationsButtonComponent do
-  let(:translations) { [{}] }
+  let(:translations) { [RemoteTranslation.new] }
   let(:component) { Layout::RemoteTranslationsButtonComponent.new(translations) }
 
   before do

From fe3c9d47fa2d4c00d5095ea8c11ccbdcbbd8d3b3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 29 Jan 2023 17:54:17 +0100
Subject: [PATCH 4/5] Make the translation button condition more readable

I was finding this part hard to follow, but after changing the name of
the condition suddenly I understood what was going on.
---
 .../layout/remote_translations_button_component.html.erb    | 6 +++---
 .../layout/remote_translations_button_component.rb          | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/components/layout/remote_translations_button_component.html.erb b/app/components/layout/remote_translations_button_component.html.erb
index 99a590def..faa551128 100644
--- a/app/components/layout/remote_translations_button_component.html.erb
+++ b/app/components/layout/remote_translations_button_component.html.erb
@@ -1,11 +1,11 @@
 <div class="remote-translations-button">
-  <% if display_remote_translation_button? %>
+  <% if translations_in_progress? %>
+    <%= t("remote_translations.all_remote_translations_enqueued_text") %>
+  <% else %>
     <%= form_tag remote_translations_path do %>
       <%= hidden_field_tag :remote_translations, remote_translations.to_json %>
       <%= t("remote_translations.text") %>
       <%= submit_tag t("remote_translations.button") %>
     <% end %>
-  <% else %>
-    <%= t("remote_translations.all_remote_translations_enqueued_text") %>
   <% end %>
 </div>
diff --git a/app/components/layout/remote_translations_button_component.rb b/app/components/layout/remote_translations_button_component.rb
index 44e3a380f..593f2cdaf 100644
--- a/app/components/layout/remote_translations_button_component.rb
+++ b/app/components/layout/remote_translations_button_component.rb
@@ -12,7 +12,7 @@ class Layout::RemoteTranslationsButtonComponent < ApplicationComponent
 
   private
 
-    def display_remote_translation_button?
-      remote_translations.none?(&:enqueued?)
+    def translations_in_progress?
+      remote_translations.any?(&:enqueued?)
     end
 end

From 2b9f9ed557dfa8bf184535bf74047464c9d4aa49 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Mon, 30 Jan 2023 17:07:57 +0100
Subject: [PATCH 5/5] Fix crash translating an already translated text

The page was crashing when at least part of the content of the page had
been translated between the request showing the remote translations
button and the moment people pressed the button.
---
 app/models/remote_translation.rb            |  8 ++++--
 spec/shared/system/remotely_translatable.rb | 28 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/app/models/remote_translation.rb b/app/models/remote_translation.rb
index b186a9d3a..1ef0f893d 100644
--- a/app/models/remote_translation.rb
+++ b/app/models/remote_translation.rb
@@ -31,11 +31,11 @@ class RemoteTranslation < ApplicationRecord
   def self.create_all(remote_translations_params)
     remote_translations_params.map do |remote_translation_params|
       new(remote_translation_params)
-    end.reject(&:enqueued?).each(&:save!)
+    end.reject(&:already_translated?).reject(&:enqueued?).each(&:save!)
   end
 
   def already_translated_resource
-    if remote_translatable&.translations&.where(locale: locale).present?
+    if already_translated?
       errors.add(:locale, :already_translated)
     end
   end
@@ -45,4 +45,8 @@ class RemoteTranslation < ApplicationRecord
                      locale: locale,
                      error_message: nil).any?
   end
+
+  def already_translated?
+    remote_translatable&.translations&.where(locale: locale).present?
+  end
 end
diff --git a/spec/shared/system/remotely_translatable.rb b/spec/shared/system/remotely_translatable.rb
index c38678e46..1204b8f64 100644
--- a/spec/shared/system/remotely_translatable.rb
+++ b/spec/shared/system/remotely_translatable.rb
@@ -1,3 +1,5 @@
+require "sessions_helper"
+
 shared_examples "remotely_translatable" do |factory_name, path_name, path_arguments|
   let(:arguments) do
     path_arguments.transform_values { |path_to_value| resource.send(path_to_value) }
@@ -192,6 +194,32 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume
         expect(RemoteTranslation.count).to eq(0)
         expect(resource.translations.count).to eq(2)
       end
+
+      scenario "request a translation of an already translated text" do
+        microsoft_translate_client_response = generate_response(resource)
+        expect_any_instance_of(RemoteTranslations::Microsoft::Client).to receive(:call).and_return(microsoft_translate_client_response)
+
+        in_browser(:one) do
+          visit path
+          select "Español", from: "Language:"
+
+          expect(page).to have_button "Traducir página"
+        end
+
+        in_browser(:two) do
+          visit path
+          select "Español", from: "Language:"
+          click_button "Traducir página"
+
+          expect(page).to have_content "Se han solicitado correctamente las traducciones"
+        end
+
+        in_browser(:one) do
+          click_button "Traducir página"
+
+          expect(page).not_to have_button "Traducir página"
+        end
+      end
     end
   end
 end