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 000000000..37acb06cb --- /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 000000000..2b00353d3 --- /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 37acb06cb..69e10508e 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 2b00353d3..7d3dbbba5 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -72,5 +72,48 @@ describe "Globalize tasks" do 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 6e27583a8..6767d8e84 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 e472f0622..8e6fde21c 100644 --- a/db/migrate/20180718115545_create_i18n_content_translations.rb +++ b/db/migrate/20180718115545_create_i18n_content_translations.rb @@ -6,7 +6,10 @@ class CreateI18nContentTranslations < ActiveRecord::Migration 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 7678a34a1..4c4c9391e 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 415964536..b344f95c3 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 4a4fa72a4..75495d327 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 519751fd7..fb76a42d2 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 a167ea00a..f62fbc161 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 91643f443..3203d7a1f 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 7c0fb9eb3..569689a73 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 6dc939dec..250f46dee 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 69e10508e..02c1e5f02 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 02c1e5f02..f71a252ae 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 7d3dbbba5..84592f566 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -11,6 +11,7 @@ describe "Globalize tasks" do 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 @@ describe "Globalize tasks" do 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 f71a252ae..ca47eaa2c 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 84592f566..5b9d4a435 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -11,7 +11,6 @@ describe "Globalize tasks" do 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 @@ describe "Globalize tasks" do 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 ca47eaa2c..51a23042e 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 5b9d4a435..b685bb95e 100644 --- a/spec/lib/tasks/globalize_spec.rb +++ b/spec/lib/tasks/globalize_spec.rb @@ -146,5 +146,23 @@ describe "Globalize tasks" do 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