From d3422acbb7d33b10263f164deefbcf7a9ceba376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 7 Jun 2019 16:45:14 +0200 Subject: [PATCH 1/5] Add validation to check translations amount on updates In order to not allow users to remove all persited translations from any resource. A few exceptions were added: * Does not apply to globalizable models without translatable attributes required * Make a copy of main model error on current translations to be more realistic --- app/models/concerns/globalizable.rb | 40 +++++++++++++++++++++++ config/locales/en/activerecord.yml | 1 + config/locales/es/activerecord.yml | 1 + spec/shared/features/edit_translatable.rb | 13 ++++++++ 4 files changed, 55 insertions(+) diff --git a/app/models/concerns/globalizable.rb b/app/models/concerns/globalizable.rb index 3523f1c29..96f6340c9 100644 --- a/app/models/concerns/globalizable.rb +++ b/app/models/concerns/globalizable.rb @@ -1,4 +1,5 @@ module Globalizable + MIN_TRANSLATIONS = 1 extend ActiveSupport::Concern included do @@ -8,11 +9,18 @@ module Globalizable def locales_not_marked_for_destruction translations.reject(&:_destroy).map(&:locale) end + validate :check_translations_number, on: :update, if: :translations_required? + after_validation :copy_error_to_current_translation, on: :update def description self.read_attribute(:description).try :html_safe end + + def translations_required? + translated_attribute_names.any?{|attr| required_attribute?(attr)} + end + if self.paranoid? && translation_class.attribute_names.include?("hidden_at") translation_class.send :acts_as_paranoid, column: :hidden_at end @@ -21,6 +29,38 @@ module Globalizable private + def required_attribute?(attribute) + presence_validators = [ActiveModel::Validations::PresenceValidator, + ActiveRecord::Validations::PresenceValidator] + + attribute_validators(attribute).any?{|validator| presence_validators.include? validator } + end + + def attribute_validators(attribute) + self.class.validators_on(attribute).map(&:class) + end + + def check_translations_number + errors.add(:base, :translations_too_short) unless traslations_count_valid? + end + + def traslations_count_valid? + translations.reject(&:marked_for_destruction?).count >= MIN_TRANSLATIONS + end + + def copy_error_to_current_translation + return unless errors.added?(:base, :translations_too_short) + + if locales_persisted_and_marked_for_destruction.include?(I18n.locale) + locale = I18n.locale + else + locale = locales_persisted_and_marked_for_destruction.first + end + + translation = translation_for(locale) + translation.errors.add(:base, :translations_too_short) + end + def searchable_globalized_values values = {} translations.each do |translation| diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 6e5f78f68..576436da1 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -448,6 +448,7 @@ en: valuation: cannot_comment_valuation: "You cannot comment a valuation" messages: + translations_too_short: Is mandatory to provide one translation at least record_invalid: "Validation failed: %{errors}" another_poll_active: There is another poll active for the given period restrict_dependent_destroy: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index e8d8a4efb..7935a2b46 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -450,6 +450,7 @@ es: valuation: cannot_comment_valuation: "No puedes comentar una evaluación" messages: + translations_too_short: El obligatorio proporcionar una traducción como mínimo record_invalid: "Error de validación: %{errors}" another_poll_active: Hay otra encuesta activa para este periodo. restrict_dependent_destroy: diff --git a/spec/shared/features/edit_translatable.rb b/spec/shared/features/edit_translatable.rb index 9a49f1407..c189561be 100644 --- a/spec/shared/features/edit_translatable.rb +++ b/spec/shared/features/edit_translatable.rb @@ -236,6 +236,19 @@ shared_examples "edit_translatable" do |factory_name, path_name, input_fields, t expect_not_to_have_language "Español" end + scenario "Remove all translations should show an error message", :js do + skip("can't have invalid translations") if required_fields.empty? + + visit path + + click_link "Remove language" + click_link "Remove language" + + click_button update_button_text + + expect(page).to have_content "Is mandatory to provide one translation at least" + end + scenario "Remove a translation with invalid data", :js do skip("can't have invalid translations") if required_fields.empty? From 041abe90440756e861d8ad1b193e27e9ab2108ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 7 Jun 2019 16:50:50 +0200 Subject: [PATCH 2/5] Add persisted but marked for destruction translations to logic Now we take into consideration locales persisted but marked for destruction to complete some logic and to be able to show best translations on different situations. --- app/helpers/globalize_helper.rb | 58 +++++++++++++++++++---------- app/models/concerns/globalizable.rb | 14 +++++-- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/app/helpers/globalize_helper.rb b/app/helpers/globalize_helper.rb index 74ebc5ba0..fb33ae49e 100644 --- a/app/helpers/globalize_helper.rb +++ b/app/helpers/globalize_helper.rb @@ -1,7 +1,7 @@ module GlobalizeHelper def options_for_select_language(resource) - options_for_select(available_locales(resource), first_available_locale(resource)) + options_for_select(available_locales(resource), selected_locale(resource)) end def available_locales(resource) @@ -13,20 +13,22 @@ module GlobalizeHelper def enabled_locale?(resource, locale) return site_customization_enable_translation?(locale) if resource.blank? - if resource.translations.empty? - locale == I18n.locale - else + if resource.locales_not_marked_for_destruction.any? resource.locales_not_marked_for_destruction.include?(locale) + elsif resource.locales_persisted_and_marked_for_destruction.any? + locale == first_marked_for_destruction_translation(resource) + else + locale == I18n.locale end end - def first_available_locale(resource) + def selected_locale(resource) return first_i18n_content_translation_locale if resource.blank? - if translations_for_locale?(resource, I18n.locale) - I18n.locale - elsif resource.translations.any? - resource.translations.first.locale + if resource.locales_not_marked_for_destruction.any? + first_translation(resource) + elsif resource.locales_persisted_and_marked_for_destruction.any? + first_marked_for_destruction_translation(resource) else I18n.locale end @@ -41,9 +43,24 @@ module GlobalizeHelper end end - def translations_for_locale?(resource, locale) - resource.present? && resource.translations.any? && - resource.locales_not_marked_for_destruction.include?(locale) + def first_translation(resource) + if resource.locales_not_marked_for_destruction.include? I18n.locale + I18n.locale + else + resource.locales_not_marked_for_destruction.first + end + end + + def first_marked_for_destruction_translation(resource) + if resource.locales_persisted_and_marked_for_destruction.include? I18n.locale + I18n.locale + else + resource.locales_persisted_and_marked_for_destruction.first + end + end + + def translations_for_locale?(resource) + resource.locales_not_marked_for_destruction.any? end def selected_languages_description(resource) @@ -52,7 +69,7 @@ module GlobalizeHelper def active_languages_count(resource) if resource.blank? - languages_count + no_resource_languages_count elsif resource.locales_not_marked_for_destruction.size > 0 resource.locales_not_marked_for_destruction.size else @@ -60,7 +77,7 @@ module GlobalizeHelper end end - def languages_count + def no_resource_languages_count count = I18nContentTranslation.existing_languages.count count > 0 ? count : 1 end @@ -70,11 +87,14 @@ module GlobalizeHelper end def display_translation?(resource, locale) - if !resource || resource.translations.empty? || - resource.locales_not_marked_for_destruction.include?(I18n.locale) - locale == I18n.locale + return locale == I18n.locale if resource.blank? + + if resource.locales_not_marked_for_destruction.any? + locale == first_translation(resource) + elsif resource.locales_persisted_and_marked_for_destruction.any? + locale == first_marked_for_destruction_translation(resource) else - locale == resource.translations.first.locale + locale == I18n.locale end end @@ -83,7 +103,7 @@ module GlobalizeHelper end def display_destroy_locale_link?(resource, locale) - first_available_locale(resource) == locale + selected_locale(resource) == locale end def options_for_add_language diff --git a/app/models/concerns/globalizable.rb b/app/models/concerns/globalizable.rb index 96f6340c9..9999e3502 100644 --- a/app/models/concerns/globalizable.rb +++ b/app/models/concerns/globalizable.rb @@ -6,9 +6,6 @@ module Globalizable globalize_accessors accepts_nested_attributes_for :translations, allow_destroy: true - def locales_not_marked_for_destruction - translations.reject(&:_destroy).map(&:locale) - end validate :check_translations_number, on: :update, if: :translations_required? after_validation :copy_error_to_current_translation, on: :update @@ -16,6 +13,17 @@ module Globalizable self.read_attribute(:description).try :html_safe end + def locales_not_marked_for_destruction + translations.reject(&:marked_for_destruction?).map(&:locale) + end + + def locales_marked_for_destruction + I18n.available_locales - locales_not_marked_for_destruction + end + + def locales_persisted_and_marked_for_destruction + translations.select{|t| t.persisted? && t.marked_for_destruction? }.map(&:locale) + end def translations_required? translated_attribute_names.any?{|attr| required_attribute?(attr)} From 58164ee2ecc3dcc99cb0d4a4272daf35d1de2cbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 7 Jun 2019 16:52:11 +0200 Subject: [PATCH 3/5] Show validation error at translation interface Only when present --- app/helpers/globalize_helper.rb | 11 +++++++++++ app/views/shared/_common_globalize_locales.html.erb | 1 + 2 files changed, 12 insertions(+) diff --git a/app/helpers/globalize_helper.rb b/app/helpers/globalize_helper.rb index fb33ae49e..65021b350 100644 --- a/app/helpers/globalize_helper.rb +++ b/app/helpers/globalize_helper.rb @@ -67,6 +67,17 @@ module GlobalizeHelper t("shared.translations.languages_in_use_html", count: active_languages_count(resource)) end + def select_language_error(resource) + return if resource.blank? + + current_translation = resource.translation_for(selected_locale(resource)) + if current_translation.errors.added? :base, :translations_too_short + content_tag :div, class: "small error" do + current_translation.errors[:base].join(", ") + end + end + end + def active_languages_count(resource) if resource.blank? no_resource_languages_count diff --git a/app/views/shared/_common_globalize_locales.html.erb b/app/views/shared/_common_globalize_locales.html.erb index c7250a718..43232b2b9 100644 --- a/app/views/shared/_common_globalize_locales.html.erb +++ b/app/views/shared/_common_globalize_locales.html.erb @@ -10,6 +10,7 @@ options_for_select_language(resource), prompt: t("shared.translations.select_language_prompt"), class: "js-select-language" %> + <%= select_language_error(resource) %>
<% if manage_languages %> <% I18n.available_locales.each do |locale| %> From 966b5b19ba9beac97290b0409c9772bf190c8d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 7 Jun 2019 16:55:57 +0200 Subject: [PATCH 4/5] Mark all translations that do not exist for destruction When a translation not exists yet we can mark them all for destruction by default. They already should be initialized correctly from database or via nested attributes. --- app/helpers/translatable_form_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/helpers/translatable_form_helper.rb b/app/helpers/translatable_form_helper.rb index 807fa8d6f..cfe37e490 100644 --- a/app/helpers/translatable_form_helper.rb +++ b/app/helpers/translatable_form_helper.rb @@ -64,9 +64,7 @@ module TranslatableFormHelper def new_translation_for(locale) @object.translations.new(locale: locale).tap do |translation| - unless locale == I18n.locale && no_other_translations?(translation) - translation.mark_for_destruction - end + translation.mark_for_destruction end end From 907fc18ff73134be9a62b2b75f736ecec594f449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?= Date: Fri, 7 Jun 2019 16:59:51 +0200 Subject: [PATCH 5/5] Override translations _destroy value depending of our translations managment logic During any translatable resource edit, if you remove all translations you will be redirected to same form with errors showing you one of persisted but marked to destroy translations, without this patch _destroy field value wil be true and you will no able to persist without re-addding the same language through translation interface. --- app/helpers/translatable_form_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/helpers/translatable_form_helper.rb b/app/helpers/translatable_form_helper.rb index cfe37e490..458c7a929 100644 --- a/app/helpers/translatable_form_helper.rb +++ b/app/helpers/translatable_form_helper.rb @@ -38,6 +38,7 @@ module TranslatableFormHelper @template.content_tag :div, translations_options(translations_form.object, locale) do @template.concat translations_form.hidden_field( :_destroy, + value: !@template.enabled_locale?(translations_form.object.globalized_model, locale), data: { locale: locale } )