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.
2018-10-07 20:03:41 +02:00
parent 7646b9f166
commit 71601bd3f8
8 changed files with 101 additions and 87 deletions
--- a/app/assets/javascripts/globalize.js.coffee
+++ b/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")
--- a/app/controllers/admin/banners_controller.rb
+++ b/app/controllers/admin/banners_controller.rb
@@ -38,10 +38,9 @@ class Admin::BannersController < Admin::BaseController
  private

    def banner_params
-      attributes = [:title, :description, :target_url,
-                    :post_started_at, :post_ended_at,
+      attributes = [:target_url, :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
--- a/app/controllers/concerns/translatable.rb
+++ b/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]
-
-      resource_model.translated_attribute_names.product(enabled_translations).map do |attr_name, loc|
-        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
+      {
+        translations_attributes: [:id, :_destroy, :locale] +
+                                 resource_model.translated_attribute_names
+      }
    end

    def enabled_translations
--- a/app/helpers/translatable_form_helper.rb
+++ b/app/helpers/translatable_form_helper.rb
@@ -1,13 +1,6 @@
 module TranslatableFormHelper
-  def translatable_form_for(record_or_record_path, options = {})
-    object = record_or_record_path.is_a?(Array) ? record_or_record_path.last : record_or_record_path
-
-    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
-
+  def translatable_form_for(record, options = {})
+    form_for(record, options.merge(builder: TranslatableFormBuilder)) do |f|
      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
-          @object.globalize_locales.each do |locale|
-            Globalize.with_locale(locale) do
-              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
+        locale = options.delete(:locale)
+        final_options = @template.merge_translatable_field_options(options, locale)
+        send(field_type, method, final_options)
      end
  end
 end
--- a/app/models/banner.rb
+++ b/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
--- a/app/views/admin/banners/_form.html.erb
+++ b/app/views/admin/banners/_form.html.erb
@@ -28,13 +28,26 @@
  </div>

  <div class="row">
-    <div class="small-12 medium-6 column">
-      <%= f.translatable_text_field :title,
-                                    placeholder: t("admin.banners.banner.title"),
-                                    data: {js_banner_title: "js_banner_title"},
-                                    label: t("admin.banners.banner.title") %>
-    </div>
+    <%= f.translatable_fields do |translations_form, locale| %>
+      <div class="small-12 medium-6 column">
+        <%= translations_form.translatable_text_field :title,
+          locale: locale,
+          placeholder: t("admin.banners.banner.title"),
+          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") %>
--- a/spec/features/admin/banners_spec.rb
+++ b/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 'banner_description_en', with: 'many text wow link'
+    fill_in 'Title', with: 'Such banner'
+    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 'banner_description_en', with: 'Edited text'
+    fill_in 'Title', with: 'Modified title'
+    fill_in 'Description', with: 'Edited text'

    page.find("body").click

--- a/spec/shared/features/translatable.rb
+++ b/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|
-        translatable.dup.tap { |duplicate| duplicate.send(:"#{field}=", '') }.valid?
-      end
+      skip("can't have translatable blank fields") if optional_fields.empty?

-      skip("can't have translatable blank fields") if possible_blanks.empty?
-
-      field = possible_blanks.sample
+      field = optional_fields.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