From 6278175f5767a8a681e07fa5db872cbab4d62e12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 9 Oct 2018 18:06:50 +0200 Subject: [PATCH] Update legislation drafts translatable fields Updating it required reorganizing the form so translatable fields are together. We also needed to add a `hint` option to the form label and input methods so the hint wouldn't show up for every language. Finally, the markdown editor needed to use the same globalize attributes as inputs, labels and hints, which adds a bit of duplication. --- .../javascripts/markdown_editor.js.coffee | 6 +- .../legislation/draft_versions_controller.rb | 6 +- app/helpers/translatable_form_helper.rb | 38 +++++-- app/models/legislation/draft_version.rb | 8 +- .../legislation/draft_versions/_form.html.erb | 98 ++++++++++--------- config/locales/en/activerecord.yml | 4 + config/locales/es/activerecord.yml | 4 + .../admin/legislation/draft_versions_spec.rb | 40 ++------ spec/models/legislation/draft_version_spec.rb | 1 + spec/shared/features/translatable.rb | 92 +++++++++++------ 10 files changed, 169 insertions(+), 128 deletions(-) diff --git a/app/assets/javascripts/markdown_editor.js.coffee b/app/assets/javascripts/markdown_editor.js.coffee index 29e74e51c..8e22932ab 100644 --- a/app/assets/javascripts/markdown_editor.js.coffee +++ b/app/assets/javascripts/markdown_editor.js.coffee @@ -3,12 +3,12 @@ App.MarkdownEditor = refresh_preview: (element, md) -> textarea_content = App.MarkdownEditor.find_textarea(element).val() result = md.render(textarea_content) - element.find('#markdown-preview').html(result) + element.find('.markdown-preview').html(result) # Multi-locale (translatable) form fields work by hiding inputs of locales # which are not "active". find_textarea: (editor) -> - editor.find('textarea:visible') + editor.find('textarea') initialize: -> $('.markdown-editor').each -> @@ -26,7 +26,7 @@ App.MarkdownEditor = return editor.find('textarea').on 'scroll', -> - $('#markdown-preview').scrollTop($(this).scrollTop()) + editor.find('.markdown-preview').scrollTop($(this).scrollTop()) editor.find('.fullscreen-toggle').on 'click', -> editor.toggleClass('fullscreen') diff --git a/app/controllers/admin/legislation/draft_versions_controller.rb b/app/controllers/admin/legislation/draft_versions_controller.rb index 703d8a543..4d83382aa 100644 --- a/app/controllers/admin/legislation/draft_versions_controller.rb +++ b/app/controllers/admin/legislation/draft_versions_controller.rb @@ -41,13 +41,9 @@ class Admin::Legislation::DraftVersionsController < Admin::Legislation::BaseCont def draft_version_params params.require(:legislation_draft_version).permit( - :title, - :changelog, :status, :final_version, - :body, - :body_html, - *translation_params(Legislation::DraftVersion) + translation_params(Legislation::DraftVersion) ) end diff --git a/app/helpers/translatable_form_helper.rb b/app/helpers/translatable_form_helper.rb index 7b4be72cc..868cb7028 100644 --- a/app/helpers/translatable_form_helper.rb +++ b/app/helpers/translatable_form_helper.rb @@ -10,11 +10,6 @@ module TranslatableFormHelper class: "#{options[:class]} js-globalize-attribute".strip, style: "#{options[:style]} #{display_translation?(locale)}".strip, data: options.fetch(:data, {}).merge(locale: locale), - label_options: { - class: "#{options.dig(:label_options, :class)} js-globalize-attribute".strip, - style: "#{options.dig(:label_options, :style)} #{display_translation?(locale)}".strip, - data: (options.dig(:label_options, :data) || {}) .merge(locale: locale) - } ) end @@ -56,14 +51,43 @@ module TranslatableFormHelper class TranslationsFieldsBuilder < FoundationRailsHelper::FormBuilder %i[text_field text_area cktext_area].each do |field| define_method field do |attribute, options = {}| - super attribute, translations_options(options) + final_options = translations_options(options) + + custom_label(attribute, final_options[:label], final_options[:label_options]) + + help_text(final_options[:hint]) + + super(attribute, final_options.merge(label: false, hint: false)) end end + def locale + @object.locale + end + + def label(attribute, text = nil, options = {}) + label_options = options.merge( + class: "#{options[:class]} js-globalize-attribute".strip, + style: "#{options[:style]} #{@template.display_translation?(locale)}".strip, + data: (options[:data] || {}) .merge(locale: locale) + ) + + hint = label_options.delete(:hint) + super(attribute, text, label_options) + help_text(hint) + end + private + def help_text(text) + if text + content_tag :span, text, + class: "help-text js-globalize-attribute", + data: { locale: locale }, + style: @template.display_translation?(locale) + else + "" + end + end def translations_options(options) - @template.merge_translatable_field_options(options, @object.locale) + @template.merge_translatable_field_options(options, locale) end end end diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb index 18e4489b7..9e8ded1eb 100644 --- a/app/models/legislation/draft_version.rb +++ b/app/models/legislation/draft_version.rb @@ -10,12 +10,16 @@ class Legislation::DraftVersion < ActiveRecord::Base translates :body_html, touch: true translates :toc_html, touch: true globalize_accessors + accepts_nested_attributes_for :translations, allow_destroy: true belongs_to :process, class_name: 'Legislation::Process', foreign_key: 'legislation_process_id' has_many :annotations, class_name: 'Legislation::Annotation', foreign_key: 'legislation_draft_version_id', dependent: :destroy - validates :title, presence: true - validates :body, presence: true + translation_class.instance_eval do + validates :title, presence: true + validates :body, presence: true + end + validates :status, presence: true, inclusion: { in: VALID_STATUSES } scope :published, -> { where(status: 'published').order('id DESC') } diff --git a/app/views/admin/legislation/draft_versions/_form.html.erb b/app/views/admin/legislation/draft_versions/_form.html.erb index 672f1b119..729ed071c 100644 --- a/app/views/admin/legislation/draft_versions/_form.html.erb +++ b/app/views/admin/legislation/draft_versions/_form.html.erb @@ -15,19 +15,59 @@ <% end %> -
- <%= f.translatable_text_field :title, - placeholder: t("admin.legislation.draft_versions.form.title_placeholder") %> -
+ <%= f.translatable_fields do |translations_form| %> +
+ <%= translations_form.text_field :title, + placeholder: t("admin.legislation.draft_versions.form.title_placeholder") %> +
-
- <%= f.label :changelog %> - <%= t("admin.legislation.draft_versions.form.use_markdown") %> - <%= f.translatable_text_area :changelog, - label: false, - rows: 5, - placeholder: t("admin.legislation.draft_versions.form.changelog_placeholder") %> -
+
+ <%= translations_form.text_area :changelog, + hint: t("admin.legislation.draft_versions.form.use_markdown"), + rows: 5, + placeholder: t("admin.legislation.draft_versions.form.changelog_placeholder") %> +
+ +
+ <%= translations_form.label :body, nil, hint: t("admin.legislation.draft_versions.form.use_markdown") %> +
+ + <%= content_tag :div, + class: "markdown-editor clear js-globalize-attribute", + data: { locale: translations_form.locale }, + style: display_translation?(translations_form.locale) do %> + +
+
+ <%= t("admin.legislation.draft_versions.form.title_html", + draft_version_title: @draft_version.title, + process_title: @process.title ) %> +
+ +
+ <%= f.submit(class: "button", value: t("admin.legislation.draft_versions.#{admin_submit_action(@draft_version)}.submit_button")) %> +
+ + <%= link_to "#", class: 'fullscreen-toggle' do %> + " + data-open-text="<%= t("admin.legislation.draft_versions.form.close_text_editor")%>"> + <%= t("admin.legislation.draft_versions.form.launch_text_editor")%> + + + <% end %> +
+ +
+ <%= translations_form.text_area :body, + label: false, + rows: 10, + placeholder: t("admin.legislation.draft_versions.form.body_placeholder") %> +
+ +
+
+ <% end %> + <% end %>
<%= f.label :status %> @@ -45,40 +85,6 @@ <%= t("admin.legislation.draft_versions.form.hints.final_version") %>
-
- <%= f.label :body %> - <%= t("admin.legislation.draft_versions.form.use_markdown") %> -
- -
-
-
- <%= t("admin.legislation.draft_versions.form.title_html", - draft_version_title: @draft_version.title, - process_title: @process.title ) %> -
- -
- <%= f.submit(class: "button", value: t("admin.legislation.draft_versions.#{admin_submit_action(@draft_version)}.submit_button")) %> -
- - <%= link_to "#", class: 'fullscreen-toggle' do %> - " - data-open-text="<%= t("admin.legislation.draft_versions.form.close_text_editor")%>"> - <%= t("admin.legislation.draft_versions.form.launch_text_editor")%> - - <% end %> -
-
- <%= f.translatable_text_area :body, - label: false, - placeholder: t("admin.legislation.draft_versions.form.body_placeholder") %> -
- -
-
-
-
<%= f.submit(class: "button success expanded", value: t("admin.legislation.draft_versions.#{admin_submit_action(@draft_version)}.submit_button")) %>
diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index d79b0a070..8fe133eb5 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -231,6 +231,10 @@ en: changelog: Changes status: Status final_version: Final version + legislation/draft_version/translation: + title: Version title + body: Text + changelog: Changes legislation/question: title: Title question_options: Options diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 9d74308e4..660e2ed10 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -231,6 +231,10 @@ es: changelog: Cambios status: Estado final_version: Versión final + legislation/draft_version/translation: + title: Título de la version + body: Texto + changelog: Cambios legislation/question: title: Título question_options: Respuestas diff --git a/spec/features/admin/legislation/draft_versions_spec.rb b/spec/features/admin/legislation/draft_versions_spec.rb index 185bc05ab..a17c2522e 100644 --- a/spec/features/admin/legislation/draft_versions_spec.rb +++ b/spec/features/admin/legislation/draft_versions_spec.rb @@ -10,7 +10,8 @@ feature 'Admin legislation draft versions' do it_behaves_like "translatable", "legislation_draft_version", "edit_admin_legislation_process_draft_version_path", - %w[title changelog] + %w[title changelog], + { "body" => :markdownit } context "Feature flag" do @@ -58,9 +59,9 @@ feature 'Admin legislation draft versions' do click_link 'Create version' - fill_in 'legislation_draft_version_title_en', with: 'Version 3' - fill_in 'legislation_draft_version_changelog_en', with: 'Version 3 changes' - fill_in 'legislation_draft_version_body_en', with: 'Version 3 body' + fill_in 'Version title', with: 'Version 3' + fill_in 'Changes', with: 'Version 3 changes' + fill_in 'Text', with: 'Version 3 body' within('.end') do click_button 'Create version' @@ -91,11 +92,11 @@ feature 'Admin legislation draft versions' do click_link 'Version 1' - fill_in 'legislation_draft_version_title_en', with: 'Version 1b' + fill_in 'Version title', with: 'Version 1b' click_link 'Launch text editor' - fill_in 'legislation_draft_version_body_en', with: '# Version 1 body\r\n\r\nParagraph\r\n\r\n>Quote' + fill_in 'Text', with: '# Version 1 body\r\n\r\nParagraph\r\n\r\n>Quote' within('.fullscreen') do click_link 'Close text editor' @@ -106,31 +107,4 @@ feature 'Admin legislation draft versions' do expect(page).to have_content 'Version 1b' end end - - context "Special translation behaviour" do - - let!(:draft_version) { create(:legislation_draft_version) } - - scenario 'Add body translation through markup editor', :js do - edit_path = edit_admin_legislation_process_draft_version_path(draft_version.process, draft_version) - - visit edit_path - - select "Français", from: "translation_locale" - - click_link 'Launch text editor' - - fill_in 'legislation_draft_version_body_fr', with: 'Texte en Français' - - click_link 'Close text editor' - click_button "Save changes" - - visit edit_path - - click_link "Français" - click_link 'Launch text editor' - - expect(page).to have_field('legislation_draft_version_body_fr', with: 'Texte en Français') - end - end end diff --git a/spec/models/legislation/draft_version_spec.rb b/spec/models/legislation/draft_version_spec.rb index d88cdcee6..435693174 100644 --- a/spec/models/legislation/draft_version_spec.rb +++ b/spec/models/legislation/draft_version_spec.rb @@ -17,6 +17,7 @@ RSpec.describe Legislation::DraftVersion, type: :model do end it "renders and saves the html from the markdown body field with alternative translation" do + legislation_draft_version.title_fr = "Français" legislation_draft_version.body_fr = body_markdown legislation_draft_version.save! diff --git a/spec/shared/features/translatable.rb b/spec/shared/features/translatable.rb index edec51356..862417ff3 100644 --- a/spec/shared/features/translatable.rb +++ b/spec/shared/features/translatable.rb @@ -1,4 +1,4 @@ -shared_examples "translatable" do |factory_name, path_name, fields| +shared_examples "translatable" do |factory_name, path_name, input_fields, textarea_fields = {}| let(:language_texts) do { es: "en español", @@ -10,6 +10,11 @@ shared_examples "translatable" do |factory_name, path_name, fields| let(:translatable_class) { build(factory_name).class } + let(:input_fields) { input_fields } # So it's accessible by methods + let(:textarea_fields) { textarea_fields } # So it's accessible by methods + + let(:fields) { input_fields + textarea_fields.keys } + let(:attributes) do fields.product(%i[en es]).map do |field, locale| [:"#{field}_#{locale}", text_for(field, locale)] @@ -41,23 +46,19 @@ shared_examples "translatable" do |factory_name, path_name, fields| visit path select "Français", from: "translation_locale" - - fields.each do |field| - fill_in field_for(field, :fr), with: text_for(field, :fr) - end - + fields.each { |field| fill_in_field field, :fr, with: text_for(field, :fr) } click_button update_button_text visit path field = fields.sample - expect(page).to have_field(field_for(field, :en), with: text_for(field, :en)) + expect_page_to_have_translatable_field field, :en, with: text_for(field, :en) click_link "Español" - expect(page).to have_field(field_for(field, :es), with: text_for(field, :es)) + expect_page_to_have_translatable_field field, :es, with: text_for(field, :es) click_link "Français" - expect(page).to have_field(field_for(field, :fr), with: text_for(field, :fr)) + expect_page_to_have_translatable_field field, :fr, with: text_for(field, :fr) end scenario "Add an invalid translation", :js do @@ -66,15 +67,16 @@ shared_examples "translatable" do |factory_name, path_name, fields| field = required_fields.sample visit path + select "Français", from: "translation_locale" - fill_in field_for(field, :fr), with: "" + fill_in_field field, :fr, with: "" click_button update_button_text expect(page).to have_css "#error_explanation" click_link "Français" - expect(page).to have_field(field_for(field, :fr), with: "") + expect_page_to_have_translatable_field field, :fr, with: "" end scenario "Update a translation", :js do @@ -84,17 +86,17 @@ shared_examples "translatable" do |factory_name, path_name, fields| field = fields.sample updated_text = "Corrección de #{text_for(field, :es)}" - fill_in field_for(field, :es), with: updated_text + fill_in_field field, :es, with: updated_text click_button update_button_text visit path - expect(page).to have_field(field_for(field, :en), with: text_for(field, :en)) + expect_page_to_have_translatable_field field, :en, with: text_for(field, :en) select('Español', from: 'locale-switcher') - expect(page).to have_field(field_for(field, :es), with: updated_text) + expect_page_to_have_translatable_field field, :es, with: updated_text end scenario "Update a translation with invalid data", :js do @@ -105,16 +107,16 @@ shared_examples "translatable" do |factory_name, path_name, fields| visit path click_link "Español" - expect(page).to have_field(field_for(field, :es), with: text_for(field, :es)) + expect_page_to_have_translatable_field field, :es, with: text_for(field, :es) - fill_in field_for(field, :es), with: "" + fill_in_field field, :es, with: "" click_button update_button_text expect(page).to have_css "#error_explanation" click_link "Español" - expect(page).to have_field(field_for(field, :es), with: "") + expect_page_to_have_translatable_field field, :es, with: "" end scenario "Remove a translation", :js do @@ -142,17 +144,17 @@ shared_examples "translatable" do |factory_name, path_name, fields| click_link "Remove language" click_link "English" - fill_in field_for(field, :en), with: "" + fill_in_field field, :en, with: "" click_button update_button_text expect(page).to have_css "#error_explanation" - expect(page).to have_field(field_for(field, :en), with: "") + expect_page_to_have_translatable_field field, :en, with: "" expect(page).not_to have_link "Español" visit path click_link "Español" - expect(page).to have_field(field_for(field, :es), with: text_for(field, :es)) + expect_page_to_have_translatable_field field, :es, with: text_for(field, :es) end scenario 'Change value of a translated field to blank', :js do @@ -161,24 +163,20 @@ shared_examples "translatable" do |factory_name, path_name, fields| field = optional_fields.sample visit path - expect(page).to have_field(field_for(field, :en), with: text_for(field, :en)) + expect_page_to_have_translatable_field field, :en, with: text_for(field, :en) - fill_in field_for(field, :en), with: '' + fill_in_field field, :en, with: '' click_button update_button_text visit path - expect(page).to have_field(field_for(field, :en), with: '') + expect_page_to_have_translatable_field field, :en, with: '' end scenario "Add a translation for a locale with non-underscored name", :js do visit path select "Português brasileiro", from: "translation_locale" - - fields.each do |field| - fill_in field_for(field, :"pt-BR"), with: text_for(field, :"pt-BR") - end - + fields.each { |field| fill_in_field field, :"pt-BR", with: text_for(field, :"pt-BR") } click_button update_button_text visit path @@ -186,7 +184,7 @@ shared_examples "translatable" do |factory_name, path_name, fields| select('Português brasileiro', from: 'locale-switcher') field = fields.sample - expect(page).to have_field(field_for(field, :"pt-BR"), with: text_for(field, :"pt-BR")) + expect_page_to_have_translatable_field field, :"pt-BR", with: text_for(field, :"pt-BR") end end @@ -215,11 +213,11 @@ shared_examples "translatable" do |factory_name, path_name, fields| visit path field = fields.sample - expect(page).to have_field(field_for(field, :en), with: text_for(field, :en)) + expect_page_to_have_translatable_field field, :en, with: text_for(field, :en) click_link "Español" - expect(page).to have_field(field_for(field, :es), with: text_for(field, :es)) + expect_page_to_have_translatable_field field, :es, with: text_for(field, :es) end scenario "Select a locale and add it to the form", :js do @@ -231,7 +229,7 @@ shared_examples "translatable" do |factory_name, path_name, fields| click_link "Français" - expect(page).to have_field(field_for(fields.sample, :fr)) + expect_page_to_have_translatable_field fields.sample, :fr, with: "" end end end @@ -250,6 +248,36 @@ def field_for(field, locale) end end +def fill_in_field(field, locale, with:) + if input_fields.include?(field) + fill_in field_for(field, locale), with: with + else + fill_in_textarea(field, textarea_fields[field], locale, with: with) + end +end + +def fill_in_textarea(field, textarea_type, locale, with:) + if textarea_type == :markdownit + click_link class: "fullscreen-toggle" + fill_in field_for(field, locale), with: with + click_link class: "fullscreen-toggle" + end +end + +def expect_page_to_have_translatable_field(field, locale, with:) + if input_fields.include?(field) + expect(page).to have_field field_for(field, locale), with: with + else + textarea_type = textarea_fields[field] + + if textarea_type == :markdownit + click_link class: "fullscreen-toggle" + expect(page).to have_field field_for(field, locale), with: with + click_link class: "fullscreen-toggle" + end + end +end + # FIXME: button texts should be consistent. Right now, buttons don't # even share the same colour. def update_button_text