Validate translations in banners

This change forces us to use nested attributes for translations, instead
of using the more convenient `:"title_#{locale}"` methods.

On the other hand, we can use Rails' native `_destroy` attribute to
remove existing translations, so we don't have to use our custom
`delete_translations`, which was a bit buggy since it didn't consider
failed updates.

app/assets/javascripts/globalize.js.coffee

@@ -34,10 +34,13 @@ App.Globalize =
     App.Globalize.disable_locale(locale)
 
   enable_locale: (locale) ->
-    $("#enabled_translations_" + locale).val(1)
+    App.Globalize.destroy_locale_field(locale).val(false)
 
   disable_locale: (locale) ->
-    $("#enabled_translations_" + locale).val(0)
+    App.Globalize.destroy_locale_field(locale).val(true)
+
+  destroy_locale_field: (locale) ->
+    $(".destroy_locale[data-locale=" + locale + "]")
 
   refresh_visible_translations: ->
     locale = $('.js-globalize-locale-link.is-active').data("locale")

app/controllers/admin/banners_controller.rb

@@ -38,10 +38,9 @@ class Admin::BannersController < Admin::BaseController
   private
 
     def banner_params
-      attributes = [:title, :description, :target_url,
+      attributes = [:target_url, :post_started_at, :post_ended_at,
-                    :post_started_at, :post_ended_at,
                     :background_color, :font_color,
-                    *translation_params(Banner),
+                    translation_params(Banner),
                     web_section_ids: []]
       params.require(:banner).permit(*attributes)
     end

app/controllers/concerns/translatable.rb

@@ -1,29 +1,13 @@
 module Translatable
   extend ActiveSupport::Concern
 
-  included do
-    before_action :delete_translations, only: [:update]
-  end
-
   private
 
     def translation_params(resource_model)
-      return [] unless params[:enabled_translations]
+      {
-
+        translations_attributes: [:id, :_destroy, :locale] +
-      resource_model.translated_attribute_names.product(enabled_translations).map do |attr_name, loc|
+                                 resource_model.translated_attribute_names
-        resource_model.localized_attr_name_for(attr_name, loc)
+      }
-      end
-    end
-
-    def delete_translations
-      locales = resource.translated_locales
-                        .select { |l| params.dig(:enabled_translations, l) == "0" }
-
-      locales.each do |l|
-        Globalize.with_locale(l) do
-          resource.translation.destroy
-        end
-      end
     end
 
     def enabled_translations

app/helpers/translatable_form_helper.rb

@@ -1,13 +1,6 @@
 module TranslatableFormHelper
-  def translatable_form_for(record_or_record_path, options = {})
+  def translatable_form_for(record, options = {})
-    object = record_or_record_path.is_a?(Array) ? record_or_record_path.last : record_or_record_path
+    form_for(record, options.merge(builder: TranslatableFormBuilder)) do |f|
-
-    form_for(record_or_record_path, options.merge(builder: TranslatableFormBuilder)) do |f|
-
-      object.globalize_locales.each do |locale|
-        concat translation_enabled_tag(locale, enable_locale?(object, locale))
-      end
-
       yield(f)
     end
   end
@@ -39,29 +32,30 @@ module TranslatableFormHelper
       translatable_field(:cktext_area, method, options)
     end
 
+    def translatable_fields(&block)
+      @object.globalize_locales.map do |locale|
+        Globalize.with_locale(locale) do
+          fields_for(:translations, @object.translations.where(locale: locale).first_or_initialize) do |translations_form|
+            @template.concat translations_form.hidden_field(
+              :_destroy,
+              value: !@template.enable_locale?(@object, locale),
+              class: "destroy_locale",
+              data: { locale: locale })
+
+            @template.concat translations_form.hidden_field(:locale, value: locale)
+
+            yield translations_form, locale
+          end
+        end
+      end.join.html_safe
+    end
+
     private
 
       def translatable_field(field_type, method, options = {})
-        @template.capture do
+        locale = options.delete(:locale)
-          @object.globalize_locales.each do |locale|
+        final_options = @template.merge_translatable_field_options(options, locale)
-            Globalize.with_locale(locale) do
+        send(field_type, method, final_options)
-              localized_attr_name = @object.localized_attr_name_for(method, locale)
-
-              label_without_locale = @object.class.human_attribute_name(method)
-              final_options = @template.merge_translatable_field_options(options, locale)
-                                       .reverse_merge(label: label_without_locale)
-
-              if field_type == :cktext_area
-                @template.concat content_tag :div, send(field_type, localized_attr_name, final_options),
-                                             class: "js-globalize-attribute",
-                                             style: @template.display_translation?(locale),
-                                             data: { locale: locale }
-              else
-                @template.concat send(field_type, localized_attr_name, final_options)
-              end
-            end
-          end
-        end
       end
   end
 end

app/models/banner.rb

@@ -6,10 +6,13 @@ class Banner < ActiveRecord::Base
   translates :title,       touch: true
   translates :description, touch: true
   globalize_accessors
+  accepts_nested_attributes_for :translations, allow_destroy: true
+
+  translation_class.instance_eval do
+    validates :title, presence: true, length: { minimum: 2 }
+    validates :description, presence: true
+  end
 
-  validates :title, presence: true,
-                    length: { minimum: 2 }
-  validates :description, presence: true
   validates :target_url, presence: true
   validates :post_started_at, presence: true
   validates :post_ended_at, presence: true

app/views/admin/banners/_form.html.erb

@@ -28,13 +28,26 @@
   </div>
 
   <div class="row">
-    <div class="small-12 medium-6 column">
+    <%= f.translatable_fields do |translations_form, locale| %>
-      <%= f.translatable_text_field :title,
+      <div class="small-12 medium-6 column">
-                                    placeholder: t("admin.banners.banner.title"),
+        <%= translations_form.translatable_text_field :title,
-                                    data: {js_banner_title: "js_banner_title"},
+          locale: locale,
-                                    label: t("admin.banners.banner.title") %>
+          placeholder: t("admin.banners.banner.title"),
-    </div>
+          data: {js_banner_title: "js_banner_title"},
+          label: t("admin.banners.banner.title") %>
+      </div>
 
+      <div class="small-12 column">
+        <%= translations_form.translatable_text_field :description,
+          locale: locale,
+          placeholder: t("admin.banners.banner.description"),
+          data: {js_banner_description: "js_banner_description"},
+          label: t("admin.banners.banner.description")  %>
+      </div>
+    <% end %>
+  </div>
+
+  <div class="row">
     <div class="small-12 medium-6 column">
       <%= f.label :target_url, t("admin.banners.banner.target_url") %>
       <%= f.text_field :target_url,
@@ -43,15 +56,6 @@
     </div>
   </div>
 
-  <div class="row">
-    <div class="small-12 column">
-      <%= f.translatable_text_field :description,
-                                    placeholder: t("admin.banners.banner.description"),
-                                    data: {js_banner_description: "js_banner_description"},
-                                    label: t("admin.banners.banner.description")  %>
-    </div>
-  </div>
-
   <div class="row">
     <div class="small-12 column">
       <%= f.label :sections, t("admin.banners.banner.sections_label") %>

spec/features/admin/banners_spec.rb

@@ -89,8 +89,8 @@ feature 'Admin banners magement' do
 
     click_link "Create banner"
 
-    fill_in 'banner_title_en', with: 'Such banner'
+    fill_in 'Title', with: 'Such banner'
-    fill_in 'banner_description_en', with: 'many text wow link'
+    fill_in 'Description', with: 'many text wow link'
     fill_in 'banner_target_url', with: 'https://www.url.com'
     last_week = Time.current - 7.days
     next_week = Time.current + 7.days
@@ -115,7 +115,7 @@ feature 'Admin banners magement' do
 
     fill_in 'banner_background_color', with: '#850000'
     fill_in 'banner_font_color', with: '#ffb2b2'
-    fill_in 'banner_title_en', with: 'Fun with flags'
+    fill_in 'Title', with: 'Fun with flags'
 
     # This last step simulates the blur event on the page. The color pickers and the text_fields
     # has onChange events that update each one when the other changes, but this is only fired when
@@ -146,8 +146,8 @@ feature 'Admin banners magement' do
 
     click_link "Edit banner"
 
-    fill_in 'banner_title_en', with: 'Modified title'
+    fill_in 'Title', with: 'Modified title'
-    fill_in 'banner_description_en', with: 'Edited text'
+    fill_in 'Description', with: 'Edited text'
 
     page.find("body").click
 

spec/shared/features/translatable.rb

@@ -16,6 +16,16 @@ shared_examples "translatable" do |factory_name, path_name, fields|
     end.to_h
   end
 
+  let(:optional_fields) do
+    fields.select do |field|
+      translatable.translations.last.dup.tap { |duplicate| duplicate.send(:"#{field}=", "") }.valid?
+    end
+  end
+
+  let(:required_fields) do
+    fields - optional_fields
+  end
+
   let(:translatable) { create(factory_name, attributes) }
   let(:path) { send(path_name, *resource_hierarchy_for(translatable)) }
   before { login_as(create(:administrator).user) }
@@ -70,6 +80,24 @@ shared_examples "translatable" do |factory_name, path_name, fields|
       expect(page).to have_field(field_for(field, :es), with: updated_text)
     end
 
+    scenario "Update a translation with invalid data", :js do
+      skip("can't have invalid translations") if required_fields.empty?
+
+      field = required_fields.sample
+
+      visit path
+      click_link "Español"
+
+      expect(page).to have_field(field_for(field, :es), with: text_for(field, :es))
+
+      fill_in field_for(field, :es), with: ""
+      click_button update_button_text
+
+      expect(page).to have_css "#error_explanation"
+
+      # TODO: check the field is now blank.
+    end
+
     scenario "Remove a translation", :js do
       visit path
 
@@ -85,13 +113,9 @@ shared_examples "translatable" do |factory_name, path_name, fields|
     end
 
     scenario 'Change value of a translated field to blank', :js do
-      possible_blanks = fields.select do |field|
+      skip("can't have translatable blank fields") if optional_fields.empty?
-        translatable.dup.tap { |duplicate| duplicate.send(:"#{field}=", '') }.valid?
-      end
 
-      skip("can't have translatable blank fields") if possible_blanks.empty?
+      field = optional_fields.sample
-
-      field = possible_blanks.sample
 
       visit path
       expect(page).to have_field(field_for(field, :en), with: text_for(field, :en))
@@ -105,10 +129,12 @@ shared_examples "translatable" do |factory_name, path_name, fields|
 
     scenario "Add a translation for a locale with non-underscored name", :js do
       visit path
-      field = fields.sample
 
       select "Português brasileiro", from: "translation_locale"
-      fill_in field_for(field, :pt_br), with: text_for(field, :"pt-BR")
+
+      fields.each do |field|
+        fill_in field_for(field, :"pt-BR"), with: text_for(field, :"pt-BR")
+      end
 
       click_button update_button_text
 
@@ -116,7 +142,8 @@ shared_examples "translatable" do |factory_name, path_name, fields|
 
       select('Português brasileiro', from: 'locale-switcher')
 
-      expect(page).to have_field(field_for(field, :pt_br), with: text_for(field, :"pt-BR"))
+      field = fields.sample
+      expect(page).to have_field(field_for(field, :"pt-BR"), with: text_for(field, :"pt-BR"))
     end
   end
 
@@ -176,7 +203,7 @@ def field_for(field, locale)
   if translatable_class.name == "I18nContent"
     "contents_content_#{translatable.key}values_#{field}_#{locale}"
   else
-    "#{translatable_class.model_name.singular}_#{field}_#{locale}"
+    find("[data-locale='#{locale}'][id$='#{field}']")[:id]
   end
 end