From 9915236e42f5c23629efae8d0dbfd1d166afce37 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Mon, 10 Jul 2023 10:06:40 +0800 Subject: [PATCH] FEATURE: Warn about outdated translation overrides in admin dashboard (#22384) This PR adds a feature to help admins stay up-to-date with their translations. We already have protections preventing admins from problems when they update their overrides. This change adds some protection in the other direction (where translations change in core due to an upgrade) by creating a notice for admins when defaults have changed. Terms: - In the case where Discourse core changes the default translation, the translation override is considered "outdated". - In the case above where interpolation keys were changed from the ones the override is using, it is considered "invalid". - If none of the above applies, the override is considered "up to date". How does it work? There are a few pieces that makes this work: - When an admin creates or updates a translation override, we store the original translation at the time of write. (This is used to detect changes later on.) - There is a background job that runs once every day and checks for outdated and invalid overrides, and marks them as such. - When there are any outdated or invalid overrides, a notice is shown in admin dashboard with a link to the text customization page. Known limitations The link from the dashboard links to the default locale text customization page. Given there might be invalid overrides in multiple languages, I'm not sure what we could do here. Consideration for future improvement. --- .../controllers/admin-site-text-index.js | 15 +++- .../addon/routes/admin-site-text-index.js | 3 +- .../admin/addon/templates/site-text-index.hbs | 11 +++ .../tests/acceptance/admin-site-text-test.js | 10 ++- .../admin/site_texts_controller.rb | 15 +++- .../scheduled/check_translation_overrides.rb | 23 ++++++ app/models/admin_dashboard_data.rb | 9 ++- app/models/translation_override.rb | 81 +++++++++++-------- config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + ...052_add_status_to_translation_overrides.rb | 8 ++ .../translation_override_fabricator.rb | 7 ++ spec/jobs/check_translation_overrides_spec.rb | 29 +++++++ spec/models/admin_dashboard_data_spec.rb | 22 +++++ spec/models/translation_override_spec.rb | 39 +++++++++ 15 files changed, 233 insertions(+), 41 deletions(-) create mode 100644 app/jobs/scheduled/check_translation_overrides.rb create mode 100644 db/migrate/20230703035052_add_status_to_translation_overrides.rb create mode 100644 spec/fabricators/translation_override_fabricator.rb create mode 100644 spec/jobs/check_translation_overrides_spec.rb diff --git a/app/assets/javascripts/admin/addon/controllers/admin-site-text-index.js b/app/assets/javascripts/admin/addon/controllers/admin-site-text-index.js index 962944145c0..2f747246d07 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-site-text-index.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-site-text-index.js @@ -8,10 +8,11 @@ export default class AdminSiteTextIndexController extends Controller { searching = false; siteTexts = null; preferred = false; - queryParams = ["q", "overridden", "locale"]; + queryParams = ["q", "overridden", "outdated", "locale"]; locale = null; q = null; overridden = false; + outdated = false; init() { super.init(...arguments); @@ -21,7 +22,10 @@ export default class AdminSiteTextIndexController extends Controller { _performSearch() { this.store - .find("site-text", this.getProperties("q", "overridden", "locale")) + .find( + "site-text", + this.getProperties("q", "overridden", "outdated", "locale") + ) .then((results) => { this.set("siteTexts", results); }) @@ -58,6 +62,13 @@ export default class AdminSiteTextIndexController extends Controller { discourseDebounce(this, this._performSearch, 400); } + @action + toggleOutdated() { + this.toggleProperty("outdated"); + this.set("searching", true); + discourseDebounce(this, this._performSearch, 400); + } + @action search() { const q = this.q; diff --git a/app/assets/javascripts/admin/addon/routes/admin-site-text-index.js b/app/assets/javascripts/admin/addon/routes/admin-site-text-index.js index ecf7baafd55..daf37261093 100644 --- a/app/assets/javascripts/admin/addon/routes/admin-site-text-index.js +++ b/app/assets/javascripts/admin/addon/routes/admin-site-text-index.js @@ -6,13 +6,14 @@ export default class AdminSiteTextIndexRoute extends Route { queryParams = { q: { replace: true }, overridden: { replace: true }, + outdated: { replace: true }, locale: { replace: true }, }; model(params) { return this.store.find( "site-text", - getProperties(params, "q", "overridden", "locale") + getProperties(params, "q", "overridden", "outdated", "locale") ); } diff --git a/app/assets/javascripts/admin/addon/templates/site-text-index.hbs b/app/assets/javascripts/admin/addon/templates/site-text-index.hbs index e3f7caa855c..2a6c64a6125 100644 --- a/app/assets/javascripts/admin/addon/templates/site-text-index.hbs +++ b/app/assets/javascripts/admin/addon/templates/site-text-index.hbs @@ -34,12 +34,23 @@ + +

diff --git a/app/assets/javascripts/discourse/tests/acceptance/admin-site-text-test.js b/app/assets/javascripts/discourse/tests/acceptance/admin-site-text-test.js index 38cfee1af71..36bb84091f6 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/admin-site-text-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/admin-site-text-test.js @@ -24,7 +24,7 @@ acceptance("Admin - Site Texts", function (needs) { assert.ok(exists(".site-text.overridden")); // Only show overridden - await click(".search-area .filter-options input"); + await click(".search-area .filter-options #toggle-overridden"); assert.strictEqual( currentURL(), "/admin/customize/site_texts?overridden=true&q=Test" @@ -32,6 +32,14 @@ acceptance("Admin - Site Texts", function (needs) { assert.ok(!exists(".site-text:not(.overridden)")); assert.ok(exists(".site-text.overridden")); + await click(".search-area .filter-options #toggle-overridden"); + + // Only show outdated + await click(".search-area .filter-options #toggle-outdated"); + assert.strictEqual( + currentURL(), + "/admin/customize/site_texts?outdated=true&q=Test" + ); }); test("edit and revert a site text by key", async function (assert) { diff --git a/app/controllers/admin/site_texts_controller.rb b/app/controllers/admin/site_texts_controller.rb index 8f2e851ccb7..bc434c41a00 100644 --- a/app/controllers/admin/site_texts_controller.rb +++ b/app/controllers/admin/site_texts_controller.rb @@ -20,17 +20,18 @@ class Admin::SiteTextsController < Admin::AdminController def index overridden = params[:overridden] == "true" + outdated = params[:outdated] == "true" extras = {} query = params[:q] || "" locale = fetch_locale(params[:locale]) - if query.blank? && !overridden + if query.blank? && !overridden && !outdated extras[:recommended] = true results = self.class.preferred_keys.map { |k| record_for(key: k, locale: locale) } else - results = find_translations(query, overridden, locale) + results = find_translations(query, overridden, outdated, locale) if results.any? extras[:regex] = I18n::Backend::DiscourseI18n.create_search_regexp(query, as_string: true) @@ -188,10 +189,18 @@ class Admin::SiteTextsController < Admin::AdminController raise Discourse::NotFound end - def find_translations(query, overridden, locale) + def find_translations(query, overridden, outdated, locale) translations = Hash.new { |hash, key| hash[key] = {} } search_results = I18n.with_locale(locale) { I18n.search(query, only_overridden: overridden) } + if outdated + outdated_keys = + TranslationOverride.where(status: %i[outdated invalid_interpolation_keys]).pluck( + :translation_key, + ) + search_results.select! { |k, _| outdated_keys.include?(k) } + end + search_results.each do |key, value| if PLURALIZED_REGEX.match(key) translations[$1][$2] = value diff --git a/app/jobs/scheduled/check_translation_overrides.rb b/app/jobs/scheduled/check_translation_overrides.rb new file mode 100644 index 00000000000..11d7d4cd97a --- /dev/null +++ b/app/jobs/scheduled/check_translation_overrides.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Jobs + class CheckTranslationOverrides < ::Jobs::Scheduled + every 1.day + + def execute(args) + invalid_ids = [] + outdated_ids = [] + + TranslationOverride.find_each do |override| + if override.invalid_interpolation_keys.present? + invalid_ids << override.id + elsif override.original_translation_updated? + outdated_ids << override.id + end + end + + TranslationOverride.where(id: outdated_ids).update_all(status: "outdated") + TranslationOverride.where(id: invalid_ids).update_all(status: "invalid_interpolation_keys") + end + end +end diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index 2b85dc6b5c0..a57b4d7c1b4 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -206,7 +206,8 @@ class AdminDashboardData :out_of_date_themes, :unreachable_themes, :watched_words_check, - :google_analytics_version_check + :google_analytics_version_check, + :translation_overrides_check register_default_scheduled_problem_checks @@ -360,6 +361,12 @@ class AdminDashboardData end end + def translation_overrides_check + if TranslationOverride.exists?(status: %i[outdated invalid_interpolation_keys]) + I18n.t("dashboard.outdated_translations_warning", base_path: Discourse.base_path) + end + end + def image_magick_check if SiteSetting.create_thumbnails && !system("command -v convert >/dev/null;") I18n.t("dashboard.image_magick_warning") diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb index 92b8ed59df0..0cad3951a8b 100644 --- a/app/models/translation_override.rb +++ b/app/models/translation_override.rb @@ -45,14 +45,18 @@ class TranslationOverride < ActiveRecord::Base validate :check_interpolation_keys + enum :status, %i[up_to_date outdated invalid_interpolation_keys] + def self.upsert!(locale, key, value) params = { locale: locale, translation_key: key } translation_override = find_or_initialize_by(params) sanitized_value = translation_override.sanitize_field(value, additional_attributes: ["data-auto-route"]) + original_translation = + I18n.overrides_disabled { I18n.t(transform_pluralized_key(key), locale: :en) } - data = { value: sanitized_value } + data = { value: sanitized_value, original_translation: original_translation } if key.end_with?("_MF") _, filename = JsLocaleHelper.find_message_format_locale([locale], fallback_to_english: false) data[:compiled_js] = JsLocaleHelper.compile_message_format(filename, locale, sanitized_value) @@ -125,39 +129,48 @@ class TranslationOverride < ActiveRecord::Base private_class_method :i18n_changed private_class_method :expire_cache - private + def original_translation_updated? + return false if original_translation.blank? - def check_interpolation_keys + transformed_key = self.class.transform_pluralized_key(translation_key) + + original_translation != I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) } + end + + def invalid_interpolation_keys transformed_key = self.class.transform_pluralized_key(translation_key) original_text = I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) } - if original_text - original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text) - new_interpolation_keys = I18nInterpolationKeysFinder.find(value) - custom_interpolation_keys = [] + return [] if original_text.blank? - ALLOWED_CUSTOM_INTERPOLATION_KEYS.select do |keys, value| - custom_interpolation_keys = value if keys.any? { |key| transformed_key.start_with?(key) } - end + original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text) + new_interpolation_keys = I18nInterpolationKeysFinder.find(value) + custom_interpolation_keys = [] - invalid_keys = - (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys - - custom_interpolation_keys - - if invalid_keys.present? - self.errors.add( - :base, - I18n.t( - "activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys", - keys: invalid_keys.join(I18n.t("word_connector.comma")), - count: invalid_keys.size, - ), - ) - - false - end + ALLOWED_CUSTOM_INTERPOLATION_KEYS.select do |keys, value| + custom_interpolation_keys = value if keys.any? { |key| transformed_key.start_with?(key) } end + + (original_interpolation_keys | new_interpolation_keys) - original_interpolation_keys - + custom_interpolation_keys + end + + private + + def check_interpolation_keys + invalid_keys = invalid_interpolation_keys + + return if invalid_keys.blank? + + self.errors.add( + :base, + I18n.t( + "activerecord.errors.models.translation_overrides.attributes.value.invalid_interpolation_keys", + keys: invalid_keys.join(I18n.t("word_connector.comma")), + count: invalid_keys.size, + ), + ) end end @@ -165,13 +178,15 @@ end # # Table name: translation_overrides # -# id :integer not null, primary key -# locale :string not null -# translation_key :string not null -# value :string not null -# created_at :datetime not null -# updated_at :datetime not null -# compiled_js :text +# id :integer not null, primary key +# locale :string not null +# translation_key :string not null +# value :string not null +# created_at :datetime not null +# updated_at :datetime not null +# compiled_js :text +# original_translation :text +# status :integer default("up_to_date"), not null # # Indexes # diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 6ae5abc6d18..31b336b653d 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -6068,6 +6068,7 @@ en: go_back: "Back to Search" recommended: "We recommend customizing the following text to suit your needs:" show_overriden: "Only show overridden" + show_outdated: "Only show outdated/invalid" locale: "Language:" more_than_50_results: "There are more than 50 results. Please refine your search." interpolation_keys: "Available interpolation keys:" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 297e50eb14e..61f878df49c 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1478,6 +1478,7 @@ en: image_magick_warning: 'The server is configured to create thumbnails of large images, but ImageMagick is not installed. Install ImageMagick using your favorite package manager or download the latest release.' failing_emails_warning: 'There are %{num_failed_jobs} email jobs that failed. Check your app.yml and ensure that the mail server settings are correct. See the failed jobs in Sidekiq.' subfolder_ends_in_slash: "Your subfolder setup is incorrect; the DISCOURSE_RELATIVE_URL_ROOT ends in a slash." + outdated_translations_warning: "Some of your translation overrides are out of date. Please check your text customizations." email_polling_errored_recently: one: "Email polling has generated an error in the past 24 hours. Look at the logs for more details." other: "Email polling has generated %{count} errors in the past 24 hours. Look at the logs for more details." diff --git a/db/migrate/20230703035052_add_status_to_translation_overrides.rb b/db/migrate/20230703035052_add_status_to_translation_overrides.rb new file mode 100644 index 00000000000..d210514d968 --- /dev/null +++ b/db/migrate/20230703035052_add_status_to_translation_overrides.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddStatusToTranslationOverrides < ActiveRecord::Migration[7.0] + def change + add_column :translation_overrides, :original_translation, :text + add_column :translation_overrides, :status, :integer, null: false, default: 0 + end +end diff --git a/spec/fabricators/translation_override_fabricator.rb b/spec/fabricators/translation_override_fabricator.rb new file mode 100644 index 00000000000..4e329fbc72c --- /dev/null +++ b/spec/fabricators/translation_override_fabricator.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +Fabricator(:translation_override) do + translation_key "title" + value "Discourse" + locale "en" +end diff --git a/spec/jobs/check_translation_overrides_spec.rb b/spec/jobs/check_translation_overrides_spec.rb new file mode 100644 index 00000000000..2982af7acb1 --- /dev/null +++ b/spec/jobs/check_translation_overrides_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.describe Jobs::CheckTranslationOverrides do + fab!(:up_to_date_translation) { Fabricate(:translation_override, translation_key: "title") } + fab!(:outdated_translation) do + Fabricate(:translation_override, translation_key: "posts", original_translation: "outdated") + end + fab!(:invalid_translation) { Fabricate(:translation_override, translation_key: "topics") } + + it "marks translations with invalid interpolation keys" do + invalid_translation.update_attribute("value", "Invalid %{foo}") + + expect { described_class.new.execute({}) }.to change { invalid_translation.reload.status }.from( + "up_to_date", + ).to("invalid_interpolation_keys") + end + + it "marks translations that are outdated" do + expect { described_class.new.execute({}) }.to change { + outdated_translation.reload.status + }.from("up_to_date").to("outdated") + end + + it "does not mark up to date translations" do + expect { described_class.new.execute({}) }.not_to change { + up_to_date_translation.reload.status + } + end +end diff --git a/spec/models/admin_dashboard_data_spec.rb b/spec/models/admin_dashboard_data_spec.rb index bc7b947e5b5..4f93c786fd3 100644 --- a/spec/models/admin_dashboard_data_spec.rb +++ b/spec/models/admin_dashboard_data_spec.rb @@ -345,4 +345,26 @@ RSpec.describe AdminDashboardData do expect(dashboard_data.out_of_date_themes).to eq(nil) end end + + describe "#translation_overrides_check" do + subject(:dashboard_data) { described_class.new } + + context "when there are outdated translations" do + before { Fabricate(:translation_override, translation_key: "foo.bar", status: "outdated") } + + it "outputs the correct message" do + expect(dashboard_data.translation_overrides_check).to eq( + I18n.t("dashboard.outdated_translations_warning", base_path: Discourse.base_path), + ) + end + end + + context "when there are no outdated translations" do + before { Fabricate(:translation_override, status: "up_to_date") } + + it "outputs nothing" do + expect(dashboard_data.translation_overrides_check).to eq(nil) + end + end + end end diff --git a/spec/models/translation_override_spec.rb b/spec/models/translation_override_spec.rb index d8c8a5c3971..e690eb04fc6 100644 --- a/spec/models/translation_override_spec.rb +++ b/spec/models/translation_override_spec.rb @@ -283,4 +283,43 @@ RSpec.describe TranslationOverride do end end end + + describe "#original_translation_updated?" do + context "when the translation is up to date" do + fab!(:translation) { Fabricate(:translation_override, translation_key: "title") } + + it { expect(translation.original_translation_updated?).to eq(false) } + end + + context "when the translation is outdated" do + fab!(:translation) do + Fabricate(:translation_override, translation_key: "title", original_translation: "outdated") + end + + it { expect(translation.original_translation_updated?).to eq(true) } + end + + context "when we can't tell because the translation is too old" do + fab!(:translation) do + Fabricate(:translation_override, translation_key: "title", original_translation: nil) + end + + it { expect(translation.original_translation_updated?).to eq(false) } + end + end + + describe "invalid_interpolation_keys" do + fab!(:translation) do + Fabricate( + :translation_override, + translation_key: "system_messages.welcome_user.subject_template", + ) + end + + it "picks out invalid keys and ignores known and custom keys" do + translation.update_attribute("value", "Hello, %{name}! Welcome to %{site_name}. %{foo}") + + expect(translation.invalid_interpolation_keys).to contain_exactly("foo") + end + end end