From ef7be4fc5521ae439b076c446a9686c3ec4a93fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 5 Oct 2018 14:15:24 +0200 Subject: [PATCH 1/7] Add task to migrate data to translation tables We forgot to do it when we created the translation tables, and so now we need to make sure we don't overwrite existing translations. --- lib/tasks/globalize.rake | 34 ++++++++++++++ spec/lib/tasks/globalize_spec.rb | 76 ++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 lib/tasks/globalize.rake create mode 100644 spec/lib/tasks/globalize_spec.rb diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake new file mode 100644 index 00000000000..37acb06cb3e --- /dev/null +++ b/lib/tasks/globalize.rake @@ -0,0 +1,34 @@ +namespace :globalize do + desc "Migrates existing data to translation tables" + task migrate_data: :environment do + [ + AdminNotification, + Banner, + Budget::Investment::Milestone, + I18nContent, + Legislation::DraftVersion, + Legislation::Process, + Legislation::Question, + Legislation::QuestionOption, + Poll, + Poll::Question, + Poll::Question::Answer, + SiteCustomization::Page, + Widget::Card + ].each do |model_class| + Logger.new(STDOUT).info "Migrating #{model_class} data" + + fields = model_class.translated_attribute_names + + model_class.find_each do |record| + fields.each do |field| + if record.send(:"#{field}_#{I18n.locale}").blank? + record.send(:"#{field}_#{I18n.locale}=", record.untranslated_attributes[field.to_s]) + end + end + + record.save! + end + end + end +end diff --git a/spec/lib/tasks/globalize_spec.rb b/spec/lib/tasks/globalize_spec.rb new file mode 100644 index 00000000000..2b00353d335 --- /dev/null +++ b/spec/lib/tasks/globalize_spec.rb @@ -0,0 +1,76 @@ +require "rails_helper" +require "rake" + +describe "Globalize tasks" do + + describe "#migrate_data" do + + before do + Rake.application.rake_require "tasks/globalize" + Rake::Task.define_task(:environment) + end + + let :run_rake_task do + Rake::Task["globalize:migrate_data"].reenable + Rake.application.invoke_task "globalize:migrate_data" + end + + context "Original data with no translated data" do + let(:poll) do + create(:poll).tap do |poll| + poll.translations.delete_all + poll.update_column(:name, "Original") + poll.reload + end + end + + it "copies the original data" do + expect(poll.send(:"name_#{I18n.locale}")).to be nil + expect(poll.name).to eq("Original") + + run_rake_task + poll.reload + + expect(poll.name).to eq("Original") + expect(poll.send(:"name_#{I18n.locale}")).to eq("Original") + end + end + + context "Original data with blank translated data" do + let(:banner) do + create(:banner).tap do |banner| + banner.update_column(:title, "Original") + banner.translations.first.update_column(:title, "") + end + end + + it "copies the original data" do + expect(banner.title).to eq("") + + run_rake_task + banner.reload + + expect(banner.title).to eq("Original") + expect(banner.send(:"title_#{I18n.locale}")).to eq("Original") + end + end + + context "Original data with translated data" do + let(:notification) do + create(:admin_notification, title: "Translated").tap do |notification| + notification.update_column(:title, "Original") + end + end + + it "maintains the translated data" do + expect(notification.title).to eq("Translated") + + run_rake_task + notification.reload + + expect(notification.title).to eq("Translated") + expect(notification.send(:"title_#{I18n.locale}")).to eq("Translated") + end + end + end +end From a84a0f2b7dd455342a171a3f3aec617e04646494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 5 Oct 2018 14:19:44 +0200 Subject: [PATCH 2/7] Migrate custom pages data to their locale --- lib/tasks/globalize.rake | 10 ++++++-- spec/lib/tasks/globalize_spec.rb | 43 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake index 37acb06cb3e..69e10508e18 100644 --- a/lib/tasks/globalize.rake +++ b/lib/tasks/globalize.rake @@ -22,8 +22,14 @@ namespace :globalize do model_class.find_each do |record| fields.each do |field| - if record.send(:"#{field}_#{I18n.locale}").blank? - record.send(:"#{field}_#{I18n.locale}=", record.untranslated_attributes[field.to_s]) + locale = if model_class == SiteCustomization::Page && record.locale.present? + record.locale + else + I18n.locale + end + + if record.send(:"#{field}_#{locale}").blank? + record.send(:"#{field}_#{locale}=", record.untranslated_attributes[field.to_s]) end end diff --git a/spec/lib/tasks/globalize_spec.rb b/spec/lib/tasks/globalize_spec.rb index 2b00353d335..7d3dbbba586 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -72,5 +72,48 @@ expect(notification.send(:"title_#{I18n.locale}")).to eq("Translated") end end + + context "Custom page with a different locale and no translations" do + let(:page) do + create(:site_customization_page, locale: :fr).tap do |page| + page.translations.delete_all + page.update_column(:title, "en Français") + page.reload + end + end + + it "copies the original data to both the page's locale" do + expect(page.title).to eq("en Français") + expect(page.title_fr).to be nil + expect(page.send(:"title_#{I18n.locale}")).to be nil + + run_rake_task + page.reload + + expect(page.title).to eq("en Français") + expect(page.title_fr).to eq("en Français") + expect(page.send(:"title_#{I18n.locale}")).to be nil + end + end + + context "Custom page with a different locale and existing translations" do + let(:page) do + create(:site_customization_page, title: "In English", locale: :fr).tap do |page| + page.update_column(:title, "en Français") + end + end + + it "copies the original data to the page's locale" do + expect(page.title_fr).to be nil + expect(page.title).to eq("In English") + + run_rake_task + page.reload + + expect(page.title).to eq("In English") + expect(page.title_fr).to eq("en Français") + expect(page.send(:"title_#{I18n.locale}")).to eq("In English") + end + end end end From be25e5fc452b742020cd17ab02fb4cc99ec69e8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 17 Oct 2018 03:51:50 +0200 Subject: [PATCH 3/7] Use `migrate_data` option for globalize This way the task to migrate the data doesn't have to be run manually if these migrations weren't already executed. --- ...20180323190027_add_translate_milestones.rb | 11 ++++--- ...115545_create_i18n_content_translations.rb | 5 +++- .../20180727140800_add_banner_translations.rb | 7 +++-- ...20800_add_homepage_content_translations.rb | 11 ++++--- .../20180730213824_add_poll_translations.rb | 9 ++++-- ...800_add_admin_notification_translations.rb | 7 +++-- ...31173147_add_poll_question_translations.rb | 3 +- ...9_add_poll_question_answer_translations.rb | 7 +++-- ..._collaborative_legislation_translations.rb | 30 ++++++++++++------- .../20180924071722_add_translate_pages.rb | 14 +++++---- 10 files changed, 69 insertions(+), 35 deletions(-) diff --git a/db/migrate/20180323190027_add_translate_milestones.rb b/db/migrate/20180323190027_add_translate_milestones.rb index 6e27583a8e9..6767d8e8423 100644 --- a/db/migrate/20180323190027_add_translate_milestones.rb +++ b/db/migrate/20180323190027_add_translate_milestones.rb @@ -1,9 +1,12 @@ class AddTranslateMilestones < ActiveRecord::Migration def self.up - Budget::Investment::Milestone.create_translation_table!({ - title: :string, - description: :text - }) + Budget::Investment::Milestone.create_translation_table!( + { + title: :string, + description: :text + }, + { migrate_data: true } + ) end def self.down diff --git a/db/migrate/20180718115545_create_i18n_content_translations.rb b/db/migrate/20180718115545_create_i18n_content_translations.rb index e472f06222e..8e6fde21cf5 100644 --- a/db/migrate/20180718115545_create_i18n_content_translations.rb +++ b/db/migrate/20180718115545_create_i18n_content_translations.rb @@ -6,7 +6,10 @@ def change reversible do |dir| dir.up do - I18nContent.create_translation_table! :value => :text + I18nContent.create_translation_table!( + { value: :text }, + { migrate_data: true } + ) end dir.down do diff --git a/db/migrate/20180727140800_add_banner_translations.rb b/db/migrate/20180727140800_add_banner_translations.rb index 7678a34a105..4c4c9391edb 100644 --- a/db/migrate/20180727140800_add_banner_translations.rb +++ b/db/migrate/20180727140800_add_banner_translations.rb @@ -2,8 +2,11 @@ class AddBannerTranslations < ActiveRecord::Migration def self.up Banner.create_translation_table!( - title: :string, - description: :text + { + title: :string, + description: :text + }, + { migrate_data: true } ) end diff --git a/db/migrate/20180730120800_add_homepage_content_translations.rb b/db/migrate/20180730120800_add_homepage_content_translations.rb index 41596453622..b344f95c347 100644 --- a/db/migrate/20180730120800_add_homepage_content_translations.rb +++ b/db/migrate/20180730120800_add_homepage_content_translations.rb @@ -2,10 +2,13 @@ class AddHomepageContentTranslations < ActiveRecord::Migration def self.up Widget::Card.create_translation_table!( - label: :string, - title: :string, - description: :text, - link_text: :string + { + label: :string, + title: :string, + description: :text, + link_text: :string + }, + { migrate_data: true } ) end diff --git a/db/migrate/20180730213824_add_poll_translations.rb b/db/migrate/20180730213824_add_poll_translations.rb index 4a4fa72a4df..75495d3272c 100644 --- a/db/migrate/20180730213824_add_poll_translations.rb +++ b/db/migrate/20180730213824_add_poll_translations.rb @@ -2,9 +2,12 @@ class AddPollTranslations < ActiveRecord::Migration def self.up Poll.create_translation_table!( - name: :string, - summary: :text, - description: :text + { + name: :string, + summary: :text, + description: :text + }, + { migrate_data: true } ) end diff --git a/db/migrate/20180731150800_add_admin_notification_translations.rb b/db/migrate/20180731150800_add_admin_notification_translations.rb index 519751fd7b1..fb76a42d29a 100644 --- a/db/migrate/20180731150800_add_admin_notification_translations.rb +++ b/db/migrate/20180731150800_add_admin_notification_translations.rb @@ -2,8 +2,11 @@ class AddAdminNotificationTranslations < ActiveRecord::Migration def self.up AdminNotification.create_translation_table!( - title: :string, - body: :text + { + title: :string, + body: :text + }, + { migrate_data: true } ) end diff --git a/db/migrate/20180731173147_add_poll_question_translations.rb b/db/migrate/20180731173147_add_poll_question_translations.rb index a167ea00a29..f62fbc1613d 100644 --- a/db/migrate/20180731173147_add_poll_question_translations.rb +++ b/db/migrate/20180731173147_add_poll_question_translations.rb @@ -2,7 +2,8 @@ class AddPollQuestionTranslations < ActiveRecord::Migration def self.up Poll::Question.create_translation_table!( - title: :string + { title: :string }, + { migrate_data: true } ) end diff --git a/db/migrate/20180801114529_add_poll_question_answer_translations.rb b/db/migrate/20180801114529_add_poll_question_answer_translations.rb index 91643f4433a..3203d7a1f51 100644 --- a/db/migrate/20180801114529_add_poll_question_answer_translations.rb +++ b/db/migrate/20180801114529_add_poll_question_answer_translations.rb @@ -2,8 +2,11 @@ class AddPollQuestionAnswerTranslations < ActiveRecord::Migration def self.up Poll::Question::Answer.create_translation_table!( - title: :string, - description: :text + { + title: :string, + description: :text + }, + { migrate_data: true } ) end diff --git a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb b/db/migrate/20180801140800_add_collaborative_legislation_translations.rb index 7c0fb9eb32a..569689a73ce 100644 --- a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb +++ b/db/migrate/20180801140800_add_collaborative_legislation_translations.rb @@ -2,25 +2,33 @@ class AddCollaborativeLegislationTranslations < ActiveRecord::Migration def self.up Legislation::Process.create_translation_table!( - title: :string, - summary: :text, - description: :text, - additional_info: :text, + { + title: :string, + summary: :text, + description: :text, + additional_info: :text, + }, + { migrate_data: true } ) Legislation::Question.create_translation_table!( - title: :text + { title: :text }, + { migrate_data: true } ) Legislation::DraftVersion.create_translation_table!( - title: :string, - changelog: :text, - body: :text, - body_html: :text, - toc_html: :text + { + title: :string, + changelog: :text, + body: :text, + body_html: :text, + toc_html: :text + }, + { migrate_data: true } ) Legislation::QuestionOption.create_translation_table!( - value: :string + { value: :string }, + { migrate_data: true } ) end diff --git a/db/migrate/20180924071722_add_translate_pages.rb b/db/migrate/20180924071722_add_translate_pages.rb index 6dc939dec00..250f46dee7d 100644 --- a/db/migrate/20180924071722_add_translate_pages.rb +++ b/db/migrate/20180924071722_add_translate_pages.rb @@ -1,10 +1,14 @@ class AddTranslatePages < ActiveRecord::Migration def self.up - SiteCustomization::Page.create_translation_table!({ - title: :string, - subtitle: :string, - content: :text - }) + SiteCustomization::Page.create_translation_table!( + { + title: :string, + subtitle: :string, + content: :text + }, + { migrate_data: true } + ) + change_column :site_customization_pages, :title, :string, :null => true end From 3c48059f07c78dbdca69ef2ecea784210af6a17c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 17 Oct 2018 03:55:59 +0200 Subject: [PATCH 4/7] Log failed data migrations In theory, it should never happen, but that's why exceptions exist. --- lib/tasks/globalize.rake | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake index 69e10508e18..02c1e5f029e 100644 --- a/lib/tasks/globalize.rake +++ b/lib/tasks/globalize.rake @@ -1,6 +1,9 @@ namespace :globalize do desc "Migrates existing data to translation tables" task migrate_data: :environment do + logger = Logger.new(STDOUT) + logger.formatter = proc { |severity, _datetime, _progname, msg| "#{severity} #{msg}\n" } + [ AdminNotification, Banner, @@ -16,7 +19,7 @@ namespace :globalize do SiteCustomization::Page, Widget::Card ].each do |model_class| - Logger.new(STDOUT).info "Migrating #{model_class} data" + logger.info "Migrating #{model_class} data" fields = model_class.translated_attribute_names @@ -33,7 +36,11 @@ namespace :globalize do end end - record.save! + begin + record.save! + rescue ActiveRecord::RecordInvalid + logger.error "Failed to save #{model_class} with id #{record.id}" + end end end end From 7fff57a25f0794ebc696a9f76ef0f7b428d1d3a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 17 Oct 2018 13:41:14 +0200 Subject: [PATCH 5/7] Add task to simulate data migration This way we can check everything is OK before actually migrating the data to the translations tables. --- lib/tasks/globalize.rake | 49 ++++++++++++++++++++++++++++---- spec/lib/tasks/globalize_spec.rb | 30 +++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake index 02c1e5f029e..f71a252ae8d 100644 --- a/lib/tasks/globalize.rake +++ b/lib/tasks/globalize.rake @@ -1,9 +1,5 @@ namespace :globalize do - desc "Migrates existing data to translation tables" - task migrate_data: :environment do - logger = Logger.new(STDOUT) - logger.formatter = proc { |severity, _datetime, _progname, msg| "#{severity} #{msg}\n" } - + def translatable_classes [ AdminNotification, Banner, @@ -18,7 +14,13 @@ namespace :globalize do Poll::Question::Answer, SiteCustomization::Page, Widget::Card - ].each do |model_class| + ] + end + + def migrate_data + @errored = false + + translatable_classes.each do |model_class| logger.info "Migrating #{model_class} data" fields = model_class.translated_attribute_names @@ -40,8 +42,43 @@ namespace :globalize do record.save! rescue ActiveRecord::RecordInvalid logger.error "Failed to save #{model_class} with id #{record.id}" + @errored = true end end end end + + def logger + @logger ||= Logger.new(STDOUT).tap do |logger| + logger.formatter = proc { |severity, _datetime, _progname, msg| "#{severity} #{msg}\n" } + end + end + + def errored? + @errored + end + + desc "Simulates migrating existing data to translation tables" + task simulate_migrate_data: :environment do + logger.info "Starting migrate data simulation" + + ActiveRecord::Base.transaction do + migrate_data + raise ActiveRecord::Rollback + end + + if errored? + logger.error "Simulation failed! Please check the errors and solve them before proceeding." + raise "Simulation failed!" + else + logger.info "Migrate data simulation ended successfully" + end + end + + desc "Migrates existing data to translation tables" + task migrate_data: :simulate_migrate_data do + logger.info "Starting data migration" + migrate_data + logger.info "Finished data migration" + end end diff --git a/spec/lib/tasks/globalize_spec.rb b/spec/lib/tasks/globalize_spec.rb index 7d3dbbba586..84592f56650 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -11,6 +11,7 @@ end let :run_rake_task do + Rake::Task["globalize:simulate_migrate_data"].reenable Rake::Task["globalize:migrate_data"].reenable Rake.application.invoke_task "globalize:migrate_data" end @@ -115,5 +116,34 @@ expect(page.send(:"title_#{I18n.locale}")).to eq("In English") end end + + context "Invalid data" do + let!(:valid_process) do + create(:legislation_process).tap do |process| + process.translations.delete_all + process.update_column(:title, "Title") + process.reload + end + end + + let!(:invalid_process) do + create(:legislation_process).tap do |process| + process.translations.delete_all + process.update_column(:title, "") + process.reload + end + end + + it "simulates the task and aborts without creating any translations" do + expect(valid_process).to be_valid + expect(invalid_process).not_to be_valid + + expect { run_rake_task }.to raise_exception("Simulation failed!") + + expect(Legislation::Process::Translation.count).to eq 0 + expect(valid_process.reload.title).to eq "Title" + expect(invalid_process.reload.title).to eq "" + end + end end end From 934bce5932ffcb6f9c1de047897a42de7b3e4eb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 22 Oct 2018 11:13:07 +0200 Subject: [PATCH 6/7] Don't abort the migration if the simulation fails We think aborting the migration will generate more headaches to system administrators, who will have to manually check and fix every invalid record before anything can be migrated. --- lib/tasks/globalize.rake | 11 +++++++---- spec/lib/tasks/globalize_spec.rb | 9 +++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake index f71a252ae8d..ca47eaa2c89 100644 --- a/lib/tasks/globalize.rake +++ b/lib/tasks/globalize.rake @@ -41,7 +41,7 @@ namespace :globalize do begin record.save! rescue ActiveRecord::RecordInvalid - logger.error "Failed to save #{model_class} with id #{record.id}" + logger.warn "Failed to save #{model_class} with id #{record.id}" @errored = true end end @@ -68,17 +68,20 @@ namespace :globalize do end if errored? - logger.error "Simulation failed! Please check the errors and solve them before proceeding." - raise "Simulation failed!" + logger.warn "Some database records will not be migrated" else logger.info "Migrate data simulation ended successfully" end end desc "Migrates existing data to translation tables" - task migrate_data: :simulate_migrate_data do + task migrate_data: :environment do logger.info "Starting data migration" migrate_data logger.info "Finished data migration" + + if errored? + logger.warn "Some database records couldn't be migrated; please check the log messages" + end end end diff --git a/spec/lib/tasks/globalize_spec.rb b/spec/lib/tasks/globalize_spec.rb index 84592f56650..5b9d4a435f7 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -11,7 +11,6 @@ end let :run_rake_task do - Rake::Task["globalize:simulate_migrate_data"].reenable Rake::Task["globalize:migrate_data"].reenable Rake.application.invoke_task "globalize:migrate_data" end @@ -134,14 +133,16 @@ end end - it "simulates the task and aborts without creating any translations" do + it "ignores invalid data and migrates valid data" do expect(valid_process).to be_valid expect(invalid_process).not_to be_valid - expect { run_rake_task }.to raise_exception("Simulation failed!") + run_rake_task - expect(Legislation::Process::Translation.count).to eq 0 + expect(valid_process.translations.count).to eq 1 expect(valid_process.reload.title).to eq "Title" + + expect(invalid_process.translations.count).to eq 0 expect(invalid_process.reload.title).to eq "" end end From 9404cb8b3a88a1581c2204b5b217cef0f04e5450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 22 Oct 2018 11:29:32 +0200 Subject: [PATCH 7/7] Fix bug with non-underscored locales Ruby can't have hyphens in method names, so sending something like `record.title_pt-BR` would raise an exception. Using globalize's `localized_attr_name_for` method fixes the bug. Thanks Marko for the tip. --- lib/tasks/globalize.rake | 6 ++++-- spec/lib/tasks/globalize_spec.rb | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/tasks/globalize.rake b/lib/tasks/globalize.rake index ca47eaa2c89..51a23042e02 100644 --- a/lib/tasks/globalize.rake +++ b/lib/tasks/globalize.rake @@ -33,8 +33,10 @@ namespace :globalize do I18n.locale end - if record.send(:"#{field}_#{locale}").blank? - record.send(:"#{field}_#{locale}=", record.untranslated_attributes[field.to_s]) + translated_field = record.localized_attr_name_for(field, locale) + + if record.send(translated_field).blank? + record.send(:"#{translated_field}=", record.untranslated_attributes[field.to_s]) end end diff --git a/spec/lib/tasks/globalize_spec.rb b/spec/lib/tasks/globalize_spec.rb index 5b9d4a435f7..b685bb95e1d 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -146,5 +146,23 @@ expect(invalid_process.reload.title).to eq "" end end + + context "locale with non-underscored name" do + before { I18n.locale = :"pt-BR" } + + let!(:milestone) do + create(:budget_investment_milestone).tap do |milestone| + milestone.translations.delete_all + milestone.update_column(:title, "Português") + milestone.reload + end + end + + it "runs the migration successfully" do + run_rake_task + + expect(milestone.reload.title).to eq "Português" + end + end end end