Merge pull request #5073 from consul/remote_translations_crash

Fix crash translating an already translated text

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>

app/components/layout/remote_translations_button_component.rb

@@ -12,9 +12,7 @@ class Layout::RemoteTranslationsButtonComponent < ApplicationComponent
 
   private
 
-    def display_remote_translation_button?
-      remote_translations.none? do |remote_translation|
-        RemoteTranslation.remote_translation_enqueued?(remote_translation)
-      end
+    def translations_in_progress?
+      remote_translations.any?(&:enqueued?)
     end
 end

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.for(*args)
     end
 
     def api_key_has_been_set_in_secrets?

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

app/models/remote_translation.rb

@@ -12,16 +12,41 @@ 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?
+  def self.for(*args)
+    resources_groups(*args).flatten.select { |resource| translation_empty?(resource) }.map do |resource|
+      new(remote_translatable: resource, locale: I18n.locale)
+    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.translation_empty?(resource)
+    resource.class.translates? && resource.translations.where(locale: I18n.locale).empty?
+  end
+
+  def self.create_all(remote_translations_params)
+    remote_translations_params.map do |remote_translation_params|
+      new(remote_translation_params)
+    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
+
+  def enqueued?
+    self.class.where(remote_translatable: remote_translatable,
+                     locale: locale,
+                     error_message: nil).any?
+  end
+
+  def already_translated?
+    remote_translatable&.translations&.where(locale: locale).present?
+  end
 end

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

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