From 3aa53449c863e49b13c9b881f7113be3f9f8e68a Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Wed, 1 Aug 2018 13:49:30 +0200 Subject: [PATCH 1/7] Fix Translatable when field values are changed to blank If we ignore all params that are blank, there is no way to "remove" an attribute (i.e. change its value to blank) On the other hand, we don't want to create new translations where all fields are empty, so the new code keeps only the blank fields which belong to existing translations. --- app/controllers/concerns/translatable.rb | 13 ++++++++++--- spec/features/translations_spec.rb | 12 ++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/translatable.rb b/app/controllers/concerns/translatable.rb index 5c3b1be62..21dab63f3 100644 --- a/app/controllers/concerns/translatable.rb +++ b/app/controllers/concerns/translatable.rb @@ -8,12 +8,15 @@ module Translatable private def translation_params(params) - resource_model.globalize_attribute_names.select { |k, v| params.include?(k.to_sym) && params[k].present? } + resource_model + .globalize_attribute_names + .select { |k| params[k].present? || + resource_model.translated_locales.include?(get_locale_from_attribute(k)) } end def delete_translations - locales = resource_model.globalize_locales. - select { |k, v| params[:delete_translations].include?(k.to_sym) && params[:delete_translations][k] == "1" } + locales = resource_model.translated_locales + .select { |l| params.dig(:delete_translations, l) == "1" } locales.each do |l| Globalize.with_locale(l) do resource.translation.destroy @@ -21,4 +24,8 @@ module Translatable end end + def get_locale_from_attribute(attribute_name) + locales = resource_model.globalize_locales + attribute_name.to_s.match(/(#{locales.join('|')})\Z/)&.captures&.first + end end diff --git a/spec/features/translations_spec.rb b/spec/features/translations_spec.rb index 4ff482ee4..4ac85bc4d 100644 --- a/spec/features/translations_spec.rb +++ b/spec/features/translations_spec.rb @@ -71,6 +71,18 @@ feature "Translations" do expect(page).not_to have_link "Español" end + scenario 'Change value of a translated field to blank' do + milestone.update_attributes!(status: create(:budget_investment_status)) + visit @edit_milestone_url + + fill_in 'budget_investment_milestone_description_en', with: '' + + click_button "Update milestone" + expect(page).to have_content "Milestone updated successfully" + + expect(milestone.reload.description).to be_blank + end + context "Globalize javascript interface" do scenario "Highlight current locale", :js do From cb716e07d779d03e9d7c9c33195ab7e6b642b96e Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Fri, 3 Aug 2018 15:52:25 +0200 Subject: [PATCH 2/7] Extract translation helper from translatable form views So that individual form partials don't depend on the implementation of how translations are deleted. --- app/helpers/globalize_helper.rb | 12 ++++++++++++ app/helpers/site_customization_helper.rb | 6 +++++- app/views/admin/banners/_form.html.erb | 4 +--- .../budget_investment_milestones/_form.html.erb | 3 ++- .../information_texts/_form.html.erb | 2 +- 5 files changed, 21 insertions(+), 6 deletions(-) diff --git a/app/helpers/globalize_helper.rb b/app/helpers/globalize_helper.rb index b64c1ff09..fac263a1b 100644 --- a/app/helpers/globalize_helper.rb +++ b/app/helpers/globalize_helper.rb @@ -14,6 +14,18 @@ module GlobalizeHelper same_locale?(neutral_locale(I18n.locale), neutral_locale(locale)) ? "" : "display: none" end + def render_translations_to_delete(resource) + capture do + resource.globalize_locales.each do |locale| + concat translation_enabled_tag(locale, enable_locale?(resource, locale)) + end + end + end + + def translation_enabled_tag(locale, enabled) + hidden_field_tag("enabled_translations[#{locale}]", (enabled ? 1 : 0)) + end + def css_to_display_translation?(resource, locale) resource.translated_locales.include?(neutral_locale(locale)) || locale == I18n.locale ? "" : "display: none" end diff --git a/app/helpers/site_customization_helper.rb b/app/helpers/site_customization_helper.rb index d9318fd9b..9e54fbf4e 100644 --- a/app/helpers/site_customization_helper.rb +++ b/app/helpers/site_customization_helper.rb @@ -1,5 +1,9 @@ module SiteCustomizationHelper + def site_customization_enable_translation?(locale) + I18nContentTranslation.existing_languages.include?(neutral_locale(locale)) || locale == I18n.locale + end + def site_customization_display_translation?(locale) - I18nContentTranslation.existing_languages.include?(neutral_locale(locale)) || locale == I18n.locale ? "" : "display: none" + site_customization_enable_translation?(locale) ? "" : "display: none" end end diff --git a/app/views/admin/banners/_form.html.erb b/app/views/admin/banners/_form.html.erb index 495cc5e8a..105a872fb 100644 --- a/app/views/admin/banners/_form.html.erb +++ b/app/views/admin/banners/_form.html.erb @@ -4,9 +4,7 @@ <%= render 'errors' %> - <% @banner.globalize_locales.each do |locale| %> - <%= hidden_field_tag "delete_translations[#{locale}]", 0 %> - <% end %> + <%= render_translations_to_delete(@banner) %>
<% date_started_at = @banner.post_started_at.present? ? I18n.localize(@banner.post_started_at) : "" %> diff --git a/app/views/admin/budget_investment_milestones/_form.html.erb b/app/views/admin/budget_investment_milestones/_form.html.erb index d39c736dd..5598a51cd 100644 --- a/app/views/admin/budget_investment_milestones/_form.html.erb +++ b/app/views/admin/budget_investment_milestones/_form.html.erb @@ -2,6 +2,8 @@ <%= form_for [:admin, @investment.budget, @investment, @milestone] do |f| %> + <%= render_translations_to_delete(@milestone) %> + <%= f.hidden_field :title, value: l(Time.current, format: :datetime), maxlength: Budget::Investment::Milestone.title_max_length %> @@ -16,7 +18,6 @@ <%= f.label :description, t("admin.milestones.new.description") %> <% @milestone.globalize_locales.each do |locale| %> - <%= hidden_field_tag "delete_translations[#{locale}]", 0 %> <% globalize(locale) do %> <%= f.text_area "description_#{locale}", rows: 5, class: "js-globalize-attribute", diff --git a/app/views/admin/site_customization/information_texts/_form.html.erb b/app/views/admin/site_customization/information_texts/_form.html.erb index 861315691..f31aa17f4 100644 --- a/app/views/admin/site_customization/information_texts/_form.html.erb +++ b/app/views/admin/site_customization/information_texts/_form.html.erb @@ -2,7 +2,7 @@ <%= form_tag admin_site_customization_information_texts_path do %> <% I18n.available_locales.each do |l| %> - <%= hidden_field_tag "delete_translations[#{l}]", 0 %> + <%= translation_enabled_tag l, site_customization_enable_translation?(l) %> <% end %> <% contents.each do |group| %> <% group.each do |content| %> From 4603a30f955d7fb62dbfdc6889eb017cdd4b9f96 Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Mon, 6 Aug 2018 16:21:53 +0200 Subject: [PATCH 3/7] Change Translatable impl to keep track of enabled locales --- app/assets/javascripts/globalize.js.coffee | 9 +++++++- .../information_texts_controller.rb | 3 ++- app/controllers/concerns/translatable.rb | 22 +++++++++++-------- app/helpers/globalize_helper.rb | 6 ++++- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/globalize.js.coffee b/app/assets/javascripts/globalize.js.coffee index 0de799318..027ac498e 100644 --- a/app/assets/javascripts/globalize.js.coffee +++ b/app/assets/javascripts/globalize.js.coffee @@ -1,6 +1,7 @@ App.Globalize = display_locale: (locale) -> + App.Globalize.enable_locale(locale) $(".js-globalize-locale-link").each -> if $(this).data("locale") == locale $(this).show() @@ -27,7 +28,13 @@ App.Globalize = next = $(".js-globalize-locale-link:visible").first() App.Globalize.highlight_locale(next) App.Globalize.display_translations(next.data("locale")) - $("#delete_translations_" + locale).val(1) + App.Globalize.disable_locale(locale) + + enable_locale: (locale) -> + $("#enabled_translations_" + locale).val(1) + + disable_locale: (locale) -> + $("#enabled_translations_" + locale).val(0) initialize: -> $('.js-globalize-locale').on 'change', -> diff --git a/app/controllers/admin/site_customization/information_texts_controller.rb b/app/controllers/admin/site_customization/information_texts_controller.rb index 25594fc4a..5da7293c1 100644 --- a/app/controllers/admin/site_customization/information_texts_controller.rb +++ b/app/controllers/admin/site_customization/information_texts_controller.rb @@ -51,7 +51,8 @@ class Admin::SiteCustomization::InformationTextsController < Admin::SiteCustomiz end def delete_translations - languages_to_delete = params[:delete_translations].select { |k, v| params[:delete_translations][k] == '1' }.keys + languages_to_delete = params[:enabled_translations].select { |_, v| v == '0' } + .keys languages_to_delete.each do |locale| I18nContentTranslation.destroy_all(locale: locale) end diff --git a/app/controllers/concerns/translatable.rb b/app/controllers/concerns/translatable.rb index 21dab63f3..3e357a56d 100644 --- a/app/controllers/concerns/translatable.rb +++ b/app/controllers/concerns/translatable.rb @@ -7,16 +7,19 @@ module Translatable private - def translation_params(params) - resource_model - .globalize_attribute_names - .select { |k| params[k].present? || - resource_model.translated_locales.include?(get_locale_from_attribute(k)) } + # TODO change method interface to remove unnecessary argument + def translation_params(_) + enabled_translations.flat_map do |locale| + resource_model.translated_attribute_names.map do |attr_name| + resource_model.localized_attr_name_for(attr_name, locale) + end + end.tap { |x| Rails.logger.debug "permitted translation params:"; p x} end + # TODO change to resource def delete_translations locales = resource_model.translated_locales - .select { |l| params.dig(:delete_translations, l) == "1" } + .select { |l| params.dig(:enabled_translations, l) == "0" } locales.each do |l| Globalize.with_locale(l) do resource.translation.destroy @@ -24,8 +27,9 @@ module Translatable end end - def get_locale_from_attribute(attribute_name) - locales = resource_model.globalize_locales - attribute_name.to_s.match(/(#{locales.join('|')})\Z/)&.captures&.first + def enabled_translations + params.fetch(:enabled_translations) + .select { |_, v| v == '1' } + .keys end end diff --git a/app/helpers/globalize_helper.rb b/app/helpers/globalize_helper.rb index fac263a1b..3dcd9520d 100644 --- a/app/helpers/globalize_helper.rb +++ b/app/helpers/globalize_helper.rb @@ -27,7 +27,11 @@ module GlobalizeHelper end def css_to_display_translation?(resource, locale) - resource.translated_locales.include?(neutral_locale(locale)) || locale == I18n.locale ? "" : "display: none" + enable_locale?(resource, locale) ? "" : "display: none" + end + + def enable_locale?(resource, locale) + resource.translated_locales.include?(neutral_locale(locale)) || locale == I18n.locale end def highlight_current?(locale) From 3c4f221e049801091b84660a4a8385717be19356 Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Mon, 6 Aug 2018 16:04:52 +0200 Subject: [PATCH 4/7] Change Translatable interface --- app/controllers/admin/banners_controller.rb | 7 ++----- .../budget_investment_milestones_controller.rb | 7 ++----- .../information_texts_controller.rb | 4 ++-- app/controllers/concerns/translatable.rb | 15 +++++++-------- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/controllers/admin/banners_controller.rb b/app/controllers/admin/banners_controller.rb index 7d3773e24..b17c17d1b 100644 --- a/app/controllers/admin/banners_controller.rb +++ b/app/controllers/admin/banners_controller.rb @@ -41,8 +41,9 @@ class Admin::BannersController < Admin::BaseController attributes = [:title, :description, :target_url, :post_started_at, :post_ended_at, :background_color, :font_color, + *translation_params(Banner), web_section_ids: []] - params.require(:banner).permit(*attributes, *translation_params(params[:banner])) + params.require(:banner).permit(*attributes) end def banner_styles @@ -65,8 +66,4 @@ class Admin::BannersController < Admin::BaseController @banner = Banner.find(params[:id]) unless @banner @banner end - - def resource_model - Banner - end end diff --git a/app/controllers/admin/budget_investment_milestones_controller.rb b/app/controllers/admin/budget_investment_milestones_controller.rb index d54dc0174..49f2df5bf 100644 --- a/app/controllers/admin/budget_investment_milestones_controller.rb +++ b/app/controllers/admin/budget_investment_milestones_controller.rb @@ -47,9 +47,10 @@ class Admin::BudgetInvestmentMilestonesController < Admin::BaseController image_attributes = [:id, :title, :attachment, :cached_attachment, :user_id, :_destroy] documents_attributes = [:id, :title, :attachment, :cached_attachment, :user_id, :_destroy] attributes = [:title, :description, :publication_date, :budget_investment_id, :status_id, + *translation_params(Budget::Investment::Milestone), image_attributes: image_attributes, documents_attributes: documents_attributes] - params.require(:budget_investment_milestone).permit(*attributes, translation_params(params[:budget_investment_milestone])) + params.require(:budget_investment_milestone).permit(*attributes) end def load_budget_investment @@ -64,10 +65,6 @@ class Admin::BudgetInvestmentMilestonesController < Admin::BaseController Budget::Investment::Milestone.find(params[:id]) end - def resource_model - Budget::Investment::Milestone - end - def resource get_milestone end diff --git a/app/controllers/admin/site_customization/information_texts_controller.rb b/app/controllers/admin/site_customization/information_texts_controller.rb index 5da7293c1..b1ab89469 100644 --- a/app/controllers/admin/site_customization/information_texts_controller.rb +++ b/app/controllers/admin/site_customization/information_texts_controller.rb @@ -9,7 +9,7 @@ class Admin::SiteCustomization::InformationTextsController < Admin::SiteCustomiz def update content_params.each do |content| - values = content[:values].slice(*translation_params(content[:values])) + values = content[:values].slice(*translation_params(I18nContent)) unless values.empty? values.each do |key, value| @@ -43,7 +43,7 @@ class Admin::SiteCustomization::InformationTextsController < Admin::SiteCustomiz end def resource - resource_model.find(content_params[:id]) + I18nContent.find(content_params[:id]) end def content_params diff --git a/app/controllers/concerns/translatable.rb b/app/controllers/concerns/translatable.rb index 3e357a56d..2350817d4 100644 --- a/app/controllers/concerns/translatable.rb +++ b/app/controllers/concerns/translatable.rb @@ -7,19 +7,18 @@ module Translatable private - # TODO change method interface to remove unnecessary argument - def translation_params(_) - enabled_translations.flat_map do |locale| + def translation_params(resource_model) + enabled_translations.flat_map do |loc| resource_model.translated_attribute_names.map do |attr_name| - resource_model.localized_attr_name_for(attr_name, locale) + resource_model.localized_attr_name_for(attr_name, loc) end - end.tap { |x| Rails.logger.debug "permitted translation params:"; p x} + end end - # TODO change to resource def delete_translations - locales = resource_model.translated_locales - .select { |l| params.dig(:enabled_translations, l) == "0" } + locales = resource.translated_locales + .select { |l| params.dig(:enabled_translations, l) == "0" } + locales.each do |l| Globalize.with_locale(l) do resource.translation.destroy From 54d00681342a06856e5244b28424bd394cd852ec Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Mon, 6 Aug 2018 16:29:15 +0200 Subject: [PATCH 5/7] Remove unused code This method has never been used as far as I've been able to tell from the git history. --- .../site_customization/information_texts_controller.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/app/controllers/admin/site_customization/information_texts_controller.rb b/app/controllers/admin/site_customization/information_texts_controller.rb index b1ab89469..32483d5ed 100644 --- a/app/controllers/admin/site_customization/information_texts_controller.rb +++ b/app/controllers/admin/site_customization/information_texts_controller.rb @@ -33,15 +33,6 @@ class Admin::SiteCustomization::InformationTextsController < Admin::SiteCustomiz private - def i18n_content_params - attributes = [:key, :value] - params.require(:information_texts).permit(*attributes, translation_params(params[:information_texts])) - end - - def resource_model - I18nContent - end - def resource I18nContent.find(content_params[:id]) end From 9fdc8a8a35c40de315b3fa4628409d35b5085fe5 Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Wed, 15 Aug 2018 11:25:57 +0200 Subject: [PATCH 6/7] Avoid checking DB records in feature spec --- spec/features/translations_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/translations_spec.rb b/spec/features/translations_spec.rb index 4ac85bc4d..7077a9a4b 100644 --- a/spec/features/translations_spec.rb +++ b/spec/features/translations_spec.rb @@ -72,7 +72,6 @@ feature "Translations" do end scenario 'Change value of a translated field to blank' do - milestone.update_attributes!(status: create(:budget_investment_status)) visit @edit_milestone_url fill_in 'budget_investment_milestone_description_en', with: '' @@ -80,7 +79,8 @@ feature "Translations" do click_button "Update milestone" expect(page).to have_content "Milestone updated successfully" - expect(milestone.reload.description).to be_blank + expect(page).to have_content "Milestone updated successfully" + expect(page).not_to have_content "Description in English" end context "Globalize javascript interface" do From 96b798865a89efdd51445f488cc4272f0c6dc777 Mon Sep 17 00:00:00 2001 From: Marko Lovic Date: Thu, 16 Aug 2018 13:37:14 +0200 Subject: [PATCH 7/7] Refactor Translatable#translation_params to improve readability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code by Javier Martín --- app/controllers/concerns/translatable.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/translatable.rb b/app/controllers/concerns/translatable.rb index 2350817d4..aecf42fb4 100644 --- a/app/controllers/concerns/translatable.rb +++ b/app/controllers/concerns/translatable.rb @@ -8,10 +8,8 @@ module Translatable private def translation_params(resource_model) - enabled_translations.flat_map do |loc| - resource_model.translated_attribute_names.map do |attr_name| - resource_model.localized_attr_name_for(attr_name, loc) - end + resource_model.translated_attribute_names.product(enabled_translations).map do |attr_name, loc| + resource_model.localized_attr_name_for(attr_name, loc) end end